From 7cee1a1ab9cbc3c84588a1613ed667bbb3f42fa6 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Wed, 21 Oct 2026 08:19:30 -0500 Subject: [PATCH] row 249 (#1751): a grant whose install never returned records hostLineLeft, abort and timeout tests, G8 and the :432 comment (Gizmoduck) --- agents/gizmoduck/work/queue-249/BUILD.md | 82 ++++++++++++++++++++++ packages/bus/src/broker.mjs | 5 ++ packages/bus/tests/grant-executor.test.mjs | 61 +++++++++++++++- 3 files changed, 145 insertions(+), 3 deletions(-) create mode 100644 agents/gizmoduck/work/queue-249/BUILD.md diff --git a/agents/gizmoduck/work/queue-249/BUILD.md b/agents/gizmoduck/work/queue-249/BUILD.md new file mode 100644 index 000000000..71d0ccf52 --- /dev/null +++ b/agents/gizmoduck/work/queue-249/BUILD.md @@ -0,0 +1,82 @@ +# Row 249 build notes (Gizmoduck) + +Gitea #1751, Vikunja P19 445 under #436. Brief: docs/plans/2026-10-20_host-credentials.md, section +"Machine grant row b follow-up: the undo can't prove the remote add has ended" (6de1ebc62). Reviewer +Duckworth, gate owner Sage. Branch gizmoduck/row249 from origin/refactor bc579f7a8. No file under +packages/bus/src changed between 6d5545d44 and bc579f7a8, so the brief's line numbers hold on the base. +Line numbers below are on this branch. + +## Correction to the brief + +The brief, copying gizmoduck-0139, puts the sweep's host-unknown refusal at broker.mjs:1350. On +6d5545d44 it is :1349. On this branch it is :1354. + +## The change + +`#runHostGrant` in packages/bus/src/broker.mjs: + +- :943 adds `installed = false` next to `installing`. +- :956 sets `installed = true` on the line after `await this.#hostGrant.install(...)`, before the + `stop()` that follows it. An abort that lands after install returned still ends revoked, with no hint, + because install returning means every request it sent has exited. +- :1025, in the catch, inside `if (installing)` and after the remove: `if (!installed) extra = + { hostLineLeft: tag };`. The remove still runs first and its result doesn't matter. A remove that + throws sets the same hint as before (:1021). + +The rule keys on install not returning, not on `signal.aborted`. That covers an abort (host-run.mjs:136, +SIGKILL to the local group), a timeout (host-run.mjs:131-134, SIGTERM, then SIGKILL after `graceMs`) and +a dropped connection, which fails the request with no signal at all. `#hostGrantFailed` (:845) already +adds "A line tagged X may be left on the host. Run: mosaic hosts sweep " when `hostLineLeft` is +set (:852), so the message needed no change. + +False hints. install is one call that runs `list`, maybe `remove`, then `add` (host-grant.mjs:329-333). +Any failure inside it now records the hint, including a `list` that exits 1 before any add was sent, and +an abort that lands before the first ssh starts (the G23 X5 case at "a revoke right after the spend"). +The brief accepts this for an add that exits nonzero. The other cases are the same kind: the line isn't +on the host, and a sweep that doesn't list the tag drops the hint (G24, "a left tag no longer on the +host", broker.mjs:1358). Narrowing it would need install to report which request was in flight, which is +a change to host-grant.mjs's API that the brief doesn't ask for. + +## Row a's enroll + +The race can't happen there, because enrollment sends nothing to the host. `#runEnrollment` +(broker.mjs:772-842) writes the record, calls `this.#hostGrant.enroll(business, host)` (:800) and +commits the fingerprint. `enroll` (host-grant.mjs:247-286) runs ssh-keygen locally and writes the +private half to its file. It opens no ssh session. The installer line goes on the host by hand: the +notice tells ops-01 to write it, then to run `mosaic hosts check`. The undo at :835 is `unenroll`, which +deletes the local key file. There is no remote add to outlive a kill, and no remote undo to race with. + +## Tests + +packages/bus/tests/grant-executor.test.mjs: + +- `lateAdd` (:450) is the shared tail. After the grant ends it asserts the code, `hostLineLeft` on + grant.failed, the sweep line in the notice, and that the undo's remove was the last request. Then it + writes `release`, so the held add writes its line and exits 0, and waits for the line in `granted`. It + checks there's no record, `hosts.status` lists the tag in `hostLineLeft`, `hosts.remove` refuses + host-line-left, `hosts.sweep` returns `removed: [tag]` and the line is gone, then `hosts.remove` + succeeds. +- :467, the abort case. `hold-add`, the human approves, the add holds, `abortClaim` aborts the + executor's claim. Ends revoked. +- :479, the timeout case. A 30 s wait is too long for the file, and making the timeout an option would + add a src seam, so the test wraps `install` through `opts.wrap` instead. The wrapper passes install its + own AbortController signal, waits for `holding`, aborts it, which SIGKILLs the local ssh group as the + timer does, then throws as a timed-out request does. The executor's signal isn't aborted, so it ends + host-failed. Nothing under src changes for it. +- G8 (:282) changes. fail-list and fail-add now carry `hostLineLeft` and the sweep line. noop-add fails at + verify, after install returned, so it keeps no hint and the old message. +- The comment at :435 (was :432) now says `skip` makes the held add exit without writing and that a + killed session's add can still write. G13 has no such comment. + +Both new tests write `release`, so their held stub exits before `t.after` removes the dir. Row 248's dir +check isn't needed for them. + +## Mutant + +/mnt/storage/scratch/gizmo-235-out/mutants-249.py, on row 235's runner. C1 removes the :1025 line and +runs grant-executor.test.mjs. Both new tests and G8 should fail under it. `--check` matches its anchor +once. + +## Runs + +Pending Sage's slot: grant-executor.test.mjs whole, then C1. The bus suite runs on Sage's gate. diff --git a/packages/bus/src/broker.mjs b/packages/bus/src/broker.mjs index 3baa709f9..135a1699a 100644 --- a/packages/bus/src/broker.mjs +++ b/packages/bus/src/broker.mjs @@ -940,6 +940,7 @@ export class Broker { if (signal.aborted) fail('revoked'); }; let installing = false, + installed = false, registered = false, committed = false; try { @@ -952,6 +953,7 @@ export class Broker { step('install'); installing = true; await this.#hostGrant.install(business, g.host, record, tag, key.publicLine, signal); + installed = true; stop(); await this.#hostGrant.verify(g.host, record, key, signal); stop(); @@ -1018,6 +1020,9 @@ export class Broker { } catch { extra = { hostLineLeft: tag }; } + // An abort or a timeout kills only the local ssh, so an add that hasn't returned may still write + // its line after the remove has run. Only install returning proves it ended (row 249). + if (!installed) extra = { hostLineLeft: tag }; } try { this.#store.transaction(() => this.#hostGrantFailed(business, decision, g, code, extra)); diff --git a/packages/bus/tests/grant-executor.test.mjs b/packages/bus/tests/grant-executor.test.mjs index 2eda5459a..1992f1525 100644 --- a/packages/bus/tests/grant-executor.test.mjs +++ b/packages/bus/tests/grant-executor.test.mjs @@ -287,14 +287,17 @@ test('G8: a failure at install or verify removes the line and leaves no record, const { id, end } = await f.approve(); assert.equal(end.code, 'host-failed', flag); assert.equal(end.tag, f.tag); - assert.equal('hostLineLeft' in end, false); + // An install that didn't return keeps the sweep hint, since its add may still be running (row 249). + // Here the hint is false, and a sweep drops it (G24). noop-add fails at verify, after install returned. + const returned = flag === 'noop-add'; + assert.equal(end.hostLineLeft, returned ? undefined : f.tag, flag); assert.equal(f.requests().at(-1), `remove ${USER} ${f.tag}`, flag); assert.deepEqual(f.granted(), []); assert.equal(f.record(), undefined); assert.equal(existsSync(f.keyFile), false); assert.equal(f.state(), undefined); assert.deepEqual(f.grantEvents().filter((e) => e.grantDecision === id).map((e) => e.action), ['grant.step', 'grant.failed']); - assert.match(f.messages(id)[0].body, /failed: host-failed\.$/); + assert.match(f.messages(id)[0].body, returned ? /failed: host-failed\.$/ : /Run: mosaic hosts sweep web1$/); } }); @@ -430,7 +433,8 @@ test('G23: a revoke during install ends grant.failed revoked with no key file, n await until(() => existsSync(join(f.dir, 'holding'))); assert.deepEqual(f.credentials.abortClaim('demo/ops', 'host.web1'), { pending: true }); await until(() => f.ended(d.id)); - // The held add then exits without writing, as a killed session's would. + // `skip` makes the held add exit without writing. A killed session's add can still write; the row 249 + // tests below cover that. f.flag('skip'); f.flag('release'); assert.equal(f.ended(d.id).code, 'revoked'); @@ -442,6 +446,57 @@ test('G23: a revoke during install ends grant.failed revoked with no key file, n assert.equal(f.credentials.busy('demo/ops', 'host.web1'), false); }); +// Row 249. An abort and a timeout kill only the local ssh, and sshd sends a non-pty command no SIGHUP, +// so the held add goes on. Released after the undo's remove, it writes its line and exits 0. +const lateAdd = async (f, id, code) => { + await until(() => f.ended(id)); + const end = f.ended(id); + assert.equal(end.code, code); + assert.equal(end.hostLineLeft, f.tag); + assert.match(f.messages(id)[0].body, new RegExp(`tagged ${f.tag} may be left on the host\\. Run: mosaic hosts sweep web1$`)); + assert.equal(f.requests().at(-1), `remove ${USER} ${f.tag}`); + f.flag('release'); + await until(() => f.granted().some((l) => l.endsWith(' ' + f.tag))); + assert.equal(f.record(), undefined); + assert.deepEqual((await f.call('human', 'hosts.status'))[0].hostLineLeft, [f.tag]); + await refusesWith(f.call('human', 'hosts.remove', { host: 'web1' }), 'host-line-left'); + assert.deepEqual(await f.call('human', 'hosts.sweep', { host: 'web1' }), { host: 'web1', removed: [f.tag], hostLineLeft: [] }); + assert.deepEqual(f.granted(), []); + assert.deepEqual(await f.call('human', 'hosts.remove', { host: 'web1' }), { host: 'web1', removed: true }); +}; + +test('G23 (row 249): an add that writes after an abort\'s remove leaves hostLineLeft until the sweep', async (t) => { + const f = await setup(t); + f.flag('hold-add'); + const d = await f.raise(); + f.call('human', 'decision.resolve', { id: d.id, choice: 'yes' }); + await until(() => existsSync(join(f.dir, 'holding'))); + assert.deepEqual(f.credentials.abortClaim('demo/ops', 'host.web1'), { pending: true }); + await lateAdd(f, d.id, 'revoked'); +}); + +// The timeout's kill without its 30 s wait: install gets its own signal, which SIGKILLs the local ssh +// group as host-run.mjs's timer does, and the call then fails as a timed-out request does. +test('G23 (row 249): an add that writes after a timed-out install\'s remove leaves hostLineLeft until the sweep', async (t) => { + const f = await setup(t, { + wrap: (hg) => ({ + ...hg, + install: async (business, host, record, tag, line) => { + const timer = new AbortController(); + const out = hg.install(business, host, record, tag, line, timer.signal); + await until(() => existsSync(join(f.dir, 'holding'))); + timer.abort(); + await out.catch(() => {}); + throw new Error('timed out'); + }, + }), + }); + f.flag('hold-add'); + const d = await f.raise(); + f.call('human', 'decision.resolve', { id: d.id, choice: 'yes' }); + await lateAdd(f, d.id, 'host-failed'); +}); + test('G23: a revoke recorded after the raise and before the start ends step 1 revoked, and a held slot ends host-busy', async (t) => { const f = await setup(t); const revoked = f.fixtureGrant();