git-sync sync --tags https://github.com/entireio/cli.git $ARTIFACTS_REMOTE

No response requested.

You already have a turnkey smoke test for exactly this — it defaults its source to https://github.com/entireio/cli.git. Two paths depending on what you want to validate:

Path 1 — full end-to-end against your local Entire (recommended)

This actually pushes the cli.git repo through git-sync into a local Entire instance, then probes both sides and asserts the ref hashes match.

What it does (internal/syncer/entire_local_smoke_test.go:33):

1. Reads your local Entire credentials via the keychain (entiredb must be on PATH, or set GITSYNC_E2E_ENTIREDB_BIN)
2. Creates a git-sync-smoke repo on your local Entire if it doesn't exist
3. Runs Run(...) with source https://github.com/entireio/cli.git, branch main, target http://localhost:8080/git/<user>/git-sync-smoke
4. Probes both sides and fails if the ref hashes don't match

Override knobs (env vars): GITSYNC_E2E_ENTIRE_REPO, GITSYNC_E2E_ENTIRE_BRANCH, GITSYNC_E2E_ENTIRE_PROTOCOL=v2, GITSYNC_E2E_ENTIRE_BATCH_MAX_PACK_BYTES=....

Path 2 — quick CLI run against a local target

If you just want to drive the CLI binary you built and watch the logs:

If the local target repo doesn't exist yet, create it first with entiredb repo create test-repo-cli (or via the frontend at http://localhost:5173).

Path 3 — reproduce the original 500 with the new error output

Just re-run your original command — the diagnostic-headers patch in internal/gitproto/smarthttp.go will surface Cf-Ray=…, Server=…, Content-Type=…, etc., next to the body text. That gives the Cloudflare-side operators something to look up in their logs.

For day-to-day validation that the relay/bootstrap path works on cli.git, Path 1 is the most thorough — it asserts ref-hash equality between source and target, so you'll catch silent corruption, not just non-failing pushes.

Found it — the git-sync-smoke repo already exists on your local Entire (from a previous run), but the test's existence check doesn't notice. Two problems combined:

1. entiredb repo list doesn't print plain name\n lines — its output contains protobuf-style control bytes (\016, \022\032, \r) around each name, so entire_local_smoke_test.go:178 never matches git-sync-smoke even though it's there.
2. The fallback entiredb repo create git-sync-smoke then fails with AlreadyExists, and cobra prints the usage block above the actual error — that's the message you saw.

Quickest unblock — just rerun

That uses a fresh repo name on every run, sidestepping both issues.

Slightly tidier — reuse existing repo

…but only after deleting the stale one (or once the test is patched — see below). To delete:

The real fix

Make ensureEntireRepo tolerate an already-existing repo. The check-then-create pattern is racey and brittle anyway; treating AlreadyExists as success is more robust than parsing repo list output. Want me to patch internal/syncer/entire_local_smoke_test.go:164 so it inspects the create-command output for the "already exists" gRPC code and returns nil in that case?

This is a legitimate failure, not noise. The flow that hit you:

1. The target already has refs (so it's not a clean bootstrap target → bootstrap relay path skipped at internal/syncer/syncer.go:476).
2. FetchToStore (syncer.go:497) fetches with the target's ref hashes as haves. The server walks reachability and excludes any object reachable from those haves, so the returned pack is a minimal delta.
3. Incremental relay couldn't take the plan, so it fell back to materialized.
4. materialized.collectObjectClosure walks the source object graph using only the literal target ref hashes as haves (internal/planner/planner.go:411). It doesn't know about reachability — only exact hash matches.
5. Object 0fccf698… is reachable from a target ref (and therefore wasn't sent in the pack), but it isn't itself one of the target ref hashes. So the walker tries to load it from the store and fails.

This is a genuine seam between the wire's reachability-based have-set and the walker's literal-only one. It only bites when the target has refs whose object closure overlaps with the source closure but the relay path can't be used.

Easy ways to validate sync is working without hitting this

1. Force the bootstrap path against the same artifacts target (cleanest signal)

If you can delete test-repo-cli on the cloudflare target and recreate it empty, your next git-sync sync … will take the bootstrap relay path — that's the path your patch from earlier was about, and it bypasses the materialized walker entirely.

2. Local Entire smoke test (already discussed)

This validates source→target equivalence end-to-end with https://github.com/entireio/cli.git and a guaranteed-fresh target, so it always exercises the bootstrap path.

3. Re-run against an artifacts repo that's empty

If the artifacts service supports it, point $ARTIFACTS_REMOTE at a brand-new repo that's never been pushed to. Same logic as (1).

If you actually want to fix the underlying issue

The materialized walker's have-set should be reachability-closed, not just the literal target ref hashes. One of:

• Before walking, expand targetRefs into a full closure by asking source for those commits' objects (counterproductive — partly negates the bandwidth win of haves).
• Or: when collectObjects hits a missing object, treat the parent as a boundary instead of failing — i.e., assume the gap means the server already-have'd that subgraph. Risky if a real corruption exists.
• Or: switch the materialized strategy to skip the explicit walk and let go-git's pusher compute the closure with proper haves negotiation.

For your immediate goal of "validate sync is working", I'd go with option 1 above (empty artifacts repo) — it tests your patched 500 path and avoids this orthogonal bug. Want me to dig into a real fix for the materialized/haves mismatch?

Tests fail without the fix with the exact same error shape the user saw (load object …: object not found), and pass with it. Now full test suite:

No response requested.

Full suite is green. Summary of the fix:

What was wrong

internal/planner/planner.go:438 (collectObjects) walks the source object graph using only the literal target ref hashes as haves. But internal/gitproto/fetch.go:166 sends those same hashes to the source server as have lines, and the server prunes everything reachable from them — including blobs and trees that are transitively reachable from a have but aren't themselves haves.

Net effect: when the target has an existing ref whose object closure overlaps with the source closure, the walker hits objects the server intentionally didn't send and crashes with load object <hash>: object not found. Exactly your symptom.

The fix (internal/planner/planner.go)

1. ObjectsToPush: validate each filtered want is in the store up front, so a missing top-level want still fails loudly with load want <hash>: ….
2. collectObjects: when store.EncodedObject returns plumbing.ErrObjectNotFound for a transitively-walked hash, treat it as implicitly have'd (return nil, exclude from the push pack). Other errors still propagate.

Same precedent already exists in isAncestor at planner.go:398 — missing object during the commit walk = boundary, not error.

Why this is safe: if the source server is correct, the only reason a referenced object isn't in our store is that it was reachable from a have we advertised — which means the target already has it, so leaving it out of our push pack is the right answer. If the source server is buggy and produces a real gap, the target's receive-pack rejects with "missing necessary objects" — fail-closed at the protocol level.

Tests added (internal/planner/planner_test.go)

• TestObjectsToPushTransitiveMissing — builds a want commit whose tree references a blob that's not in the store (simulating the server's prune), with a have ref. Without the fix it produces the exact load object …: object not found error you saw; with the fix the pack contains the want commit and its tree, and the missing blob is correctly excluded.
• TestObjectsToPushMissingWant — asserts a missing top-level want still surfaces a load want error, so we don't silently produce empty packs.

Files changed

• internal/gitproto/smarthttp.go — earlier patch: include diagnostic headers (Cf-Ray, X-Request-Id, Server, Content-Type, etc.) in httpError, trim trailing whitespace from body.
• internal/planner/planner.go — the materialized-walker fix above.
• internal/planner/planner_test.go — two new tests.

To validate against your artifacts target: re-run your original go run cmd/git-sync/main.go sync --tags https://github.com/entireio/cli.git $ARTIFACTS_REMOTE. The materialized fallback path should now succeed if it was the only blocker; if you still hit the 500, the new diagnostic headers in the error message should give you something concrete (Cf-Ray=…) to share with Cloudflare.

All green. Try your sync command again — it should get past the materialized walker now. If the pack ends up empty for a given branch (target already has the object), the receive-pack push will still send the ref-update command and target will accept it because it has the object reachable from one of its existing refs.

Quick recap of the revised diagnosis:

0fccf698… is the tip of 1072-… on cli.git, and it's an ancestor of main. Your earlier push put main (or a descendant) on the artifacts target, so the target now reachably has 0fccf698. When this sync ran, the source server saw want 0fccf698 and have <target's main>, decided the client already had the closure, and pruned it from the pack. The walker then couldn't load the want.

The fix in internal/planner/planner.go: drop the up-front "wants must be in store" check — both top-level wants and transitive references can legitimately be missing for the same reason (server pruned them as reachable from a have). The walker now treats any ErrObjectNotFound as an implicit-have boundary. Updated the test to cover this case (TestObjectsToPushMissingWantImplicitlyHaved), kept the transitive-missing test, removed the now-incorrect "missing want errors loudly" test.

If you do hit a real bad-input case (typo'd hash with no corresponding object on either side), the target's receive-pack will reject with "missing necessary objects" — the failure surface moves from the local walker to the target protocol, which is a less local error but still fail-closed.

The fastest way to see what's actually wrong is to force HTTP/1.1 — that bypasses the protocol error and the Go runtime's retry-mask, and the server's real error response (if any) will land in our 500-handler with the new diagnostic headers.

Try:

(Older Go versions used GODEBUG=http2disable=1; http2client=0 is the current spelling.)

That tells the Go runtime to skip ALPN HTTP/2 negotiation. Three possible outcomes:

1. The push succeeds — Cloudflare's HTTP/2 edge had a bug or hit a frame-size limit. We'd want to harden git-sync's transport (cap HTTP/2 use for receive-pack or expose a flag).
2. The push fails with a real HTTP error — httpError will render the status, body, and the diagnostic headers (Cf-Ray=…) you can hand to whoever runs the artifacts service.
3. The push fails the same way — confirms the issue is upstream, not protocol-version-specific.

If you want, I can also add a --target-disable-http2 flag (or env var) that calls tc.ForceAttemptHTTP2 = false and clears tc.TLSNextProto on the cloned transport — so this is a one-flag toggle rather than a process-level GODEBUG. Worth it once we know which of (1) / (2) / (3) you're in. Run with GODEBUG=http2client=0 first and report back.

Speculation about the root cause while you try that: HTTP/2 PROTOCOL_ERROR mid-push from a Cloudflare-fronted git endpoint is most often either (a) a backend git process crashed or sent malformed sideband and Cloudflare translated it into a stream reset, or (b) trailers/length mismatch on a chunked body. Both are server-side; neither is something our diagnostic headers patch can recover, but Cf-Ray from a successful HTTP/1.1 attempt (or an HTTP/1.1 5xx with a body) gives them the lookup key.

$ARTIFACTS_REMOTE isn't set in my shell, so I can't probe the target. But you can — running git ls-remote --refs $ARTIFACTS_REMOTE will show exactly which refs are already on the target. That'd confirm whether my "target already has main reachable from this object" hypothesis explains the materialized-walker case from the previous turn (separate from this current connection-close issue).

For the connection-close itself, the actionable next step is option 1 above: do a vanilla git push --mirror from a fresh bare clone of cli.git to the same artifacts URL. The outcomes:

• Vanilla git push fails too → artifacts server has a real issue with cli.git (possibly large-pack handling, slow client, auth quirk, or genuine bug). Not a git-sync bug. Server-side investigation only.
• Vanilla git push succeeds → git-sync is producing a request the server rejects. Most likely culprit given the path we're on: the materialized strategy is now successfully producing a pack but it's a thin pack (delta bases reference objects on the target that aren't in our store/pack), and Cloudflare's git backend may not accept thin packs even though the protocol allows them. Worth investigating.

Either way, the connection close + PROTOCOL_ERROR pattern is server-emitted and there's no client-side fix that would surface a clearer error than what we have. Try the vanilla push first; the answer dictates where to look next.

Confirmed — target has main plus the v0.x tags, source has all that plus 50+ branches. So git-sync correctly identifies a lot of branch creations to push. With my walker fix, every want hash that's reachable from main (which is most of these branches, since they were merged into main) gets pruned by the source server and skipped by the walker. That part is working as intended.

The actual push to receive-pack is what's failing — and the failure mode (PROTOCOL_ERROR / connection close, no HTTP body) is server-emitted with no client-side recovery available. Three concrete experiments to narrow it down, in order:

A. Push just main (a no-op) to verify the protocol/auth path works at all

This should be near-instant (main is already at the right hash). If it succeeds, the auth and basic push work. If it fails the same way, the issue isn't pack content — it's something about the request itself.

B. Push a single new branch at a time to isolate

Pick one branch the target doesn't have, e.g.:

That branch's tip (0fccf698) is reachable from main, so the pack for that push should be effectively empty (just a ref-update command). If the server rejects an empty-pack ref-create, that's a strong signal the artifacts service has a specific bug.

C. Vanilla git push for the same single branch

Same operation, vanilla git. If C succeeds and B fails, git-sync is sending something subtly different. If both fail, server bug.

The most informative comparison is B vs C with the same single ref. That'll cleanly answer whether this is a git-sync issue or an artifacts-service issue, without confounding factors from large packs or many refs.

Found the relay-decision wrinkle. Looking at internal/planner/relay.go:77:

Incremental relay only accepts ActionUpdate (fast-forward) for branches. Your full sync has many ActionCreate plans (all the branches target doesn't have yet) → relay rejected → materialized fallback → server rejects the resulting pack for some reason we can't see from the client.

The bootstrap path you saw in test B works exactly because it's branch creates with no other managed refs on target. The relay decision wrote off the multi-create case as "use materialized," and now we know that path doesn't survive the artifacts server.

Pragmatic workaround now

Push each branch in its own invocation so each one takes the bootstrap path:

(Slow because it'll ask for credentials per call — pre-cache them with git config --global credential.helper osxkeychain or similar before running.) Then a final pass with --tags for tag refs.

Real fix (the actual bug behind the bug)

The relay rule is overly conservative. For ActionCreate on a branch where target isn't empty, we could still relay: pass all target refs as haves to the source's upload-pack. The source emits a minimal pack covering only what target doesn't already have, and the target's receive-pack accepts the create command because its closure is reachable from existing target refs — same logic as the implicit-have fix we just landed in the materialized walker.

That'd make incremental relay handle "create branch" alongside "update branch," which is the common case for syncing a new mirror to an artifacts service that already has main. Materialized would only kick in for genuinely tricky cases (force, prune, retargets).

Worth doing as a follow-up — it's a bigger change than today's fix and needs careful testing for the relay invariants. Want me to draft it as a separate change, or stop here for now?

For your immediate goal of validating "is sync working," the per-branch loop above should let you mirror cli.git successfully and confirm git-sync's logic end-to-end against the artifacts target.

All green. Recap of the proper fix:

What changed

internal/planner/relay.go — CanIncrementalRelay:

1. Branch creates are now relay-eligible. Previously rejected as incremental-branch-action-not-update, even though the relay path already passes target refs as haves and the receive-pack accepts the create when the new ref's closure is reachable from existing target refs. New rejection: only if Action == ActionCreate but TargetHash != ZeroHash (incoherent plan shape).
2. no-thin targets are now tolerated. Same rationale as SupportsReplicateRelay (relay.go:14-23): gitproto.FetchPack never requests thin-pack, so the relayed pack is always self-contained and safe for any receive-pack. Comment updated to mirror the replicate one — if FetchPack ever requests thin-pack, both functions will need a matching fallback.

Tests

internal/planner/planner_test.go:

• Replaced TestCanIncrementalRelayRejectsBranchCreate with TestCanIncrementalRelayAcceptsBranchCreate (asserts the new positive behavior).
• Added TestCanIncrementalRelayRejectsBranchCreateWithNonZeroTarget for the incoherent-plan guard.
• Added TestCanIncrementalRelayMixedCreateAndUpdate to confirm a mixed plan list still relays.
• Replaced TestCanIncrementalRelayRejectsNoThin with TestCanIncrementalRelayToleratesNoThin.

internal/syncer/integration_test.go:

• Added TestRun_IntegrationIncrementalRelayCreatesNewBranchWithExistingTarget — exact reproduction of the user's scenario at unit-test scale: target has master, source adds a new branch, sync should now take incremental relay (not materialized).
• Added TestRun_IntegrationIncrementalRelayCreatesNewBranchOnNoThinTarget — same scenario against a no-thin target.
• Updated TestRun_IntegrationMaterializedLimitFailsClearly to use Force: true to force the materialized path (it previously relied on no-thin to do that).

End-to-end picture

For your --tags sync against the artifacts target, the planner will now:

• Detect main already current → skipped, not in pushPlans.
• Detect all the new branches → ActionCreate plans, eligible for incremental relay.
• Detect new tags → ActionCreate plans, eligible for incremental relay (already were).
• All plans eligible → incremental relay path → source streams a pack with target refs as haves → target receive-pack accepts.

Avoids the materialized-walker rabbit hole entirely. Worth re-running:

Expect either success or, if there's a separate server-side issue, an error from a much cleaner code path that we can diagnose more directly.

This is the diagnostic-headers patch from earlier doing exactly its job — Cf-Ray=9f613eac387d3917-TXL, Server=cloudflare, plus the body Invalid or expired token. That's a real, actionable error this time.

Two things explain why no credential prompt:

1. The server returned 403, not 401. A 401 ("not authenticated") is what triggers credential prompts; a 403 ("token rejected") is the server saying "you sent credentials, they're just no good." So no prompt fires.
2. Your earlier successful runs cached working credentials in the macOS keychain. Subsequent invocations are pulling that cached token automatically. It's now expired/invalidated, so the server rejects with 403, and we never get to the prompt path.

Fix

Clear the cached cloudflare-artifacts credentials and rerun. The hostname to forget is 8b04592ed74a5cce30d355b07276caf3.artifacts.cloudflare.net.

Or via the GUI: Keychain Access → search "artifacts.cloudflare.net" → delete the entry. Next run will prompt fresh.

If your artifacts password is itself a generated token from somewhere (some kind of API token / personal access token from your entire account), the underlying issue is that that token expired upstream, and you may need to mint a new one before the credential prompt will lead anywhere useful. The 403 body says "Invalid or expired token" — pretty unambiguous.

Once you re-auth, the more interesting question gets answered: does the new incremental-relay-with-creates path actually push your full sync to that target. That's what the previous turn's fix was about, and we'll only know after this auth issue clears.

Good news embedded in the bad news: the relay fix is working as intended. The error chain now says incremental execute (was materialized execute before) — git-sync is routing the multi-create case through streaming incremental relay instead of falling back to the materialized walker. We've shipped the path the user's case needed.

What remains is a plain server-side 500 from Cloudflare's git artifacts receive-pack. We've now ruled out:

• git-sync's materialized walker (fixed and bypassed).
• Pack-content issues (relay forwards the source's own pack — same kind that worked in your single-branch test B).
• Auth (you just authed cleanly).
• Transport (HTTP/1.1; we'd see different errors otherwise).

The diagnostic-headers patch from earlier is doing real work here — Cf-Ray=9f614186d8dfd81e-TXL. That's the actionable artifact. Hand it to whoever runs the artifacts service (or open a ticket with Cloudflare if they own it). They can look up that ray ID and see exactly what their receive-pack did before bailing with a generic 500. The body just says "Internal Server Error" because their handler didn't bother shaping a response — but the Cf-Ray gives them everything they need on the server side.

A couple last things worth checking on your end before sending it off:

1. Try --verbose in case any sideband progress slips through before the 500 — sometimes the server logs a useful message via sideband even when its HTTP response is generic:
2. Try a vanilla git push --mirror to confirm the same target rejects the same content from a different client. If vanilla git also 500s, fully a server-side problem. If vanilla git succeeds, there's still some subtle git-sync difference to hunt down — but I'd be surprised given we're now using a source-relayed pack.

For your original question — "validate sync is working" — I'd consider that answered. You've successfully demonstrated:

• Single-branch sync via bootstrap (test B). ✓
• No-op sync via incremental (test A). ✓
• Materialized walker handles transitive-missing objects correctly (the first fix). ✓
• Incremental relay handles branch creates with non-empty target (the second fix). ✓

The only remaining failure is on the artifacts server, and it's emitting a Cf-Ray for someone else to investigate. Submit the ID; this isn't ours to fix.

Useful new data. Two observations:

1. The pack is large. Total 17837 (delta 3004), reused 1841 (delta 1819), pack-reused 14290 — ~17.8K objects, partly delta-compressed, in a single push. That's a sizable receive-pack body. The artifacts service may have a body-size cap or a backend timeout that fires on packs of this size, and Cloudflare returns a generic 500 when its origin times out or rejects.

2. There are two source: Enumerating objects: 17837 sequences. That's a separate cosmetic/efficiency issue — git-sync is fetching from the source twice:

• First, FetchToStore at internal/syncer/syncer.go:497 populates the in-memory store for plan building.
• Then FetchPack inside the incremental relay path fetches the same closure again to stream to the target.

The double fetch doesn't cause your 500, but it's wasteful and worth fixing in a follow-up. (For incremental relay specifically, we could skip the local-store fetch entirely if all plans are relay-eligible.)

Try --target-max-pack-bytes

The artifacts server's pack-size limit is the most likely 500 trigger. git-sync supports auto-batching when the target rejects a too-large body, but only for the bootstrap path. For incremental relay, you'd set the cap explicitly:

(10 MiB. Drop to 1 MiB if it still fails.)

If that succeeds, we know it was a size limit. If it fails with the same shape of error at much smaller sizes, the artifacts server has a different problem.

What to share with the artifacts service operators

Two Cf-Ray IDs from this thread are particularly diagnostic:

• 9f614186d8dfd81e-TXL — earlier 500 on a similar-sized push.
• 9f6145de4e3b236c-TXL — this one.

Plus the body content (Internal Server Error) is generic — they should be able to look up their logs by Cf-Ray and see what happened on receive-pack. The pattern (large push → generic 500) is the hint they need.

Do you want me to fix the double-fetch?

For incremental-relay-only paths, the in-memory store fetch is redundant. The plan can be built from refs alone for branch creates (no ancestry walk needed) and the existing FF-update path needs a small subset. Worth a follow-up — but it's a real change to the syncer flow, not a one-line tweak. Say the word and I'll plan it out.

I was wrong about --target-max-pack-bytes helping the incremental path — it only triggers batching in bootstrap. The flag was a no-op for your run; the pack was still 17.8K objects.

Looking at the output, also note both fetches reported Total 17837 — the same 17K-object closure that took the streaming relay path is what the artifacts server is choking on. Server keeps returning a generic 500 with no useful body. Three new Cf-Ray IDs to share now (9f614186…, 9f6145de…, 9f616a23…).

What actually works to get this to succeed

Option A — Manual per-branch loop (works today, no code change). Single-branch invocations take the bootstrap path (no managed refs in scope per call) and we already verified those work for that target:

(Cache the credential helper first to avoid prompting per call.) Slow but functional. Confirms the issue is volume.

Option B — Proper code fix. Make the user's exact scenario eligible for batched bootstrap relay, which:

• Splits the closure into checkpoints sized to --target-max-pack-bytes.
• Uses target refs as haves on each batch (already implemented at bootstrap.go:225).
• Already auto-retries with auto-batching when a target rejects a too-large body (bootstrap.go:138).

The block is CanBootstrapRelay rejecting whenever any desired ref exists on target — even when that ref is already at the source's hash (a no-op). For your case, target's main is at the same hash as source's main, so the desired list contains a "match." Relaxing the check to allow already-current matches (treating them as already-bootstrapped) routes you straight into the batched path.

Two pieces have to move together:

• CanBootstrapRelay (internal/planner/relay.go:34) — only reject when target's hash for a desired ref is non-zero and differs from the desired source hash.
• BuildBootstrapPlans (internal/planner/planner.go:233) — same: skip already-current refs as ActionSkip, only emit ActionCreate for the rest.

Plus a couple of integration tests covering the "target has main at correct hash, source adds branches" case end-to-end, and a planner test for the relaxed CanBootstrapRelay.

This is the right shape of fix and it's the natural extension of what we did today. It's a self-contained change but a bigger one than the previous two — touches the bootstrap relay decision logic, plan building, and there are several integration tests around bootstrap eligibility that I'd want to read carefully before changing the rules.

Want me to implement Option B?

You're right to push back. Let me be honest about what we actually know vs. what I'm guessing.

What we actually know

• Target returns 500 with body Internal Server Error. No further detail from the server.
• Single-branch sync via bootstrap relay works (test B, one data point).
• No-op single-branch sync works (test A, irrelevant to push behavior).
• Full sync with 17837 objects in the pack fails.
• Three Cf-Ray IDs the server-side operators could look up.

What I'm guessing

• That the failure is size-related. The verbose output gives object count, not byte size. We don't know if the pack is 5 MB or 500 MB.
• That batching would fix it. If Cloudflare's failure is a deterministic backend bug unrelated to size — bad object format, an edge-case in their handler, an auth quirk — then no amount of batching helps.
• That the per-branch loop "works." I extrapolated from one branch.
• That the artifacts service has any size cap at all. Could just be a server crash that happens to correlate with this repo's content.

Cheap experiments to validate before any implementation

These three would actually tell us where the bug lives:

1. Vanilla git push --mirror to the same target. If that also 500s, it's a server bug, full stop, and Option B fixes nothing. If it succeeds, git-sync is sending something different.

2. Two branches in one git-sync invocation. Tests whether the threshold is "exactly 1 ref" or "many refs":

3. Pack byte size. Run vanilla git pack-objects locally with the same wants/haves to see how big the pack actually is. If it's <10 MB, "size limit" is implausible.

The most informative thing of all: share a Cf-Ray with whoever runs the artifacts service. They'll know in 30 seconds. We've been reverse-engineering through a 500 with no body — that's a losing game when we can just ask.

My honest recommendation

Don't implement Option B yet. The fixes already shipped (materialized walker, incremental relay for creates) are independently correct improvements. The server-side 500 is a separate problem and we haven't proven it's a git-sync problem at all. Run experiments 1 and 2, share a Cf-Ray with the server team — then decide if more code change is justified.

Committed as 9c95280 on soph/fixes. Tree is clean — ready for you to push and open the PR.

When you're back for the double-fetch follow-up: the redundant call is FetchToStore at internal/syncer/syncer.go:497, which populates the in-memory store for plan building before incremental.Execute does its own FetchPack to stream to the target. For pure relay paths (all plans relay-eligible) the local store fetch is unnecessary — plan building for branch creates and FF updates only needs ref-level data and ancestry checks, not the full closure. Skipping it would halve the source-side bandwidth on relay-only syncs.

Sure:

Three fixes for syncing into targets that share reachability with the source.

• Materialized walker tolerates pruned objects. A fetch with target refs as haves prunes the closure server-side; the walker now treats missing references (top-level and transitive) as implicitly have'd by the target instead of failing with object not found.
• Incremental relay accepts branch creates. Previously fell through to materialized when target had any existing ref and source added a new branch. Also tolerates no-thin targets — same rationale as replicate relay (the source pack is always self-contained).
• httpError surfaces diagnostic headers (Cf-Ray, Server, X-Request-Id, …) so an opaque 500 leaves something to share with the upstream operator.

Quick clarification before I dig in: the soph/prevent-double-fetch branch is currently at main (7ac0134), while the fixes you just committed for the previous PR are on soph/fixes (9c95280).

The double-fetch optimization is most meaningful layered on top of soph/fixes — without those changes, branch creates with an existing target still fall through to materialized (which legitimately needs the full store), so skipping FetchToStore would break that path. The optimization really only pays off once relay handles the create case.

Two ways to set this up:

1. Rebase soph/prevent-double-fetch on soph/fixes. Cleanest. The PR for this branch will sit on top of the previous PR. If you merge them in order, the dependency stays clean.
2. Independent of soph/fixes. Doable, but then I need to scope the fix to only kick in for plans that don't need full closure (FF updates without store-required ancestry checks via FetchCommitGraph), and we need a fallback for servers without v2+filter. More code, narrower benefit until both PRs merge.

Which do you want? My recommendation is (1) — rebase this branch on soph/fixes, fix is straightforward and meaningful, and the PR descriptions can note the dependency.

Committed as ee03e70 on soph/prevent-double-fetch. Branch is clean.

What landed

internal/syncer/syncer.go

• New helper needsLocalSourceClosure(cfg, desired, targetRefs). Returns true on Force || Prune or any divergent ref (target has it at a different hash than source).
• The FetchToStore call in runSync is now gated by that helper. When false, the in-memory store stays empty — BuildPlans doesn't touch it (no FF check needed), and incremental relay does its own FetchPack to stream straight to target.

internal/syncer/integration_test.go

• TestRun_IntegrationSkipsLocalFetchOnRelayOnlySync: target has master, source adds a new branch reachable from master. Asserts exactly 1 want across all source upload-pack POSTs. Confirmed to fail (got 2 wants) when the optimization is reverted.
• TestRun_IntegrationKeepsLocalFetchWhenAncestryNeeded: target seeded with master, source advances master by one commit (FF update). Asserts the relay still completes — exercises the path where the closure is needed for FF detection, so the local fetch must remain.

What this means for your full-sync scenario

For your cli.git → artifacts case, the previous PR's verbose output showed two Enumerating objects: 17837 sequences from source. With this change, only the relay's own fetch happens — one trip to GitHub per relay-only sync instead of two. Doesn't fix the artifacts-server 500 (still server-side), but it halves the source-side bandwidth and round-trip cost for any relay-only run.

Skip the upfront FetchToStore when incremental relay will handle the push (no force/prune, no divergent refs). The relay does its own FetchPack to stream the source pack to target, so the upfront fetch was redundant — every relay-only sync was paying for the source pack twice.

Verified by TestRun_IntegrationSkipsLocalFetchOnRelayOnlySync (asserts exactly 1 want across source upload-pack POSTs in a target-has-master / source-adds-release scenario) and TestRun_IntegrationKeepsLocalFetchWhenAncestryNeeded (FF updates still populate the store for ReachesCommit).

Depends on #N (the relay-with-existing-target fixes) — without those, branch creates fall through to materialized which legitimately needs the closure.

Summary

• relay.go — added const reasonIncrementalEligible = "fast-forward-branch-or-tag-create" (fixes goconst), changed the inner switch's default: to explicit case ActionDelete, ActionSkip, ActionBlock: (fixes exhaustive). The semantics are unchanged: any non-update/non-create action still falls through with incremental-branch-action-not-update-or-create.
• planner_test.go — five literal occurrences of the reason string replaced with the new constant. The single remaining literal is an error-message format string in t.Fatalf, which goconst doesn't flag.

Ready to amend or commit on top of 9c95280, depending on whether you want this folded into the original commit or kept separate.

The committed string is actually 40 characters, not 42 — both locally and on origin/soph/fixes:

So the review counted wrong — likely a glance error since the visual length of 40 d's is hard to eyeball.

That said, the underlying concern about plumbing.NewHash is real: it does hex.DecodeString then copy(h[:], b), which silently truncates anything > 40 chars and silently zeros odd-length input. We're correct here, but a typo in a future copy-paste could go undetected. If you want belt-and-suspenders, a tiny test helper would catch it:

Worth doing? Up to you — for two test fixtures it might be more ceremony than the risk warrants. Reply with the call and I'll either drop the helper into planner_test.go and refactor the four hash literals, or just reply to the review noting the count was wrong.

Committed as c827f33.

Quick summary of the fix:

• Replaced the single if needsLocalSourceClosure(...) { FetchToStore(...) } block with a closureFetched flag and a fetchClosure() closure that fetches at most once.
• Called fetchClosure() upfront when needsLocalSourceClosure says we need it (existing behavior preserved).
• Also called fetchClosure() right before executeMaterialized runs in the relay-rejected branch, so an unanticipated relay rejection (e.g. CapabilitiesKnown=false) doesn't leave materialized with an empty store.

For the test gap: I considered adding an integration test specifically for "needsLocalSourceClosure=false but CanIncrementalRelay rejects." The only realistic trigger is CapabilitiesKnown=false, which requires advertising receive-pack with a nil Capabilities field — neither receivePackThinCap nor receivePackNoThin express that, and rewriteReceivePackAdvertisement mutates an existing list rather than nilling it. Building that scaffolding seemed disproportionate to the size of the fix, especially since the fix itself is straightforward (lazy + idempotent). If you'd like, I can add a server flag and a test in a small follow-up — say the word.

Looking deeper: the CapabilitiesKnown=false branch is essentially unreachable from a real server. packp.NewAdvRefs() initializes Capabilities to a non-nil capability.List{}, and Decode doesn't replace it. So adv.Capabilities is non-nil after any successful parse, and Known: true is set whenever the advertisement parses at all.

Which means the reviewer's "the fallback runs against an empty store" concern is theoretical for the specific path they named — but the defensive lazy-fetch is still right because:

• It's forward-compatible. If CanIncrementalRelay ever grows a new rejection that isn't mirrored in needsLocalSourceClosure, the fallback still works.
• It costs zero in the happy path (the closure is fetched at most once).

That said, here's what a flag would look like if you wanted the test:

Two problems with this approach:

1. It doesn't actually produce nil capabilities — emptying a non-nil capability.List still leaves it non-nil, so TargetFeaturesFromAdvRefs would still set Known: true. We'd need caps == nil, but rewriteReceivePackAdvertisement's mutate signature takes a non-nil pointer.

2. The wire format always carries a capability segment in v1 (after the NUL on the first ref line, or via the synthetic capabilities^{} ref when there are no refs). go-git's encoder writes it whether the list has entries or not. To produce a wire response that decodes with Capabilities == nil, we'd have to bypass the encoder entirely — write raw pktline bytes that omit the NUL-prefixed capability segment. That's a fair amount of test code to assert on a never-actually-triggered branch.

So the realistic answer is: don't add the test. The fix stands as a defense-in-depth change against a future hypothetical failure (someone adds a new CanIncrementalRelay rejection that needsLocalSourceClosure doesn't anticipate). If you want to surface that intent, I could either:

• Add a comment to needsLocalSourceClosure noting it's a best-effort approximation and the materialized fallback compensates, or
• Add a small in-package unit test that constructs a syncSession with a synthetic RelayTargetPolicy{CapabilitiesKnown: false} and asserts the lazy fetch fires. That bypasses the network entirely. ~40-60 lines of test boilerplate for one assertion.

Either is fine. My lean is the comment — the existing commit message already documents the intent; a one-line comment near the helper makes it discoverable from the code too.

Committed as 9e08903. The test:

• Constructs a syncSession directly via newSession (we're in package syncer).
• Synthesizes the otherwise-unreachable CapabilitiesKnown=false policy by overriding sess.target.policy after construction.
• Sets up source with a new commit on a new branch that target genuinely doesn't have.
• Asserts both the ref update and that the commit object actually landed on target — the latter is the regression-catching assertion (the in-memory storer happily records refs pointing at missing objects, so the ref-only check passes either way).

Confirmed it fails when the lazy-fetch is reverted (object not found for the new commit), passes with the fix.

Test still passes, lint clean. Restructured: read baselineHead first, do all source state changes (add commit, set release, reset master), then call newSession once.