Repository navigation
Conversation
jussielo-amd
left a comment
There was a problem hiding this comment.
Nice fix — the FIFO-parked-shell decoy is a cleaner way to dodge the multicall-coreutils cmdline matching problem than the old fs::copy approach, and I confirmed the match logic (cmdline_matches_rule) only ever looks at /proc/<pid>/cmdline, never the file content, so dropping that copy step doesn't lose any coverage. Cleanup, the selftest_require_alive wiring across the other decoy-based fixtures, and the zombie fixture correctly staying unaffected all check out too.
One small thing I noticed, non-blocking: in the self-test liveness check (workflow_contract.rs around line 2868), there's a narrow TOCTOU window — if the decoy process exits between try_wait() and the /proc/<pid>/cmdline read, the code falls into the assert!(named, ...) branch and panics claiming the process "is alive but its command line does not start with {argv0}", when actually it just exited. It's a sub-millisecond race on an already-rare path, so it'd only ever mislead someone debugging a future flake rather than cause a false pass/fail — not something that needs to hold up this PR, but might be worth a follow-up if you want the panic message to stay accurate under that edge case.
Two things before this is mergeable, separate from the code review itself:
- GitHub currently shows this PR as having merge conflicts with the base branch — will need a rebase.
- The only check reporting here is the Read the Docs build; I don't see the usual
cargo test/clippygates on this PR, so I can't confirm they've actually run.
Approving on the logic/design itself — thanks for tracking down the root cause on ubuntu-26.04. Just flagging the conflict and CI status so they don't get missed before merge.
The reclaim decoys now copy no binary and fork no child. The xtask preflight test runs /bin/sh under the decoy path as argv[0], parked in the `read` builtin on a pipe the test holds; reclaim-gpu.sh's spawn_decoy runs its own bash the same way via `exec -a`, reading a FIFO it holds open read-write. Killing the one pid leaves nothing behind (the bash-loop decoys left a `sleep 1` child for up to a second), and the xtask decoy exits by itself if the test dies first. A decoy that dies at spawn - as a copied multicall `sleep` does on ubuntu-26.04 - used to surface as a gate or selection failure. Both tests now check each decoy is alive before reading it and fail naming the decoy, and the xtask test checks the decoy outlived the scan before trusting a clear verdict. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
ubuntu-latest moved from 24.04 to 26.04 without notice and turned every merge-group run red. Name the images instead. build-and-test, Test (affected crates), E2E tests and the remote-ssh lane run on ubuntu-24.04 and ubuntu-26.04. Branch protection matches required checks by job name, so the 24.04 leg keeps each job's bare name and only the 26.04 leg gets an " (ubuntu-26.04)" suffix. Every other hosted Linux job is pinned to ubuntu-24.04 with a comment saying why one LTS is enough: lint, metadata, reporting and publishing jobs, clippy, the coverage measurement, and the release builds, which run in the manylinux container. Compile caches are now keyed by image. rust-cache keys on "Linux-x64" only and sccache on the compiler binary, the same rustup download on every image, so a proc macro or build script linked against 26.04's glibc would otherwise be restored on 24.04. E2E tests now runs its scenarios whenever build-and-test reached a verdict, not only on success: that result aggregates both images, and a 26.04 failure must not let the required 24.04 leg report green without running. The 26.04 leg's report is uploaded under a name the consolidated report does not read, since that report has one mock platform. New workflow contract tests: no workflow names ubuntu-latest, the split jobs keep their required names on 24.04, and hosted Linux compile caches name the runner image. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
a81658b to
db1e550
Compare
GitHub repointed
ubuntu-latestfrom ubuntu-24.04 to ubuntu-26.04 (imageubuntu26/20261012.163). The last merge group to pass before the switch, #710, ran on 24.04; every merge-group run after it went red, with no change in this repository. Ubuntu 26.04 ships uutils coreutils, whose/bin/sleepis a multicall binary. The reclaim decoys were copies of it namedllama-server, so they printedcoreutils: unknown program 'llama-server'and exited at once. That failedpreflight_runs_the_real_reclaim_assert_clearin Test (affected crates) and Coverage, andreclaim-gpu.sh --self-testin build-and-test. #712 fixed the decoys by copying bash and running asleep 1loop.This PR builds on #712 in two ways.
1. Fork-free decoys that must be seen alive
/bin/shwith the decoy path as argv[0] (CommandExt::arg0). The shell waits in thereadbuiltin on a stdin pipe the test holds.spawn_decoyinreclaim-gpu.shruns its own bash the same way, viaexec -a, and reads a FIFO it holds open for both reading and writing. Killing the one pid leaves nothing behind; fix(ci): keep the GPU reclaim decoys alive on multicall coreutils #712's loop left asleep 1child running for up to a second. The xtask decoy also exits by itself if the test dies before killing it, because the pipe closes. Running the realsleepunder another argv[0] is not an option: uutils dispatches on argv[0] on some targets and on the executed path on others.cp /bin/sleepdecoy on ubuntu-26.04, the self-test now opens with:the decoy engine process (pid=…) exited at spawn (exit status: 1) ….2. Linux CI on both LTS images instead of
ubuntu-latestbuild-and-testbuild-and-test,build-and-test (ubuntu-26.04)testTest (affected crates),Test (affected crates) (ubuntu-26.04)e2eE2E tests,E2E tests (ubuntu-26.04)remote-sshremote control channel (containerised),… (ubuntu-26.04)Required checks. Branch protection matches required checks by job name. The 24.04 leg of each split job therefore keeps its exact current name, using
name: ${{ matrix.os == 'ubuntu-24.04' && '<name>' || format('<name> ({0})', matrix.os) }}. I checked that every required context is still produced by exactly one job.changesis the exception: it appears twice, once inci.ymland once ine2e-selfhosted.yml, and was already duplicated on main. For an admin: to make 26.04 blocking, add these contexts as required:build-and-test (ubuntu-26.04)Test (affected crates) (ubuntu-26.04)E2E tests (ubuntu-26.04)remote control channel (containerised) (ubuntu-26.04)is not required today, and neither is its 24.04 leg.Single-LTS jobs. Each of these has a trailing comment on its
runs-onsaying why one image is enough:Test (affected crates)runs the tests on both images);ubuntu-latestalso keeps the image they run on from changing silently.Caches keyed by image. rust-cache keys only on
Linux-x64, and sccache keys on the compiler binary, which is the same rustup download on every image. Without the image in the key, a target directory or proc macro linked against 26.04's glibc would be restored on 24.04, and caches written by 26.04 runs underubuntu-latestwould be restored onto the jobs now pinned to 24.04. Every hosted Linuxsetup-rust-toolchainnow passescache-keywith the image. The sccache prefixes becomerocm-cli/build-and-test/<image>; clippy shares the 24.04 one. The coverage and cargo-aboutactions/cachekeys now includeubuntu-24.04. As a result, the first run after merge starts with cold caches.E2E gate.
E2E testsused to run its scenarios only whenbuild-and-testsucceeded. That result now combines both images, so a failure on 26.04 alone would have let the required 24.04 leg report green without running anything. The gate now runs the scenarios wheneverbuild-and-testreached a verdict.E2E artifacts. The 24.04 leg still uploads
e2e-report, which feedsE2E consolidated report. The 26.04 leg uploadsubuntu-26.04-e2e-results, which is for inspection only. The consolidated report has a single mock platform, and two inputs labelled as it would collide. Showing both images in that grid needsparse_descriptorsupport and is left as a follow-up.Contract tests in
xtask/src/workflow_contract.rs:no_workflow_runs_on_the_moving_ubuntu_latest_labelscans every workflow file.moving_ubuntu_label_detector_flags_values_but_not_commentscovers the detector itself.lts_split_jobs_keep_their_required_name_on_24_04pins the exact name expression,runs-on: ${{ matrix.os }}, both images andfail-fast: falsefor the four split jobs.hosted_linux_compile_caches_are_keyed_by_runner_imagechecks that rust-cachecache-key,SCCACHE_AZURE_KEY_PREFIXand cargoactions/cachekeys name the job's image.I mutation-checked each test. Restoring
ubuntu-lateston one job, using a plain matrix name, dropping 26.04 from the matrix, droppingfail-fast: false, and removing the image from a rust-cache key, the sccache prefix or the coverage key each made the matching test fail.Testing
reclaim-gpu.sh --self-test(plain, and with a redirected TMPDIR) andpreflight_runs_the_real_reclaim_assert_clearpass inubuntu:24.04(GNU coreutils), inubuntu:26.04(Ubuntu's uutils), and inubuntu:24.04with the uutils 0.12.0 release binary bind-mounted over/usr/bin/sleep. No decoy process is left running afterwards.cargo test -p xtask(321 passed),cargo fmt --check, both clippy invocations from CI with-D warnings, andcargo check -p xtask --tests --target x86_64-pc-windows-gnu.prekon the changed files passes, except for the hawkeye hook, whose local binary cannot parselicenserc.toml(a local version mismatch that also fails on main). TheLicense header check (hawkeye)CI job covers it.actionlint1.7.12 adds no new findings apart fromlabel "ubuntu-26.04" is unknown. That release predates the label, which is listed in actions/runner-images. It already reported the self-hosted labels before this change.This change affects CI and test fixtures only. CLI behaviour does not change, so no Gherkin scenario applies.