Skip to content

test: add cross-instance hash-consistency harness for bazel-diff serve - #447

Open
tinder-maxwellelliott wants to merge 1 commit into
masterfrom
claude/bazel-diff-hash-consistency-33a988
Open

test: add cross-instance hash-consistency harness for bazel-diff serve#447
tinder-maxwellelliott wants to merge 1 commit into
masterfrom
claude/bazel-diff-hash-consistency-33a988

Conversation

@tinder-maxwellelliott

Copy link
Copy Markdown
Collaborator

Why

Nothing today can catch a serve fleet disagreeing about a commit's hashes. E2ETest#testGenerateHashesIsHermeticAcrossWorkspacePaths runs two hashes in one JVM on one machine, and tools/serve_harness.py drives a single instance. If target hashes carry host-specific state — absolute paths, output_base, environment — replicas return different impacted targets for the same commit and nothing notices.

What

tools/serve_consistency.py runs N serve instances against one mock S3 cache tier and compares what they each wrote.

The S3 tier is the right observation point: HashService.generate persists the full {label: hash} map per revision, so a failure names which targets diverged rather than just reporting that the impacted sets differed. It also gives S3HashCacheStorage / TieredHashCacheStorage their first live-wire coverage — until now they were unit-tested against a mocked S3Client only.

Each instance writes under its own --s3Prefix so writes stay attributable. --s3Prefix is absent from computeConfigFingerprint(), so the instances still agree on the cache key. The mock's read mode decides visibility: isolated (everyone generates independently → the poisoning detector) or shared (the real fleet topology).

Skew is injected only outside the config fingerprint — clone paths of differing length, hence differing Bazel output bases, and optionally HOME/TMPDIR/USER. Skewing a fingerprinted flag would split the fleet across cache keys and leave nothing to compare.

Cases

case asserts
baseline instances generate independently; payloads must agree
shared followers must be served by the leader's objects over S3 — the mock's read log proves it was the remote tier, since a local-disk hit also reports cacheHit: true
concurrent racing writers may both generate (per-JVM lock, by design) but must not disagree
negative-control one instance seeded differently must be detected — inverted polarity, so a green run means something
version-skew opt-in --bazel-alt; the Bazel version is absent from the fingerprint, so two versions share a cache key by construction
real opt-in; the same comparison against a real repository

Details that took some finding

  • PutObject arrives aws-chunked. The SDK pins CHUNK_ENCODING_ENABLED for PutObject and the default CRC32 trailer turns framing on, so a naive body read records framed bytes rather than JSON — every comparison downstream would be garbage. mock_s3.decode_aws_chunked handles the signed and unsigned spellings. Observed on every write in practice.
  • --s3Region is mandatory. The region resolves eagerly in createS3Storage() with no try/catch, so omitting it kills the process at startup. Credentials are separately mandatory because S3HashCacheStorage swallows every SdkException into a silent miss — which is also why the harness asserts the mock actually received writes. Without that guard a wholly disconnected fleet would report "no divergence".
  • Byte-identity is the wrong gate. HashService.serialize gsons a HashMap built by Collectors.toMap over a parallel stream, so JSON key order tracks the host's CPU count. Two hosts can legitimately emit the same hashes in a different order. The comparator is semantic; key-order-only differences pass with a note.

serve_harness.py

Gains defaulted extra_args / env / port / bazel kwargs plus a reserve_ports() helper (free_port() closes its socket before returning, which races when a fleet allocates N ports in a loop). Existing behaviour is unchanged — verified by a full serve_harness.py run — and serve_stress.py benefits too.

Verification

  • Hermetic matrix: 47 pass / 0 fail (also green with --env-skew).
  • Against this repository at b7ee642..a4c50ac: three instances on three distinct output bases produced byte-identical payloads for all 319 targets, and identical impacted sets (141 stable targets).
  • bazel test //tools:mock_s3_test //tools:serve_consistency_test — 60+ unit tests over the chunked decoder, the mock's live HTTP surface, and the comparator.
  • python3 tools/serve_harness.py — 41 pass / 0 fail, no regression.

CI

Cron-only (30 3 * * *, clear of every existing schedule) plus workflow_dispatch, matching serve-harness.yml's precedent: three concurrent Bazel servers is too heavy to gate every PR on until the harness has a track record. Note that scheduled workflows only run from the default branch, so the cron starts firing once this merges.

Follow-up found while running this (not fixed here)

--fineGrainedHashExternalRepos fails hard on this repository under bzlmod:

bazel-diff generate-hashes -w . --fineGrainedHashExternalRepos @bazel_skylib out.json
# ERROR: No repository visible as '@bazel_lib' from main repository
# java.lang.RuntimeException: Bazel query failed, exit code 7

expandFineGrainedHashExternalReposWithBzlmodDependents (cli/src/main/kotlin/com/bazel_diff/di/Modules.kt) expands the user's repo set to every transitive dependent and appends each as @<apparentName> — but bazel mod graph apparent names are the names a dependent module uses, which need not be visible from the root module. graph.rootApparentNames is already passed into that function, so filtering by it looks like the fix. Happy to send that separately.

🤖 Generated with Claude Code

Nothing today can catch a `serve` fleet disagreeing about a commit's hashes.
`E2ETest#testGenerateHashesIsHermeticAcrossWorkspacePaths` runs two hashes in
one JVM on one machine, and `serve_harness.py` drives a single instance. If
target hashes carry host-specific state, replicas return different impacted
targets and nothing notices.

`tools/serve_consistency.py` runs N instances against one mock S3 cache tier
and compares what they each wrote. The S3 tier is the observation point
because `HashService.generate` persists the full `{label: hash}` map per
revision, so a failure names the diverging targets rather than just reporting
that the impacted sets differed. It also gives `S3HashCacheStorage` /
`TieredHashCacheStorage` their first live-wire coverage.

Each instance writes under its own `--s3Prefix` so writes stay attributable;
`--s3Prefix` is absent from `computeConfigFingerprint()`, so they still agree
on the cache key. Skew is injected only outside that fingerprint (clone paths
of differing length, hence differing Bazel output bases; optionally
HOME/TMPDIR/USER) -- skewing a fingerprinted flag would split the fleet across
cache keys and leave nothing to compare.

Cases: baseline (independent generation, payloads must agree), shared
(followers must be served by the leader's objects over S3), concurrent
(racing writers must not disagree), negative-control (one instance seeded
differently *must* be detected, so a green run means something), plus opt-in
version-skew and real-repository modes.

Details that took some finding:

- PutObject arrives aws-chunked. The SDK pins CHUNK_ENCODING_ENABLED for
  PutObject and the default CRC32 trailer turns framing on, so a naive body
  read records framed bytes rather than JSON. `mock_s3.decode_aws_chunked`
  handles the signed and unsigned spellings.
- `--s3Region` is mandatory: the region resolves eagerly in
  `createS3Storage()` with no try/catch, so omitting it kills the process at
  startup. Credentials are separately mandatory because `S3HashCacheStorage`
  swallows every `SdkException` into a silent miss -- which is also why the
  harness asserts the mock actually received writes.
- Byte-identity is the wrong gate. `HashService.serialize` gsons a HashMap
  built by `Collectors.toMap` over a parallel stream, so JSON key order tracks
  the host's CPU count. The comparator is semantic and reports key-order-only
  differences as a pass.

`serve_harness.serve()` gains defaulted `extra_args` / `env` / `port` /
`bazel` kwargs plus a `reserve_ports()` helper; existing behaviour is
unchanged and `serve_stress.py` benefits too.

Verified: hermetic matrix 47 pass / 0 fail; against this repository at
b7ee642..a4c50ac, three instances on three output bases produced
byte-identical payloads for all 319 targets and identical impacted sets.

The workflow is cron-only for now, matching serve-harness.yml's precedent --
three concurrent Bazel servers is too heavy to gate every PR on until it has
a track record.

Co-Authored-By: Claude Opus 5 <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