gizmoduck: row 302 (#1809, Vikunja 588) idle-stop race design round d1 for Dewey: idle-released code from the broker's own record of threadIdle's release, runner maps it to stopped/0, forcing cli test holds the window open until a call lands, threads.test:393 is the one changed pin (sage-3200, mos-1411)
This commit is contained in:
jason.woltje committed
1 parent
0e30a85fac
commit
87e67a765f
1 file changed
+197
@@ -0,0 +1,197 @@
|
||||
# Row 302 (#1809, Vikunja 588): the idle-stop race
|
||||
|
||||
Owner Gizmoduck. Reviewer Dewey. Gate owner Sage. Brief:
|
||||
docs/plans/2026-10-23_idle-stop-race-row.md, "Brief: the idle-stop race (Vikunja 588)". Base 0e30a85fa.
|
||||
This is the design, written static only. Nothing gets built until Dewey approves it (sage-3200).
|
||||
|
||||
## The race, as the code has it at 0e30a85fa
|
||||
|
||||
- `packages/cli/src/launcher.mjs:703-717`. The idle tick calls the `threadIdle` op. On `released` it adds
|
||||
the run to `stopping`, records `thread-idle`, and only then sends SIGTERM (:716).
|
||||
- `packages/bus/src/broker.mjs:1044-1057`. `threadIdle` releases the claim in the same synchronous call
|
||||
(`#claimEnd(..., 'release', null)`, :1055). The comment at :1040-1043 says the order is deliberate:
|
||||
row 121 r2, Dewey's F3 in 28591.
|
||||
- `broker.mjs:1002-1008`. `#agent` is the only place a thread run's own call gets `not-holder`, because
|
||||
its session's latest claim row isn't a `claim` of its generation. `message.receive` (:5586-5591) and
|
||||
`state` (:5805-5807) both go through it, and so do `message.send` and `message.read`.
|
||||
- `packages/harness/src/runner.mjs:55`. `SESSION_GONE` holds `unauthenticated`, `not-holder` and
|
||||
`run-ended`. Four sites set `code = EXIT.session` (22) on any of them, without checking `stopping`:
|
||||
the landing `state` (:500-506), `message.receive` (:526-531), the turn's `state` (:545-552) and the
|
||||
reply (:575-582).
|
||||
- `packages/seat/src/session.mjs:340` maps 22 to `session-invalid`.
|
||||
|
||||
So if a runner call reaches the broker after the release and before SIGTERM sets `stopping`, the run
|
||||
exits 22 by itself and ends `session-invalid`. A call already in flight when the release lands counts
|
||||
too. Row 292's diagnostic saw this once in about 100 idle stops. The clear test's run r846b8d083502 logged
|
||||
`not-holder` and then `exiting 22`.
|
||||
|
||||
## Design: a distinct answer for a run the idle stop released (Mos's suggestion, adopted)
|
||||
|
||||
### Broker (`packages/bus/src/broker.mjs`)
|
||||
|
||||
1. Add a new in-memory set, `#idled`, keyed `business:run`, next to `#ended`, `#received` and `#stops`.
|
||||
`threadIdle` adds the run's key in the same synchronous call that releases it, and only when it
|
||||
returns `{released:true}`. Release and mark happen together, so no call can be answered between them.
|
||||
2. In `#agent`, the refusal becomes
|
||||
`fail(this.#idled.has(s.business + ':' + s.run) ? 'idle-released' : 'not-holder')`. Nothing else in
|
||||
`#agent` changes. A run the idle stop released now gets `idle-released` at every call after the
|
||||
release, whether it's a receive, `state`, a send or a read, and whether or not another run has
|
||||
claimed the session since. Every other path that takes a claim away still answers `not-holder`:
|
||||
- `assistant.stop` (:5975), `instance.end` (:6009), the chief lifecycle revoke (:4682) and
|
||||
`role.revoke` (:5230, :5235)
|
||||
- the session clear (:1146)
|
||||
- the limit stop (`#limitStop`, :4842-4855). It releases a thread claim and then refuses
|
||||
`not-holder` on purpose, so that the run exits 22.
|
||||
- another run's newer generation, where the run wasn't idle-released
|
||||
3. `endLaunch` (:1300) drops the key along with `#ended.add`. After the end, the run's caps are gone and
|
||||
its calls answer `unauthenticated`, as they do today. The set lives only in memory, like `#runs`. A
|
||||
broker that restarts loses `#runs` too, so an old run's calls answer `unauthenticated` there either
|
||||
way.
|
||||
4. `threadIdle`'s own refusals don't change. A second `threadIdle` on a released run is still
|
||||
`not-holder` (threads.test:391). The answer comes from the `#agent` path only, for the run's own
|
||||
calls.
|
||||
|
||||
Why a new code and not a flag on `not-holder`: the wire carries only `error: <code>`
|
||||
(server.mjs:96-160), and the client accepts only `^[a-z-]{1,64}$` (client.mjs:42-43). A new code changes
|
||||
neither of them. Tools and docs that switch on `not-holder` keep their meaning, because `not-holder`
|
||||
still means "your claim was taken away".
|
||||
|
||||
### Runner (`packages/harness/src/runner.mjs`)
|
||||
|
||||
5. A new constant, `IDLE_RELEASED = "idle-released"`. It is not added to `SESSION_GONE`. At each of the
|
||||
four sites, `idle-released` is checked before `SESSION_GONE`. The runner logs
|
||||
`runner: session released by the launcher's idle stop`, leaves `code` at `EXIT.stopped` (0), sets a
|
||||
local `released`, and leaves the loop the same way the site does for `SESSION_GONE` today.
|
||||
6. The `role.release` at exit (:589-595) is skipped when `released`. The claim is already released, so
|
||||
the call would only be refused and logged.
|
||||
7. If the runner didn't handle the code, the receive loop would count `idle-released` as an
|
||||
unreachable broker and retry (:532-537). The run would then end at SIGTERM (0), or after 30 polls
|
||||
(23). Every site handles it so that neither of those decides how the run ends.
|
||||
|
||||
How this meets mos-1411 condition 1: `unauthenticated` (the caps went at the end), `run-ended`, and every
|
||||
`not-holder` that isn't an idle release still exit 22 and end `session-invalid`. Only the broker's own
|
||||
record that `threadIdle` released this run maps to `stopped/0`. The launcher's `stopping` set and SIGTERM
|
||||
play no part in the mapping. That also covers a poll that lands before the signal, and a call that was
|
||||
in flight when the release landed.
|
||||
|
||||
### Launcher and session.mjs
|
||||
|
||||
8. No change. The runner exits 0 by itself, and SIGTERM's start-time check (:716) then finds no process,
|
||||
which the code already handles. `endReason(0)` is `stopped`.
|
||||
|
||||
## Tests
|
||||
|
||||
### The forcing test (mos-1411 condition 2): new file `packages/cli/tests/idle-stop-race.test.mjs`
|
||||
|
||||
This is a thread launch on the same fixture shape as thread-launch.test.mjs (`start(t, prepared, { ops })`
|
||||
replaces launch ops, :78-101), with `sessionDefaults.pollInterval` set to 100. The test replaces the
|
||||
`threadIdle` op:
|
||||
|
||||
```js
|
||||
threadIdle: async (m) => {
|
||||
const r = runtime.threadIdle(m.record);
|
||||
if (r.released) window.set(m.record.run, await held(m.record.run)); // returns only after the runner's own exit, or 20 s
|
||||
return r;
|
||||
}
|
||||
```
|
||||
|
||||
`held(run)` waits for two things, in order. First, an `action.refused` event whose actor is `run` and
|
||||
whose seq is after the release's `thread_claims` row. That proves a runner call reached the broker inside
|
||||
the window. Second, the runner's own exit (the run's `runner: exiting N` line in runner.log, or its pid
|
||||
gone), with no SIGTERM sent. The launcher sends SIGTERM only after the op returns, so the window stays
|
||||
open until the runner has decided how it ends. The test then asserts all of the following:
|
||||
|
||||
- `held` saw a refusal from the run after the release, and the runner exited before the op returned.
|
||||
- The refusal's code is `idle-released`.
|
||||
- The run's launch `end` event is `['stopped', 0, null]`.
|
||||
- runner.log has the idle-released line and no `session stopped working:` line.
|
||||
|
||||
Against the code today, the refusal is `not-holder`, the runner exits 22 inside the window, and the end
|
||||
is `['session-invalid', 22, null]`. The test fails, and fails every time, not at some rate: the
|
||||
window is held open until a call has landed in it. If the runner fix is dropped, the runner retries and
|
||||
doesn't exit inside the window, so `held` times out and the "exited before the op returned" assertion
|
||||
fails. If the broker fix is dropped, the end is 22.
|
||||
|
||||
A control test in the same file: after an idle release, a `role.revoke` the human approved on another
|
||||
thread's run (or `assistant.stop`) still ends that run `['session-invalid', 22, null]`. That shows a
|
||||
real not-holder still exits 22.
|
||||
|
||||
### Bus: new tests in `packages/bus/tests/idle-release.test.mjs` (a new file, so no existing assertion moves)
|
||||
|
||||
- After `threadIdle` releases a-run, a-run's `message.receive`, `state`, `message.send` and
|
||||
`message.read` each refuse `idle-released`. Each writes an `action.refused` event with code
|
||||
`idle-released`, and none of them delivers a message or writes a receipt.
|
||||
- `not-holder` still comes back for: a run released by `assistant.stop`, a run released by a session
|
||||
clear, a limit stop (stop then receive), a run whose session another run claimed without any idle
|
||||
release, and a second `threadIdle` on a released run. After `endLaunch`, the idle-released run's
|
||||
calls get `unauthenticated`, as today (its caps go at :1310-1311).
|
||||
- A role run's `role.release` followed by its own receive still gets `not-holder` (no idle).
|
||||
|
||||
### Harness: no new runner.test case
|
||||
|
||||
runner.test.mjs's `setup` (:105) binds role runs only, and its claims sit in `role_claims` (:876), so it
|
||||
can't reach `threadIdle`. The runner's own exit at receive is covered by the cli forcing test above,
|
||||
which runs the real runner. The forcing test reaches only the site that lands in the window, which in
|
||||
practice is the receive poll. The landing `state`, the turn's `state` and the reply sites get the same
|
||||
four lines, but no test forces each one. Q4 asks whether Dewey wants a per-site case. The existing 22
|
||||
tests (:539-560, :630-647, :772) stay unchanged and pass unchanged.
|
||||
|
||||
## The one existing assertion that changes (goes to Mos through Sage, per the brief)
|
||||
|
||||
`packages/bus/tests/threads.test.mjs:393`,
|
||||
`assert.throws(() => call('a', 'message.receive'), { code: 'not-holder' })`. This is a receive by the run
|
||||
that `threadIdle` released at :388, so under this design it answers `idle-released`. The assertion is
|
||||
changed, not loosened: the new pin is `{ code: 'idle-released' }`, and the property right below it
|
||||
(:394, the message stays `pending` for the next holder) stays. I've found no other existing assertion
|
||||
that pins `not-holder` for an idle-released run's own call. The build reruns
|
||||
`git grep -n "not-holder" 'packages/*/tests/*'` against every idle path before in-review, and names any
|
||||
other hit for Mos first.
|
||||
|
||||
## Docs
|
||||
|
||||
- packages/bus/README.md:320-326 (threadIdle). Add: after a release, the run's own calls answer
|
||||
`idle-released`, not `not-holder`. Also say why.
|
||||
- packages/harness/README.md:295-302, the exit table. Row 0 becomes "stopped, by a signal, or because the
|
||||
launcher's idle stop released the session (`idle-released`)". Row 22 is unchanged.
|
||||
- packages/bus/README.md:207 (`assistant.stop`): no change. It still gives `not-holder`.
|
||||
|
||||
## Open questions for Dewey
|
||||
|
||||
- Q1. `landRelease` (broker.mjs:4144-4165, launcher.mjs:876-889) has the same shape for a role run's
|
||||
land-relaunch: it releases, then the launcher sends SIGTERM, so a poll in between ends the run 22. I
|
||||
recommend folding it in. That means one line in `landRelease` (add to `#idled` on release), one bus
|
||||
assertion, and one cli forcing case like the one above. It falls outside this brief's words, so it
|
||||
needs Sage's yes. If the answer is no, I'll write it up as a follow-up row with the same evidence.
|
||||
- Q2. The code name `idle-released`. It names the cause, and if Q1 is yes it covers land-relaunch too,
|
||||
since that is also a release made by the launcher. A more general name like `launcher-released` would
|
||||
fit both. I prefer `released-by-launcher` only if Q1 is yes. Dewey picks.
|
||||
- Q3. A limit stop on a run that was already idle-released. `#limitStop` (:4842-4855) finds no claim to
|
||||
end and refuses `not-holder`, so the run ends 22. I'd leave that alone: a limit hit is a real stop and
|
||||
ends session-invalid, as it does today. It's possible only if the limit checker answers inside the
|
||||
window.
|
||||
|
||||
- Q4. Per-site coverage in the runner. Only the receive site is forced. I can add a cli case per site
|
||||
(hold the window until the run's landing `state`, or a turn's `state`, lands in it), at about one
|
||||
fixture each. I'd add them only if Dewey asks, since all four sites share one helper in the build.
|
||||
|
||||
## Gate (brief)
|
||||
|
||||
Dewey approves this design, then the manifest. Runs: the new forcing test, then
|
||||
compactions-launch.test.mjs whole, then the bus, cli and harness suites whole, then every
|
||||
`scripts/test-*.sh`. The mos-1411 point 4 rerun rule doesn't apply to this row's own gate.
|
||||
|
||||
Mutants. Each one must die on the forcing test, with the bus test also named where it applies:
|
||||
|
||||
- M1. `threadIdle` doesn't add to `#idled`. Dies on the forcing test (end 22) and the bus test.
|
||||
- M2. `#agent` ignores `#idled`. Same deaths as M1.
|
||||
- M3. The runner drops the `idle-released` branch at receive. Dies on the forcing test (no exit inside
|
||||
the window).
|
||||
- M4. The runner maps `idle-released` to `EXIT.session`. Dies on the forcing test (end 22).
|
||||
- M5. `#claimEnd` marks every release, not just threadIdle's. Dies on the bus not-holder controls and
|
||||
the cli control.
|
||||
- M6. `endLaunch` doesn't drop the key. This is not a correctness mutant: caps are gone, so the answer
|
||||
is `unauthenticated` either way. I'll drop it unless Dewey wants an assertion on it, and record it as
|
||||
an equivalent mutant.
|
||||
|
||||
Limits will be set in the run plan after the design round (mos-1275: `timeout -k 10` at three times or
|
||||
more the longest run).
|
||||
Reference in new issue
Block a user