fix(storage): ServerCredentialStore orphan-reason discriminator + metadata-first delete

[sroussey]

Summary

Two high-priority correctness fixes for ServerCredentialStore in packages/storage/src/credentials/.

Fix 1 — orphanReason discriminator on orphan markers

CredentialMetadataRow.orphanedAt previously left operators guessing which failure branch produced a sticky orphan marker. This change adds an optional orphanReason field with one of three string-literal values:

• "vault-write-failed" — put() new-entry rollback path: metadata row written, vault.setSecret threw, and the follow-up metadata.delete also threw, so the pending row was rewritten as a sticky marker.
• "metadata-commit-failed" — put() new-entry commit path: vault.setSecret succeeded but the pending: true -> false flip threw, so the vault holds bytes the metadata can no longer expose.
• "vault-delete-failed" — delete() path (introduced by Fix 2 below): metadata.delete succeeded but vault.deleteSecret threw, so the vault may still hold bytes for a key no metadata row exposes.

A OrphanReason type alias is exported (NonNullable<CredentialMetadataRow["orphanReason"]>) for downstream consumers (reconciliation tooling, dashboards). The JSDoc on orphanedAt now points at orphanReason for branch semantics.

Fix 2 — delete() is metadata-first; deleteAll() aggregates per-id failures

Previously, delete() called vault.deleteSecret first and metadata.delete second. If the vault call threw, the row remained visible to readers but its secret was already gone — a torn state. The new ordering is:

1. Read the existing row; return false if absent.
2. await this.metadata.delete(id) — failure bubbles, vault untouched.
3. await this.vault.deleteSecret(id) — failure triggers a best-effort orphan marker (orphanReason: "vault-delete-failed") and throws a wrapped Error whose cause is the vault error.

delete() itself is now a thin wrapper around a private deleteById(id, key), and deleteAll() iterates scopedIds() calling the same path. Per-id errors are collected and surfaced as an AggregateError once every id has been attempted, so one bad row no longer aborts cleanup of the rest. The failing ids retain sticky orphan markers for operator reconciliation; succeeding ids are fully removed.

get() and has() self-eviction calls (await this.delete(key) on expiry) are now wrapped in try { ... } catch {} so the new throwy delete() cannot turn an expired-read miss into a thrown error for callers — preserving the prior swallow-on-eviction behaviour.

Backward compatibility

• orphanReason is optional. Legacy persisted rows with orphanedAt but no orphanReason continue to be treated as orphan markers (they are pending: true, so they were already invisible to readers). A regression test covers this case.
• Behaviour change: deleteAll() now throws AggregateError on per-id failure instead of throwing the first vault error and aborting the rest of the cleanup. Callers that previously only await-ed deleteAll() continue to see a throw on failure; callers that depended on early-abort semantics will need to update.
• Behaviour change: delete() now throws a wrapped Error (with cause) when vault.deleteSecret fails after metadata removal, instead of leaving a visible-but-empty row. The previous behaviour was a silent torn state; callers must now be prepared to handle this throw.

Files modified

• packages/storage/src/credentials/ServerCredentialStore.ts — orphanReason field + OrphanReason export, both put() branches tagged, new private deleteById(), rewritten delete() / deleteAll(), swallow-wrapped self-eviction in get() / has().
• packages/storage/src/credentials/ServerCredentialStore.test.ts — extended two existing orphan-path tests to assert orphanReason, added four new tests (delete() vault-failure, delete() metadata-failure, deleteAll() AggregateError, legacy orphan compatibility), updated the existing delete removes both test to verify metadata-before-vault ordering via spy call-order.

Test plan

• bun test packages/storage/src/credentials/ServerCredentialStore.test.ts passes locally
• Type-check (tsc --noEmit) clean across the package
• Lint clean on the two touched files
• Spot-check downstream consumers of CredentialMetadataRow for compile compatibility with the new optional field
• Confirm no caller in the repo depends on deleteAll() throwing the first vault error rather than AggregateError

---

Generated by Claude Code

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	61.95%	24614 / 39730
🔵	Statements	61.81%	25468 / 41200
🔵	Functions	62.92%	4662 / 7409
🔵	Branches	50.66%	12066 / 23813

File Coverage

No changed files found.

Generated in workflow #2454 for commit 1193ddd by the Vitest Coverage Report Action