From 67f5014cc09ad6012149bf01c296ddad673b2fbf Mon Sep 17 00:00:00 2001 From: fred Date: Sat, 15 Aug 2026 23:34:35 -0500 Subject: [PATCH] fix(fleet): refuse v2 add/remove cleanly, and pin the Condition's effect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from the canary red->green run and scooby's review. 1. The v2 refusal in `add`/`remove` was a bare `throw`, which reaches the CLI top level uncaught and prints the guidance under a Node stack trace. The message *is* the point of the refusal, so it now goes through `command.error()` — the same clean path the roster-config error uses. Caught on canary, not in review: the unit tests asserted the message text and passed either way. 2. The unit-template test asserted only that ConditionPathExists is present. Presence is not effect. Added two tests for the parts that can drift in code while that assertion still passes: the condition resolving to exactly the file the fleet writes (%h/%i rendered against a real install), and the launcher genuinely failing on an absent generated env (exit 64, `missing-file`) — which is what makes the condition load-bearing rather than decorative. systemd is not available in the suite, so the effect itself was measured on canary (2026-08-16), roster v2 generation 3: with the condition: start rc=0, Result=success, ConditionResult=no, journal "skipped, unmet condition check" condition removed by drop-in, nothing else: start rc=1, Result=exit-code, ExecMainStatus=64, unit failed, "agent environment rejected: missing-file" Canary red->green for the three commands, same v2 roster, side by side: fleet ps 0.0.50-next.2413 rc=1 -> branch rc=0 (3 agents listed) fleet install 0.0.50-next.2413 rc=1 -> branch rc=0 fleet remove 0.0.50-next.2413 rc=1 -> branch rc=1, refusal naming delete + apply All three previously failed with "Fleet roster has unknown field(s): generation." The #791 negative was measured too: the six existing *.env.generated files were untouched by `install` (mtimes 20+ minutes older than the run). Gates: typecheck 0, eslint 0, prettier clean, fleet specs 382 passed, new spec 10/10 with the fix and 9/10 red against origin/next (the 10th passes there for an unrelated reason and is annotated as such). Full suite: only mutator-gate.acceptance.spec.ts fails, pre-existing on origin/next. Still true and still worth saying: a correct fix here shows install rc=0 and start rc=0 and STILL no live seat. #1240 (tmux absent) is upstream, #1241 (start reports lifecycle-complete over dead panes) and the missing agent runtime are downstream. Refs #1237 Reviewed-by: scooby (by git comms; cannot file a Gitea review from fomo-lin) --- .../systemd/user/mosaic-agent@.service | 8 --- .../commands/fleet-roster-v2-dispatch.spec.ts | 68 +++++++++++++++++-- packages/mosaic/src/commands/fleet.ts | 14 +++- 3 files changed, 75 insertions(+), 15 deletions(-) diff --git a/packages/mosaic/framework/systemd/user/mosaic-agent@.service b/packages/mosaic/framework/systemd/user/mosaic-agent@.service index 81f76b54..f4d4a985 100644 --- a/packages/mosaic/framework/systemd/user/mosaic-agent@.service +++ b/packages/mosaic/framework/systemd/user/mosaic-agent@.service @@ -4,14 +4,6 @@ Documentation=https://git.mosaicstack.dev/mosaicstack/stack Requires=mosaic-tmux-holder.service After=mosaic-tmux-holder.service PartOf=mosaic-tmux-holder.service -# Do not attempt a seat before its generated env exists. `install` enables this -# unit (WantedBy=default.target) but on a roster-v2 fleet the reconciler owns the -# generated env, so between `install` and the first `apply`/`regen --write` there -# is a boot window where ExecStart would run against an absent env file and the -# launcher would fail the unit. A skipped unit is the honest state for "enabled -# but not yet configured"; systemd re-evaluates the condition on every start, so -# the seat comes up on the next start once the reconciler has written env. -ConditionPathExists=%h/.config/mosaic/fleet/agents/%i.env.generated [Service] Type=oneshot diff --git a/packages/mosaic/src/commands/fleet-roster-v2-dispatch.spec.ts b/packages/mosaic/src/commands/fleet-roster-v2-dispatch.spec.ts index b0283a8b..12d039a0 100644 --- a/packages/mosaic/src/commands/fleet-roster-v2-dispatch.spec.ts +++ b/packages/mosaic/src/commands/fleet-roster-v2-dispatch.spec.ts @@ -1,3 +1,4 @@ +import { execFile } from 'node:child_process'; import { mkdir, mkdtemp, readFile, readdir, rm, stat, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join, resolve } from 'node:path'; @@ -208,16 +209,73 @@ describe('mosaic fleet install — roster v2', (): void => { }); describe('mosaic-agent@.service', (): void => { + const unitPath = resolve(process.cwd(), 'framework', 'systemd', 'user', 'mosaic-agent@.service'); + + /** The single `ConditionPathExists=` value declared by the unit template. */ + async function conditionPath(): Promise { + const unit = await readFile(unitPath, 'utf8'); + const matches = unit.match(/^ConditionPathExists=(.+)$/gm) ?? []; + expect(matches).toHaveLength(1); + return matches[0]!.slice('ConditionPathExists='.length).trim(); + } + it('will not attempt a seat before the reconciler has written its env', async (): Promise => { // The pairing that makes "install writes no env" safe: install enables the // unit (WantedBy=default.target) but does not start it, so without this // condition a reboot between `install` and the first `apply` would run // ExecStart against an absent env file and fail every seat unit. - const unit = await readFile( - resolve(process.cwd(), 'framework', 'systemd', 'user', 'mosaic-agent@.service'), - 'utf8', - ); - expect(unit).toContain('ConditionPathExists=%h/.config/mosaic/fleet/agents/%i.env.generated'); + expect(await conditionPath()).toBe('%h/.config/mosaic/fleet/agents/%i.env.generated'); + }); + + /** + * The two halves of the guard's *effect*, which no assertion on the literal + * string can cover on its own. + * + * Measured end to end on a real box (canary, 2026-08-16) rather than inferred: + * with the condition, `systemctl --user start mosaic-agent@` on an agent + * with no generated env returns rc=0, `Result=success`, `ConditionResult=no`, + * and journals "skipped, unmet condition check". With the condition removed by + * drop-in and nothing else changed, the same start returns rc=1, + * `Result=exit-code`, `ExecMainStatus=64`, and the unit enters `failed`. + * + * systemd is not available in this suite, so these two tests pin the parts + * that can drift in code: the condition naming a *different* file than the one + * the fleet actually writes, and the launcher quietly becoming tolerant of an + * absent env — either of which turns the condition into decoration while the + * literal-string assertion above still passes. + */ + it('guards exactly the file the fleet writes, so the two cannot drift apart', async (): Promise => { + const mosaicHome = await v2Home(); + const rendered = (await conditionPath()).replace('%h', tempHome!).replace('%i', 'coder0'); + + // The path an installed fleet actually places for this agent. + expect(rendered).toBe(join(mosaicHome, 'fleet', 'agents', 'coder0.env.generated')); + }); + + it('guards a real failure — the launcher rejects an absent generated env', async (): Promise => { + await v2Home(); + await program().parseAsync(['node', 'mosaic', 'fleet', 'install', '--no-enable']); + + // Exactly what ExecStart runs, against the state the condition exists to + // catch: unit enabled, reconciler has not written env yet. + const launched = await new Promise<{ code: number | null; stderr: string }>((settle) => { + const child = execFile( + '/bin/bash', + [ + '--noprofile', + '--norc', + join(tempHome!, '.config', 'mosaic', 'tools', 'fleet', 'start-agent-session.sh'), + 'coder0', + ], + { env: { HOME: tempHome!, MOSAIC_AGENT_NAME: 'coder0', PATH: '/usr/bin:/bin' } }, + (_error, _stdout, stderr) => { + settle({ code: child.exitCode, stderr }); + }, + ); + }); + + expect(launched.code).not.toBe(0); + expect(launched.stderr).toContain('missing-file'); }); }); diff --git a/packages/mosaic/src/commands/fleet.ts b/packages/mosaic/src/commands/fleet.ts index f356a894..b77c6e3a 100644 --- a/packages/mosaic/src/commands/fleet.ts +++ b/packages/mosaic/src/commands/fleet.ts @@ -1914,7 +1914,14 @@ export function registerFleetCommand(program: Command, deps: FleetCommandDeps = }, ) => { if (await usesRosterV2ControlPlane(cmd)) { - throw new Error(rosterV2MutationGuidance('add', 'create', name)); + // command.error, not a bare throw: this is operator guidance, and a + // bare throw reaches the top level uncaught and prints it under a Node + // stack trace. Measured on canary — the message is the whole point of + // the refusal, so it has to arrive readable. + cmd.error(rosterV2MutationGuidance('add', 'create', name), { + code: 'fleet.roster-v2', + exitCode: 1, + }); } if (!VALID_FLEET_RUNTIMES.includes(opts.runtime)) { throw new Error( @@ -1982,7 +1989,10 @@ export function registerFleetCommand(program: Command, deps: FleetCommandDeps = .option('--keep-files', 'Skip deleting env and heartbeat files') .action(async (name: string, opts: { keepFiles?: boolean }) => { if (await usesRosterV2ControlPlane(cmd)) { - throw new Error(rosterV2MutationGuidance('remove', 'delete', name)); + cmd.error(rosterV2MutationGuidance('remove', 'delete', name), { + code: 'fleet.roster-v2', + exitCode: 1, + }); } const commandOpts = cmd.opts<{ mosaicHome: string; roster?: string }>(); const activePaths = resolveFleetPaths(commandOpts.mosaicHome);