Rotate a credential in one transaction

[zopeVaibhav]

Closes #53.

Changed course since the last push. The review offered two ways to take the index: convert the colliding callers in this PR, or defer the index until they are converted. I took the second and said the index would follow separately. It is back here instead, with all four callers converted and the backfill corrected, because the index is the half that actually stops the leak and splitting it meant merging twice to fix one bug. This PR is now the reviewer's first option, finished. Two commits: the atomic rotation, then the index and the callers.

Also covers item 4 of #88, which was written against f725fb5 and so predates both commits.

The problem

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, unreferenced and unannounced. Retrying wrote another one.

The first attempt at this, in #54, compensated in the caller. That does not work, and the review of that PR was right about why. 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; the compensating revoke then retires the new credential on top of a previous one that really was revoked, leaving the key with nothing live. The same fault that broke the first revoke is the one most likely to break the compensating one. And nothing held in the process survives the process being killed between the two writes.

Underneath that, nothing said a key had one live credential. Every leaked row above was legal. readModelSecret picked the newest and every other reader followed a stored id, so a deployment could hold several live credentials for one key with no way to tell which one it meant.

Rotation, in one transaction

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 writes 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, rather than overwriting an existing timestamp and reporting success. That is what let two callers each believe they were the one who retired a credential.

Revoke goes before insert inside the transaction. Both orders are invisible from outside it, and this one never holds two live rows for a key even in the middle, which is the invariant the index depends on.

One live credential per key

credentials_active_key_idx is 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.

The backfill decides which duplicate survives

Existing databases have to be reconciled before the index can be built, and 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. Keeping the newest revokes the credential actually in use and the deployment comes back up authenticating with nothing.

0006_credentials_one_live_key.sql ranks a referenced row first — referenced meaning named by connector_instances.credential_id, mcp_servers.credential_id, or an agent's configuration -> 'auth' ->> 'credentialId' — and falls back to the newest only where nothing points at either. On a database with no duplicates it rewrites no rows.

The four callers

Each stored a new credential for a key without retiring what was there. All four 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 the reference would leave that agent's key impossible to replace, with every later edit failing on the same stale id.
• 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 MCP token before deleting the server row, so adding the same server again does not meet its own leftover. The revoke goes first deliberately: two tables, 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.
• createCredential 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. It now treats a key that already holds a live credential as a replacement and rotates, atomically, recording credential.rotated naming what it replaced.

Threading the caller's transaction

storeAgentAuth takes an executor 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 on a small pool.

What is not covered

credential.rotated is written after the transaction returns rather than inside it, so a rotation that commits and then loses the audit INSERT still writes a half story. The same pattern is on every audit-writing path in the app, so folding audit into the transaction is a wider change than this bug; as agreed in the review of #54, it belongs to a separate issue about the audit interface.

The vault's errors still surface as 500s. There is no app.onError and the credential routes have no try/catch, so "not found or already revoked" and a mismatched key both come back as 500 rather than 404 and 409. Also a follow-up, and unchanged here.

Deleting a Bot still leaves its credential live. That is the remaining half of #88's item 4 and is not touched by either commit.

Verification

• Rotation, against Postgres: a rotation retires the previous credential and stores the new one together; an already-revoked previous credential is refused with nothing written; an absent previous credential is refused with nothing written; a mismatched kind, provider or keyId is refused with the previous credential left live; revoking twice is refused. writes no audit event when the rotation fails pins that a failed rotation records nothing.
• Backfill ordering, against Postgres: given two live duplicates with a connector_instances row pointing at the older, the older survives and the orphan is revoked.
• Each converted caller, against Postgres: an agent key edit, a Drive reconfigure onto a new subject, an MCP remove-then-re-add, and a second Add for a provider and keyId that already holds a live credential — each leaves exactly one live row and no unique violation.
• Migrations applied from an empty database, 0000 through 0006, on pgvector/pgvector:pg17. Resulting schema checked against meta/0006_snapshot.json: the index is unique with the partial predicate, accounts.issuer is nullable, and the sso_providers foreign key is set null.
• drizzle-kit check reports everything's fine and the journal agrees with the directory.
• Full suite from the repo root: 917 pass, 5 skip, 0 fail across 92 files.
• bun run typecheck and bun run format:check clean. bun run lint reports 27 warnings, four of them noNonNullAssertion in the credential tests this branch adds; the rest are on files it does not touch.

A note for whoever generates the next migration

meta/0005_snapshot.json on main is stale. It records accounts.issuer as NOT NULL and the sso_providers foreign key as cascade; #87 then changed both in core.ts and did not regenerate it. drizzle-kit generate therefore emits an ALTER COLUMN issuer DROP NOT NULL and a foreign key drop-and-re-add that belong to that release, not to this one. I dropped them from 0006, whose SQL is the backfill and the index only. The regenerated 0006 snapshot carries the corrected state, so the drift stops here, but the SQL for it was never the problem — 0004 already applies the foreign key change and the NOT NULL was never applied at all, which the clean-slate migrate above confirms.

[jeonjw85]

The transaction approach is the right fix. Compensation was always going to lose to a post-commit throw or a SIGTERM between the two writes, and FOR UPDATE plus revoke-before-insert is the order the unique index forces. The tests look good, especially the one that a failed rotate writes no audit event.

The index is the problem though.

credentials_active_key_idx forbids two live rows for the same (kind, provider, key_id). That's what rotate needs. But three other paths on main already create a new live row without revoking the old one:

1. Agent key edit. profile-store update calls storeAgentAuth, which calls store.create with keyId = agentId. Editing an agent that already has a key unique-violates and rolls back the whole profile update.
2. Google Drive reconfigure. configureGoogleDrive uses keyId = impersonationSubject. Same subject, same collision.
3. MCP re-add. storeMcpToken uses keyId = mcp-${serverId}, addServer does onConflictDoUpdate, and removeServer never revokes. Remove-then-re-add still collides.

Those currently leave orphans, which is the bug in #53. With the index they become a raw unique violation and a 500. Either those callers need to go through rotate (or revoke-then-create) in this PR, or the index should wait until they do.

Two smaller things:

• The migration is a bare CREATE UNIQUE INDEX. rotateCredential leaves an orphan credential if the revoke of the previous one fails #53 is specifically about duplicate live rows already existing, so this can fail on a real database. Needs a cleanup step, or at least a note.
• rotate doesn't check that the previous row's kind/provider/keyId match the input. Rotating key X with input key Y silently leaves X with no live credential.

The audit-after-commit gap you called out is fine as a follow-up.

[zopeVaibhav]

Thanks for this, it was the right call to look at the index separately from the rotation.

I checked all three callers you named and you are right about every one. storeAgentAuth at agents/auth-header.ts:62 inserts with keyId = agentId and the update path at profile-store.ts:363 reaches it, configureGoogleDrive at connectors.ts:78 inserts with keyId = impersonationSubject, and removeServer at plugins/store.ts:352 deletes the server row without touching the token. All three leak orphans today and all three would have unique-violated with the index in place.

Going back through the callers turned up two more things, and together they changed my mind about shipping the index here at all.

There is a fourth colliding caller and it is the most used one. app/src/lib/credentials/mutations.ts has create and revoke factories and no rotate factory, so POST /api/admin/credentials/:id/rotate has no client caller anywhere. The admin Credentials page offers Add and Revoke, which means replacing a model key is done by adding a second one for the same provider and keyId. Under the index that is a unique violation on the primary path for the feature.

The bigger one is that my backfill revoked the wrong row. It kept the newest live credential per key, but the duplicates this is meant to clean up come from rotations that inserted the new row and then failed to revoke the old one, so the caller saw an error and never repointed anything. The reference still names the older row. I set this up against Postgres with a connector_instances row pointing at the older of two live duplicates, and the backfill revoked the referenced one and kept the orphan. Every deployment the cleanup exists to rescue would have come back up authenticating with a revoked credential. Thanks for pushing on that migration, I would not have gone looking otherwise.

So I have taken the index, its migration and the caller conversions out, and this PR is now only the atomic rotation plus two guards. Nothing in it changes any caller's behaviour, and it stands on its own for the bug in #53. Your second option, in other words: the index waits until the callers are ready for it.

The mismatch guard you suggested is in this PR rather than deferred. POST /api/admin/credentials/:id/rotate takes the id from the URL while the key comes from the body, so a mismatched pair would retire one key's only live credential and store the new secret under another. That felt worth closing now even though no client calls the route yet.

The audit-after-commit gap stays a follow-up, as you and the earlier review both suggested. Same for mapping vault errors onto 404 and 409 instead of 500, which needs an app.onError since the credential routes have no try/catch at all today.

The follow-up PR has the index, the corrected backfill that prefers a referenced row over the newest, and all four callers converted. It also has to thread the caller's transaction into the store: profile-store calls storeAgentAuth from inside its own transaction while holding row locks, so rotate opening a second transaction on a second pooled connection would commit separately from the profile update and can deadlock on a small pool. I will open it once this one lands, since it builds on store.rotate.

[zopeVaibhav]

Changed course on the split. I said the index would come as its own PR once this landed, and it is in this one instead.

Your first option, in the end. All four callers now go through rotate-or-create: storeAgentAuth checks liveness rather than trusting the agent's stored reference (an administrator can revoke a key from the Credentials page without anything repointing the agent, and rotating onto a revoked row is refused, so trusting it would leave that key impossible to replace), configureGoogleDrive retires the credential a subject change abandons, removeServer revokes before deleting the server row, and createCredential treats a second Add for a live key as a replacement — which is the path the product actually uses, since there is no rotate control in the UI.

The backfill prefers a referenced row over the newest now. Proved it against Postgres rather than reasoning about it: two live duplicates with a connector_instances row pointing at the older, run the migration, the older survives and the orphan is revoked, and the unique index builds after it.

Reason for folding it back in: the index is the half that stops the leak. Splitting meant merging twice to fix #53, and the rotation alone changes no caller's behaviour, so on its own it fixes nothing an operator would notice. Rebased onto current main as well — the migration is 0006 now, since #46 took 0005.

Audit-inside-the-transaction and the 500-vs-404/409 mapping stay follow-ups, as agreed.