Let the target decide when a bootstrap checkpoint cannot be split further

[nodo]

Follow-up to #116 / ENT-2054, fixing the failure that landing #116 exposed in production. Tracked in ENT-2060.

Problem

Batched bootstrap pushes history in chunks split between commits. Once every gap is one commit there is nothing left to split, and a single commit whose pack exceeded the budget failed the whole run.

The budget it failed against was our own, and 20× stricter than the target's. TargetMaxPack defaults to 512 MiB; autoTargetMaxPackBytes derives 5 GiB from the target's announced 10 GiB limit and then discards it for being larger than that default. The observer then aborts at 95% of the 512 MiB. So the run gave up on a pack the target would likely have accepted, having never asked it.

Seen in production on a >10 GiB mirror once #116 got it onto the bootstrap route:

target rejected pack — switching to batched mode (limit 512 MB)
projected to exceed target limit (512 MB) — splitting 1 → 3 packs
Sync failed: push bootstrap batch for refs/heads/main:
             pack upload aborted early: projected to exceed target body limit

Zero batches landed, and every redelivery repeated it — a full multi-GiB source fetch each time.

Solution

The abort budget for a span you cannot split is the target's number, not ours.

The small budget stays for planning: it bounds the waste of a doomed push and makes the temp ref advance often — both of which need a smaller pack to be possible. On a one-commit gap neither is, so that push uses the target's announced limit instead, decided before the push rather than after a doomed attempt.

This costs no extra bytes — a ceiling is an abort threshold, not an upload size, so a pack that fits sends the same bytes either way — and it avoids fetching an indivisible multi-GiB commit twice. At that ceiling there is no 95% margin and no projection: an early cut would invent a rejection the target never issued.

That turns the failure into something classifiable:

Outcome	Classification
Abort against our budget, no word from the target	retryable — a larger budget or raised server limit could still work
Target refused it, or it overshot the target's own announced limit	permanent: ErrCheckpointExceedsTargetLimit
Deadline (408/504)	retryable — availability, not size; with zero bytes sent there is no size evidence

The sentinel is aliased into the root package so the mirror worker can match it with errors.Is and stop redelivering an identical pack ten times. It is also the evidence that would justify object-level splitting — the only remedy left for a genuinely oversized commit, and something nobody should design until a repo proves it is in that state.

Two guards keep this honest:

• A measured cutoff is never escalated past. If the budget was inferred from bytes actually sent — a middlebox that cuts without announcing — that is evidence about size, and the ceiling stays put.
• A deadline decides nothing, in either direction. A target that drained the body and then timed out told us about time, not size, so it neither claims that provenance nor erases it. Claiming it would disable escalation for the rest of the run; erasing it would let escalation jump past a demonstrated cutoff, where an abort is classified permanent. The rule is one named function, nextBudgetProvenance, because both directions are costly and easy to conflate.

Subdividing only happens when the current span can shrink. subdivideToFactor and subdivideCheckpoints split every remaining gap, so a splittable gap later in the branch used to grow the checkpoint list even for a one-commit span — and the retry then re-fetched and re-pushed a byte-identical pack, once per later split. Both the post-failure path and the pre-flight estimate now decline instead. The pre-flight declines rather than being skipped, so the pack header is still parsed and object_count stays real in the logs an operator reads when a giant commit stalls.

Also here

bootstrapWithInputs returned Result{} on error, discarding RelayMode, RelayReason, batch counts and temp refs. Those describe the route, not the outcome, and are set before anything can fail — zeroing them is why a failed sync could report its strategy only when it succeeded, the gap that made ENT-2054 a source read instead of a log query. Batching is now recorded before checkpoint planning (whose commit-graph fetch is the likeliest failure for exactly the repos that batch), and unstable.Client.Bootstrap no longer throws the result away either.

Scope worth stating: this covers the bootstrap route only. A failed incremental or materialized relay still reports an empty transfer mode — incremental.Execute returns a zero result on every error path, and the syncer returns before copying its relay facts — and most other syncer error paths still return a zero result. Same blind spot, one strategy over; it needs its own tests and is filed rather than bundled here.

Tests

Each mutation-verified to fail when the behaviour it names is removed:

• a divisible span still aborts at the small budget — pins that only unsplittable checkpoints escalate
• an indivisible one converges at the announced ceiling, in one push
• no safety margin at that ceiling: a pack in the top 5% must not be cut
• the >= boundary, where an in-batching rejection leaves budget and announced limit equal and escalating still sheds the margin
• a measured cutoff blocks escalation; a deadline neither claims nor erases one (table test over all four cases)
• an indivisible span is not re-pushed while later gaps split, post-failure and in the pre-flight (asserted by source-fetch count: 9 vs 11)
• a deadline stays retryable; a hard rejection is permanent immediately; unrelated errors (401/500/reset/hook decline) stay retryable
• both AnnouncedTargetLimit capture sites — the feature's only production sources, since nothing configures it
• a failed batched run still reports its route

Two fixtures aimed at the provenance guard through Execute turned out vacuous — the runs never reached an escalation decision, so they passed with the guard removed. They were deleted rather than kept, and the rule was extracted into a function so it could be tested directly.

Rollout — please read before merging

1. The worker-side change must ship in the same release as the git-sync pin bump. Without it the permanent case still redelivers ten times, now paying a source re-fetch per attempt. It is pushed on nodo/ent-2060-worker-permanent-checkpoint in mirror-pipeline — sentinel classification, failure-route reporting on spans and logs, and the pin — currently pinned at this branch's head and needing a re-point at the merge commit.
2. Needs an entiredb owner's sign-off: this sends a multi-GiB receive-pack POST from inside batched mode. Previously only the one-shot path approached the cap, and the runbook documents a push rejected there leaving "refs present, 0 objects".
3. Scoped to entiredb→entiredb until the preflight-auth fix lands. lookupGitHubRepoSizeKB sends its api.github.com request unauthenticated, so it 404s on private repos; a GitHub-kind repo entering batching via that preflight never sees a rejection, so never learns an announced limit.

Notes

• Rebased onto main after Keep the batched cutover's resume marker when the target refuses the branch create #117 and squashed: the five original commits were an iterative refinement of the same lines, and resolving their conflicts one at a time against the moved main produced worse results than resolving the net change once. Soph's cutover tests pass alongside these.
• Whether the repo that motivated this converges depends on whether its one oversized commit fits under 10 GiB. This change is what will tell us, instead of failing against a number we picked. That placement is suspended meanwhile.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AriFousNMM5gxkW7Akq1SW

[nodo]

All three blockers were real and are fixed in 703d9da. Each now has a regression test that I confirmed fails with the bug reintroduced.

1 — sentinel fired on unrelated errors. Correct and embarrassing: the gate sat outside the size-failure branch and keyed off !subdivide, which is true for everything that isn't a size rejection. The classification now lives inside if subdivide && len(chain) > 0, so a 401/500/reset/hook-decline can't reach it. TestExecuteBatchedUnrelatedErrorStaysRetryable covers four shapes; with the old gate restored it fails with your exact string (…cannot be subdivided further: http 401 unauthorized).

2 — a genuine 413 could never reach the sentinel. Also correct. Terminal now covers both verdicts the target can give: a rejection it actually sent (isBatchableTargetPushError), or a retry that already ran at its announced limit and still overshot. TestExecuteBatchedHardRejectionIsPermanentWithoutSelfImposedAbort pins the first case at 1 push, no retry.

3 — the relaxed ceiling leaked. Fixed by making it a per-attempt relaxedBudget that overrides the aborter for one push and is cleared when the checkpoint advances; selfImposedBudget is never written. TestExecuteBatchedRelaxedBudgetDoesNotLeakToLaterCheckpoints runs a 4-commit chain and asserts a later checkpoint still aborts on the small budget — it fails (aborts=[1]) with the leak restored. Your point that this also disabled the fix for branch 2+ was the one I'd have missed.

Correctness findings, all taken:

• 4 — isIndivisibleCheckpoint now takes current. You were right about the resume path specifically: with idx-1 it counted the gap from the chain root after the stale-temp-ref re-plan, on exactly the ENT-2054 route.
• 5 — in-loop parsedLimit now feeds AnnouncedTargetLimit, so a run that enters batching without a one-shot attempt can still relax. This also drops the landing-order coupling with the preflight-auth fix that the PR description had to warn about.
• 6 — Batching/RelayMode set where the route is chosen, not where it succeeds.
• 8 — a budget ratcheted down from bytes actually sent is now flagged as a measured cutoff and the retry won't jump past it.
• 9 — aliased into the root package with the errors.Is doc block, next to the other caller-facing sentinels. You're right that the internal one was unreachable from mirror-pipeline.
• 10 / 7 — error-path Result now carries Plans and Measurement, and client.go / unstable/client.go no longer zero the result on the syncer.Run error path.
• 11 — helper is now chainPosition(cp) - chainPosition(current) == 1, no chain-wide map per failure, and the doc matches what it does. Dropped the dead len(chain) == 0 disjunct.
• 12 / 13 — the store is built once and both the desired hash and parent map derive from it; the fixed packBytes parameter is gone; there's now a multi-checkpoint case.
• 14 — the double source fetch is stated in the comment.

One thing I did not do: your suggestion that a test should exercise the projection branch of shouldAbortPush rather than the absolute one. You're right that the current tests only hit bytesSent >= threshold (budgets are far below the 8 MiB minBytesBeforeAbort), and right that the fake pack's version field makes the scanner reject it so totalObjects stays 0. Reaching the projection path needs a valid multi-MiB packfile in a unit test, which I judged not worth the fixture weight for a branch that only picks which number sizes the next subdivision. Happy to be argued out of that.

Full suite and golangci-lint green.

[nodo]

Rebased onto main (post-#117) and addressed the adversarial review. Head is now 0cf1d733, mergeable.

Rebase note: main had moved 8 commits including #117, which touches the same executeBatched code. Cherry-picking my five commits individually kept colliding — they were an iterative refinement of the same lines, so each resolution fought the next — so I applied the net change once and squashed. Params keeps both sides' fields; the test file I rebuilt deterministically from main plus my new functions rather than trusting conflict boundaries that cut mid-function, then re-applied the makePackHeader hoist since main still had it as a local closure. Verified Soph's cutover tests pass alongside mine, and my behaviours are still mutation-caught on the merged tree.

The review's core finding was coverage, not correctness — three mutants survived the entire suite. All three now fail:

• The gate itself was untested. Dropping isIndivisibleCheckpoint from the ceiling decision left everything green, so nothing pinned the feature's central claim that only an unsplittable checkpoint escalates. That mutant pushes every checkpoint at the announced ceiling with no margin and no projection — silently destroying the bound on wasted upload that is the whole reason TargetMaxPack is small. The budget-leak test I deleted during the simplification had been the closest thing to coverage, and I replaced it with nothing. Now pinned by a divisible span that must still abort at the small budget.
• The >= boundary was untested. Now pinned by a pack at 97.8% of an announced limit the run learns from its own one-shot rejection.
• The measured-cutoff guard is covered at last, using the reviewer's fixture. They also diagnosed why mine couldn't bite, which I'd failed to explain: on a one-commit chain the guard is structurally unreachable — any failure that would set it ends the run on that same checkpoint, leaving no later iteration to read it, and a pusher that never drains leaves sentBytes at 0 so it is never set at all. It needs two commits: a divisible span to take the observation, then an indivisible one to consult it. That reasoning is now recorded in the test.

Worth flagging for anyone else mutation-testing this file: removing that guard from the condition makes the variable unused, so the mutant doesn't compile, and a grep for --- FAIL reports a false negative. That bit me once here.

Also fixed the three comments still describing the retry this design removed — including a dangling paragraph about the deleted relaxedBudget fused onto the new comment — and the announced-ceiling notice now fires once per branch instead of per push, since the ceiling is chosen per push and an uneven-gap stretch would otherwise repeat it for packs nowhere near either number. Structured log still records every push.

Full suite + golangci-lint green.

[nodo]

bugbot run

[nodo]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit ca89abb. Configure here.}

[nodo]

bugbot run

[nodo]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit fcd1aa2. Configure here.}

[nodo]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit fcf98d1. Configure here.}