Repository navigation
Conversation
Popen() can return while the child is still mid-exec, when its /proc/<pid>/cmdline reads empty. test_reads_real_cmdline_without_a_trailing_empty read the run flags at once and failed with `assert [] == ['--host-client']`. Replace the per-reader read_pid() poll with _await_exec(), which waits for the child's cmdline itself and fails clearly after 5s. Every test that reads a just-spawned child's cmdline now calls it first, including the two that expect no match: an empty cmdline made those pass without testing anything. Signed-off-by: Ivan Yang <yifanyang@nvidia.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly identifies the CloudXR test change and the command-line polling behavior addressed by the pull request. |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 1 files. (1 skipped: 1 unsupported.)
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
…dline On the posix_spawn path the child shares our memory until exec swaps it, so /proc/<pid>/cmdline can briefly read pytest's own argv. _await_exec then returned at once, and the next read could still miss the service. CPython 3.13 takes that path by default where os.POSIX_SPAWN_CLOSEFROM exists; uv's managed builds lack it, so CI is unaffected. Forced onto posix_spawn, the previous helper failed 7 of 10,000 runs of the cmdline tests; this one failed none. Signed-off-by: Ivan Yang <yifanyang@nvidia.com>
Description
cloudxr_test_backgroundfails intermittently onmain.TestReadRunFlags::test_reads_real_cmdline_without_a_trailing_emptyfailed withassert [] == ['--host-client']onbuild-ubuntu (Debug, 3.12, arm64)for #1372, which does not touch this code (run 40632105330). Fail-fast then cancelled the other build jobs.Popen()returns once the child has started exec, but its/proc/<pid>/cmdlinereads empty until the kernel has set up the new argv. The test read the run flags straight away, so in that windowread_pid()did not recognise the child andread_run_flags()returned[]. #1209 and #1327 fixed the same race for other tests in this file by pollingread_pid(); this one was missed._poll_read_pid()with_await_exec(pid), which waits for the child's cmdline itself. It gives up after 5 s withpytest.fail("/proc/<pid>/cmdline is still empty 5s after the spawn").test_none_when_pid_belongs_to_something_else,test_a_process_that_merely_names_the_module_is_not_the_service): an empty cmdline made them pass without exercising the matching at all, which pollingread_pid()cannot fix.tests/AGENTS.mdto say the wait comes before any check, negative ones included.No product code changes.
status,stopand the launcher read the cmdline of a service that finished exec long ago.start_and_wait()does callread_pid()right afterspawn()and shares this window, but this test does not cover it; that is product code and left for a separate change.Type of change
Testing
All on a 48-core x86_64 host, against a Release build of
main.uv run --python 3.12 --extra dev pytest -v --tb=short test_background.py→ 20 passed.pytest --count 300(pytest-repeat) on the failing test, understress-ng --cpu 96 --fork 16:assert [] == ['--host-client']--count 100under the same load: 32,000 passed, 0 failed.Popen()followed by an immediate cmdline read came back empty 1,824 of 16,000 times under the load above._await_exec()on an unreaped zombie fails after 5 s with the message above, and the test'sfinallystill reaps the child.posix_spawnpath (CPython 3.13's default whereos.POSIX_SPAWN_CLOSEFROMexists; forced here withclose_fds=False): the child can briefly show the parent's argv, so_await_execalso waits past that. Before: 7 of 10,000 cmdline-test runs failed; after: 0 of 10,000, and 0 of 12,000 understress-ngon both paths.SKIP=check-copyright-year pre-commit run --all-filespassed.This PR is itself the test fix, so it adds no new test.
Not covered yet: arm64, where CI failed; this PR's CI run covers it.
Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCOSummary by CodeRabbit