Repository navigation
fix(master): retain ownership across ambiguous legacy merge replies - #452
Merged
Merged
Conversation
beinan
marked this pull request as ready for review
October 21, 2026 21:38
beinan
force-pushed
the
codex/retain-legacy-merge-ownership
branch
from
October 21, 2026 22:23
e0f551b to
185315d
Compare
beinan
added a commit
that referenced
this pull request
Oct 22, 2026
Legacy merge tasks currently retry an unreachable worker with a fresh failure budget whenever the canonical task is recreated. In production this produced hundreds of failed connection attempts while healthy endpoints still had useful work. Reuse the durable per-target, per-endpoint failure ledger before legacy admission. A proven pre-connect failure now releases its execution and persists the failure count atomically; subsequent tasks skip that endpoint until its next probe, while continuing healthy endpoints. Successful responses clear only the responding endpoint’s failure. Ambiguous requests retain the execution protection introduced by #452. Remove request URLs from connection error classification so table names cannot turn a network failure into a permanent configuration failure. This builds on merged #452 and changes only the master merge path. It does not bound canonical queue churn or all traffic to one failed worker across different tables, and old master binaries do not honor this ledger on the legacy path. Existing unrecorded writers still require a safe migration. Validation: all 10 legacy connection/ownership tests pass on the rebased final commit against real isolated etcd (0 failed, 0 ignored), including the five-task retry regression, redirect/timeout ambiguity, claim lease loss and successful release. The new regression checks counts persist as 1→2→3, skipped probes do not charge attempts, and the healthy endpoint completes all five passes. Full CI is running on the rebased commit. Rollout: this adds one failure-key read per visited worker. Validate the fixed image’s private-etcd load and mixed-version behavior in staging before broad deployment. Co-authored-by: Beinan Wang <beinanwang@microsoft.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A legacy merge POST can lose its response while the worker keeps writing. Previously the task could finish as FAILED with no durable execution record, delete its target lock, and allow another task to dispatch the same table. Stopping the current fan-out did not protect the next task or a restarted master.
Record a protocol-0 running execution and a persistent target owner atomically with the canonical claim before each legacy POST. Release it only after a valid successful response or a connection failure that proves the request was not admitted. Ambiguous responses retain ownership across task completion and lease loss. Protocol-2 cancellation and storage recovery cannot retire these unfenced executions. An acknowledged terminal record remains releasable after a master crash.
This adds protection for newly dispatched legacy work; it neither retrospectively accounts for existing legacy writers nor makes them automatically recoverable. A crash after recording ownership but before sending is conservatively unresolved. Initial production retirement still requires verified termination/storage evidence, and direct clients outside canonical admission are outside this guarantee. Serial fan-out and current progress-based recovery for fenced executions remain in place.
Validation: existing HTTP classification tests plus new real loopback-etcd cases cover a live handler after a lost response, persistent target protection after task completion, blocked second POST, refusal of protocol-2 recovery, claim lease loss before HTTP, and successful multi-endpoint cleanup. All 9 targeted tests and 11 existing fenced-execution/progress compatibility tests passed against isolated local etcd. Rebased onto main including #447, which fixes the snapshot leader/transport-accounting assumptions that failed earlier CI; its four snapshot tests also pass on this final tree. Full CI must pass on the final head before merge; this is not deployed.
The new path adds durable coordinator operations per legacy worker request. Production rollout must measure private-etcd request load as well as merge progress; it is not a throughput optimization.