fix(platform-ext): wait for the instance flock before reporting a stop - #987
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe process stop lifecycle now waits for inherited flock release after orphan cleanup, with timeout and regression tests. SIGKILL cleanup warns about unreaped children. Docker observation timing tests exclude image pulling from the measured window. ChangesInstance lock lifecycle
Sequence Diagram(s)sequenceDiagram
participant stop_instance
participant _sweep_orphans
participant InstanceProcess
participant InstanceFlock
stop_instance->>_sweep_orphans: reap orphan processes
_sweep_orphans->>InstanceProcess: SIGKILL tracked children
_sweep_orphans->>InstanceProcess: wait for killed children
stop_instance->>InstanceFlock: poll for release
InstanceProcess-->>InstanceFlock: release inherited flock
InstanceFlock-->>stop_instance: report lock free
stop_instance->>stop_instance: remove descriptor and return
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/nemo_platform_ext/src/nemo_platform_ext/local/process.py (1)
692-717: 🩺 Stability & Availability | 🔵 TrivialGating and logic look correct.
_wait_for_lock_releaseis only invoked on the success path (never in the early-return PermissionError/SIGKILL-timeout branches), matching the regression test's expectations. One note: worst case,stop_instancecan now block for SIGTERM timeout + SIGKILL wait + up to 2×_sweep_orphanstimeout (10s default) +_LOCK_RELEASE_TIMEOUT(5s) — worth keeping in mind for any caller-side timeout budgets (e.g. CLI/RPC callers ofstop_instance).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_platform_ext/src/nemo_platform_ext/local/process.py` around lines 692 - 717, Keep the current success-path gating and _wait_for_lock_release behavior unchanged. Review callers of stop_instance, especially CLI/RPC entry points, and increase or otherwise align their timeout budgets to accommodate the possible SIGTERM, SIGKILL, orphan-sweep, and lock-release wait durations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nemo_platform_ext/src/nemo_platform_ext/local/process.py`:
- Around line 598-610: Add a test case to TestSweepOrphans that exercises the
kill_sent path with psutil.wait_procs returning unreaped children, using the
configured timeout and asserting logger.warning reports the remaining child PID
after SIGKILL. Keep the existing escalation behavior and other test cases
unchanged.
---
Nitpick comments:
In `@packages/nemo_platform_ext/src/nemo_platform_ext/local/process.py`:
- Around line 692-717: Keep the current success-path gating and
_wait_for_lock_release behavior unchanged. Review callers of stop_instance,
especially CLI/RPC entry points, and increase or otherwise align their timeout
budgets to accommodate the possible SIGTERM, SIGKILL, orphan-sweep, and
lock-release wait durations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1b923dc8-59ee-4c74-a244-3a1d14145d83
⛔ Files ignored due to path filters (2)
sdk/python/nemo-platform/src/nemo_platform/local/process.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/local/test_daemon_lifecycle.pyis excluded by!sdk/**
📒 Files selected for processing (2)
packages/nemo_platform_ext/src/nemo_platform_ext/local/process.pypackages/nemo_platform_ext/tests/local/test_daemon_lifecycle.py
Contributor
|
This was referenced Jul 30, 2026
Merged
SandyChapman
added a commit
that referenced
this pull request
Jul 30, 2026
… window test_never_deployment_outlives_observe_wait_then_succeeds asserts that create_deployment returns within observe_timeout + 2.0s. It pre-pulls alpine:3.20 with a comment saying this keeps an uncached pull out of the timed window, but that is not what happens: the backend is built with pull_images=True, and create_deployment pulls unconditionally rather than only when the image is missing locally. The warm-up avoids re-downloading layers; it does nothing about the registry round-trip, which still lands inside the measurement. A fully cached alpine:3.20 pull measures ~1.7s locally, so the unfixed test runs at 2.85s against a 3.0s budget on a fast machine with a warm cache -- 0.15s of margin, all of it hostage to Docker Hub latency. CI has been tipping over it on main and on unrelated branches: run 30554298719 (main) 4.39s run 30550043391 (PR #987) 3.84s run 30549563915 (experimentalist-run-progress) 3.42s Build the backend with pull_images=False so the test's own pre-pull is what puts the image on the host, and the timed window covers container create, start, and the observe wait -- what the assertion is actually about. Measured 1.18s across three runs afterwards, stable, with 1.82s of headroom. This only changes the one helper used by that test; the other three tests in the file keep the default pull_images=True. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
added a commit
that referenced
this pull request
Jul 30, 2026
The sweep now warns when a child outlives the post-SIGKILL wait, but TestSweepOrphans only reached the escalation path -- the warning branch itself was uncovered. Nothing survives SIGKILL on demand. The real trigger is a process wedged in uninterruptible sleep, which a test cannot arrange, so `wait_procs` is stubbed to keep reporting the child as alive. The terminate and kill calls stay real; only the observation is simulated. Verified the test is a genuine guard: with the `logger.warning` call removed it fails on the empty caplog, and passes with it restored. Raised by CodeRabbit on #987. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline 97394dc7bf
git show --format=fuller --find-renames --find-copies 97394dc7bf -- \
packages/nemo_platform_ext/src/nemo_platform_ext/local/process.py \
packages/nemo_platform_ext/tests/local/test_daemon_lifecycle.py
ast-grep outline packages/nemo_platform_ext/src/nemo_platform_ext/local/process.py \
--items all --type function --match 'sweep_orphans'
ast-grep outline packages/nemo_platform_ext/tests/local/test_daemon_lifecycle.py \
--items all --type class,function --match 'SweepOrphans|sweep_orphans'Length of output: 1221 🏁 Script executed: #!/bin/bash
set -euo pipefail
git diff-tree --no-commit-id --name-status -r 97394dc7bf
git show --format= --find-renames --find-copies 97394dc7bf -- \
':(glob)**/test_services_process.py'
sed -n '550,640p' packages/nemo_platform_ext/src/nemo_platform_ext/local/process.py
fd -a 'test_services_process\.py$' . -x sh -c '
printf "\n=== %s ===\n" "$1"
ast-grep outline "$1" --items all --type class,function --match "SweepOrphans|sweep_orphans"
' sh {}Length of output: 9343
✏️ Learnings added
You are interacting with an AI system. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nemo_platform_ext/tests/cli/commands/test_services_process.py`:
- Around line 871-878: Update the _sweep_orphans test to mock psutil.wait_procs
with a two-item side_effect representing both wait phases, then capture the mock
and assert it was called twice. Preserve the existing killed-process and
warning-log assertions while ensuring the test verifies the post-SIGKILL wait
occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 35eba9ef-a111-4785-8e7c-484417983f28
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/commands/test_services_process.pyis excluded by!sdk/**
📒 Files selected for processing (1)
packages/nemo_platform_ext/tests/cli/commands/test_services_process.py
SandyChapman
added a commit
that referenced
this pull request
Jul 30, 2026
The mock returned the same value for every call, so the test passed even against an implementation that logged after a single wait_procs -- it did not actually guard the post-SIGKILL wait this PR introduces. Use a two-item side_effect and assert wait_procs is called twice. Verified against a single-wait implementation: the warning still fires, but the test now fails on call_count. Raised by CodeRabbit on #987. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
`stop_instance` returned as soon as the descriptor PID was dead, but a process exiting and its flock being released are not the same instant — the kernel drops the lock while closing fds during teardown, and any process that inherited the fd keeps it held until it is gone too. Callers treat a successful stop as "the scope is free now" and probe with `is_instance_alive` immediately, so they raced that teardown. This is what has been failing `test_daemonize_services_spawns_child_that_becomes_ready` intermittently on main (`assert not True` at the post-stop liveness check); it went red on d478469 and bef8f45 among others. Two fixes: - `_sweep_orphans` escalated to SIGKILL and returned without reaping the survivors, so it could hand back control while a killed child still held the lock. It now waits on the processes it killed and warns about any that outlive the wait. - `stop_instance` now holds its own post-condition: it polls until the flock is actually free (bounded by `_LOCK_RELEASE_TIMEOUT`) before removing the descriptor, and warns rather than failing if something outside the sweep still holds it. The new integration test fails against the old code with the same `assert not True` signature seen in CI, and passes with the fix. Unit tests cover `_wait_for_lock_release` directly and both wait phases of `_sweep_orphans`. Also unflakes the second integration test that has been red on main, `test_never_deployment_outlives_observe_wait_then_succeeds`. Two problems, neither of them timing jitter: - `create_deployment` pulls the image unconditionally rather than only when it is missing, so a warm cache did not keep Docker Hub latency out of the timed window. The backend is now built with `pull_images=False` and the test pre-pulls. - The assertion could not distinguish the two outcomes it was meant to separate. With a 5s job and a 1s observe wait, "returned during the observe wait" and "blocked until the job exited" were only 4s apart, less than the cost of container create/start on a contended CI daemon (~3s observed). The job now runs 20s, so blocking for it is unmistakable, and the bound is deliberately loose — the STARTING assertions are what pin the real behaviour. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
fix-stop-instance-lock-race/schapman
branch
from
July 30, 2026 17:00
c5af9fe to
c8244c8
Compare
anastasia-nesterenko
approved these changes
Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
stop_instancereturned as soon as the descriptor PID was dead. But a process exiting and its flock being released are not the same instant — the kernel drops the lock while closing fds during teardown, and any process that inherited the fd keeps it held until it is gone too. Callers treat a successful stop as "the scope is free now" and probe withis_instance_aliveimmediately, so they raced that teardown.Why now
This is what has been failing
test_daemonize_services_spawns_child_that_becomes_readyintermittently on main, withassert not Trueat the post-stop liveness check:d47846975test_daemonize_services_spawns_child_that_becomes_readybef8f45a2test_daemonize_services_spawns_child_that_becomes_ready(The
Python integration testsjob has been red on several recent main runs. The second recurring failure — the docker-backend observe-wait test — is also fixed here; see below.)The fix
_sweep_orphansescalated to SIGKILL and returned without reaping the survivors, so it could hand back control while a killed child still held the lock. It now waits on the processes it killed, and warns about any that outlive the wait.stop_instancenow holds its own post-condition: it polls until the flock is actually free (bounded by_LOCK_RELEASE_TIMEOUT = 5.0) before removing the descriptor. If something outside the sweep still holds it, that is logged as a warning rather than turned into a failure — the PID we targeted did exit, and the old behaviour was to say nothing at all.Also here: the docker-backend observe-wait flake (folded in from #989)
These two PRs blocked each other, so #989 is closed and its commit lives here.
test_daemonize_services_spawns_child_that_becomes_readytest_never_deployment_outlives_observe_wait_then_succeedsEach fixed the other's blocker, so neither could go green alone.
test_never_deployment_outlives_observe_wait_then_succeedsassertscreate_deploymentreturns withinobserve_timeout + 2.0s, and pre-pullsalpine:3.20with a comment claiming this keeps the pull out of the timed window. It does not: the backend is built withpull_images=True, andcreate_deploymentpulls unconditionally rather than only when the image is missing locally. The warm-up avoids re-downloading layers, not the registry round-trip.A fully cached
alpine:3.20pull measures ~1.7s, leaving the test at 2.85s against a 3.0s budget on a fast machine with a warm cache. Building that one backend withpull_images=Falsemoves it to 1.18s:Already validated in CI on #989's run, where the docker test passed and dropped off the slowest-25 durations. Only the helper used by that one test changes; the other three tests in the file keep
pull_images=True.Combining a production fix with an unrelated test-only change in one PR is not a good default, and the added commits are self-contained if you would rather review them separately.
Tests
test_stop_instance_releases_lock_held_by_surviving_child— the regression guard. Verified it fails against the old code with the sameassert not True/is_instance_alivesignature seen in CI, and passes with the fix (5/5 runs).The lock holder is deliberately outside the parent's process tree. A first attempt used a SIGTERM-ignoring child inside the tree, which was faithful to the CI shape but passed against the unfixed code locally — the post-SIGKILL window is scheduler-dependent and sub-millisecond on a fast machine. An independent holder makes the window fixed, and models the case the sweep genuinely cannot reach: a lock-inheriting process spawned after the child snapshot was taken.
test_wait_for_lock_release_blocks_until_holder_exits/..._times_out_while_held— unit coverage for the new helper.test_warns_when_child_survives_sigkill— covers the new post-SIGKILL warning branch in_sweep_orphans, whichTestSweepOrphansdid not reach (raised by CodeRabbit). Nothing survives SIGKILL on demand — the real trigger is a child wedged in uninterruptible sleep — sowait_procsis stubbed while the terminate and kill calls stay real. It assertswait_procsis called twice, so a single-wait implementation cannot satisfy it: verified that against a deliberately single-wait build, where the warning still fires but the test fails oncall_count.test_daemon_lifecycle.py+test_services_process.py: 77 passed.test_docker_backend.py: 4 passed.ruff check/ruff formatclean;lint-python-typesandlint-sdk-vendoredexit 0.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests