From 4f74f4189daf7bbfd42334b850243197b7cebe13 Mon Sep 17 00:00:00 2001 From: Cole Murray Date: Sat, 25 Apr 2026 23:11:07 -0700 Subject: [PATCH 1/2] fix: fail sandbox spawn when secrets cannot be loaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, failures to load global or repo secrets were caught and swallowed with a warning, allowing the sandbox to proceed without user-configured secrets. This is no longer acceptable — missing secrets should prevent the sandbox from spawning. Remove the try/catch wrappers in getUserEnvVars() so errors propagate to the caller (lifecycle manager), which already handles spawn failures. --- .../src/session/durable-object.ts | 28 ++++--------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/packages/control-plane/src/session/durable-object.ts b/packages/control-plane/src/session/durable-object.ts index d535f215a..ef5d4534d 100644 --- a/packages/control-plane/src/session/durable-object.ts +++ b/packages/control-plane/src/session/durable-object.ts @@ -1686,30 +1686,14 @@ export class SessionDO extends DurableObject { return undefined; } - // Fetch global secrets - let globalSecrets: Record = {}; - try { - const globalStore = new GlobalSecretsStore(this.env.DB, this.env.REPO_SECRETS_ENCRYPTION_KEY); - globalSecrets = await globalStore.getDecryptedSecrets(); - } catch (e) { - this.log.error("Failed to load global secrets, proceeding without", { - error: e instanceof Error ? e.message : String(e), - }); - } + // Fetch global secrets (fail hard — secrets are required for sandbox operation) + const globalStore = new GlobalSecretsStore(this.env.DB, this.env.REPO_SECRETS_ENCRYPTION_KEY); + const globalSecrets = await globalStore.getDecryptedSecrets(); // Fetch repo secrets - let repoSecrets: Record = {}; - try { - const repoId = await this.ensureRepoId(session); - const repoStore = new RepoSecretsStore(this.env.DB, this.env.REPO_SECRETS_ENCRYPTION_KEY); - repoSecrets = await repoStore.getDecryptedSecrets(repoId); - } catch (e) { - this.log.warn("Failed to load repo secrets, proceeding without", { - repo_owner: session.repo_owner, - repo_name: session.repo_name, - error: e instanceof Error ? e.message : String(e), - }); - } + const repoId = await this.ensureRepoId(session); + const repoStore = new RepoSecretsStore(this.env.DB, this.env.REPO_SECRETS_ENCRYPTION_KEY); + const repoSecrets = await repoStore.getDecryptedSecrets(repoId); // Merge: repo overrides global const { merged, totalBytes, exceedsLimit } = mergeSecrets(globalSecrets, repoSecrets); From e96fbec43aa957b85b04dc517e6b991834e15192 Mon Sep 17 00:00:00 2001 From: Cole Murray Date: Sat, 25 Apr 2026 23:34:29 -0700 Subject: [PATCH 2/2] fix: keep ensureRepoId in fail-fast path, add regression test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback: - Keep ensureRepoId() in the fail-fast path — if we can't resolve the repo ID, repo secrets would be silently dropped, defeating the purpose of this change. Failing here is the correct behavior. - Add regression test verifying that getUserEnvVars() rejection during spawn correctly fails the sandbox and resets isSpawning. --- .../src/sandbox/lifecycle/manager.test.ts | 27 +++++++++++++++++++ .../src/session/durable-object.ts | 3 +-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/packages/control-plane/src/sandbox/lifecycle/manager.test.ts b/packages/control-plane/src/sandbox/lifecycle/manager.test.ts index 960241cf5..460b0c5aa 100644 --- a/packages/control-plane/src/sandbox/lifecycle/manager.test.ts +++ b/packages/control-plane/src/sandbox/lifecycle/manager.test.ts @@ -679,6 +679,33 @@ describe("SandboxLifecycleManager", () => { expect(storage.calls).toContain("updateSandboxStatus:failed"); }); + it("fails spawn when getUserEnvVars rejects", async () => { + const sandbox = createMockSandbox({ status: "pending", created_at: Date.now() - 60000 }); + const storage = createMockStorage(createMockSession(), sandbox); + storage.getUserEnvVars = vi.fn(async () => { + throw new Error("D1 decryption failure"); + }); + const broadcaster = createMockBroadcaster(); + const wsManager = createMockWebSocketManager(false); + const provider = createMockProvider(); + + const manager = new SandboxLifecycleManager( + provider, + storage, + broadcaster, + wsManager, + createMockAlarmScheduler(), + createMockIdGenerator(), + createTestConfig() + ); + + await manager.spawnSandbox(); + + expect(provider.createSandbox).not.toHaveBeenCalled(); + expect(storage.calls).toContain("updateSandboxStatus:failed"); + expect(manager.isSpawning()).toBe(false); + }); + it("skips spawn when already spawning", async () => { const sandbox = createMockSandbox({ status: "spawning" }); const storage = createMockStorage(createMockSession(), sandbox); diff --git a/packages/control-plane/src/session/durable-object.ts b/packages/control-plane/src/session/durable-object.ts index ef5d4534d..eaae33cf5 100644 --- a/packages/control-plane/src/session/durable-object.ts +++ b/packages/control-plane/src/session/durable-object.ts @@ -1686,11 +1686,10 @@ export class SessionDO extends DurableObject { return undefined; } - // Fetch global secrets (fail hard — secrets are required for sandbox operation) + // Fail hard on secret loading — sandboxes must not silently lose secrets const globalStore = new GlobalSecretsStore(this.env.DB, this.env.REPO_SECRETS_ENCRYPTION_KEY); const globalSecrets = await globalStore.getDecryptedSecrets(); - // Fetch repo secrets const repoId = await this.ensureRepoId(session); const repoStore = new RepoSecretsStore(this.env.DB, this.env.REPO_SECRETS_ENCRYPTION_KEY); const repoSecrets = await repoStore.getDecryptedSecrets(repoId);