Skip to content

serve: integration harness + fixes for --trackDeps over-reporting and JGit fetch failures#413

Merged
tinder-maxwellelliott merged 3 commits into
masterfrom
claude/strange-noether-c1a97a
Jul 7, 2026
Merged

serve: integration harness + fixes for --trackDeps over-reporting and JGit fetch failures#413
tinder-maxwellelliott merged 3 commits into
masterfrom
claude/strange-noether-c1a97a

Conversation

@tinder-maxwellelliott

Copy link
Copy Markdown
Collaborator

Summary

Hardens the bazel-diff serve query service. Adds a standalone integration harness that exercises serve end to end (which surfaced a real correctness bug), plus two fixes:

  1. --trackDeps over-reportingserve --trackDeps returned inflated /impacted_targets sets on cache hits.
  2. JGit fetch robustness — the on-demand refetch added in fix(serve): resolve missing revisions via on-demand refetch #411 fails on shallow/partial clones with Missing delta base; it now falls back to native git.

The --trackDeps bug (commit 1)

TargetHash is a data class whose generated equals() includes the deps field, but TargetHash.fromJson (used to read cache entries) only parses type#hash~directHash and always leaves deps = null. So a cache-miss to (freshly generated, deps populated under --trackDeps) compared against a cache-hit from (deserialized, deps = null) reported every dep-carrying target as changed even when its hash was identical:

revisions correct before this PR
C1→C2 (both fresh) {core, core.txt, mid}
C2→C3 (from cached) {core, core.txt, mid} + other
C2→C4 (from cached) {other, other.txt} + core, mid

Anyone running serve --trackDeps (required for /impacted_targets_with_distances) got over-broad impacted sets on cache hits. The CLI never hit it (both sides deserialized → symmetric). Fixed by comparing hash identity (type#hash~directHash), not the full TargetHash, in computeSimpleImpactedTargets and computeAllDistances.

JGit fetch fallback (commit 2)

JGit 5.13 can't fetch shallow (--depth) or partial (--filter=blob:none) clones whose thin packs are delta-compressed against absent objects (Missing delta base <sha>) — the class of the reported production error. JGitClient.fetch() now retries with the native git binary (at --gitPath) on any in-process fetch failure, and warns once at startup if the clone is shallow/partial. JGit stays the default for resolve/checkout; --gitEngine=subprocess still skips the in-process attempt.

Repro caveat: I could not reproduce Missing delta base locally — JGit 5.13.3 negotiates shallow/partial fetches cleanly against a cooperative localhost git daemon; the real failure needs a real remote's repack state. The fix is therefore defensive-but-correct (it's exactly what --gitEngine=subprocess already does for the whole flow), and the harness's shallow/partial cases are engine-parity gates the fallback keeps green.

Harness + CI (commit 3)

tools/serve_harness.py drives the real binary over HTTP against a live git:// remote (git daemon) across both git engines × full/shallow/partial clones + the on-demand refetch path — coverage E2ETest#testServeEndToEnd (a single local --no-initial-fetch repo) can't reach. Run python3 tools/serve_harness.py (--only, --skip-build, --keep-artifacts, -v, --bazel).

Wired into CI as .github/workflows/serve-harness.yml, cron-only for now (12h schedule + workflow_dispatch) while it proves stable on runners; promote to push/PR gating later (one-line change, documented at the top of the workflow). Intentionally not a py_binary — a nested bazel run would deadlock on the output-base lock.

Testing

  • Harness: 50/50 across the full matrix, via the exact CI invocation (--bazel).
  • New unit tests: CalculateImpactedTargetsInteractorTest (deps-asymmetry, both endpoints), JGitClientTest (fallback-disabled surfaces the JGit error; both-fail names the native fallback; shallow-clone fetch).
  • Full //cli suite green except 4 E2ETest sub-tests that need a system JDK for their nested local_jdk toolchain (environment-only, unrelated to this change) — testServeEndToEnd and the core get-impacted-targets tests pass.

🤖 Generated with Claude Code

tinder-maxwellelliott and others added 3 commits July 7, 2026 10:36
… 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>
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>
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>
@tinder-maxwellelliott tinder-maxwellelliott marked this pull request as ready for review July 7, 2026 14:57
@tinder-maxwellelliott tinder-maxwellelliott merged commit 986c75e into master Jul 7, 2026
28 of 29 checks passed
@tinder-maxwellelliott tinder-maxwellelliott deleted the claude/strange-noether-c1a97a branch July 7, 2026 16:44
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