fix(node,git): bound a hung served git with a total-duration timeout (#62)#165
fix(node,git): bound a hung served git with a total-duration timeout (#62)#165beardthelion wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a configurable timeout for served git smart-HTTP operations, maps timeout failures to HTTP 504, enforces the deadline in ChangesGit service timeout enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 927-941: The push-path classifier in git_receive_pack is missing
the AppError::BadRequest arm that git_upload_pack already handles, so malformed
pushes are being logged at error level instead of warn. Update the match inside
repos.rs’s receive-pack error handling to mirror the fetch-path classification
in git_upload_pack: keep Timeout as warn, add a BadRequest warn branch, and
leave only unexpected failures in the error branch while still returning the
mapped AppError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8c06bcf-7ffd-4301-a85c-97b37da698e0
📒 Files selected for processing (4)
crates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/config.rscrates/gitlawb-node/src/error.rscrates/gitlawb-node/src/git/smart_http.rs
beardthelion
added
crate:node
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found one issue that needs to be addressed before this is ready.
Findings
- [P3] Complete CodeRabbit's request to mirror BadRequest logging on receive-pack
crates/gitlawb-node/src/api/repos.rs:935
CodeRabbit's inline request is still valid:git_service_app_errorcan classify malformed push input asAppError::BadRequest, but thegit_receive_packlogging match only handlesTimeoutseparately and sends every other mapped error through the generictracing::error!branch. That means client-caused malformed pushes return the right 400 but are still logged as server errors, unlike the upload-pack path. Please complete that review item by adding theAppError::BadRequestwarn branch here and leaving only unexpected failures in the error branch.
…#62) Part 1 of #62. A served git upload-pack/receive-pack that neither finishes nor disconnects parks forever, pinning a git PID (and, for receive-pack, holding the write lock); on a public repo an anonymous caller can trigger it. No tower TimeoutLayer exists and axum imposes no default. Wrap the child interaction in tokio::time::timeout, bounded by a new GITLAWB_GIT_SERVICE_TIMEOUT_SECS knob (default 600, must be positive). Drain stdout/stderr concurrently with the stdin write so a large body can't deadlock. On expiry run_git_service returns a typed GitServiceTimeout and reaps the WHOLE process group before returning (SIGTERM so git clears its .git/*.lock, wait for the leader and its grandchildren like index-pack to exit, escalate to SIGKILL past a grace, hard-capped), so a caller releasing the receive-pack write lock can't race a still-live git on the same repo. The handlers map GitServiceTimeout to a distinct 504 (not the generic 500 git error, and clear of the read-gate 404 / auth 401 the client keys retries on) via a pure, unit-testable classifier rather than matching the anyhow string. Surface git's own non-zero exit (its stderr) before a stdin-write EPIPE so a malformed body is classified 400, not 500. Out of scope, deferred in #62: the info/refs advertisement and the withheld-blob path (spawn_blocking, which tokio timeout cannot cancel); both remain unbounded (noted in the config knob's docs). Tests: the timeout fires and tears the group down; it reaps the whole group before returning (RED if the reap is leader-only); the wrap is load-bearing (RED via an outer bound, not a hang); the error must be the typed timeout; the 504 mapping and classifier are unit-tested; config rejects 0. Full suite green.
- test(node): cover the `protocol error` arm of git_service_app_error independently (the input has no "bad line length" substring, so it isolates the second classifier arm and goes RED if that arm is removed) - test(node): make the reap-before-return test's grandchild IGNORE SIGTERM (`trap "" TERM`) and outlive the ~4s reap cap, so the SIGKILL escalation is load-bearing. Previously the grandchild self-exited under the cap, so neutering the SIGKILL still passed; the escalation was exercised but not required. Verified: RED with SIGKILL disabled, GREEN with it. - test(node): add a receive-pack timeout test. Every prior timeout test used an upload-pack fake; this proves the push path (which also holds the repo write lock) is bounded too. RED-verified with the internal timeout removed. - test(node): poll for the pidfile in the reap-before-return timeout test so a loaded runner can't false-panic before the fake git writes it - remove the redundant `sigkilled` guard in reap_group_on_timeout; step == 200 fires exactly once in the 0..400 loop, so no re-entry guard is needed and the SIGKILL still fires exactly once
git_receive_pack's error classifier routed every non-Timeout error, including client-caused BadRequest (400), through tracing::error!. Mirror the git_upload_pack arm so a malformed push logs at warn level instead of as a server error.
beardthelion
force-pushed
the
fix/served-git-timeout
branch
from
a18d631 to
41eca0b
Compare
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found a couple of issues that need to be addressed before this is ready.
Findings
-
[P2] Make the smart_http tests pass under the normal parallel test runner
crates/gitlawb-node/src/git/smart_http.rs:1097
The new timeout tests pass when run by themselves, but the normal grouped module run still fails under the default parallel runner:cargo test -p gitlawb-node git::smart_http::tests -- --nocapturepanics with ``async fnresumed after completionfrom this polling loop. The loop ignores the `Ok(result)` case from `tokio::time::timeout(..., &mut fut)`, so if `run_git_service` returns before the pidfile is observed under parallel process-test load, the next iteration polls an already-completed future and aborts the test. Please make this test handle early completion explicitly, or otherwise isolate the process tests, so the changed timeout coverage is stable under the same parallel mode as `cargo test --workspace`. -
[P3] Add the new timeout knob to the env reference docs
.env.example:90
The PR addsGITLAWB_GIT_SERVICE_TIMEOUT_SECSinConfigwith a default of 600 seconds, but the operator-facing env reference still jumps fromGITLAWB_PUBLIC_READtoGITLAWB_MAX_PACK_BYTES, and the README says.env.exampleis the full configuration reference. Operators relying on that documented reference will not discover the new bound or the documented caveat thatinfo/refsandupload_pack_excludingremain unbounded. Please add the new variable to.env.exampleand, if it is considered an important node setting, the README configuration table as well.
… runner (#165) Addresses jatmn's review of 41eca0b. [P2] The #62 timeout tests failed under `cargo test --workspace`: - run_git_service_tears_down_group_when_future_dropped re-polled a completed future ("async fn resumed after completion"): the advance-until-pids loop ignored the timeout's Ok(_) (future-finished) case and polled the resolved future again. It now breaks on early completion. - Under fork-storm load a freshly-written fake `git` transiently fails to exec (ETXTBSY) or is timed out before recording its pids. Added a fake_git_run_with_pids retry helper (used by the four pid-reading tests) plus a matching inline retry on the drop test, with growing backoff for the correlated bursts and a 12-attempt cap that still fails loudly on a genuine never-spawns regression. Reverted two tests from a speculative 2000ms bound back to 300ms/1000ms now that the retry covers the timeout-vs-pidfile race. [P3] Documented GITLAWB_GIT_SERVICE_TIMEOUT_SECS in .env.example and the README config table (default 600; bounds upload-pack/receive-pack, not info/refs or the withheld-blob path). The tests stay load-bearing: breaking process_group(0), the post-reap disarm, the timeout-arm reap, or the internal timeout each still turns the corresponding test red (mutation-verified).
|
@jatmn both addressed, pushed as [P2] The re-poll: [P3] |
2 participants
Part 2 of #62 (part 1, the teardown-wiring test, merged in #150).
A served
git upload-pack/receive-packthat neither finishes nor disconnects parks forever, pinning a git PID and, for receive-pack, holding the repo write lock.upload-packis reachable by an anonymous caller on a public repo, and there is no tower timeout layer (axum imposes no default), so an unauthenticated request can wedge a worker indefinitely.This wraps the child interaction in
tokio::time::timeout, bounded by a newGITLAWB_GIT_SERVICE_TIMEOUT_SECS(default 600s, must be positive). The stdin write and the stdout/stderr drain run concurrently under one deadline so a large body cannot deadlock. On expiry the whole process group is torn down (SIGTERM, then SIGKILL past a ~2s grace, hard-capped) and reaped before returning, so a caller releasing the receive-pack write lock cannot race a still-live git. The handlers map the typed timeout to a distinct 504 via a pure classifier, clear of the 500 git error and the 404/401 a client keys retries on, rather than string-matching the anyhow chain.Deliberately out of scope, documented on the config knob: the
info/refsadvertisement and the withheld-blob path (upload_pack_excluding, aspawn_blockinga tokio timeout cannot cancel) stay unbounded. A follow-up to bound those will come separately.Tests: the timeout fires and tears the group down for both upload-pack and receive-pack; the whole group including grandchildren is reaped before returning, with the SIGKILL escalation load-bearing; the 504 mapping, every classifier arm, and the config rejecting 0 are covered. Full
gitlawb-nodesuite green.Summary by CodeRabbit