row 249: row 235 follow-up, a grant whose install never returned records hostLineLeft, since the remote add can outlive an abort or a timeout (#1751)
An abort or a timeout kills only the local ssh. A non-pty sshd command gets no SIGHUP, so the remote
add can still write its line after the undo's remove. The broker now marks install done only after
`await install(...)` returns. When it never returned, the undo records hostLineLeft with the tag, so
grant.failed and status name the line, hosts.remove refuses until `mosaic hosts sweep` clears it.
The :432 test comment now matches. Row a's enroll does no remote add, so it has no such race.
Candidate gizmoduck/row249-on-8a04 05595794b (Gizmoduck), manifest 3a969bb136bc, approved by
Duckworth in round 1 (rev 2889-2890 request c30068, verdict c30070). Gate on Sage's slot 14:53Z
(sage-2026): grant-executor.test.mjs whole 39/0 and bus 598/0 with no stub left, and C1, which
drops the `if (!installed)` branch, fails G8 and both G23 tests. Webby's N1 on X5 `sleep 30` is
deferred by Duckworth (DEFERRED.md).
This commit is contained in:
1 parent
8ca064158a
commit
9de5a57abd
3 files changed
+161
-3
No files matched your search
@@ -0,0 +1,98 @@
|
||||
# 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-on-8a04, on row 248's landing 8a04f4aa4 (sage-2012).
|
||||
No file under packages/bus/src changed between 6d5545d44 and 8a04f4aa4, so the brief's src 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 <host>" 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` (:451) 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.
|
||||
- :468, the abort case. `hold-add`, the human approves, the add holds, `abortClaim` aborts the
|
||||
executor's claim. Ends revoked.
|
||||
- :480, 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 (:283) 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 :436 (:432 at 6d5545d44) 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 (:67, :72) is the backstop.
|
||||
|
||||
## 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
|
||||
|
||||
On Sage's grant sage-2015, at 8457af076 (7cee1a1ab plus this file), TMPDIR /mnt/storage/scratch/g249t:
|
||||
|
||||
| Run | Window | Result |
|
||||
|---|---|---|
|
||||
| grant-executor.test.mjs whole | 13:29:01-13:29:21Z | 39 of 39 pass |
|
||||
| C1 | 13:29:21-13:29:40Z | killed: both row 249 tests and G8 fail on `hostLineLeft` undefined |
|
||||
|
||||
After C1 the worktree was clean and broker.mjs matched HEAD. After the runs, no node, sshd or sh process had a
|
||||
cwd under the worktree or g249t, and no `fake/add` or `fake/remove` stub was running. The bus suite runs on
|
||||
Sage's gate.
|
||||
|
||||
## Rebase onto 8a04f4aa4 (sage-2012)
|
||||
|
||||
The first commit, 02bfd184b, sat on bc579f7a8 as gizmoduck/row249, which was never pushed. Row 248 landed
|
||||
as 8a04f4aa4, changing grant-executor.test.mjs at :54, :67 and :72. gizmoduck/row249-on-8a04 starts at
|
||||
8a04f4aa4 and cherry-picks 02bfd184b with no conflict. 248's lines stay as landed. The patch-id is the
|
||||
same on both (c33823941e61). Test line numbers moved down by one, and this file was updated for that.
|
||||
@@ -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));
|
||||
|
||||
@@ -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();
|
||||
|
||||
Reference in new issue
Block a user