fix(pty): bound terminal sessions and never block the loop killing one (F-05) - #50
Merged
Merged
Conversation
…e (F-05) The remainder of F-05, after PR #43 landed frame validation and a bounded output queue. Two things were left, and the second is the more serious. **Termination could hang the entire server.** `_terminate` sent SIGTERM and then called a blocking `os.waitpid` with no WNOHANG, directly on the asyncio event loop. A child that ignores SIGTERM froze not just its own tab but every request the server was serving, with no escalation and no timeout. That is a denial of service reachable by the terminal's own child process. `terminate()` is now a coroutine: SIGTERM, poll with WNOHANG until a grace period, then SIGKILL, then poll again. Giving up after the second grace leaves a zombie, which is the right trade: one process table entry against the hang this exists to remove. **Nothing counted sessions.** Every accepted websocket spawned a `replicant menu` process, so a page reconnecting in a loop, or a few tabs, multiplied real processes against one host. `SessionRegistry` caps them globally (4) and per client (2). The per-client cap is what stops one browser taking every slot and locking the operator out from another machine. A refusal returns a sentence, not a bool, and it is written to the terminal before the socket closes with 1013. A tab that silently fails to open is exactly the kind of thing that gets reported as "the web UI is broken", which this session has already demonstrated twice. This also bounds F-08 in passing: the eps cap is per-process, so capping processes caps how far it can be multiplied against one collector. It does not answer F-08, which still needs a decision. `_spawn_command` is split out of `_spawn` so the SIGTERM-ignoring case is actually testable. It could not be reached at all while the argv was hardcoded, which is why this defect had no coverage. The last guard asserts the shape, not just the timing: a timing test passes on a fast machine even when the blocking call comes back. 839 py. black, ruff, mypy clean. All 11 guards observed to fail first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The remainder of F-05, after PR #43 landed frame validation and a bounded output
queue. Two things were left, and the second is the more serious.
Termination could hang the entire server.
_terminatesent SIGTERM and thencalled a blocking
os.waitpidwith no WNOHANG, directly on the asyncio eventloop. A child that ignores SIGTERM froze not just its own tab but every request
the server was serving, with no escalation and no timeout. That is a denial of
service reachable by the terminal's own child process.
terminate()is now a coroutine: SIGTERM, poll with WNOHANG until a graceperiod, then SIGKILL, then poll again. Giving up after the second grace leaves a
zombie, which is the right trade: one process table entry against the hang this
exists to remove.
Nothing counted sessions. Every accepted websocket spawned a
replicant menuprocess, so a page reconnecting in a loop, or a few tabs, multiplied realprocesses against one host.
SessionRegistrycaps them globally (4) and perclient (2). The per-client cap is what stops one browser taking every slot and
locking the operator out from another machine.
A refusal returns a sentence, not a bool, and it is written to the terminal
before the socket closes with 1013. A tab that silently fails to open is exactly
the kind of thing that gets reported as "the web UI is broken", which this
session has already demonstrated twice.
This also bounds F-08 in passing: the eps cap is per-process, so capping
processes caps how far it can be multiplied against one collector. It does not
answer F-08, which still needs a decision.
_spawn_commandis split out of_spawnso the SIGTERM-ignoring case isactually testable. It could not be reached at all while the argv was hardcoded,
which is why this defect had no coverage.
The last guard asserts the shape, not just the timing: a timing test passes on a
fast machine even when the blocking call comes back.
839 py. black, ruff, mypy clean. All 11 guards observed to fail first.