Walk the audit trail without skipping rows written in the same millisecond - #586
Merged
davidmckayv merged 2 commits intoSep 16, 2026
Merged
Conversation
…ame millisecond are not skipped created_at keeps microseconds and a JavaScript Date keeps milliseconds, so the cursor built from a row named a moment just before that row. The rows the next page should start with, written earlier in the same millisecond or at the same instant, then compared as newer than the cursor and were on no page at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kevin9327
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso,
mxmzb and
tylerslaton
as code owners
September 16, 2026 21:36
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
davidmckayv
approved these changes
Sep 16, 2026
davidmckayv
left a comment
Contributor
There was a problem hiding this comment.
Code-verified clean; CI green on this sha.
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.
What this changes
GET /api/admin/audit-eventspages on(created_at, id), and itsnextCursorcarries the last row'screatedAt. That value came from the row as the driver returns it, a JavaScriptDate, which keeps milliseconds.audit_events.created_atis a plaintimestamptzdefaulted tonow(), which keeps microseconds.So the cursor names a moment slightly before the row it was taken from. The next page asks for
created_at < cursor OR (created_at = cursor AND id < cursor.id). A row written earlier in the same millisecond, or at the same instant (one statement or one transaction sharesnow()), is newer than the truncated cursor. It fails both halves and lands on no page. The walk carries on or ends with nothing to show a row was missed.The schema already expects this case. The comment on the audit indexes says "two rows written in the same millisecond are ordered by id", and the cursor is meant to handle that. It could not, because both rows compare as newer than the cursor.
The fix, in
createAuditReader.list:created_atformatted to the microsecond in UTC (to_char(... 'YYYY-MM-DD"T"HH24:MI:SS.US"Z"')), andnextCursorcarries that string. The field is removed before the events are returned, so the response body is unchanged.::timestamptzrather than passed throughnew Date(...), which would truncate it again.A cursor issued before this change still decodes and still pages as it did.
decodeCursoris unchanged: the microsecond ISO string passes itsDate.parsecheck. Ordering, filters, the limit and the response body are unchanged.The channel roster cursor (
channels/routes.ts) has the same shape, but its timestamps rarely share a millisecond:last_message_atis written from a JavaScriptDate, andcreated_atonly decides for a channel with no messages. I left it alone to keep this change to the audit trail.Where it runs
Boundary and audit
Changelog
CHANGELOG.mdunderUnreleased.Proof
Two cases added to
server/tests/audit-cursor.integration.test.ts. Each writes three rows in oneINSERT, so they sharenow(), atdate_trunc('milliseconds', now())plus a few microseconds. It then walkscreateAuditReader(database).listwithlimit: 1, followingnextCursor. The first case puts the rows microseconds apart. The second puts them at one instant, so only the id orders them.I have no local PostgreSQL on this machine, so this ran on GitHub Actions in my fork. The job used the same
pgvector/pgvector:pg17service,bun install --frozen-lockfile,drizzle-kit migrateand Bun 1.3.14 asci.yml.With
server/src/audit.tsfrommain(only the test added):The walk returned the first row and a cursor, and the next page was empty.
With the fix:
bun test --coverage server/tests/audit-cursor.integration.test.ts: 5 pass, 0 fail. Every changed line inaudit.tsexecutes. The uncovered lines the report lists are all outside this change.bun testonaudit-cursor.test.ts,audit.test.ts,audit-limit.test.ts,audit-initiator.integration.test.ts,audit-retention.integration.test.ts,computer-policy-dry-run-limit.test.tsandpolicy-dry-run.test.ts: 83 pass, 0 fail.bun run typecheck(app, server, worker) exits 0,bunx biome checkon the changed files is clean, and the three non-database audit test files pass 30/30.This PR adds its entry at the top of
## Unreleased, where other open PRs add theirs, so the changelog may conflict. I can rebase if it does.🤖 Generated with Claude Code