Skip to content

test: cover the River stale-snapshot restore and receive-pack's lenient hydrate - #775

Merged
ThomasK33 merged 1 commit into
mainfrom
river-stale-restore-tests
Nov 8, 2026
Merged

ThomasK33 merged 1 commit into
mainfrom
river-stale-restore-tests

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented Nov 8, 2026 •

Copy link
Copy Markdown
Member

Why

#762's verifier passed it with three test gaps. Each one let a mutant survive, or left a probe out of the repository:

  1. The River branch of the stale held snapshot (worker_push.go, the restore after a refused loopback fetch) had no test for a failing RestorePush or for a shutdown during the restore. Three mutants survived: W6 returns nil after a failed restore, W7 drops shutdownErr around the restore error, and W8 drops it around the run's error.
  2. hydrate's strict rule is service == "upload-pack" && claims.SyncClaim != "". A mutant that drops the service check survived: a receive-pack with a sync claim gives the same error either way, because receive-pack tolerates ref-sync failures.
  3. The stale held snapshot had an end-to-end replay for the local schedule only. The verifier's PostgreSQL probe for River's wiring stayed in its scratch folder.

This PR changes tests only.

What changes

  • TestPushRunStaleHeldSnapshotRiverRestoreFailures (internal/sync) refuses the loopback fetch with 423 and wraps the store's RestorePush. It checks three cases:
    • The restore fails: the run returns the restore's error, so River does not complete the job.
    • A shutdown lands during a failing restore: the error is also ErrShutdown, which keeps the job for the next process.
    • A shutdown lands during a restore that succeeds: the error is ErrShutdown, and the force request is handed back with no counted run.
  • TestServerHydrate_ReceivePackWithClaimIsLenient (internal/githttp) sets up a held-skip verdict for a stale held ref. A receive-pack with a sync claim must reuse that verdict (no new scan of the authority's refs), and an upload-pack with the claim must scan.
  • TestRiverForwardHeldStaleRefRestores (internal/githttp, PostgreSQL only) is the verifier's probe. It shares its body with TestLocalForwardHeldStaleRefRestores (forwardHeldStaleRefRestores), which leaves out the local lanes for River. On River, the first run must return an error and hand its request back with no counted run. The lane then settles with the upstream at the REST write's commit. It calls Worker.RunPush directly with PostgreSQL stores and no local lanes, so it pins the worker's River branch. River's own job handling (the snooze after the error) is not part of this test.

No covered production file changes, so the verify-lane-schedule fingerprints and the SKILL.md sync line stay as they are.

Verification

Mutants, each applied alone:

W6 (return nil after a failed restore):           --- FAIL: .../restore_fails, .../shutdown_during_a_failing_restore
W7 (restore error without shutdownErr):           --- FAIL: .../shutdown_during_a_failing_restore
W8 (run error without shutdownErr):               --- FAIL: .../shutdown_during_the_restore
S2 (strict := claims.SyncClaim != ""):            hydrate_strict_test.go:117: receive-pack with a sync claim: 1 ref scans, want scans=false
S1 (strict := false in server.go, PostgreSQL):   local_held_snapshot_e2e_test.go:46: refused River run returned nil; want an error, so River runs the job again

S1 was already red in older tests (TestServerHydrate_StrictOnlyForPushRunFetch, TestServeStrictSyncFetch, TestLocalForwardHeldStaleRefRestores). This PR adds its red on the River branch. S2 is the mutant that no test caught before.

Lanes at ad2c112, each test lane under the host's shared heavy-validation lock:

  • go build ./..., go vet -tags=openstorage_viewcheck ./... and golangci-lint: pass, 0 issues.
  • verify-lane-schedule and verify-orphan-sweep check.sh --fingerprints-only: RESULT: PASS.
  • go test ./internal/sync -race on PostgreSQL, local and S3-WAL: ok.
  • go test ./internal/githttp -race -run 'TestServerHydrate_|TestHydrateStrictHeld|TestLocalForwardHeldStaleRefRestores|TestRiverForwardHeldStaleRefRestores|TestServeStrictSyncFetch' on PostgreSQL, local and S3-WAL: 5 pass and 1 skip on each. The skip is the other schedule's replay: TestLocalForwardHeldStaleRefRestores skips on PostgreSQL, and TestRiverForwardHeldStaleRefRestores skips on local and S3-WAL.
  • The branch merges with main at 536ab6a with no conflict, and go vet of internal/githttp and internal/sync passes on that merge, with and without the viewcheck tag.

Size

155 changed lines, all tests.


Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: xhigh

…nt hydrate

probe that the tests did not keep:

- TestPushRunStaleHeldSnapshotRiverRestoreFailures: a River push run
  whose loopback fetch is refused for a stale held ref fails when its
  RestorePush fails, and its error is ErrShutdown when a shutdown
  lands during the restore, failing or not. Mutants W6, W7 and W8
  (worker_push.go: return nil, or drop shutdownErr) now fail it.
- TestServerHydrate_ReceivePackWithClaimIsLenient: a receive-pack whose
  token carries a sync claim reuses the held-skip verdict and does not
  scan the authority again. The mutant that drops service ==
  "upload-pack" from hydrate's strict rule now fails it.
- TestRiverForwardHeldStaleRefRestores: the verifier's PostgreSQL probe,
  the end-to-end replay of the stale held snapshot on River's wiring. It
  shares its body with TestLocalForwardHeldStaleRefRestores.

Change-Id: I08d0da600391204f8dd9ee0f048e2234533c3480
Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 8, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-08T00:57:35.062122Z ad2c112 Manual request
🔒 Security Review ✅ Completed 2026-11-08T00:57:35.142236Z ad2c112 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: ad2c112faf

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: ad2c112faf

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@ThomasK33
ThomasK33 added this pull request to the merge queue Nov 8, 2026
Merged via the queue into main with commit 6f1c069 Nov 8, 2026
11 checks passed
@ThomasK33
ThomasK33 deleted the river-stale-restore-tests branch November 8, 2026 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant