Skip to content

fix(resume-library): add deterministic tiebreaker for same-millisecond saves #908

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
s-annam merged 4 commits into from
Aug 27, 2026
Merged

fix(resume-library): add deterministic tiebreaker for same-millisecond saves #908

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
24f4b44
fix(resume-library): add deterministic tiebreaker for same-millisecon…
shubhransh-gupta Aug 27, 2026
811a39f
fix(storage): route softDeleteRecord through monotonicNow and add for…
shubhransh-gupta Aug 27, 2026
b0812a9
test(storage): add regression test for softDeleteRecord monotonic tim…
shubhransh-gupta Aug 27, 2026
a6cfae9
test(resume-library): stub getAllResumes in reverse order to verify t…
shubhransh-gupta Aug 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/lib/resume-library.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,14 @@ describe("resume-library: save + list", () => {
expect(list.map((e) => e.filename)).toEqual(["tailored.pdf", "general.pdf"]);
expect(list[0]).toMatchObject({ scoreOverall: 84, sourceKind: "pdf", hasCachedParse: true });
});

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);
});

@s-annam s-annam Aug 27, 2026

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.

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.

});

describe("resume-library: load", () => {
Expand Down
2 changes: 1 addition & 1 deletion src/lib/resume-library.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ export async function listLibrary(): Promise<ResumeLibraryEntry[]> {
hasCachedParse: snap !== null,
};
})
.sort((a, b) => b.savedAt - a.savedAt);
.sort((a, b) => b.savedAt - a.savedAt || a.id.localeCompare(b.id));
}

/**
Expand Down
14 changes: 13 additions & 1 deletion src/lib/storage/crud.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,18 @@ export async function putRecordIntoExisting<T extends StoredRecord>(
return putRecordVia(getExistingDB, store, record, options);
}

let lastTimestamp = 0;

function monotonicNow(): number {
const now = Date.now();
if (now <= lastTimestamp) {
lastTimestamp += 1;
return lastTimestamp;
}
lastTimestamp = now;
return now;
}

async function putRecordVia<T extends StoredRecord>(
opener: () => Promise<IDBPDatabase<any>>,
store: StoreName,
Expand All @@ -177,7 +189,7 @@ async function putRecordVia<T extends StoredRecord>(
options: { touch?: boolean } = {},
): Promise<T> {
const db = await looseDB(opener);
const now = Date.now();
const now = monotonicNow();

@s-annam s-annam Aug 27, 2026

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.

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.

const existing = (await db.get(store, record.id)) as T | undefined;
const written = {
...record,
Expand Down
Loading