fix(core): preserve audit append order for timestamp ties - #2803
fix(core): preserve audit append order for timestamp ties#2803juanbermudez wants to merge 1 commit into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Visual recap — skippedThe visual recap job did not run for this pull request. This is informational only and does not block the PR. Recap skipped for |
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🔴
Review Details
Code Review Summary
PR #2803 adds a database-owned append sequence to audit records, extends ordering to (created_at, append_order), and backfills legacy SQLite/PostgreSQL tables. The pagination assertions and provider-specific coverage are useful, and the append allocator avoids exposing a new public API. I assessed this as standard risk because it changes shared persistence and request-time schema behavior.
Key Findings
- 🔴 HIGH: Existing PostgreSQL deployments cannot safely adopt the new schema from the request path. The initializer now requires the new column, allocator table, trigger, and indexes, but the PR does not add an authorized release migration. In production serverless, schema mutation is disabled, so legacy installations can reject every audit read/write during initialization.
- 🟡 MEDIUM: SQLite/D1 initialization is not atomic when the client has no transaction method. Concurrent cold starts can interleave the column check, backfill, trigger, and index creation; a duplicate
ALTER TABLEor partially upgraded schema can make audit operations fail, and an insert in the trigger-install window can retain a null append order.
The local SQLite audit suite passes (13 tests). PostgreSQL integration coverage is gated on a local test URL and was not available here.
🧪 Browser testing: Skipped — PR only modifies backend/database code, tests, and a changeset; no UI impact.
| "idx_audit_created", | ||
| `CREATE INDEX IF NOT EXISTS idx_audit_created ON agent_audit_log (created_at)`, | ||
| ); | ||
| await ensurePostgresAppendOrder(client); |
There was a problem hiding this comment.
🔴 Provide an authorized migration for existing PostgreSQL schemas
ensurePostgresAppendOrder() is now the only path that adds/backfills append_order and creates the allocator table, trigger, and indexes, but this PR does not add a release migration for those objects. In production serverless, schema mutation is disabled for request-time initialization, so a deployed legacy agent_audit_log causes ensureAuditTables() to reject and breaks both audit reads and writes; move this additive backfill/schema change into the authorized migration path before making the new column part of the query.
Additional Info
Reported by 2 of 3 parallel reviewers.
| if (client.transaction) { | ||
| await client.transaction(initialize); | ||
| } else { |
There was a problem hiding this comment.
🟡 Make SQLite append-order initialization safe for concurrent D1 cold starts
When client.transaction is unavailable, the full upgrade runs as independent statements. Concurrent D1/libSQL initializers can both observe the legacy schema, race on ALTER TABLE, or allow an old writer to insert between the backfill and trigger creation, leaving append_order null and breaking the ordering guarantee. Use the client’s atomic batch/transaction mechanism or make each migration step race-safe and re-probe before proceeding.
Additional Info
Reported by 2 of 3 parallel reviewers.
Audit pagination can skip or duplicate rows when multiple events share the same millisecond timestamp.
This adds database-owned append ordering for SQLite and PostgreSQL, with equal-timestamp pagination and legacy-schema coverage on both providers. It includes a patch changeset for
@agent-native/coreand makes no public API changes.