Ensure daemon integration children are cleaned up on assertion failure

Breadway commented

2026-10-11 18:25:16 +08:00

Owner

Found while continuing RAM-29: crates/breadramd/tests/ipc.rs spawn_daemon returns a bare Child and stop is called only after assertions. A panic or readiness timeout drops Child without killing/waiting, leaving a live daemon and inherited harness pipes; command_surface.rs has an owner but duplicates readiness polling. Confidence: high, established by the bare Child lifetime and Rust Child drop semantics. Fix in RAM-29 existing branch: shared RAII daemon owner, suppressed inherited pipes, bounded readiness, and an unwind cleanup regression. Related to RAM-29 and distinct from the temporary-directory defect fixed by PR #19.

Breadway commented

2026-10-11 20:51:08 +08:00

Author

Owner

The correction this card asks for is contained in the RAM-29 branch: commit "fix: share bounded daemon test ownership and panic cleanup" gives the daemon integration suites a shared RAII owner that kills and reaps on panic, suppresses inherited pipes, bounds readiness, and adds an unwind regression, verifying child removal from /proc and socket removal.

It ships with the RAM-29 PR: #32

No separate implementation PR, per this card.

The correction this card asks for is contained in the RAM-29 branch: commit "fix: share bounded daemon test ownership and panic cleanup" gives the daemon integration suites a shared RAII owner that kills and reaps on panic, suppresses inherited pipes, bounds readiness, and adds an unwind regression, verifying child removal from /proc and socket removal. It ships with the RAM-29 PR: https://git.breadway.dev/Breadway/breadram/pulls/32 No separate implementation PR, per this card.

Breadway commented

2026-10-11 21:09:34 +08:00

Author

Owner

Reviewed, fix verified, left in review.

The RAM-56 correction on branch fix/ram-29-test-temp-cleanup (PR #32) is complete and correct:

A shared RAII daemon owner replaces the bare Child in both daemon suites; a panic unwinds through Drop, which kills and reaps the child, closes the inherited pipes (stdin, stdout and stderr at crates/breadramd/tests/support/mod.rs:20-24) and removes the socket it owns.
Readiness is bounded by a ten second deadline (support/mod.rs:60) instead of an effectively unbounded poll loop, and a timeout fails with the socket path named.
The restart case is preserved: stop() kills and reaps while deliberately keeping the stale socket (support/mod.rs:39-42).
The unwind regression asserts the child is gone from /proc and the socket is removed after an injected assertion failure, and that explicit stop reaps too (support/mod.rs:71-90).
ipc.rs and command_surface.rs both use the shared owner now, so the duplicate readiness polling is gone.

I am not moving this card to done yet, and it is not a problem with the fix: RAM-56 has no branch of its own, so it rides on PR #32, which I have sent back to todo as a partial delivery of RAM-29 (the shared fixtures and the capped pressure harness). When that branch lands, this fix lands with it. No action needed from the implementer on this card; keep the fix commits intact.

**Reviewed, fix verified, left in review.** The RAM-56 correction on branch fix/ram-29-test-temp-cleanup (PR #32) is complete and correct: - A shared RAII daemon owner replaces the bare Child in both daemon suites; a panic unwinds through Drop, which kills and reaps the child, closes the inherited pipes (stdin, stdout and stderr at crates/breadramd/tests/support/mod.rs:20-24) and removes the socket it owns. - Readiness is bounded by a ten second deadline (support/mod.rs:60) instead of an effectively unbounded poll loop, and a timeout fails with the socket path named. - The restart case is preserved: stop() kills and reaps while deliberately keeping the stale socket (support/mod.rs:39-42). - The unwind regression asserts the child is gone from /proc and the socket is removed after an injected assertion failure, and that explicit stop reaps too (support/mod.rs:71-90). - ipc.rs and command_surface.rs both use the shared owner now, so the duplicate readiness polling is gone. I am not moving this card to done yet, and it is not a problem with the fix: RAM-56 has no branch of its own, so it rides on PR #32, which I have sent back to todo as a partial delivery of RAM-29 (the shared fixtures and the capped pressure harness). When that branch lands, this fix lands with it. No action needed from the implementer on this card; keep the fix commits intact.

Breadway commented

2026-10-11 22:50:59 +08:00

Author

Owner

Re-verified at the current PR #32 head (6a8bd30), which added the shared core fixtures after the previous review. The RAM-56 correction is intact: crates/breadramd/tests/support/mod.rs still owns the child with a Drop that kills and reaps it and removes its socket, stop() reaps while preserving the stale socket for restart tests, readiness is bounded at ten seconds, and the unwind regression assertion_unwind_reaps_the_child still asserts the child is gone from /proc and the socket removed. Left in review: the fix rides on PR #32, which is still a partial delivery of RAM-29, so it lands with that card. No action needed on this card.

Re-verified at the current PR #32 head (6a8bd30), which added the shared core fixtures after the previous review. The RAM-56 correction is intact: crates/breadramd/tests/support/mod.rs still owns the child with a Drop that kills and reaps it and removes its socket, `stop()` reaps while preserving the stale socket for restart tests, readiness is bounded at ten seconds, and the unwind regression `assertion_unwind_reaps_the_child` still asserts the child is gone from /proc and the socket removed. Left in review: the fix rides on PR #32, which is still a partial delivery of RAM-29, so it lands with that card. No action needed on this card.

Breadway commented

2026-10-12 09:27:49 +08:00

Author

Owner

Re-verified at PR #32 head 363bb7f (rebased onto main 2d0965e, CI success, mergeable true). The RAM-56 correction is intact:

Shared RAII daemon owner in crates/breadramd/tests/support/mod.rs replaces the bare Child in both ipc.rs and command_surface.rs. Drop kills and reaps the child and removes its owned socket. Pipes are suppressed (stdin, stdout, stderr null).
Readiness is bounded by a ten second deadline with the socket path named on timeout.
stop() kills and reaps while preserving the stale socket for restart tests.
Unwind regression assertion_unwind_reaps_the_child still asserts the child is gone from /proc and the socket removed after an injected failure, plus explicit stop reaps.

What changed this pass: rebase onto main 2d0965e with the policy_replay.rs conflict resolved (shared support fixtures match main), and the PR #32 body updated to Part of RAM-29 with the remaining scope named. No regression in the RAM-56 scope.

Left in review: this fix rides on PR #32, which is a partial delivery of RAM-29 (PR body says Part of RAM-29), so it lands with that card. No action needed on this card. Parent RAM-29 next change: move the in-process fake provider and the action fixtures into crates/breadram-core/tests/support and point provider_execution plus action_coordinator at them.

Re-verified at PR #32 head 363bb7f (rebased onto main 2d0965e, CI success, mergeable true). The RAM-56 correction is intact: - Shared RAII daemon owner in crates/breadramd/tests/support/mod.rs replaces the bare Child in both ipc.rs and command_surface.rs. Drop kills and reaps the child and removes its owned socket. Pipes are suppressed (stdin, stdout, stderr null). - Readiness is bounded by a ten second deadline with the socket path named on timeout. - stop() kills and reaps while preserving the stale socket for restart tests. - Unwind regression assertion_unwind_reaps_the_child still asserts the child is gone from /proc and the socket removed after an injected failure, plus explicit stop reaps. What changed this pass: rebase onto main 2d0965e with the policy_replay.rs conflict resolved (shared support fixtures match main), and the PR #32 body updated to Part of RAM-29 with the remaining scope named. No regression in the RAM-56 scope. Left in review: this fix rides on PR #32, which is a partial delivery of RAM-29 (PR body says Part of RAM-29), so it lands with that card. No action needed on this card. Parent RAM-29 next change: move the in-process fake provider and the action fixtures into crates/breadram-core/tests/support and point provider_execution plus action_coordinator at them.

Breadway commented

2026-10-12 09:52:50 +08:00

Author

Owner

Verified on main 44a23a56be (PR #32 squash merge, contains 15a1c2f). The RAM-56 correction is present:

Shared RAII owner: crates/breadramd/tests/support/mod.rs defines Daemon { child, socket } with Drop that kills/reaps and removes the owned socket. Pipes suppressed (stdin, stdout, stderr null).
Bounded readiness: wait_until_ready uses a ten second tokio timeout and names the socket path on failure.
Unwind regression: assertion_unwind_reaps_the_child injects a panic, then asserts the child is gone from /proc and the socket is removed; explicit stop() reaps while preserving the stale socket for restart tests.
Both suites use it: ipc.rs spawn_daemon returns Daemon and uses the shared wait_until_ready; command_surface.rs imports Daemon and wait_until_ready from support, so the duplicate polling is gone.

Moving to done with RAM-29, which carried this fix.

Verified on main 44a23a56be5982bce3af25b25f0f31c01a09f7ed (PR #32 squash merge, contains 15a1c2f). The RAM-56 correction is present: - Shared RAII owner: crates/breadramd/tests/support/mod.rs defines Daemon { child, socket } with Drop that kills/reaps and removes the owned socket. Pipes suppressed (stdin, stdout, stderr null). - Bounded readiness: wait_until_ready uses a ten second tokio timeout and names the socket path on failure. - Unwind regression: assertion_unwind_reaps_the_child injects a panic, then asserts the child is gone from /proc and the socket is removed; explicit stop() reaps while preserving the stale socket for restart tests. - Both suites use it: ipc.rs spawn_daemon returns Daemon and uses the shared wait_until_ready; command_surface.rs imports Daemon and wait_until_ready from support, so the duplicate polling is gone. Moving to done with RAM-29, which carried this fix.

Breadway closed this issue

2026-10-12 09:52:50 +08:00

Rows
Columns

Ensure daemon integration children are cleaned up on assertion failure #23