Do not re-push an indivisible span while later gaps still split

Claude Code·Opus 5.[1m]·nodo·4d ago·16hr 12min·4 Checkpoints·12 file changes·+331/-47·5.4K tokens

can you gather context on https://github.com/entireio/git-sync/pull/118 I have some questions

4d ago·1m·12 calls

Context loaded. Here's the state of entireio/git-sync#118 — "Let the target decide when a bootstrap checkpoint cannot be split further".

Status: open, nodo/ent-2060-bottom-out-server-verdict → main, +835/−26 across 7 files, head 3e55d75b. CI green (test/lint/govulncheck/licenses); reviewDecision: REVIEW_REQUIRED — no human review yet, only Cursor Bugbot.

Core change (internal/strategy/bootstrap/bootstrap.go): when checkpoint subdivision bottoms out at one commit per gap, the push abort ceiling escalates from TargetMaxPack (self-imposed, ~512 MiB default) to AnnouncedTargetLimit (new Params field, parsed from a target rejection), with no 95% margin and no projection at that ceiling. Gated by isIndivisibleCheckpoint(batch, current, idx) and !budgetFromObservation (measured-cutoff guard). Terminal condition isTargetBodyLimitError(pushErr) || (atAnnounced && abortedEarly) returns ErrCheckpointExceedsTargetLimit, aliased into the root package in errors.go.

Secondary change: partial Result on error paths — bootstrapWithInputs now returns route facts (RelayMode, Batching, BatchCount, TempRefs, Plans) instead of Result{}; Batching/RelayMode set before checkpoint planning; client.go and unstable/client.go stop zeroing on error.

One open Bugbot finding (medium, unaddressed, posted 15:23 against current head 3e55d75b) — "Doomed pack retried while later gaps split":

atAnnounced and the permanent sentinel both key off the current span being one commit, but the retry still runs subdivideToFactor over all remaining checkpoints. A later gap that can still split makes continue re-fetch and re-push the same indivisible pack at the announced ceiling. The sentinel only fires after every later gap is also one commit, so a commit the target already refused can be uploaded several times in one run.

An earlier Bugbot finding (high, "sentinel misfires on non-size errors") was against the pre-rebase d4649ff1 and you've replied that both halves are fixed.

Two things worth knowing before you ask:

  1. Your local git-sync worktree is on the pre-rebase branch — ahead 5, behind 10, tip b06c763d. The remote was force-pushed to the squashed 2-commit version. Local code ≠ PR code.
  2. The PR body carries three rollout blockers: worker-side classification must ship in the same release as the pin bump (branch nodo/ent-2060-worker-permanent-checkpoint in mirror-pipeline, go.mod still points at this branch); needs an entiredb owner's sign-off for multi-GiB receive-pack from batched mode; scoped to entiredb→entiredb until the lookupGitHubRepoSizeKB preflight-auth fix lands.

Minor: bootstrap_test.go has a duplicated drainAbort doc comment (the comment block appears twice, once orphaned).

What are your questions?

can you explain me this change like I am 5?

4d ago·26.8s

The one-sentence version: git-sync was giving up on packs the server would have happily accepted, because it was measuring against its own much stricter rule and never actually asking the server.

The story

You're mailing a huge pile of books to a friend. The post office's limit is 10 kg per box. But you made yourself a stricter house rule: never pack more than 0.5 kg per box.

Why the stricter rule? Two good reasons. If a box gets rejected, you only wasted a little effort. And lots of small boxes means your friend can tick off "got it!" often, so if you get interrupted you don't start over.

So you pack: too heavy → split the pile in half. Still too heavy → split again. Again. Again.

Eventually you're down to one single book. And that book weighs 0.6 kg.

You can't split a book in half.

Old behavior: you look at your 0.5 kg house rule, say "too heavy, I give up," and stop. The whole delivery fails. But the post office would have taken it fine — it's 0.6 kg, and their limit is 10 kg. You never asked them. And then you'd try the whole thing again tomorrow, hauling the entire pile from home each time, and fail identically. Ten times.

New behavior: when you're down to one unsplittable book, you stop using your own house rule and use the post office's actual limit instead. Let them decide.

Two nice things about this:

  • It costs nothing. A weight limit is a "when do I stop," not a "how much do I send." A book that fits sends exactly the same book either way.
  • You find out the truth. If the post office genuinely says "no, too heavy" — now you know. That's a real, permanent no. So git-sync raises a special flag (ErrCheckpointExceedsTargetLimit) meaning "stop retrying, this will never work", and the worker stops hauling the pile from home ten more times to learn the same thing.

The rules for who gets blamed:

What happenedVerdict
Your own house rule stopped youkeep trying — a bigger rule might work
The post office actually refused itpermanent — stop
The post office was closed / slowkeep trying — that's not about weight

The second, smaller change

Separately: the delivery receipt used to only get filled in if the delivery succeeded. So when things failed, nobody could tell whether you'd gone by truck or by mail — you had to go dig through the original pile to find out. Now the receipt records which route you took as soon as you pick it, even if the trip then fails.

Translation table

StoryReal thing
the pile of booksthe repo's git history
a boxone pack pushed to the target
your 0.5 kg house ruleTargetMaxPack (512 MiB default)
the post office's 10 kg limitAnnouncedTargetLimit (what entiredb says)
splitting the pilecheckpoint subdivision
one unsplittable booka single commit — subdivision splits between commits
hauling the pile from homea full multi-GiB source fetch, per retry
ca89abbDo not re-push an indivisible span while later gaps still split subdivideToFactor splits EVERY remaining gap, so a splittable gap later in the branch grows the checkpoint list even when the current span is already one commit. The growth branch ran before classification, so that growth triggered a retry — and a one-commit gap has no midpoint to gain, so the retry re-fetched and re-pushed a byte-identical pack. Repeated once per later split, that is the same doomed upload several times in one run, and on a deadline at the announced ceiling each repeat is multi-GiB. Subdividing is now attempted only when the current span can actually shrink. When it cannot, there is nothing to retry for: fall through and classify, which either returns the target's size verdict or a retryable error whose redelivery re-plans from the temp ref. Pinned by a 5-commit chain planned into 4 batches, which yields gaps of 1,1,1,2 — the first checkpoint indivisible while the last gap can still split — failing on a deadline so the old code took the growth path. Two pushes of the identical pack before, one after. Entire-Checkpoint: 01M1C6YZ80MHFE69RAW9ZJPCP2+67/-6

thanks buddy

4d ago·3.1s

Anytime. That open Bugbot finding about re-pushing the doomed pack when later gaps can still split is the one thing still worth a look before merge — ping me if you want to dig into whether it's real.

9e82686A deadline is not size evidence, for provenance as well as classification A target that drains the body and then times out — GitHub's 408 shape — leaves abortedEarly false and sentBytes above zero, so the budget ratcheted to those bytes AND recorded itself as a measured server cutoff. That flag gates escalation and is cleared only by a later parseable 413, so one deadline disabled the feature for the remainder of the run: exactly the wrong outcome on the flaky multi-GiB targets this path exists to serve. It also contradicted the design's own principle. Classification already treats a deadline as availability rather than size; provenance was treating the same error as measured size evidence. The ratchet stays — smaller packs genuinely do finish inside the window, so the smaller budget is useful information about time — but it no longer masquerades as a size limit. Pinned by a run that takes a one-shot 413 announcing 1 MiB, then a drain-then-408 on a divisible span, and must still escalate an indivisible span afterwards. Without the fix it fails with the same "aborted early: projected to exceed target body limit" the reviewer's probe produced. The pre-flight subdivide had the same asymmetry the previous commit fixed after a failed push: subdivideCheckpoints splits every remaining gap, so a splittable later gap grew the list and re-planned an identical checkpoint even for a span that cannot shrink. Cheap there — a 12-byte header read, then Close — but the same shape, so it is closed the same way, which also sidesteps comparing the estimate against TargetMaxPack rather than the ceiling an indivisible span is actually pushed at. Docs: the sentinel's comment claimed a Bootstrap method the root package does not have (it is unstable.Client.Bootstrap, and that comment is the mirror worker's contract); Plan, Sync and Replicate now document that the result is populated on error, what may be read from it, and that a validation or config failure still returns a zero result; and a duplicated doc comment from the rebase is removed. Entire-Checkpoint: 01M1CG7DAC6WEPQGWG55D25GN4+112/-9
fcd1aa2A deadline decides nothing about budget provenance, in either direction The previous commit stopped a deadline from CLAIMING a measured cutoff, but wrote the answer unconditionally — so a deadline that ratcheted the budget also assigned false, erasing a cutoff an earlier unparseable size rejection had recorded. Escalation would then jump past a limit the server had already demonstrated, and an abort at that ceiling is classified permanent: a flaky target turned into a false permanent failure. Strictly worse than the bug it replaced. The rule is now a named function rather than an inline expression, because getting it wrong in either direction is costly and the two directions are easy to conflate. A deadline preserves whatever was known; a parsed limit means the target stated its own bound and supersedes any measurement; anything else that ratcheted the budget did so from observed bytes, which is a measurement. Extracting it also made it testable. Two fixtures aimed at this through Execute were vacuous — the runs never reached an escalation decision, so they passed with the guard removed — and I deleted them rather than keep tests that prove nothing. The table test on the function covers all four cases and fails when the deadline branch is removed. Entire-Checkpoint: 01M1CMYZYQ31YWA5NR9PG91CGT+66/-1
fcf98d1Decline the pre-flight subdivide instead of skipping it, and pin the behaviour Skipping checkPackSizeAndSubdivide for an unsplittable span also skipped the only thing that parses the pack header, so packObjectCount fell to 0 on exactly the push this feature exists for: object_count and estimated_bytes logged as zero on both lines, and calibrateBytesPerObject got a zero denominator. The calibration loss was inert — an indivisible span never takes the growth path now, so no later iteration consumes it — but the logs are the ones an operator reads when a giant commit stalls. The call is made unconditionally again; the callback declines instead. Same outcome for subdivision, header still parsed, counts still real. The skip was also unpinned: removing it passed the entire suite, which stands out in a change whose every other claim is mutation-verified. Now pinned by fetch count — 5 commits into 4 batches gives gaps of 1,1,1,2, and a header declaring 200 objects makes the estimate exceed the budget so the pre-flight actually fires. Nine fetches with the decline, eleven without: each wasted fetch is an indivisible span re-planned before pushing. Docs: the sentinel's comment had a 107-character line in a block that wraps at 79; observedSubdivisionFactor's doc block sat orphaned above a different function and had started documenting nextBudgetProvenance, so it moves down to the function it describes; and the client comment claimed Counts describe attempted work when the error path does not populate them at all — Applied is 0 even where refs were pushed before the failure. That comment is the mirror worker's contract, so it now says to read Refs and ignore Counts. Entire-Checkpoint: 01M1DY4KRMG0JM8ZA6NC56Z3AZ+86/-31