Skip to main content

ADR-0188: An audit write may join the caller's transaction, and a lent session must declare its authority

id: 0188-audit-writes-may-join-the-callers-transaction sidebar_position: 189 title: "ADR-0188: An audit write may join the caller's transaction, and a lent session must declare its authority" ---# ADR-0188: An audit write may join the caller's transaction, and a lent session must declare its authority

Date: 2026-08-18 Status: Accepted Deciders: Adrian (project owner), Claude Opus 5, Codex (review, PR #1142) Relates to: ADR-0109 (append-only hash chain), ADR-0064 (audit immutability), ADR-0023 / ADR-0050 (RLS), ADR-0187 (an unchained row is not tampering)

Context​

34 writers across 20 modules construct AuditEvent(...) and hand it to a session directly, so their rows reach audit_events with no chain_seq and no entry_hash — outside the tamper-evidence ADR-0109 claims (#1034). On the pilot that is 292 of 2,804 rows.

The obvious remedy is to point all 34 at AuditService.log_event. Measured before writing any code, that remedy is wrong as the code stood.

log_event opened its own session (get_tenant_session / get_session) and committed independently. Most of those 34 add their audit row to a session that already carries the business writes it audits — api/goaml.py::_write_draft_audit, services/maker_checker.py::_approve, services/case_service.py::update_case_with_audit, and 20 more. Classified by whether the audit row shares a transaction with any other write: 23 functions share one, 9 do not.

So migrating them as-was would have traded a chain gap for an atomicity gap: a business change that rolls back leaving a fabricated audit row behind, or a failed audit write letting an unaudited change proceed. Both are worse than the gap being closed.

That reframes the bypass. It was not carelessness. It was the only shape available that kept the audit row atomic with the change it records, because log_event could not join the caller's transaction.

Decision​

log_event(..., session=s) writes into a transaction the caller owns. The row is added and flushed, never committed; the caller's commit decides.

pg_advisory_xact_lock is transaction-scoped, and its job is to serialise the chain-head read against the insert. Both therefore belong inside whichever transaction is doing the writing. This is the correct semantics, not a workaround.

There is exactly one hashing path. This module's own docstring is emphatic that a second, subtly-different hash implementation would make the chain unverifiable, so the borrowed session runs the same body under a conditional context manager rather than a copy of it.

A lent session must DECLARE its authority. Two shapes are accepted:

1. app.current_tenant equal to the row's tenant — the ordinary get_tenant_session case; 2. app.rls_bypass = 'true' — what get_admin_session sets, for a system write that deliberately crosses tenants.

Anything else is refused, including a session with no RLS context at all.

The first implementation allowed the no-context case, justified in code and in a test docstring as "the admin/bypass session the get_session() path already takes for case-less system events". That was measured false: get_session calls _set_tenant_context(session, DEMO_TENANT_ID), so it sets the scope to the demo tenant and is not the unset case at all. The permissive arm rested on a claim the code contradicted, and admitted any raw session — including one whose RLS context had never been established — into a writer for an immutable, tenant-owned, hash-chained table. Owner decision, 2026-08-17: require the declaration.

The flag's value decides, not its presence: 'false' is refused. Reading a flag's presence as its value is the presence-≠-evidence shape CR1 of the Calibration Review checklist names, and a mutation replacing the comparison with a truthiness test survived until a test set the flag to 'false' explicitly.

Consequences​

Positive​

- The 34-writer migration becomes possible without an atomicity regression. Nine functions can move immediately; 23 need session=, and now it exists. - An audit row written this way commits — or rolls back — with the change it records. That is a stronger guarantee than the sanctioned writer previously offered, not merely a restoration of the bypass's behaviour. - The flush surfaces an integrity error at the call site that supplied the row, rather than later at the caller's commit where it is attributable to nothing. Demonstrated accidentally: an FK violation in this change's own test fixture raised at the flush and named the missing case. - A cross-tenant lend is refused at the boundary, where the mismatch can be named, instead of surfacing as an RLS refusal at the caller's commit.

Negative​

- The chain advisory lock is now held for the caller's whole transaction, not just for the insert. A long business transaction serialises that tenant's audit writes for its duration. Measured at 1.50 s in a two-connection demonstration. - A new lock-ordering exists: business lock, then chain lock. Nothing today takes them in the other order, but a future writer that did would deadlock, and nothing detects that. - log_event returns an id for a row that is flushed but not committed. A caller that records that id and then rolls back has recorded an id for a row that never existed. - RLS context provision moves to the caller. A caller that lends a session must now think about its scope. That is the point, but it is a burden the previous signature did not impose. - Three transaction-boundary tests in this change's own suite had to be repaired, because their fixture lent a bare session — a shape no production caller has and the guard now rejects. The guard found its first defect in the tests written to exercise it.

Neutral​

- The mode is dormant. 0 of ~50 production call sites pass session= today; every existing caller omits it and behaves exactly as before. The migration is separate work (#1034). - No schema change, no migration, no feature flag.

Alternatives Considered​

Alternative 1: Migrate the 34 writers to the existing log_event​

- Point every bypassing writer at the sanctioned one as it stood. - Why rejected: measured, 23 of the 32 enclosing functions write something else in the same transaction. This trades a chain gap for an atomicity gap — a rolled-back business change leaving a fabricated audit row, which is a worse compliance defect than an unchained one. It also would have been discovered only in production, since no test covers a rollback path across that seam.

Alternative 2: Leave the 34 writers alone and accept the chain gap​

- Treat the bypass as permanent and document it. - Why rejected: ADR-0109's tamper-evidence is a claim the compliance story leans on, and 292 rows outside it make that claim conditional in a way no artifact states. ADR-0187 makes the condition visible; it does not make it acceptable.

Alternative 3: Allow a lent session with no RLS context​

- The first implementation, defended as matching get_session's behaviour. - Why rejected: the justification was false (get_session sets the scope), and once that is known the arm admits any raw session into an immutable, tenant-owned table on no declared authority. Refusing costs a caller one set_config; allowing costs an audit row under the wrong tenant with nothing naming the mistake.

Alternative 4: Give log_event a commit=False flag instead of a session​

- Keep the own-session path and let the caller suppress the commit. - Why rejected: it does not solve the problem. The row would still be in a different session from the caller's business writes, so it could not roll back with them — the atomicity gap survives, now with a flag that implies otherwise.

Decision context: - Latency: one extra round trip per lent write (the scope/bypass probe), against a path that already performs an advisory lock, a head read and a SHA-256. The lock-hold duration is the real cost and is recorded above as a negative. - Dependency surface: none added. - Debuggability: improved at the boundary — a refusal names the tenant, the scope it found, and the three ways to satisfy it. Worse in one place: the advisory lock is now held across code log_event cannot see, so a lock-wait is diagnosed in the caller, not here. - Reversibility: one optional parameter and one guard. Removing them restores the previous behaviour exactly; every existing caller omits the parameter. - Blast radius: audit_service.py only. Additive: no signature change for any current caller, no schema, no runtime path altered while the mode is dormant. - Alternative considered: migrate the writers to the existing signature — rejected because it trades a chain gap for an atomicity gap (Alternative 1).