Repository navigation
fix(sync): refuse stale held snapshots on PostgreSQL and restore River push runs - #762
Conversation
On PostgreSQL a push run's loopback fetch skips a push-held ref and can snapshot it at a value older than the authority (verify-lane-schedule finding 2, fixed for the local schedule only). Serve does not ask PG fetches to hydrate strictly, and RunPush's River branch has no answer to the 423 that a strict fetch returns: it ends the run done, consumes the request and emits started and failed. TestPushRunStaleHeldSnapshotRestoresRiverRequest refuses the fetch with 423 and expects the force request handed back, no counted run, no event and an error for River to snooze on. It fails on this commit: the run consumed the request (push_pending 0, push_run_count 1). Change-Id: I2e6f4be75ab68048f076386723eb53ae4f67935f Signed-off-by: Thomas Kosiewski <tk@coder.com>
…r push runs Serve hydrated a push run's loopback fetch strictly only on the local schedule. On PostgreSQL a River push run could therefore snapshot a push-held ref at a value older than the authority, forward it and end done, and the newer write was never forwarded (verify-lane-schedule finding 2, which #678 fixed for SQLite and S3-WAL only). githttp.Server now hydrates every upload-pack with a sync claim strictly, and the StrictSyncFetch switch goes. RunPush's River branch answers the resulting errHeldRefStale like a failed publication wait: RestorePush hands the request back with its force level, and the run returns an error, so the job's completion gate snoozes it and runs it again. The run emits no event on either backend. ADR-44's PostgreSQL amendment and http-api.md describe the River restore, and the StrictSnapshot fact follows the new RunPush branch. Change-Id: I02744bfbc4e3bc6d07548ac8132c87e017b32f82 Signed-off-by: Thomas Kosiewski <tk@coder.com>
… skill verify-lane-schedule's finding 2 now says that every push run's fetch is strict, and how a River run restores. ADR-44's amendment loses an over-long line. Change-Id: I15c986dcf264531dad7b8cbbb1a281384e09972b Signed-off-by: Thomas Kosiewski <tk@coder.com>
…ne on any backend RunPush no longer sends started for a run whose loopback fetch is refused for a stale held ref, and since the previous commit that holds on PostgreSQL too. The restoreLocal comment still said such a run restores after its started event, and webhooks.md still limited the held-back case to SQLite and S3WAL. Change-Id: I3aea8667e43a5d961396d33e21b378ad63495429 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…failed final write A write to the ephemeral namespace takes no hold and reads no upstream link (EphemeralUpdate in RefStore.runBeforeCAS), so a failed link read does not refuse it. A push run whose metadata write at the end fails (finish push ...) also ends with started and no terminal event. Change-Id: I3bd7409f22d80d23989ab2dd2affe4e8eadb69a8 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…etch check.sh --record: LaneSchedule.cfg PASS (3535660 distinct states) OtherWriters.cfg PASS (56705945 distinct states) Liveness.cfg PASS (348735 distinct states) NoLeaseAssumption.cfg fails as expected RESULT: PASS Change-Id: I7ec9fe0c80a193a5f7bbcbc7a3b47c79b6e8b94e Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
Codex Review Summary: ✅ CompletedThis 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. Can't wait for the next one! 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. |
the verify-lane-schedule skill, which check.sh --record rewrites next. Change-Id: I9ae5e427b98cb9c0635ef529774e889a2365643b Signed-off-by: Thomas Kosiewski <tk@coder.com>
check.sh --record: LaneSchedule.cfg PASS (3535660 distinct states) OtherWriters.cfg PASS (56705945 distinct states) Liveness.cfg PASS (348735 distinct states) NoLeaseAssumption.cfg fails as expected RESULT: PASS Change-Id: I639bbc6eae49a1dd46adaafe4a1197f7d5300923 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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. |
Why
Every accepted write to a
forward_on_pushrepository must reach the upstream at its newest value. #678 made a local push run's loopback fetch refuse a push-held ref whose cached value is older than the authority (HTTP 423), so the run restores its request instead of forwarding the old value and endingdone(verify-lane-schedulefinding 2). PostgreSQL kept the old behavior: serve setStrictSyncFetchonly on the local schedule, because a River run had no answer to the 423. So on PostgreSQL a River push run could still forward a held ref at its older value, enddoneand consume the request of the newer write. Nothing then forwarded that write.What changes
githttp.Server.hydratehydrates an upload-pack with a sync claim strictly on every backend. TheStrictSyncFetchfield and serve's line that set it are gone.RunPushanswerserrHeldRefStaleon PostgreSQL the way it answers a failed publication wait:RestorePushhands the request back with its force level, and the run returns an error, so the job's completion gate snoozes the job and River runs it again. A failed restore returns its error the same way (shutdownErrkeeps a shutdown a shutdown).repo.push_upstream.started. A River run now skips it too.StrictSnapshotcode fact follows the newRunPushbranch and no longer readscmd/openstorage/main.go. ADR-44's PostgreSQL amendment says that a 423 restores a River run, and ADR-50's amendment now says "a push run" instead of "a local push run".docs/reference/http-api.mdsays that an outbound run emits no event while a push holds a ref, on every backend.Behaviour a user can see
repo.push_upstream.startedand a terminal event, and the newer write never reached the upstream.Verification
Red first, the test committed before its fix:
TestPushRunStaleHeldSnapshotRestoresRiverRequest(internal/sync, PostgreSQL,c89b7230) refuses the loopback fetch with 423 and expects the force request handed back, no counted run, no event and an error for River to snooze on. Old code:after a refused snapshot: {... PushPending:0 ...}: the run consumed the request.More tests:
TestServeStrictSyncFetch(internal/githttp) now expects 423 for a sync-claim fetch of a stale held ref on PostgreSQL too.TestServerHydrate_StrictOnlyForPushRunFetchdrops the switch from its table.Round 2 merges main (#755, #756, #757 and #759). The only conflict was the sync line of the verify-lane-schedule skill, which
check.sh --recordthen rewrote: RESULT: PASS with the same state counts as round 1. At this head,internal/sync,internal/jobs,internal/eventsandcmd/openstoragepass on PostgreSQL, local and S3-WAL (all with-race, and with-tags=openstorage_viewcheckexcept on the local backend), both--fingerprints-onlychecks pass, golangci-lint reports 0 issues, and the doc-link check finds 0 broken links. The PR's own diff is the same as in round 1 in every file except the two that--recordwrites.Round 1 (head
bd61fa6d): lanes all with-race, and with-tags=openstorage_viewcheckexcept on the local backend, as in CI:cmd/openstorage,internal/events,internal/sync,internal/api,internal/jobs,internal/gitreadandinternal/simtestpass on PostgreSQL, local and S3-WAL, andinternal/githttppasses on all three. The first localinternal/githttprun died in TestMain withlisten tcp 127.0.0.1:0: bind: address already in usewhile the host had run out of loopback ports. The rerun passed.verify-lane-schedule/check.sh --record: RESULT: PASS (LaneSchedule 3,535,660, OtherWriters 56,705,945 and Liveness 348,735 distinct states, and NoLeaseAssumption fails as expected).mutate.sh: 12 of 12 code mutations and 12 of 12 model mutations failcheck.sh. golangci-lint: 0 issues.go vet ./...is clean. Both--fingerprints-onlychecks pass, and the doc-link check finds 0 broken links.Size
207 changed lines: 71 of production Go, 79 of tests, and 57 of docs and the lane-schedule skill (SKILL.md, facts, fingerprints).
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh