now it's the right branch

I’m reviewing the branch as a code review: first I’ll identify the PR tied to the current branch, read its description, and inspect the diff and touched files before I assess risks and regressions.

I have the PR description and the change surface. Next I’m reading the implementation and tests in the touched bootstrap files to look for behavioral regressions around the new streaming observer and early-abort path.

The main logic is clear now. I’m checking two things before I write findings: how PushPack/transport surfaces read errors from the upload body, and whether the new observer’s lifecycle can block or misclassify failures.

The package tests pass. I’m checking the exact upload path now, because the highest-risk issues here are not in the happy path; they’re in how an early body-read error interacts with net/http and whether the observer can deadlock or race when the upload stops mid-stream.

I found one likely regression in the retry heuristic and I’m pulling exact line references now. After that I’ll write up the review with findings first, plus the PR description summary you asked me to read.

Findings

1. Medium: the new selfImposedBudget ratchet can become far smaller than the target’s real body limit when the server returns an explicit 413 before consuming much of the request body. In bootstrap.go you correctly parse a server-advertised limit into limit, but in bootstrap.go you still ratchet selfImposedBudget down to raw sentBytes on every non-self-aborted failure. If a proxy/server rejects early, sentBytes may be only a few MiB even though the actual cap is, say, 100 MiB. Subsequent retries will then self-abort around that much smaller budget and over-subdivide aggressively. I’d gate the sentBytes ratchet on “no parseable server limit”, or at least avoid preferring sentBytes when targetBodyLimit(pushErr) returned a trustworthy explicit limit.

I read the PR description for PR #40, “Streaming packfile parsing”. The stated goal is to stream-parse outgoing packs, abort oversized uploads early based on projected size, and use observed object counts to improve post-failure subdivision/calibration so blob-heavy repos converge in 1–2 retries instead of many more.

I also ran go test ./internal/strategy/bootstrap/...; the package tests pass. Other than the budget-ratcheting issue above, I didn’t find another clear regression in this diff.

I’m doing a second pass as a stricter code review rather than rephrasing the first one. I’m re-reading the changed code with adjacent context and checking for retry, lifecycle, and concurrency edge cases that the current tests may not cover.

The branch moved since the first pass; there’s now additional batching logic beyond the original streaming observer change. I’m incorporating that newer diff into the review, because it changes both the retry behavior and the shape of source fetches.

I’m re-running the review against the current branch state and the current PR, rather than relying on the earlier branch contents. First I’ll read the branch/PR metadata and the current diff, then I’ll inspect the touched code and tests for regressions.

I have the current PR description and the current change surface. Next I’m reading the bootstrap implementation and tests as they exist on this branch now, then I’ll verify behavior with targeted test runs before I give findings.

I’ve narrowed the remaining review work to the new retry math and observer edge cases. The happy path is covered; what matters now is whether the early-abort logic still converges for packs whose first completed object arrives late, and whether the tests exercise that case.

Findings

1. Medium: the early-abort path still degrades back to binary subdivision when the first completed object arrives late. In bootstrap.go calibration only switches to objectsSent when objectsSent > 0, and in bootstrap.go projected sizingBytes is only used when objectsSent > 0 && totalObjects > 0. For a pack whose first object is a very large blob, the observer can self-abort before any object finishes scanning, leaving objectsSent == 0; the retry then sizes from raw sentBytes at the abort floor and computes factor == 2 again. That recreates the slow 1→2→4→… convergence this PR is trying to remove, and there’s no test covering the abortedEarly && totalObjects > 0 && objectsSent == 0 case in bootstrap_test.go.

2. Medium: minBytesBeforeAbort makes learned budgets below 8 MiB impossible to enforce client-side. shouldAbortPush returns false until bytesSent >= 8 MiB regardless of the actual budget in bootstrap.go and bootstrap.go. But nextSelfImposedBudget can ratchet selfImposedBudget down below that, e.g. to a 5 MiB proxy cutoff, in bootstrap.go and bootstrap.go. In that state the self-abort path can never trigger before the server does, so retries keep paying for server-side rejection instead of the advertised early cutoff. The current tests cover learning a 5 MiB budget but not honoring it on the next push.

The current PR description says the branch adds a streaming packfile.Scanner, aborts doomed uploads early from projected overshoot, and uses observed object counts to improve subdivision/calibration so blob-heavy retries converge in 1–2 rounds.

I also ran go test ./internal/strategy/bootstrap/...; it passes.

I’m re-running the review from the current branch state instead of relying on the last pass. I’ll refresh the diff, re-read the changed bootstrap logic and tests, and only keep findings that still apply to what’s actually checked out now.

The worktree is dirty in bootstrap_test.go, so I’m separating the committed branch diff from any local edits in that file. I’m checking whether the current branch already addressed the last findings and whether the unstaged test changes materially change the review.

The current branch does include fixes for the two issues I called out last time: it now synthesizes one observed object for the “header parsed, no object completed” abort case, and it now honors learned sub-8 MiB budgets via the absolute threshold path in shouldAbortPush. I’m doing one more pass for anything still wrong in the updated retry logic rather than repeating stale findings.

I’m down to verifying whether the dirty bootstrap_test.go is substantive. If it’s only test coverage and the committed code already incorporates the functional fixes, the correct review outcome may simply be “no findings” with a note about residual risk.

git diff is clean despite git status showing a modified test file, so there’s likely a line-ending or metadata discrepancy rather than additional content I need to review. I’m just finishing the full test sweep now to make sure the branch is otherwise consistent.

No findings in the current committed diff.

The two issues from the prior review are addressed now: the retry path handles abortedEarly && objectsSent == 0 via effectiveObjectsSent(...), and shouldAbortPush(...) now honors learned budgets below minBytesBeforeAbort through the absolute threshold path. The new tests in bootstrap_test.go cover both regressions, and go test ./... passes.

Residual risk is still the one implied by the PR itself: this changes streaming upload behavior in a concurrent reader/scanner setup, so the remaining uncertainty is integration-level behavior against real HTTP intermediaries rather than an obvious logic bug in the branch.