fix(storage): ServerCredentialStore put-race + vault orphan on rollback failure (sec)

packages/storage/src/credentials/ServerCredentialStore.test.ts

@@ -4,7 +4,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { describe, expect, it } from "vitest";
+import { describe, expect, it, vi } from "vitest";
 import type { IKvStorage } from "../kv/IKvStorage";
 import { InMemoryKvStorage } from "../kv/InMemoryKvStorage";
 import type { SecretVault } from "./SecretVault";
@@ -113,4 +113,254 @@ describe("ServerCredentialStore", () => {
     const remaining = (await meta.getAll()) ?? [];
     expect(remaining).toHaveLength(0);
   });
+
+  it("update-in-flight: a concurrent get() returns the prior committed value (never the not-yet-committed new value, never undefined)", async () => {
+    // A vault whose setSecret blocks until we explicitly resolve it, but only
+    // for ids that already have a value (i.e., updates, not the initial seed).
+    const map = new Map<string, string>();
+    let releaseSet: (() => void) | undefined;
+    const blockedSet = new Promise<void>((resolve) => {
+      releaseSet = resolve;
+    });
+    const vault: SecretVault = {
+      async setSecret(id, v) {
+        // First put commits synchronously; subsequent puts block on the gate
+        // BEFORE writing the map, so map still holds the prior value while
+        // the in-flight put is suspended.
+        if (map.has(id)) {
+          await blockedSet;
+        }
+        map.set(id, v);
+      },
+      async getSecret(id) {
+        return map.get(id);
+      },
+      async deleteSecret(id) {
+        map.delete(id);
+      },
+    };
+    const meta: IKvStorage<string, CredentialMetadataRow> = new InMemoryKvStorage();
+    const store = new ServerCredentialStore({
+      vault,
+      metadata: meta,
+      userId: "u1",
+      projectId: "p1",
+    });
+
+    // Seed a prior committed value (this is now an existing-row update on the
+    // second put — metadata is non-pending so get() must succeed).
+    await store.put("k", "v1");
+
+    // Start a second put without awaiting; it will block inside setSecret.
+    const inflight = store.put("k", "v2");
+
+    // While the update is in flight, get() must return the OLD vault value
+    // (metadata is committed and non-pending; vault.setSecret hasn't yet
+    // written the new value because it's blocked on the gate). Critically:
+    // not the new value, and not undefined.
+    const observed = await store.get("k");
+    expect(observed).toBe("v1");
+
+    // Release the gate and let the in-flight put finish.
+    releaseSet!();
+    await inflight;
+    expect(await store.get("k")).toBe("v2");
+  });
+
+  it("new-entry put: metadata write fails — vault is never touched", async () => {
+    const vault = makeVault();
+    const deleteSpy = vi.spyOn(vault, "deleteSecret");
+    const setSpy = vi.spyOn(vault, "setSecret");
+    // Failing metadata: every put() rejects. get/getAll/delete still work.
+    const inner: IKvStorage<string, CredentialMetadataRow> = new InMemoryKvStorage();
+    const meta: IKvStorage<string, CredentialMetadataRow> = Object.create(inner);
+    meta.put = vi.fn(async () => {
+      throw new Error("metadata put boom");
+    }) as typeof inner.put;
+    meta.get = (key) => inner.get(key);
+    meta.getAll = () => inner.getAll();
+    meta.delete = (key) => inner.delete(key);
+
+    const store = new ServerCredentialStore({
+      vault,
+      metadata: meta,
+      userId: "u1",
+      projectId: "p1",
+    });
+
+    await expect(store.put("k", "v")).rejects.toThrow();
+    // Vault must hold no value (the implementation writes metadata first, so
+    // a metadata.put failure on a NEW entry never reaches vault.setSecret).
+    expect(await vault.getSecret("u1/p1/k")).toBeUndefined();
+    expect(setSpy).not.toHaveBeenCalled();
+    expect(deleteSpy).not.toHaveBeenCalled();
+  });
+
+  it("new-entry put: metadata write fails AND vault rollback fails — throws wrapped error and leaves orphan marker", async () => {
+    // Vault.setSecret throws to trigger rollback; metadata.delete also throws
+    // so the rollback path persists an orphan marker.
+    const vault: SecretVault = {
+      async setSecret() {
+        throw new Error("vault boom");
+      },
+      async getSecret() {
+        return undefined;
+      },
+      async deleteSecret() {
+        // No-op (won't be reached on this path).
+      },
+    };
+    const inner: IKvStorage<string, CredentialMetadataRow> = new InMemoryKvStorage();
+    const meta: IKvStorage<string, CredentialMetadataRow> = Object.create(inner);
+    meta.put = (key, value) => inner.put(key, value);
+    meta.get = (key) => inner.get(key);
+    meta.getAll = () => inner.getAll();
+    meta.delete = vi.fn(async () => {
+      throw new Error("metadata delete boom");
+    }) as typeof inner.delete;
+
+    const store = new ServerCredentialStore({
+      vault,
+      metadata: meta,
+      userId: "u1",
+      projectId: "p1",
+    });
+
+    let caught: unknown;
+    try {
+      await store.put("k", "v");
+    } catch (e) {
+      caught = e;
+    }
+    expect(caught).toBeInstanceOf(Error);
+    expect((caught as Error).message).toContain("rollback");
+    expect((caught as Error).message).toContain("u1/p1/k");
+    expect((caught as Error & { cause?: unknown }).cause).toBeInstanceOf(Error);
+    expect(((caught as Error & { cause?: Error }).cause as Error).message).toBe("vault boom");
+
+    const row = await inner.get("u1/p1/k");
+    expect(row).toBeDefined();
+    expect(row!.pending).toBe(true);
+    expect(typeof row!.orphanedAt).toBe("string");
+    expect(row!.orphanedAt!.length).toBeGreaterThan(0);
+  });
+
+  it("new-entry put: commit-step metadata write fails — vault retained, orphan marker persisted, wrapped error thrown", async () => {
+    // First metadata.put (pending:true) succeeds; vault.setSecret succeeds;
+    // second metadata.put (commit pending:false) throws; third metadata.put
+    // (orphan marker) succeeds.
+    const vault = makeVault();
+    const inner: IKvStorage<string, CredentialMetadataRow> = new InMemoryKvStorage();
+    let putCount = 0;
+    const meta: IKvStorage<string, CredentialMetadataRow> = Object.create(inner);
+    meta.put = vi.fn(async (key: string, value: CredentialMetadataRow) => {
+      putCount++;
+      if (putCount === 2) throw new Error("commit boom");
+      return inner.put(key, value);
+    }) as typeof inner.put;
+    meta.get = (key) => inner.get(key);
+    meta.getAll = () => inner.getAll();
+    meta.delete = (key) => inner.delete(key);
+
+    const store = new ServerCredentialStore({
+      vault,
+      metadata: meta,
+      userId: "u1",
+      projectId: "p1",
+    });
+
+    let caught: unknown;
+    try {
+      await store.put("k", "v");
+    } catch (e) {
+      caught = e;
+    }
+    expect(caught).toBeInstanceOf(Error);
+    expect((caught as Error).message).toContain("commit failed");
+    expect((caught as Error).message).toContain("u1/p1/k");
+    expect((caught as Error & { cause?: unknown }).cause).toBeInstanceOf(Error);
+    expect(((caught as Error & { cause?: Error }).cause as Error).message).toBe("commit boom");
+
+    // Vault still holds the bytes — commit-step failures do not roll back.
+    expect(await vault.getSecret("u1/p1/k")).toBe("v");
+    // Metadata row is a sticky orphan marker.
+    const row = await inner.get("u1/p1/k");
+    expect(row).toBeDefined();
+    expect(row!.pending).toBe(true);
+    expect(typeof row!.orphanedAt).toBe("string");
+    expect(row!.orphanedAt!.length).toBeGreaterThan(0);
+  });
+
+  it("pending row is invisible to get/has/listMetadata/keys", async () => {
+    const { store, meta } = makeStore();
+    // Seed a pending row directly via the metadata KV — bypassing put() so
+    // it stays in the pending state.
+    await meta.put("u1/p1/ghost", {
+      userId: "u1",
+      projectId: "p1",
+      key: "ghost",
+      label: undefined,
+      provider: undefined,
+      createdAt: new Date().toISOString(),
+      updatedAt: new Date().toISOString(),
+      expiresAt: undefined,
+      pending: true,
+    });
+
+    expect(await store.get("ghost")).toBeUndefined();
+    expect(await store.has("ghost")).toBe(false);
+    expect(await store.listMetadata()).toEqual([]);
+    expect(await store.keys()).toEqual([]);
+  });
+
+  it("deleteAll scope isolation", async () => {
+    const meta: IKvStorage<string, CredentialMetadataRow> = new InMemoryKvStorage();
+    const vault = makeVault();
+    const metaDeleteSpy = vi.spyOn(meta, "delete");
+    const vaultDeleteSpy = vi.spyOn(vault, "deleteSecret");
+
+    const u1p1 = new ServerCredentialStore({
+      vault,
+      metadata: meta,
+      userId: "u1",
+      projectId: "p1",
+    });
+    const u1p2 = new ServerCredentialStore({
+      vault,
+      metadata: meta,
+      userId: "u1",
+      projectId: "p2",
+    });
+    const u2p1 = new ServerCredentialStore({
+      vault,
+      metadata: meta,
+      userId: "u2",
+      projectId: "p1",
+    });
+
+    await u1p1.put("a", "v");
+    await u1p2.put("a", "v");
+    await u2p1.put("a", "v");
+
+    // Reset spies after seed puts so we only observe deleteAll's activity.
+    metaDeleteSpy.mockClear();
+    vaultDeleteSpy.mockClear();
+
+    await u1p1.deleteAll();
+
+    const metaDeletedIds = metaDeleteSpy.mock.calls.map((args) => args[0] as string);
+    const vaultDeletedIds = vaultDeleteSpy.mock.calls.map((args) => args[0] as string);
+
+    for (const id of metaDeletedIds) {
+      expect(id.startsWith("u1/p1/")).toBe(true);
+    }
+    for (const id of vaultDeletedIds) {
+      expect(id.startsWith("u1/p1/")).toBe(true);
+    }
+
+    // The other scopes are untouched.
+    expect(await u1p2.get("a")).toBe("v");
+    expect(await u2p1.get("a")).toBe("v");
+    expect(await u1p1.get("a")).toBeUndefined();
+  });
 });