fix(daemon): tear down serial session on ws early-return (#51)

[zackees]

Summary

Root-cause fix for #51 — the daemon was staying resident after autoresearch ended because handle_serial_ws leaked serial-session state on two failure paths after open_port had already created it.

Specifically:

• After open_port succeeds, if attach_reader returns None or socket.send(Attached) fails, the handler returned without running the cleanup block at the end. The SerialSession stays in SharedSerialManager.sessions forever.
• attach_reader additionally mutated session.reader_client_ids even on the None-return path, so has_clients() stayed true forever and the self-eviction loop never fired.

Changes

• Extracted session teardown into cleanup_ws_serial_session() (detach reader, release writer, close port if no remaining clients) and invoke it before both early-return paths.
• Made attach_reader all-or-nothing: no session mutation when it returns None.
• Regression test verifies the all-or-nothing contract.

Complements the diagnostic-logging PR (#58) — the logs introduced there will now correctly show Daemon staying alive: ... for cases this PR fixes, and Self-eviction triggered: ... will actually fire once the leaked session is cleaned up.

Test plan

• uv run cargo clippy --workspace --all-targets -- -D warnings clean
• uv run cargo test -p fbuild-serial --lib — 40 passed (1 new)
• uv run cargo test -p fbuild-daemon --lib — 88 passed
• CI green on Linux / macOS / Windows

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Fixed a bug where serial manager session state could be incorrectly modified when a broadcaster was unavailable.

• Refactor

  • Improved consistency and reliability of WebSocket serial monitor session cleanup across all exit paths.

[coderabbitai]

📝 Walkthrough

Walkthrough

Refactored WebSocket serial-session cleanup into a reusable helper function and updated handle_serial_ws to invoke it consistently across all exit paths. Concurrently, modified SharedSerialManager::attach_reader to validate broadcaster existence before mutating session state, with a regression test ensuring no leaked reader IDs on failure.

Changes

Cohort / File(s)	Summary
WebSocket Handler Refactoring crates/fbuild-daemon/src/handlers/websockets.rs	Extracted teardown logic into new cleanup_ws_serial_session helper that detaches readers, conditionally releases writers, and closes ports. Updated handle_serial_ws to call this helper on error paths and replace inline teardown, ensuring consistent cleanup semantics across all exits.
Serial Manager State Protection crates/fbuild-serial/src/manager.rs	Modified SharedSerialManager::attach_reader to perform broadcaster existence check before any session state mutation, returning None immediately when broadcaster is absent. Added regression test validating that session reader_client_ids remain unmodified when broadcaster is missing, preventing leaked reader IDs.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A helper born from scattered teardown lines,
Now cleanup flows through one design.
Early checks prevent the leaked state,
Broadcasters checked before too late.
Sessions safe, no ghost IDs to trace! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(daemon): tear down serial session on ws early-return (#51)' directly and clearly summarizes the main change: fixing a serial session teardown bug in the WebSocket handler on early-return paths.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-51-serial-session-leak

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

crates/fbuild-serial/src/manager.rs (1)

560-605: Good regression coverage for the #51 contract.

The test directly asserts the all-or-nothing invariant by constructing the exact pathological state (session present, broadcaster absent) that produced the leak. The assertion messages document the regression clearly.

Optional nit: constructing SerialSession inline with all fields makes this test brittle to unrelated field additions in SerialSession. Consider a SerialSession::new(port, baud) constructor (which already exists per Line 224) if it exposes enough surface for the test, to keep the test tethered to behavior rather than struct shape.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/fbuild-serial/src/manager.rs` around lines 560 - 605, The test builds
a full SerialSession struct inline which is brittle; replace the inline
construction in attach_reader_missing_broadcaster_does_not_mutate_session_state
with SerialSession::new(port, 115200) (the existing constructor around Line 224)
and then mutate only the necessary fields (e.g., ensure is_open=false,
writer_client_id=None, reader_client_ids empty, stop_flag set) before inserting
into SharedSerialManager::sessions so the test checks behavior without depending
on every SerialSession field layout; keep the call to mgr.attach_reader(port,
client) and the subsequent asserts unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/fbuild-serial/src/manager.rs`:
- Around line 560-605: The test builds a full SerialSession struct inline which
is brittle; replace the inline construction in
attach_reader_missing_broadcaster_does_not_mutate_session_state with
SerialSession::new(port, 115200) (the existing constructor around Line 224) and
then mutate only the necessary fields (e.g., ensure is_open=false,
writer_client_id=None, reader_client_ids empty, stop_flag set) before inserting
into SharedSerialManager::sessions so the test checks behavior without depending
on every SerialSession field layout; keep the call to mgr.attach_reader(port,
client) and the subsequent asserts unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d37b35a3-1743-464c-b74c-4c5f935090b2

📥 Commits

Reviewing files that changed from the base of the PR and between 28a2767 and 222a72c.

📒 Files selected for processing (2)

• crates/fbuild-daemon/src/handlers/websockets.rs
• crates/fbuild-serial/src/manager.rs