Skip to content

fix(supervisor): serialize child registration with reaping - #3142

Open
drew wants to merge 3 commits into
mainfrom
codex/fix-detached-fast-exit
Open

fix(supervisor): serialize child registration with reaping#3142
drew wants to merge 3 commits into
mainfrom
codex/fix-detached-fast-exit

Conversation

@drew

@drew drew commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fix a supervisor child-reaping race exposed by the fast detached-main coverage in #3128. A child that exited between spawn() and managed-PID registration could be consumed by the orphan reaper, causing the canonical wait to fail with ECHILD instead of reporting the workload's real exit code.

Related Issue

No issue required: this is a localized process-supervision race fix exposed by #3128.

Changes

  • Hold the managed-child registry lock across child spawn and PID registration.
  • Hold the same lock across the orphan membership check and waitpid.
  • Apply atomic registration to canonical workload and SSH session children.
  • Add unit coverage for managed/unmanaged reap decisions.
  • Add a Docker e2e regression for an immediately exiting detached main process, including retained exit code 11.

Testing

  • mise run pre-commit passes
  • mise run test passes (with OPENSHELL_SYSTEM_GATEWAY_DIR pointed at an empty directory to isolate the host's installed gateway configuration)
  • cargo test -p openshell-supervisor-process passes (257 passed, 2 ignored)
  • Unit tests added/updated
  • E2E test passed 10 consecutive Docker runs

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (not applicable; this restores the existing lifecycle contract)

@elezar elezar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While reviewing the PR, I had an agent author a failing unit test that is fixed by the changes here. (This was verified locally and should not affect CI).

That means that I've updated this to the following stack:

  • test(supervisor): reproduce fast child reaping race is intentionally red when checked out before the next commit. The following fix changes it to use the atomic registration API, so the history documents both the original failure and its resolution. The PR tip remains green.
  • The PR commit.
  • test(supervisor): cover child registration reaper race provides the durable deterministic interleaving check for the new locking invariant.

In applying the PR commit, I had to make changes to the test, which triggered a deeper investigation. It seems as if clients have to be sure to switch from spawn to spawn_registered. This seems to indicate that callers may need to decide between two different implementations and may select the wrong one. I thus created #3154 to track a follow-up to reduce the likelihood of this happening.

@elezar elezar added the test:e2e Requires end-to-end coverage label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

Label test:e2e applied for f931959. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

elezar and others added 3 commits September 3, 2026 14:38
Signed-off-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: Evan Lezar <elezar@nvidia.com>
@elezar
elezar force-pushed the codex/fix-detached-fast-exit branch from f931959 to 0d3ac48 Compare September 3, 2026 12:39
@elezar

elezar commented Sep 3, 2026

Copy link
Copy Markdown
Member

Follow-up: #3156 is stacked on this PR and makes managed child lifecycle ownership structural across the canonical-process and SSH paths. It should be reviewed/merged after this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants