Skip to content

fix(serve): resolve missing revisions via on-demand refetch#411

Merged
tinder-maxwellelliott merged 1 commit into
masterfrom
claude/mystifying-gates-f7abcf
Jul 6, 2026
Merged

fix(serve): resolve missing revisions via on-demand refetch#411
tinder-maxwellelliott merged 1 commit into
masterfrom
claude/mystifying-gates-f7abcf

Conversation

@tinder-maxwellelliott

Copy link
Copy Markdown
Collaborator

Problem

The bazel-diff serve query service returned an opaque 400 for commits it should have been able to serve:

Error: bazel-diff server returned 400: { "error": "git error: JGit checkout 9ea41ff… failed: Missing unknown 9ea41ff…" }

Two compounding causes:

  1. resolveSha didn't verify the commit exists. JGit's Repository.resolve("<40-hex>") parses the hex into an ObjectId without an object-DB lookup, and git rev-parse <full-sha> echoes any well-formed SHA straight back. So a full SHA that isn't in the local clone "resolved" successfully, and the real failure (MissingObjectException) was deferred to the later checkout — surfacing as a confusing checkout error.
  2. The service only fetched once, at startup. Any commit that landed on the remote afterwards (or lived on a ref the default refspec doesn't cover) could never be served without a restart.

Fix

  • Validate object existence in resolveSha for both git engines, raising a new retryable MissingRevisionException:
    • JGit: RevWalk.parseCommit(objectId) (also peels annotated tags to their commit).
    • Subprocess: rev-parse --verify <rev>^{commit} instead of a bare rev-parse.
  • On-demand refetch + retry in ImpactedTargetsService.resolveBoth: if either revision is missing, run one git fetch and retry. Serialized and double-checked via a fetchLock, so a burst of requests for the same just-landed commit triggers at most one fetch.

Resulting behavior

Scenario Before After
Commit landed after startup 400 … Missing unknown … Fetched on demand and served
Genuinely unknown SHA / uncovered ref 400 (opaque) 400 after one refetch, clear message: revision '<x>' is missing from the local clone
Named ref at a stale-but-present tip unchanged unchanged (fetch-once staleness is a separate concern, out of scope)

The --gitEngine=subprocess path had the same latent bug and is fixed the same way.

Tests

All green via bazel test:

  • JGitClientTest — absent full SHA → MissingRevisionException; end-to-end repro (resolveShaSeesCommitOnlyAfterFetch: a commit lands in an origin after clone, is rejected locally, then resolves post-fetch()); non-commit object → hard error.
  • GitClientTest (subprocess) — absent full SHA → MissingRevisionException.
  • ImpactedTargetsServiceTest — refetch-then-succeeds; double-check skips the redundant fetch; persistent miss propagates after exactly one fetch.

Ran //cli:{GitClientTest,JGitClientTest,ImpactedTargetsServiceTest,BazelDiffServerTest,HashServiceTest,ServeCommandTest} (6/6 pass) and applied //cli/format (ktfmt).

Notes for reviewers

🤖 Generated with Claude Code

The serve query service only fetched at startup, so a request for a commit
that landed afterwards (or on a ref the default refspec doesn't cover) failed
with an opaque `400 git error: ... Missing unknown <sha>`. Two compounding
causes:

- resolveSha reported success for a full 40-char SHA that isn't in the local
  clone: JGit's Repository.resolve() parses the hex without an object-DB lookup,
  and `git rev-parse <full-sha>` echoes it straight back. The miss therefore
  surfaced later at checkout instead of at resolution.
- The service never refetched, so a just-landed commit could not be served
  without a restart.

Fixes:
- Validate object existence in resolveSha for both git engines (JGit
  RevWalk.parseCommit; subprocess `rev-parse --verify <rev>^{commit}`), raising
  a new retryable MissingRevisionException when the commit is absent.
- ImpactedTargetsService performs one on-demand `git fetch` + retry when a
  revision is missing, serialized and double-checked via a lock so concurrent
  requests for the same new commit fetch at most once.

Genuinely-unknown revisions still return 400, now with a clear
"revision '<x>' is missing from the local clone" message.

Adds unit + integration coverage across both git engines and the service retry
paths, including an end-to-end JGit repro of a commit that only resolves after
a fetch.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@tinder-maxwellelliott tinder-maxwellelliott marked this pull request as ready for review July 6, 2026 16:15
@tinder-maxwellelliott tinder-maxwellelliott merged commit 56408d6 into master Jul 6, 2026
15 checks passed
@tinder-maxwellelliott tinder-maxwellelliott deleted the claude/mystifying-gates-f7abcf branch July 6, 2026 16:42
tinder-maxwellelliott added a commit that referenced this pull request Jul 7, 2026
… JGit fetch failures (#413)

* fix(serve): stop --trackDeps over-reporting impacted targets on cache hits

The serve query service returned inflated /impacted_targets sets whenever it was
started with --trackDeps and served a `from` revision from the per-SHA hash cache.

TargetHash is a data class whose generated equals() includes the `deps` field, but
TargetHash.fromJson (used to read cache/JSON entries) only parses
type#hash~directHash and always leaves deps=null. A cache-miss `to` (freshly
generated, deps populated under --trackDeps) compared against a cache-hit `from`
(deserialized, deps=null) therefore reported every dep-carrying target as changed
even when its hash was identical. C1->C2 (both freshly generated) was correct;
C2->C3 / C2->C4 (with `from` served from cache) were not. The CLI never hit this
because both sides are deserialized, hence symmetric.

Fix: compare targets by their hash identity (type#hash~directHash) rather than the
full TargetHash in computeSimpleImpactedTargets and computeAllDistances -- `deps` is
auxiliary data used only to derive distance metrics, not part of a target's
impactedness. Adds regression tests for both the impacted and distance paths.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* feat(serve): fall back to native git when a JGit fetch fails

JGit 5.13 cannot fetch some clone shapes native git handles fine -- notably shallow
(--depth) and partial (--filter=blob:none) clones, whose thin packs are
delta-compressed against objects absent from the clone ("Missing delta base <sha>").
With the on-demand refetch added in #411, such a fetch now surfaces to callers as a
400.

JGitClient.fetch() now retries a failed in-process fetch with the native `git` binary
(at --gitPath) before giving up, and logs a one-time warning at startup if the
workspace is a shallow or partial clone. JGit stays the default for resolve/checkout.
If native git also fails, both causes are combined so neither is hidden.
--gitEngine=subprocess still skips the in-process attempt entirely.

Adds JGitClientTest coverage (fallback-disabled surfaces the JGit error; both-fail
names the native fallback; a shallow clone can still fetch a just-landed commit) and
documents the behavior in the serve README section.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test(serve): add serve integration harness + cron CI workflow

Adds tools/serve_harness.py, a standalone integration harness that exercises
`bazel-diff serve` end to end against a live git:// remote (git daemon) across both
git engines x full/shallow/partial clones and the on-demand refetch path -- coverage
the in-process E2ETest cannot reach. It builds //cli:bazel-diff, drives the real
binary over HTTP, and asserts on the deterministic rule+source impacted subset
(generated-file targets are filtered as noisy). This harness is what surfaced the
--trackDeps over-reporting bug fixed earlier in this branch.

Wired into CI as .github/workflows/serve-harness.yml, cron-only for now (12h schedule
+ workflow_dispatch) until it proves stable on runners; promote to push/PR gating
afterward. The harness gained a --bazel flag so CI can point it at bazelisk. It is
intentionally not a py_binary (a nested `bazel run` would deadlock on the output-base
lock).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant