Skip to content

fix: use atomic counter for tempdir uniqueness in bench_tail tests - #22

Closed
rominf wants to merge 1 commit into
mainfrom
fix-bench-tail-flaky-test
Closed

fix: use atomic counter for tempdir uniqueness in bench_tail tests#22
rominf wants to merge 1 commit into
mainfrom
fix-bench-tail-flaky-test

Conversation

@rominf

@rominf rominf commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Replaces the SystemTime::now().as_nanos() suffix in tempdir() with an AtomicU64 counter
  • Windows SystemTime has ~15ms resolution; parallel tests running within the same timer tick could collide on the same directory path, causing tests to read/write each other's results.csv and produce wrong deserialization results
  • Observed failure: missing_30col_fields_default_safely got rows[0].run == 1 (from another test's ROW2) instead of 7

Root cause

bench_tail::tests::tempdir() built its path as rocm-dash-test-{pid}-{nanos}. Under full parallel test load on Windows CI, the three tests in the module ran within the same 15ms clock tick → same suffix → same directory → same results.csv → file contents from one test leaked into another.

An atomic counter increments on every call so each invocation gets a distinct suffix regardless of clock resolution.

Test plan

  • cargo test -p rocm-dash-collectors passes (54/54)
  • Windows CI green

Fixes flaky failure observed in https://github.com/ROCm/rocm-cli/actions/runs/27690632058/job/81900409577

Windows SystemTime has ~15ms resolution, so nanosecond timestamps can
collide when parallel tests run within the same timer tick. An atomic
counter guarantees a unique directory name per call regardless of clock
resolution.
@rominf
rominf force-pushed the fix-bench-tail-flaky-test branch from edfaff3 to d9244f0 Compare June 17, 2026 13:56
@rominf
rominf requested a review from Copilot June 17, 2026 13:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a Windows-specific test flake in rocm-dash-collectors by ensuring each test invocation uses a uniquely named temporary directory even when multiple tests run within the same coarse SystemTime tick.

Changes:

  • Replaced the SystemTime::now().as_nanos()-based temp directory suffix with a process-local AtomicU64 counter.
  • Ensured parallel unit tests in bench_tail cannot collide on the same temp directory / results.csv path on Windows.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@rominf

rominf commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as already-fixed. When rebasing onto current main, the only remaining diff is the tempdir prefix string — the atomic-counter fix that is the substance of this PR already landed on main via #15 (Enable strictest practical linting across the workspace), which uses the more descriptive prefix rocm-dash-bench-tail-{pid}-{n}. No code change is needed.

@rominf rominf closed this Jun 22, 2026
fredespi added a commit that referenced this pull request Jul 12, 2026
…I300X first)

MI300X ran `install sdk` 9× (once per isolated scenario), each a multi-GB TheRock
cold install, pushing the job past its time cap (~37min, cancelled → no report).
Raising the cap failed twice; the fix is to install the runtime ONCE and share it.

- harness: `use_shared_runtimes()` opts a scenario into a shared runtimes tree by
  symlinking its `data/runtimes` at `E2E_SHARED_RUNTIMES_DIR` (only `runtimes/`;
  config/services/engines stay isolated). No-op when the env var is unset (local
  + mock runs stay fully isolated — verified mock still 7 pass / 2 xfail). Called
  from the "a managed runtime is active" precondition only; the clean-slate
  scenarios ("no CLI-managed runtimes", "Installing the SDK") deliberately do NOT
  opt in. Active-runtime resolution falls back to most-recently-installed, so a
  shared registry is active without touching isolated config.
- ci (e2e-gpu / MI300X only for now): pre-warm ONE runtime SERIALLY in the run
  step before the suite (avoids two concurrent scenarios racing installs into the
  shared dir), then point E2E_SHARED_RUNTIMES_DIR at it. Validating on MI300X
  before propagating to the two Strix jobs.

Relates to task #22; unblocks #23 (same cold-install root cause).

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
fredespi added a commit that referenced this pull request Jul 12, 2026
The first cut of the shared-runtime pre-warm (1817c5b) failed validation on
MI300X (run 29205441532): the pre-warm used `cargo run --release` (a redundant
release rebuild), the cold first-run still paid the full multi-GB TheRock
download, the job exceeded the 35-min cap, and a scenario still ran its own
`install sdk` (sharing didn't take effect) — and it wedged the runner. Revert the
CI pre-warm so the branch is back to the safe pre-sharing baseline.

The harness `use_shared_runtimes()` stays in place but dormant: it's a no-op
unless E2E_SHARED_RUNTIMES_DIR is set, so no platform's behaviour changes. The
sharing design is being reworked (WIP task #22) — key fixes: use the prebuilt
debug binary for pre-warm, verify runtimes-list works through a symlinked
registry, move pre-warm off the E2E clock, and persist the shared tree across
runs.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
fredespi added a commit that referenced this pull request Jul 12, 2026
Priority is a complete consolidated report with all four platform columns; time
optimization (shared runtime, task #22) is deferred. The MI300X collapsed run
takes ~37min and was being cancelled at the 35min cap before writing
platform.json. Set all three GPU jobs to 90min so they complete and produce
their reports even on a cold runner.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants