Repository navigation
bench: recover the container-written clone before each cold reset - #204
Merged
Merged
Conversation
…eset A launch leaves its clone owned by the container's user (uid 1000 in the standard devcontainer base image), which is not a GitHub runner's own uid. `dl rm` deleted the workspace, warned that the clone would not go and exited 0, and the next cold run died writing .git/index.lock into a clone it did not own -- so the cold-recreate shape reported a failed run instead of a time (run 31838698495, the first real run of the workflow). The mismatch is the environment's, not dl's, so the recovery is a sudo chown -R of this job's own clone cache composed into the shape's --before command, where it runs before EVERY run: runs 2..5 recreate the same files run 1 did.
Reviewer's GuideAdjusts the bench workflow cold-recreate reset command to chown container-owned git clones back to the runner user before forced removal, and adds tests ensuring the reset command’s behavior, scope, and per-run cardinality. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
--beforecommand currently lets a failingsudo chownshort‑circuit the subsequentdl $TARGET rm --forceand return a non‑zero status, which conflicts with the intent that the guard leave the exit status to the remove; consider restructuring the shell (e.g.,if test -d ...; then sudo chown ... || true; fi; dl ...) so thatdl rmalways runs and controls the overall exit code where appropriate. - The
cold_reset()helper picks the--beforeline using a substring search for"--before", which could be tripped by future comments or unrelated lines; tightening this to match only actual setting lines (e.g., via a prefix or regex) would make the tests more robust against incidental text changes inbench_settings().
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `--before` command currently lets a failing `sudo chown` short‑circuit the subsequent `dl $TARGET rm --force` and return a non‑zero status, which conflicts with the intent that the guard leave the exit status to the remove; consider restructuring the shell (e.g., `if test -d ...; then sudo chown ... || true; fi; dl ...`) so that `dl rm` always runs and controls the overall exit code where appropriate.
- The `cold_reset()` helper picks the `--before` line using a substring search for `"--before"`, which could be tripped by future comments or unrelated lines; tightening this to match only actual setting lines (e.g., via a prefix or regex) would make the tests more robust against incidental text changes in `bench_settings()`.
## Individual Comments
### Comment 1
<location path="test/test_bench_workflow.py" line_range="144-150" />
<code_context>
+ things this job did not create."""
+ assert "XDG_CACHE_HOME" in cold_reset()
+
+ def test_the_recovery_runs_per_run_rather_than_once_before_the_bench(self):
+ """Every run recreates those files, so a one-time step before the bench
+ recovers run 1 and leaves runs 2..5 with exactly the clone that broke
+ it. `--before` is the only place with the right cardinality."""
+ assert bench_settings().count("chown") == 1, (
+ "a second chown is a one-time step doing per-run work"
+ )
</code_context>
<issue_to_address>
**suggestion (testing):** Use a more targeted way to count `chown` occurrences to avoid false positives/negatives.
Counting plain "chown" in `bench_settings()` can match comments or unrelated text, and may miss future variations of the command. Consider instead counting only in the `--before` section (e.g. `cold_reset().count("chown") == 1`), or matching the exact `chown` command you expect, so the test stays focused on the intended call and is less brittle to wording changes.
```suggestion
def test_the_recovery_runs_per_run_rather_than_once_before_the_bench(self):
"""Every run recreates those files, so a one-time step before the bench
recovers run 1 and leaves runs 2..5 with exactly the clone that broke
it. `--before` is the only place with the right cardinality."""
reset = cold_reset()
assert reset.count("sudo chown") == 1, (
"a second chown in the cold-reset command would incorrectly do per-run work once"
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+144
to
+150
| def test_the_recovery_runs_per_run_rather_than_once_before_the_bench(self): | ||
| """Every run recreates those files, so a one-time step before the bench | ||
| recovers run 1 and leaves runs 2..5 with exactly the clone that broke | ||
| it. `--before` is the only place with the right cardinality.""" | ||
| assert bench_settings().count("chown") == 1, ( | ||
| "a second chown is a one-time step doing per-run work" | ||
| ) |
There was a problem hiding this comment.
suggestion (testing): Use a more targeted way to count chown occurrences to avoid false positives/negatives.
Counting plain "chown" in bench_settings() can match comments or unrelated text, and may miss future variations of the command. Consider instead counting only in the --before section (e.g. cold_reset().count("chown") == 1), or matching the exact chown command you expect, so the test stays focused on the intended call and is less brittle to wording changes.
Suggested change
| def test_the_recovery_runs_per_run_rather_than_once_before_the_bench(self): | |
| """Every run recreates those files, so a one-time step before the bench | |
| recovers run 1 and leaves runs 2..5 with exactly the clone that broke | |
| it. `--before` is the only place with the right cardinality.""" | |
| assert bench_settings().count("chown") == 1, ( | |
| "a second chown is a one-time step doing per-run work" | |
| ) | |
| def test_the_recovery_runs_per_run_rather_than_once_before_the_bench(self): | |
| """Every run recreates those files, so a one-time step before the bench | |
| recovers run 1 and leaves runs 2..5 with exactly the clone that broke | |
| it. `--before` is the only place with the right cardinality.""" | |
| reset = cold_reset() | |
| assert reset.count("sudo chown") == 1, ( | |
| "a second chown in the cold-reset command would incorrectly do per-run work once" | |
| ) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #204 +/- ##
=======================================
Coverage 92.48% 92.48%
=======================================
Files 24 24
Lines 3380 3380
=======================================
Hits 3126 3126
Misses 254 254 🚀 New features to boost your workflow:
|
blooop
commented
Aug 14, 2026
blooop
left a comment
blooop
left a comment
Owner
Author
There was a problem hiding this comment.
This was generated by AI during review.
Inline review (small follow-up fix; reviewer did not author the diff): Approve.
Mechanics verified from the diff: the step shell expands $XDG_CACHE_HOME/$(id -u):$(id -g)/$TARGET once at step time, leaving shlex.split an exact ['bash','-c','test -d … && sudo chown -R …; dl … rm --force'] argv — per-run recovery inside --before, which is the only placement with the right cardinality (runs 2..5 recreate the same container-owned files). ; before the remove keeps the command's exit the remove's; test -d covers the nothing-there-yet first reset. Scope deliberately stops at the job's own repos tree. Root-cause correction accepted and better than the dispatch hypothesis: uid 1001 runner vs uid 1000 container user — the exact class README's 'when part of the cache will not go' documents, which is also why PR #203's uid-1000 local rehearsal couldn't reproduce it. Guards extend the existing text-assertion style and are mutation-checked (chown-after-rm and hoisted-one-time-step both fail). All pre-existing charter guards untouched. Validation is the merge push: this merge triggers the next bench run on a real runner.
Verdict: Approve.
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.
The first real run of the launch-benchmark workflow never reached a trend point:
the cold-recreate shape's per-run reset could not remove the clone, and run 1's
launch then died on a
.gitit could not write.Run 31838698495
(the run of the workflow #203 added for issue #198 — follow-up, not a reopen):
Why it is CI-only, and why the fix is not in
dlA launch leaves its clone owned by the container's user — uid 1000 (
vscode) inthe standard devcontainer base image. On a developer's own uid-1000 host nothing
comes up, which is why #203's local rehearsal of the same two shapes was green;
a GitHub runner's user is not that uid. README.md already names this case ("when
part of the cache will not go"):
dlcannot unlink what it does not own, so itdeletes the workspace, warns about the clone, and exits 0. The next launch in the
shape then inherits a permission-broken clone and fails in
git checkout— and--beforefailing to establish the cold state is exactly the thing that makesthe shape report a failed run instead of a time.
So nothing about
dlis wrong here, and nothing indlchanges. The mismatchbelongs to the environment that creates it, and GitHub-hosted runners have
passwordless sudo.
The fix
The cold shape's
--beforebecomessudo chown -Rof this job's own clone cacheand then the remove, as one command:
--before, not a step of its own.--beforeruns before EVERY run;runs 2..5 recreate the same container-owned files run 1 did, so a one-time step
would recover run 1 and hand runs 2..5 the clone that broke it.
$XDG_CACHE_HOME/devlaunch/repos— the cache the job alreadyscoped into
/tmp— rather than anything wider.test -dbecause a reset alsoruns before the first launch, when there may be nothing there.
Guards
test/test_bench_workflow.pygains one class, in the same text-assertion styleas the rest of the charter (three tests): the reset chowns before it removes, it
reclaims only this job's clone cache, and
chownappears exactly once in theworkflow's settings — a second one would be a one-time step doing per-run work.
The alert-don't-gate guards and #195's job-fails-trend-untouched discipline are
untouched; the workflow is still not in
ci.yml'sneedsand stillfail-on-alert: false.pixi run ciis green locally (12 passed intest_bench_workflow.py, full suiteand lint clean). The
--beforeplumbing — that the composed command splits intobash -c …, that the chown and the remove both run on every run, and that thetest -dguard leaves the exit status the remove's — was rehearsed against thereal
scripts/bench_launch.pywith a stubdlandsudo.Validation is the merge push itself. The defect only reproduces on a runner,
and the workflow triggers on push to
main, so merging this PR runs the nextbench — which either publishes the first cold-recreate point or shows the same
step failing with the chown in place.
Summary by Sourcery
Tests: