Skip to content

docs(hostmon): the reader's own doc still denied #1967's fix - #1981

Merged
justinchuby merged 1 commit into
mainfrom
seb/hostmon-run-owner-doc
Aug 24, 2026
Merged

justinchuby merged 1 commit into
mainfrom
seb/hostmon-run-owner-doc

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

One doc comment. crates/onnx-runtime-hostmon/src/hostlock.rs, on field_from_env:

Note also that hostlock.sh run does not export --owner, so passing the flag alone leaves a child unable to recognise its own parent's lock; that reports Held, which is unattributed and unprotected, and is the honest answer.

#1967 (8d6377f00) made that false. It is the doc comment on the function that decides mine: versus held:, so the reader it misleads is precisely the one deciding whether a benchmark row can be trusted — and it steers them away from run --owner X, which is now the invocation that works.

Repo-wide grep finds no other copy: #1967 updated scripts/ort_ab/README.md, and this was the only sentence it missed.

The replacement states the asymmetry too, since a reader will otherwise re-derive it: a declared owner (--owner, or HOSTLOCK_OWNER already in the environment) is exported; an owner defaulted from $USER is not, because every agent here is the same unix user and a false mine: reads as protected. A run that declared nothing still reports Held.

How I found it, and what I checked

I had been fixing #1929 in parallel and did not notice #1967 until my branch stopped getting CI (see below). Reviewing that PR against mine, this was the one thing its diff did not cover. I also checked the two adjacent claims in mine that #1967 might have missed, and it had not missed them:

  • Its OWNER_DECLARED is initialised in the defaults block, so an inherited environment variable of that name cannot stand in for the flag. My first attempt read ${OWNER_DECLARED:-0} and initialised it nowhere, which let OWNER_DECLARED=1 hostlock.sh run -- child export the $USER default — the exact false mine: the change exists to prevent. fix(hostlock): let run --owner certify the rows it wraps (#1929) #1967 does not have that hole.
  • I checked that its guard is test-covered rather than merely present, by deleting the OWNER_DECLARED=0 line on main and running the full suite twice: 4 red in a clean environment (set -u catches the unguarded read), and 1 red with OWNER_DECLARED=1 inherited (an owner defaulted from the unix user is NOT exported). Covered in both directions, so there is nothing for me to add there.

Validation

  • RUSTDOCFLAGS="-D warnings" cargo doc -p onnx-runtime-hostmon --no-deps — clean, and the intra-doc link still resolves.
  • cargo test -p onnx-runtime-hostmon — green including doctests; cargo fmt --all --check clean.

Unrelated, but worth recording for whoever hits it next

My #1968 sat for forty minutes with no Actions runs at all — not queued, not failed: no GitHub Actions check suite was ever created for the head SHA, while other branches dispatched normally. Close/reopen did not help, and neither did a force-push to a fresh SHA. The cause was that the branch conflicted with main (because #1967 had landed in the same files): GitHub cannot compute the merge ref, so it creates no pull_request runs. A conflicted PR and a backlogged CI queue look identical from gh pr checks, which says "no checks reported" for both. The tell is gh api repos/{o}/{r}/commits/{sha}/check-suites — third-party app suites appear, the github-actions one does not.

`field_from_env`'s rustdoc told readers that `hostlock.sh run` does not export
`--owner` and that passing the flag alone leaves a child unable to recognise
its own lock. #1967 made that false three commits ago. It is the doc comment
attached to the function that decides `mine:` versus `held:`, so the reader it
misleads is exactly the one deciding whether a benchmark row can be trusted --
and it advises against the invocation that now works.

Repo-wide grep finds no other copy; #1967 updated `scripts/ort_ab/README.md`
and this was the only sentence it missed.

Replaced with what the script does, including the asymmetry, since that is the
part a reader will otherwise re-derive: a declared owner is exported, an owner
defaulted from `$USER` is not, and a run that declared nothing still reports
`Held` -- unattributed, unprotected, and honest.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@codecov

codecov Bot commented Aug 24, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.01%. Comparing base (b3a9f6c) to head (53a85d9).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1981      +/-   ##
==========================================
+ Coverage   80.25%   81.01%   +0.76%     
==========================================
  Files         407      424      +17     
  Lines      188524   207552   +19028     
  Branches   188524   207552   +19028     
==========================================
+ Hits       151293   168152   +16859     
- Misses      31822    33760    +1938     
- Partials     5409     5640     +231     
Flag Coverage Δ
cli-ort-windows 72.10% <ø> (?)
mlas 85.33% <ø> (?)
offline 81.18% <ø> (+0.93%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
crates/onnx-runtime-hostmon/src/hostlock.rs 92.39% <ø> (-0.09%) ⬇️

... and 65 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@justinchuby
justinchuby merged commit 5081ddb into main Aug 24, 2026
16 of 17 checks passed
@justinchuby
justinchuby deleted the seb/hostmon-run-owner-doc branch August 24, 2026 12:51
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.

1 participant