Repository navigation
fix(jobs): never complete a sync job while its sync claim is held - #716
Merged
Merged
Conversation
…h-lane schedule Model-checks the ADR-50 push lane (SQLite and S3-WAL) against git pushes, push-upstream calls, runs, steals, retries, restores, crashes and shutdown. Nine code facts tie the model to both backends and the worker; fingerprints in the lane-schedule-fingerprints CI job flag covered-code changes. The NoCompletionAssumption probe records two findings: a push that dies after its authority CAS can lose its forward. Change-Id: I07baac11eba4d51333064aa0fb5e90dd84e48653 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…e model variables Change-Id: Iafc903e2e9a284d33bd0cb076b93f5c621837eb2 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…print the push-hold path Codex round 1 on #675: fingerprint the push-hold lifecycle, both refspec vars and LocalScheduler.Start; model pushes that record no intent; state that ref deletions are not forwarded by design. The model now counts a write as owed at its CAS, adds REST writes (OtherWriters.cfg) and a HeldRefsFresh probe that fails with no fault. Change-Id: I863914ac2567ec75ba1b8f92a9b68663b3816c6d Signed-off-by: Thomas Kosiewski <tk@coder.com>
On PostgreSQL a River push run on the node that took the push can snapshot the cache before receive-pack publishes the pushed ref, and a hook that loses its CAS outcome after the commit records no forward intent. Both tests fail on this commit: the upstream never gets the accepted main. Change-Id: Ia41ff2be8d3b15ff159e7531f8955a667355b237 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…pshots A git push's River job exists from its authority CAS on, before receive-pack publishes the cache ref. A push run on the same node could claim it, snapshot the cache while the push's hold kept the old value, forward that, and end done: the accepted push never reached the upstream. River push runs now call the publication wait after the claim, like local runs. A failed wait hands the request back with RestorePush and fails the run, so the job's completion gate snoozes it. Fixes TestPGHookForwardBeforeCachePublication. Change-Id: I5c7fc02af849c5fe27d90e121156e1b29d6d4c4d Signed-off-by: Thomas Kosiewski <tk@coder.com>
…EST CAS On PostgreSQL a REST write records forward intent only after its ref CAS commits. A serve process that dies between the two leaves the accepted write with no push job. The test fails on this commit. Change-Id: I4f1721fdef341cf8834e00c5a79a87f484afa930 Signed-off-by: Thomas Kosiewski <tk@coder.com>
On PostgreSQL a write to a forward_on_push repository recorded its forward intent (push_pending and the River push job) only after the ref CAS committed. A hook or serve process that died in between, or a failed RequestPush or EnqueuePush, left the accepted write with no intent, and nothing on PostgreSQL scans for it. Metadata and refs share one PostgreSQL database, so the CAS now writes the intent in its own transaction: ref.WithCASTxHook runs a hook after a won swap statement and before COMMIT, and the serve and hook wiring set it for every write the sync poke forwards. The pending level, the push job and the ref commit together or not at all. The after-CAS poke stays, for old replicas during a rolling upgrade and for wins recovered from another caller's identical write. Fixes TestPGHookForwardAfterLostCASOutcome and TestServeRecordsForwardIntentWithTheCAS. Change-Id: I18b0962ee614728adffac868277ab8164c8ce97a Signed-off-by: Thomas Kosiewski <tk@coder.com>
…saction Change-Id: Ib7dd1c7257f42c1f6a65197cfb066cec9616b9a9 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…efore the poke The CAS transaction now inserts the push job, so a River run can claim it before the decorator's after-CAS sync poke runs. The poke then marks push_pending again, and the job's completion schedules a second forward run with its own repo.push_upstream events. The new subtest fails on this commit (PushPending:1 after the claim). Change-Id: Id72e26deed6ab966bbc467120b2295901d8efae2 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…e job first The sync poke after a PostgreSQL CAS re-marked a forward that the CAS transaction had already recorded. When a run claimed the new job before the poke, that scheduled a second forward run. The CAS hook now reports through ref.CASTxHook.Committed that its committed transaction recorded the intent, and the poke skips the forward then. A win that ref.IdempotentCAS recovered still gets the poke. RecordForwardTx now inserts the push job before it marks push_pending. It used to hold the upstream row while it waited for a job row held by the job's completion transaction, which reads that row through another pool. With that pool full of statements waiting on the row, neither side could move, and PostgreSQL cannot see that cycle. Fixes TestServeRecordsForwardIntentWithTheCAS/claimed_before_poke; adds TestRiverRecordForwardTxWaitsForJobBeforeUpstreamRow. Change-Id: Id5c9241b5b84457fbe41988cc62e57a2a30bd9ef Signed-off-by: Thomas Kosiewski <tk@coder.com>
… River publication wait A River push run whose publication wait the shutdown drain cancelled restored its request through the 30-second detached finish instead of the short shutdown restore that a cancelled transfer uses. It now uses restoreForShutdown and returns ErrShutdown, so River keeps the job. TestRiverPushRestoredRunSnoozesForRetry pins that the job of a run that restored its request after a failed wait snoozes and runs again. Change-Id: I4c97e34e77aa6812b2949a34cee0d96358cc6867 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I008f14e5ef848e5505344bebcf6288b8e3bec719 Signed-off-by: Thomas Kosiewski <tk@coder.com>
… caller's write Change-Id: Id162f732f5606bf1f0157e29aa2a6425c5039415 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…inish Two River push runs end with their claim held and the request they consumed: a run whose publication-wait restore the shutdown drain cancels returns an error without ErrShutdown, and a run whose finish or restore write fails returns a plain error. The push job's completion gate then completes the job, and nothing forwards the request. Both new tests fail on this commit. Change-Id: Id0fdef5b68c69bfebd9e5177657e4523f9035505 Signed-off-by: Thomas Kosiewski <tk@coder.com>
… held A River push run whose finish or restore write failed returned a plain error with its claim still held and the request it had consumed, and the job's completion gate completed the job. The gate now snoozes such a job until the claim's lease runs out; the re-run steals the claim and its force level, and forwards. A run that the shutdown drain cancels during its publication-wait restore now also returns ErrShutdown, so River keeps that job too. Fixes TestPushRunShutdownDuringFailedWaitRestoreKeepsJob and TestRiverPushFailedFinishKeepsJobUntilClaimExpires. Change-Id: I7eb270392d85832ea7b510eb3dd331bca0ee46d0 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…ead or completion The completion gate keeps a push job whose failed run left its claim held only when that same Work call saw the run fail and read the row before the run. When the pre-run read fails, or when the first completion fails and the re-run meets the unexpired claim, the gate completes the job and the consumed request is lost. Both new subtests fail on this commit. Change-Id: I7e7b8b8a6012adf537cd91c54e2d8fdffdd4c70e Signed-off-by: Thomas Kosiewski <tk@coder.com>
Only the push job runs a repository's push lane, so a claim still held at completion belongs to one of the job's own runs whose finish or restore failed, or that died, and it holds the request it consumed. The completion gate now snoozes the job until the lease can be stolen whatever this Work call saw, instead of only when it saw the run fail and knew the run count. Fixes TestRiverPushFailedFinishKeepsJobUntilClaimExpires subtests pre_run_read_fails and completion_fails_once. Change-Id: I0b69741189f40561b874cb313fec0436a11e55e3 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Also correct the shutdownSnooze and RecordForwardTx docs. Change-Id: Ida509a3c433a7cc4b746c6621ef89e1868414704 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…im held The gate snoozes a push job while its push claim is held, so that the re-run can steal the claim. A run for a repository deleted while queued returns before it claims, so nothing stole the claim, and the job snoozed every 5 seconds forever. On the River lane RunPush now returns ErrRepoGone for a deleted repository, and the push worker completes the job on it. Also correct the pushWorker and RecordForwardTx docs. Change-Id: I7345fd33816f75cf1d1725592507c742f2d77af3 Signed-off-by: Thomas Kosiewski <tk@coder.com>
… log no warning for a deleted repo's run Change-Id: I892bc4f7a4d8cba5b77ed9d4ad4e76543473342d Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I5b2a152ddfdc740e0956b9e08d1d309eb31c5d31 Signed-off-by: Thomas Kosiewski <tk@coder.com>
… its claim held On PostgreSQL a sync run whose FinishSync fails returns a plain error with its claim still held and the request it consumed, for example the mirror-back pull a forward marked. The sync job's completion gate keeps the job only while SyncPending is set, which the claim cleared, so it completes the job and nothing pulls. TestRiverSyncFailedFinishKeepsJobUntilClaimExpires fails on this commit in all three subtests. TestWorkerDeletedRepoOnRiverLane and TestRiverSyncDeletedRepoCompletesWithClaimHeld pin the deleted-repo exit that the fix needs. Change-Id: Idf901c182691d1d4042cc68ac61a75543687d14d Signed-off-by: Thomas Kosiewski <tk@coder.com>
A River sync run whose FinishSync failed, whose claim committed but reported an error, or that died left its claim holding the request it consumed: a pull-upstream, or the mirror-back pull that a forward marked. The sync job's completion gate completed the job, because the claim had cleared SyncPending, and nothing pulled. The gate now snoozes the job while a sync claim is held, until just past the lease (legacy claims at legacy pace), and the re-run steals the claim. On the River lane Run returns ErrRepoGone for a deleted repository, and the sync worker completes that job. Fixes TestRiverSyncFailedFinishKeepsJobUntilClaimExpires, TestRiverSyncDeletedRepoCompletesWithClaimHeld and TestWorkerDeletedRepoOnRiverLane. Change-Id: I412b17659a9e3d6758992f4854c37a0ccf00822d Signed-off-by: Thomas Kosiewski <tk@coder.com>
… held Change-Id: I2f271296217395ee253f7ceeafc79e7350eca645 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…l lane's deleted-repo return Change-Id: Iaf3ab76ca0c94a5adba58f938e3bb894ea0375fd Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I5ac1cada3ab8e82912c6f54360b49dc00446ae7a Signed-off-by: Thomas Kosiewski <tk@coder.com>
…te fix Change-Id: Ibe9caf585a866e46f2060cc1268761ebf08ae53c Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I442745c1b126d52ff1e4f0da92d24f0ed71e2551 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Change-Id: Ic3f65baa776dada1745dd212ee6ea38fdc51e97a Signed-off-by: Thomas Kosiewski <tk@coder.com>
…st snoozes the job The gate's held-claim case also covers an adopted legacy claim whose grace lapsed: the run skips without OPENSTORAGE_SYNC_STEAL_LEGACY, and the job must snooze at legacy pace instead of completing over the claim. Only the unit test of the snooze delay covered that pace, so excluding legacy claims from the case turned no test red. Change-Id: I20649e7d25745a2b69c4381102baee2fb1941e59 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I87154081f1614228b85513656e6554b14f92c6d9 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
ThomasK33
added a commit
that referenced
this pull request
Nov 6, 2026
check.sh --record at 0be01a7: 11 facts TRUE, LaneSchedule.cfg 3535660, OtherWriters.cfg 20496186 and Liveness.cfg 348735 distinct states pass, both probes fail as expected, and every covered declaration matches the fingerprints. Change-Id: I28528f22a12ea6ef0a49dc3af4bee258bd9c26d9 Signed-off-by: Thomas Kosiewski <tk@coder.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.
Why
Every accepted write to a
forward_on_pushrepository must reach the upstream, and no crash window may silently lose a forward. After a forward, the push run marks a mirror-back pull so the cache picks up the upstream's state. On PostgreSQL, the sync job's completion gate completed the job while a sync claim was still held. A run whoseFinishSync(orRestoreSync) failed after its retries kept its claim and the request it had consumed. The job completed, and nothing re-ran the pull. This PR applies the push gate's rule from #679 to the pull lane. SQLite and S3-WAL behave as before.Fix
syncSnoozeDelayuntil the lease ends. Its re-run steals the claim and does the pull. A lapsed legacy claim with no pending request behaves differently: the job used to complete, and it now snoozes every 5 minutes (legacyClaimSnooze) until the legacy holder finishes orOPENSTORAGE_SYNC_STEAL_LEGACYlets the re-run steal the claim.Worker.Runnow returnsErrRepoGonewhen the repo lookup finds no repository. The sync worker completes the job before its gate, whatever claim the row still shows. The local lane still returns nil.Rolling upgrade. No schema change, the same job kind and args. An old binary still completes over a held claim. A new binary's job snoozes until it can steal such a claim. Rollback is safe.
Tests
Each reproduction fails on main (faea6e1):
TestRiverSyncFailedFinishKeepsJobUntilClaimExpires(plain,pre_run_read_fails,completion_fails_once)internal/jobsSyncing:true SyncPending:false RunCount:0)TestRiverSyncDeletedRepoCompletesWithClaimHeldinternal/jobsTestWorkerDeletedRepoOnRiverLaneinternal/syncRunreturned nil for a deleted repo on the River laneTestRiverSyncLapsedLegacyClaimSnoozesWithoutPending(internal/jobs) drives the job over a lapsed legacy claim with no pending request: the run skips, and the job must snooze forlegacyClaimSnoozewith the claim still held.Five source mutations each turn at least one of these tests red: the held-claim case removed, its snooze delay, the
ErrRepoGoneearly return,Run'sErrRepoGone, and the held-claim case skipping legacy holders (up.Syncing && up.SyncClaimHolder != sync.LegacyClaimHolder).The
verify-lane-schedulemodel covers only the local schedule. This PR changes only the River branch ofRun, and the local lane's deleted-repo return stays nil (pinned byTestWorkerDeletedRepoOnRiverLane), so the model still describes the local code.check.shpasses, andcheck.sh --recordrecorded the merge of main as the sync commit (the only changed covered declaration isWorker.Run).Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh