fix(db): add a rowid tiebreak to signal_snapshots' latest-row reads - #4665
Merged
Conversation
listSignalSnapshots ordered by generatedAt (millisecond precision) alone,
and listLatestSignalSnapshotsForTargets used a secondary `id DESC`
tiebreak -- id is a crypto.randomUUID(), with no relationship to
insertion order. Two writes for the same (signalType, targetKey) within
one millisecond (a realistic case: an API-record write immediately after
a seeded/prior one) tie on generatedAt, and SQLite's own docs make no
guarantee about tie order ("the order ... is undefined",
sqlite.org/lang_select.html) -- so which row an unbroken ORDER BY tie
returns is an accident of the current query plan, not a contract, and can
change as the table grows or the engine version changes. Verified
Cloudflare D1 (GA) currently routes every read for a database to a single
active instance, so there's no live multi-replica divergence risk today --
but Cloudflare's own roadmap (a Sessions API with read replicas) would
reintroduce exactly that risk, and SQLite's "insertion order" appearance
is documented as incidental, not guaranteed, independent of replicas.
Fixes both reads to use rowid (SQLite's actual monotonic per-insert
counter) instead, matching this table's own documented invariant
elsewhere in the codebase (retention.ts's dedupeSignalSnapshots: "'Latest'
is the highest rowid per key ... rowid, unlike generated_at, can never
tie") and the same pattern already used for orb_enrollments' own
most-recently-inserted read (orb/relay.ts).
A regression test with deliberately adversarial ids (chosen to sort
alphabetically BACKWARDS from insertion order) confirms
listLatestSignalSnapshotsForTargets' prior `id DESC` tiebreak was a real,
reproducible bug -- reverting the fix makes that specific test fail. The
listSignalSnapshots side couldn't be reproduced as a concrete bug against
the local D1 test shim (its own untiebroken fallback happens to already
match insertion order there), so its tests instead pin the fix's
mechanism (genuine rowid-based order, not an id string comparison) as
protection against a future regression, rather than proving today's local
behavior is wrong.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4665 +/- ##
=======================================
Coverage 94.13% 94.13%
=======================================
Files 434 434
Lines 38554 38554
Branches 14053 14053
=======================================
Hits 36291 36291
Misses 1604 1604
Partials 659 659
🚀 New features to boost your workflow:
|
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.
Summary
listSignalSnapshotsordered bygeneratedAt(millisecond precision) alone, andlistLatestSignalSnapshotsForTargetsused a secondaryid DESCtiebreak —idis acrypto.randomUUID(), with no relationship to insertion order. Two writes for the same(signalType, targetKey)within one millisecond (a realistic case: an API-record write immediately after a seeded/prior one) tie ongeneratedAt.rowid(SQLite's actual monotonic per-insert counter) instead, matching this table's own documented invariant elsewhere in the codebase (retention.ts'sdedupeSignalSnapshots: "'Latest' is the highest rowid per key ... rowid, unlike generated_at, can never tie") and the same pattern already used fororb_enrollments' own most-recently-inserted read (orb/relay.ts).listLatestSignalSnapshotsForTargets' priorid DESCtiebreak was a real, reproducible bug — reverting the fix makes that specific test fail. ThelistSignalSnapshotsside couldn't be reproduced as a concrete bug against the local D1 test shim (its own untiebroken fallback happens to already match insertion order there), so its tests instead pin the fix's mechanism (genuine rowid-based order, not an id string comparison) as protection against a future regression.Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally — neither change introduces a new JS-level branch (therowidadditions are a plain function-chain argument and a raw-SQL-string edit, not a conditional), confirmed via precise lcov line/branch cross-referencing against the diff; both changed statements are covered by thousands of pre-existing calls plus the new regression testsnpm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderate— 0 vulnerabilitieslistSignalSnapshotsandlistLatestSignalSnapshotsForTargets, a 3-row insertion-order regression, and a "genuinely later generatedAt still wins" sanity check. Bug-injection verified: temporarily reverted the fix and confirmed thelistLatestSignalSnapshotsForTargetstest fails for the expected reason (picks the wrong "latest" row) before restoring it.Safety
UI Evidencesection. — N/A, backend-only change, no visible UI.