Skip to content

fix(core): preserve audit append order for timestamp ties - #2803

Open
juanbermudez wants to merge 1 commit into
BuilderIO:mainfrom
juanbermudez:codex/audit-append-order
Open

fix(core): preserve audit append order for timestamp ties#2803
juanbermudez wants to merge 1 commit into
BuilderIO:mainfrom
juanbermudez:codex/audit-append-order

Conversation

@juanbermudez

Copy link
Copy Markdown

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/core and makes no public API changes.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@github-actions

Copy link
Copy Markdown
Contributor

Visual recap — skipped

The visual recap job did not run for this pull request. This is informational only and does not block the PR.

Recap skipped for 851c6de: external fork PR requires a maintainer to apply the recap label to the current head SHA.

@builder-io-integration builder-io-integration Bot left a comment

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.

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 TABLE or 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);

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.

🔴 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.

Fix in Builder

Comment on lines +235 to +237
if (client.transaction) {
await client.transaction(initialize);
} else {

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.

🟡 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.

Fix in Builder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant