Roll back a credential rotation when revoking the old one fails

[zopeVaibhav]

Closes #53.

The problem

rotateCredential writes the new credential, then calls store.revoke on the previous one. The two steps are independent, so a failure on the revoke leaves the new credential live in the vault while the caller sees an error and treats the rotation as failed.

readModelSecret orders by createdAt desc, so the orphan is what the runtime resolves to next time. No credential.rotated audit event is written for a failed attempt, so nothing on the audit trail says two live credentials exist for the same (provider, keyId). A caller that retries writes another orphan each time.

The approach

Wrap the revoke in a try/catch. If it throws, revoke the credential that was just written and rethrow the original error. The secondary revoke swallows its own error so the caller still sees the real cause. The credential.rotated audit event stays where it was, past the try/catch, so the trail continues to reflect only rotations that actually happened.

Either both sides moved or neither did.

What is not covered

• Pre-existing orphans from earlier failed rotations. This change stops new ones; it does not clean up the vault.
• A crash between persistCredential and the revoke (process killed, node lost) still leaves an orphan. A durable compensation would need a workflow table or a two-phase write; out of scope for the bug on the table.
• Rotation is not made atomic across create and revoke. The store interface does not expose a transaction, and adding one touches every backend that implements CredentialStore.

Verification

• Added rolls back the new credential when revoke of the previous one fails in server/tests/credentials.test.ts. It fails on main with Expected to contain: "credential-new" / Received: [ "credential-old" ] and passes with the fix. All 13 tests in that file green.
• Full test suite from repo root: bun run test reports 684 pass, 5 skip, 0 fail across 78 files.
• bun run typecheck, bun run format, bun run build all clean. bun run lint reports the same 25 warnings that are already on main.

[davidmckayv]

Closing this one, and I want to be specific about why, because the bug you found in #53 is real and worth fixing — it is the chosen remedy I cannot take.

The rollback can leave no working credential at all.

revoke (server/src/credentials.ts:171-183) throws on a row-absent result or on any database fault. A fault can happen after the UPDATE commits: a statement timeout, a connection reset, the pool being torn down as the response returns. In that case the old credential really is revoked, this PR's compensation then revokes the new one too, and the (kind, provider, keyId) triple is left with nothing live.

For a model credential that degrades quietly to the OPENAI_API_KEY environment variable (credentials.ts:149). For connector, agent and mcp there is no fallback and the plaintext is unrecoverable, so it is a hard lockout: somebody has to go and find the secret again. Trading an orphaned old credential for no credential is the wrong direction — the safe ordering leaves a working credential even in the bad case.

The compensation is silent, and it is silent exactly when it matters.

await service.store.revoke(credential.id).catch(() => {}) (credentials.ts:314) swallows its own failure. The database fault that broke the first revoke is very likely to break the second, and when it does there is no log, no audit event and no metric — you are back to the pre-fix orphan state with nothing saying so. That is worse than the original bug, which at least left a detectable orphan.

It does not survive the process dying.

If the server is killed between the write and the revoke — a rolling-deploy SIGTERM, an OOM — the compensation never runs and the orphan persists exactly as it does on main. A compensating action that only exists in memory cannot fix a durability problem. That case is the common one in a deployment that rolls several times a day.

The one-step fix is available here.

The PR body gives as its reason that create and revoke cannot be made atomic. That is not the case: both rows are in the same Postgres table, createCredentialStore (credentials.ts:153) is the only real CredentialStore implementation, and this repository already does exactly this for the analogous agent-key path — database.transaction with FOR UPDATE row locks in server/src/agents/profile-store.ts:188 and :338. A single transaction wrapping the insert and the revoke removes the orphan, needs no compensation, and survives the process dying, because Postgres rolls it back rather than your code.

Two things worth folding in while you are there, both surfaced reviewing this:

• The rollback calls store.revoke directly and bypasses recordAuditEvent, unlike revokeCredential (credentials.ts:339-347), so a failed rotation still writes zero audit rows. The trail is as silent about the failure as it was before, which is the other half of rotateCredential leaves an orphan credential if the revoke of the previous one fails #53.
• Nothing serialises two replicas rotating the same key. There is no partial unique index on (kind, provider, key_id) WHERE revoked_at IS NULL (server/drizzle/0000_schema.sql:110-120), and revoke has no isNull(revokedAt) guard, so it re-stamps an already-revoked row and cannot distinguish "I revoked it" from "another replica did". Two concurrent rotations both insert and both compensate against the same target. That index is worth adding regardless of how the rotation is written.

Credit where it is due: the secret handling on this path is clean — nothing is logged, and no plaintext reaches an audit payload or the rethrown error. If you want to send the transaction version I will review it quickly.