fix(daemon): close the serial port after deploy's post-flash monitor (#1426)

[zackees]

Fixes #1426.

Cause

detach_reader() only removes the client from reader_client_ids — it does not touch serial_handle. Both other detach sites follow it with close_port_after_grace_if_idle() to release the OS handle:

• handlers/operations/monitor.rs:361
• handlers/websockets.rs:101

The deploy path's post-flash monitor did not. So after every deploy the SerialSession and its file descriptor stayed open for the life of the daemon, and the next client got EBUSY on a board that was enumerated and perfectly healthy.

Why it was hard to see

Three things conspired:

• The error blames the device — open_port(...) exceeded 3s; serial driver may be wedged — so the natural response is to power-cycle or reflash a healthy board.
• lsof /dev/ttyACM0 shows nothing, because the fd lives in the daemon, not in the CLI process that appears to fail. Only walking /proc/*/fd finds it.
• It is intermittent in a way that tracks deploy duration, so it looks like a fault in whatever test happened to be running.

Evidence

Four distinct fbuild-daemon pids each held /dev/ttyACM0 across one FastLED bench session — every one of them after a deploy:

holder pid=414394 comm=fbuild-daemon
holder pid=434691 comm=fbuild-daemon
holder pid=544820 comm=fbuild-daemon
holder pid=551628 comm=fbuild-daemon

open() took 13.3 s while held and 0.00 s after killing the daemon. Killing it also let the same board deploy and pass RPC smoke immediately.

It broke a real test path. FastLED's autoresearch rp2350w --net-peer --ota failed at its first RPC because the companion board's port was still held by that same run's deploy:

Mode	Companion deploy	Outcome
--net-peer	96.5 s	passes — 6 full cycles, bidirectional HTTP
--net-peer --ota	25.7 s	first RPC to the companion times out

The shorter deploy loses the race against the leaked handle; the longer one wins it. That is the intermittency, and it is why the failure was initially attributed to OTA logic (FastLED#3956) rather than to serial lifetime.

Change

One call site, mirroring monitor.rs exactly — same has_clients guard, same 2 s grace, so a close → immediate reconnect still does not thrash the USB CDC handle. The comment records why this site differed, since that is the non-obvious part.

Verification

• cargo check -p fbuild-daemon --all-targets — clean

Not yet verified end-to-end on the bench: that needs a daemon built from this branch and installed ahead of the pinned release, which I would rather not do to a live fixture mid-session. The reproduction is deterministic (fbuild deploy, then check /proc/*/fd for a holder), so it should be quick to confirm.

Related: FastLED#3956 (the test path this unblocks), #1424 (same family — a healthy port reported unusable for a host-side reason).

🤖 Generated with Claude Code

https://claude.ai/code/session_01KkufoNxfnNRU9psT3R9F51

Summary by CodeRabbit

• Bug Fixes
  • Fixed deploy monitor sessions remaining open after the post-deploy reader detached.
  • Subsequent clients can now reconnect to healthy boards without encountering “resource busy” errors.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: bbeb7abc-2232-4466-a338-9742954df079

📥 Commits

Reviewing files that changed from the base of the PR and between 60cbc07 and fc4f042.

📒 Files selected for processing (1)

• crates/fbuild-daemon/src/handlers/operations/deploy.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The deploy handler now checks for attached monitor clients after post-deploy reader detachment. If no clients remain, it schedules the physical serial handle for closure after a two-second grace period.

Changes

Serial port cleanup

Layer / File(s)	Summary
Idle serial port cleanup crates/fbuild-daemon/src/handlers/operations/deploy.rs	After detach_reader, the handler checks has_clients. When no clients remain, it schedules close_port_after_grace_if_idle with a two-second grace period.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to fc4f0

Post-deploy monitoring now releases an idle serial handle after a short grace period while preserving immediate reconnects. The change is ready to merge with no identified current-head blocking risk.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the primary change: closing the serial port after the deploy post-flash monitor completes.
Linked Issues check	✅ Passed	The change addresses the serial file-descriptor leak in the deploy monitor path described by issue #1426. The provided context does not show changes for the issue's separate daemon-stop visibility or …
Out of Scope Changes check	✅ Passed	The change is limited to deploy-session serial cleanup and directly supports issue #1426. No unrelated changes are identified.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.

✨ 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/1426-deploy-serial-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.}

[zackees]

Flagging a verification gap before this gets merged, since it is approved.

What I actually ran: cargo check -p fbuild-daemon --all-targets — clean.

What I did not run: the fbuild-daemon test suite. I attempted it twice (cargo test -p fbuild-daemon, then narrowed to --lib -j 2); both were killed by host memory pressure while linking the daemon crate, so I have no test result either way.

That matters more than usual here because this change touches serial-session lifetime on the deploy path, which every flashed board goes through. If it regresses anything around session state or close generations, the blast radius is wide.

I am raising it because I found the same gap elsewhere today: a change of mine passed bash lint and had quietly broken a test suite I had not run. I would rather say so than let this merge on an assumption.

The change itself is small and mirrors existing code — handlers/operations/monitor.rs:361 and handlers/websockets.rs:101 already follow detach_reader() with close_port_after_grace_if_idle(); the deploy path's post-flash monitor did not. Same has_clients guard, same 2 s grace.

The bug is well evidenced. Four distinct fbuild-daemon pids each held /dev/ttyACM0 across one bench session, every one after a deploy; open() took 13.3 s while held versus 0.00 s after killing the holder; and it broke autoresearch rp2350w --net-peer --ota at its first RPC.

Someone with a machine that can link the crate should run cargo test -p fbuild-daemon before merge. Happy to retry here if host memory frees up.

[zackees]

Retracting the verification gap I flagged above — the tests run clean:

$ cargo test -p fbuild-daemon --lib
test result: ok. 250 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out

Together with cargo check -p fbuild-daemon --all-targets (clean), this PR is verified as far as the daemon crate goes.

I should correct the reason I gave for not running them earlier. I attributed the failed attempts to host memory pressure, and that was a misreading on my part: I was watching the free column, which drops as page cache fills with build artifacts. The machine has ~90 GiB available throughout, has never recorded a kernel OOM kill, and my session cgroup has no memory limit and zero pressure events. There was no shortage — the runs were being cut short by a supervisor heuristic, not by the host.

So the coverage gap I described is closed, and the caution about merging without it no longer applies.