004-credential-rotation-mutual-exclusion

ADR 004: Mutual Exclusion for Concurrent Credential-Rotation Sessions

Status: Proposed — design for #5967. No implementation lands in this PR; this ADR is the plan the follow-up implementation issue(s) will execute against. Date: 2026-08-09 Issue: #5967 Relates: #5925 (the rotation that surfaced the race), 5954 (competing + reconciling PRs), #5923 (parent .env.local exposure incident), #5556 (unified dev_r_rotation_log), #2728 (credential-isolation policy), #1680 (per-issue agent_tasks mutex — the prior art this reuses in spirit)

Context

On 2026-08-09 two independent p24-infra sessions both worked #5925 (rotate PINBOX24_MONGODB_URI for the et_oper MongoDB user) at the same time with zero cross-session visibility:

• Session A rotated the MongoDB password, merged PR #5947 at 09:43:42 UTC (Implements: #5925, which closed the issue), and secrets-sync.yml deployed its new password to Vercel. Its PR only touched secrets/et-operational-platform.env.sops — it missed secrets/n8n-bms4.env.sops, a bootstrap-copied consumer the issue explicitly named.
• Session B was unaware #5925 had already closed (it had been blocked earlier by the Auto Mode safety classifier on the live rs0 write, then resumed after explicit user confirmation without re-checking issue state) and ran its own db.updateUser on the rs0 PRIMARY at ~09:48–09:49 UTC, after Session A’s sync had already shipped Session A’s password.

Net effect: main/Vercel held Session A’s now-stale password while the live rs0 server held Session B’s newer one, and n8n-bms4.env.sops was left on the original exposed value entirely. The /zarzad/pinbox feature and the pinbox-async-export n8n workflow were both broken against the live credential until reconciliation PR #5954 fixed all three files/targets in one pass.

Why this is a structural problem, not a one-off

A live-server credential write (MongoDB updateUser, S3/IAM rotate-access-key, any provider API that mints a new value in place) has three properties that a normal code change does not:

1. Non-idempotent across sessions. Each session generates its own new secret value. Running the “same” rotation twice does not converge — the second write silently overwrites the first with a different value.
2. Not git-mergeable. The effect lives on a live server, not in a file. Git’s conflict machinery — the thing that catches two people editing the same file — sees nothing.
3. No error signal to either party. Whoever writes last wins on the server; whoever’s secrets-sync.yml ran last wins in SOPS/Vercel/containers. When those two are different sessions, the distributed value and the live value diverge with no exception, no failed check, nothing.

The only cross-session signal available today is GitHub issue open/closed state, and it is checked (if at all) at task start — never immediately before the live write. That gap is exactly what #5925 fell through.

What already exists (and why we should build on it, not beside it)

dev_r_rotation_log (#5556) is already the single rotation log for desktop, worker, and automated rotations. Its lifecycle is a near-perfect fit for a lock:

• Every rotation is required to open a pending row before touching the key (scripts/rotation-log-entry.py open --secret NAME → scripts/lib/rotation_log.py:open_rotation).
• It closes the row (completed) or fails it (failed) when done.
• Alertmanager already fires on stalled pending rows (status='pending' AND created_at < NOW()-2h).
• It is keyed on secret_name, has a repo column, a created_at timestamp, and an offline fallback path — everything a per-secret advisory lock needs, minus the deny semantics.

In other words: a “pending rotation row for secret X” is already a “rotation of X is in progress” marker. It just does not yet stop a second session from opening its own.

Options considered

The issue proposed three candidate directions. All were evaluated against: does it stop the #5925 race, what does it cost, and how does it behave when a session dies mid-rotation.

(a) Re-check issue state immediately before the live write

Before any non-idempotent live write, re-run gh issue view <N> --json state,closedAt fresh and abort if already closed.

• Pro: trivial; zero new infrastructure; catches the precise #5925 failure (Session B proceeded after the issue was closed).
• Con: issue state is a coarse and incomplete signal. Two sessions can both hold the issue open simultaneously (Session A had not closed it yet when Session B started work). Not every rotation maps 1:1 to an issue — scheduled rotate-credentials.py runs and ad-hoc desktop rotations often have no issue at all. And it does nothing for two sessions dispatched against the same issue.
• Verdict: necessary but insufficient as the primary mechanism. Kept as a cheap pre-write backstop (see Decision), not relied on alone.

(b) New dev_r_rotation_locks table

A purpose-built table keyed by secret name / issue number, claimed before the write and released after, with a TTL.

• Pro: clean, self-documenting, dedicated TTL column.
• Con: it duplicates ~90% of what a pending dev_r_rotation_log row already is (per-secret in-progress marker + timestamp + owner), and pays the full new-primitive tax: a new migration, a new compliance registration in dev_r_services, RLS + grant hardening, a new Grafana panel/alert, a new CLI, and a second place that can drift from the rotation log. Two tables that both mean “rotation of X is happening” is exactly the kind of split #5556 just spent an issue collapsing.
• Verdict: rejected — disproportionate for a rare race when a strictly cheaper option exists.

(c) Repurpose the rotation-log open entry as the lock

Extend open so it first checks for an existing open, unclosed entry for the same secret and refuses/warns if one exists.

• Pro: reuses the table, CLI, offline fallback, and alerting we already have; the open already runs at the right moment (before the key is touched); needs no new table and no new columns — only a uniqueness guard and a new acquire verb.
• Con (must be designed around, not ignored):
  1. open_rotation is deliberately fail-open (always exit 0 — logging must never block a rotation). A lock must be able to deny. So the lock cannot just be “open with a warning”; it needs a distinct, fail-closed acquire path.
  2. A naive “SELECT pending for secret; if none INSERT” has a TOCTOU window — two sessions both read “none” and both insert. Closing a rare race with a mechanism that has its own race is pointless.
  3. The current stalled threshold is 2 h — far too long for a lock TTL (a crashed session would block the next attempt for two hours).
• Verdict: chosen, with all three cons explicitly engineered away below.

Decision

Adopt a two-layer design: a real advisory lock built on dev_r_rotation_log (option c, hardened), backed by a cheap pre-write re-check (option a) as defence in depth. Option (b) is rejected.

Layer 1 — advisory lock on dev_r_rotation_log (primary)

Scope — this is the crux of keeping false positives near zero. The lock is acquired only for rotations that pair a SOPS write with a live server-side credential write (MongoDB updateUser, S3/IAM key rotation, any provider API that rotates a value in place). A SOPS-only edit — adding a new key, editing a non-live config value, re-encrypting — does not acquire the lock: those are git-mergeable, and a second author simply resolves a normal PR conflict. The danger is the non-idempotent live write, not the SOPS file, so that is the only thing gated. Consequence:

• Two rotations of different secrets never block each other (lock is keyed on (repo, secret_name)).
• The large majority of SOPS edits (pure config/new-key changes) never touch the lock at all.

The lock is therefore as narrow as the actual race — same repo, same secret, live write — which directly answers the issue’s “false-positive cost” concern.

Atomicity (closes con #2). Add a partial unique index so the database itself guarantees at most one in-flight rotation per secret:

CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS dev_r_rotation_log_one_pending_per_secret
  ON public.dev_r_rotation_log (repo, secret_name)
  WHERE status = 'pending';

Keying on (repo, secret_name) (not secret_name alone) prevents a false collision between two different repos that happen to use the same key name. The acquire is then simply the existing open POST: if it returns a 409 unique-violation, another session holds the lock. There is no TOCTOU window — the uniqueness is enforced at commit, not by a read.

Migration safety note. A CREATE UNIQUE INDEX fails if the live table already contains two pending rows for the same (repo, secret_name). The implementation migration must first query for and resolve any such duplicates (mark the older ones failed with a note) before creating the index. The table is small (~48 rows as of #5556), so this is a one-time cleanup, but it must be in the migration, not assumed away. Apply via the per-repo ledger runner — never mcp__claude_ai_Supabase__apply_migration (banned project-wide, #5653) — writing the file to supabase/migrations/ and letting the ledger runner apply it. CONCURRENTLY cannot run inside a transaction block; if the ledger runner wraps migrations in a transaction, split this into its own non-transactional step or drop CONCURRENTLY (acceptable given the table’s size).

TTL / crash recovery (closes con #3, answers “crash/timeout handling”). A dead lock-holder must not block forever, so acquire has a reclaim step keyed on a short TTL (proposed 30 min, distinct from the 2 h human-visible stalled alert):

1. acquire POSTs the pending row.
2. On 409, re-read the conflicting pending row.
3. If its created_at < NOW() - <TTL>, it is presumed crashed. Atomically reclaim it with a guarded PATCH:

   PATCH dev_r_rotation_log?id=eq.<stale>&status=eq.pending&created_at=lt.<cutoff>
     {"status":"failed","error_message":"lock expired — presumed crashed, auto-reclaimed by <session>"}
   

   The status=eq.pending filter makes this race-safe: concurrent reclaimers serialise on the row lock and only the first re-matches pending; losers PATCH 0 rows and fall through. (This mirrors the exact supa_claim_task stale-reclaim pattern already used for the per-issue agent_tasks mutex — #3592.)
4. Retry the POST once. If it 409s again (someone else acquired in the gap), abort — do not loop.

This gives automatic unblock after a crash without a permanent lock and without a background sweeper: the next legitimate acquirer does the cleanup. The existing 2 h stalled-rotation Grafana alert stays as the human-visible backstop for the (now much smaller) window where reclaim never runs because no one attempts the same rotation again.

Fail-closed acquire (closes con #1) — the one deliberate contract split. Logging stays fail-open; the acquire is fail-closed. If acquire cannot reach Supabase to verify that no lock is held, it must not proceed blindly — “can’t verify” during a live credential write is precisely the moment the race is dangerous. It aborts and escalates, with a documented, logged --force override for the genuine emergency where a human has confirmed out-of-band that no other session is running. Rationale: the race is rare, the DB being unreachable is rare, both at once is very rare, and the override exists for that corner. The offline fallback file is not a substitute here — a local markdown spill buffer on one host cannot see a lock held by a session on another host, so for lock acquisition (unlike logging) the fallback provides no safety and must not be treated as success.

Release. No new release path. The existing close (completed) / fail (failed) already flips the row out of pending, which is exactly what frees the partial-unique-index slot. Reuse it.

human-action rows. A row escalated to status='human-action' is not pending, so it does not hold the index slot. That is intentional: an escalated, human-owned rotation should not machine-block a later automated attempt indefinitely — the human owns reconciliation at that point. The design notes this so it is a decision, not an accident.

Layer 2 — pre-write re-check (backstop, option a)

Immediately before the non-idempotent live write (not at task start), re-verify both:

1. This session still holds the lock — the pending row still exists with our row_id and has not been reclaimed/stolen. This catches the nasty case where a merely-slow-but-alive session (e.g. stalled on the Auto Mode classifier, like #5925’s Session B) had its lock reclaimed as “stale” while it was paused, and would otherwise do a late, blind write. If our row is gone → abort and reconcile.
2. If the rotation is tied to a GitHub issue, the issue is still open (gh issue view <N> --json state). If closed by someone else → abort and reconcile rather than write blind. This is the direct, literal fix for the exact #5925 sequence.

Either check failing routes to reconcile, not to a silent overwrite: comment on the issue, and re-verify the live-vs-distributed value before deciding whether a write is even still needed.

Where the check/lock lives

Layer	Location	Change
Acquire + reclaim + TTL	scripts/lib/rotation_log.py	new acquire_lock(repo, secret_name, ...) (fail-closed) + holds_lock(row_id) re-check helper
CLI surface	scripts/rotation-log-entry.py	new acquire and check subcommands (thin wrappers), --force on acquire
DB guarantee	supabase/migrations/<ts>_rotation_lock_partial_unique_5967.sql	partial unique index + dup-cleanup, via per-repo ledger runner
Process doc (mandatory step)	infra/agent-prompts/worker-secret-manager.md §General pattern, docs/playbooks/secret-manager-rotation-log.md, and the live-write rotation playbooks (docs/playbooks/mongodb-credential-rotation.md, docs/playbooks/w3-w4-rotation-orchestration.md, wasabi/IAM)	require acquire before the first live/SOPS mutation + the Layer-2 pre-write re-check
Secondary, complementary	infra-src/meta-dispatcher	optional refuse-to-double-dispatch-same-issue guard

On the dispatcher option. The dispatcher refusing to double-dispatch the same issue number is a useful complementary coarse layer, but it is explicitly not the primary mechanism and the design must not rely on it: it cannot see manual desktop rotations or scheduled rotate-credentials.py cron runs, both of which bypass the dispatcher entirely — and those are exactly the paths #5925’s two sessions used. Treat it as optional defence-in-depth, gated behind the real lock.

Consequences

Positive

• The exact #5925 sequence is blocked twice: Layer 1 stops Session B from acquiring while Session A holds the lock; if Session B somehow acquired (e.g. A had already released), Layer 2’s issue-state and lock-ownership re-checks stop the blind late write.
• No new table, no new column, no new compliance surface — one partial index + code, on infrastructure #5556 already made canonical.
• False-positive blast radius is minimal by construction (per-(repo, secret_name), live-write-only).
• Crash recovery is automatic (30-min reclaim) with a human backstop (2 h alert) already in place.

Negative / accepted trade-offs

• One deliberate contract split: acquire is fail-closed while logging remains fail-open. Mitigated by the logged --force override and documented clearly so it is not surprising.
• Honour-system enforcement: like the role-scope mechanism (#4705) and the per-issue mutex, the lock only protects rotations that actually call acquire. The design’s job is to make acquire the single documented first step of every live-write rotation and to wire it into the secret-manager worker prompt + playbooks so it is the path of least resistance. Hard enforcement (e.g. a hook that blocks a live updateUser/IAM call unless a matching lock row exists) is a possible follow-up, noted but out of scope here.
• The partial index adds a uniqueness invariant to all open_rotation callers (including SOPS-only and scheduled). This is assessed as correct and desirable (“at most one in-flight rotation per secret” holds universally), but the implementation must verify no existing caller relies on holding two concurrent pending rows for one secret before shipping the index.

Follow-up issues to open (implementation, not this PR)

1. Migration + index — dup-cleanup + dev_r_rotation_log_one_pending_per_secret, via the per-repo ledger runner. Includes a test that a second concurrent open for the same (repo, secret_name) gets a 409.
2. rotation_log.py acquire/reclaim/holds_lock + CLI acquire/check + --force — with unit tests for: happy-path acquire, 409-collision abort, stale-reclaim after TTL, race-safe reclaim (two reclaimers, one wins), and fail-closed-on-DB-unreachable.
3. Playbook + worker-prompt wiring — make acquire the mandatory first step and add the Layer-2 pre-write re-check to infra/agent-prompts/worker-secret-manager.md, docs/playbooks/secret-manager-rotation-log.md, and each live-write rotation playbook.
4. (Optional) dispatcher double-dispatch guard and (optional) hook-level hard enforcement — separate, lower-priority hardening issues.

Verification (how we will know it works)

• Unit tests per follow-up 2 above (409 on concurrent open; reclaim after TTL; fail-closed).
• A scripted two-process integration test: process A acquires TEST_KEY, process B acquires the same key and must be denied; A closes; B acquires and now succeeds; simulate a crash by leaving A’s row pending with a back-dated created_at and confirm B reclaims after the TTL.
• Manual: re-run the #5925 shape in a dry-run harness (two sessions, same secret) and confirm the second is denied at acquire and, if forced past it, aborted at the Layer-2 pre-write re-check.