emrg: internal errors never reach the TUI screen, only the log (#1235) - #1248
Conversation
The host pasted a logging.Handler.handleError report from the TUI session area and stated the policy, not just the bug: internal exceptions belong in the log and must never appear in the TUI. The path was: send_command -> websockets' protocol logger -> that handler's emit raised (a RotatingFileHandler whose stream had gone bad) -> handleError wrote the report to sys.stderr - and for a TUI, sys.stderr IS the screen. The daemon already dies silently into ~/.emrg/emrgd-crash.log (_redirect_std_streams, rant 2026-08-25T09:25:32); the client died visibly because nothing had claimed its stderr. _contain_stderr_for_tui() + a _StderrContainment handle, installed by run_client() before asyncio.run and handed back only on a normal exit. Sink: <cwd>/.emrg/emrg-client-crash.log, separate from the structured client log for the same reason the daemon keeps emrgd-crash.log apart from emrgd.log. The containment is about the channel, not one cause: handleError, an unhandled traceback, asyncio's default handler and a third-party print(file=sys.stderr) all land in the same place. Handlers that already captured the terminal are repointed, root's AND named loggers' - StreamHandler(stream=None) binds sys.stderr at construction, and websockets.client is the logger in the reported traceback, so a root-only loop leaves the reported case uncontained. logging.lastResort is deliberately not walked (read-only stream property returning the current sys.stderr; assigning raises AttributeError from inside the call). loggerDict is snapshotted because it is live and a logger created mid-iteration would raise out of run_client. The messages a user must read are not contained: run_client captures the terminal stream before installing and passes it to interactive as console. Boundaries are stated in the docstring rather than implied - no containment when stderr is not a TTY, stdout untouched, no restore on an abnormal exit (that traceback is printed by the caller after the session), best-effort if the sink cannot be opened, and fd 2 written directly (faulthandler, C-level aborts) still reaches the terminal. tests/test_client_stderr_containment.py: 10 cases. Driven both ways - against master's unpatched app.py 9 fail and the single survivor is the case asserting the behaviour that must not change (a connect failure still reaches the user). Full suite 2003 passed, 1 skipped = master's 1994 + 10.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-000605
I drove the containment instead of reading it. Arms loaded with the sha of the loaded emrg/client/app.py printed beside the result: master 371398735a3ec7d1, this head ef8ac3f402531cd0.
Setup: a fake stream that claims to be a TTY and records every write, cwd a scratch dir, and a StreamHandler bound to that terminal on websockets.client before installation.
- master's
app.pyhas no_contain_stderr_for_tuiat all —hasattris False, so nothing can be contained, which is the bug as reported; - this head:
sys.stderrbecomes the sink, the pre-existing named-logger handler is repointed, and all three channels land in./.emrg/emrg-client-crash.log(313 bytes): an unhandled traceback, a record through the repointed handler, and a plainprint(..., file=sys.stderr); - the fake terminal received 0 characters — that is the claim the host made the policy about;
restore()puts the terminal stream back.
Two things I looked for because they are where this kind of change goes wrong, and both are handled: the repoint walks named loggers' handlers, not only root's (a root-only loop would have left the reported websockets.client case uncontained), and loggerDict is snapshotted; the messages a user must read are routed through the console stream captured before installation, so the "This client requires a real terminal" and "Failed to connect to emrgd" paths are not swallowed with the internal errors. The docstring states the boundaries instead of implying them (no containment when stderr is not a TTY, stdout untouched, fd 2 written directly still reaches the terminal), which is what makes the partial enforcement honest rather than quiet.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-013029
Reviewed 391fe74b; both states driven, not inferred. Its new file (tests/test_client_stderr_containment.py) is 10 passed on this head and 9 failed / 1 passed when copied verbatim into a worktree of unpatched master 7e7cd598 — and the single survivor is the assertion about behaviour that must not change (a connect failure still reaching the terminal), so the instrument discriminates instead of merely agreeing with the patch.
The containment is installed before asyncio.run and handed back only on a normal exit, the root and named loggers are repointed while logging.lastResort is deliberately left alone (stream is read-only there), and fd 2 written directly still reaches the terminal — boundaries stated in the docstring rather than left to be discovered. No test stops or restarts a daemon.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-020149
Reviewed 391fe74b; its test file re-run on its own arm this cycle — tests/test_client_stderr_containment.py is 10 passed, and the same file copied verbatim into a worktree of unpatched master is 9 failed / 1 passed, the survivor being the assertion that a connect failure still reaches the terminal (the behaviour that must not change). So the instrument discriminates rather than agreeing with the patch.
The containment is installed before asyncio.run, handed back only on a normal exit, repoints the root and named loggers while deliberately leaving logging.lastResort alone (its stream is a read-only property), and states its boundaries in the docstring rather than leaving them to be discovered: no install when stderr is not a TTY, stdout untouched, fd 2 written directly still reaches the terminal. No test stops or restarts a daemon.
Closes #1235.
The report
The host pasted a
logging.Handler.handleErrorreport straight from the TUI session area, and stated the policy, not just the bug:The traceback shows the path:
await conn.send_command("ping")→websockets/protocol.py,logger.debug("> %s", frame)→ that handler'semitraised (aRotatingFileHandlerwhose stream had gone bad) →handleErrorwrote the report tosys.stderr— and for a TUI,sys.stderris the screen.The daemon already solves its half of this:
_redirect_std_streams(rant 2026-08-25T09:25:32) makesemrgddie silently into~/.emrg/emrgd-crash.log. The client died visibly, because nothing had claimed its stderr.The fix
_contain_stderr_for_tui()+ a_StderrContainmenthandle, installed byrun_client()beforeasyncio.run— so records emitted while the event loop is being created are covered too — and handed back only on a normal exit. Sink:<cwd>/.emrg/emrg-client-crash.log, deliberately separate from the structured client log, for the same reason the daemon keepsemrgd-crash.logapart fromemrgd.log(unstructured, multi-line, written by code that is already failing).Because
handleErroris only one cause, the containment is about the channel, not that bug: an unhandled traceback, asyncio's default exception handler, and any third-partyprint(..., file=sys.stderr)all land in the same place.Handlers that already captured the terminal are repointed.
StreamHandler(stream=None)bindssys.stderrat construction, so a handler built before the swap keeps writing to the terminal afterwards. The repoint loop walks the root logger's handlers and those on named loggers —websockets.clientis the logger in the host's traceback, and a root-only loop leaves the exact reported case uncontained.logging.lastResortis deliberately not walked: it is a_StderrHandlerwhosestreamis a read-only property returning the currentsys.stderr, so it follows the swap by itself, and assigning to it raisesAttributeErrorfrom inside the containment call._handlers_bound_tosnapshotsloggerDicton purpose — it is live, and a thread creating a logger mid-iteration makes the view raise "dictionary changed size during iteration", out ofrun_client, onto the terminal.The messages a user must read are not contained.
run_clientcaptures the terminal stream before installing containment and passes it tointeractiveasconsole; "This client requires a real terminal (TTY)." and "Failed to connect to emrgd: …" go there, never tosys.stderr.Boundaries, stated rather than implied (in the function docstring):
sys.stderris not a TTYemrg 2>errors.txtasked for stderr there, and that terminal cannot be corruptedsys.stdoutrun_client's caller, after the session is over, and that is exactly what the host sawfaulthandler, C-level aborts)os.dup2and is left for its own changeVerification
Both directions, driven. The new tests run against master's unpatched
app.py(the patch to that one file stashed): 9 failed, 1 passed. The single survivor istest_a_connect_failure_still_reaches_the_terminal— the assertion about the behaviour that must not change. So the instrument is not simply "red on master": every containment assertion discriminates, and the one that guards against over-containing is green in both arms.emrg/client/app.pyat masterFull suite on this branch: 2003 passed, 1 skipped (= 2004 collected; master collects 1994, so the delta is exactly this file's +10 tests).
scripts/check-doc-count.pyrc=0. The sink path is gitignored (.gitignore:20), so running the tests leaves no dirt.No test stops or restarts a daemon, and none touches the upgrade chain.