Skip to content

fix: multi-process-safe audit integrity chain via atomic per-org append#35

Open
rares04 wants to merge 5 commits into
mainfrom
fix/atomic-audit-integrity-sequence
Open

fix: multi-process-safe audit integrity chain via atomic per-org append#35
rares04 wants to merge 5 commits into
mainfrom
fix/atomic-audit-integrity-sequence

Conversation

@rares04

@rares04 rares04 commented Jul 15, 2026

Copy link
Copy Markdown

Problem

The tamper-evident integrity audit (createGovernance({ integrityAudit })) held the chain's sequence and previousHash as process-local state, resumed from the durable head only once at boot (loadChainHead, guarded by state.loaded), then advanced an in-process counter under an in-process lock.

Under any multi-process deployment — Kubernetes replicas, a pm2 cluster, or serverless instances all writing to one shared database — that assumption breaks:

  • Collision → silent drop. Two processes derive the same sequence for the same org. One INSERT wins; the other loses to the UNIQUE (COALESCE(organization_id,''), integrity_sequence) index (Postgres 23505). With the default onFailure: 'allow', that governance decision's tamper-evident record is silently dropped.
  • Fork (even without a collision). The per-process previousHash links to that process's last write, not the true durable head, so the hash chain forks across processes and isn't sound under multi-process regardless of collisions.

storage-postgres.ts createAuditEventWithIntegrity() was a single INSERT with the pre-computed sequence — no ON CONFLICT, no retry, no head re-read.

Fix

Sequence allocation and previous-hash derivation are now atomic against the database, per org, on every write — never from process-local state.

  • New optional GovernanceStorage.appendToAuditChain(event, computeIntegrity) — the documented storage contract for atomic chain appends. The adapter, under a per-org lock spanning the whole operation, reads the org's durable head, calls computeIntegrity(head) (the HMAC signing key stays in the SDK core, never in storage), and persists event + integrity as one indivisible write.
  • Postgres: BEGIN ISOLATION LEVEL READ COMMITTEDpg_advisory_xact_lock(<classid>, hashtext(org)) → head SELECTINSERTCOMMIT. The advisory lock (released at COMMIT) serialises each org's chain against itself across all writers, including separate processes; unrelated orgs proceed in parallel. READ COMMITTED is pinned deliberately: it's what guarantees the post-lock head SELECT sees the prior writer's just-committed row (a fixed-snapshot isolation level would re-read stale and re-collide). The two-arg lock form namespaces the key under a fixed classid so other advisory-lock users of the same database can't contend. A pool exposing only query() (no transaction handle) falls back to a bounded read-head → insert → retry-on-23505 loop with jittered backoff, which is also multi-writer-safe.
  • Memory adapter: a per-org async lock spanning head-read → compute → write. Correct within a single process; memory is not shared across processes, so it is single-process by design (documented).
  • createGovernance() prefers appendToAuditChain when present; the previous createAuditEventWithIntegrity + process-local sequence path remains only as a single-writer fallback for third-party adapters that predate this method.

Why advisory lock over ON CONFLICT-retry

Retry alone doesn't fix the head-read race or the chain fork: each retry still needs to re-read the durable head and re-hash from it, so a naive retry that reuses process-local previousHash produces a forked chain. The advisory lock makes head-read → hash → insert indivisible in one pass. The retry-on-23505 path is kept only as the fallback for pools without connect() — there it does re-read the head each attempt, so it's correct; the bound is deterministic, not probabilistic (every 23505 means a competitor committed the derived sequence, so a writer loses at most once per concurrent same-org contender).

Verification order = sequence, not wall clock

createdAt is stamped before the append lock allocates the sequence, so under concurrent writers a lower sequence can carry a later timestamp (lock-wait inversion, cross-process clock skew). verifyAuditIntegrity() and integrityChain.export() now order by the HMAC-covered sequence (createdAt only tiebreaks legacy forked rows) — the old createdAt-first sort could report a valid multi-writer chain as tampered. Tamper-safe: the sequence is inside the signed hash, so forging it still fails the hash / previous-hash checks.

API surface unchanged

Existing consumers need no code change beyond upgrading — createGovernance({ integrityAudit }) picks up the safe path automatically once its storage adapter (a real pg.Pool) exposes connect(). Canonical hash form, per-org scoping, verify(), export(), and stats() behaviour are unchanged.

One behavioural note for reviewers: the transactional path now holds a pooled connection across BEGIN → lock-wait → SELECT → hash → INSERT → COMMIT (the old path was grab/insert/release per statement). A same-instant burst across many orgs would queue on the pool and degrade to connectionTimeout / statement_timeout → the same onAuditError path (not a silent drop).

Multi-process guidance + docs (this PR)

The fix is generalised for the SDK's whole audience, not only the deployment that surfaced it:

  • README gains a "Multi-process deployments" section: what appendToAuditChain guarantees, a per-adapter support table (Postgres / memory / third-party fallback), and how a custom storage adapter should implement atomic append (row lock, advisory lock, serializable txn, or CAS-retry on its uniqueness constraint).
  • The standalone createIntegrityAudit() wrapper (audit-integrity.ts) is the other entry point that keeps its chain in process memory — the same unsoundness class on a separate surface. It holds no storage handle to append against, so it cannot reuse the contract; it is now documented as single-process only (prominent JSDoc + README + a regression test asserting a second wrapper starts its own sequence at 1 rather than resuming the durable head), with consumers pointed at createGovernance({ integrityAudit }) for durable multi-process audit.
  • integrityChain.stats() still reports this process's last-written sequence/hash (a process-local cache); export() + verifyAuditIntegrity() are the authoritative durable readers. A DB-backed stats() is deferred — it would turn a sync method async (a breaking signature change) — and noted in the CHANGELOG.

Tests

npm test1448 pass, tsc --noEmit clean.

  • audit-chain-multiwriter.test.ts — the load-bearing regression test: two createGovernance instances sharing one storage (two-writer model) fire N concurrent interleaved writes → exactly N rows, contiguous 1..N sequences (no duplicates/gaps), and the interleaved chain verifies standalone. Also covers two writers × two orgs, and a timestamp-inverted-vs-sequence chain (fails on the old createdAt sort).
  • audit-chain-append-postgres.test.ts — asserts the Postgres SQL shape (BEGIN ISOLATION → advisory-lock → head SELECTINSERTCOMMIT), DB-head-derived sequencing across sequential appends, an orchestration run of two instances over one pool, retry-fallback recovery from a 23505, and dead-connection release on ROLLBACK failure.
  • audit-integrity.test.ts — new: documents the createIntegrityAudit() single-process boundary.

Verified against a real Postgres (not just the mock)

The committed pg test models pg_advisory_xact_lock as an in-process mutex (the repo harness has no Postgres), so it validates orchestration + SQL shape but can't prove real cross-connection locking. The two-writer scenario was additionally run out-of-band against an ephemeral real Postgres 14 with two independent pg.Pools (real connect(), real advisory lock, real unique index), 60 concurrent writes to one org:

Path Attempted Persisted Dropped (23505) Chain
Before this PR (legacy path) 60 30 30 forked
After this PR 60 60 0 contiguous 1..60, valid

The "before" row reproduces the failure mode; the "after" row demonstrates the fix on real cross-connection locks. (Throwaway harness — not committed.)

Rollout note

The advisory lock only protects writers that take it. During a mixed-version rolling deploy, pre-0.18.2 processes still allocate from their process-local counters and can collide with or fork past locked writers. Replace all writers together; expect residual 23505 warnings until the last old process drains.

Compatibility

Backward compatible. appendToAuditChain is an optional interface method — adapters that omit it fall back to the existing single-writer path. Canonical hash form and per-org scoping are unchanged, so existing chains keep verifying. Versioned as a patch (0.18.2) per the repo's convention (fix + optional interface addition).

🤖 Generated with Claude Code


Note

High Risk
Changes audit integrity under concurrent production writers (silent event loss / chain fork before); rolling deploys need all writers on 0.18.2+ or collisions can persist during mixed versions.

Overview
Release 0.18.2 fixes tamper-evident audit (integrityAudit) when several processes share one store: process-local sequence/previousHash used to collide on the per-org unique index (silent drops) and fork the HMAC chain.

New storage contract appendToAuditChain — adapters read the durable head under a per-org lock, call back into the SDK for HMAC metadata, and insert atomically. createGovernance() uses it when present; legacy createAuditEventWithIntegrity + in-process counters remain for single-writer adapters, with a one-time onAuditError warning if durable integrity exists without atomic append.

Postgres implements transactional append (READ COMMITTED, pg_advisory_xact_lock, head SELECT scoped with COALESCE(organization_id,'')) plus a query-only pool fallback (bounded 23505 retries). Memory gets per-org async locks. Export/verify now sort by integrity sequence (not createdAt first), read integrity whenever getAuditIntegrity exists, and document that createIntegrityAudit() stays in-process only.

README adds a Multi-process deployments section; extensive regression tests cover multi-writer chains, partial adapters, and Postgres orchestration.

Reviewed by Cursor Bugbot for commit fadddbd. Bugbot is set up for automated code reviews on this repo. Configure here.

Comment thread packages/governance/src/index.ts
… (multi-pod safe)

The integrity audit chain held `sequence` and `previousHash` as process-local
state, resumed from the durable head only once at boot. Behind multiple pods
sharing one Postgres database, each process advanced its own counter, so two
pods derived the same sequence for the same org — one INSERT won and the other
lost to the UNIQUE (COALESCE(organization_id,''), integrity_sequence) index
(Postgres 23505), silently dropping that governance decision's tamper-evident
record. The per-process previousHash also forked the chain.

Sequence allocation and previous-hash derivation are now atomic against the
database, per org, on every write:

- New optional GovernanceStorage.appendToAuditChain(event, computeIntegrity):
  reads the org's durable head, invokes computeIntegrity(head) (signing key
  stays in the SDK core), and persists event + integrity as one operation.
- Postgres: transaction with pg_advisory_xact_lock(hashtext(org)) → head SELECT
  → INSERT → COMMIT, serialising each org's chain against itself across all
  writers; unrelated orgs proceed in parallel. Pools exposing only query()
  fall back to a bounded read-head → insert → retry-on-23505 loop.
- Memory adapter: per-org async lock spanning head-read → compute → write.
- createGovernance() prefers appendToAuditChain; the previous
  createAuditEventWithIntegrity + process-local sequence path remains as a
  single-writer fallback for third-party adapters.

API surface unchanged for consumers — createGovernance({ integrityAudit })
picks up the safe path automatically. Canonical hash form, per-org scoping,
verify(), export(), and stats() are unchanged.

Tests: two governance instances over one shared storage (two-pod model) fire
N concurrent writes → N rows, contiguous 1..N sequences, standalone-verifiable
chain; Postgres path asserts BEGIN/advisory-lock/SELECT/INSERT/COMMIT shape and
DB-head-derived sequencing; retry fallback recovers from a 23505.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rares04
rares04 force-pushed the fix/atomic-audit-integrity-sequence branch from 1724a65 to 4c0fca0 Compare July 15, 2026 08:43
Comment thread packages/governance/src/storage-postgres.ts Outdated
…append path

createdAt is stamped before the append lock allocates the sequence, so under
concurrent writers a lower sequence can carry a later timestamp (lock-wait
inversion, cross-pod clock skew). verifyAuditIntegrity() and export() sorted
createdAt-first and could report a valid multi-writer chain as tampered. Both
now order by the HMAC-covered sequence (createdAt tiebreaks); forging a
sequence still breaks the hash/previous-hash checks.

Also on the transactional append path:
- a failed ROLLBACK destroys the pooled connection (release(err)) instead of
  returning a dead/aborted client to the pool
- the advisory lock uses the namespaced two-arg form under a fixed classid so
  other advisory-lock users of the same database can't contend
- the chain-head SELECT scopes by COALESCE(organization_id,''), the exact
  partition of the unique index and lock key, so the head read can never
  disagree with the uniqueness/lock scope

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread packages/governance/src/storage-postgres.ts
@rares04

rares04 commented Jul 15, 2026

Copy link
Copy Markdown
Author

bugbot run

…ly append fallback

Every unique-violation on the fallback path means a competitor committed the
derived sequence, so a writer loses at most once per concurrent same-org
contender. Raise the cap to 12 (was 8) and desynchronise contenders with
jittered backoff so re-reads rarely re-collide; a 10-way contention test
completes deterministically under the bound.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@rares04

rares04 commented Jul 15, 2026

Copy link
Copy Markdown
Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 42c4a20. Configure here.

@rares04
rares04 requested a review from scotty595 July 16, 2026 07:03
@rares04 rares04 changed the title fix: allocate audit integrity sequence atomically per-org in Postgres (multi-pod safe) fix: multi-process-safe audit integrity chain via atomic per-org append Jul 20, 2026
…warn createIntegrityAudit is single-process

The atomic-append fix hardens the durable createGovernance({ integrityAudit })
path but leaves two things undocumented for SDK consumers who aren't the one
large multi-process deployment that surfaced the bug:

- appendToAuditChain is the storage contract that makes the chain
  multi-process-safe, but it was documented only in the interface JSDoc. Add a
  "Multi-process deployments" section to the README: what the method
  guarantees, a per-adapter support table (Postgres = advisory-lock
  transaction; memory = single-process by design; third-party = single-writer
  fallback), and how a custom adapter should implement it.
- The standalone createIntegrityAudit() wrapper keeps its chain in process
  memory and never persists integrity metadata, so it forks across processes
  and loses verifiability across restarts — the same unsoundness class, on a
  separate entry point. It cannot reuse the storage contract (it holds no
  storage handle), so it is documented rather than rewired: a prominent
  single-process warning in its JSDoc, the README note, and a regression test
  asserting a second wrapper over the same governance starts its own sequence
  at 1 instead of resuming the durable head. Consumers needing durable,
  multi-process audit are pointed at createGovernance({ integrityAudit }).

Also neutralise process-specific wording (multi-pod -> multi-process,
cross-pod -> cross-process) in the CHANGELOG, and record that DB-backed
stats() is deferred because it would turn a sync method async.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rares04
rares04 force-pushed the fix/atomic-audit-integrity-sequence branch from dcaa5f9 to 738b433 Compare July 20, 2026 10:52
Comment thread packages/governance/src/index.ts
Comment thread packages/governance/src/index.ts
… append fallback

Two capability-detection gaps for custom storage adapters that implement a
partial subset of the integrity contract (shipped Postgres/memory adapters
implement all methods, so they are unaffected):

- export()/verify() gated durable integrity reads on the legacy
  createAuditEventWithIntegrity. An adapter implementing the new
  appendToAuditChain write contract plus getAuditIntegrity — but not the legacy
  write method — persisted integrity durably yet exported an empty chain, since
  the read fell back to the (empty) in-process index. Gate the durable read on
  getAuditIntegrity alone (storageCanReadIntegrity).
- An adapter with durable integrity but no appendToAuditChain used process-local
  sequence allocation (single-writer-safe only) with no signal, contradicting
  the README's "falls back safely and warns". Emit a one-time onAuditError
  advisory; the pure-legacy session-local path keeps its per-write warn.

Adds audit-chain-partial-adapter.test.ts covering both subsets (each fails
without its fix); full suite 1450 pass, tsc clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rares04

rares04 commented Jul 20, 2026

Copy link
Copy Markdown
Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit fadddbd. Configure here.

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