Repository navigation
Stream git packs through consumePack instead of buffering them - #656
Merged
Merged
Conversation
Mounting a large repo as a worktree exceeded the Overseer's 128 MB memory limit. consumePack collected the whole pack, inflated every object, and deflated each one again before storing anything, so a TypeScript- or vscode-size mount held 173-224 MiB of buffers at once. decodePackStream replaces decodePackBytes. It decodes the pack in one pass, keeping only each entry's oid by offset, and resolves every delta base through resolveBase. It reads the stream with a BYOB reader in 64 KiB chunks, enforces the pack size cap as bytes arrive, and hashes exactly the bytes before the trailer. A failed decode cancels the source. Every existing malformed-pack check is kept. MAX_DELTA_DEPTH is gone because nothing recurses any more. consumePackFromGatekeeper stores blobs of up to 1 MiB as they arrive. It holds commits, trees, tags and oversized blobs (a later delta may name one as its base) and stores them in one transaction once the trailer verifies, so a failed pack never leaves a commit that fetchCommit would treat as mounted. #storeVerifiedObject now handles the oversized case for both put() and consumePack. A delta must now follow its base. That holds for every pack git upload-pack sends a fetch with no haves, which is the only kind the gatekeepers request. Real depth-1 mount packs (react, next.js, TypeScript, vscode, and a 52 MiB internal repo) went through the real code on Durable Object SQLite in workerd. Every object was stored, or measured if oversized. A pack with a corrupted trailer left none of its commits or trees.
|
LGTM! |
Preview:
|
ndisidore
marked this pull request as ready for review
October 3, 2026 16:35
ndisidore
marked this pull request as draft
October 3, 2026 16:45
Mounting a vscode fork on a preview exceeded the Overseer's 30 s per-invocation CPU limit. In local workerd, deflating every stored object with pako in encodeLooseObject was the largest single cost of consumePackFromGatekeeper (about 4 of 10 s for vscode). encodeLooseObject and decodeLooseObject now use workerd's native node:zlib (deflateSync/inflateSync, available under nodejs_compat). The output is standard zlib at the same default level, so stored records stay readable by isomorphic-git and by records written before. decodeLooseObject views inflateSync's Buffer as a plain Uint8Array, so the payloads it hands out stay plain arrays. The pack decoder keeps pako, which it needs to find where each entry's zlib stream ends. consumePackFromGatekeeper in local workerd, three runs each, real Overseer DO storage, workerd process CPU: - vscode (23,289 objects): 10.3 s -> 7.4 s - TypeScript (65,040 objects): 18.6 s -> 13.3 s Every object was stored or measured, oids recompute, isomorphic-git reads a sample byte-for-byte, and stored bytes and SQLite size are unchanged. Whether a vscode mount now fits the production limit is not yet verified.
|
LGTM! |
A vscode mount still reset the Overseer on the preview, now on CPU: the invocation running consumePack used 32.5 s against the 30 s default. Run on Cloudflare against the real vscode mount pack, in a throwaway Worker's SQLite Durable Object, consumePackFromGatekeeper used 23.7-98 s of CPU, and 3 of 7 runs were reset on a storage timeout. Most of the excess was the one transaction around the held commits and trees: each metadata put inside it opens a nested transaction, and ~29k of those in one transaction cost 14-18 s there. Locally the whole transaction takes under 1 s. Held objects are now stored one per transaction, like the small blobs, with commits last. No await separates the stores, so they still reach disk together. A store that throws rolls back only its own object, and with commits last no commit is stored before the pack's trees. A new test covers a tree the cache refuses (mode 100664, which git only reports as informational) arriving after its commit, as in the packs git sends. Same production round, interleaved: vscode 13.1-14.5 s (was 23.7-27.4 s), every object stored. TypeScript-size packs (65k objects) still take about 38 s (was 48.6 s), over the default limit.
|
LGTM! |
ndisidore
marked this pull request as ready for review
October 4, 2026 17:05
ndisidore
commented
Oct 4, 2026
Maximo-Guk
added a commit
that referenced
this pull request
Oct 4, 2026
Each blob consumePack stores as it arrives sat in its own storage.transaction, and the metadata put inside opens a nested one for its index. A transaction is there to undo a store that fails after its first write, and what can fail there is parsing the objects the stored one names, in #referentEntries and the marking walk. A blob names none, and an oversized object is only measured, so both are now stored without the wrapper. Commits, trees and tags keep theirs. If the metadata write itself failed, the blob's row would stay with no metadata: a hash-verified object no gatekeeper can read, which the next pull completes. First pull in local workerd on Durable Object SQLite, best of three, on a machine that was busy for part of the "before" runs: - vscode (23,280 objects): 8.2 s -> 7.3 s - TypeScript (65,044 objects): 14.5 s -> 11.2 s Not measured in production, where the third commit of #656 found the nesting far more expensive than it is locally. Co-Authored-By: Claude Code <noreply@anthropic.com>
Maximo-Guk
added a commit
that referenced
this pull request
Oct 4, 2026
Two things #656 depends on had no test. Every consumePack test builds a byte stream by hand, the one shape that is certain to support BYOB reads. Production gets a default stream that gatekeeper-github's demuxGitFetchResponse makes in another Worker and Workers RPC carries over; handed to the decoder directly, such a stream throws "This ReadableStream does not support BYOB reads". The new test loads a small Worker through the LOADER binding, which calls consumePack on a GitCacheImpl stub with a default pull stream, as a gatekeeper does. A ref-delta whose base comes later in the pack decoded on main and is now rejected. Git writes a base before its deltas, so nothing real should send one, but the rule was only stated in a comment. Co-Authored-By: Claude Code <noreply@anthropic.com>
Maximo-Guk
added a commit
that referenced
this pull request
Oct 4, 2026
The GitCache.consumePack doc called it exactly equivalent to a put() of each object. It differs in ways a gatekeeper can see: an oversized object is left out of the result where put() throws and, since #656, a delta has to follow its base and a pack that fails partway can leave some of its objects stored. The return value was not described at all. Co-Authored-By: Claude Code <noreply@anthropic.com>
Maximo-Guk
added a commit
that referenced
this pull request
Oct 4, 2026
consumePack keeps every object over MAX_GIT_OBJECT_SIZE inflated until the pack ends, because a later delta may name one as its base. A blob fetch sends no filter, so a batch read can carry any number of them, and the pack's 64 MiB cap bounds only their compressed size. Thirty 5 MiB files that compress well are 150 MiB against the Overseer's 128 MB. #656 kept them the same way, in `held`. They are now kept up to MAX_OVERSIZED_BASE_BYTES (32 MiB), the oldest let go first and the newest always kept, since git writes a delta soon after its base. A delta that names one already let go fails the pack with "delta base ... is unavailable". Every oversized object before it has been measured by then, so the next pull does not ask for them, and without them in the pack the blob that needed the base should arrive whole. That retry is reasoned, not exercised. Nothing here measures Worker memory; the test checks which base is still on hand once two blobs pass the budget. Co-Authored-By: Claude Code <noreply@anthropic.com>
Maximo-Guk
added a commit
that referenced
this pull request
Oct 4, 2026
The budget on oversized delta bases failed a pack whenever a delta named a base it had let go, on the reasoning that the sizes recorded by then keep those objects out of the next request. That holds only for a request that named each blob. A mount wants a commit and sends no haves, so the remote answers a retry with the same pack, and it fails the same way: a valid pack could never be mounted. Devin's review caught this. Bases are now let go only from a pack that has carried nothing but blobs, which is what the answer to explicit blob wants looks like. A pack with a commit, tree or tag in it keeps every oversized base, as #656 does. For the blob case the retry now happens inside the same read. ensureGitObjects checked recorded sizes once, before its first pull; it now checks on every round, so after a pull fails on a dropped base the objects it measured surface as GitObjectTooLargeError, ensureBlobs takes them out of the batch, and the next pull brings the dependent blob whole. This also turns "every connection failed" into the too-large error whenever a failed pull was what measured the object. Two tests fail on the previous commit: a pack with a commit and two blobs past the budget is consumed, and a batch read whose pack fails on a dropped base completes in one ensureBlobs call with two pulls. Co-Authored-By: Claude Code <noreply@anthropic.com>
Member
|
🤖 Reviewed, feel free to take whatever makes sense from #660 |
Maximo-Guk
added a commit
that referenced
this pull request
Oct 4, 2026
MAX_GIT_PACK_BYTES did two jobs: it capped the pack, and through maxObjectSize it capped each object inflated from one. The 64 MiB came from gatekeeper-context's artifact sync, where it bounds a repository loaded into memory, and consumePack matched it while it still read the whole pack into memory before decoding. Since #656 the pack streams through, so its cap no longer bounds memory, only how much one pull may download, decode and store. The per-object cap still is a memory bound. They are now separate: MAX_GIT_PACK_OBJECT_SIZE stays at 64 MiB, and MAX_GIT_PACK_BYTES goes to 256 MiB, the smallest round figure that admits a depth-1 mount of torvalds/linux (190.8 MiB, 98,591 objects). That pack through consumePack, local workerd on Durable Object SQLite: every object stored (222.8 MiB of rows), the tree listing all 102,334 paths, in 44 s fed from a local stream and 70 s through an RPC hop in GitHub-sized pieces. In production it would run past the 30 s CPU limit, which this does not touch. Co-Authored-By: Claude Code <noreply@anthropic.com>
Maximo-Guk
added a commit
that referenced
this pull request
Oct 4, 2026
fetchGitUploadPack gave the whole fetch 120 s, body included. That suited a body read straight into memory. Since #656 the pack streams on to the overseer, which reads it only as fast as it stores it, so the budget now covers decoding and storing the pack too. A depth-1 mount of torvalds/linux took 70 s that way in local workerd, and a pack at the 256 MiB cap would be past 90 s. The 120 s now covers the wait for the response and an error body. Once the pack is streaming, git-transport.ts fails the fetch if the server sends nothing for GIT_FETCH_STALL_MS (60 s) while the pull is waiting on it. Time the reader takes between reads does not count. git's upload-pack sends a keepalive every few seconds when it has nothing else to send, so a live fetch stays well inside that. Two tests with fake timers: a body that goes quiet fails after 60 s, and a reader that pauses for ten times that between reads does not. The change to fetchGitUploadPack itself has no test; nothing covered its timeout before either. Co-Authored-By: Claude Code <noreply@anthropic.com>
Maximo-Guk
added a commit
that referenced
this pull request
Oct 4, 2026
Native inflate needs an entry's whole compressed stream at once, and joins its output from 16 KiB chunks. For one large object that is the stream, the chunks and the joined copy all live together: review worked it through for a 48 MiB blob deflated at level 0, about 150 MiB against the Overseer's 128 MB. The pako reader in #656 held one read of input, so the same entry fit there. An entry declaring more than 1 MiB now goes back to that loop: pako, a read at a time, with no input kept beyond the read in hand. Its output is written into one buffer of the declared size, so the chunk list and the copy that joined it are gone too, and inflating that 48 MiB entry takes about 48 MiB where #656 took 96. Entries up to 1 MiB, which is all of a mount pack but the odd large tree, stay on native inflate, where the most one can buffer is now a little over a megabyte. pako's Inflate import, its internals interface and that cast are back for this path. It is given windowBits 15, so it reads zlib only, not the gzip it would otherwise also accept. Run for real: the 48 MiB level-0 entry goes through consumePack and is measured; the vscode, TypeScript and Linux mount packs decode to the same totals, TypeScript's two trees over 1 MiB on the new path. Memory is reasoned from what each path holds, not measured. Co-Authored-By: Claude Code <noreply@anthropic.com>
The decoder checks each entry's declared size against maxObjectSize before inflating it, but a size varint with enough zero continuation digits overflows its multiplier to Infinity, and 0 * Infinity leaves a NaN size that the comparison lets through. On a 1,024-byte cap such an entry inflated 8 MiB before failing. The varint loop now rejects the entry once the next digit would be worth more than the cap, since no size within the cap needs it.
Deflating each object in encodeLooseObject is the largest single CPU cost of storing a pack, and CPU is what still stops large mounts. On this repo's 1,378 tracked files of up to 64 KiB, level 1 deflated 1.53x faster than the default level 6 and stored 11% more bytes. In local workerd a vscode first pull went from 7.3 s to 6.2 s with 12% more stored bytes, and a TypeScript one stored 9% more. Any zlib level is a valid loose object: oids hash the inflated bytes, isomorphic-git and decodeLooseObject read every level, and rows already stored at level 6 stay readable. Push packs are re-compressed by buildPackBytes, so they are unaffected. The extra bytes are kept for as long as the workspace is, since nothing repacks the store.
A pull sends no haves, so a retried pull, or one for another commit of an already-mounted repository, delivers mostly objects the store already holds. #storeVerifiedObject rewrote each of them: a fresh deflate, an object put and a metadata put, for no change. An object that is already present, measured and proven on this gatekeeper's remote is now left as it is. Nothing it would write can differ: the payload is fixed by the oid, its referents were recorded and its pending-push marks propagated when it was first stored, and marking walks descend into present objects themselves. Another gatekeeper's copy is still recorded as that gatekeeper's proof. In local workerd, delivering the same vscode pack again went from 6.7-7.9 s to 2.8-3.2 s.
|
LGTM! |
Maximo-Guk
approved these changes
Oct 5, 2026
The consumePack tests hand the decoder a byte stream. A gatekeeper
sends a default stream from another Worker, which the decoder's BYOB
reader refuses when handed one directly ("This ReadableStream does not
support BYOB reads"); it works only because Workers RPC delivers it as
a byte stream. A new test sends a real pack that way, from a Worker
Loader worker in 100-byte chunks, and checks every object is stored
with its bytes intact.
It said consumePack is exactly equivalent to decoding the pack and put()ting each object. Since packs are streamed that no longer holds: a delta must follow its base, and a pack that fails can leave some of its objects stored, though never a commit without its trees. The doc also now says what the result holds and that an object too large to store is left out of it rather than thrown on.
When the fetch response fails -- a server ERR line, the transfer limit, a truncated response -- consumePack sees the pack stream it reads across RPC only end early, and rejects with "disconnected prematurely". That is what gitPull reported, so an agent mounting a repo too large to fetch was told the connection dropped, and retried. demuxGitFetchResponse takes an optional onFailure callback, and pullGitObjectsIntoCache rethrows the failure it reports in place of consumePack's rejection. A consumePack failure of its own still surfaces when the fetch itself succeeded.
ndisidore
force-pushed
the
chore/git-pack-streaming
branch
from
October 5, 2026 11:45
76c2dfa to
6685c06
Compare
|
LGTM! |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Mounting a large repo as a worktree ran the Overseer Durable Object out of memory.
consumePackheld the whole pack, every inflated object and a deflated copy of each before storing anything. For a TypeScript- or vscode-size mount that came to 173-224 MiB, against a 128 MB limit.decodePackStreamnow decodes the pack in one pass.consumePackFromGatekeeperstores blobs of up to 1 MiB as they arrive. It holds commits, trees, tags and oversized blobs until the trailer verifies, then stores them one per transaction with commits last, so a failed pack can leave verified trees and blobs behind but never a commit that looks mounted. Loose objects are compressed with nativenode:zlibinstead of pako.Smaller changes in the same PR, one commit each. A pack entry whose size varint overflows is now rejected; before, its NaN size slipped past the object size cap. Loose objects are deflated at level 1, about 1.5x faster for 9-12% more stored bytes. An object a gatekeeper already stored is left untouched when a pull delivers it again, which took a repeat vscode pull from about 7 s to 3 s locally. A failed git fetch now reports the server's error or the transfer limit, not a stream that disconnected prematurely. The
consumePackcontract doc now describes streaming, and a new test sends a pack over real Workers RPC.This should let a repo like vscode mount, as long as its mount pack fits the 64 MiB cap. The real mount packs for react, next.js, TypeScript, vscode and a 52 MiB internal repo went through this code on Durable Object SQLite in workerd. Every object was stored, except TypeScript's two trees over 1 MiB, which were measured and skipped as before. In Node, the decoder held about 8 MiB after GC on the TypeScript pack.
haves