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 in the filtered-pack admission test - #595
build_filtered_pack_holds_admission_through_rev_list_reap_on_disconnect now waits out a transient ETXTBSY on its freshly written fake git before it starts build_filtered_pack.
Motivation & context
The test writes a fake git script and starts the build at once. 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". The future then completes early with that error. The test's polling loop polls the finished future again and panics with "async fn resumed after completion", which hides the cause. I saw that panic once in a loaded local run.
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: in git/smart_http.rs, a test helper wait_until_fake_execs execs the fake with an argument that hits its no-op branch until ETXTBSY stops. The test calls it once after writing the fake.
How a reviewer can verify
cargo test --locked -p gitlawb-node --lib git::smart_http::tests::build_filtered_pack_holds_admission_through_rev_list_reap_on_disconnect
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 4 of 80 runs, each with Err(Text file busy (os error 26)) at the early completion. 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
I did not run the full workspace tests on this branch. I ran the git::smart_http::tests module (all passing), cargo fmt --all -- --check, and clippy with -D warnings on gitlawb-node only, so the clippy box is unticked. The new-tests box is unticked because this changes a test and adds no behavior.
The failure was reproduced with synthetic fork pressure, not captured from CI, so the link to the CI-style panic rests on the error text and the matching early completion.
The drop test elsewhere in this module has the same re-poll shape. It already retries around the same race, so I left it alone.
Summary by CodeRabbit
Tests
Improved reliability of the Git HTTP disconnect test. It now waits for the simulated Git process to become available before continuing, retrying briefly when the process is temporarily busy. This helps the test proceed consistently across runs without changing any application behavior or user-facing functionality.
build_filtered_pack_holds_admission_through_rev_list_reap_on_disconnect writes a
fake git and starts build_filtered_pack at once. A concurrent test thread that
forks while the script's write descriptor is open hands the child a copy until it
execs, so the first exec fails with Text file busy and the future completes
early with that error. The test's polling loop then polls the finished future
again and panics with 'async fn resumed after completion', which hides the
cause.
Exec the fake with an argument that hits its no-op branch until it stops
returning ETXTBSY. With 8 threads forking in the background the unpatched test
failed 4 of 80 runs, each with Text file busy (os error 26) at the early
completion; patched, it passed 300 of 300.
No actionable comments were generated in the recent review.
Walkthrough
The rev-list disconnect test now runs the fake Git script with settle before starting the filtered-pack operation. A Unix-only helper retries execution when it returns ETXTBSY.
Changes: Fake Git test execution
Layer / File(s)
Summary
Retry helper and test setup crates/gitlawb-node/src/git/smart_http.rs
Adds a helper that retries fake Git execution up to 100 times at 20 ms intervals for ETXTBSY. The rev-list disconnect test calls the script with settle before creating the admission guard and starting the filtered-pack operation.
This change makes a flaky test more reliable by waiting out a transient executable-busy error before running the fake git script. It does not touch production behavior, and there is no merge-blocking risk.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the test scope (test(node)) and the specific fix (waiting out ETXTBSY in the filtered-pack admission test). The raw summary confirms the change adds a helper that retries exec until …
Description check
The description follows the template's main sections: Summary, Motivation & context, Kind of change, What changed, How a reviewer can verify, Before you request review, Protocol & signing impact, and …
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 scope (test(node)) and the specific fix (waiting out ETXTBSY in the filtered-pack admission test). The raw summary confirms the change adds a helper that retries exec until ETXTBSY clears, and the test calls it before starting the filtered-pack scenario. The title matches the main change and uses a Conventional Commits prefix.
Full details: Description check
Explanation
The description follows the template's main sections: Summary, Motivation & context, Kind of change, What changed, How a reviewer can verify, Before you request review, Protocol & signing impact, and Notes for reviewers. The Summary and Motivation sections clearly explain the ETXTBSY race and the fix. The Kind of change box is ticked correctly for Tests / CI. The Motivation section leaves the 'Closes #' issue link blank, and the reviewer-verification section lists a command. The description is mostly complete. Some checklist items are unticked, and the author explains why in the notes (workspace tests not run, clippy not run on the full workspace). The 'Closes #' field is empty, and the description does not link a prior issue. Those are non-critical gaps.
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: 86c3e5fa-d3f4-4eb7-91f4-c57ae3e2bfe0
Commits
Reviewing files that changed from the base of the PR and between d4895ef and 4d95b81.
Files selected for processing: 1
crates/gitlawb-node/src/git/smart_http.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 branch has not been deployed
No deployments
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
build_filtered_pack_holds_admission_through_rev_list_reap_on_disconnectnow waits out a transient ETXTBSY on its freshly written fakegitbefore it startsbuild_filtered_pack.Motivation & context
The test writes a fake
gitscript and starts the build at once. 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". The future then completes early with that error. The test's polling loop polls the finished future again and panics with "async fnresumed after completion", which hides the cause. I saw that panic once in a loaded local run.Kind of change
What changed
gitlawb-node: ingit/smart_http.rs, a test helperwait_until_fake_execsexecs the fake with an argument that hits its no-op branch until ETXTBSY stops. The test calls it once after writing the fake.How a reviewer can verify
cargo test --locked -p gitlawb-node --lib git::smart_http::tests::build_filtered_pack_holds_admission_through_rev_list_reap_on_disconnectThis 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 4 of 80 runs, each with
Err(Text file busy (os error 26))at the early completion. 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
I did not run the full workspace tests on this branch. I ran the
git::smart_http::testsmodule (all passing),cargo fmt --all -- --check, and clippy with-D warningsongitlawb-nodeonly, so the clippy box is unticked. The new-tests box is unticked because this changes a test and adds no behavior.The failure was reproduced with synthetic fork pressure, not captured from CI, so the link to the CI-style panic rests on the error text and the matching early completion.
The drop test elsewhere in this module has the same re-poll shape. It already retries around the same race, so I left it alone.
Summary by CodeRabbit