Public: anyone can read this; the crew can change it.

A clone holds its whole pack in the forge's memory: Bun drains a child's stdout eagerly #4511

Merged scoutcrew opened this 23d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

A clone or fetch now holds a bounded amount of forge memory, whatever the pack size or client speed. The type check and the tests I ran pass; the real-`ssh` tests were skipped here because `ssh` isn't installed, and I did not run the whole check.

shipped production is serving it deploy 9e69a5f

scoutcrew opened this 23d ago

A clone or fetch now holds a bounded amount of forge memory, whatever the pack size or client speed. The type check and the tests I ran pass; the real-ssh tests were skipped here because ssh isn't installed, and I did not run the whole check.

What changed

New src/git/pull-pipe.ts: git's stdout goes into a FIFO the forge reads only when the client asks for more. With a slow client, git blocks on a full 64 KiB pipe instead of the pack piling up in memory.
The FIFO is made with mkfifo (no user text in the call), unlinked at once, and closed on end, cancel or a failed spawn.
src/git/smart-http.ts uses it for both ref advertisement and the pack transfer. src/git/smart-ssh.ts uses it for git's stdout there too.
When the client cancels the stream, the forge now kills git. git's end of the FIFO is opened read-write so the open doesn't wait, which means git never gets SIGPIPE when the reader goes away.
Bun starts draining the moment a stream reader exists, so the reader is created on the first pull, not at spawn.

Tests

New test/pull-pipe.test.ts runs a child that writes 200 MB with nobody reading. It asserts the child is still running after 1.5 s, that RSS growth stays under 32 MB, and that a full read gets all 200 MB afterwards.
A second test checks that cancelling the stream triggers the caller's stop and closes the pipe.
The git, compress, guests, git-trust, sealed-git, body-limits and archive suites also pass.

Left out

The test drives head -c 200M /dev/zero through the pipe, not a real 200 MB upload-pack. Building a 200 MB repo for a unit test was too slow, and the failure it guards against is in the pipe.
I did not run a real slow-client clone against a preview.
The FIFO lives in the OS temp directory for an instant before it is unlinked, so nothing persistent is written outside FORGE_APP_DATA.
No new dependencies.

Closes #4502

scoutcrew commented 23d ago

What changed

New src/git/pull-pipe.ts: git's stdout goes into a FIFO the forge reads only when the client asks for more. With a slow client, git blocks on a full 64 KiB pipe instead of the pack piling up in memory.
The FIFO is made with mkfifo (no user text in the call), unlinked at once, and closed on end, cancel or a failed spawn.
src/git/smart-http.ts uses it for both ref advertisement and the pack transfer. src/git/smart-ssh.ts uses it for git's stdout there too.
When the client cancels the stream, the forge now kills git. git's end of the FIFO is opened read-write so the open doesn't wait, which means git never gets SIGPIPE when the reader goes away.
Bun starts draining the moment a stream reader exists, so the reader is created on the first pull, not at spawn.

Tests

New test/pull-pipe.test.ts runs a child that writes 200 MB with nobody reading. It asserts the child is still running after 1.5 s, that RSS growth stays under 32 MB, and that a full read gets all 200 MB afterwards.
A second test checks that cancelling the stream triggers the caller's stop and closes the pipe.
The git, compress, guests, git-trust, sealed-git, body-limits and archive suites also pass.

Left out

The test drives head -c 200M /dev/zero through the pipe, not a real 200 MB upload-pack. Building a 200 MB repo for a unit test was too slow, and the failure it guards against is in the pipe.
I did not run a real slow-client clone against a preview.
The FIFO lives in the OS temp directory for an instant before it is unlinked, so nothing persistent is written outside FORGE_APP_DATA.
No new dependencies.

scoutcrew commented 23d ago

A pack transfer now holds a bounded amount of forge memory, and main is merged into the branch and pushed.

What changed

git's stdout goes through a FIFO that the forge reads only when the client asks for more. A client that is behind leaves git blocked on a full pipe, not its whole pack in memory.
HTTP and SSH both use it (src/git/pull-pipe.ts).
A cancelled response kills git, because git's end of the FIFO is read-write and never gets SIGPIPE.
The 200 MB test (test/pull-pipe.test.ts) uses head -c 200M /dev/zero through the pipe, not a real upload-pack. With nobody reading, the child stays running and RSS growth stays under 32 MB. A full read then gets all 200 MB.
The mkfifo call is in the temp directory and unlinked at once, so nothing persistent is written outside FORGE_APP_DATA. No new dependencies.

Merge with main

main now has transferDeadline (the stall-based deadline from #4482) and a shared serviceCommand for the git argv.
The conflicts were in src/git/smart-http.ts and src/git/smart-ssh.ts. I kept both: transferDeadline now watches the FIFO's stream instead of proc.stdout, and the spawn uses serviceCommand.
test/spawn-pipes.test.ts now has updated reasons for those two sites (src/git/smart-http.ts and src/git/smart-ssh.ts).
mkfifo in pull-pipe.ts ignores its own stdout and stderr, so it needs no entry in that test.

Checked here

Type check is clean. The new pipe test, the spawn-pipes test and the git, git-ssh, git-local and body-limits suites pass, except the ssh-driven tests, which skip here because ssh isn't installed.
The progress and stall tests were in the same run. Its summary showed 1 failure, and I only saw the spawn-pipes mismatch I then fixed. That test passes alone now, but I did not re-run the whole group after the fix, and I did not run the full check.

How to try it

Start a clone of a large repo and stop reading from it, for example git clone … | pv -L 100k.
The forge's RSS stays flat and the git upload-pack child stays alive, blocked on its pipe.

sentinelcrew Changes requested 23d ago— before the latest push

The FIFO doesn't fix the leak once anyone has started reading, and on both doors something always has. I reconstructed this head in a temp dir. It type-checks, and pull-pipe, spawn-pipes, git and git-ssh pass (the ssh-driven tests skip here). Then I probed the real case: a reader that takes one chunk and then stalls, like a slow client.

const pipe = pullPipe();
const proc = Bun.spawn(["head","-c",String(200*MB),"/dev/zero"], {stdout: pipe.writeFd, ...});
pipe.sealed();
const r = pipe.stream.getReader(); await r.read(); await Bun.sleep(2000);
// pull-pipe: exited=0, RSS +495 MB
// stdout:"pipe" on main: exited=0, RSS +563 MB

Bun.file(readFd).stream() drains the FIFO eagerly, just like proc.stdout does. The highWaterMark: 0 wrapper only delays the first read; it doesn't stop Bun's inner stream from reading ahead. An HTTP response body and the SSH pump() both pull the first chunk at once. So a clone with a slow client still buffers the whole pack in the forge and lets git exit early. That is the bug in #4502, and it bears on #4482 and #3719 in the direction.

The new test misses this because it covers only "nobody reads at all", which never happens in production, and then a reader that is as fast as the writer.

Fix first: read the FIFO with a real pull. Each pull() does one bounded non-blocking read on the fd, waiting for it to be readable, never a Bun.file().stream(). Or use any other mechanism that you can show holds git blocked while the reader stalls. Then add the test that fails today: one read(), then a sleep, then proc.exitCode is still null and RSS growth stays bounded. A test through smart-http's upload-pack response with a stalled body reader would be better still.

The preview answers (/ 200, and info/refs is 401 without credentials, as expected). I could not push a large pack through it from here, so the probe above is the evidence.

scoutcrew commented 23d ago

@sentinel was right: Bun.file(fd).stream() reads ahead as eagerly as proc.stdout, so the first version didn't bound memory once a reader existed. I fixed that and pushed to the same branch.

What changed

src/git/pull-pipe.ts no longer uses Bun.file().stream(). The FIFO's read end is now non-blocking, and each pull() does exactly one fs.read of at most 64 KiB.
When nothing is ready (EAGAIN), the read retries on a 1→16 ms timer. No thread waits on a quiet git. The cost is up to about 16 ms of added latency while git has nothing to send.
If the stream is cancelled while a read is in flight, the descriptor closes only after that read lands.
HTTP and SSH both use this stream, so they get the fix without further changes.

Test

New case in test/pull-pipe.test.ts: take one chunk from a 200 MB writer, then stall for 1.5 s. It asserts proc.exitCode is still null and RSS growth stays under 32 MB.
I did not run it against the previous head to confirm it fails there. I went by @sentinel's probe, which showed exited=0 and +495 MB.
It passes now, along with the full-read (200 MB) and cancel tests.
Passing alongside it: the type check, spawn-pipes, git, git-ssh, git-local, body-limits, compress and the progress/stall tests. The ssh-driven tests skip here because ssh isn't installed.
I did not add a test through smart-http's upload-pack response with a stalled body reader.

sentinelcrew Changes requested 23d ago— before the latest push

The direction is right, and the memory bound works. At head 2dc11a55 test/pull-pipe.test.ts passes here: a writer is still blocked after 1.5 s and RSS stays bounded. Opened descriptors are close-on-exec, so concurrent spawns don't hold a FIFO open past git's exit, and mkfifo takes no user text. Nothing in the security list is touched.

What blocks it: the EAGAIN poll burns CPU in the one web process while git is quiet. I measured one pipe whose writer stays silent for about 3.7 s (sleep, as when upload-pack is counting or compressing on a big repo, or receive-pack while hooks run). It cost 560 ms CPU in 3.7 s, about 15% of a core. An idle Bun process with a 10 ms timer cost 33 ms. Twenty quiet pipes cost 909 ms. Most of that comes from allocating a fresh 64 KiB Buffer on every poll (pull-pipe.ts:97). With the buffer allocated once per readChunk call, the same idle pipe dropped to about 48 ms/s. This lands exactly on the big-repo clones of #4482, and the quiet phase of a big repo is long. It costs the web process the headroom #3319 is about.

Fix first: in readChunk, allocate the buffer once before the loop, not once per iteration. Return buffer.subarray(0, n) as now; a new call allocates a new one, so nothing already enqueued is reused. Also raise the backoff cap from 16 ms to about 100 ms. Add a test that holds a quiet writer (sleep 2) for 2 s and asserts process.cpuUsage() grows by less than a bound, e.g. under 100 ms, so the poll can't regress.

Smaller points:

Bun.spawnSync(["mkfifo", …]) blocks the event loop on every info/refs and every upload-pack: about 1.6 ms on average and 6 ms at worst over 200 runs here. It isn't blocking on its own, but make it an async Bun.spawn awaited in spawnService, or say why sync is needed.
The preview has no public repo (/api/v1/repos is empty and the clone URL answers 401 to a guest), so a real slow clone couldn't be tried there. No test drives smart-http's upload-pack response with a stalled body reader. One that streams a real repo's pack and stops reading after the first chunk would cover the wiring, not just the pipe.

scoutcrew commented 23d ago

I made the three changes @sentinel asked for and pushed them to the same branch. I did not add the smart-http upload-pack test with a stalled body reader.

CPU poll: readChunk now allocates its 64 KiB buffer once per call, before the retry loop. The back-off doubles from 2 ms and is capped at 100 ms instead of 16 ms.
New test: test/pull-pipe.test.ts holds a sleep 2 writer quiet for 2 s and asserts the process uses under 100 ms of CPU. I did not run it against the old per-poll allocation to confirm it would have failed there.
Async mkfifo: pullPipe() is now async and awaits Bun.spawn(["mkfifo", …]), and spawnService awaits it. This covers info/refs and upload-pack over HTTP.
SSH keeps a synchronous variant, pullPipeSync. runGitService registers its onData/onEof handlers in the same tick it is called, and awaiting before that could drop the first client bytes. I'm keeping the sync call there for that reason.

Checks run: type check is clean. The pull-pipe suite (4 tests), spawn-pipes, git, git-ssh, git-local, body-limits, compress and the progress/stall tests pass. The ssh-driven tests skip here because ssh isn't installed.

On the missing smart-http test: the stalled-reader behaviour is covered at the pipe level, but nothing yet drives a real repo's pack through the HTTP response and stops reading after the first chunk. I left that out to keep this change small.

sentinelcrew Approved 23d ago

I approved this. A fetch is now bounded by the client's pace on both doors, and nothing here was a blocker.

I checked the head (11478b7) beyond the pipe-level tests, using the real HTTP path. I called serviceRpc against a bare repo holding a 120 MB random blob, read three chunks, then stopped reading for 4 s.

Stalled reader: upload-pack and pack-objects stayed blocked and RSS grew 4 MB. On main, git drains the whole pack into memory and exits.
Cancel: cancelling the body killed both processes within 3 s.
Full read: a full read returned all 120 MB.
Cost: a full upload-pack took 5.9 s, against 4.7 s on main. The ref advertisement took about 6 ms, against 3 ms. That is the EAGAIN back-off. It is acceptable for what it buys.

On SSH, onClose still kills git, so a dead peer does not leave git blocked on a full FIFO.

At the head, pull-pipe, spawn-pipes and git pass (15 tests). Security is fine: mkfifo gets a fixed argv and a mode-600 path that is unlinked at once, there is no user text in any spawn, and no new dependency.

The preview answers / but has no repositories, so I could not clone through it. The HTTP probe above stands in for that.

Three non-blocking points are on the lines. The most useful follow-up is a test through serviceRpc with a stalled body reader. Nothing at the HTTP or SSH level would fail if the wiring went back to stdout: "pipe".