You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
test(node): wait out ETXTBSY on the fake git in the re-probe test - #593
absent_probe_still_reprobes_when_the_budget_allows now waits out a transient ETXTBSY on its freshly written fake git before running the probe it tests.
Motivation & context
The test writes a fake git script and execs it immediately. If another test thread forks while the script's write descriptor is open, the child holds a copy until it execs, and the first exec fails with "Text file busy". batch_check_probe maps that to Transient("failed to spawn git cat-file"), so the fake never runs and the test sees zero spawns instead of two (left: 0, right: 2). That is the failure in CI job 132007293693. smart_http.rs already retries around the same race for its fake-git tests; this one had no guard.
Kind of change
Bug fix
Feature
Security fix
Docs
Tests / CI
Refactor (no behavior change)
Breaking or protocol change (issue required first)
What changed
gitlawb-node: add a test helper wait_until_fake_execs in git/store.rs that execs the fake until it stops returning ETXTBSY. The test calls it after chmod and deletes the log that run wrote before probing.
How a reviewer can verify
cargo test --locked -p gitlawb-node --lib git::store::tests
This passes with or without the change on an idle machine. The race needs other threads forking while the fake is written. With 8 threads forking in the background (temporary code, not in this PR), the unpatched test failed 3 of 100 runs, every one with Text file busy (os error 26) and the same left: 0, right: 2. Patched, it passed 300 of 300 under the same pressure.
Before you request review
Scope is one logical change; no unrelated churn
cargo test --workspace passes locally
New behavior is covered by tests (required for fixes)
cargo fmt --all and cargo clippy --workspace --all-targets -- -D warnings are clean
Commit titles use Conventional Commits (feat(...), fix(...), docs(...))
Docs / .env.example updated if behavior or config changed (or N/A)
Checked existing PRs so this isn't a duplicate
Protocol & signing impact
Touches DID / did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formats
Discussed in an issue before implementation
Backward-compatible with existing nodes and previously signed history
None of these apply: the change is inside a test module and touches no protocol code.
Notes for reviewers
The full workspace tests were not run on this branch; I ran git::store::tests (30 passed) and clippy with -D warnings on gitlawb-node only. The checkbox for new tests is unticked because this change fixes a test and adds no behavior; the fork-pressure runs above are the evidence.
The failure was reproduced with synthetic fork pressure, not captured from a CI runner, so the link to the CI failure rests on the matching assertion values and the Text file busy error.
Other fixtures in store.rs write and exec a fake the same way and can hit the same race. I left them alone. Separately, absent_probe_skips_a_reprobe_it_cannot_afford failed once in my load runs for a different reason: it burns 0.9s of a 1s budget and overshot to 2.05s.
Summary by CodeRabbit
Tests
Tests for Git probe behavior now tolerate transient script-execution errors and verify probe counts without including helper output. This makes those test results more consistent when temporary execution issues occur.
absent_probe_still_reprobes_when_the_budget_allows writes a fake git script and
execs it straight away. A concurrent test thread that forks while the script's
write descriptor is open hands the child a copy until it execs, so the exec
fails with Text file busy. The probe maps that to a Transient spawn error and the
test sees zero spawns instead of two.
Exec the fake until it stops returning ETXTBSY, then clear the log that run
wrote. With 8 threads forking in the background the unpatched test failed 3 of
100 runs, all with Text file busy (os error 26); patched, 0 of 300.
No actionable comments were generated in the recent review.
Walkthrough
The test module adds a Unix helper that retries fake Git execution after transient ETXTBSY errors. The ample-budget re-probe test uses the helper and clears its log before counting probe spawns.
Changes: Fake Git Test Readiness
Layer / File(s)
Summary
Retry handling and re-probe test crates/gitlawb-node/src/git/store.rs
A test helper retries fake Git execution up to 100 times when it receives ETXTBSY. The ample-budget re-probe test waits for execution, then clears the helper's log before counting probe spawns.
This change only makes one test more reliable by retrying a transient executable-busy error. It has no production impact and is low risk to merge.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the test module and the specific fix: waiting out ETXTBSY on the fake git script in the re-probe test. This matches the single change in store.rs, which adds a retry helper and calls i…
Description check
The description follows the repository template closely. It has a clear Summary, Motivation & context with the root cause, Kind of change with Tests / CI checked, a What changed section, and a reviewe…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title names the test module and the specific fix: waiting out ETXTBSY on the fake git script in the re-probe test. This matches the single change in store.rs, which adds a retry helper and calls it in the re-probe test. The title is short, specific, and uses the Conventional Commits form the repository requires.
Full details: Description check
Explanation
The description follows the repository template closely. It has a clear Summary, Motivation & context with the root cause, Kind of change with Tests / CI checked, a What changed section, and a reviewer verification command. It also fills the Before you request review and Protocol & signing sections, and it explains the unticked items. Two gaps remain. The 'Closes #' issue link is blank, and the 'How a reviewer can verify' section does not say how to reproduce the race without the temporary fork-pressure code. Neither gap is critical for a test-only fix.
Finishing Touches
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: defaults
Review profile: CHILL
Plan: Advanced
Run ID: e07040aa-04da-4fa7-a9b4-51b1159f12b5
Commits
Reviewing files that changed from the base of the PR and between d4895ef and e5e6d7a.
Files selected for processing: 1
crates/gitlawb-node/src/git/store.rs
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
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
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.
Summary
absent_probe_still_reprobes_when_the_budget_allowsnow waits out a transient ETXTBSY on its freshly written fakegitbefore running the probe it tests.Motivation & context
The test writes a fake
gitscript and execs it immediately. If another test thread forks while the script's write descriptor is open, the child holds a copy until it execs, and the first exec fails with "Text file busy".batch_check_probemaps that toTransient("failed to spawn git cat-file"), so the fake never runs and the test sees zero spawns instead of two (left: 0, right: 2). That is the failure in CI job 132007293693.smart_http.rsalready retries around the same race for its fake-git tests; this one had no guard.Kind of change
What changed
gitlawb-node: add a test helperwait_until_fake_execsingit/store.rsthat execs the fake until it stops returning ETXTBSY. The test calls it afterchmodand deletes the log that run wrote before probing.How a reviewer can verify
cargo test --locked -p gitlawb-node --lib git::store::testsThis passes with or without the change on an idle machine. The race needs other threads forking while the fake is written. With 8 threads forking in the background (temporary code, not in this PR), the unpatched test failed 3 of 100 runs, every one with
Text file busy (os error 26)and the sameleft: 0, right: 2. Patched, it passed 300 of 300 under the same pressure.Before you request review
cargo test --workspacepasses locallycargo fmt --allandcargo clippy --workspace --all-targets -- -D warningsare cleanfeat(...),fix(...),docs(...)).env.exampleupdated if behavior or config changed (or N/A)Protocol & signing impact
did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formatsNone of these apply: the change is inside a test module and touches no protocol code.
Notes for reviewers
The full workspace tests were not run on this branch; I ran
git::store::tests(30 passed) and clippy with-D warningsongitlawb-nodeonly. The checkbox for new tests is unticked because this change fixes a test and adds no behavior; the fork-pressure runs above are the evidence.The failure was reproduced with synthetic fork pressure, not captured from a CI runner, so the link to the CI failure rests on the matching assertion values and the Text file busy error.
Other fixtures in
store.rswrite and exec a fake the same way and can hit the same race. I left them alone. Separately,absent_probe_skips_a_reprobe_it_cannot_affordfailed once in my load runs for a different reason: it burns 0.9s of a 1s budget and overshot to 2.05s.Summary by CodeRabbit