Skip to content

Stamp created and updated times at the version a refused, re-logged retry commits under - #3979

Merged
kriszyp merged 3 commits into
mainfrom
fix/refused-retry-rekey-restamp
Nov 5, 2026
Merged

kriszyp merged 3 commits into
mainfrom
fix/refused-retry-rekey-restamp

Conversation

@kriszyp

@kriszyp kriszyp commented Nov 3, 2026 •

Copy link
Copy Markdown
Member

Kris's daughter re-dates the packed tag on a crate of datefruit sent back from the canyon gate, while Kris's son copies the first-picked date from the stash post

⊙ 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/@createdTime were stamped once, before either change. The record stored version K₁ with times K₀. A create that lost a race kept its own @createdTime over 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 main today.

⚖️ Alternatives

Rejected approaches
  • Reset saved so 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.
  • Run validate again in save(): that repeats checkOverloaded() (a 503 on a retry already admitted), schema checks and blob preparation.
  • Restamp whenever the version differs from validate's: misses the explicit-timestamp case, where only the base changes.

🤔 No fresh log key under a caller's timestamp

rekeyLocalRetry still 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

  • Four new RocksDB cases use a real write intent to force an intentBeforeLog refusal (create; concurrent creator by PUT and by PATCH; concurrent creator under an explicit timestamp). All four fail on origin/main sources 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.
  • Write intents off: unchanged, since the restamp needs relogOnRetry.
  • PR QA: Under concurrent creates with write intents on, does every record's @updatedTime equal its version and @createdtime equal the first committed create's, across a restart? — queued @ 5e4eded, qa-pr-harper-3979-5e4eded

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 relogOnRetry in 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

Late Grok findings

2 surviving (highest minor), 3 previously adjudicated — Grok reviewed fb21161
  • minor — resources/Table.ts:5775 — on a caching table with a write-through source.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 one
  • minor — resources/Table.ts:5931 — the restamp only runs while relogOnRetry === true. Two cases are left that keep a stale @createdTime and 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 ev
  • nit — unitTests/resources/abortMarkersAdoption.test.js:342 — the fixed delay(20) before releasing the holder can lose the race under load (previously adjudicated)
  • nit — resources/Table.ts:5931 — the restamp is keyed on the handle's relogOnRetry, not on a per-write record of the version it was stamped at (previously adjudicated)
  • nit — resources/Table.ts:1798 — the added comment repeats the stampRecordTimes body (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 saved flag 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-1

Review-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

kriszyp and others added 3 commits November 3, 2026 09:58
…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>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kriszyp
kriszyp marked this pull request as ready for review November 5, 2026 23:50
@kriszyp
kriszyp merged commit 89808d7 into main Nov 5, 2026
74 of 76 checks passed
@kriszyp
kriszyp deleted the fix/refused-retry-rekey-restamp branch November 5, 2026 23:50
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