Fix/checkout robustness - #6
Merged
Merged
Conversation
Three standard-mode (non-openvox) failures hit on a repo's first sync: - An empty upstream (no commits yet) aborted the whole sync with "remote repository is empty". It is now a benign no-op success that keeps polling until commits are pushed. - When `checkout` is unset the working tree was never populated; it now defaults to the remote HEAD (default) branch, skipping gracefully if that branch isn't mirrored locally. - An incomplete local object store (a ref resolves but its tree/blobs are missing, e.g. an interrupted prior fetch on a persistent volume) made checkout fail with "object not found". go-git treats the present-but-partial commit as a "have" and won't repair it in place, so SyncRepo now wipes the local mirror and rebuilds once. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012fn7LgEVQzxhwZq6Ff3BnK
- Recover the first-sync case where the shared cache holds refs but has a dangling HEAD (the upstream default branch isn't "master"): fall back to init+alternates instead of failing every per-branch clone with "reference not found". - Don't take the up-to-date fast path when a ref resolves but its objects are missing locally; verify object presence so the branch/tag is refetched instead of failing checkout with "object not found". - Treat "object not found" as a recoverable error so the existing recreate-and-retry path self-heals it. - Treat an empty upstream as a benign no-op success. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012fn7LgEVQzxhwZq6Ff3BnK
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012fn7LgEVQzxhwZq6Ff3BnK
Rishikesh01
marked this pull request as draft
September 1, 2026 05:06
Bump the go directive in go.mod, the Dockerfile builder image, and the docs to Go 1.27. CI reads the version from go.mod (go-version-file), so the workflows need no change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012fn7LgEVQzxhwZq6Ff3BnK
Surfaced once golangci-lint could parse Go 1.27: - errcheck: check previously-ignored error returns in the new test helpers (SetReference, CreateRemote, WriteFile, Add, Worktree). - goconst: use the existing MainBranch constant instead of repeated "main" string literals. - revive: drop the unused receiver on recordSyncSuccess. No behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012fn7LgEVQzxhwZq6Ff3BnK
golangci-lint v2.12.2 cannot decode Go 1.27 export data ("export data
version 4 is greater than maximum supported version 2"), failing the
lint job after the toolchain bump. v2.13.2 is built with go1.27.0.
Updated in both the GitHub and Gitea workflows.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012fn7LgEVQzxhwZq6Ff3BnK
…anch
Branch selection could only be expressed as patterns, so a caller that
does not know the default branch name up front had no way to ask for it.
The choice was to guess ("main", "master") and fail when the guess was
wrong, or widen to "*" and mirror every branch in the repo.
gfetch already reads the default branch from the remote's advertised HEAD
symref; it just could not act on it. default_branch_only makes that
available: the branch patterns are ignored and the only branch synced is
the one HEAD points at, whatever it is called.
Branches and tags become optional when it is set, since the option
selects the branch by itself. It is rejected together with openvox, which
exists to sync every matched branch.
extractRemoteRefState now resolves HEAD in its own pass before matching,
because a server may advertise HEAD after the branch refs and the
selection depends on knowing it.
default_branch_only takes the branch from the remote HEAD, so branches and checkout do not merely duplicate it, they contradict it. Silently ignoring them leaves an operator staring at config that has no effect. Reject all three combinations instead: branches, checkout, and openvox, which exists to sync every matched branch. Tags stay allowed. They are a separate axis and syncing the default branch plus a set of tags is a coherent thing to ask for. Inherited branch patterns are skipped rather than rejected. A defaults block naming branches would otherwise fail every default_branch_only repo under it, for config the operator never wrote that way. The default_branch_only merge moves ahead of the pattern merges so that check can see it.
Three defects found in review of the default_branch_only work. default_branch_only leaves repo.Branches empty by design, so matching obsolete branches against it marked every local branch obsolete, including the one just synced. With prune enabled that branch was then deleted while the sync still reported success. The guard in pruneBranches could not prevent it: it keys on repo.Checkout, which validation forbids in exactly this mode. Match against the branches actually selected instead. The implicit checkout path returned a warning without setting result.Err, so for any repo with no checkout configured - which is every repo the API writes - result.Err stayed nil and SyncRepo's object-repair retry could never fire. A partial object store reported healthy. Only skip silently when the ref really is absent locally; if it is present the checkout failed for a real reason. The old warning text was also wrong: the ref was present, its objects were not. The inherited-branches test asserted against a Config it built itself and called Validate, which never runs applyDefaults - so it passed with the guard deleted and proved nothing. It now goes through Load, and fails if the guard is removed. Also hoist two test constants that broke goconst under the pinned linter.
Two ways a repo could get permanently stuck, both found on live syncs. go-git cannot fetch objects from Azure DevOps. The fetch returns nil, the refs are written, and the object store is left completely empty - no pack, no loose objects, no error. Checkout then fails with "object not found" and every later poll repeats it. Reproduced outside the cluster against ebillet's repo with all four shapes go-git offers: narrow refspec, wildcard refspec, and both PlainClone variants. All wrote refs and zero objects, while the git binary fetched 645. v5.19.2 is the latest v5, so there is nothing to upgrade to. Shell out to git for those hosts. Note this also means the object-repair retry could never have helped such a repo: there is nothing partial to rebuild, so wiping and refetching just produces another empty store. Separately, a truncated .git/index made checkout fail forever with "malformed index signature file" on gitea-hosted repos. go-git reads the index before touching the worktree, so once it is corrupt nothing can proceed - and the gfetch container runs without a memory limit, so being killed mid-write is expected rather than exotic. The index is pure cache rebuildable from HEAD, so delete it and retry the checkout instead of re-cloning. The runtime image gains git and openssh-client, which the exec path needs.
The Azure DevOps workaround only covered the branch fetch, so any other call that pulls objects still went through go-git and would still come back empty. Tags are the obvious one - they carry objects like any other ref - and the staleness checks fetch commits to read their dates. execFetch is now generic over flags and refspecs, with execFetchBranch and execFetchTags as thin wrappers, and the tag and staleness paths pick the git binary on the same host check. The openvox resolver's staleness fetch passes a nil config on purpose: it fetches into the shared resolver repo at cachePath rather than repo.LocalPath, so pointing the exec path at it would run git in the wrong directory. openvox keeps using go-git there.
ensureCloned stat'd repo.LocalPath, which succeeds for any directory that happens to exist - one the shared-PVC init container created, or one a failed earlier sync left behind. PlainOpen then ran against a path holding no repository, and because that branch returns early it also skipped CreateRemote, so the init path that configures "origin" never ran. Stat .git instead, so a directory without a repository in it takes the init path as intended. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CthHiNTaGktfu1N1coRDb6
Root cause of the empty object stores, and it is not host-specific after all. go-git ships with MultiACK, MultiACKDetailed and ThinPack all declared unsupported, so it never advertises them. Azure DevOps answers a client offering no multi_ack with an empty pack rather than an error: the fetch returns nil, the refs are written, and no objects arrive. Checkout then fails with "object not found" and nothing self-heals, because the refs already point at the right commits. Measured against ebillet's repo: stock capabilities produced zero objects across all eleven call shapes we tried, while re-enabling multi_ack produced the same 172K pack the git binary sends. This replaces the exec workaround from the previous commits, which is now deleted along with the host list, the tag and staleness plumbing it needed, and the git/openssh-client packages in the runtime image. One assignment covers every remote instead of special-casing four hostnames. ThinPack stays unsupported: unlike the other two that reflects a real gap in go-git's pack handling rather than a negotiation preference. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CthHiNTaGktfu1N1coRDb6
The option is named for what it does, so tags belong with branches and checkout in the rejected set rather than being a separate axis you can still opt into. Reverses the earlier call to allow them. Inherited tags are skipped rather than rejected, matching the treatment of inherited branches: a defaults block naming tags must not fail every default_branch_only repo underneath it for config the operator never wrote. This changes nothing for the configs the API writes - they already set no tags - but it stops a repo half-using the option and quietly syncing tags nobody asked for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CthHiNTaGktfu1N1coRDb6
The multi_ack assignment was an init() in its own file, which mutated a go-git global as a side effect of importing the package. Move it into Syncer.New so it happens when a syncer is actually constructed; both the daemon and the CLI go through there. Record the upgrade constraint in AGENTS.md under "Known future work": go-git v6 removes transport.UnsupportedCapabilities, so this will not compile against it, and the fetch, clone and capability plumbing all need reworking rather than a version bump. The Azure DevOps behaviour has to be re-verified against whatever replaces it. Kept out of the code as a literal TODO because godox rejects those; the repo already tracks this kind of thing in AGENTS.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CthHiNTaGktfu1N1coRDb6
writeKnownHostsFile was split out of buildKnownHostsAuth so the temp file
could be shared with the git-exec fetch path. That path is gone - the
Azure DevOps empty-pack problem is handled by the multi_ack capabilities
instead - so the helper has had exactly one caller since, while its doc
comment still advertised "the exec path, which hands the path to ssh as
UserKnownHostsFile".
Inlining it restores the defer os.Remove and drops the hand-threaded
cleanup func, including the two error returns that handed back a
do-nothing func(){}. It also puts buildKnownHostsAuth's own doc comment
back on buildKnownHostsAuth: the split had stranded it above
writeKnownHostsFile, leaving that function with two stacked comments and
the exported behaviour undocumented.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H1xm6zw8ancQ9bcLwA1xp9
Four cleanups to the standard sync path, no behaviour change: recordSyncSuccess was a copy of SyncRepo's own success tail - observe duration, logSyncSuccess, SyncSuccessTotal, LastSuccessTimestamp. The empty-remote path always has a nil result.Err, so it now falls through to that shared tail and the helper is gone. The object-repair retry moves into repairIncompleteObjectStore, which returns early when the wipe fails instead of hanging the retry off an else. SyncRepo no longer carries the repair detail inline. handleCheckout gets the implicit case resolved up front. It used to check out the default branch unconditionally, then classify the failure by re-resolving the ref a third time to decide whether it was benign - so a repo whose patterns exclude HEAD paid a doomed checkout on every sync. Resolving the ref first drops handleImplicitCheckoutErr and the implicit flag entirely, along with an "if implicit" nested under a condition that was unconditionally true whenever implicit was set. findStaleBranches keeps taking repo.Branches rather than the patterns synthesized for the obsolete check. Under default_branch_only those are the branch just synced, and stale pruning would delete it for being old - the very deletion the obsolete path was fixed to prevent. Noted in a comment, since the asymmetry looks like an oversight otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H1xm6zw8ancQ9bcLwA1xp9
The .git/index repair was added to checkoutRef, but openvox calls checkoutRefContext directly for both branches and tags, so it never got the repair. That is backwards: openvox keeps N worktrees per repo and checks them out from concurrent workers, so it is the mode most exposed to a container killed mid-index-write - and isRecoverableOpenVoxRepoError does not list the malformed-index error either, leaving such a branch dir permanently fatal with no retry. Move the detect-remove-retry into checkoutRefContext, which is the one function that decodes the index, and add checkoutRefOnce for the raw attempt. checkoutRef stays as the background-context wrapper. Every caller now gets the repair, including any future one. The test asserts the precondition against checkoutRefOnce and the repair against checkoutRefContext, so it pins the behaviour at the layer openvox actually uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H1xm6zw8ancQ9bcLwA1xp9
Error identification stopped relying on hand-written message text. Every
match now sources its string from the go-git error value, so the two
cannot drift: plumbing.ErrObjectNotFound, dotgit.ErrEmptyRefFile,
index.ErrMalformedSignature, transport.ErrEmptyRemoteRepository and
plumbing.ErrReferenceNotFound. The "couldn't find remote ref" match
became errors.Is(err, git.NoMatchingRefSpecError{}), which that type
supports via its Is method, so no string is involved at all.
isRecoverableOpenVoxRepoError also gained the errors.Is arm for
ErrEmptyRefFile, which it never had, and now calls isObjectNotFoundErr
rather than keeping a second, weaker copy of the same test. The text
fallbacks stay: only ErrObjectNotFound is confirmed to survive wrapping,
and dotgit returns ErrEmptyRefFile bare from readReferenceFrom.
objectPresentLocally is gone. It guarded the openvox fast path against a
ref that resolves while its objects are missing, but it only checked the
tip object - and decoding a commit succeeds while its tree is missing,
which is exactly the shape an interrupted fetch leaves behind. Verified:
with the tree object deleted it still reported present, so the fetch was
skipped on the one case it existed for, while charging every healthy ref
an object decode per poll (and, through alternates, a rebuilt DotGit plus
a decode of every pack index in the shared cache). That state produces
"object not found" at checkout, which isRecoverableOpenVoxRepoError
already classifies, so the ref dir is recreated and re-synced either way.
The cost now lands on the broken ref instead of all the healthy ones.
isBranchUpToDateLocal and isTagUpToDateLocal were identical apart from
the ref constructor, so they fold into isRefUpToDateLocal. The tests
cover both predicate arms - wrapped sentinel and message text - with the
expected text taken from the sentinels, and TestIsRefUpToDateLocal pins
the hash-only contract that replaces the probe.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H1xm6zw8ancQ9bcLwA1xp9
TestCheckoutRef_RepairsCorruptIndex hand-rolled init + worktree + write + add + commit, which is what initTestRepoWithCommitAtPath already does - the helper takes a path precisely so the caller can poke at .git afterwards. It also invented its own author instead of the DefaultTestName/DefaultTestEmail constants every other test uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H1xm6zw8ancQ9bcLwA1xp9
The option was added without documentation in either the README or the configuration reference the README points to. The reference gains a field-table row, a Default Branch Only section, and two validation rules; the branches/tags requirement is relaxed to note it does not apply in this mode. The section covers what the option is for, that HEAD is re-resolved on every sync so a changed upstream default is followed, the four combinations that are rejected and why, the defaults-inheritance carve-out, and how prune and prune_stale behave. The README gains a feature bullet, the option in the defaults example, a repo using it, and a note that it replaces branches/tags rather than supplementing them. Behaviour was checked against the binary rather than read off the validation code: all four rejections produce the documented errors, a config with neither branches nor tags validates, and gfetch cat confirms a default_branch_only repo resolves to empty branches/tags while a sibling repo inherits from defaults. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H1xm6zw8ancQ9bcLwA1xp9
VILJkid
approved these changes
Sep 4, 2026
VILJkid
left a comment
VILJkid
left a comment
Member
There was a problem hiding this comment.
This is noice!
This is tested against Azure DevOps mechanism.
Let's test this against other Git hosting providers as well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.