Repository navigation
Stamp created and updated times at the version a refused, re-logged retry commits under - #3979
Merged
Merged
Conversation
…atch A commit refused before its log write retries under a fresh key, so its record version moves past the write that beat it, and the retry reloads the commit base. Validate ran once, before either, so the record kept the discarded attempt's @updatedTime and a @createdtime taken from the old base, which overwrote a concurrent creator's. The commit of a re-logged retry now re-stamps both from the version and base it commits under. A write under a caller's timestamp keeps its version and its reserved key; the docstring now says why the key is not re-minted there. Dispatch-Task: harper-write-intents-refused-retry-rekey Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispatch-Task: harper-write-intents-refused-retry-rekey Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispatch-Task: harper-write-intents-refused-retry-rekey Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a helper function stampRecordTimes in resources/Table.ts to consolidate the logic for assigning created and updated timestamps to records. It also updates the transaction retry mechanism to re-stamp these times when a transaction is re-logged after a refusal, ensuring correct timestamps are applied when committing under a new version or onto a reloaded base. Corresponding unit tests have been added to verify timestamp correctness under various retry and race conditions, and the design documentation has been updated to reflect these changes. I have no feedback to provide as there are no review comments.
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.
⊙ Problem
With
storage.rocks.transactionLogWriteIntents: true, a commit refused before its log write retries. Its record version moves to the new log key (#3928). The retry also reloads the record it applies to. But@updatedTime/@createdTimewere stamped once, before either change. The record stored version K₁ with times K₀. A create that lost a race kept its own@createdTimeover the creator's, also under an explicit timestamp.🤔 Is this the right requirement?
Yes. Both times must reflect the stored version and the record the write applies to.
💡 Solution
A re-logged retry now stamps the times again at commit, using its own version and reloaded base (the restamp in
commit). The stamping rules moved, unchanged, into one helper shared by validate and the retry (stampRecordTimes). Review the base choice closely (priorStaged ?? existingEntry). The abort-marker design note records the rule.🤔 Caching sources keep the first attempt's times
A write-through source received the record before the commit was refused. Sending it again would be a second external write per retry, so this PR does not. After a refill from the source, the record has the same times it has on
maintoday.⚖️ Alternatives
Rejected approaches
savedso validate runs again (the task's suggestion): the retry loop skips writes that are not saved, so the write would be dropped. Forcing a re-save would also call the source again.save(): that repeatscheckOverloaded()(a 503 on a retry already admitted), schema checks and blob preparation.🤔 No fresh log key under a caller's timestamp
rekeyLocalRetrystill keeps the reserved key when the version is the caller's value (transaction({ timestamp }), lock holds). There the version never follows the key, so a new key orders nothing. The reserved key was never written and stays above the floor. I corrected its docstring and did not change the code.🤔 A retry after a conflict at commit still keeps validate's times
This includes the default setup (write intents off) and a re-logged attempt that later conflicts at commit. Its log batch is already written, so the fix needs a re-log, not a restamp. Pre-existing; filed as A retry after a conflict at commit keeps its own @createdTime over a concurrent creator's (#3975).
✅ Verification
intentBeforeLogrefusal (create; concurrent creator by PUT and by PATCH; concurrent creator under an explicit timestamp). All four fail onorigin/mainsources after a forced rebuild, and pass with the fix.abortMarkersAdoption.test.js: 19 passing.npm run test:unit:resources: 6462 passing, 3 failing. All 3 pass alone; their tables have no time attributes, so the new code does not run there.relogOnRetry.Refs: Dispatch task
harper-write-intents-refused-retry-rekey. Docs: none needed (no API or config change).Framing-Verdict: better-alternative-exists — adopted (restamp gated on
relogOnRetryin commit, no per-write marker)Claude Opus 5.5
🤖 Generated with Claude Code
Release target: main — fixes code from #3928, which is only on main
Related PRs: #3928 overlaps (this fixes a gap in its re-log path)
Complexity: complicated
Deferred findings
1209e9e264aa6025minor correctness —resources/Table.ts— at-commit retry skips restamp; stale createdTime overwrites concurrent creator's — filed A retry after a conflict at commit keeps its own @createdTime over a concurrent creator's #3975Late Grok findings
2 surviving (highest minor), 3 previously adjudicated — Grok reviewed fb21161
resources/Table.ts:5775— on a caching table with a write-throughsource.put/source.patch, the source keeps the first attempt's@createdTime, and the next fill from the source puts that value back over the corrected local oneresources/Table.ts:5931— the restamp only runs whilerelogOnRetry === true. Two cases are left that keep a stale@createdTimeand overwrite a concurrent creator's: a re-logged attempt that then conflicts at commit, and the default configuration (write intents off), where the first conflict is at commit and no restamp evunitTests/resources/abortMarkersAdoption.test.js:342— the fixeddelay(20)before releasing the holder can lose the race under load (previously adjudicated)resources/Table.ts:5931— the restamp is keyed on the handle'srelogOnRetry, not on a per-write record of the version it was stamped at (previously adjudicated)resources/Table.ts:1798— the added comment repeats thestampRecordTimesbody (previously adjudicated)Not standing: none. Grok ran 17m (18m from launch to report); the fold adjudication (
cursor-grok@fb21161f074f-a7f11607, claude) took 1m.Origin — the dispatch brief this PR was written from
With storage.rocks.transactionLogWriteIntents: true, a transaction refused before it writes its log batch (ConflictKind.intentBeforeLog, transaction.logWritten() === false) is retried by noteRetryBoundary / rekeyRefusedRetry (resources/DatabaseTransaction.ts ~1631-1658), which advances this.timestamp to K_1. save() (~2652) then skips validateWrite because the write's
savedflag is never reset, so @createdTime/@updatedTime stay at the discarded K_0 while the version becomes K_1 (and can overwrite a concurrent creator's later @createdtime with an earlier one). Fix: reset the per-write saved state on a rekeyed retry so validateWrite re-stamps times at the new timestamp. Second arm: rekeyLocalRetry (resources/originClosedFloor.ts ~420-431) returns undefined when reservation.aroundCallerValue is true (transaction({ timestamp }) and Table.lock(id, { hold: true })), so attempt 2's local log entry keeps K_0 below the holder's key. Its docstring says the caller's timestamp is deliberately frozen: decide whether the stale log key is acceptable, fix it if a small change is clearly correct, otherwise explain the reasoning in the PR body.Acceptance
Unit test with transactionLogWriteIntents on that forces an intentBeforeLog refusal and asserts @createdTime/@updatedTime equal the committed version timestamp; red on origin/main sources (forced rebuild), green after. Behaviour with the option off unchanged.
Dispatch: task
harper-write-intents-refused-retry-rekey· queued by finding-triage/wave-46 · ran by claude/opus/high · worker kzyp-xps-1Review-Coverage: authored=claude; ran=cursor-composer,cursor-grok,gemini,codex; late=cursor-grok:adjudicated; adjudicated=domain; blocked=claude(fallback)(out-of-budget); declined=muse; rounds=4; full=3 @ 5e4eded
Review-Attention: study ~20m (critical: Table.ts; decisions: restamp-in-commit-closure, sourced-table-relog-semantics, at-commit-retry-scope) @ 5e4eded