neuwirth
/
pmacs
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(gpu): 1-pre round 2 --- a read ceiling, and the A4 consequence was wrong 

Two review findings, one of them a real defect.

**THE SENTINEL READ COULD HANG FOREVER.** `read_until_sentinel` blocked
with no bound, so a writer or encoder that regressed after `enqueue`
would WEDGE THE GATE rather than redden it --- and a hang is the worst
failure shape there is, because it looks like slowness until the job is
killed. A 30 s `READ_CEILING` is armed on the daemon socket.

The distinction is kept explicit in the code, because collapsing it is
how this fix would undo the design it protects: **the sentinel remains
the success condition and the ceiling is only an error ceiling.**
Arrival is still decided by the sentinel, so the harness never infers
"nothing was sent" from a duration --- the core-count assumption behind
PR #235's CI red is not reintroduced. The ceiling sits far above any
plausible drain, so reaching it means broken, never busy.

M24 proves it fires rather than trusting it: drop the sentinel enqueue
entirely and the row fails in under a second with a diagnostic naming
both candidate causes and the partial transcript, instead of hanging.

**THE STAGE 1a CONSEQUENCE WAS WRONG IN FOUR PLACES.** Every record
claimed A4 would leave `EventOutcome` with one variant, so the type
should go with the Escape branch. It will not, and it should not.
`LifecycleRoute::Exit` --- a native window close --- returns
`EventOutcome::Exit` too. A4 removes the KEYBOARD producer only, leaving
one `Exit` producer.

And **one producer is not one variant**: the type survives because
`dispatch_window_event` must still distinguish `Continue` from `Exit` on
every event it handles --- nearly all must not exit, and the close must.
What A4 actually changes is `apply_keyboard`'s signature. Corrected in
the `EventOutcome` doc, the Escape-branch comment, the framing and the
ledger; the framing's superseded paragraph is deleted rather than
patched, since it also carried the stale "two `event_loop.exit()`
call sites" count. **There is exactly one executable
`event_loop.exit()`**, in `window_event`.

Also: the sentinel-tag comment claimed four modifier bits and used
three. It now says three, wrapping every eight steps, and why that
suffices --- each sentinel is read before the next is issued, so a tag
only has to differ from its immediate predecessor.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai
This commit is contained in:
Levi Neuwirth 2026-08-12 11:07:41 +02:00
parent 68c99fb2e2
commit 9f08f5278c
No known key found for this signature in database
4 changed files with 153 additions and 79 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

86
docs/active-work.md
View File

@ -268,10 +268,11 @@ waits for a signal that is not coming.
Stage 1's framing for **all** slices and governs the later branches;
only Stage 0 was framed by the arc document itself.
- **Scope of THIS branch: 1-pre only — the input seam. No behaviour
change.** `App::window_event` was **655 lines** and is now **33**: one
`route_event` call and one arm per route. Nothing below it could be
witnessed without a display, which is why the seam precedes every
other slice.
change.** `App::window_event` was **655 lines** and is now **four** —
call `dispatch_window_event`, exit if it asks. The dispatch it calls
is one `route_event` call and one arm per route. Nothing below it
could be witnessed without a display, which is why the seam precedes
every other slice.
- **IMPLEMENTED in four commits, one per event family**, so each lands
with its own witnesses and mutations rather than as one 600-line
diff: `014110f` lifecycle (close / modifiers / resize), `7f0f9db`
@ -321,22 +322,26 @@ waits for a signal that is not coming.
free function composing one decision function per family
(`route_lifecycle`, `route_keyboard` + `route_key_action`,
`route_pointer`). Performing stays on `App` in seven `apply_*`
methods. **A route names its LOCAL EFFECT**, not merely the family
that claims it, and the harness records routes — because
`CloseRequested` and `RedrawRequested` send the daemon nothing, so a
transcript of protocol traffic could not tell a handled arm from a
dropped one.
methods. A route carries the **decision**, not the effect: a wheel
route holds a delta, and whether that becomes a viewport update, a
panel event, a terminal event or nothing depends on `State`. Effects
are witnessed separately — see the P2 bullet below.
- **Every moved body verified as the original, mechanically.** The
keyboard body is byte-identical modulo two named conversions; the four
pointer bodies were checked by re-running rustfmt on the pre-move text
at the new indent level and diffing, since de-indenting by 8 columns
lets rustfmt rejoin lines. **13 witnesses, 17 mutations.**
- **P3 IS NOW MEASURED, NOT ASSUMED.** Replacing `window_event`'s whole
body with `let _ = (event_loop, event);` — a GUI that responds to no
input at all — leaves **all 256 `pmacs-gpu` tests green**. That is the
exception's true extent: `ActiveEventLoop` cannot exist outside a live
event loop, so **no** headless test in the crate observes the
delegation, not merely none of the new ones.
lets rustfmt rejoin lines. **22 witnesses — 13 routing, 9 effect — and
23 mutations**, each failing its own rows, plus the P3 exception check
which must stay green.
- **P3 IS NOW MEASURED, NOT ASSUMED — and re-measured after the effect
harness landed.** Replacing `window_event`'s whole body with `let _ =
(event_loop, event);` — a GUI that responds to no input at all —
leaves **all 265 `pmacs-gpu` tests green** under `PMACS_REQUIRE_GPU=1`
at the current shape (it was 256 before the effect rows; the number is
re-run, not carried forward). That is the exception's true extent:
`ActiveEventLoop` cannot exist outside a live event loop, so **no**
headless test in the crate observes the delegation. What it covers is
now **one `if`**, not a 33-line match.
- **A SECOND ACCEPTED STRUCTURAL EXCEPTION, found here and winit's
rather than ours.** `KeyEvent` carries a `pub(crate)
platform_specific` field, so **no `WindowEvent::KeyboardInput` can be
@ -347,12 +352,19 @@ waits for a signal that is not coming.
the exception down); the family's only real decision is factored into
`route_key_action(ElementState)` and witnessed directly; and what
stays unwitnessed is one pattern arm containing a match and a call.
- **`event_loop.exit()` now appears in exactly two places, both inside
`window_event`.** The keyboard body was its second caller (the idle
Escape), so `apply_keyboard` returns an `EventOutcome` rather than
taking an `&ActiveEventLoop` — which is what keeps the bodies
reachable in principle. **Stage 1a's A4 deletes that branch**, at
which point `EventOutcome` has one variant and should go.
- **The crate now has exactly ONE executable `event_loop.exit()`**, in
`window_event`. Bodies return an `EventOutcome` rather than taking an
`&ActiveEventLoop`, which is what keeps them reachable in principle.
**`EventOutcome` has TWO producers and they differ in kind**: a native
close (`LifecycleRoute::Exit`), which must always exit, and
`apply_keyboard`'s idle Escape, which is a local quit. **Stage 1a's A4
removes the keyboard producer only**, leaving **one** `Exit`
producer — the native close. **One producer is not one variant**:
`EventOutcome` survives because `dispatch_window_event` must still
distinguish `Continue` from `Exit` on every event, and only the close
exits. A4 changes `apply_keyboard`'s signature, not the type. An
earlier revision of this bullet claimed the type would collapse and
should go with the Escape branch; that was wrong.
- **Slice order (each its own branch and PR):** `1-pre` → `1a`\* → `1b`
→ `1c` → `1d` → `1e`\*. **`1a` and `1e` are protocol-bearing (v24
`TextInput`, v25 `OpenTarget`/`OpenTargetResult`) and are
@ -374,10 +386,11 @@ waits for a signal that is not coming.
file-watcher tests — with every other target green. Established as
environmental three ways, in increasing strength:
- **Structurally impossible for this branch to cause.** The branch
changes six files, five of them under `docs/`; **the whole
executable diff is `pmacs-gpu/src/main.rs`**, and `pmacs-gpu` is a
workspace **member but not a dependency** of the root package, so
the `m4_acceptance` binary never links it.
changes seven files, five of them under `docs/`; **the whole
executable diff is inside the `pmacs-gpu` crate**
(`src/main.rs` and `src/attach.rs`), and `pmacs-gpu` is a workspace
**member but not a dependency** of the root package, so the
`m4_acceptance` binary never links it.
- **The marker is older than the session.** `/tmp/.git` is empty and
was created at 20:14 CEST, **3.5 h before** the gate run at 23:45;
`/tmp` held 8,920 entries. That is handoff §1's recorded hazard
@ -400,12 +413,21 @@ waits for a signal that is not coming.
unnecessary, and the isolated `TMPDIR` is the correct remedy. It must
be **outside `/tmp` and outside every git worktree** — a child of
`/tmp` is not isolated, because `/tmp/.git` remains its ancestor.
- **GREEN under an isolated `TMPDIR`: all nine gates pass** (log
`20260811T215605Z-2664352`). fmt, clippy, lib, lib-crdt,
`gpu_invocation_acceptance`, **m4 168/0/3**, `PMACS_REQUIRE_GPU=1 -p
pmacs-gpu`, the **117-target `--workspace --no-fail-fast` sweep with
zero failures anywhere** (`m4_acceptance` running all 171 in it), and
`diff-check`.
- **GREEN under an isolated `TMPDIR`: all nine gates pass on the final
tree** (log `20260812T090034Z-2989598`, review round 2). fmt, clippy,
lib, lib-crdt, `gpu_invocation_acceptance`, **m4 168/0/3**,
`PMACS_REQUIRE_GPU=1 -p pmacs-gpu` (**265 tests, none filtered and
none skipped**, all nine effect rows included), the **117-target
`--workspace --no-fail-fast` sweep with zero failures anywhere**
(`m4_acceptance` running all 171 in it), and `diff-check`. Earlier
full-green runs at `20260811T215605Z-2664352` (round 1) and
`20260812T083735Z-2869707` (the P2 harness) are superseded by this
one. **What changed after it is doc comments and prose only** — the
`EventOutcome` correction below and this paragraph; `cargo fmt
--check`, `clippy -D warnings` and the 22 routing/effect rows were
re-run on the result. Stated rather than glossed, because "the gate
was green" and "the gate was green on exactly this tree" are
different claims.
## The GUI arc — Stage 0 MERGED as #236 (`f8ad3e7`)

2
docs/agent-handoff.md
View File

@ -216,7 +216,7 @@ commands, read `docs/active-work.md` immediately after this file.
roots but **not `TMPDIR`**, so `tempfile::tempdir()` still lands
under a `/tmp` that may carry a marker. It surfaced inside a gate
run on an unrelated lane (GUI 1-pre, whose whole **executable** diff
is `pmacs-gpu/src/main.rs`) as **`m4_24_bare_string_glob_stays_relative`
is inside the `pmacs-gpu` crate) as **`m4_24_bare_string_glob_stays_relative`
and `m4_24_d3_fallback_base_is_the_smallest_attachment_dir`**, in
both the `m4` step and the `--workspace` sweep, with every other
target in the corpus green.

46
docs/gui-stage1-input-framing.md
View File

@ -148,11 +148,14 @@ blanket one:
directly, with both misroute mutations failing that row alone.
- **P3 is now measured, not assumed.** Replacing `window_event`'s entire
body with `let _ = (event_loop, event);` — a GUI that responds to no
input at all — leaves **all 256 `pmacs-gpu` tests green**, not merely
the 13 routing rows. That is the exception's true extent: no headless
test anywhere in the crate observes the delegation. *(Revision 11
shrinks what the exception covers: `window_event` is now four lines,
so the unwitnessed residue is one `if` rather than a 33-line match.)*
input at all — leaves **every `pmacs-gpu` test green**. That is the
exception's true extent: no headless test anywhere in the crate
observes the delegation. **Re-measured at the current shape after
revision 11: 265/265 under `PMACS_REQUIRE_GPU=1`** (it was 256 before
the effect rows existed, and the number is re-run rather than carried
forward). Revision 11 also shrinks what the exception *covers*:
`window_event` is four lines, so the unwitnessed residue is one `if`
rather than a 33-line match.
**Revision 11 — P2 IS IMPLEMENTED AS WRITTEN. Revision 10's argument
here was wrong and is retracted.**
@ -194,17 +197,30 @@ directions, which is P2's contract executable rather than asserted.
The routing rows stay, and the division of labour is deliberate: the
routing harness answers *where did this event go*, the effect harness
answers *what did it do*. The transcript row remains the routing
harness's sole P2-recording owner, which keeps its own mutation
surgical.
answers *what did it do*. **P2 is owned by the effect rows.** The
routing transcript row is only the sole owner of the *routing*
harness's own recording, which is what keeps that one mutation
surgical — revision 10 claimed it owned P2, and it never did.
**One design consequence worth carrying into 1a.** The keyboard arm was
the second caller of `event_loop.exit()` — the idle-Escape local quit —
so its body returns an `EventOutcome` rather than taking an
`&ActiveEventLoop`. `event_loop.exit()` now appears in exactly two
places, both inside `window_event`, and nowhere else in the crate.
**A4 deletes the Escape branch, at which point `EventOutcome` has one
variant and should go with it.**
**One further correction revision 11 carries, and it is the design
consequence 1a inherits.** Revision 10 stated that Stage 1a's A4 would
leave `EventOutcome` with a single variant and that the type should go
with the Escape branch. **Both are wrong.**
`EventOutcome` has **two producers today**: `LifecycleRoute::Exit`, a
native window close that must always exit, and `apply_keyboard`'s idle
Escape, a local quit. **A4 removes the keyboard one**, leaving **exactly
one `Exit` producer** — the native close.
**One producer is not one variant.** The type survives because
`dispatch_window_event` still has to distinguish `Continue` from
`Exit` on every event it handles: the overwhelming majority of
dispatches must *not* exit, and the native close must. What A4 actually
removes is `apply_keyboard`'s need to return an outcome at all, which is
a change to that one signature rather than to this type.
The crate has **exactly one** executable `event_loop.exit()`, in
`window_event`.
### 1a — `TextInput` (v24)

98
pmacs-gpu/src/main.rs
View File

@ -3226,8 +3226,9 @@ impl App {
} else {
// Q#S1-1 / A4 — the local quit, unchanged here and
// deleted by Stage 1a: an idle Escape must reach the
// daemon. `window_event` performs the exit; this is
// the only reason a body needs an outcome at all.
// daemon. `window_event` performs the exit. This is the
// only reason a BODY needs an outcome; `EventOutcome`
// itself outlives A4, since a native close still exits.
return EventOutcome::Exit;
}
return EventOutcome::Continue;
@ -3395,13 +3396,11 @@ impl App {
/// exception and how far it reaches. **Performing** stays on `App`, in
/// the `apply_*` methods the router's variants name.
///
/// The decision is what the route *is*, not merely which family claims
/// it: `Exit` is the local exit effect, `Resize` carries the clamped
/// surface extent, `Modifiers` carries the state mutation. **Two arms —
/// `CloseRequested` and `RedrawRequested` — send nothing outbound at
/// all**, so a harness recording only protocol traffic would leave them
/// invisible; that is why a route names its local effect and the harness
/// records routes.
/// A route carries the decision, not merely the family: `Resize` holds
/// the clamped extent, `Modifiers` the new state. **What a route does
/// NOT carry is the effect** — a `Wheel` may become a viewport update, a
/// panel event, a terminal event or nothing at all, depending on
/// `State`. Effects are witnessed separately, by `EffectHarness`.
#[derive(Debug, Clone, Copy, PartialEq)]
enum Route<'a> {
/// The lifecycle family — see [`route_lifecycle`].
@ -3417,16 +3416,25 @@ enum Route<'a> {
Unrouted,
}
/// What the event loop must do once a family's body has run. Only the
/// keyboard family produces anything but `Continue` today: an idle
/// Escape is a local quit. Returning the decision rather than taking an
/// `&ActiveEventLoop` is what keeps every body reachable from a test —
/// the crate's two `event_loop.exit()` call sites, this one and
/// `LifecycleRoute::Exit`, both sit in `window_event` and nowhere else.
/// What the event loop must do once a family's body has run.
///
/// Stage 1a's A4 deletes that branch (an idle Escape must reach the
/// daemon and never exit), at which point this type has one variant and
/// should go.
/// **Two producers, and they are not the same kind of thing.**
/// `LifecycleRoute::Exit` is a native window close, which must always
/// exit; `apply_keyboard` returns `Exit` for an idle Escape, which is a
/// local quit. Returning the decision rather than taking an
/// `&ActiveEventLoop` is what keeps every body reachable from a test:
/// the crate has **exactly one** executable `event_loop.exit()`, in
/// `window_event`.
///
/// **Stage 1a's A4 removes the KEYBOARD producer only** — an idle
/// Escape must reach the daemon and never exit — leaving **one** `Exit`
/// producer, the native close.
///
/// **One producer is not one variant.** This type survives A4 because
/// `dispatch_window_event` must still distinguish `Continue` from
/// `Exit` on every event it handles: nearly all of them must not exit,
/// and the close must. What A4 changes is `apply_keyboard`'s signature,
/// not this type.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum EventOutcome {
Continue,
@ -3585,7 +3593,7 @@ fn route_pointer(event: &WindowEvent) -> Option<PointerRoute> {
/// deliberately, not of outbound protocol traffic: `CloseRequested`
/// exits and `RedrawRequested` repaints, and neither sends the daemon
/// anything, so a transcript of daemon traffic alone cannot tell a
/// handled arm from a dropped one. A route names its local effect —
/// handled arm from a dropped one. A route records the decision —
/// exit, resize extent, modifier mutation — which is what makes those
/// arms observable here at all.
///
@ -3691,6 +3699,13 @@ impl EffectHarness {
/// anything a body would legitimately send.
const SENTINEL: char = '\u{e000}';
/// Error ceiling on one outbound read. **Not a pacing device** — the
/// sentinel decides arrival — so this is set far above any plausible
/// drain: reaching it means the writer or encoder is broken, never
/// that the machine is busy. Without it a regression downstream of
/// `enqueue` wedges the gate instead of reddening it.
const READ_CEILING: std::time::Duration = std::time::Duration::from_secs(30);
/// Build the harness, or fail loudly.
///
/// **Never skips.** `State::new_headless` returns `None` when no
@ -3717,6 +3732,9 @@ impl EffectHarness {
let client = crate::attach::connect_stream_for_test(client_stream, |_| true)
.expect("attach over socketpair");
let daemon = handshake.join().expect("handshake thread");
daemon
.set_read_timeout(Some(Self::READ_CEILING))
.expect("arm the outbound read ceiling");
// A document tall enough to scroll. A two-line fixture made the
// wheel row pass vacuously: `scroll_by_lines` returns `None`
@ -3822,10 +3840,17 @@ impl EffectHarness {
/// Push a sentinel through the same outbox and read until it comes
/// back. Everything ahead of it belongs to the step just dispatched.
///
/// This is a **condition**, not a sleep: it blocks on the socket
/// until the writer thread has drained past the sentinel, so it is
/// insensitive to how many cores are free — the mistake PR #235's CI
/// red was made of.
/// **The sentinel is the success condition, and the timeout is only
/// an error ceiling** — the two are not the same thing and the
/// distinction is the whole design. Arrival is decided by the
/// sentinel, so the harness never infers "nothing was sent" from a
/// duration and is insensitive to how many cores are free; that is
/// the mistake PR #235's CI red was made of. But a blocking read
/// with no bound turns a regressed writer or encoder into a **wedged
/// gate** rather than a red one, and a hang is the worst failure
/// shape there is: it looks like slowness until the job is killed.
/// [`Self::READ_CEILING`] is therefore set far above any plausible
/// drain, so reaching it means broken, never busy.
fn read_until_sentinel(&mut self) -> Vec<pmacs_protocol::FrontendEvent> {
self.sentinel_seq += 1;
let tag = self.sentinel_seq;
@ -3837,7 +3862,16 @@ impl EffectHarness {
let mut seen = Vec::new();
loop {
let event: pmacs_protocol::FrontendEvent =
pmacs_protocol::read_message(&mut self.daemon).expect("read outbound");
match pmacs_protocol::read_message(&mut self.daemon) {
Ok(event) => event,
Err(e) => panic!(
"outbound read failed before the sentinel arrived \
after {:?}: {e}. Either the writer or the encoder \
regressed, or the step produced no sentinel at all. \
Recorded so far: {seen:?}",
Self::READ_CEILING
),
};
if is_sentinel(&event, tag) {
return seen;
}
@ -3856,9 +3890,11 @@ struct EffectSnapshot {
}
/// Modifier bits carrying the sentinel's sequence number, so a stale
/// sentinel cannot end the wrong step. Four bits is plenty: the harness
/// reads every sentinel it writes, so the counter only has to
/// distinguish neighbours.
/// sentinel cannot end the wrong step. **Three bits — Ctrl, Alt, Shift —
/// so the tag wraps every eight steps.** That is sufficient rather than
/// sloppy: the harness reads every sentinel it writes before issuing the
/// next, so a tag only ever has to distinguish itself from the one
/// immediately before it.
#[cfg(test)]
fn sentinel_mods(tag: u32) -> Modifiers {
let mut mods = Modifiers::NONE;
@ -3915,10 +3951,10 @@ mod input_routing_tests {
}
/// The per-variant rows below drive [`route_event`] directly and the
/// transcript row drives the harness. That split is deliberate: it
/// leaves the transcript row as P2's sole owner, so a harness that
/// stopped recording an effect fails exactly one row instead of
/// every row.
/// transcript row drives [`RoutingHarness`]. That split keeps the
/// transcript row the only one that fails when the ROUTING harness
/// stops recording, so that mutation stays surgical. **P2 itself is
/// owned by the effect rows** — see [`EffectHarness`].
///
/// Events are bound to locals rather than passed as temporaries
/// because a `Route` borrows the event it came from — the keyboard