fix(storage): grow the upload buffer instead of sizing it to chunk_size

Stack

Merge in order. !2192 (merged) targets this MR's branch, so its diff shows only its own changes.

Rebase constraint for !2192 (merged). Its docs/dev/configuration-reference.md hunk was cut against the pre-fix wording: it reinstates "a fill holds about its own transfer size" and "the peak ... up to twice its current size", both of which this branch established as false and replaced. Those removed lines no longer exist at this head, so the patch conflicts at merge time and a naive resolution regresses a published fix in the user-facing reference. On rebase, keep this branch's arithmetic and apply only the default split (5MiB GCS, 64MiB S3).

#	MR	What
1	this MR	fix(storage): grow the upload buffer instead of sizing it to chunk_size, plus GOMEMLIMIT
2	!2192 (merged)	fix(config): lower the GCS chunk_size default to the driver's own constant and bound it at 64MiB

What

The GCS driver sized its upload staging buffers from storage.gcs.chunk_size instead of from the payload, and allocated them eagerly. At the 64 MiB default that charged about 128 MiB of heap per in-flight blob upload whatever the layer weighed, which is what OOM-killed the staging pod during a docker push of two blobs totalling 3.6 MB.

The buffer now grows toward chunk_size as bytes arrive, so chunk_size bounds a session rather than sizing one. GOMEMLIMIT is set alongside it so the Go runtime paces its GC against the pod's memory limit at all.

Root cause

Two allocations, both independent of the payload:

• internal/storage/driver/gcs/gcs.go:585 allocated make([]byte, d.chunkSize) the moment driver.Writer() returned.
• internal/storage/driver/gcs/writer.go:128 allocated a second buffer of the same size in ReadFrom, which is exactly what the OCI upload path calls (internal/format/oci/store.go:126, internal/format/oci/upload.go:1393).

driver.Writer() runs on every request that touches an upload session, at internal/storage/pg_session.go:128 (POST initiate), internal/storage/pg_blobstore.go:575 (PATCH and PUT resume), internal/storage/pg_session.go:1040 (commit re-open) and internal/storage/pg_session.go:1636 (cancel re-open).

The mechanism is GC pacing, not resident set

This is the part worth reading, because the naive reading of the numbers above is wrong. A large make([]byte, n) is faulted in a page at a time, so an untouched 64 MiB buffer costs accounted heap rather than RSS. internal/format/npm/packument_cache.md had already measured it on the npm path: four such buffers with 64 KiB written into each reported 256 MiB of heap-inuse against 6 MiB of RSS growth.

Accounted heap is what the GC paces against. With GOGC=100 setting the heap goal at twice the live heap and nothing telling the runtime about the cgroup, two concurrent layer uploads moved the goal past 512Mi, so collection did not run early enough and genuine garbage accumulated until the kernel killed the pod. That is consistent with the observed symptom: SIGKILL with no Go out of memory trace, surfaced to clients by Envoy as 503 upstream connect error.

Two aggravating findings

internal/config/storage.go:37 sets defaultGCSChunkSize to 64 MiB, silently overriding the driver's own gcs.DefaultChunkSize of 5 MiB at internal/storage/driver/gcs/gcs.go:81, which documents itself as "matching the Container Registry default". Because the config loader always substitutes a value, the chunkSize <= 0 fallback at cmd/artifact-registry/wire_storage.go:313-315 is dead code, and the GCS conformance suite tests 5 MiB while staging runs 64 MiB. This MR does not change the default; the fix makes the buffer proportional to the payload, so the default now bounds a ceiling instead of setting a floor. Reconciling the two constants is out of scope here and still needs its own issue.

The S3 driver was never affected the same way: its part buffer is a bytes.Buffer that grows on demand (internal/storage/driver/s3/s3.go:1500). That is why a local MinIO-backed instance sat at 81 MiB RSS after 3 days 21 hours of uptime while GCS staging died. Its ReadFrom did carry the same full-chunk scratch allocation, and gets the same bound here.

Relationship to #447

This retro-explains #447 exactly. At 256Mi a single PATCH's 128 MiB pair is half the limit, which is why a one-layer image survived and a three-layer image crash-looped. #447 was mitigated by raising the limit to 512Mi in !1221 (merged), which said in terms that it was not a fix and must be reverted once a root cause landed, and was then closed as Complete against its own author's note asking to keep it open. The comment in .runway/fairway.yaml still points at it.

Two places in the tree had already written the hazard down and neither was actioned: docs/dev/configuration-reference.md ("neither driver pools it"), and docs/plans/2026-08-14-npm-packument-streaming-generation.md, which named this very fix and deferred it as "a session option for a staging buffer sized to the document, which is storage-layer work this plan does not take on". It landed in the driver rather than as a session option, so no such option exists.

What changed

fix(storage), the defect:

• The writer grows buffer toward chunkSize on demand (growBuffer) instead of being allocated at it, and carries chunkSize as a field so the flush boundary no longer depends on len(buffer). Growth doubles but clamps the capacity at chunkSize, so the slack a bare append leaves does not outlive the growth. Measured filling one 64 MiB chunk in 256 KiB steps on go1.26.6: cap 64.0 MiB rather than 68.1 MiB, 9 reallocations rather than 22, 127 MiB of churn rather than 337 MiB, 96.3 MiB peak accounted heap rather than 133.3 MiB. main charges a flat 128 MiB per session for the same upload.
• Write rejects a chunk size below MinChunkSize. Below it writeChunk has no aligned prefix to flush, so the copy loop can neither fill nor drain the buffer and would spin on a request goroutine with no error and no log. The zero value a writer built without the field carries sits in that range.
• writer.driver is removed. ReadFrom was its last reader, and no linter flags an assigned-but-unread struct field.
• ReadFrom copies through one flush granularity (256 KiB) instead of a second full chunk, on the GCS and S3 drivers alike.
• initResume grows the buffer to MinChunkSize before its read loop, which is bounded by len(buffer), to preserve the invariant its doc comment states.
• The doc passages that described the old preallocation are corrected: docs/dev/configuration-reference.md, internal/format/npm/packument_cache.md, and a Research Findings note on the packument streaming plan.

fix(runway), the guard:

• GOMEMLIMIT: "400MiB" in .runway/values.yaml, below the main container's limits.memory in .runway/fairway.yaml (512Mi; the glaz-sidecar block in that file carries its own, smaller memory limit). It is a soft limit, so the GC works harder as the heap approaches it rather than failing an allocation, and a pod that would have been killed stays alive and diagnosable.

The two commits must not be split, and must not be reverted separately. An earlier revision of this description offered to split the runway commit out; that was wrong. Against a buffer allocated at chunk_size, GOMEMLIMIT=400MiB is worse than leaving it unset: those buffers are accounted heap the GC cannot reclaim while a session is open, so the runtime collects continuously and still does not get under the limit, trading a fast SIGKILL for sustained GC against a 200m CPU limit. internal/format/npm/packument_cache.md measures four such buffers reporting 256 MiB of heap-inuse against 6 MiB of RSS. Reverting fix(storage) alone is the revert an incident reaches for first, so the .runway/values.yaml comment states the coupling next to the value.

Effect

A 3.6 MB two-blob push now holds roughly its own transfer size in buffers rather than about 576 MiB of accounted churn.

Testing

• Five new tests in internal/storage/driver/gcs/writer_test.go pin the properties: buffer length tracks bytes staged rather than chunkSize (including a 123-byte layer against the 64 MiB default), neither length nor capacity exceeds chunkSize, a flush leaves the buffer for reuse so growth never repeats, each growth step doubles, growth preserves unflushed bytes, the ReadFrom scratch stays within MinChunkSize, and a chunk size of 0 or below MinChunkSize is refused instead of spun on. MinChunkSize itself is covered as a positive case, so the guard's boundary is pinned on both sides.
• internal/storage/driver/s3/writer_test.go carries the mirror of the ReadFrom scratch test, so the S3 half of that change is covered too.
• go build ./... clean; ./internal/storage/... and ./internal/format/npm/... pass.
• golangci-lint (pinned 2.13.2) reports 0 issues on the GCS and S3 packages, both plain and with --build-tags=integration. One pre-existing staticcheck deprecation in s3/driver_integration_test.go:189 (ReverseProxy.Director) is unrelated and untouched.
• Comment caps gate passes.

Verification gap to be explicit about: the GCS writer's Write, ReadFrom and resume paths are covered by the //go:build integration conformance suite, and there is deliberately no GCS emulator, so those paths could not be exercised locally. The end-to-end proof has to come from test:integration:gcs-key-creds and test:integration:conformance:gcs-key-creds in this pipeline. Please treat those jobs as the gate rather than the unit tests above.

e2e scenarios

No scenario in docs/testing/ is added or affected. The change alters how much memory the upload path allocates, not any observable protocol behavior: request and response shapes, status codes, digests and resume semantics are unchanged, which is what the GCS conformance suite asserts.

Review follow-ups

Two commits on top of the original two, from a first review of this branch:

• fix(storage): guard the writer chunk size and clamp buffer growth covers the Write guard, the capacity clamp, the removal of writer.driver, the three failpoints_test.go writers that omitted chunkSize (and the comments that named the buffer length as the mechanism), the missing S3 test, and GitLab Duo's wording fix in the configuration reference.
• fix(runway): tie GOMEMLIMIT to the driver behavior it assumes names the main container's limits.memory and records the revert coupling above.

A second review round added eleven commits across ten findings, the eleventh being the AGENTS.md convention paired with the packument-cache fix: the fill-memory ceiling arithmetic in the configuration reference, the GOMEMLIMIT comment's grown-vs-allocated claim and its sidecar antecedent, oomChunkSize's comment, a post-flush and doubling pin on the growBuffer table, the scratch test's bound and name, the append idiom the comment misnamed, the S3 test's move to writer_test.go, the packument-cache steady-state claim, and hoisting growBuffer's new capacity onto its own line.

A third round added six commits across nine findings: the S3 twin's rename to the bound it asserts, the GOMEMLIMIT comment's branch state and positional pointer, the AGENTS.md rule's opening sentence, three further claims in the fill-memory passage, the review-time vocabulary in oomChunkSize's comment, and the assertion the value pin already implied. Three of the nine resolved without a commit: the min(payload, chunk_size) claim, corrected here and in #1035; the !2192 (merged) rebase constraint, recorded under the stack table; and the question of whether the AGENTS.md convention should ship on its own MR, kept here as a deliberate call recorded in its thread.

A fourth round added one commit across three findings, naming the fill path's real step size (1MiB, not the driver ReadFrom scratch's 256KiB). The other two corrected #1035's Evidence section, which still quoted the replaced sentence, and this record.

One finding is deliberately not addressed. The Research Findings correction to docs/plans/2026-08-14-npm-packument-streaming-generation.md rides in the fix(storage) commit, where AGENTS.md's path-prefix table asks for docs(plans):, which is the prefix every commit touching docs/plans/ on main uses. Splitting it out means rewriting a non-HEAD commit and force-pushing a branch that is already under review, so it is left as the operator's call.

What this does not do

• Reconciling defaultGCSChunkSize with gcs.DefaultChunkSize, and bounding storage.gcs.chunk_size: now !2192 (merged), stacked on this MR.
• The two legacy comment blocks in internal/format/npm/packument_cache_internal_test.go that still say the driver "preallocates" the chunk size: also !2192 (merged), where they are compressed to pointers at internal/format/npm/packument_cache.md.
• An admission cap on concurrent upload sessions, the OCI analogue of npm's rebuildMaxRendering: now #1035. It is the term this MR does not remove. Once this merges a session holds the smallest doubled capacity that covers the payload, up to chunk_size, so a transfer past half of chunk_size still holds the full chunk_size and nothing bounds how many are in flight. Budgeting sessions at min(payload, chunk_size) under-provisions by 2x in the 32-64MiB band.
• A note on initResume that its read loop can now stop on a full buffer as well as on EOF, because the buffer it grows is MinChunkSize rather than chunk_size. Behavior is unchanged for every marker this driver writes, whose tail is always below MinChunkSize; a longer one would fail the offset check rather than be read. The 2-line comment cap at that site would cost the more useful half of the comment there now, which is why the growBuffer call exists at all.
• Exposing /debug/pprof, tracked in #58.

Withdrawn from this list

An earlier revision of this description claimed a missing defer session.Cancel() on the handler paths that return with a session open, and called it a distinct backend-resource-leak defect. That was wrong and is withdrawn. An OCI upload session is designed to outlive the request that opened it: handleInitiate persists the upload_sessions row precisely so a later PATCH can resume it, and the recoverable faults (the PATCH 413, the 416 offset mismatch) leave the session intact deliberately, which the code comments at those sites state. The terminal paths do cancel, through cancelOnFinalizeError and session.Cancel(). What remains true is that an abandoned session is reclaimed by nothing, and that is already #498 rather than anything new.

Why Related to and not Closes

#1023 (closed)'s ## Ask names three things: profile the upload path with heap pprof, find the retention, and fix it. This MR does the second and third. It does not do the first, because /debug/pprof/* is not exposed on any listener today (#58), so no heap profile of a running pod was possible; the root cause was established by reading the allocation sites and the in-tree measurement instead. The issue also asks that "512Mi should be comfortable", which wants a staging observation after this deploys. Both remain, so the issue should stay open after this merges.

Also still open once this lands: .runway/fairway.yaml carries a comment pointing at the closed #447 and asking for the 512Mi bump to be reverted. Whether to revert it is a decision for after this change has run in staging, and it should be retargeted at #1023 (closed) either way.

Related to #1023 (closed)