From fd24bd18238c64f9b9e470a5cfd81fff0e5862e5 Mon Sep 17 00:00:00 2001 From: Daniel Gollahon Date: Mon, 27 Jul 2026 14:50:52 -0700 Subject: [PATCH] Scope macro digest cache to each hash invocation The query service reuses RuleHasher across base and head checkouts. Its path-only .bzl cache could therefore reuse a base digest at head and hide macro-only changes from impacted targets. Make the cache invocation-local while retaining within-run parallel deduplication. Add unit coverage for cache lifetime and a two-commit query-service regression. --- .../com/bazel_diff/hash/BuildGraphHasher.kt | 10 ++- .../bazel_diff/hash/HashInvocationContext.kt | 23 +++++ .../kotlin/com/bazel_diff/hash/RuleHasher.kt | 29 +++--- .../com/bazel_diff/hash/TargetHasher.kt | 9 +- .../test/kotlin/com/bazel_diff/e2e/E2ETest.kt | 89 +++++++++++++++---- .../bazel_diff/hash/BuildGraphHasherTest.kt | 49 +++++++++- .../bazel_diff/hash/FakeSourceFileHasher.kt | 6 +- .../hash/RuleHasherAlwaysAffectedTagsTest.kt | 3 +- .../workspaces/serve_bzl_cache/BUILD | 3 + .../workspaces/serve_bzl_cache/MODULE.bazel | 4 + .../workspaces/serve_bzl_cache/README.md | 14 +++ .../workspaces/serve_bzl_cache/WORKSPACE | 1 + .../workspaces/serve_bzl_cache/defs.bzl | 6 ++ 13 files changed, 205 insertions(+), 41 deletions(-) create mode 100644 cli/src/main/kotlin/com/bazel_diff/hash/HashInvocationContext.kt create mode 100644 cli/src/test/resources/workspaces/serve_bzl_cache/BUILD create mode 100644 cli/src/test/resources/workspaces/serve_bzl_cache/MODULE.bazel create mode 100644 cli/src/test/resources/workspaces/serve_bzl_cache/README.md create mode 100644 cli/src/test/resources/workspaces/serve_bzl_cache/WORKSPACE create mode 100644 cli/src/test/resources/workspaces/serve_bzl_cache/defs.bzl diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt b/cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt index bc5bc77b..3b0182f8 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt @@ -44,6 +44,7 @@ class BuildGraphHasher(private val bazelClient: BazelClient) : KoinComponent { modifiedFilepaths: Set = emptySet(), timings: HasherPhaseTimings? = null ): Map { + val hashInvocationContext = HashInvocationContext() val (sourceDigests, allTargets) = runBlocking { val queryStartNanos = System.nanoTime() @@ -85,7 +86,8 @@ class BuildGraphHasher(private val bazelClient: BazelClient) : KoinComponent { sourceDigests, allTargets, ignoredAttrs, - modifiedFilepaths) + modifiedFilepaths, + hashInvocationContext) timings?.targetHashMillis = (System.nanoTime() - targetHashStartNanos) / 1_000_000 return result } @@ -132,7 +134,8 @@ class BuildGraphHasher(private val bazelClient: BazelClient) : KoinComponent { sourceDigests: ConcurrentMap, allTargets: List, ignoredAttrs: Set, - modifiedFilepaths: Set + modifiedFilepaths: Set, + hashInvocationContext: HashInvocationContext ): Map { val ruleHashes: ConcurrentMap = ConcurrentHashMap() val targetToRule: MutableMap = HashMap() @@ -150,7 +153,8 @@ class BuildGraphHasher(private val bazelClient: BazelClient) : KoinComponent { seedHash, packageBzlSeeds, ignoredAttrs, - modifiedFilepaths) + modifiedFilepaths, + hashInvocationContext) Pair( target.name, TargetHash( diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/HashInvocationContext.kt b/cli/src/main/kotlin/com/bazel_diff/hash/HashInvocationContext.kt new file mode 100644 index 00000000..5564fc8d --- /dev/null +++ b/cli/src/main/kotlin/com/bazel_diff/hash/HashInvocationContext.kt @@ -0,0 +1,23 @@ +package com.bazel_diff.hash + +import java.util.concurrent.ConcurrentHashMap + +/** + * Mutable state that may be shared while one build graph is hashed. + * + * Hashers are application singletons and may outlive a workspace checkout. Revision-derived data + * must therefore live here instead of on a hasher. The concurrent map preserves digest + * deduplication while targets are hashed in parallel. + */ +class HashInvocationContext { + private val bzlDigests = ConcurrentHashMap() + + fun bzlDigest(path: String, calculate: () -> ByteArray?): ByteArray? { + val digest = bzlDigests.computeIfAbsent(path) { calculate() ?: MISSING_DIGEST } + return digest.takeUnless { it === MISSING_DIGEST } + } + + private companion object { + val MISSING_DIGEST = ByteArray(0) + } +} diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt b/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt index ff0c034a..77c0ca67 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt @@ -6,7 +6,6 @@ import com.bazel_diff.bazel.decodeConfiguredRuleInputLabel import com.bazel_diff.log.Logger import com.google.common.annotations.VisibleForTesting import java.nio.file.Path -import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.ConcurrentMap import org.koin.core.component.KoinComponent import org.koin.core.component.inject @@ -21,10 +20,6 @@ class RuleHasher( private val logger: Logger by inject() private val sourceFileHasher: SourceFileHasher by inject() - // main-repo `.bzl` path -> content digest; EMPTY marks a non-main-repo/missing file (skip). - private val bzlDigestCache = ConcurrentHashMap() - private val EMPTY = ByteArray(0) - /** * Per-rule `.bzl` seed: digests of the main-repo `.bzl` files in this rule's macro instantiation * stack, so a macro edit re-hashes only the rules that macro produced (issue #365). @@ -33,7 +28,11 @@ class RuleHasher( * off) so the caller falls back to the package seed; a macro-less rule returns a stable constant, * so the caller does NOT fall back. */ - private fun ruleBzlSeed(rule: BazelRule, modifiedFilepaths: Set): ByteArray? { + private fun ruleBzlSeed( + rule: BazelRule, + modifiedFilepaths: Set, + hashInvocationContext: HashInvocationContext + ): ByteArray? { val stack = rule.instantiationStack if (stack.isEmpty()) return null val bzlPaths = sortedSetOf() @@ -49,11 +48,11 @@ class RuleHasher( return sha256 { for (path in bzlPaths) { val digest = - bzlDigestCache.computeIfAbsent(path) { + hashInvocationContext.bzlDigest(path) { sourceFileHasher.softDigest( - BazelSourceFileTarget("//$it", ByteArray(0)), modifiedFilepaths) ?: EMPTY + BazelSourceFileTarget("//$path", ByteArray(0)), modifiedFilepaths) } - if (digest.isNotEmpty()) { + if (digest != null) { safePutBytes(path.toByteArray()) safePutBytes(digest) } @@ -84,7 +83,8 @@ class RuleHasher( packageBzlSeeds: Map, depPath: LinkedHashSet?, ignoredAttrs: Set, - modifiedFilepaths: Set + modifiedFilepaths: Set, + hashInvocationContext: HashInvocationContext ): TargetDigest { val depPathClone = if (depPath != null) LinkedHashSet(depPath) else LinkedHashSet() if (depPathClone.contains(rule.name)) { @@ -111,7 +111,8 @@ class RuleHasher( // Per-rule macro seed (see ruleBzlSeed), else the package-wide fallback. Each rule // resolves its own seed, so this is stable under the memoized recursion below (#365). putDirectBytes( - ruleBzlSeed(rule, modifiedFilepaths) ?: packageBzlSeeds[labelToPackage(rule.name)]) + ruleBzlSeed(rule, modifiedFilepaths, hashInvocationContext) + ?: packageBzlSeeds[labelToPackage(rule.name)]) // Mixed into the *direct* digest (not transitively) so the tagged target is classified // as DIRECT-impacted for distance metrics; it still bubbles into the overall digest, so // any rdeps are conservatively re-hashed too. @@ -144,7 +145,8 @@ class RuleHasher( packageBzlSeeds, depPathClone, ignoredAttrs, - modifiedFilepaths) + modifiedFilepaths, + hashInvocationContext) putTransitiveBytes(inputLabel, ruleInputHash.overallDigest) } else -> { @@ -192,7 +194,8 @@ class RuleHasher( packageBzlSeeds, depPathClone, ignoredAttrs, - modifiedFilepaths) + modifiedFilepaths, + hashInvocationContext) putTransitiveBytes(packageGroupLabel, packageGroupHash.overallDigest) } } diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/TargetHasher.kt b/cli/src/main/kotlin/com/bazel_diff/hash/TargetHasher.kt index 6cce4c47..c4544715 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/TargetHasher.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/TargetHasher.kt @@ -18,7 +18,8 @@ class TargetHasher : KoinComponent { seedHash: ByteArray?, packageBzlSeeds: Map, ignoredAttrs: Set, - modifiedFilepaths: Set + modifiedFilepaths: Set, + hashInvocationContext: HashInvocationContext ): TargetDigest { return when (target) { is BazelTarget.GeneratedFile -> { @@ -41,7 +42,8 @@ class TargetHasher : KoinComponent { packageBzlSeeds, depPath = null, ignoredAttrs, - modifiedFilepaths) + modifiedFilepaths, + hashInvocationContext) } // Add the generating rule name as a dep of the generated file. @@ -58,7 +60,8 @@ class TargetHasher : KoinComponent { packageBzlSeeds, depPath = null, ignoredAttrs, - modifiedFilepaths) + modifiedFilepaths, + hashInvocationContext) } is BazelTarget.SourceFile -> { // Mix in the `.bzl` seed for this source file's own package only, so a macro edit in diff --git a/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt b/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt index f2cb98a2..a385aa21 100644 --- a/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt @@ -1,6 +1,7 @@ package com.bazel_diff.e2e import assertk.assertThat +import assertk.assertions.contains import assertk.assertions.isEqualTo import com.bazel_diff.cli.BazelDiff import com.google.gson.Gson @@ -166,26 +167,16 @@ class E2ETest { @Test fun testServeEndToEnd() { val workspace = copyTestWorkspace("distance_metrics") - fun git(vararg args: String): String { - val proc = - ProcessBuilder(listOf("git") + args) - .directory(workspace) - .redirectErrorStream(true) - .start() - val output = proc.inputStream.readBytes().decodeToString() - check(proc.waitFor() == 0) { "git ${args.joinToString(" ")} failed: $output" } - return output.trim() - } - git("init", "-q") - git("config", "user.email", "test@example.com") - git("config", "user.name", "test") - git("add", "-A") - git("commit", "-q", "-m", "base") - val fromSha = git("rev-parse", "HEAD") + git(workspace, "init", "-q") + git(workspace, "config", "user.email", "test@example.com") + git(workspace, "config", "user.name", "test") + git(workspace, "add", "-A") + git(workspace, "commit", "-q", "-m", "base") + val fromSha = git(workspace, "rev-parse", "HEAD") File(workspace, "lib.sh").writeText("echo changed\n") - git("add", "-A") - git("commit", "-q", "-m", "change lib.sh") - val toSha = git("rev-parse", "HEAD") + git(workspace, "add", "-A") + git(workspace, "commit", "-q", "-m", "change lib.sh") + val toSha = git(workspace, "rev-parse", "HEAD") val cacheDir = temp.newFolder() val port = java.net.ServerSocket(0).use { it.localPort } @@ -247,6 +238,66 @@ class E2ETest { } } + @Test + fun testServeDoesNotReuseMacroDigestAcrossRevisions() { + val workspace = copyTestWorkspace("serve_bzl_cache") + git(workspace, "init", "-q") + git(workspace, "config", "user.email", "test@example.com") + git(workspace, "config", "user.name", "test") + git(workspace, "add", "-A") + git(workspace, "commit", "-q", "-m", "base") + val fromSha = git(workspace, "rev-parse", "HEAD") + + File(workspace, "defs.bzl").appendText("\n# second revision\n") + git(workspace, "add", "-A") + git(workspace, "commit", "-q", "-m", "change macro source") + val toSha = git(workspace, "rev-parse", "HEAD") + + val cacheDir = temp.newFolder() + val port = java.net.ServerSocket(0).use { it.localPort } + val serveThread = + Thread { + CommandLine(BazelDiff()) + .execute( + "serve", + "-w", + workspace.absolutePath, + "-b", + "bazel", + "--cacheDir", + cacheDir.absolutePath, + "--port", + port.toString(), + "--no-initial-fetch") + } + .apply { + isDaemon = true + start() + } + + try { + awaitServeHealthy(port) + val (code, body) = + httpGetServe("http://localhost:$port/impacted_targets?from=$fromSha&to=$toSha") + assertThat(code).isEqualTo(200) + val parsed: Map = + Gson().fromJson(body, object : TypeToken>() {}.type) + @Suppress("UNCHECKED_CAST") val impacted = parsed["impactedTargets"] as List + assertThat(impacted.map { it.removePrefix("@@") }).contains("//:generated") + } finally { + serveThread.interrupt() + serveThread.join(10_000) + } + } + + private fun git(workspace: File, vararg args: String): String { + val proc = + ProcessBuilder(listOf("git") + args).directory(workspace).redirectErrorStream(true).start() + val output = proc.inputStream.readBytes().decodeToString() + check(proc.waitFor() == 0) { "git ${args.joinToString(" ")} failed: $output" } + return output.trim() + } + /** Polls `/health` until it returns 200, up to ~30s, failing the test otherwise. */ private fun awaitServeHealthy(port: Int) { repeat(60) { diff --git a/cli/src/test/kotlin/com/bazel_diff/hash/BuildGraphHasherTest.kt b/cli/src/test/kotlin/com/bazel_diff/hash/BuildGraphHasherTest.kt index 90db9571..654f372e 100644 --- a/cli/src/test/kotlin/com/bazel_diff/hash/BuildGraphHasherTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/hash/BuildGraphHasherTest.kt @@ -60,6 +60,8 @@ class BuildGraphHasherTest : KoinTest { add(createRuleTarget(name = "rule2", inputs = ArrayList(), digest = "rule2Digest")) } Mockito.reset(bazelClientMock) + fakeSourceFileHasher.softDigestValue = "fake-soft-digest".toByteArray() + fakeSourceFileHasher.softDigestCalls.set(0) } @Test @@ -344,16 +346,61 @@ class BuildGraphHasherTest : KoinTest { "rule3" to listOf("rule0"), "rule0" to listOf("rule1"), "rule1" to emptyList()) } + @Test + fun testMacroDigestIsScopedToOneHashInvocation() = runBlocking { + val target = + createRuleTarget( + name = "rule1", + inputs = emptyList(), + digest = "rule1Digest", + instantiationStack = listOf("defs/macro.bzl:1:1: macro")) + whenever(bazelClientMock.queryAllTargets()).thenReturn(listOf(target)) + + fakeSourceFileHasher.softDigestValue = "v1".toByteArray() + val first = hasher.hashAllBazelTargetsAndSourcefiles() + fakeSourceFileHasher.softDigestValue = "v2".toByteArray() + val second = hasher.hashAllBazelTargetsAndSourcefiles() + fakeSourceFileHasher.softDigestValue = "v1".toByteArray() + val third = hasher.hashAllBazelTargetsAndSourcefiles() + + assertThat(second["rule1"]).isNotEqualTo(first["rule1"]) + assertThat(third["rule1"]).isEqualTo(first["rule1"]) + } + + @Test + fun testMacroDigestIsReusedWithinHashInvocation() = runBlocking { + val instantiationStack = listOf("defs/macro.bzl:1:1: macro") + val first = + createRuleTarget( + name = "rule1", + inputs = emptyList(), + digest = "rule1Digest", + instantiationStack = instantiationStack) + val second = + createRuleTarget( + name = "rule2", + inputs = emptyList(), + digest = "rule2Digest", + instantiationStack = instantiationStack) + whenever(bazelClientMock.queryAllTargets()).thenReturn(listOf(first, second)) + + hasher.hashAllBazelTargetsAndSourcefiles() + + assertThat(fakeSourceFileHasher.softDigestCalls.get()).isEqualTo(1) + } + private fun createRuleTarget( name: String, inputs: List, - digest: String + digest: String, + instantiationStack: List = emptyList() ): BazelTarget.Rule { val target = mock() val rule = mock() whenever(rule.name).thenReturn(name) whenever(rule.ruleInputList(false, emptySet())).thenReturn(inputs) whenever(rule.digest(emptySet())).thenReturn(digest.toByteArray()) + whenever(rule.instantiationStack).thenReturn(instantiationStack) whenever(target.rule).thenReturn(rule) whenever(target.name).thenReturn(name) return target diff --git a/cli/src/test/kotlin/com/bazel_diff/hash/FakeSourceFileHasher.kt b/cli/src/test/kotlin/com/bazel_diff/hash/FakeSourceFileHasher.kt index 382b98e5..09838e9b 100644 --- a/cli/src/test/kotlin/com/bazel_diff/hash/FakeSourceFileHasher.kt +++ b/cli/src/test/kotlin/com/bazel_diff/hash/FakeSourceFileHasher.kt @@ -2,9 +2,12 @@ package com.bazel_diff.hash import com.bazel_diff.bazel.BazelSourceFileTarget import java.nio.file.Path +import java.util.concurrent.atomic.AtomicInteger class FakeSourceFileHasher : SourceFileHasher { var fakeDigests: MutableMap = mutableMapOf() + var softDigestValue: ByteArray? = "fake-soft-digest".toByteArray() + val softDigestCalls = AtomicInteger() override fun digest( sourceFileTarget: BazelSourceFileTarget, @@ -18,7 +21,8 @@ class FakeSourceFileHasher : SourceFileHasher { sourceFileTarget: BazelSourceFileTarget, modifiedFilepaths: Set ): ByteArray? { - return "fake-soft-digest".toByteArray() + softDigestCalls.incrementAndGet() + return softDigestValue } fun add(name: String, digest: ByteArray) { diff --git a/cli/src/test/kotlin/com/bazel_diff/hash/RuleHasherAlwaysAffectedTagsTest.kt b/cli/src/test/kotlin/com/bazel_diff/hash/RuleHasherAlwaysAffectedTagsTest.kt index ac2822fb..3de7df69 100644 --- a/cli/src/test/kotlin/com/bazel_diff/hash/RuleHasherAlwaysAffectedTagsTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/hash/RuleHasherAlwaysAffectedTagsTest.kt @@ -58,7 +58,8 @@ class RuleHasherAlwaysAffectedTagsTest : KoinTest { emptyMap(), null, emptySet(), - emptySet()) + emptySet(), + HashInvocationContext()) } @Test diff --git a/cli/src/test/resources/workspaces/serve_bzl_cache/BUILD b/cli/src/test/resources/workspaces/serve_bzl_cache/BUILD new file mode 100644 index 00000000..9f7e64cc --- /dev/null +++ b/cli/src/test/resources/workspaces/serve_bzl_cache/BUILD @@ -0,0 +1,3 @@ +load(":defs.bzl", "make_generated") + +make_generated(name = "generated") diff --git a/cli/src/test/resources/workspaces/serve_bzl_cache/MODULE.bazel b/cli/src/test/resources/workspaces/serve_bzl_cache/MODULE.bazel new file mode 100644 index 00000000..9a07071c --- /dev/null +++ b/cli/src/test/resources/workspaces/serve_bzl_cache/MODULE.bazel @@ -0,0 +1,4 @@ +module( + name = "serve_bzl_cache", + version = "0.0.0", +) diff --git a/cli/src/test/resources/workspaces/serve_bzl_cache/README.md b/cli/src/test/resources/workspaces/serve_bzl_cache/README.md new file mode 100644 index 00000000..81a62809 --- /dev/null +++ b/cli/src/test/resources/workspaces/serve_bzl_cache/README.md @@ -0,0 +1,14 @@ +# Query-service `.bzl` digest-cache reproducer + +This workspace reproduces a false negative in the long-running query service. +It has one native rule that a Starlark macro creates. + +The regression test creates two Git revisions. The second revision changes only +the source of `defs.bzl`. It then asks one query-service process to compare the +two revisions with an empty disk cache. + +The generated rule must be impacted because bazel-diff uses the macro +instantiation stack as part of that rule's identity. In v38.0.0, the service +keeps the first revision's `.bzl` digest in a process-wide `RuleHasher` cache. +The second revision therefore reuses the first digest and omits +`//:generated`. diff --git a/cli/src/test/resources/workspaces/serve_bzl_cache/WORKSPACE b/cli/src/test/resources/workspaces/serve_bzl_cache/WORKSPACE new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/cli/src/test/resources/workspaces/serve_bzl_cache/WORKSPACE @@ -0,0 +1 @@ + diff --git a/cli/src/test/resources/workspaces/serve_bzl_cache/defs.bzl b/cli/src/test/resources/workspaces/serve_bzl_cache/defs.bzl new file mode 100644 index 00000000..3ca2bf51 --- /dev/null +++ b/cli/src/test/resources/workspaces/serve_bzl_cache/defs.bzl @@ -0,0 +1,6 @@ +def make_generated(name): + native.genrule( + name = name, + outs = [name + ".txt"], + cmd = "echo generated > $@", + )