Skip to content

test(cloudxr): fix flaky cmdline-read race in test_background.py - #1209

Merged
gareth-morgan-nv merged 1 commit into
mainfrom
gmorgan/fix-background-test-flake
Oct 7, 2026
Merged

gareth-morgan-nv merged 1 commit into
mainfrom
gmorgan/fix-background-test-flake

Conversation

@gareth-morgan-nv

@gareth-morgan-nv gareth-morgan-nv commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes an intermittent CI failure in test_recognises_a_service_under_any_shipped_module_name, which fails with AssertionError: assert None == <pid> roughly 1 in 10 runs.

What changed

  • background.read_pid() reads /proc/<pid>/cmdline immediately after subprocess.Popen() returns, but on a loaded runner the child's /proc entry isn't guaranteed to be populated that instantly.
  • The test now polls read_pid() for up to 2 seconds instead of asserting on the very first read.

Validation

  • Confirmed this is a genuine, pre-existing race unrelated to any in-flight PR — reproduced the identical failure on unmodified main (commit 6960d4a59) and on test(cloudxr): reusable fake-adb test double (FakeAdb/mock_adb) #1147 (which doesn't touch background.py at all).
  • Reproduced the failure locally on the original test: ~11% failure rate across 70 runs (4 failures / 30 + 40 runs), hitting both the isaaccapture.cloudxr.service and legacy isaacteleop.cloudxr.service parametrizations roughly evenly — confirms it's a genuine timing race, not specific to either module string.
  • Stress-tested the fixed test: 60/60 passed, 0 failures, same machine/loop.
  • ruff check / ruff format --check clean.
  • pre-commit run (matching CI's SKIP=check-copyright-year hook set) clean.

Known limitations / follow-ups

None — this is a minimal, targeted fix to the test's own timing assumption; background.py's production logic is untouched.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

Summary by CodeRabbit

  • Tests
    • Updated automated checks to allow background processes time to start before verifying service recognition. This makes the check more resilient to startup timing differences while preserving the same expected behavior. No end-user functionality or behavior has changed.

test_recognises_a_service_under_any_shipped_module_name intermittently
failed with "assert None == <pid>" - background.read_pid() reads
/proc/<pid>/cmdline right after Popen() returns, but on a loaded CI
runner (e.g. parallel ctest matrix jobs) the child's /proc entry isn't
always populated by then. Poll briefly instead of asserting on the
first read. Confirmed pre-existing on main (reproduced the same way on
commit 6960d4a, unrelated to any in-flight PR); ran the fixed test 5x
with no failures.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Gareth Morgan <gmorgan@nvidia.com>
@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: NVIDIA/IsaacCapture/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 96fbe4cb-231a-4143-82f2-0b2f0623b81d
📥 Commits

Reviewing files that changed from the base of the PR and between bb59d39 and 7f36a22.

📒 Files selected for processing (1)
  • tests/python/core/cloudxr/test_background.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

The service-recognition test now polls background.read_pid() for up to two seconds. It waits 50 ms between reads and then asserts that the returned PID matches the spawned process.

Priority: ⬇️ Low

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

Merge Risk: ⚪ Minimal · up to 7f36a

No merge-blocking risk is identified in the described test-only change.

🚥 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 identifies the CloudXR test and the flaky command-line read race it fixes. It matches the main change in the pull request.
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.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@ivany-nv ivany-nv added the NVIDIA label Oct 7, 2026
@gareth-morgan-nv
gareth-morgan-nv enabled auto-merge (squash) October 7, 2026 22:00
@gareth-morgan-nv
gareth-morgan-nv merged commit c1d285c into main Oct 7, 2026
35 checks passed
@gareth-morgan-nv
gareth-morgan-nv deleted the gmorgan/fix-background-test-flake branch October 7, 2026 22:09

This branch was successfully deployed

1 active deployment
dev — 7f36a227 Deployed Oct 7, 2026 by gareth-morgan-nv via publish-wheel #5359
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants