[API] Audit hash-chain forks under concurrent writes — verify_chain false positives on busy instances

[ToddHebebrand]

Description

On busy instances, audit_log_verify_chain() reports breaks even with zero tampering. Surfaced on the US droplet immediately after the v0.68.2 deploy activated the audit hash chain for the first time on a high-traffic instance: ~20 false breaks accumulated within the first few minutes, all from live agent telemetry rows (agent.sessions.submit, agent.security_status.submit, agent.patches.submit, …), and the count grows continuously under load. The 89k historical (backfilled) rows verify clean — only concurrently-inserted live rows break.

This is a pre-existing design bug in the chain (PR #900), not a regression from the convert_to bytea fix (#994). The deploy merely ran the chain at write-concurrency for the first time. EU (low traffic, ~5k rows) shows 0 breaks but has the same latent bug.

No availability or data impact: every audit row is written and individually checksummed correctly; only the chain linkage forks, which degrades the tamper-detection signal (false positives make real tampering indistinguishable).

Root Cause

The BEFORE INSERT trigger audit_log_compute_checksum() selects its predecessor with no serialization:

SELECT checksum INTO prev
FROM audit_logs
WHERE org_id IS NOT DISTINCT FROM NEW.org_id
  AND id <> NEW.id
ORDER BY timestamp DESC, id DESC
LIMIT 1;

When N transactions insert audit rows for the same org concurrently, each reads the same latest committed checksum as prev and chains off it → the chain forks. Verified on US: 6 distinct prev_checksum values were each used by 2–5 rows (a clean serial chain uses each exactly once), all within one 4-minute window of concurrent agent telemetry. audit_log_verify_chain walks timestamp, id order and flags every forked row as a break.

The same-timestamp variant of this was already noted as known future-work in the test suite:
apps/api/src/__tests__/integration/audit-checksum.integration.test.ts:110-111 — "requires a chain_seq bigserial + per-org advisory lock — tracked as future-task hardening." In practice it manifests under plain write concurrency, not just same-transaction batches.

Proposed Fix

1. Serialize per-org inserts in the trigger with a transaction-scoped advisory lock before the predecessor SELECT, so concurrent same-org inserts each see the prior committed checksum:

   PERFORM pg_advisory_xact_lock(<namespace_const>, hashtext(COALESCE(NEW.org_id::text, 'NULL')));

   (or the chain_seq bigserial approach). New migration that CREATE OR REPLACEs the trigger function.
2. Heal migration to re-anchor already-forked rows (re-run the per-org backfill in timestamp, id order). Must deploy the lock fix first, otherwise new forks keep appearing during/after the heal.
3. Concurrency regression test (TDD): insert many same-org audit rows concurrently, assert verify_chain returns 0 breaks.

Design tension to weigh

A per-org hash chain inherently serializes that org's audit writes. For high-frequency agent telemetry (the rows that forked here), a per-org advisory lock could become a write-throughput bottleneck. Worth deciding whether every telemetry event belongs in the tamper-evident chain, or only security-relevant events (auth, RBAC, config, retention), with high-volume telemetry excluded.

Affected Files

• apps/api/migrations/2026-05-25-c-audit-log-checksum-canonical-fix.sql:38 — canonical audit_log_compute_checksum() trigger (predecessor SELECT, no lock)
• apps/api/migrations/2026-05-25-b-audit-log-checksum-chain.sql:25 — original definition (same issue)
• apps/api/src/__tests__/integration/audit-checksum.integration.test.ts:100-111 — existing note documenting the limitation
• New migrations: per-org advisory-lock trigger fix + forked-row heal

Reported By

Discovered during the v0.68.2 production deploy (2026-05-29). Both EU and US are on v0.68.2 and healthy; US's verify_chain accumulates false breaks under load until the lock fix ships.

[ToddHebebrand]

Heads-up for whoever picks this up: the obvious fix (per-org advisory lock in the trigger) does NOT work — it deadlocks.

Tried it in draft PR #1240 (advisory lock held to commit + a timestamp-monotonicity nudge + heal migration). The unit concurrency test passed, but it deadlocks deterministically against audit-logs-rls.integration.test.ts > "transaction isolation: audit row persists even when caller request tx rolls back".

Why: the lock is held to commit, and audit rows are written on two connections in one flow — (a) the caller's own audit insert inside its request tx, and (b) logSessionAudit (routes/remote/helpers.ts), which escapes onto a separate pooled connection. Same org → (a) holds the lock for the whole caller tx, (b) blocks on it, and the caller tx is awaiting (b). It's a JS-level deadlock Postgres can't detect — it would hang real request flows, not just the test. Any held-to-commit per-org serialization (advisory lock, SELECT … FOR UPDATE on a head row) has the same hang.

Direction that does work: decouple chain computation from the insert path — chain_seq bigserial + content hash at insert (no lock), then a single-threaded sealer computes the linear chain out-of-band. A fork-tolerant verifier is not a substitute (a forked DAG has many leaf rows, and leaves delete without detection). Will spec the sealer separately.

[ToddHebebrand]

Redesigned fix is up: PR #1247 (supersedes draft #1240).

Approach: deferred commit-time sealing — audit inserts compute a content-only hash (no lock, no predecessor read); a DEFERRABLE INITIALLY DEFERRED constraint trigger seals each row into an append-only audit_log_chain side table at COMMIT under a per-org advisory lock held only through commit processing. The held-to-commit deadlock documented above is structurally impossible (the rollback-isolation test that hung #1240 at 30 s passes in ~0.6 s), the 25-way concurrency test returns 0 breaks, and same-transaction batches — the other documented limitation — now chain correctly too. Retention switches to chain-order prefix-cut deletes with no re-anchor, so both audit_logs and the chain are strictly append-only. Design spec on the branch: docs/superpowers/specs/2026-06-11-audit-chain-deferred-sealing-design.md.