From 482f64e343786154cd15b144494a4829e3ebdf4c Mon Sep 17 00:00:00 2001 From: Jannes Stubbemann Date: Fri, 12 Jun 2026 02:47:17 +0200 Subject: [PATCH] fix(plugin-kubernetes): resolve sandbox pod by exact name (controller labels pods with sandbox-name-hash, not sandbox-name) (#7982) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path Production e2e on the merged #5790 plugin failed on every fresh lease with "Failed to install the adapter runtime command" for a harness that was present in the runtime image. Tracing the lease showed the first exec resolved no pod: the exact-label fallback added during the #5790 review queries `agents.x-k8s.io/sandbox-name=`, but the kubernetes-sigs agent-sandbox controller labels pods only with `agents.x-k8s.io/sandbox-name-hash` (see `sandboxLabel` in its `controllers/sandbox_controller.go`) and NAMES the backing pod exactly after the Sandbox CR. The selector matches nothing, `findPodForSandbox` returns null, execute returns "podName could not be resolved", and adapter-utils misreports it as a missing runtime command. ## What Changed Between the `status.podName` read and the label fallback, try an exact-name pod GET (`readNamespacedPod({namespace, name})`). This is collision-free, so the original review concern (name-prefix matching execing into a concurrent sandbox's pod) stays honored. A 404 falls through to the existing full-name label selector for controller versions that do set such a label. Non-404 errors propagate unchanged. ## Verification - New unit test pins the controller reality: pod named exactly like the sandbox, only a `sandbox-name-hash` label, no full-name label; fails before the fix, passes after. - Review-feedback round: the primary-path test now asserts the exact-name GET is never called, and a new test covers non-404 error propagation (403 rejects, no fallback). 153/153 plugin tests green, tsc clean. - Production-verified on our deployment: agent runs were broken on every fresh lease before this patch and complete end-to-end after it (gVisor sandbox pool, agent-sandbox controller v0.4.6; verified run with cost event and agent reply on a fresh tenant). ## Risks Low: one additional pod GET per first-exec on a fresh lease, only when `status.podName` is unset. Non-404 errors from the GET propagate unchanged (now test-pinned). ## Issue No existing issue; the defect is described in full under Thinking Path (introduced by the review-round fallback change in #5790, first hit in production e2e on 2026-06-11). ## Model Used Claude Fable 5 (claude-fable-5, Claude Code CLI, extended reasoning, tool use) ## Duplicate search Searched open and closed PRs for `findPodForSandbox`, `sandbox-name-hash`, and pod-resolution fixes; no duplicate found. Related parent: #5790 (introduced the fallback this PR repairs). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots (no UI change) - [x] I have updated relevant documentation to reflect my changes (code comments; no doc surface affected) - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- .../kubernetes/src/sandbox-cr-orchestrator.ts | 25 ++++++++++- .../test/unit/sandbox-cr-orchestrator.test.ts | 43 ++++++++++++++++--- 2 files changed, 61 insertions(+), 7 deletions(-) diff --git a/packages/plugins/sandbox-providers/kubernetes/src/sandbox-cr-orchestrator.ts b/packages/plugins/sandbox-providers/kubernetes/src/sandbox-cr-orchestrator.ts index 668286e3..b38062e5 100644 --- a/packages/plugins/sandbox-providers/kubernetes/src/sandbox-cr-orchestrator.ts +++ b/packages/plugins/sandbox-providers/kubernetes/src/sandbox-cr-orchestrator.ts @@ -152,8 +152,29 @@ export async function findPodForSandbox( return podName; } - // Fallback: list pods by the controller's sandbox-name label, which uniquely - // identifies the pod for THIS sandbox. A broader managed-by selector plus + // Secondary: the agent-sandbox controller (v0.4.x) names the backing pod + // EXACTLY after the Sandbox CR and labels it only with + // agents.x-k8s.io/sandbox-name-hash (a hash, not the full name), so the + // full-name label selector below matches nothing on that controller. An + // exact-name GET is collision-free (unlike name-prefix matching, which + // could hit a concurrent sandbox sharing a prefix) and resolves the pod on + // every controller version that keeps pod name == sandbox name. + try { + const pod = (await clients.core.readNamespacedPod({ namespace, name })) as { + metadata?: { name?: string }; + }; + if (pod?.metadata?.name) { + return pod.metadata.name; + } + } catch (err) { + const code = + (err as { code?: number; statusCode?: number }).code ?? + (err as { code?: number; statusCode?: number }).statusCode; + if (code !== 404) throw err; + } + + // Fallback: list pods by a full-name sandbox label, for controller versions + // that label pods with the sandbox name. A broader managed-by selector plus // name-prefix narrowing could match a concurrent sandbox whose generated // name shares a prefix, and exec would target the wrong lease's pod. const result = await clients.core.listNamespacedPod({ diff --git a/packages/plugins/sandbox-providers/kubernetes/test/unit/sandbox-cr-orchestrator.test.ts b/packages/plugins/sandbox-providers/kubernetes/test/unit/sandbox-cr-orchestrator.test.ts index bdd25137..0237d941 100644 --- a/packages/plugins/sandbox-providers/kubernetes/test/unit/sandbox-cr-orchestrator.test.ts +++ b/packages/plugins/sandbox-providers/kubernetes/test/unit/sandbox-cr-orchestrator.test.ts @@ -101,16 +101,35 @@ describe("findPodForSandbox", () => { const get = vi.fn().mockResolvedValue(makeCr("Ready", "pc-abc-pod-xyz")); const clients = { custom: { getNamespacedCustomObject: get }, - core: { listNamespacedPod: vi.fn() }, + core: { readNamespacedPod: vi.fn(), listNamespacedPod: vi.fn() }, }; const podName = await findPodForSandbox(clients as never, "ns", "pc-abc"); expect(podName).toBe("pc-abc-pod-xyz"); - // Should NOT have called listNamespacedPod (primary path succeeded) + // Primary path succeeded: neither the exact-name GET nor the label list runs. + expect(clients.core.readNamespacedPod).not.toHaveBeenCalled(); expect(clients.core.listNamespacedPod).not.toHaveBeenCalled(); }); + it("resolves the pod by EXACT NAME when the controller names it after the sandbox (v0.4.x: pods carry only agents.x-k8s.io/sandbox-name-hash, never the full-name label)", async () => { + const get = vi.fn().mockResolvedValue(makeCr("Ready")); // no podName in status + const read = vi.fn().mockResolvedValue({ + metadata: { name: "pc-abc", labels: { "agents.x-k8s.io/sandbox-name-hash": "1a2b3c" } }, + status: { phase: "Running" }, + }); + const list = vi.fn().mockResolvedValue({ items: [] }); // full-name label selector matches nothing on v0.4.x + const clients = { + custom: { getNamespacedCustomObject: get }, + core: { readNamespacedPod: read, listNamespacedPod: list }, + }; + const podName = await findPodForSandbox(clients as never, "ns", "pc-abc"); + expect(read).toHaveBeenCalledWith({ namespace: "ns", name: "pc-abc" }); + expect(podName).toBe("pc-abc"); + expect(list).not.toHaveBeenCalled(); + }); + it("falls back to pod listing scoped by the unique sandbox-name label", async () => { const get = vi.fn().mockResolvedValue(makeCr("Pending")); // no podName + const read = vi.fn().mockRejectedValue({ code: 404 }); // no exact-name pod const list = vi.fn().mockResolvedValue({ items: [ { @@ -121,7 +140,7 @@ describe("findPodForSandbox", () => { }); const clients = { custom: { getNamespacedCustomObject: get }, - core: { listNamespacedPod: list }, + core: { readNamespacedPod: read, listNamespacedPod: list }, }; const podName = await findPodForSandbox(clients as never, "ns", "pc-abc"); expect(list).toHaveBeenCalledWith( @@ -143,18 +162,32 @@ describe("findPodForSandbox", () => { }); const clients = { custom: { getNamespacedCustomObject: get }, - core: { listNamespacedPod: list }, + core: { readNamespacedPod: vi.fn().mockRejectedValue({ code: 404 }), listNamespacedPod: list }, }; const podName = await findPodForSandbox(clients as never, "ns", "pc-abc"); expect(podName).toBeNull(); }); + it("propagates non-404 errors from the exact-name pod GET instead of falling through", async () => { + const get = vi.fn().mockResolvedValue(makeCr("Pending")); + const read = vi.fn().mockRejectedValue({ code: 403, message: "forbidden" }); + const list = vi.fn(); + const clients = { + custom: { getNamespacedCustomObject: get }, + core: { readNamespacedPod: read, listNamespacedPod: list }, + }; + await expect(findPodForSandbox(clients as never, "ns", "pc-abc")).rejects.toMatchObject({ + code: 403, + }); + expect(list).not.toHaveBeenCalled(); + }); + it("returns null when no pod is found in fallback", async () => { const get = vi.fn().mockResolvedValue(makeCr("Pending")); const list = vi.fn().mockResolvedValue({ items: [] }); const clients = { custom: { getNamespacedCustomObject: get }, - core: { listNamespacedPod: list }, + core: { readNamespacedPod: vi.fn().mockRejectedValue({ code: 404 }), listNamespacedPod: list }, }; const podName = await findPodForSandbox(clients as never, "ns", "pc-abc"); expect(podName).toBeNull();