Redesigning ProcessMonitor: detect exit from the child, not its pipes #5769
Closed
SeanTAllen
started this conversation in
Standard Library
Replies: 1 comment
|
Implemented. |
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
ProcessMonitorruns a child process and reports two things back: the child's output, and its exit status once the child exits. The problem is the exit status. The monitor often can't detect that the child has exited, so it hangs, leaves the child as a zombie, and never reports the status.The monitor never detects the child's exit directly. It relies on the pipes: when the child's stdout and stderr both reach end-of-file, the monitor reports the child as exited. But end-of-file on a pipe means only that every handle on the write end has been closed, and the child is not always the last process holding that write end open. A grandchild the child spawned can inherit that write end. The parent can still hold the child's stdin open. While something else holds the pipe open, it stays open after the child exits; the EOF never arrives, and the monitor never reports exit. This one wrong assumption causes both #5764 and #5748.
The fix is to detect the child's actual exit, delivered as a native per-platform event, and treat that as the only signal that the child has exited. Separately, a factory does the fallible setup before the actor exists and returns either a live monitor or an error, so the monitor is only ever created around a running child and can't be in a "failed to start" state at all. The redesign also fixes two related bugs: a kill-after-reap race (#5765) and a file-descriptor leak on fork failure (#5766).
This is larger than a bug fix. It changes the construction API, the exit-detection mechanism, and the actor's lifecycle. It lands as a single cutover, built as reviewable sections on one branch.
The issues this addresses
sh -c "some_daemon & exit 7"). The pipes don't reach EOF until the grandchild exits, sodisposeis delayed until then — or never fires, for a grandchild that outlives the parent (a daemon) — and the child stays a zombie in the meantime. This is the case that motivated the redesign.dispose()callskill()on a pid that may already have been reaped and recycled. The redesign makes the guard against this structural.Divergences from today's behavior
The first item is the fix; the rest are behavior or API changes. "Today" means released ponyc (
main).dispose(Exited(7))promptly.ProcessMonitor(...)no longer always returns a monitor. Construction becomes a factory that returns either aProcessMonitoror aProcessError. Preconditions that were reported asynchronously throughProcessNotify.failed(no exec capability, missing file, and now an unsupported kernel) are returned synchronously instead, and no monitor is created for them.pidfd_open(kernel ≥ 5.3); on an older kernel, construction returns a factory error instead of failing silently by never disposing. macOS, the BSDs (EVFILT_PROC), and Windows (a waitable process handle) have no version floor.The
ProcessNotifycallback contract is otherwise unchanged, and "all of the child's own output is delivered beforedispose" is preserved, with two narrow exceptions listed under Out of scope.The design
Construction: a supply-chain factory
Actor constructors can't fail, so the current constructor does all the fallible setup (capability check, file check, pipe creation, fork) inside the actor and reports failures asynchronously through
ProcessNotify.failed, because a constructor has no other way to signal. The supply-chain pattern moves that work before the actor exists:The factory (name provisional, see Decisions taken) does every fallible step: validate the exec capability, check that the file exists, probe kernel support (
pidfd_openon our own pid, whereENOSYSmeans the kernel is too old), create the four pipes, and fork/exec. On any synchronous failure it closes whatever it opened and returns aProcessError. On success it builds the actor around the live child and the pipes.The pipes and the child cross into the actor as
isoand are consumed. The actor's constructor callsbegin()on each pipe, registering its asio event with the actor as owner (the same call today's constructor already makes), registers the exit event, and firescreated. A compile probe that models this pattern — a factory returning a monitor-or-error union, withisoresources consumed into an actor that owns their asio events — compiles, so the pattern is feasible in Pony's type system.So the capability, missing-file, unsupported-kernel, and synchronous-fork failures are factory errors with no actor. The genuinely asynchronous failures —
execveorchdirfailing in the forked child, surfaced later through the_errrelay pipe — remain events that the running actor reports throughProcessNotify.failed, exactly as today.Exit detection: a native per-platform event
The child's exit is delivered as an asio event — the same machinery the pipes use — not a poll. When it fires, the actor reaps the child (
waitpid, the Windows handle wait, orGetExitCodeProcess) to collect the status and clear the zombie, then runs the reap edge (the atomic teardown-and-report sequence defined below). The exit event is the only signal of exit. Pipe-EOF only means "stop reading this pipe," and a read or write error on a pipe closes only that pipe: a pipe-level problem is not a child exit.Per platform:
pidfd_open(): the pidfd goes readable when the child exits and registers with epoll like any other fd. Pony's epoll backend already delivers arbitrary fds through the read-event path with no runtime C change. A pidfd refers to the exact process, so it is immune to pid recycling, which strengthens the ProcessMonitor.dispose can signal an unrelated process after the child has been reaped #5765 guarantee rather than relying on it.EVFILT_PROC+NOTE_EXIT: registers with the existing kqueue poller. This one needs a contained backend extension, because kqueue keys on a pid, not an fd, so there is no arbitrary-fd path today.RegisterWaitForSingleObject— the same pattern the console-stdin path already uses. Windows also polls its pipe reads on a timer today (PeekNamedPipe), with no asio wired for pipes there, so on Windows we move both the exit event and the pipe reads onto asio. That is the substantial part of the platform's change.The per-child exit poll is gone on every platform, and once the Windows pipe reads are on asio there is no poll timer left anywhere. A polling design would have needed a per-child
Timersactor, which allocates eagerly; with no timer, that cost is gone too.State model: three states
Because the factory guarantees a live child before the actor exists, the actor has no "not started" and no "failed to start"; its existence means a child is running. The lifecycle flags collapse to one named, exhaustively-matched union:
_Running: child alive, being monitored. Reads deliver stdout/stderr, writes queue to stdin with backpressure, exec/chdir errors surface via the_errpipe, and the exit event is armed._Disposing: the client calleddispose(), so we killed the child, closed the pipes, and are awaiting the exit event. Distinct from_Runningbecause the reap that follows must not deliver more output (the client cancelled), where a natural-exit reap does, and because a seconddispose()here is a no-op rather than a redundantkill()._Reaped: the exit event fired and we collected the status (or a reap error);dispose/failedfired; absorbing and inert.dispose()here is a no-op and never callskill(). It still destroys asio events on thedisposablecallback, so nothing leaks and the actor can be collected.The factory makes "the actor exists only if a child is running" true, and the type system enforces it. A trait-based state-object machine was rejected: the pipes, read buffer, and pending-write list are per-actor and shared across
_Runningand the reap, so a state-object split would not help.Transitions
Each invariant-bearing action happens on exactly one edge.
_Runningbegin()each pipe, arm the exit event,notifier.created_Running_Reaped_Reaped, drain, close all,notifier.dispose(status)_Running_Reapednotifier.failed(WaitpidError)(nodispose)_Runningdispose()_Disposing_child.kill(), close all pipes (releases backpressure); exit event stays armed_Disposing_Reapednotifier.dispose(status), no drain (pipes already closed; client cancelled)_Disposing_Reapednotifier.failed(WaitpidError)(nodispose)_Disposingdispose()_Disposingkill()again_Reapeddispose()_Reapedkill()(#5765)notifier.dispose(status)runs only on a status-carrying edge into_Reaped; those edges are mutually exclusive and_Reapedis absorbing, so dispose fires exactly once, by construction.The reap edge
When the exit event fires and the reap returns a status, the edge is atomic: one behavior turn, no yield in the middle.
_Reaped(the status is a local threaded to step 4;_Reapedholds no payload);_errwith a dedicated synchronous, non-yielding loop (not the 4 KB-yielding_pending_readspath, which would either truncate the drain or break the edge's atomicity), reading each to EAGAIN/EOF, bounded by a cap;notifier.dispose(status).Draining preserves "all of the child's own output is delivered before
dispose." When the exit event fires, the child's last writes may still sit unread in the pipe buffer. A dead child can't still be writing, and its writes reached the buffer before it exited, so the unread remainder is bounded by one pipe capacity. Draining_errhere is necessary: on an exec/chdir failure the child writes one byte to_errand exits, so the reap must read that byte and firenotifier.failed(...)before closing_err, or the failure report is silently dropped.The drain is capped so that a descendant flooding the pipe the instant the child dies can't stall teardown. The cap must be at least one pipe capacity, and it must be a fixed constant or bounded ceiling, not a function of the pipe's buffer size, because the child can grow that buffer (Linux
F_SETPIPE_SZ, up to 1 MiB) and inflate the per-reap work. It is a security bound, not a performance tuning parameter.Pipe lifecycle
The factory creates the four pipes.
begin()runs in the actor after it is created, creating the asio event and closing the far end.done_writing()(once pending writes drain), clientdispose(), or a hard write error — plus teardown cleanup. It is never closed as a side effect of stdout/stderr reaching EOF. The client owns stdin's lifetime.Every running-state stdin close routes through one
_close_stdin()helper (close the near fd, release the backpressure flag, clear the pending queue), so backpressure can never be stranded. Teardown closes all four pipes with_Pipe.close()(both ends).Invariants
disposefires exactly once: one status-carrying edge into the absorbing_Reaped, as derived under Transitions._close_stdin()(see Pipe lifecycle), so no path abandons stdin without releasing.kill()never targets a reaped pid (ProcessMonitor.dispose can signal an unrelated process after the child has been reaped #5765):kill()is reachable only on the_Running→_Disposingtransition;_Disposingand_Reapednever call it. At that point the child is an unreaped zombie we own, so its pid can't be recycled. On Linux the pidfd refers to the exact process regardless, and Windows waits on a handle that never recycles.disposablecallback.Error vocabulary
The factory return type carries construction failures as data. Today
ExecveErrormeans two different things: "the file doesn't exist" (a precondition, checked in the constructor) and "execve failed in the child" (an async failure, via the_errrelay). The redesign gives the precondition its own variant, so missing-file becomes a distinct error type returned synchronously by the factory, and leavesExecveErrorfor the actual execve failure. The two meanings are now distinct types, not just distinct delivery channels. A variant is also added for an unsupported kernel. Moving the construction failures to the factory also removes a major source of thefailedcallback's double meaning; what remains onfailedis the operational async errors (exec/chdir relay, write errors) plus a terminal reap error.Latent bugs this redesign also fixes
_pending_writes, which runs only while the pending queue is non-empty (so backpressure is applied), the non-EAGAIN write-error branch closes stdin without releasing backpressure, stranding the client. Routing every stdin close through_close_stdin()fixes it. (The matching branch in_write_finalruns only with an empty pending queue, where no backpressure is applied, so it doesn't strand.)_errfd/event leak on teardown. Today's_close()closes stdin, stdout, and stderr, but never_err. Closing all four on teardown fixes it, and it is also what makes draining_erron the reap edge necessary (see The reap edge).Decisions taken (open to discussion feedback)
These were settled deliberately while writing the plan. Push back on any of them in the comments:
_Reapedis a plain marker: the exit status goes out throughdispose(status)at the transition and isn't read afterward, so the state doesn't need to hold it (unlike the factory's error, which is a return value)._Processseam, adopted. See Testing.StartProcess,_Running/_Disposing/_Reaped, the unsupported-kernel error). Renaming is cheap and separable from the design.Implementation notes
EVFILT_PROCarm needs a shared asio flag (the backend keys its other filters on an fd), and the Windows bridge has to reconcile process-handle ownership with the reap (the current Windows wait closes the handle).forkthenpidfd_open, orclone3withCLONE_PIDFDto get the pidfd atomically at fork. Both are safe (the child's pid can't be recycled before we reap); an implementation choice, not a blocker.waitpidto collect the status and reap. Keep it non-blocking so it can't hold a scheduler thread, and pin theWNOHANG-without-WUNTRACEDassumption with a comment (a stopped child would otherwise be counted as exited)._ProcessNoneand its fabricatedExited(255)largely disappear: with the factory, the actor is never created without a real child.Testing
The key trap: asserting that
dispose(Exited(7))fires is not a discriminating test for #5764. On buggy code, when the grandchild eventually releases the pipes, the same status is reported, just late or never. A status-only assertion passes even on the bug. The only thing that separates correct from broken is time, so every #5764 and stdin test must be built around a categorical time separation (grandchild outlives the timeout, an unbounded self-terminating grandchild, a wideelapsedwindow), not a tuned latency threshold. The existing suite's discipline carries over: assert on the eventual observable outcome, use a generouslong_testtimeout as the hang detector, serialize withexclusion_group, and prove negatives with a bounded timer window.New tests: the silent grandchild (
sh -c "sleep 30 & exit 7", timeout well under the grandchild's life); the grandchild case delivering the child's buffered output; a flooding grandchild not stalling teardown (drain cap); a child that closes its output and blocks on stdin, completing only after a delayeddone_writing(); dispose-exactly-once against a reap race; and a coarse fd-leak stress loop. The factory also makes the precondition failures (capability, missing file, unsupported kernel) synchronous and directly assertable, with no notifier round-trip.The injectable
_Processseam is the primary way to verify the state machine, and it is what makes generative testing possible. The factory gives the actor its child as a value, so a package-private test factory can substitute a spy_Processthat recordskill()and returns a chosen status fromwait()on demand, with no public surface and no real process. On top of it, a PonyCheck property generates random orderings of the client-facing events (write,done_writing,dispose, "child exits now") and asserts the invariants hold for every ordering:disposeexactly once,kill()never after a reap (#5765), backpressure always released. This exercises the message-order races where the concurrency bugs occur (dispose-vs-natural-exit, write-after-exit, double-dispose), and since each behavior is atomic (the reap edge is one synchronous turn), message order is the whole race space. It does not fake pipe bytes: output delivery and the OS-level behavior (fd inheritance, a grandchild holding the pipes) stay with the real-process tests above. Two layers, then: property-based tests over the state machine, and real-process tests for integration.Regression set to keep green, each pinning a contract clause: the exec/chdir-failure path fires both
failedanddispose(strengthen these to assertfailed, given the_errdrop risk);disposenever fires when construction fails (now a factory error, so assert the error return); kill-while-running yieldsSignaled; and the stdin/stdout, ordering, expect, and #5748 tests.Scope and sequencing
One branch, event-based end state, no interim polling release, landed as a cutover. Reviewable sections:
pidfdexit event (Pony-only).EVFILT_PROCbackend extension and wiring.RegisterWaitForSingleObjectbridge, wiring, and handle ownership.There is no interim fix in released ponyc (PR #5763 was closed), so #5748 stays open until this lands, alongside #5764 and #5765.
Out of scope
failedcallback still carries two async meanings (an operational error, withdisposeto follow, and a terminal reap error). The factory removes the construction-failure case; the remaining split is pre-existing and preserved.expect(qty)set, a final sub-qtyresidual at EOF is not delivered — a pre-existing exception to "all of the child's output is delivered."All reactions