feat(config): derive the upload session concurrency cap and its wait (upload-session-concurrency-cap plan: 1/12)

Step 1 of the upload session concurrency cap plan.

What

Adds storage.max_concurrent_upload_sessions and storage.upload_session_wait_timeout, with the cap derived at boot from the live GOMEMLIMIT and the effective driver chunk_size. No enforcement: Step 11 reads these at the composition root.

Why now

#1254. Cloudflare granted the 50 GB upload limit on the AR zone only with Unbuffered Uploads enabled (case 02317090, consented), which streams request bodies straight to the origin. Slow-upload mitigation is ours from that point on, and each in-flight upload holds an object-storage staging buffer. Nothing caps how many exist at once (#1035), so a handful of trickling clients can exhaust pod memory.

The derivation

Half the live GOMEMLIMIT, divided by a per-slot reservation of 1.2 × chunk_size + 1MiB on GCS and 2 × chunk_size + 1MiB on S3, floored at 2. At the shipped defaults:

Driver	chunk_size	Per-slot	200 MiB ÷ slot	Cap
GCS	5 MiB	7 MiB	28.57	28
S3	64 MiB	129 MiB	1.55	2, floor binding

Both literals the plan fixes are named constants rather than inlined: the 0.5 budget fraction, and the 400 MiB fallback for an unset GOMEMLIMIT, where debug.SetMemoryLimit(-1) reports math.MaxInt64 and would otherwise derive an effectively unbounded cap. Unset is the common case — CI sets none — so that fallback is the path most runs take.

On a default S3 install the floor binds and over-admits: two slots at 129 MiB is 129% of the budget. The plan accepts this and Step 1 logs a warning saying so, because the alternative at that chunk_size is a cap of 1, which deadlocks the npm rebuild outright. The lever for that install is chunk_size, not the cap.

The floor is 2 because an npm rendering rebuild holds the full and abbreviated sessions together. At 1 it takes the only slot, waits for a second only it could release, burns the whole budget and sheds — every time, permanently.

The boot records

Every path that settles a cap leaves one, because the plan makes the INFO line the thing that turns "set a large value" into an actionable escape:

• Derived: the cap, the live GOMEMLIMIT, the driver, its chunk_size, the budget, and whether the floor bound.
• Floor bound: a warning naming the floor, the budget, the per-slot reservation, and derived_before_floor — the pre-floor quotient, which is how far short the derivation fell.
• Configured: the value in force, source=configured, the budget it spends against, and derived_max_concurrent_upload_sessions, the cap the derivation would have chosen. Without this an operator taking the documented escape got no record at all, and a cap of 100000 against a 200 MiB budget was accepted silently.

A config the cross-field invariants go on to refuse emits none of them, so the boot log of a failed start never advertises a cap that never took effect.

The wait

Defaults to 5s and is clamped to 15s rather than refused above it, with a warning naming both values.

⚠️ Reviewer attention here. The plan pins the budget fraction and the GOMEMLIMIT fallback as literals, but never fixes the wait default or its ceiling — it only says the wait must stay "short relative to" npm's 30s rebuildTimeout and single_flight_wait_timeout (5m). So these two numbers were chosen, not derived from the plan, which is exactly the failure mode the plan warns about for the other two literals. The reasoning for both rests on the same 30s rebuildTimeout: the ceiling is half of it, so even a clamped wait leaves a parked rebuild half its budget, and the default sits well under that. Both are named constants, easy to change — but they deserve a second opinion rather than a nod.

An earlier revision justified the 5s default as matching "the flat 5s both sibling shed paths already use". That was an inverted borrow, corrected in 191b300b: inlineBuildShedRetryAfter (internal/format/npm/inline_build.go:58) and markedImageRetryAfter (internal/format/oci/manifest_push.go:675) are Retry-After header values — how long a client is told to wait — not server-side wait budgets, so neither was ever a precedent for this knob.

The ceiling is restated rather than imported, since internal/config must not depend on a format package.

Rejections

A non-positive value for either field is refused at load, but by different guards, and only the cap needs one of its own. The wait is a duration string, so parseDurationField already refuses a non-positive value and names the field; the field-specific sentinel an earlier revision added for it was unreachable behind that, and 191b300b drops it. The rejection table now asserts the wording each half actually produces, so neither case can pass through the other's guard.

The cap's rejection plus the derivation's floor is what licenses Step 2's panic on a non-positive capacity: the rejection covers an operator-set value, and the floor covers the paths that set none — an omitted field, a present storage block selecting no backend, and an omitted storage block, which until 52ecb75e skipped resolveUploadSessionControls entirely and left both controls at their Go zero values (raised in Duo review; TestLoad_Storage_AbsentBlock now asserts them). A server-only config therefore carries a cap of 2 and a wait of 5s rather than 0 and 0s, and no path through Load can hand Step 2 a value it panics on.

The seam

GOMEMLIMIT and the log sink are injected through an unexported loadOptions parameter threaded from Load, exposed to tests via export_test.go. Not package-level vars: this package runs 43 t.Parallel() cases against shared state, and a mutable global would make them order-dependent. Mirrors the func-value seams in internal/storage (pg_blobstore_options.go's WithClock).

This is what pushes the source half over forecast — see below.

Testing

21 cases across the two controls. Locally at b378d300: gofmt, go vet, whole-repo go build, the full config package, golangci-lint on ./internal/config/..., and scripts/ci/check-comment-caps.sh all clean.

The cases were checked for bite rather than assumed. Changing the floor from 2 to 1 reddens exactly the two cases that assert it and nothing else; and each fix in the review round below reddens exactly its own case — dropping derived_before_floor reddens the floor-warning pair, dropping the configured-cap record reddens the explicit-cap case, and restoring derive-before-validate reddens the refused-config case.

Per the acceptance criteria: the caps are asserted as 28 and 2 rather than as "different caps"; the floor case uses a chunk_size at or above the whole budget, which is what makes the floor falsifiable; the floor warning is asserted as a pair, firing on the S3 default and silent on the GCS one, so it cannot degrade into noise; and the clamp is asserted as the clamped value, the case that reddens if the clamp is deleted and the knob silently accepts ten minutes again.

Both boot records are asserted field by field, not by their message. Asserting only that the floor warning fired passed against a warning missing every field, which is the version worth catching.

Review round

Three commits answer a review pass over the merged step:

• 191b300b — the wait's unreachable guard and sentinel, and the default's inverted precedent (above).
• a1f8c01d — the configured cap's missing boot record, and derived_before_floor on the floor warning, which the acceptance criteria asks for by name.
• b378d300 — finalizeStorage validates before deriving, so a refused config leaves no boot record.

e2e scenarios

No scenario in docs/testing/ is added or affected. This step adds configuration surface and boot-time logging only; nothing refuses or delays a request until Step 11 wires the gate at the composition root, which is where the shed becomes observable to a client.

LOC

574 excluding generated code — 277 source, 279 test, 18 docs and config — counted as added plus deleted against merge base f5c23d9c, measured at b378d300. Forecast was ~180 / ~470 / ~650. Under overall, but the source half is ~95 over because the seam had to be threaded through configFromProto and optionalStorageFromProto rather than living in storage.go alone. The test half came in ~190 under, since the fixture helpers already existed.

Docs

Only what is true at this merge, since the file documents as-is behaviour and this step enforces nothing. Both rows, plus the chunk_size coupling on the storage.s3 and storage.gcs rows because the derivation reads them, and the boot-record sentence covering the configured path as well as the derived one. The rows describing the control refusing work belong to Step 11, which is the step that makes them true.

Closes #1231 (closed)

🤖 Generated with Claude Code