From 0e3e543144a710c25a8c40d8d7d069abafaf9213 Mon Sep 17 00:00:00 2001 From: Maxwell Elliott Date: Mon, 6 Jul 2026 12:12:10 -0400 Subject: [PATCH] fix(serve): resolve missing revisions via on-demand refetch 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 `. 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 ` 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 ^{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 '' 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 --- .../kotlin/com/bazel_diff/server/GitClient.kt | 34 +++++- .../server/ImpactedTargetsService.kt | 40 ++++++- .../com/bazel_diff/server/JGitClient.kt | 23 +++- .../com/bazel_diff/server/GitClientTest.kt | 10 ++ .../server/ImpactedTargetsServiceTest.kt | 101 ++++++++++++++++++ .../com/bazel_diff/server/JGitClientTest.kt | 56 +++++++++- 6 files changed, 248 insertions(+), 16 deletions(-) diff --git a/cli/src/main/kotlin/com/bazel_diff/server/GitClient.kt b/cli/src/main/kotlin/com/bazel_diff/server/GitClient.kt index b0ba83d..9dd3280 100644 --- a/cli/src/main/kotlin/com/bazel_diff/server/GitClient.kt +++ b/cli/src/main/kotlin/com/bazel_diff/server/GitClient.kt @@ -10,9 +10,21 @@ import org.koin.core.component.KoinComponent import org.koin.core.component.inject /** Thrown when a `git` subprocess exits non-zero or produces unusable output. */ -class GitClientException(message: String, cause: Throwable? = null) : +open class GitClientException(message: String, cause: Throwable? = null) : RuntimeException(message, cause) +/** + * Thrown when a revision cannot be found in the local clone -- either it did not resolve at all, or + * it resolved (a full 40-char SHA always parses to an ObjectId) but the underlying commit object is + * absent from the object database. + * + * The query service treats this as *retryable*: it only fetches at startup, so a commit that landed + * on the remote afterwards is simply not here yet. Callers refetch and retry once before surfacing + * the failure (see [com.bazel_diff.server.ImpactedTargetsService]). + */ +class MissingRevisionException(val revision: String, cause: Throwable? = null) : + GitClientException("revision '$revision' is missing from the local clone", cause) + /** * Thin wrapper over a `git` binary (assumed on `$PATH`) scoped to a single workspace clone. The * query service checks out the workspace at the revisions it is asked about; see @@ -25,8 +37,10 @@ interface GitClient { fun fetch() /** - * Resolves a revision (branch, tag, short or full SHA) to a full 40-char commit SHA. Throws - * [GitClientException] if the revision cannot be resolved. + * Resolves a revision (branch, tag, short or full SHA) to a full 40-char commit SHA, verifying + * that the commit actually exists in the local clone. Throws [MissingRevisionException] if the + * revision does not resolve or its object is absent (retryable via [fetch]); throws + * [GitClientException] for other failures (e.g. the revision names a non-commit object). */ fun resolveSha(revision: String): String @@ -50,9 +64,19 @@ class ProcessGitClient( } override fun resolveSha(revision: String): String { - val output = run("rev-parse", revision) + // `rev-parse --verify ^{commit}` both resolves the revision to a commit SHA and confirms + // that commit is present in the local clone. A bare `rev-parse ` echoes any + // well-formed 40-char SHA straight back without consulting the object database, so a commit + // that isn't here would "resolve" and only fail later at checkout. A non-zero exit means the + // revision is absent (or not a commit) locally -- retryable via a refetch. + val output = + try { + run("rev-parse", "--verify", "$revision^{commit}") + } catch (e: GitClientException) { + throw MissingRevisionException(revision, e) + } return output.firstOrNull()?.trim()?.takeIf { it.isNotEmpty() } - ?: throw GitClientException("git rev-parse $revision produced no output") + ?: throw MissingRevisionException(revision) } override fun checkout(revision: String) { diff --git a/cli/src/main/kotlin/com/bazel_diff/server/ImpactedTargetsService.kt b/cli/src/main/kotlin/com/bazel_diff/server/ImpactedTargetsService.kt index 0e79895..e9ee43a 100644 --- a/cli/src/main/kotlin/com/bazel_diff/server/ImpactedTargetsService.kt +++ b/cli/src/main/kotlin/com/bazel_diff/server/ImpactedTargetsService.kt @@ -72,13 +72,17 @@ class ImpactedTargetsService( private val bazelModService: BazelModService by inject() private val logger: Logger by inject() + // Serializes on-demand refetches so a burst of requests for a just-landed + // commit triggers at most one `git fetch` at a time (and skips it entirely + // once an earlier fetch has already brought the commit in). + private val fetchLock = Any() + override fun getImpactedTargets( fromRev: String, toRev: String, targetTypes: Set? ): ImpactedTargetsResult { - val fromSha = gitClient.resolveSha(fromRev) - val toSha = gitClient.resolveSha(toRev) + val (fromSha, toSha) = resolveBoth(fromRev, toRev) logger.i { "Computing impacted targets $fromSha -> $toSha" } val fromData = hashProvider.getHashes(fromSha) @@ -110,8 +114,7 @@ class ImpactedTargetsService( throw DistancesUnavailableException( "distances unavailable: server started without --trackDeps") } - val fromSha = gitClient.resolveSha(fromRev) - val toSha = gitClient.resolveSha(toRev) + val (fromSha, toSha) = resolveBoth(fromRev, toRev) logger.i { "Computing impacted targets with distances $fromSha -> $toSha" } val fromData = hashProvider.getHashes(fromSha) @@ -134,6 +137,35 @@ class ImpactedTargetsService( return ImpactedTargetsWithDistancesResult(fromSha, toSha, impacted) } + /** + * Resolves both revisions to commit SHAs, refetching once if either is missing from the local + * clone. The service only fetches at startup, so a commit that landed on the remote afterwards + * (or a branch this clone has not yet seen) would otherwise fail with a stale-clone + * [MissingRevisionException]; a single on-demand `git fetch` brings it in. + * + * Fetches are serialized and double-checked through [fetchLock]: concurrent requests for the same + * new commit wait on the lock, and whoever holds it re-resolves first, so an earlier fetch that + * already made the revision resolvable saves the rest a redundant fetch. A revision still missing + * after the refetch is genuinely unknown (bad SHA, or a ref this clone's refspec doesn't fetch) + * and propagates as [MissingRevisionException] -> HTTP 400. + */ + private fun resolveBoth(fromRev: String, toRev: String): Pair { + try { + return gitClient.resolveSha(fromRev) to gitClient.resolveSha(toRev) + } catch (missing: MissingRevisionException) { + logger.i { "Revision '${missing.revision}' not in local clone; refetching and retrying" } + } + synchronized(fetchLock) { + // A concurrent request may have refetched while we waited on the lock; retry before fetching. + try { + return gitClient.resolveSha(fromRev) to gitClient.resolveSha(toRev) + } catch (stillMissing: MissingRevisionException) { + gitClient.fetch() + } + } + return gitClient.resolveSha(fromRev) to gitClient.resolveSha(toRev) + } + // Synthetic //external:* labels are not buildable in bzlmod-only mode; mirror the // get-impacted-targets default of excluding them when Bzlmod is enabled (issue #326). private fun excludeExternalTargets(): Boolean = bazelModService.isBzlmodEnabled diff --git a/cli/src/main/kotlin/com/bazel_diff/server/JGitClient.kt b/cli/src/main/kotlin/com/bazel_diff/server/JGitClient.kt index fb02002..095e8f5 100644 --- a/cli/src/main/kotlin/com/bazel_diff/server/JGitClient.kt +++ b/cli/src/main/kotlin/com/bazel_diff/server/JGitClient.kt @@ -3,6 +3,9 @@ package com.bazel_diff.server import com.bazel_diff.log.Logger import java.nio.file.Path import org.eclipse.jgit.api.Git +import org.eclipse.jgit.errors.IncorrectObjectTypeException +import org.eclipse.jgit.errors.MissingObjectException +import org.eclipse.jgit.revwalk.RevWalk import org.eclipse.jgit.storage.file.FileRepositoryBuilder import org.eclipse.jgit.transport.RemoteConfig import org.koin.core.component.KoinComponent @@ -53,10 +56,22 @@ class JGitClient(private val workspacePath: Path) : GitClient, KoinComponent { override fun resolveSha(revision: String): String { open().use { git -> - val objectId = - git.repository.resolve(revision) - ?: throw GitClientException("could not resolve revision '$revision'") - return objectId.name + val objectId = git.repository.resolve(revision) ?: throw MissingRevisionException(revision) + // Repository.resolve() turns a full 40-char SHA into an ObjectId by parsing the + // hex WITHOUT consulting the object database, so a commit absent from this clone + // still "resolves". Parse it through a RevWalk to confirm the object is present + // (peeling annotated tags to their commit), so an absent commit becomes a + // retryable MissingRevisionException here instead of an opaque "Missing unknown + // " failure deferred to the later checkout. + val commit = + try { + RevWalk(git.repository).use { walk -> walk.parseCommit(objectId) } + } catch (e: MissingObjectException) { + throw MissingRevisionException(revision, e) + } catch (e: IncorrectObjectTypeException) { + throw GitClientException("revision '$revision' does not refer to a commit", e) + } + return commit.name } } diff --git a/cli/src/test/kotlin/com/bazel_diff/server/GitClientTest.kt b/cli/src/test/kotlin/com/bazel_diff/server/GitClientTest.kt index a82ffea..3b3954c 100644 --- a/cli/src/test/kotlin/com/bazel_diff/server/GitClientTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/server/GitClientTest.kt @@ -78,6 +78,16 @@ class GitClientTest : KoinTest { assertThrows(GitClientException::class.java) { client.resolveSha("no-such-ref") } } + @Test + fun resolveShaThrowsMissingRevisionForAbsentFullSha() { + initRepoWithTwoCommits() + val client = ProcessGitClient(temp.root.toPath()) + // `git rev-parse ` echoes any well-formed SHA back even when absent; resolveSha must + // use `--verify` so a commit that isn't in the clone is rejected as a retryable miss. + val absent = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" + assertThrows(MissingRevisionException::class.java) { client.resolveSha(absent) } + } + @Test fun fetchDoesNotThrowWithoutRemotes() { initRepoWithTwoCommits() diff --git a/cli/src/test/kotlin/com/bazel_diff/server/ImpactedTargetsServiceTest.kt b/cli/src/test/kotlin/com/bazel_diff/server/ImpactedTargetsServiceTest.kt index dbeb329..97b8691 100644 --- a/cli/src/test/kotlin/com/bazel_diff/server/ImpactedTargetsServiceTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/server/ImpactedTargetsServiceTest.kt @@ -45,6 +45,60 @@ class ImpactedTargetsServiceTest : KoinTest { override fun checkout(revision: String) = Unit } + /** [GitClient] whose [missingUntilFetch] revisions resolve only after [fetch] has run once. */ + private class RefetchGitClient(private val missingUntilFetch: Set) : GitClient { + var fetchCount = 0 + private var fetched = false + + override fun fetch() { + fetchCount++ + fetched = true + } + + override fun resolveSha(revision: String): String { + if (!fetched && revision in missingUntilFetch) throw MissingRevisionException(revision) + return revision + } + + override fun checkout(revision: String) = Unit + } + + /** + * [GitClient] that fails the first resolve of [flaky] and succeeds on the next WITHOUT a fetch -- + * models another request having refetched while this one waited on the fetch lock. + */ + private class ResolvesOnSecondAttemptGitClient(private val flaky: String) : GitClient { + var fetchCount = 0 + private var attempts = 0 + + override fun fetch() { + fetchCount++ + } + + override fun resolveSha(revision: String): String { + if (revision == flaky && ++attempts == 1) throw MissingRevisionException(revision) + return revision + } + + override fun checkout(revision: String) = Unit + } + + /** [GitClient] whose [missing] revisions never resolve, even after a fetch. */ + private class AlwaysMissingGitClient(private val missing: Set) : GitClient { + var fetchCount = 0 + + override fun fetch() { + fetchCount++ + } + + override fun resolveSha(revision: String): String { + if (revision in missing) throw MissingRevisionException(revision) + return revision + } + + override fun checkout(revision: String) = Unit + } + /** * [HashProvider] returning canned data. By default the module-change workspace path is treated as * an error (the pure hash-diff path must not touch the workspace); set [allowWorkspaceAt] to @@ -196,4 +250,51 @@ class ImpactedTargetsServiceTest : KoinTest { assertThat(result.impactedTargets).containsExactly(ImpactedTargetWithDistance("//:a", 0, 0)) assertThat(provider.workspaceAtCalls).isEqualTo(listOf("to-sha")) } + + @Test + fun refetchesOnceWhenRevisionMissingThenResolves() { + // The `to` commit isn't in the local clone yet (it landed after the last fetch); the service + // must fetch on demand and retry rather than surfacing the miss as a 400. + val from = HashFileData(mapOf("//:a" to TargetHash("Rule", "h1", "d1")), null) + val to = HashFileData(mapOf("//:a" to TargetHash("Rule", "h2", "d2")), null) + val git = RefetchGitClient(missingUntilFetch = setOf("to-sha")) + val service = + ImpactedTargetsService(git, FakeHashProvider(mapOf("from-sha" to from, "to-sha" to to))) + + val result = service.getImpactedTargets("from-sha", "to-sha", null) + + assertThat(result.impactedTargets).isEqualTo(listOf("//:a")) + assertThat(git.fetchCount).isEqualTo(1) + } + + @Test + fun skipsRefetchWhenRevisionAppearsUnderTheLock() { + // A concurrent request already refetched: the double-check re-resolve under the lock succeeds, + // so this request must not issue its own redundant fetch. + val from = HashFileData(mapOf("//:a" to TargetHash("Rule", "h1", "d1")), null) + val to = HashFileData(mapOf("//:a" to TargetHash("Rule", "h2", "d2")), null) + val git = ResolvesOnSecondAttemptGitClient(flaky = "to-sha") + val service = + ImpactedTargetsService(git, FakeHashProvider(mapOf("from-sha" to from, "to-sha" to to))) + + val result = service.getImpactedTargets("from-sha", "to-sha", null) + + assertThat(result.impactedTargets).isEqualTo(listOf("//:a")) + assertThat(git.fetchCount).isEqualTo(0) + } + + @Test + fun propagatesMissingRevisionAfterRefetchStillFails() { + // A genuinely unknown revision: one refetch is attempted, then the miss propagates (the HTTP + // layer maps it to a 400). + val git = AlwaysMissingGitClient(missing = setOf("to-sha")) + val service = ImpactedTargetsService(git, FakeHashProvider(emptyMap())) + + val ex = + org.junit.Assert.assertThrows(MissingRevisionException::class.java) { + service.getImpactedTargets("from-sha", "to-sha", null) + } + assertThat(ex.revision).isEqualTo("to-sha") + assertThat(git.fetchCount).isEqualTo(1) + } } diff --git a/cli/src/test/kotlin/com/bazel_diff/server/JGitClientTest.kt b/cli/src/test/kotlin/com/bazel_diff/server/JGitClientTest.kt index d52dead..538fa74 100644 --- a/cli/src/test/kotlin/com/bazel_diff/server/JGitClientTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/server/JGitClientTest.kt @@ -4,6 +4,7 @@ import assertk.assertThat import assertk.assertions.hasLength import assertk.assertions.isEqualTo import assertk.assertions.isNotEqualTo +import assertk.assertions.messageContains import com.bazel_diff.SilentLogger import com.bazel_diff.log.Logger import java.io.File @@ -26,9 +27,11 @@ class JGitClientTest : KoinTest { @get:Rule val temp: TemporaryFolder = TemporaryFolder() /** Runs git for test setup, returning trimmed stdout; fails the test on a non-zero exit. */ - private fun git(vararg args: String): String { - val proc = - ProcessBuilder(listOf("git") + args).directory(temp.root).redirectErrorStream(true).start() + private fun git(vararg args: String): String = runGit(temp.root, *args) + + /** Like [git] but in an explicit working directory (for multi-repo fetch tests). */ + private fun runGit(dir: File, vararg args: String): String { + val proc = ProcessBuilder(listOf("git") + args).directory(dir).redirectErrorStream(true).start() val output = proc.inputStream.readBytes().decodeToString() val code = proc.waitFor() check(code == 0) { "git ${args.joinToString(" ")} failed ($code): $output" } @@ -109,4 +112,51 @@ class JGitClientTest : KoinTest { initRepoWithTwoCommits() assertThrows(GitClientException::class.java) { client().checkout("does-not-exist") } } + + @Test + fun resolveShaThrowsMissingRevisionForAbsentFullSha() { + initRepoWithTwoCommits() + // A well-formed 40-char SHA that is not in the object database. Repository.resolve() parses the + // hex without a DB lookup, so resolveSha must detect the absence itself and flag it retryable. + val absent = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" + assertThrows(MissingRevisionException::class.java) { client().resolveSha(absent) } + } + + @Test + fun resolveShaThrowsWhenRevisionIsNotACommit() { + initRepoWithTwoCommits() + // A tree object is present but is not a commit; resolveSha must reject it as a hard error, not + // as a retryable MissingRevisionException (a refetch would never make a tree checkoutable). + val treeSha = git("rev-parse", "HEAD^{tree}") + val ex = assertThrows(GitClientException::class.java) { client().resolveSha(treeSha) } + assertThat(ex).messageContains("does not refer to a commit") + } + + @Test + fun resolveShaSeesCommitOnlyAfterFetch() { + // Reproduces the reported serve failure: a commit that lands on the remote AFTER the clone is + // absent locally, so resolveSha rejects it (rather than deferring an opaque checkout error) + // until an on-demand fetch brings it in. + val origin = File(temp.root, "origin").apply { mkdirs() } + runGit(origin, "init", "-q") + runGit(origin, "config", "user.email", "test@example.com") + runGit(origin, "config", "user.name", "test") + File(origin, "file.txt").writeText("one") + runGit(origin, "add", ".") + runGit(origin, "commit", "-q", "-m", "first") + + val workspace = File(temp.root, "workspace") + runGit(temp.root, "clone", "-q", origin.absolutePath, workspace.absolutePath) + + // A new commit lands in origin after the clone; the workspace has not fetched it yet. + File(origin, "file.txt").writeText("two") + runGit(origin, "add", ".") + runGit(origin, "commit", "-q", "-m", "second") + val sha2 = runGit(origin, "rev-parse", "HEAD") + + val client = JGitClient(workspace.toPath()) + assertThrows(MissingRevisionException::class.java) { client.resolveSha(sha2) } + client.fetch() + assertThat(client.resolveSha(sha2)).isEqualTo(sha2) + } }