From c484260666dca826eb93e6fb1b394eddb142817f Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Tue, 28 Jul 2026 14:47:33 -0400 Subject: [PATCH] fix: wait out a twin's in-flight sidecar removal instead of dying (SYD-297) SYD-270 gave the `docker rm -f syd-egress` step race handling, but the re-inspect fired once. A twin mid-removal keeps answering `docker inspect` with the container it is still tearing down -- carrying ITS allowlist, not ours -- so the single look saw a non-null, non-satisfying proxy and took `if (proxy) throw err`. The worker then exited 1. Not theoretical: both engine workers died that way during the SYD-269 go-live (18:37:33 base created the sidecar without github.com, codex and gemini both raced the rm, both FATAL'd). Steady state was still correct at 18:38:04, but only because launchd relaunched them -- a manual run or init-worker would simply have died, and every coordinated restart wrote two spurious FATALs into the logs. The single look could not distinguish "a removal is in flight" from "a wrong sidecar is genuinely stuck". Waiting can: the first ends, the second doesn't. settleAfterLostRm re-inspects on a ~3s schedule, settling early when the container is gone (ours to recreate) or when a twin replaced it with one that already covers us, and otherwise letting the original throw stand. `sleep` is injectable so tests run the whole schedule without spending it -- the pre-existing SYD-270 "still fails when a wrong sidecar is standing" case now exercises the exhausted schedule and needed it, or it would have added 3.1s to the suite. Also fixed that FATAL's remediation text, which advised rebuilding the proxy image or opting out to `egress: "open"` -- neither has anything to do with a lost race, and pointing at the full-egress escape hatch is bad advice to hand someone reading a transient failure. --- scripts/agent-worker.ts | 6 ++- scripts/worker-select.ts | 40 +++++++++++++++++-- tests/scripts/worker-select.test.ts | 62 ++++++++++++++++++++++++++++- 3 files changed, 102 insertions(+), 6 deletions(-) diff --git a/scripts/agent-worker.ts b/scripts/agent-worker.ts index b80e55f..6ca9f19 100644 --- a/scripts/agent-worker.ts +++ b/scripts/agent-worker.ts @@ -1688,8 +1688,10 @@ async function main(): Promise { } catch (err) { console.error( `FATAL: could not set up the egress guard (SYD-110): ${(err as Error).message}\n` + - 'Build the proxy image with `npm run build:worker-image`, or set egress: "open" ' + - "in switchyard-worker.json to explicitly opt out of the allowlist.", + "If the sidecar image is missing, build it with `npm run build:worker-image`; if a " + + "stale syd-egress container is wedged, `docker rm -f syd-egress` and restart. Set " + + 'egress: "open" in switchyard-worker.json only to deliberately opt out of the ' + + "allowlist — it gives sessions full egress.", ); process.exit(1); } diff --git a/scripts/worker-select.ts b/scripts/worker-select.ts index 20db2ed..4bf19d3 100644 --- a/scripts/worker-select.ts +++ b/scripts/worker-select.ts @@ -1033,7 +1033,10 @@ export async function ensureEgressGuard( config: WorkerConfig, exec: ExecFn, env: NodeJS.ProcessEnv, + opts: { sleep?: (ms: number) => Promise } = {}, ): Promise { + const sleep = + opts.sleep ?? ((ms: number) => new Promise((resolve) => setTimeout(resolve, ms))); // Several processes ensure concurrently at boot (deliver + both workers // kickstart together — observed live 2026-07-11): every mutating step below // races an identical twin, so a failure only counts if the desired state @@ -1059,6 +1062,27 @@ export async function ensureEgressGuard( return null; } }; + /** + * Re-inspects until a lost `rm -f` race resolves one way or the other + * (SYD-297), returning the last state seen. Settles early on either outcome + * that means we're fine — the container is gone (null, ours to recreate) or + * a twin replaced it with one that already covers us — and otherwise keeps + * looking until the schedule runs out, at which point a still-standing + * sidecar is a genuine failure rather than a removal in flight. + * + * Sub-second in practice: a docker removal plus recreate is far quicker than + * this schedule, which totals ~3s only to leave room for a loaded host. + */ + const settleAfterLostRm = async (): Promise>> => { + const waitsMs = [100, 200, 400, 800, 1600]; + let seen = await inspectProxy(); + for (const ms of waitsMs) { + if (seen === null || satisfied(seen)) return seen; + await sleep(ms); + seen = await inspectProxy(); + } + return seen; + }; const networkExists = async (): Promise => { try { await exec("docker", ["network", "inspect", EGRESS_NETWORK]); @@ -1121,9 +1145,19 @@ export async function ensureEgressGuard( // there first (SYD-270). This was the one mutating step with no // race handling, so it turned a won race into a FATAL. Only a real // failure if a wrong sidecar is still standing when we look again. - proxy = await inspectProxy(); - if (satisfied(proxy)) return; - if (proxy) throw err; + // + // Look REPEATEDLY, not once (SYD-297). A twin mid-`rm` keeps answering + // `docker inspect` with the container it is still tearing down, and that + // container carries the twin's allowlist rather than ours — so a single + // look sees a non-null, non-satisfying proxy and takes the throw below. + // Both engine workers exited 1 that way during the SYD-269 go-live, and + // only launchd's relaunch got them back; anything without a supervisor + // (a manual run, init-worker) would simply have died. Waiting resolves + // the ambiguity the single look couldn't: a removal in flight ends, a + // genuinely stuck sidecar doesn't. + proxy = await settleAfterLostRm(); + if (satisfied(proxy)) return; // the twin stood up one that covers us + if (proxy) throw err; // still there after the whole schedule — real } } try { diff --git a/tests/scripts/worker-select.test.ts b/tests/scripts/worker-select.test.ts index 7f9583c..20ef9c5 100644 --- a/tests/scripts/worker-select.test.ts +++ b/tests/scripts/worker-select.test.ts @@ -742,6 +742,8 @@ describe("ensureEgressGuard (SYD-110)", () => { // SYD-186: the sidecar now also injects provider creds; env supplies them and // seeds the INJECT_KEYS freshness sentinel (CLAUDE_CODE_OAUTH_TOKEN here). const egressEnv = { CLAUDE_CODE_OAUTH_TOKEN: "sk-ant-oat-REAL" } as NodeJS.ProcessEnv; + /** Runs the SYD-297 wait schedule without spending its wall-clock. */ + const noSleep = async () => {}; it("creates the internal network and starts+connects the proxy when both are missing", async () => { const { calls, exec } = mockExec(({ args }) => { @@ -867,7 +869,12 @@ describe("ensureEgressGuard (SYD-110)", () => { return ""; }); // Nothing healed it, so this is a real failure and must not be swallowed. - await expect(ensureEgressGuard(config, exec, egressEnv)).rejects.toThrow(/removal/i); + // SYD-297 made the re-inspect a schedule rather than a single look, so this + // now throws only after that schedule is exhausted — noSleep runs it + // without spending the wall-clock. + await expect(ensureEgressGuard(config, exec, egressEnv, { sleep: noSleep })).rejects.toThrow( + /removal/i, + ); }); it("survives losing the network-create race to a concurrently starting worker", async () => { @@ -917,6 +924,59 @@ describe("ensureEgressGuard (SYD-110)", () => { expect(calls.some((c) => c.args[0] === "network" && c.args[1] === "connect")).toBe(false); }); + // SYD-297: the sibling of the run-race above, for `docker rm -f`. SYD-270 + // added handling here but the re-inspect fires ONCE — and a twin mid-removal + // still answers `docker inspect` with its own (unsatisfying) sidecar, so the + // single look took `if (proxy) throw err` and the worker exited 1. Observed + // live on both engine workers during the SYD-269 go-live. + const rmInProgress = () => + new Error("Error response from daemon: removal of container syd-egress is already in progress"); + + it("survives losing the proxy-rm race: waits out a twin's in-flight removal", async () => { + let proxyInspects = 0; + const { calls, exec } = mockExec(({ args }) => { + if (args[0] === "network") return "[]"; + if (args[0] === "inspect") { + proxyInspects++; + // 1: the stale sidecar we want gone. 2-3: the twin's removal is still + // in flight, so it keeps answering. 4+: gone. + return proxyInspects <= 3 + ? "true ALLOWED_DOMAINS=api.anthropic.com INJECT_KEYS=CLAUDE_CODE_OAUTH_TOKEN" + : new Error("no such container"); + } + if (args[0] === "rm") return rmInProgress(); + return ""; + }); + await expect( + ensureEgressGuard(config, exec, egressEnv, { sleep: noSleep }), + ).resolves.toBeUndefined(); + // It still has to create the replacement — waiting out the twin is not the + // same as accepting whatever it left behind. + expect(calls.some((c) => c.args[0] === "run")).toBe(true); + }); + + it("survives losing the proxy-rm race: accepts the twin's replacement if it satisfies", async () => { + let proxyInspects = 0; + const { calls, exec } = mockExec(({ args }) => { + if (args[0] === "network") return "[]"; + if (args[0] === "inspect") { + proxyInspects++; + if (proxyInspects === 1) + return "true ALLOWED_DOMAINS=api.anthropic.com INJECT_KEYS=CLAUDE_CODE_OAUTH_TOKEN"; + // The twin finished and stood up a sidecar that covers what we need. + return proxyInspects === 2 + ? "true ALLOWED_DOMAINS=api.anthropic.com INJECT_KEYS=CLAUDE_CODE_OAUTH_TOKEN" + : `true ALLOWED_DOMAINS=${domainsCsv} INJECT_KEYS=CLAUDE_CODE_OAUTH_TOKEN`; + } + if (args[0] === "rm") return rmInProgress(); + return ""; + }); + await expect( + ensureEgressGuard(config, exec, egressEnv, { sleep: noSleep }), + ).resolves.toBeUndefined(); + expect(calls.some((c) => c.args[0] === "run")).toBe(false); + }); + it("tolerates an already-connected proxy on network connect", async () => { const { exec } = mockExec(({ args }) => { if (args[0] === "network" && args[1] === "inspect") return new Error("no such network");