Skip to content

Commit d1e898e

Browse files
committed
Order a review's timeline by attempt, because ULIDs are a coin flip
CI failed on a test that passes here: the checkpointing runner's record of a call that answered and then died. Chasing it found a real ordering bug rather than a flaky test. Every row a single call writes shares one startedAt, and the timeline was ordered by (startedAt, id) with ULID ids as the tiebreak. ULIDs are not monotonic inside a millisecond. Measured over twenty thousand pairs, the second of two generated in the same millisecond sorts before the first 9,594 times out of 19,198, so one call's rows came back in a random order and the assertion about which row held the usage was a coin flip. Ordering is by attempt within a timestamp now, which is what the sequence actually is. A test pins it, and reproducing the old ordering fails that test in eight runs out of twelve, which is the honest shape of the bug: frequent enough that CI caught it, rare enough that twelve local runs of the original test did not. Worth recording rather than just fixing, because the same trap applies anywhere a ULID is used to break a tie inside one timestamp.
1 parent 63f1de3 commit d1e898e

3 files changed

Lines changed: 44 additions & 2 deletions

File tree

docs/DECISIONS.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -778,3 +778,12 @@ verified evidence, in writing, here.
778778
- 2026-07-31 DECIDED (V7, D-44): the new-review screen links to the ruleset
779779
page rather than opening a drawer. At one screen of this size a drawer
780780
duplicates a page that already exists.
781+
- 2026-07-31 FIXED: the stage timeline was ordered by (startedAt, id), and
782+
every row a single call writes shares one startedAt. ULIDs are not monotonic
783+
within a millisecond: measured over twenty thousand pairs, the second of two
784+
generated in the same millisecond sorts before the first 9,594 times out of
785+
19,198, so one call's rows came back in a random order. It surfaced as a test
786+
that passed locally and failed in CI. Ordering is now by attempt within a
787+
timestamp, which is the real sequence, and a test reproducing the old
788+
ordering fails it about two runs in three. Recorded because the same trap
789+
applies anywhere else ULIDs are used as a tiebreak within one timestamp.

src/server/db/repositories/stage-executions.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,13 +125,23 @@ export function latestChainedSession(db: Db, reviewId: string): string | undefin
125125
);
126126
}
127127

128-
/** Everything this review has asked, oldest first, for the timeline. */
128+
/**
129+
* Everything this review has asked, oldest first, for the timeline.
130+
*
131+
* Ordered by attempt within a timestamp, not by id. Every row a single call
132+
* writes shares one startedAt, and ULIDs are not monotonic inside a
133+
* millisecond: measured over twenty thousand pairs, the second of two ids
134+
* generated in the same millisecond sorts before the first almost exactly half
135+
* the time. Ordering by id therefore returned one call's rows in a random
136+
* order, which showed up as a test that passed here and failed in CI. Attempt
137+
* numbers are the real sequence, so they are what the sequence is read from.
138+
*/
129139
export function listForReview(db: Db, reviewId: string): StageExecution[] {
130140
return db
131141
.select()
132142
.from(stageExecutions)
133143
.where(eq(stageExecutions.reviewId, reviewId))
134-
.orderBy(asc(stageExecutions.startedAt), asc(stageExecutions.id))
144+
.orderBy(asc(stageExecutions.startedAt), asc(stageExecutions.attempt), asc(stageExecutions.id))
135145
.all();
136146
}
137147

tests/server/db/stage-executions.test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -274,3 +274,26 @@ describe("the record of what a review spent", () => {
274274
expect(listForReview(db, reviewId)).toEqual([]);
275275
});
276276
});
277+
278+
describe("the order a timeline reads in", () => {
279+
it("keeps one call's rows in attempt order, whatever their ids sort like", () => {
280+
// Every row a single call writes shares one startedAt, and ULIDs are not
281+
// monotonic inside a millisecond: the second of two generated in the same
282+
// millisecond sorts before the first about half the time. Ordering by id
283+
// returned them at random, which passed locally and failed in CI.
284+
const startedAt = "2026-07-31T10:00:00.000Z";
285+
for (const attempt of [1, 2, 3]) {
286+
recordAttempt(db, {
287+
reviewId,
288+
stage: "s1_risk",
289+
promptHash: "same-question",
290+
attempt,
291+
status: attempt === 3 ? "failed" : "succeeded",
292+
outputJson: attempt === 3 ? null : "{}",
293+
startedAt,
294+
});
295+
}
296+
297+
expect(listForReview(db, reviewId).map((row) => row.attempt)).toEqual([1, 2, 3]);
298+
});
299+
});

0 commit comments

Comments
 (0)