Rotate a credential in one transaction (#76)
* Rotate a credential in one transaction Rotation was two independent store calls: insert the new credential, then revoke the previous one. A failure on the revoke returned an error to the caller and left the new credential live in the vault, where nothing referenced it and nothing said it was there. Retrying wrote another one. Compensating for that in the caller does not work, and the first attempt at this tried. `revoke` can commit its UPDATE and still throw on the way back, from a statement timeout, a dropped connection, or the pool being torn down as the response returns; a compensating revoke then retires the new credential on top of a previous one that really was revoked, and the key is left with nothing live. The same fault that broke the first revoke is the one most likely to break the compensating one, so the recovery is least available exactly when it is needed. And nothing in the process survives the process: killed between the two writes, no compensation runs at all. So the two writes are now one. `CredentialStore.rotate` opens a single transaction, locks the previous credential `FOR UPDATE`, revokes it and inserts its replacement, following the shape already used for agent profiles in `agents/profile-store.ts`. Either both land or neither does, Postgres decides that rather than this code, and a process that dies mid-rotation leaves a database that rolls itself back. `credential.rotated` is recorded only once the transaction has returned, so a rotation that never happened leaves no row claiming it did. Two guards come with it. The lock reads the previous credential's kind, provider and keyId as well as its state, and refuses a rotation whose input names a different key: `POST /api/admin/credentials/:id/rotate` takes that id straight from the URL while the key it rotates into comes from the body, so without the check a mismatched pair would retire one key's only live credential and store the new secret under another. `revoke` now stamps only a row that is still live and says "not found or already revoked" otherwise, instead of overwriting an existing timestamp and reporting success, which is what let two callers each believe they were the one who retired a credential. Tests cover both layers: that a failed rotation writes no audit event, and against Postgres that a rotation retires the previous credential and stores the new one together, that an already-revoked or absent previous credential is refused with nothing written, that a mismatched key is refused with the previous credential left live, and that revoking twice is refused. * Hold one live credential per key, and let every caller keep to it A key was free to accumulate live credentials. Nothing said which of them a deployment meant, `readModelSecret` picked the newest and every other reader followed a stored id, and the failed rotations in #53 left exactly this behind: a live row nothing referenced, invisible until somebody went looking. Two replicas rotating the same secret could also both write one, since nothing serialised them. `credentials_active_key_idx` makes it a rule the database keeps: unique on (kind, provider, key_id) where revoked_at is null. Revoked rows are excluded, so history is untouched and only what is current is constrained. Existing databases have to be reconciled before that index can be built, and which duplicate survives decides whether a deployment comes back up working. The newest is the wrong answer: in the failure this cleans up it is the new row that nothing references, while the older one is still named by the connector, MCP server or agent that was using it, so keeping the newest revokes the credential actually in use. The backfill ranks a referenced row first and falls back to the newest only where nothing points at either. Three callers stored a new credential for a key without retiring what was there. All three leaked orphans already; under the index they would have failed outright. `storeAgentAuth` rotates when the agent has a live credential and inserts when it does not, checking liveness rather than trusting the reference: an administrator can revoke a key from the Credentials page, nothing repoints the agent that names it, and rotating onto a revoked row is refused, so trusting it would leave that agent's key impossible to replace. It also takes the caller's transaction now. Agent edits are a transaction over `agents` and `agent_profiles` and the credential belongs to that same change; written on a pooled connection of its own it would commit even where the edit rolled back, and could deadlock against the locks that edit is holding. `configureGoogleDrive` retires the credential a reconfigure abandons when the impersonation subject changes. That row keeps its own key and is referenced by nothing afterwards, so a subject set, changed, and set back again would meet it again on the index. `removeServer` revokes the token before deleting the server row, so adding the same server again does not meet its own leftover. The revoke goes first deliberately: these are two tables with no transaction spanning them, and a failure between them should leave a server that removing again will finish off rather than a live token nothing can reach. The fourth caller is the one the product uses most. The Credentials page offers Add and Revoke and no rotate control, and `rotate` has no client caller at all, so replacing a model key is done by adding one for the same provider and keyId. `createCredential` therefore treats a key that already holds a live credential as a replacement and rotates, which is atomic and records `credential.rotated` naming what it replaced, rather than raising a bare unique violation on the only path the page offers. Tests cover each caller and the rule itself: an agent key created, rotated, and created again over a revoked reference; a Google Drive reconfigure under the same subject and under a changed one, including setting the original back; an MCP server removal revoking its token; the index refusing a second live row; and adding a credential for an occupied key replacing what was there. The connector tests run against the real vault rather than a stand-in, which is what would have caught the drift here in the first place. * Retire a key on the connection that holds its lock Editing a Bot's bearer token never returned. `storeAgentAuth` rotates inside the update's transaction, locking the previous credential and revoking it there, and `retireReplacedKey` then revoked the same row again from the pool. The pooled statement waited for a lock only that transaction could release, and the transaction could not commit while it was awaiting the call. Postgres saw one transaction and one waiting session rather than a cycle, so no deadlock detector fired and the edit hung to the statement timeout, which the surrounding catch then reported as a key still live. The suite missed it because the rotation tests hand `storeAgentAuth` a fake store, and a fake holds no locks. The call is gone from the update path rather than given the transaction. The rotation above it has already revoked that row, so a second revoke was redundant before it was unsafe. Deletion still needs it, and takes the transaction now: on its own connection the revoke would commit even where the delete rolled back, leaving a Bot that still exists and can no longer reach its endpoint. `agent-key-rotation.integration.test.ts` drives an edit and a deletion against a real database on one connection, which is where the hang is sharpest, and bounds the wait so a regression fails rather than hangs. It fails on the code before this commit with the deadline it was given. Two callers arrived on main since this branch was written, and both would have met the index rather than the orphan they used to leave. `registerOAuthClient` and `recordConnection` replace rather than add now, asking `findLiveByKey` rather than trusting a stored pointer, because a server row or a connection row keeps naming a credential an administrator has revoked while the key itself is free. A refused rotation records `credential.rotation_refused`. Aiming a rotation at a revoked credential, at one that does not exist, or at a key other than the credential's own is either a caller with a bug or an attempt to retire a key the caller was not asked to retire, and all three left nothing behind while only the successes were written. The test that asserted that absence now asserts the row. The migration is `0012`, and its backfill no longer reads `connector_instances`, which `0011` dropped. The tables that name a credential are `mcp_servers`, `mcp_user_credentials` and an agent's configuration. * Say that adding onto a live key replaces it, and keep the snapshot formatted The page calls this Add and has no rotate control, so an administrator saving a key that already holds a live credential retires the one an MCP server or an agent is currently authenticating with, and nothing on screen mentioned it until something stopped working. The dialog now says so before the write, from the list it already has, asking the same question the unique index asks when the save lands. _journal.json had lost its trailing newline and the new snapshot was unformatted, which failed format:check. The repo's other fifteen snapshots are already formatted, so this only brings it into line. Also finish the rebase: this branch renames the schema import in plugin-store.integration.test.ts, and a newer block on main still used the old name. --------- Co-authored-by: David McKay <davidmckayv@users.noreply.github.com>
V
Vaibhav Zope committed
653460d0fab11a728a713dd0ddc4eab7ef52e72c
Parent: 983c909
Committed by GitHub <noreply@github.com>
on 8/24/2026, 5:16:34 PM