Skip to content

plugin-audit's read-audit rows back-date created_at to the VIEW instant through an isSystem reliance the audit hook never honoured, so every batched sys_audit_log read row now gets the FLUSH instant #16829

Description

@os-musk

Found while re-running the eight-row created_at enumeration the #16312 dispatch asked its dev to re-run rather than inherit. ⭐ The enumeration on #16312 does not list this file at all, and this row is RELIANT — a second one, in a different package, with a different reliance.

The site

packages/plugins/plugin-audit/src/read-audit.ts:512 writes the view instant on purpose, and its own comment states the invariant it is defending:

const buildRow = (event: ReadAuditEvent): Record<string, unknown> => {
  const row: Record<string, unknown> = {
    action: READ_AUDIT_ACTION,
    // ⛔ The VIEW instant, not the flush instant. Batching moves the INSERT
    // off the request path by design, so `created_at`'s `NOW()` default would
    // stamp every row in a batch with one flush timestamp up to
    // `flushIntervalMs` after the fact — a ledger that answers "when did they
    // look?" with the time its own buffer drained. `created_at` is
    // engine-owned and stripped from ordinary writes (#4447), and a
    // system-context write is the declared exemption (pinned by
    // `engine-audit-anchor-write.test.ts`: "a system-context write is still
    // exempt"), which is exactly the context `persistReadAuditRows` uses.
    created_at: event.viewedAt,

and persistReadAuditRows (read-audit.ts:456-460) writes exactly that context and no other:

await engine.insert('sys_audit_log', rows as any, { context: { isSystem: true } } as any);

Why the reliance does not hold

isSystem exempts a write from the readonly strip. It has never been consulted by the audit stamp hook, which is the layer that decides created_at on an insert. sys_stamp_audit_insert (packages/objectql/src/plugin.ts, the applyToRecord body reached from the beforeInsert builtin) reads one flag and it is not this one:

const preserveAudit = session?.preserveAudit === true;
if (isInsert) {
  record.created_at = preserveAudit ? (record.created_at ?? now) : now;
}

Before the #15964 change that line was record.created_at = record.created_at ?? now — client-preferred on every insert, with no flag and no privilege required. That accident, not isSystem, is what was actually carrying event.viewedAt through. With the ternary in place the ordinary branch stamps now, and now here is the flush instant — precisely the outcome the comment above says the field exists to prevent.

the same defect class as #16312, in a different package and through a different (and differently mis-stated) reliance.

The cited pin does not cover this path

packages/objectql/src/engine-audit-anchor-write.test.ts:216, a system-context write is still exempt, is an engine.update:

await engine.update(
  'audit_task',
  { created_at: FORGED },
  { where: { id: row.id }, context: { isSystem: true } } as any,
);

sys_stamp_audit_update never writes created_at in any branch — it is guarded by if (isInsert) — so that case is green whatever the insert path does. The comment in read-audit.ts cites an update-path pin as authority for an insert-path reliance, and the insert path it actually uses has no pin at all.

Blast radius, and why it is not urgent-in-production yet

Measured on this tree: objectql's half of #15964 is still an unreleased changeset (.changeset/audit-binder-created-at-unconditional-on-create.md); packages/objectql/CHANGELOG.md at 17.3.0 carries no entry for it. So the flattening is live on main and in any build cut from it, and has not shipped in a published version. Landing a release with the objectql change and without a repair here starts stamping the read-audit ledger with buffer-drain times.

What is at stake is an audit trail: sys_audit_log's record_views rows answer "when did this user look at this record?". Batched behind flushIntervalMs, every row in a batch collapses to one timestamp, and read-order within the window is destroyed. Grading is the triage seat's — noting only that this is the ledger, not a display field.

The remedy, and the shape acceptance needs

One context key on the write, the same one #16312 takes:

await engine.insert('sys_audit_log', rows as any, { context: { isSystem: true, preserveAudit: true } } as any);

preserveAudit is the ruled historical-import channel (#3493, reaffirmed by #15964's ruling of 2026-09-06) — the door audit left open for reinstating an original timeline, not a bypass of it. Both keys are wanted: isSystem still carries the readonly-strip exemption the row needs, preserveAudit is what the stamp hook reads.

⚠️ The test is the substantive half here too. Anything that drives this through a double which runs no hooks will pass on both sides of the change, exactly as migrate-sys-notification-to-event.test.ts did for #16312 — that suite read 23 passed while the rows were being restamped. Acceptance should require a case that drives the real sys_stamp_audit_insert hook and is RED before the fix. #16312's PR adds one such harness (packages/runtime/src/notification-migration-audit-preservation.integration.test.ts, a real ObjectKernel + ObjectQLPlugin + SqliteWasmDriver) that can be copied in shape.

While repairing it, the comment quoted at the top needs correcting as part of the same change: it names the wrong mechanism, and leaving it would leave the next reader the same false premise.

Related, and not the same

Filed by the #16312 dev under the out-of-scope finding rule; ⛔ not repaired in that PR, which is fenced to packages/metadata/src/migrations/migrate-sys-notification-to-event.ts and its tests. Untriaged and unclaimed.

Activity

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

Metadata

Metadata

Assignees

Labels

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions