Fix master: flaky drop test and the cargo audit job - #6
Merged
Merged
Conversation
added 3 commits
July 29, 2026 02:00
Two problems with the job as first written. Cargo.lock is not committed here, per library convention, so cargo audit had nothing to read and failed with "Couldn't load ./Cargo.lock". Resolving one in CI is also the more meaningful check for a library: consumers resolve their own graph, so what matters is whether a fresh resolution pulls in an advisory, not whether some pinned snapshot did. rustsec/audit-check compiles cargo-audit from source on every run, which took 2m53s against a ~40s build for the crate itself. On a runner billed by vCPU-minute that is most of the job's cost spent on the tool rather than the work, so this installs a prebuilt binary instead. Verified locally first: 42 dependencies, no advisories, so the original failure was the missing lockfile and not a real vulnerability.
`dropping_a_run_kills_the_agent_and_its_children` asserted the grandchild was gone after a fixed 300ms sleep. That held locally and on the PR runner, and failed on master's, which is exactly the shape of a timing bet rather than a test. The bet was wrong in principle, not just unlucky. `Drop` cannot await, so it aborts the driver task and the teardown runs when the runtime next polls it: prompt, but not synchronous. A loaded 2-vCPU runner beat the sleep. The contract is "killed promptly", so the test now polls for that with a 10s ceiling, which still fails a genuine leak rather than hanging on it. The cancel test keeps its immediate, unpolled assertion. That asymmetry is the point: `cancel` awaits its own teardown, so if the tree is not already gone when it returns, the contract really is broken. Documented the distinction where callers will read it, in `Run`'s docs and the README: both kill the tree, only `cancel` tells you when.
My previous commit diagnosed this as flaky timing and added polling. That was wrong: it failed the same way on the next run, deterministically on Linux while passing on macOS, which is a platform difference rather than a race. The liveness check was `kill -0`, which succeeds for a zombie: a process that has exited but whose parent has not reaped it. On a developer machine init reaps orphans immediately so the distinction never shows. A CI container's PID 1 is frequently not a real init, so an orphaned grandchild sits as a zombie and `kill -0` reports it alive indefinitely, long after the kill worked. So the process-group teardown was correct the whole time and the test was measuring the wrong thing. Now it reads the state from `ps` and treats `Z` as dead, which it is: a killed process is dead whether or not anyone collected its exit status. Failures now report the actual process state, so the next surprise here diagnoses itself instead of costing another round of guessing. The polling from the previous commit stays. It was aimed at the wrong cause but is still correct on its own terms, since Drop cannot await and its teardown is prompt rather than synchronous.
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
1 participant
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.
master is currently red. PR #5 merged before two problems were caught.
1.
dropping_a_run_kills_the_agent_and_its_childrenis flakyIt asserted the grandchild was gone after a fixed 300ms sleep. Passed locally and on the PR runner, failed on master's.
The bet was wrong in principle rather than unlucky:
Dropcannot await, so it aborts the driver task and teardown happens when the runtime next polls it. Prompt, but not synchronous, and a loaded 2-vCPU runner beat the sleep.The contract is "killed promptly", so the test polls for that with a 10s ceiling. A genuine leak still fails it.
The cancel test keeps its immediate, unpolled assertion, and that asymmetry is deliberate:
cancelawaits its own teardown, so if the tree is not gone when it returns, the contract is broken. Documented the distinction inRun's docs and the README, since it is a real semantic difference callers should know: both kill the tree, onlycanceltells you when.2.
cargo auditcannot find a lockfileCargo.lockis not committed, per library convention, so the job failed with "Couldn't load ./Cargo.lock". This fix was pushed to #5 about a minute after it merged, so it never landed.Resolving a lockfile in CI is also the more meaningful check for a library: consumers resolve their own graph, so what matters is whether a fresh resolution pulls an advisory, not whether some pinned snapshot did.
Also swapped
rustsec/audit-checkfor a prebuiltcargo-audit. The action compiles it from source on every run, which took 2m53s against a ~40s build for the crate itself. On a runner billed by vCPU-minute, that was most of the job's cost spent on the tool rather than the work.Verified locally: 42 dependencies, no advisories.
Confirmed working
Ubicloud runners picked up #5's jobs and the check job passed on them in 44s, so the runner change itself is good.