Repository navigation
feat(ref): write a scan-state row only when its generation is the expected one - #698
Conversation
…ected one PutJobScanState is last-writer-wins. That is exact while River keeps one pass per job running, but a pass that outlives its rescue horizon can still be writing when the next pass takes over, and a job that resumes from its row must not let the older pass overwrite it. PutJobScanStateIf writes only when the row's generation is the one the caller read, and returns ErrScanStateConflict otherwise. An absent row counts as generation 0. It is implemented on PostgreSQL, SQLite and S3-WAL. Change-Id: I9e767d6dc355a4843f93f089cbe509926ade831b Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I62c1cef09b31114515922ce51816f348fc4ba643 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 629696e711
ℹ️ 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".
🛡️ 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. |
Two writers that expected generation 0 on a row that did not exist yet could both miss the guarded UPDATE, and the loser of the following INSERT was told that another run took the row over, although the row never left generation 0. PostgreSQL and SQLite now decide it in one upsert whose update half carries the generation guard. Refs #636 Change-Id: Iee2807b64819ac3568fc83eb32fc2e1f31af1f79 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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. |
…scan state fence safe Change-Id: Iee454987b8e98be6b7d2053365f539ec6174ebd3 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I2d256db40bbbda3bfe2d907184ffc98ee888c4f5 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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. |
… own error Change-Id: I3ee20ce147624db6ec7cc5bfbcfd0dea0f85e4b6 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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
ThomasK33 asked to fix and build the open issues and land them on main. This is part C of item 2 of #636, the resumable orphan sweep. The sweep will keep a cursor in a
job_scan_staterow.PutJobScanStateis last-writer-wins, and the header ofscanstate.goargues that River's unique jobs never race. #636 shows that a stuck sweep can still run beside the next one after the rescue horizon, so a run that lost the row could overwrite the new run's cursor. This PR adds a write that lands only when the row has the generation the writer read. Nothing calls it yet. A later PR of #636 makes the sweep save its cursor with it.What changes
ScanStateStore.PutJobScanStateIf(ctx, job, scope, payload, generation, expectGeneration)writes the row only when its current generation isexpectGeneration. An absent row counts as generation 0. Otherwise it writes nothing and returns the newErrScanStateConflict.UPDATEguarded by the expected generation. For an expected generation 0, anINSERT ... ON CONFLICT DO UPDATE ... WHERE generation = 0upsert, so an absent row is created, a row at generation 0 is rewritten, and two creates that race do not refuse each other.scanMutate), so the check holds at the write.scanstate.goheader andinternal/ref/README.mdname the new method and when to use it.ErrScanStateConflict.Scope
PutJobScanState.Verification
TestJobScanState_PutIfruns on PostgreSQL, SQLite and S3-WAL. It checks an absent row, a stale generation, the current generation, a row at generation 0, the run that lost the row, and eight concurrent writers that hold the same generation (exactly one wins). It passes on all three backends.TestJobScanState_PutIfCreateRace: six writers expect generation 0 on a new row and write generation 0, eight rounds. All succeed on the three backends. With the earlier two-statement PostgreSQL code it fails in 3 of 3 runs:a writer that expected generation 0 on a row at generation 0 got scan state row changed: conditional write refused.TestJobScanState_PutIfCreateWins: six writers expect generation 0 on a new row and write generation 1. Exactly one creates it, on the three backends. A SQLite upsert guard that also accepts a row already at the new generation fails it:6 writers created the row, want exactly 1.TestJobScanState_PutIf:UPDATEwithout its generation guard:write with a stale generation = <nil>, want ErrScanStateConflictcreate over an existing row = <nil>, want ErrScanStateConflictwrite that expects a generation on an absent row = <nil>, want ErrScanStateConflictgo test -short -raceof./internal/...and./cmd/..., andgo test -race ./internal/ref/with the container lane: pass.go build ./...andgo vet ./...: clean.golangci-lint run ./internal/ref/...(v2.15.0): 0 issues.Refs #636
Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh