diff options
| author | Gabriel Schneider <[email protected]> | 2026-08-27 15:32:02 -0300 |
|---|---|---|
| committer | Gabriel Schneider <[email protected]> | 2026-08-27 16:15:35 -0300 |
| commit | f5927a033f0c83753b5cc004e514568eec8c24f8 (patch) | |
| tree | e31d0f29aac1d4266c42dac99fcdd0013bfbe64d | |
| parent | 5f4719da21f06b58694d52d354f5fda431ff8543 (diff) | |
| download | pardes-f5927a033f0c83753b5cc004e514568eec8c24f8.tar.gz pardes-f5927a033f0c83753b5cc004e514568eec8c24f8.zip | |
detached: drop /dev/fuse from the poll set once the connection dies
Review fixes to steps 1 and 2, found by an adversarial pass over the committed
chain. One is a real bug and the rest are comments that were false.
THE BUG. `waitInput` put the FUSE descriptor in the poll set whenever the mount
existed, and the `.fuse` arm ignored every revent. Linux's `fuse_dev_poll`
answers EPOLLERR once the connection is gone, and POSIX reports POLLERR whatever
the events mask asked for -- so an external `fusermount3 -u`, a sysfs abort, or
systemd taking /run/user/$UID away at final logout (exactly when a detached
session is supposed to keep running) made poll(2) return instantly, forever. The
daemon then spun the whole pump at 100% of a core for the rest of its life, and
re-offered every parked slot to the core at that rate.
Measured on the unfixed commit: 0 CPU ticks over 10 s idle, then 1000 ticks over
the next 10 s after unmounting its own mount point. Measured after the fix: 0
ticks over 8 s in the same scenario, process alive and in state S.
fuse.zig's own poll thread has carried the equivalent guard all along, which is
why the desktop shells never showed this and the daemon did.
THE COMMENTS, each checkable and each wrong:
* `Source.fuse` said the drain is not done in `dispatch` to avoid re-entering
the core. Both `pull_wait_input` and `push_poll_frame` are called from
inside `pump`, so either re-enters. The real reason is that `dispatch` is
mid-iteration over a SNAPSHOT of the descriptors, and one `push_spawn`
replaces a pane's master under it.
* `fuse.zig`'s new import said "no cycle". There is a cycle: fs_service
imports fuse.zig back and still names `*Fs` in three signatures.
* `Transport`'s rationale said tty.zig and gui.zig store one in a struct
field. Neither does; all four call sites build it inline.
* `deinit` justified its ordering against "as long as the harvest takes".
`harvest` is waitpid(WNOHANG) and blocks for nothing. The real reason to go
first is that aborting the connection wakes a parked reader while its own
shell is still alive to run its exit path.
* `harvest` claimed pane shells are the only children this process forks.
Since step 1 it also forks `fusermount3`, three times over -- reaped by its
own spawner, so the conclusion holds and the premise did not.
* docs/detached.md, docs/lsp.md and docs/design.typ still said the daemon
implements sixteen of twenty-one methods and mounts no /dev/fuse.
| -rw-r--r-- | docs/design.typ | 5 | ||||
| -rw-r--r-- | docs/detached.md | 15 | ||||
| -rw-r--r-- | docs/lsp.md | 14 | ||||
| -rw-r--r-- | src/detached/server.zig | 73 | ||||
| -rw-r--r-- | src/fs_service.zig | 11 | ||||
| -rw-r--r-- | src/fuse.zig | 7 |
6 files changed, 84 insertions, 41 deletions
diff --git a/docs/design.typ b/docs/design.typ index d322920a..ccb94a08 100644 --- a/docs/design.typ +++ b/docs/design.typ @@ -1157,8 +1157,9 @@ running in pane 3 is still running and has been scrolling into the core the whol time. Of the twenty-one `VTable` methods, the detached core's `Session` implements -sixteen and leaves five null: `push_post_present`, `pull_gpio_toggle`, -`pull_lsp`, `pull_pipe`, `push_fs_reply`. +seventeen and leaves four null: `push_post_present`, `pull_gpio_toggle`, +`pull_lsp`, `pull_pipe`. It mounts its own `/dev/fuse` and polls it in the same +`poll(2)` as its frontends, so `--fs` works in a daemon and needs no thread. Nothing blocks indefinitely, and that property is what a detached session is *for*. Every descriptor is non-blocking; the single `poll(2)` is the only place diff --git a/docs/detached.md b/docs/detached.md index b4dde59c..d849c810 100644 --- a/docs/detached.md +++ b/docs/detached.md @@ -255,14 +255,13 @@ Stated rather than papered over: * **The screen is shared, at the smallest common grid.** Two frontends of different sizes converge on the smaller; the larger window letterboxes. Same semantics as tmux. -* **No LSP, no selection pipe, no `--fs` control filesystem** in a detached - session. The daemon implements **sixteen** of `Host.VTable`'s **twenty-one** - methods — FEWER than the tty and SDL shells, which install nineteen each, - everything but `pull_gpio_toggle` and `push_detach` — and it is the only host - that implements `push_detach` at all. The five it leaves null divide cleanly. - Three are real losses: `pull_lsp` and `pull_pipe` want a - worker pool this deliberately single-threaded loop has not got, and - `push_fs_reply` wants a `/dev/fuse` this process never mounted. They fall back +* **No LSP and no selection pipe** in a detached session. The daemon implements + **seventeen** of `Host.VTable`'s **twenty-one** methods — fewer than the tty + and SDL shells, which install nineteen each, everything but + `pull_gpio_toggle` and `push_detach` — and it is the only host that + implements `push_detach` at all. The four it leaves null divide cleanly. + Two are real losses: `pull_lsp` and `pull_pipe` want a worker pool this + deliberately single-threaded loop has not got. They fall back to the core's in-process defaults rather than failing, so the features are quiet rather than broken. The other two are not losses at all: there is no moment "after the frame is on screen" for a process with no screen diff --git a/docs/lsp.md b/docs/lsp.md index 1adb397b..b35c8eeb 100644 --- a/docs/lsp.md +++ b/docs/lsp.md @@ -39,8 +39,8 @@ shell and the ESP32-P4 object: there `lsp.supports` is empty, which makes response; a host that links a backend would add both. **In a DETACHED session every query answers EMPTY.** -`src/detached/server.zig`'s vtable implements sixteen of `host.zig`'s -twenty-one methods, and `pull_lsp` is one of the five it leaves null — a +`src/detached/server.zig`'s vtable implements seventeen of `host.zig`'s +twenty-one methods, and `pull_lsp` is one of the four it leaves null — a worker pool is precisely what its deliberately single-threaded loop does not have. A null method is NOT automatically a dropped effect: `perform` decides that per arm, and the `.lsp` arm's answer is to synthesise one on the spot — @@ -69,16 +69,14 @@ has moved by the time the prong runs, the guard fails, and the Tab really is eaten. The local shells have the same race over a wider window, so this is a property of the late-indent repair rather than of detaching. -None of the detached core's five null methods silently drops a reachable +None of the detached core's four null methods silently drops a reachable effect. `pull_lsp` and `pull_pipe` have the fallbacks above; `pull_gpio_toggle` is `orelse return Error.NoPads` (`board_memory.zig`), which lands on the message row; `push_post_present` is a `pump` hook fired after presenting, and there is -nothing to notify in a process with no screen; and `push_fs_reply` is the one -`perform` arm with no fallback at all, but it is only ever emitted in answer to -an `Event.fs_req`, which is not on the wire (`wire.zig`: it has no `ClientTag`, -because neither half of that pair may cross an attachment) and which the -daemon raises none of, mounting no `/dev/fuse` by its own vtable comment. +nothing to notify in a process with no screen. `push_fs_reply` was listed here +too until the daemon began mounting its own `/dev/fuse`; it implements that one +now, and `--fs` works in a detached session. Four rules make it safe: diff --git a/src/detached/server.zig b/src/detached/server.zig index d8373149..8e6b92d4 100644 --- a/src/detached/server.zig +++ b/src/detached/server.zig @@ -394,11 +394,22 @@ const Source = union(enum) { client: u8, pty: u8, inotify, - /// The acme filesystem's descriptor. Its arm does nothing: being in the set - /// is the whole point, because a readable `/dev/fuse` must end the sleep so - /// that `pollFrame` — which runs after `pull_wait_input` returns — reaches - /// the drain. Answering it here instead would re-enter the core from inside - /// its own `pump`. + /// The acme filesystem's descriptor. Its arm does one thing only: notice + /// that the connection has gone. Being in the set is otherwise the whole + /// point, because a readable `/dev/fuse` must END THE SLEEP so that + /// `pollFrame` — which runs after `pull_wait_input` returns — reaches the + /// drain. + /// + /// The drain is not done HERE, and the reason is not re-entrancy: both + /// `pull_wait_input` and `push_poll_frame` are called from inside + /// `Pardes.pump`, so either would re-enter. It is that `dispatch` is + /// mid-iteration over the `fds[0..n]`/`src[0..n]` SNAPSHOT `waitInput` + /// built, and a filesystem request reaches `core.perform`, which drains the + /// whole effect ring — including a `push_spawn`, whose `Session.spawn` + /// closes a pane's master and forks a new one. A later `.pty` entry in the + /// same pass would then apply the old descriptor's `revents` to a brand new + /// one, and the `fd < 0` guard cannot see it because the fd is valid, + /// merely different. Same hazard as the client slots, same reason. fuse, }; @@ -486,9 +497,18 @@ pub const Session = struct { pub fn deinit(s: *Session) void { // First, and before the pane shells: a script blocked on `event` is // holding a kernel request, and `Fs.deinit` answers everything still - // parked and aborts the connection before unmounting. Leaving it until - // after the harvest would leave that reader in uninterruptible sleep - // for as long as the harvest takes. + // parked and ABORTS THE CONNECTION before unmounting, so that reader + // wakes with ENODEV while its own shell is still alive to run its exit + // path. After `closePty` it would be woken by a hangup instead, with + // nothing left to exit into. + // + // Not, as this comment first claimed, because the harvest takes time: + // `harvest` is `waitpid(WNOHANG)` in a loop and blocks for nothing. The + // one step here that CAN take milliseconds is this one, because + // `Fs.deinit` forks `fusermount3` and waits for it untimed — which is + // also why the `.quit` owed to every frontend now queues behind a + // subprocess. Worth knowing; not worth reordering, because the shells + // matter more than the milliseconds. if (s.fs) |f| { f.deinit(); s.fs = null; @@ -1083,11 +1103,16 @@ pub const Session = struct { /// Collect every child that has exited. /// /// `waitpid(-1)` and not a pid list, because the only children this process - /// forks are pane shells (`host_io.forkShell`) — so "any exited child" and - /// "an exited pane shell" are the same set — and because the pids a list - /// would hold are exactly the ones it cannot help with: a respawn closes a - /// master, and the shell that gets the hangup exits some milliseconds later - /// with its slot already reused by a different shell. + /// LEAVES UNREAPED are pane shells (`host_io.forkShell`) — so "any exited + /// child" and "an exited pane shell" are the same set — and because the pids + /// a list would hold are exactly the ones it cannot help with: a respawn + /// closes a master, and the shell that gets the hangup exits some + /// milliseconds later with its slot already reused by a different shell. + /// Anything else this process forks — `fusermount3`, from `Fs.mount`, + /// `sweepStale` and `Fs.deinit` — is reaped by its own spawner with a + /// pid-specific blocking wait before control returns here, so the set this + /// sees is still only shells. A future worker that forks and does not wait + /// would break that, and this is the sentence it has to come back and edit. /// /// Nothing here waits, so a session whose shells are all running pays one /// syscall that returns 0. Called once per poll round and again wherever a @@ -1305,7 +1330,17 @@ pub const Session = struct { // `pollFrame` after `pull_wait_input` returns reaches the drain. The // desktop shells buy the same wake with a thread; one poll slot is // cheaper and cannot race the loop. - if (s.fs) |f| if (f.fd >= 0) { + // ...and NOT once it is dead. `fuse_dev_poll` answers `EPOLLERR` as soon + // as the connection is gone, POSIX reports `POLLERR` whatever the events + // mask asked for, and this arm cannot consume it — so an external + // `fusermount3 -u`, a sysfs abort, or systemd taking `/run/user/$UID` + // away at final logout (exactly when a detached session is supposed to + // keep running) would make `poll(2)` return instantly, forever, and burn + // a whole core for the life of the daemon. Measured at 100% of one CPU + // before this guard. fuse.zig's own poll thread has carried the + // equivalent check all along, which is why the desktop shells never + // showed it and this loop did. + if (s.fs) |f| if (f.fd >= 0 and !f.dead) { fds[n] = .{ .fd = f.fd, .events = poll_in, .revents = 0 }; src[n] = .fuse; n += 1; @@ -1413,9 +1448,13 @@ pub const Session = struct { } }, .inotify => if (pfd.revents & poll_in != 0) s.drainInotify(), - // Nothing. See `Source.fuse`: the wake IS the work, and the drain - // belongs to `pollFrame`, where re-entering the core is legal. - .fuse => {}, + // The only revents worth a word: a dead connection must leave the + // set, or the `POLLERR` it reports on every future poll spins the + // loop. `Fs.next` would set `dead` on its first failed read anyway; + // saying it here costs nothing and saves the one spinning round. + .fuse => if (pfd.revents & (poll_hup | poll_err | poll_nval) != 0) { + if (s.fs) |f| f.dead = true; + }, }; } diff --git a/src/fs_service.zig b/src/fs_service.zig index 7821e850..b440c2c2 100644 --- a/src/fs_service.zig +++ b/src/fs_service.zig @@ -43,10 +43,13 @@ const max_batch = 64; /// listener beside the mount a matter of writing one more implementation /// rather than of teaching this file about it. /// -/// It is deliberately NOT a Zig interface with `anytype`: the two callers -/// (tty.zig and gui.zig) store the transport in a struct field across frames, -/// so it has to be a value with a runtime type, which is a vtable — the same -/// shape and the same reasoning as `host.VTable`. +/// It is deliberately NOT a Zig interface with `anytype`: `drain` is ONE +/// function reached from four call sites (tty.zig, gui.zig twice, and the +/// daemon), and the second transport this seam exists for is chosen at RUN +/// time, so it has to be a value with a runtime type — which is a vtable, the +/// same shape and the same reasoning as `host.VTable`. Nothing holds a +/// `Transport` across a frame: every caller builds one inline from whatever it +/// has, which is why the thunks matter and the struct does not. /// /// The ORDER contract stays where it was, in `drain`, because it belongs to /// the caller rather than to any implementor: `retry()` to null first, then diff --git a/src/fuse.zig b/src/fuse.zig index 3a903e8b..3bd263bd 100644 --- a/src/fuse.zig +++ b/src/fuse.zig @@ -59,8 +59,11 @@ const libc = std.c; const linux = std.os.linux; const acmefs = @import("acmefs.zig"); /// Only for `Transport`, the three-function shape this mount presents to the -/// host loop. No cycle: `fs_service` names no type from here any more, which -/// is the point of the seam. +/// host loop. This IS a cycle — `fs_service` imports this file back for +/// `Fs.mount`, `sweepStale` and `exportPaneEnv`, and still names `*Fs` in three +/// of its own signatures — and Zig accepts it because imports are analysed +/// lazily. What the seam removed is `drain`'s dependency on the concrete type, +/// not the file's dependency on this one. Do not read it as more than that. const fs_service = @import("fs_service.zig"); /// Everything below the mount is Linux kernel ABI. Off Linux the module still |
