Wire --body-limit into createServer; real CONTENT_LENGTH for chunked git pushes - #562
melvincarvalho merged 2 commits into
Conversation
Sorry, something went wrong.
…nked git pushes Two halves of the same user-facing bug (#561) — 'git push of a pack over 10MB cannot succeed no matter what the operator configures': 1. bin/jss.js start parsed --body-limit / JSS_BODY_LIMIT into config.bodyLimit and then dropped it — the hand-written options object passed to createServer() omitted bodyLimit, so every CLI-started server enforced the 10MiB default. The #474 knob was dead wiring; only programmatic createServer({ bodyLimit }) worked. 2. With the limit actually raised, pushes > 1MiB still died: git sends packs over http.postBuffer (1MiB) with Transfer-Encoding: chunked and no Content-Length, and the git handler set the CGI's CONTENT_LENGTH from the absent header ('0'), so http-backend read zero bytes and receive-pack failed with 'the remote end hung up unexpectedly'. Use the buffered body's real length instead. Regression tests (test/body-limit-cli.test.js) spawn the real CLI and assert the limit reaches Fastify via flag and env — 2 of 3 fail without fix 1. Verified end-to-end: 'jss install solid-chat/app=chat JavaScriptSolidServer/git' (13.6MB / >1MiB packs, previously impossible) now completes 2/2 with extracted working trees serving 200. Full suite: 961 pass, 0 fail. Fixes #561
There was a problem hiding this comment.
Pull request overview
This PR fixes a user-facing git push failure mode for large pushes by (1) ensuring the CLI’s --body-limit / JSS_BODY_LIMIT configuration is actually forwarded into createServer(), and (2) ensuring the Git CGI environment uses the buffered request body length (not the often-missing Content-Length header for chunked pushes).
Changes:
- Wire config.bodyLimit into the CLI createServer() call so CLI-started servers respect --body-limit / JSS_BODY_LIMIT.
- Fix src/handlers/git.js to set CONTENT_LENGTH from request.body.length when available, enabling chunked pushes to be read correctly by git http-backend.
- Add an integration test that spawns bin/jss.js start and asserts Fastify enforces the configured body limit.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/body-limit-cli.test.js | Adds CLI-level integration coverage to ensure --body-limit / env wiring reaches Fastify. |
| src/handlers/git.js | Uses buffered body length for CONTENT_LENGTH so chunked pushes don’t present as 0 bytes to git http-backend. |
| bin/jss.js | Passes bodyLimit from loaded config into createServer() for the CLI start path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
| async function stopCli() { | ||
| if (!child) return; | ||
| const c = child; | ||
| child = null; | ||
| if (c.exitCode === null && c.signalCode === null) { | ||
| const gone = new Promise(r => c.once('exit', r)); | ||
| c.kill('SIGTERM'); | ||
| await Promise.race([gone, new Promise(r => setTimeout(r, 3000))]); | ||
| if (c.exitCode === null && c.signalCode === null) c.kill('SIGKILL'); | ||
| await gone; | ||
| } | ||
| } |
There was a problem hiding this comment.
Addressed in b17c30a — with one correction on the mechanism: the specific race described (child exits between the exitCode check and attaching the 'exit' listener) can't occur, because both statements are in the same synchronous block and Node can't emit 'exit' between two synchronous statements — if the event already fired, exitCode is non-null and we return before attaching. Added a comment so the next reader doesn't re-derive this.
The spirit of the finding stands, though: the final await gone was unbounded, so a pathological unkillable child (stuck in uninterruptible I/O) would hang the suite forever. It's now capped with a 2s race — SIGKILL can't be caught, so 'exit' follows promptly in every realistic case and the bound is belt-and-braces. 3/3 passing.
Sorry, something went wrong.
…ch can't race Copilot flagged a race between the exitCode check and attaching the 'exit' listener. That specific interleaving can't occur — both are in the same synchronous block, and Node can't emit 'exit' between two synchronous statements (if the event already fired, exitCode is non-null and we return before attaching). Documented in a comment. The comment's spirit stands though: the final `await gone` was unbounded, so a pathological unkillable child (stuck in uninterruptible I/O) would hang the suite forever. Capped with a 2s race — SIGKILL can't be caught, so 'exit' follows promptly in every realistic case; the bound is belt-and-braces. 3/3 passing.
Two PRs merged since 0.0.207 — completing the --body-limit / large git-push story end to end: - #562 (closes #561): wire --body-limit into createServer + real CONTENT_LENGTH for chunked git pushes. The CLI flag / JSS_BODY_LIMIT were dead wiring — bin/jss.js parsed them but never passed bodyLimit to createServer(), so jss start always used the default no matter what the operator set. Separately, git sends packs over http.postBuffer (1MiB) with Transfer-Encoding: chunked and no Content-Length, so the git CGI was told '0 bytes' — a large push could not succeed regardless of the configured limit. Both fixed. - #563 (closes #563): raise the default request body limit 10MB → 20MB — a more generous out-of-the-box cap for common workloads, still configurable via --body-limit / JSS_BODY_LIMIT. Net: large 'git push' to a pod now works out of the box, and the body-limit knob (#474) actually takes effect from the CLI.
Fixes #561. Related: #559 (default bundle vs limit), #560 (auto-init leftover), #474 (the knob this revives).
Two halves of one user-facing bug — a git push with a pack over 10MB could not succeed no matter what the operator configured:
1. --body-limit / JSS_BODY_LIMIT were dead wiring on the CLI path
bin/jss.js start parses them into config.bodyLimit, but the hand-written options object passed to createServer() omitted bodyLimit — so src/server.js always fell back to the 10MiB default. Only programmatic createServer({ bodyLimit }) ever worked (which is why test/body-limit.test.js passed all along). One line: pass bodyLimit: config.bodyLimit.
2. Chunked git pushes told the CGI "0 bytes"
git sends any pack larger than http.postBuffer (1MiB default) with Transfer-Encoding: chunked and no Content-Length header. src/handlers/git.js set the CGI env from that header (CONTENT_LENGTH: ... || '0'), so git http-backend read zero bytes from a fully-buffered 13.6MB body and receive-pack died with "the remote end hung up unexpectedly". Fixed by using the buffered body's real length (request.body.length).
Without fix 2, fix 1 just moves the failure from 413 to a hang-up — both are needed before raising the limit helps anyone.
Tests
test/body-limit-cli.test.js spawns the real CLI (same pattern as cli-flag-like-values.test.js) and asserts the limit reaches Fastify:
Verified the tests catch the bug: 2 of 3 fail with fix 1 reverted.
End-to-end
jss start --git --body-limit 200MB + jss install solid-chat/app=chat JavaScriptSolidServer/git — the two apps that have never been installable over HTTP — now completes 2/2 with extracted working trees serving 200 (previously: 413s, then with fix 1 alone: hang-up + an empty auto-init repo, the #560 artifact).
Full suite: 961 pass, 0 fail (was 958 before the 3 new tests).
GIT_TRACE_CURL evidence and the full diagnosis trail are in #561.