Fix a daemon connection leak when a subscribed client hangs up - #116
Open
isactornberg wants to merge 2 commits into
Open
isactornberg wants to merge 2 commits into
isactornberg wants to merge 2 commits into
Conversation
…he next broadcast, and a client that subscribes no longer misses a change made while its Snapshot was taken - A subscribed client that disconnected (a TUI that quit, the idle check `nebula upgrade` makes) kept its socket open in the daemon until the next broadcast: the task forwarding broadcasts to it held a sender and only noticed on its next send, so the connection's writer never ended. An idle daemon collected one socket per such client. `handle_client` now keeps that task's handle and aborts it when the client's requests end, the way it aborts the attach forwarders. A second `Subscribe` on one connection replaces the first forwarder instead of adding one. - `Subscribe` now subscribes to the broadcasts before it takes the Snapshot, not after. A status change or an upsert made between the two used to reach nobody; now it arrives after the Snapshot. A client folds those in by id, so one the Snapshot already carries changes nothing. Tests: a_subscribed_client_that_hangs_up_is_let_go_by_an_idle_daemon (fails without the fix: the daemon never closes its half).
…spawn test keeps Codex's hooks out of the developer's own ~/.codex, and the CRUD test no longer takes the echo of its typed line for the command's output - nebula_spawn_cli_starts_a_sibling_session_in_the_same_worktree spawns a Codex sibling, and a Codex spawn installs nebula's managed hooks into Codex's home. The test's daemon ran with no `CODEX_HOME`, so a run installed them into `~/.codex/hooks.json` on the machine it ran on. It now pins `CODEX_HOME` to its own temp dir, as codex_hooks_install_and_drive_status already does. - full_crud_attach_and_restart_persistence typed `echo <marker>; pwd` and read on until the marker had shown twice, taking the second for the command's output. A line typed before the shell's prompt is up is echoed by the tty and again when the line editor redraws it, so the marker showed twice before the command had run, and the cwd assertion that follows failed: about 1 run in 20 on its own, more with the rest of the file running. It now types `pwd; echo <marker>` and waits for the marker at the start of a line, which only the command prints, after `pwd`'s output. Test-only.
isactornberg
changed the title
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
1 participant
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.
Fixes a connection leak in the daemon and a small race in
Subscribe, plus two tests that depended on the machine they ran on.handle_clientcouldn't finish. On an idle daemon that can be a long time. I ran 60 one-shot subscribers against an idle daemon and it was left with 60 extra open sockets, all released by the next status change. With the fix it stays at 4 before and 4 after. The forwarder is now aborted at cleanup, the same way the attach forwarders are.~/.codex/hooks.json, so it pinsCODEX_HOMEnow. Andfull_crud_attach_and_restart_persistencefailed 3 of 40 runs on a cleanmainhere, because it took the echo of its typed line for the command's output.How I checked it
a_subscribed_client_that_hangs_up_is_let_go_by_an_idle_daemonfails onmainand passes here.cargo fmt --all -- --checkis clean and clippy shows the same 11 warnings asmainon my toolchain.cargo test --workspacepasses, with two exceptions that aren't from this change.nebula_open_from_inside_a_session_raises_the_file_tabsandtui_drag_past_the_pane_top_autoscrolls_and_copies_the_runine2e_tuifail the same way on a cleanmainfor me. And onebranch_switchunit test flaked once and passed on a rerun; that one is onmaintoo (aconfig.rstest setsPATHfor the whole process while those tests spawngit).It's 16 lines in
server.rsand the rest is tests. No protocol or store change, so a revert undoes all of it.A question while I'm here
I found this while driving nebula from a script. I talk to one hub agent that hands work to other agents, and I'd like those to be nebula sessions so they show up on the grid like everything else I run. To get there I built
nebula treeandnebula session start | wait | read | send | deleteon a branch: https://github.com/isactornberg/nebula/tree/sessions-from-a-shellIt only uses requests the daemon already answers, and it leaves
nebula spawnalone. But it's not small: about 2,800 lines, roughly half of them tests.Would you want that upstream? Whole, in parts, or would you rather build it your own way? Any of those is fine by me. It already works for me as a second binary next to a stock nebula, so no pressure at all.