From 6808bed75c6fe05cacb4ec7b99f6aa9a43d96f80 Mon Sep 17 00:00:00 2001 From: Nathan Date: Wed, 22 Jul 2026 02:24:47 +0200 Subject: [PATCH] =?UTF-8?q?fix(up):=20a=20fresh=20foreground=20`convoy=20u?= =?UTF-8?q?p`=20UN-PARKS=20its=20members=20=E2=80=94=20restore=20the=20FUL?= =?UTF-8?q?L=20fleet=20after=20an=20outage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Parking-recovery for the 2026-07-22 incident: a supervisor bring-up after a mass outage brought back only part of the fleet; the rest stayed PARKED from a prior supervisor's give-up and had to be hand-launched. Root cause: `strategy.status=flapping` + the fast-fail counter PERSIST to a session's tags (the on-disk supervision contract), so they outlive the supervisor that wrote them. An outage drives the cap to its limit → the agents park → and a fresh `convoy up`, reading those stale tags, hits classify's `isFlapping → skip` and never relaunches them. Fix: a foreground `convoy up` is a DELIBERATE bring-up — the operator gesture that says "restore the fleet" — so at startup it clears the park AND zeroes the fast-fail counter for permanent members (regardless of prior fail count), giving each a fresh cap budget. The cap still re-accrues tick-to-tick WITHIN this supervisor's watch (the real crash-loop protection). The `--once` shepherd cron does NOT un-park: it runs every few minutes, so un-parking there would relaunch a genuinely broken agent every tick — parking must stay durable for it. - flapping-cap.ts: `clearParkForFreshSupervisor` (pure) — the reset decision, unit-tested. - up.ts: the FRESH-SUPERVISOR UN-PARK startup pass (foreground only), which clears the on-disk park (removes the status tag — updateTags MERGES) and seeds the in-memory classifier state. - Tests: pure cases for the reset + a process-level proof (a parked, gone-but-recorded agent is UN-PARKED and RELAUNCHED by a fresh foreground up, while `--once` leaves it parked). Part 2 of 4 of the decoupling-hardening task. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_014gbfntB6cu21sL4YBp21LF --- src/flapping-cap.test.ts | 40 +++++++ src/flapping-cap.ts | 23 ++++ src/up-parking-recovery.test.ts | 186 ++++++++++++++++++++++++++++++++ src/up.ts | 28 +++++ 4 files changed, 277 insertions(+) create mode 100644 src/up-parking-recovery.test.ts diff --git a/src/flapping-cap.test.ts b/src/flapping-cap.test.ts index 935bfae..c892abb 100644 --- a/src/flapping-cap.test.ts +++ b/src/flapping-cap.test.ts @@ -5,6 +5,7 @@ import { describe, it, expect } from "vitest"; import { classify, classifyFailedAttempt, + clearParkForFreshSupervisor, commandFingerprint, FLAPPING_STATUS, effectiveLimit, @@ -197,3 +198,42 @@ describe("classifyFailedAttempt — a recovery attempt that never produced a lea if (parked.kind === "flap") expect(writtenTags(parked.tags)[TAG.status]).toBe(FLAPPING_STATUS); }); }); + +describe("clearParkForFreshSupervisor — a fresh foreground `convoy up` restores the FULL fleet (parking-recovery)", () => { + it("ACCEPTANCE: a PARKED member is un-parked — status cleared AND the counter zeroed (relaunchable again)", () => { + // The reproduced bug: an outage drives the cap to its limit → the agent parks → and a fresh supervisor, + // reading the persisted `status=flapping`, would `skip` it forever. A deliberate bring-up must not inherit + // that. Both fields reset: clearing status alone is not enough — a counter still at the cap re-parks on + // the very next fast fail. + const cleared = clearParkForFreshSupervisor(tags({ status: FLAPPING_STATUS, consecutiveFastFails: LIMIT })); + expect(cleared).not.toBeNull(); + expect(cleared?.status).toBeNull(); + expect(cleared?.consecutiveFastFails).toBe(0); + }); + + it("resets a NON-parked member with prior fails too — 'regardless of prior fail count' (Nathan mandate)", () => { + const cleared = clearParkForFreshSupervisor(tags({ status: null, consecutiveFastFails: LIMIT - 1 })); + expect(cleared?.consecutiveFastFails).toBe(0); + expect(cleared?.status).toBeNull(); + }); + + it("is a NO-OP for a clean member (no park, counter 0) — the caller writes no tag needlessly", () => { + expect(clearParkForFreshSupervisor(tags({ status: null, consecutiveFastFails: 0 }))).toBeNull(); + }); + + it("preserves the rest of the strategy state — only status + counter are touched", () => { + const before = tags({ status: FLAPPING_STATUS, consecutiveFastFails: LIMIT, commandHash: HASH_A, lastRespawnAt: at(500), fastFailLimitOverride: 5, fastFailWindowOverride: 120 }); + const cleared = clearParkForFreshSupervisor(before); + expect(cleared?.commandHash).toBe(HASH_A); + expect(cleared?.lastRespawnAt).toEqual(at(500)); + expect(cleared?.fastFailLimitOverride).toBe(5); + expect(cleared?.fastFailWindowOverride).toBe(120); + }); + + it("the written tags drop the park status and carry a zeroed counter (what up() persists to disk)", () => { + const cleared = clearParkForFreshSupervisor(tags({ status: FLAPPING_STATUS, consecutiveFastFails: LIMIT })); + const written = writtenTags(cleared!); + expect(written[TAG.status]).toBeUndefined(); // no park written — up() also REMOVES the on-disk status tag + expect(written[TAG.consecutive]).toBe("0"); + }); +}); diff --git a/src/flapping-cap.ts b/src/flapping-cap.ts index 1f6056c..e88af97 100644 --- a/src/flapping-cap.ts +++ b/src/flapping-cap.ts @@ -151,6 +151,29 @@ export function classifyFailedAttempt(input: { return { kind: "respawn", tags: { ...tags, lastRespawnAt: now, consecutiveFastFails: nextCounter, commandHash: currentHash, status: null } }; } +/** A fresh FOREGROUND supervisor gives every member a clean cap budget (convoy parking-recovery, 2026-07-22). + * + * The bug this fixes: `strategy.status=flapping` and the fast-fail counter PERSIST to the session's tags + * (the on-disk supervision contract), so they outlive the supervisor that wrote them. A mass outage drives + * the cap to its limit → the agents park → and then a FRESH `convoy up`, reading those stale tags, hits the + * `isFlapping(...) → skip` gate in `classify` and NEVER relaunches them. The reconstructed incident: a + * bring-up after an outage brought back only some of the fleet; the rest stayed parked from a prior + * supervisor's give-up and had to be hand-launched. + * + * A foreground `convoy up` is a DELIBERATE bring-up — the operator gesture that says "restore the fleet" — + * so it must not inherit a prior supervisor's verdict. This clears the park (status) AND zeroes the counter, + * regardless of prior fail count, giving each member a fresh budget; the cap still re-accrues tick-to-tick + * WITHIN this supervisor's watch (the real crash-loop protection). Returns the reset tags, or null when + * nothing needs clearing (not parked, counter already 0) so the caller writes no tag needlessly. + * + * The `--once` shepherd cron does NOT call this: it runs every few minutes, so un-parking there would + * relaunch a genuinely broken agent on every tick — parking MUST stay durable across `--once`. This + * reset is scoped to the rare, intentional foreground bring-up. Pure → unit-testable. */ +export function clearParkForFreshSupervisor(tags: StrategyTags): StrategyTags | null { + if (tags.status !== FLAPPING_STATUS && tags.consecutiveFastFails === 0) return null; + return { ...tags, status: null, consecutiveFastFails: 0 }; +} + /** Classify one permanent-and-gone session (spec §5.3). Pure: same inputs → same decision. */ export function classify(input: { session: string; diff --git a/src/up-parking-recovery.test.ts b/src/up-parking-recovery.test.ts new file mode 100644 index 0000000..2b2343f --- /dev/null +++ b/src/up-parking-recovery.test.ts @@ -0,0 +1,186 @@ +// PARKING-RECOVERY (Nathan mandate, convoy incident 2026-07-22) — a supervisor bring-up after a mass +// outage MUST restore the FULL fleet. The bug: `strategy.status=flapping` + the fast-fail counter persist +// to a session's tags, so an outage that drives the cap to its limit PARKS the agents, and then a fresh +// `convoy up`, reading those stale tags, `skip`s them forever (classify: `isFlapping → skip`). The +// reconstructed incident: a bring-up brought back only part of the fleet; the rest stayed parked from a +// prior supervisor's give-up and had to be hand-launched. +// +// The fix (see up.ts FRESH-SUPERVISOR UN-PARK + flapping-cap.ts clearParkForFreshSupervisor): a foreground +// `convoy up` is a DELIBERATE bring-up, so at startup it clears the park + zeroes the counter for permanent +// members (regardless of prior fail count); the cap re-accrues tick-to-tick within THIS supervisor's watch. +// The `--once` shepherd cron does NOT un-park (it runs every few minutes — un-parking there would relaunch +// a genuinely broken agent every tick), so parking stays durable for it. +// +// This proves it end to end: it stands up a PARKED, gone-but-recorded agent (via convoy's own spawn path +// plus the same `strategy.*` tags a prior supervisor would have written), then asserts a fresh FOREGROUND +// up UN-PARKS and RELAUNCHES it, while a fresh `--once` up leaves it parked. Process-level (real daemons + +// a real `convoy up`), scoped to a throwaway XDG_STATE_HOME. Lives in the vitest gate (test.yml), not the +// hermetic nix flake check. + +import { afterEach, describe, expect, it } from "vitest"; +import { spawn, spawnSync, type ChildProcess } from "node:child_process"; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { updateTags } from "@compoundingtech/pty/client"; +import { gone, PtyHost, processAlive, spawnFromPtyFile } from "./host.ts"; +import { FLAPPING_STATUS, TAG } from "./flapping-cap.ts"; + +const repoRoot = join(dirname(fileURLToPath(import.meta.url)), ".."); +const bin = join(repoRoot, "bin", "convoy"); +const sleep = (ms: number): Promise => new Promise((r) => setTimeout(r, ms)); + +let home = ""; +let net = ""; +let host: ChildProcess | null = null; +const savedPtyRoot = process.env["PTY_ROOT"]; + +function childEnv(): NodeJS.ProcessEnv { + return { ...process.env, XDG_STATE_HOME: home, ST_ROOT: "", PTY_ROOT: "" }; +} + +function freshNet(): void { + home = mkdtempSync(join(tmpdir(), "cvy-park-")); + net = join(home, "convoy", "default"); + mkdirSync(join(net, "catalog"), { recursive: true }); + mkdirSync(join(net, "smalltalk"), { recursive: true }); +} + +/** Stand up one permanent agent whose harness EXITS quickly (`sleep 1`), so it lands in the gone-but- + * recorded state a real crashed agent occupies — the shape the park tags attach to and a replay relaunches. */ +async function spawnAgent(id: string): Promise { + const workspace = join(net, "agents", id); + mkdirSync(join(workspace, ".convoy"), { recursive: true }); + writeFileSync( + join(workspace, ".convoy", "pty.toml"), + `prefix = "${id}"\n\n[sessions.claude]\nid = "${id}"\ncommand = "sleep 1"\n\n[sessions.claude.tags]\nstrategy = "permanent"\nrole = "agent"\n\n[sessions.claude.env]\nST_AGENT = "${id}"\n`, + ); + const { spawned, failed } = await spawnFromPtyFile(workspace, net); + if (failed.length > 0 || spawned.length === 0) throw new Error(`spawn ${id} failed: ${JSON.stringify({ spawned, failed })}`); +} + +/** Poll until the agent is in the real CRASHED shape: gone-but-recorded with a DEAD pid. The harness + * exits, and ~0.5s later the pty daemon writes its exit record and shuts down (pid clears) — only then is + * the pid dead, so `convoy up` treats it as a genuine death to RESPAWN rather than a transient-gone to + * ADOPT (the adopt-alive guard: reported-gone but pid-alive → adopt, never respawn). */ +async function waitCrashed(id: string, timeoutMs = 12000): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + const s = (await new PtyHost(net).sessions()).find((x) => x.name === id); + if (s && gone(s) && !processAlive(s.pid)) return; + await sleep(150); + } + throw new Error(`${id} never reached the crashed (gone + dead-pid) state`); +} + +/** Write the park a prior supervisor would have left: status=flapping at the cap. */ +function park(id: string): void { + updateTags(id, { [TAG.status]: FLAPPING_STATUS, [TAG.consecutive]: "3" }); +} + +/** The persisted strategy.status tag for an agent (undefined once cleared). */ +async function statusTag(id: string): Promise { + const s = (await new PtyHost(net).sessions()).find((x) => x.name === id); + return s?.tags[TAG.status]; +} + +function lockedHostPid(): number | null { + try { + const pid = Number.parseInt(readFileSync(join(net, "convoy.pid"), "utf8").trim(), 10); + return Number.isInteger(pid) && processAlive(pid) ? pid : null; + } catch { + return null; + } +} + +/** Start a foreground `convoy up --json`, wait until it is hosting, run for `runMs` (long enough for the + * startup un-park + the immediate first reconcile), then kill it. Returns the captured JSONL stdout. */ +async function runForegroundUp(runMs: number): Promise { + const child = spawn(process.execPath, [bin, "up", net, "--json"], { env: childEnv(), stdio: ["ignore", "pipe", "pipe"] }); + host = child; + let stdout = ""; + child.stdout?.on("data", (d: Buffer) => (stdout += d.toString())); + const deadline = Date.now() + 15000; + while (Date.now() < deadline && lockedHostPid() !== child.pid) { + if (child.exitCode !== null) throw new Error(`convoy up exited early (code ${child.exitCode})`); + await sleep(50); + } + await sleep(runMs); + const exited = new Promise((r) => child.once("exit", () => r())); + child.kill("SIGTERM"); + await exited; + host = null; + return stdout; +} + +/** Parse a JSONL stream into records. */ +function records(stream: string): Array<{ type?: string; session?: string; spawned?: string[] }> { + const out: Array<{ type?: string; session?: string; spawned?: string[] }> = []; + for (const line of stream.split("\n")) { + if (!line.trim()) continue; + try { + out.push(JSON.parse(line)); + } catch { + /* human line leaked to stdout? ignore */ + } + } + return out; +} + +afterEach(() => { + if (host) { + try { + host.kill("SIGKILL"); + } catch { + /* ignore */ + } + host = null; + } + try { + spawnSync(process.execPath, [bin, "down", net, "--force"], { env: childEnv() }); + } catch { + /* ignore */ + } + if (home) rmSync(home, { recursive: true, force: true }); + if (savedPtyRoot === undefined) delete process.env["PTY_ROOT"]; + else process.env["PTY_ROOT"] = savedPtyRoot; +}); + +describe("parking recovery — a fresh foreground `convoy up` restores a PARKED agent (Nathan mandate)", () => { + it("ACCEPTANCE: a fresh FOREGROUND up UN-PARKS and RELAUNCHES a parked gone agent (regardless of fail count)", async () => { + freshNet(); + const id = "prk-alpha"; + await spawnAgent(id); + await waitCrashed(id); + park(id); + expect(await statusTag(id), "the agent must be parked before the bring-up").toBe(FLAPPING_STATUS); + + const out = records(await runForegroundUp(2500)); + + // It was UN-PARKED (the startup pass cleared the persisted park)... + expect(out.some((r) => r.type === "unpark" && r.session === id), "a fresh foreground up must emit an unpark for the parked agent").toBe(true); + // ...and then RELAUNCHED (the reconcile respawned it once un-parking made it eligible — a still-parked + // agent would have been skipped and never respawned/replayed). + const relaunched = out.some((r) => (r.type === "respawn" && r.session === id) || (r.type === "replay" && (r.spawned ?? []).includes(id))); + expect(relaunched, "a fresh foreground up must relaunch the un-parked agent").toBe(true); + // The persisted park is gone from disk. + expect(await statusTag(id), "the flapping status tag must be cleared on disk after the bring-up").not.toBe(FLAPPING_STATUS); + }, 45000); + + it("CONTROL: a `--once` bring-up does NOT un-park — parking stays durable for the shepherd cron", async () => { + freshNet(); + const id = "prk-beta"; + await spawnAgent(id); + await waitCrashed(id); + park(id); + + const r = spawnSync(process.execPath, [bin, "up", net, "--once", "--json"], { env: childEnv(), encoding: "utf8" }); + expect(r.status, `--once should exit 0\nstderr:\n${r.stderr}`).toBe(0); + + // No un-park was emitted, and the park is still on disk — `--once` must respect it (else it would + // relaunch a genuinely broken agent every few minutes). + expect(records(r.stdout).some((rec) => rec.type === "unpark"), "--once must NOT un-park anything").toBe(false); + expect(await statusTag(id), "--once must leave the park intact").toBe(FLAPPING_STATUS); + }, 45000); +}); diff --git a/src/up.ts b/src/up.ts index f73944c..d2bd681 100644 --- a/src/up.ts +++ b/src/up.ts @@ -11,6 +11,7 @@ import { defaultConvoyNetwork, isNetworkName, networkDirForName, networkDirOfStR import { classify, classifyFailedAttempt, + clearParkForFreshSupervisor, effectiveLimit, effectiveWindow, isFlapping, @@ -359,6 +360,33 @@ export async function up(opts: UpOptions): Promise { const notify = opts.notify ?? []; const dingTargets = (crashed: SupervisedSession, sessions: readonly SupervisedSession[]): string[] => crashDingTargets(crashed, sessions, notify, busIdOf); + // FRESH-SUPERVISOR UN-PARK (parking-recovery, 2026-07-22). A foreground `convoy up` is a DELIBERATE + // bring-up — after a mass outage it MUST restore the FULL fleet, not inherit a prior supervisor's + // give-up. `strategy.status=flapping` + the fast-fail counter persist to each session's tags, so a + // parked agent stays parked across a restart (classify's `isFlapping → skip`), and a bring-up brought + // back only part of the fleet — the rest had to be hand-launched. So, ONCE at startup, clear the park + // and zero the counter for permanent members (regardless of prior fail count); the cap re-accrues + // tick-to-tick within THIS supervisor's watch. A fully-gone parked agent (no session record left) is + // relaunched by the catalog pass instead — this handles the gone-but-recorded ones the cap would skip. + // + // `--once` (the shepherd cron) SKIPS this: it runs every few minutes, so un-parking there would + // relaunch a genuinely broken agent every tick. Parking must stay durable across `--once`. + if (opts.once !== true) { + const startupNow = new Date(); + for (const s of await host.sessions()) { + if (!isPermanent(s)) continue; + const cleared = clearParkForFreshSupervisor(parseStrategyTags(s.tags)); + if (!cleared) continue; + host.removeTag(s.name, TAG.status); // updateTags MERGES — the park must be removed, not just overwritten + host.setTags(s.name, writtenTags(cleared)); // consecutive-fast-fails → 0 + state.set(s.name, cleared); + emit( + { type: "unpark", identity: logicalId(s), session: s.name, ts: isoString(startupNow) }, + `[convoy-up] fresh supervisor — cleared parked/flapping state for ${logicalId(s)} session=${s.name}; giving it a fresh cap budget`, + ); + } + } + const tick = async (): Promise => { const now = new Date(); // Manifest replay relaunches EVERY limb of an agent, so it must happen at most once per agent per