refactor(supervisor): make managed child ownership structural #3154

Activity

1. elezar commented on Sep 3, 2026

   

   elezar

   on Sep 3, 2026

   Last edited by elezar

   MemberAuthor

   More actions

   🏗️ build-plan

   Implementation Plan

   Issue type: refactor
   Complexity: Medium
   Confidence: High — the RFC 0012 ownership boundary is now established and the remaining manual child lifecycle paths are identifiable.

   Summary

   Replace the managed-child convention with a generic, cross-platform ManagedChild<C> owner at the post-RFC 0012 sandbox boundary. Linux orphan-reaper registration remains internal to the wrapper, while canonical-process and boundary-exec callers use the same managed Tokio/std child lifecycle API on every platform.

   Scope

   • crates/openshell-sandbox/src/managed_children.rs: provide the cross-platform wrapper while preserving the generation-safe Linux registry and orphan-reaper mechanics.
   • crates/openshell-sandbox/src/process.rs: migrate the canonical Tokio child path to the wrapper on every platform.
   • crates/openshell-sandbox/src/boundary_exec.rs: migrate piped and PTY boundary-exec child paths, which now serve SSH execution after RFC 0012, to the standard-child wrapper.

   Implementation Steps

   1. Keep ManagedChild<C> as the single owner of child PID registration, wait cleanup, drop cleanup, and lost-child diagnostics.
   2. Preserve the separate generation-bearing registration token for boundary operations that do not own a child handle.
   3. Route canonical process spawning, stdio extraction, waiting, and try_wait through ManagedChild<tokio::process::Child>.
   4. Route both piped and PTY BoundaryExec spawning, stdio extraction, failure cleanup, and waiter ownership through ManagedChild<std::process::Child>.
   5. Remove manual registration and unregistering from child-owning call sites while retaining the existing terminal-observation and signal-ordering guarantees.
   6. Add or update deterministic lifecycle tests and retain the PR fix(supervisor): serialize child registration with reaping #3142 reaper-race coverage.

   Test Plan

   • Unit tests: cross-platform Tokio/std lifecycle behavior; Linux registration before return, successful and failed wait cleanup, try_wait(None) retention, and existing boundary-exec coverage.
   • Integration tests: run the full openshell-sandbox test suite to exercise canonical and boundary execution composition.
   • E2E tests: no new E2E changes; retain the detached-fast-exit regression coverage already merged from PR fix(supervisor): serialize child registration with reaping #3142.

   Risks & Open Questions

   • Do not expose unrestricted raw-child access or into_inner, which would let callers bypass lifecycle cleanup.
   • Preserve BoundaryExec's terminal observation before final reap and its signal lock ordering.
   • Keep Linux-only reaper behavior internal rather than leaking platform-specific child types into callers.
   • Tests that fork or execute system binaries may behave differently under SELinux enforcement, but this refactor does not add new /proc/<pid>/exe inspection or cross-label behavior.

   Documentation Impact

   None expected — no user-facing behavior, configuration, or workflow changes.

   ---

   Revision 3 — adapt ownership scope to the RFC 0012 sandbox and BoundaryExec architecture
   Revision 2 — cross-platform ownership model
2. elezar commented on Sep 3, 2026

   

   elezar

   on Sep 3, 2026

   MemberAuthor

   More actions

   Implementation is available in stacked PR #3156 (base: #3142).

   It applies the cross-platform ManagedChild<C> ownership model from the plan: Linux reaper registration stays internal, while canonical-process and SSH callers use the same managed Tokio/std child lifecycle API on every platform. The PR includes cross-platform std/Tokio lifecycle tests and retains Linux-only reaper-race coverage.
3. elezar

   added 2 commits that reference this issue on Sep 18, 2026

   refactor(supervisor): make managed child ownership structural

   ...

   This issue will close once commit 98a7639 is merged into the 'main' branch.

   98a7639

   refactor(sandbox): manage boundary exec child ownership

   ...

   This issue will close once commit d11fc8d is merged into the 'main' branch.

   d11fc8d
4. elezar commented on Sep 18, 2026

   

   elezar

   on Sep 18, 2026

   MemberAuthor

   More actions

   🏗️ build-from-issue-agent

   Implementation Updated

   PR: #3156

   What changed

   The implementation is now aligned with RFC 0012. Both the canonical sandbox process and the BoundaryExec path used by SSH own their children through the cross-platform ManagedChild<C> abstraction. Raw Linux registry mutation is private, while terminal observation, signal ordering, process-group tracking, and cancellation behavior remain intact.

   Tests

   • Unit/integration: cargo test -p openshell-sandbox — 83 passed, 1 expected ignored
   • Pre-commit: mise run pre-commit — passed
   • E2E: not rerun; no E2E files changed, and the detached-fast-exit regression from PR fix(supervisor): serialize child registration with reaping #3142 is already on main

   Docs updated

   • None needed; no user-facing behavior or configuration changed.

   The issue will auto-close when the PR is merged.
5. github-actions commented on Oct 2, 2026

   

   github-actionsbot

   on Oct 2, 2026 – with GitHub Actions

   More actions

   This issue has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity. Comment or remove the state:stale label to keep it open.
6. github-actions

   added

   state:staleInactive item at risk of automatic closure.Inactive item at risk of automatic closure.

   on Oct 2, 2026