fix(resume-library): add deterministic tiebreaker for same-millisecond saves - #908
Conversation
s-annam
left a comment
s-annam
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES (1 Blocking finding)
Real fix for #907's actual root cause — monotonicNow() correctly eliminates the same-process, same-millisecond ties that caused the flake, since it's a synchronous JS counter and can't race within one event loop. Two things to fix before merge.
Blocking
-
The new regression test doesn't verify order (
src/lib/resume-library.test.ts:95-101). Verified by reverting both fix lines (monotonicNow()→Date.now()incrud.ts, and theid.localeComparetiebreak → plainb.savedAt - a.savedAtinresume-library.ts) and re-runningorders back-to-back saves deterministically under contention5/5 times — it still passes. It only assertslist.toHaveLength(5)andnew Set(filenames).size === 5, never the actual ordering. So it can't catch a regression to the fix it's testing, and #907's AC1 ("deterministic, correct newest-first order... when two records share the samesavedAtmillisecond") is unverified by anything in the suite — the original two-save test never forces a tie either. The PR description says this test is "verifying deterministic ordering for concurrent back-to-back saves"; it doesn't.Fix: force an actual tie and assert the resulting order.
monotonicNow()makes ties unreachable through the normal write path, so the honest way is to mirror whatbackup.ts'simportAllalready does — twoputRecord(..., { touch: false })calls with an explicit, identicalupdatedAt— then assertlist.map(e => e.id)matches the id-sorted order the comparator produces. That also gives thea.id.localeCompare(b.id)tiebreaker its first real test.
Secondary
-
New clock/old clock split on the same store family (
src/lib/storage/crud.ts:192— root of the finding;softDeleteRecordatcrud.ts:340is unchanged).putRecordVianow stampsupdatedAtvia the new module-globalmonotonicNow(), which can run ahead of the real wall clock under contention — every tie bumpslastTimestampby 1ms and it doesn't fall back untilDate.now()catches up.softDeleteRecordstill stamps with a freshDate.now(). It backsjobs.ts:134andletters.ts:98/176.Concrete failure: several jobs saved back-to-back under contention drift
lastTimestamp~2ms ahead of real time → a job is soft-deleted immediately after (rawDate.now(), still ~2ms "behind" the drifted counter) → the tombstone'supdatedAtends up smaller than the just-written job's.listRecordsUpdatedSince's own docblock says the extension replicator's cursor depends on writes landing in strictly-increasingupdatedAtorder; a replicator that already advanced its cursor past that job'supdatedAtwould never see the tombstone (updatedAt > sinceexcludes it) — a silent, permanent sync drop, not just a UI sort nit. Before this PR both paths shared one raw-Date.now()clock, so this specific inversion wasn't reachable; the fix introduces it by only patching one of the two writers.Fix: route
softDeleteRecord's stamp through the same clock (exportmonotonicNow(), or hoist a single shared helper) instead of a second, independentDate.now()call.
AC checklist (#907)
- Root cause addressed in code (
monotonicNow()removes the same-process tie source;id.localeCompareis a real tiebreaker for the remaining case — imported/restored records with a preservedupdatedAt). - "deterministic, correct newest-first order... when two records share the same
savedAtmillisecond" — not verified by any test (Blocking finding above). - No regression to existing
resume-library.test.tscoverage — all 17 pass, plus the rest of the storage suite (143 tests acrossstorage.test.ts,letters.test.ts,job-tracker.test.ts,library-changes.test.ts,resume-library.test.ts).
Description accuracy (3f)
One overclaim, covered above: "Added test... verifying deterministic ordering for concurrent back-to-back saves" — the test verifies count + uniqueness, not ordering. Everything else in the description matches the diff.
Gates
npm run typecheck— clean.npm run lint— clean.- Targeted suite (
resume-library.test.ts,storage.test.ts,library-changes.test.ts,letters.test.ts,job-tracker.test.ts) — 143/143 passing. npx fallow audit --base origin/main— 0 dead code, 0 complexity; 1 duplication group inresume-library.test.ts, flagged as an inherited (pre-existing) finding, not attributable to this diff.check:fixtures— n/a, no fixtures touched.- Design-system / style-token gates — n/a, no
src/components/**or Tailwind changes.
Reviewed by: Claude Sonnet 5 (high)
| it("orders back-to-back saves deterministically under contention", async () => { | ||
| const promises = Array.from({ length: 5 }, (_, i) => save(`resume-${i}.pdf`, 70 + i)); | ||
| await Promise.all(promises); | ||
| const list = await listLibrary(); | ||
| expect(list).toHaveLength(5); | ||
| expect(new Set(list.map((e) => e.filename)).size).toBe(5); | ||
| }); |
There was a problem hiding this comment.
Blocking — this test doesn't verify order. Reverting both fix lines (monotonicNow() → Date.now() in crud.ts, and the id.localeCompare tiebreak → plain b.savedAt - a.savedAt here) and re-running this test 5/5 times, it still passes — it only checks toHaveLength(5) and unique filenames, never the actual order. So it can't catch a regression to the fix it's named for, and #907's AC1 ("deterministic, correct newest-first order... when two records share the same savedAt millisecond") stays unverified by anything in the suite.
Suggest forcing a real tie — mirror what backup.ts's importAll already does: two putRecord(..., { touch: false }) calls with an explicit, identical updatedAt — then assert list.map(e => e.id) matches the id-sorted order the comparator produces. That also gives the a.id.localeCompare(b.id) tiebreaker its first real test.
| ): Promise<T> { | ||
| const db = await looseDB(opener); | ||
| const now = Date.now(); | ||
| const now = monotonicNow(); |
There was a problem hiding this comment.
Secondary — putRecordVia now stamps via this new module-global monotonicNow(), which can run ahead of the real wall clock under contention (each tie bumps lastTimestamp +1ms, doesn't fall back until Date.now() catches up). softDeleteRecord (unchanged, line 340) still stamps with a fresh Date.now() — it backs jobs.ts:134 and letters.ts:98/176.
Concrete failure: several jobs saved back-to-back under contention drift lastTimestamp ~2ms ahead of real time → a job is soft-deleted right after (raw Date.now(), still ~2ms "behind") → the tombstone's updatedAt ends up smaller than the just-written job's. listRecordsUpdatedSince's own docblock says the extension replicator's cursor depends on strictly-increasing updatedAt across writes — a cursor that already passed the job's updatedAt would never see the tombstone. Before this PR both paths shared one raw clock, so this specific inversion wasn't reachable; patching only one of the two writers introduces it.
Suggest routing softDeleteRecord's stamp through the same clock (export monotonicNow(), or hoist one shared helper) instead of a second, independent Date.now() call.
…ced tiebreaker tests
|
Thanks for the thorough and constructive review! Both the blocking and secondary findings have been addressed, verified, and pushed in commits 1. Blocking Finding — Verifying Deterministic Order & Tie-Breaking
2. Secondary Finding — Unified
|
s-annam
left a comment
s-annam
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES (1 Blocking finding)
Re-review after the push to b0812a9. Both findings from the prior round are addressed — softDeleteRecord now routes through monotonicNow() (crud.ts:340), closing the clock-split hazard, and it's backed by a real regression test (storage.test.ts) I verified deterministically catches a revert of that fix (5/5 fails when reverted). Good fix. One new issue surfaced by the rewritten tiebreaker test.
Blocking
-
The rewritten tiebreak test still doesn't verify the tiebreaker (
src/lib/resume-library.test.ts:97-126). Verified by reverting only theid.localeCompare(b.id)tiebreak inresume-library.ts(back to plainb.savedAt - a.savedAt) and re-runningbreaks ties deterministically on same-millisecond savedAt5 times — it still passes every time.Root cause:
listLibrary()reads viagetAllResumes()→getAllRecords()→db.getAll(store)(crud.ts:244) with no index, so IndexedDB itself already returns the two tied records in ascending primary-key order. The test's ids are literally"id-a"and"id-b"— already ascending — soArray.prototype.sort's stability alone reproduces["id-a", "id-b"]with or without the explicit comparator tiebreak. The test can't distinguish "the tiebreaker works" from "IDB's own key order happened to already match it," so AC1 of #907 ("deterministic, correct newest-first order... when two records share the samesavedAtmillisecond") is still formally unverified.This is a different failure mode than last round's (that test didn't check order at all; this one checks order but the check is confounded), so it's a fresh finding, not a repeat.
Fix: the storage layer's own ordering has to be taken out of the loop to actually exercise the comparator — e.g. stub/mock
getAllResumes(ordb.getAll) to return the two tied records in descending id order, then assertlistLibrary()still produces["id-a", "id-b"]. That proves the explicit tiebreak overrides whatever order the store returns, rather than merely agreeing with it by coincidence.
Nit
- The new
preserves newest-first save order when saves occur in the same clock millisecondtest (resume-library.test.ts:128-138) is a nice real end-to-end addition and passes reliably on this branch (5/5), but revertingmonotonicNow()inputRecordViaonly catches it 2/3 runs — withDate.nowmocked constant, the fallback ordering becomes a coin flip oncrypto.randomUUID()comparison rather than a guaranteed fail. Not blocking sincestorage.test.ts's new test already gives a deterministic regression guard for the same class of bug; just don't lean on this one alone if it's ever the only coverage for a similar fix elsewhere.
AC checklist (#907)
- Root cause addressed in code —
monotonicNow()now backs both writers (putRecordViaandsoftDeleteRecord) after this round's push. - "deterministic, correct newest-first order... when two records share the same
savedAtmillisecond" — still not verified by any test (Blocking finding above; the tiebreak itself is real, just untested). - No regression to existing coverage — 145/145 across
resume-library.test.ts,storage.test.ts,letters.test.ts,job-tracker.test.ts,library-changes.test.ts.
Description accuracy (3f)
The description wasn't updated for this round's two new commits — it still only describes the original monotonicNow() + tiebreaker change. It omits that softDeleteRecord was also moved onto monotonicNow() and that two new tests were added to cover it. That's a real behavior change (fixes a second, distinct clock-consistency bug) shipping undocumented in the PR body a future reader would trust.
Gates
npm run typecheck— clean.npm run lint— clean.- Targeted suite (
resume-library.test.ts,storage.test.ts,letters.test.ts,job-tracker.test.ts,library-changes.test.ts) — 145/145 passing. npx fallow audit --base origin/main— 0 dead code, 0 complexity on the 4 changed files; 2 duplication groups, both excluded as inherited (not attributable to this round's diff).check:fixtures— n/a, no fixtures touched.- Design-system / style-token gates — n/a, no
src/components/**or Tailwind changes.
Reviewed by: Claude Sonnet 5 (high)
| it("breaks ties deterministically on same-millisecond savedAt", async () => { | ||
| const tiedSavedAt = 1_700_000_000_000; | ||
| await putRecord<ResumeRecord>( | ||
| "resumes", | ||
| { | ||
| id: "id-b", | ||
| filename: "b.pdf", | ||
| blob: new Blob([bytes()]), | ||
| parse: { result: result(), score: score(70), sourceKind: "pdf", shapeVersion: "1:1" }, | ||
| createdAt: tiedSavedAt, | ||
| updatedAt: tiedSavedAt, | ||
| }, | ||
| { touch: false }, | ||
| ); | ||
| await putRecord<ResumeRecord>( | ||
| "resumes", | ||
| { | ||
| id: "id-a", | ||
| filename: "a.pdf", | ||
| blob: new Blob([bytes()]), | ||
| parse: { result: result(), score: score(80), sourceKind: "pdf", shapeVersion: "1:1" }, | ||
| createdAt: tiedSavedAt, | ||
| updatedAt: tiedSavedAt, | ||
| }, | ||
| { touch: false }, | ||
| ); | ||
| const list = await listLibrary(); | ||
| expect(list).toHaveLength(2); | ||
| expect(list.map((e) => e.id)).toEqual(["id-a", "id-b"]); | ||
| }); |
There was a problem hiding this comment.
Blocking: this test still doesn't verify the tiebreaker. Reverted only a.id.localeCompare(b.id) in resume-library.ts and re-ran this test 5x — still passes every time.
Root cause: getAllResumes() → getAllRecords() → db.getAll(store) (crud.ts:244) has no index, so IndexedDB itself already returns tied records in ascending primary-key order. The test's ids ("id-a", "id-b") are already ascending, so Array.prototype.sort's stability alone reproduces ["id-a", "id-b"] with or without the explicit tiebreak — the test can't distinguish "the comparator works" from "IDB's own order happened to already match it."
Fix: stub getAllResumes/db.getAll to return the tied pair in descending id order, then assert listLibrary() still comes back ["id-a", "id-b"] — that actually exercises the comparator instead of agreeing with the store's incidental order.
…iebreaker independently of IDB (offlinecv#907)
|
Thank you @s-annam for the detailed re-review! Changes in this push (
|
s-annam
left a comment
s-annam
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Third round, at a6cfae9. Both prior rounds' findings are now genuinely fixed — verified each empirically, not just by reading:
Fixed (verified)
- Tiebreaker test now actually exercises the comparator.
breaks ties deterministically on same-millisecond savedAtnow stubsgetAllResumesto return the tied pair in descending primary-key order ([recordB, recordA]), so it no longer coincides with IndexedDB's own ascending-key return order. Confirmed: revertinga.id.localeCompare(b.id)inresume-library.tsback to plainb.savedAt - a.savedAtfails this test 5/5; restoring the tiebreak passes it 5/5. This closes AC1 of #907. softDeleteRecord/putRecordViaclock split (flagged round 1) — confirmed still fixed, both route throughmonotonicNow(), backed by a deterministic regression test instorage.test.ts.
Nit (non-blocking, unchanged from round 2)
preserves newest-first save order when saves occur in the same clock millisecondis a nice end-to-end addition but only caught a revertedmonotonicNow()2/3 runs in my testing — withDate.nowmocked constant, the no-fix fallback order becomes a coin flip oncrypto.randomUUID()comparison. Not a problem in practice since thestorage.test.tstest already gives deterministic coverage for that fix; just don't rely on this one alone elsewhere.
AC checklist (#907)
-
listLibrary()returns deterministic, correct newest-first order on asavedAttie — verified by a test that actually isolates the tiebreaker. - No scheduling-dependent test.
- No regression — 145/145 across
resume-library.test.ts,storage.test.ts,letters.test.ts,job-tracker.test.ts,library-changes.test.ts.
Description accuracy (3f)
Rewritten and now accurate — correctly describes both fixes, and specifically calls out that the tiebreak test "fails if tiebreaker is reverted," which I independently verified is true.
Gates
npm run typecheck— clean.npm run lint— clean.- Targeted suite — 145/145 passing.
npx fallow audit --base origin/main— 0 dead code, 0 complexity; 2 duplication groups, both excluded as inherited.check:fixtures/ design-system / style-token gates — n/a, no fixtures orsrc/components/**touched.
Reviewed by: Claude Sonnet 5 (high)
2 participants
Summary
Fixes #907.
Adds monotonic timestamp sequencing across both storage write paths (
putRecordViaandsoftDeleteRecordinsrc/lib/storage/crud.ts), and adds a deterministic fallback tiebreaker (a.id.localeCompare(b.id)) tolistLibrary()'s sort insrc/lib/resume-library.tswhen two records share the samesavedAtmillisecond timestamp.Root Cause
listLibrary()sorted records exclusively byb.savedAt - a.savedAt. When two records were saved in the same clock millisecond, the comparator returned0and fell back to the incidental return order of the underlying IndexedDB query.putRecordViaandsoftDeleteRecordpreviously used rawDate.now(), which allowed back-to-back writes in rapid succession or under event loop contention to share the exact same timestamp, or permitted a soft-delete tombstone to drift behind writes stamped bymonotonicNow().Changes
src/lib/storage/crud.ts:monotonicNow()module-global counter:const now = Date.now(); lastTimestamp = now > lastTimestamp ? now : lastTimestamp + 1; return lastTimestamp;.putRecordVia(upsert) andsoftDeleteRecord(tombstone) throughmonotonicNow()so all record mutations have strictly increasingupdatedAttimestamps across the module.src/lib/resume-library.ts:.sort((a, b) => b.savedAt - a.savedAt || a.id.localeCompare(b.id)).src/lib/resume-library.test.ts:breaks ties deterministically on same-millisecond savedAtby stubbinggetAllResumesto return tied records in descending primary-key order ([recordB, recordA]), proving thatlistLibrary()'s explicit tiebreaker overrides the store's return order and sorts deterministically to["id-a", "id-b"](fails if tiebreaker is reverted).preserves newest-first save order when saves occur in the same clock millisecondend-to-end integration test.src/lib/storage/storage.test.ts:softDeleteRecord: tombstone updatedAt advances monotonically past preceding putRecord writesverifying tombstones never drift behind preceding writes.Verification
npm run typecheck&npm run lintclean.resume-library.test.ts,storage.test.ts,letters.test.ts,library-changes.test.ts) passing.