fix(fleet): refuse v2 add/remove cleanly, and pin the Condition's effect

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 <name>    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)
This commit is contained in:
2026-08-15 23:34:35 -05:00
parent 463745e314
commit 67f5014cc0
3 changed files with 75 additions and 15 deletions
@@ -4,14 +4,6 @@ Documentation=https://git.mosaicstack.dev/mosaicstack/stack
Requires=mosaic-tmux-holder.service Requires=mosaic-tmux-holder.service
After=mosaic-tmux-holder.service After=mosaic-tmux-holder.service
PartOf=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] [Service]
Type=oneshot Type=oneshot
@@ -1,3 +1,4 @@
import { execFile } from 'node:child_process';
import { mkdir, mkdtemp, readFile, readdir, rm, stat, writeFile } from 'node:fs/promises'; import { mkdir, mkdtemp, readFile, readdir, rm, stat, writeFile } from 'node:fs/promises';
import { tmpdir } from 'node:os'; import { tmpdir } from 'node:os';
import { join, resolve } from 'node:path'; import { join, resolve } from 'node:path';
@@ -208,16 +209,73 @@ describe('mosaic fleet install — roster v2', (): void => {
}); });
describe('[email protected]', (): void => { describe('[email protected]', (): void => {
const unitPath = resolve(process.cwd(), 'framework', 'systemd', 'user', '[email protected]');
/** The single `ConditionPathExists=` value declared by the unit template. */
async function conditionPath(): Promise<string> {
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<void> => { it('will not attempt a seat before the reconciler has written its env', async (): Promise<void> => {
// The pairing that makes "install writes no env" safe: install enables the // The pairing that makes "install writes no env" safe: install enables the
// unit (WantedBy=default.target) but does not start it, so without this // unit (WantedBy=default.target) but does not start it, so without this
// condition a reboot between `install` and the first `apply` would run // condition a reboot between `install` and the first `apply` would run
// ExecStart against an absent env file and fail every seat unit. // ExecStart against an absent env file and fail every seat unit.
const unit = await readFile( expect(await conditionPath()).toBe('%h/.config/mosaic/fleet/agents/%i.env.generated');
resolve(process.cwd(), 'framework', 'systemd', 'user', '[email protected]'), });
'utf8',
); /**
expect(unit).toContain('ConditionPathExists=%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@<name>` 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<void> => {
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<void> => {
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');
}); });
}); });
+12 -2
View File
@@ -1914,7 +1914,14 @@ export function registerFleetCommand(program: Command, deps: FleetCommandDeps =
}, },
) => { ) => {
if (await usesRosterV2ControlPlane(cmd)) { 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)) { if (!VALID_FLEET_RUNTIMES.includes(opts.runtime)) {
throw new Error( throw new Error(
@@ -1982,7 +1989,10 @@ export function registerFleetCommand(program: Command, deps: FleetCommandDeps =
.option('--keep-files', 'Skip deleting env and heartbeat files') .option('--keep-files', 'Skip deleting env and heartbeat files')
.action(async (name: string, opts: { keepFiles?: boolean }) => { .action(async (name: string, opts: { keepFiles?: boolean }) => {
if (await usesRosterV2ControlPlane(cmd)) { 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 commandOpts = cmd.opts<{ mosaicHome: string; roster?: string }>();
const activePaths = resolveFleetPaths(commandOpts.mosaicHome); const activePaths = resolveFleetPaths(commandOpts.mosaicHome);