chore(bazel): remove references to the decommissioned remote cache - #435
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesRemote cache configuration and CI policy
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant UploadModeScript
participant BazelRemoteCache
GitHubActions->>UploadModeScript: Provide event and ref
UploadModeScript-->>GitHubActions: Return upload mode
GitHubActions->>BazelRemoteCache: Configure cache writes
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/compute-plane-services/ess-agent/scripts/.bazel-remote-probe`:
- Around line 20-31: Add a reusable source-level behavioral test with a stubbed
grpcurl that exercises the shared remote-cache gate for an unset host, disabled
remote caching, and a configured host, then run it against all six
.bazel-remote-probe scripts:
src/compute-plane-services/ess-agent/scripts/.bazel-remote-probe (20-31),
src/compute-plane-services/nvca/scripts/.bazel-remote-probe (19-29),
src/control-plane-services/function-autoscaler/scripts/.bazel-remote-probe
(20-31), src/control-plane-services/helm-reval/scripts/.bazel-remote-probe
(18-29),
src/invocation-plane-services/http-invocation/scripts/.bazel-remote-probe
(20-31), and
src/invocation-plane-services/llm-api-gateway/scripts/.bazel-remote-probe
(20-31). Verify each probe’s host and remote-enabled gate produces the expected
behavior, and run the repository-native test runner.</code>
In `@src/invocation-plane-services/grpc-proxy/proxy/geo/BUILD.bazel`:
- Around line 77-80: Correct the comment describing local = True in the relevant
Bazel test configuration: state that it forces local unsandboxed execution and
disables remote execution, but does not disable all caching. Keep the
explanation that the external tag prevents Bazel from reusing cached test
results, and retain the requires-docker note.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5a518dbf-60c3-427f-9e52-5eac44c08098
📒 Files selected for processing (9)
src/compute-plane-services/byoo-otel-collector/README.mdsrc/compute-plane-services/ess-agent/scripts/.bazel-remote-probesrc/compute-plane-services/nvca/scripts/.bazel-remote-probesrc/compute-plane-services/nvsnap/.bazelrcsrc/control-plane-services/function-autoscaler/scripts/.bazel-remote-probesrc/control-plane-services/helm-reval/scripts/.bazel-remote-probesrc/invocation-plane-services/grpc-proxy/proxy/geo/BUILD.bazelsrc/invocation-plane-services/http-invocation/scripts/.bazel-remote-probesrc/invocation-plane-services/llm-api-gateway/scripts/.bazel-remote-probe
balajinvda
added a commit
that referenced
this pull request
Jul 25, 2026
The README still described the genrule as using local = True and claimed remote-cache reuse via the decommissioned internal cache. Both are now wrong: the tags are no-sandbox + no-remote-exec, the action is cache-eligible, and the host Go toolchain is bound into the action key via --action_env so a compiler bump cannot serve stale binaries. VERSION 0.157.0 -> 0.157.1. The subtree has a gate requiring a VERSION bump whenever any file under it changes, and this PR necessarily changes otelcol/BUILD.bazel. Patch-level only, so the major/minor stays in lockstep with the collector version in otel-collector-build.yaml, which the gate also cross-checks. The produced binary is unchanged in content; only how it is built and cached changes. This also clears the last reference to the retired cache host, which #435 had to leave in place precisely because that PR could not justify a VERSION bump for a comment-only edit. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/ci/test-bazel-remote-probe`:
- Around line 31-35: Update the probe execution in the test script so
source/runtime failures are not suppressed: remove the unconditional success
fallback and preserve the command’s non-zero exit status while retaining the
intentional empty-output behavior for disabled configurations. Ensure check()
can distinguish a failed probe from a valid disabled result and reports the
failure.
- Around line 69-77: Add probe coverage in the test script for both CA-path
branches: add a readable CA with TLS enabled and assert the output includes the
corresponding --tls_certificate flag, then add a configured but nonexistent CA
path and assert the probe is disabled. Keep the existing tls-no-ca case and use
the established check/probe conventions and environment variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5889ab5f-8610-48c3-9178-41cbc9c3247c
📒 Files selected for processing (2)
src/invocation-plane-services/grpc-proxy/proxy/geo/BUILD.bazeltools/ci/test-bazel-remote-probe
🚧 Files skipped from review as they are similar to previous changes (1)
- src/invocation-plane-services/grpc-proxy/proxy/geo/BUILD.bazel
The internal Buildbarn host was retired, but it was still referenced in nine files. The per-service .bazel-remote-probe scripts defaulted NVCF_BAZEL_REMOTE_HOST to it, so an unset variable pointed the probe at a dead host rather than cleanly disabling the cache. - Probe scripts (6): no default host. An unset NVCF_BAZEL_REMOTE_HOST now disables the cache and builds local-only, which is the safe fallback the scripts already document. No new endpoint is hardcoded in its place; the caller supplies it, matching the root .bazelrc. - nvsnap/.bazelrc: drop the dead endpoint from the :remote profile; the profile keeps only policy and tuning flags. - byoo README, grpc-proxy geo BUILD.bazel: comment references. While in the geo BUILD.bazel comment, corrected a factual error: it claimed does NOT bypass result caching. It does. is the union of no-sandbox, no-remote-exec and no-cache. The same misconception is what kept byoo-otel-collector recompiling on every CI run. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
The byoo-otel-collector subtree has a version gate that requires bumping its VERSION file whenever ANY file under the subtree changes, with no exemption for documentation. Bumping the collector's version to fix a one-word comment would cut a spurious release, so leave that reference for the next byoo change that bumps VERSION for a real reason. All functional references are still removed; the remaining one is a single prose mention in a README. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
Two review findings. Behavioral tests. bash -n only checks syntax, and this PR changed what the probes decide, not just how they parse. Add tools/ci/test-bazel-remote-probe: it sources every subtree probe with a stubbed grpcurl (no network) and asserts the enable/disable decision across four cases: host unset, remote explicitly off, host configured, and TLS requested without a CA pem. Six probes, four cases each. The host-unset case deliberately sets NVCF_BAZEL_REMOTE_TLS=0. Without that it was vacuous: with TLS on, an unset CA pem also disables the cache, so the case passed even with the old default host restored. Verified by reintroducing the old default on one probe, confirming the test then fails, and restoring it. Comment accuracy. The geo BUILD.bazel comment equated with disabling all caching. Measured on Bazel 9.1.1 with a scratch workspace: a genrule re-executes after a clean against a warm disk cache while an otherwise identical no-sandbox + no-remote-exec genrule reports a disk cache hit. So does stop disk/remote reuse, but it is not a blanket no-cache: the tag is what prevents reusing a cached test result. Comment now says exactly that. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
Two review findings, and the first exposed a real defect in the probes. The test discarded stderr and forced success, so a probe that CRASHED was indistinguishable from one that deliberately disabled the cache, and every disabled-case assertion would pass on a broken script. It now captures the exit status separately and fails on any non-zero exit, printing the probe's stderr. That immediately surfaced a bug: the probes guarded the TLS path with -z BAZEL_REMOTE_CA_PEM, which only catches an unset variable. A variable set to a path that does not exist fell through to the below and failed the job instead of cleanly declining the cache. The guard is now -r, covering unset and unreadable alike, and the message reports the offending path. Added the two missing cases: TLS with a readable CA pem (the success path that emits --tls_certificate, previously uncovered, asserted to produce both grpcs:// and the certificate flag so a silent downgrade to plaintext cannot pass), and TLS with an unreadable CA path. Six probes, six cases. Every assertion is backed by a negative control, each verified to fail when the defect is reintroduced: dead default host restored, CA guard reverted to -z, TLS downgraded to plaintext, and a probe that exits non-zero. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
kristinapathak
approved these changes
Jul 27, 2026
Cache writes were gated on push + refs/heads/main. Every main-push run shares the concurrency group bazel-refs/heads/main with cancel-in-progress: true, so a merge landing shortly after another cancels the first one's run before it finishes uploading. That is not hypothetical. #444 existed to make the byoo collector genrule cacheable; its main-push run was cancelled four minutes later when #471 merged, so the collector was never written to the cache. Every subsequent job then missed and rebuilt it locally in all three of its configurations, which is most of that row's runtime. Make merge_group the primary writer. Each queue entry gets its own ref, so those runs are never cancelled by the next merge, and the tree under test is exactly what becomes main. Main pushes keep uploading as a backstop for anything reaching main outside the queue. PRs stay read-only (upload defaults to false) so they still cannot poison the cache. A queue entry that fails and never merges may still have written entries. That is harmless for a content-addressed store: entries are keyed by action digest, so a later build with identical inputs is entitled to reuse them. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
balajinvda
force-pushed
the
chore/sync-bazelversion-drop-nvcfbarn
branch
from
July 27, 2026 21:36
4cef9a0 to
c74375f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/bazel.yml:
- Around line 433-450: Add regression coverage for cache-writer selection in
.github/workflows/bazel.yml lines 433-450 and 824-828, covering pull_request,
merge_group, main-push, and non-main-push events for both the container and
docker job gates. If workflow gating cannot be tested, document that limitation
in the pull request instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 134e3309-6bbd-4664-9aa4-8f1e77558856
📒 Files selected for processing (6)
.github/workflows/bazel.ymlsrc/compute-plane-services/byoo-otel-collector/README.mdsrc/compute-plane-services/ess-agent/scripts/.bazel-remote-probesrc/compute-plane-services/nvca/scripts/.bazel-remote-probesrc/control-plane-services/function-autoscaler/scripts/.bazel-remote-probesrc/control-plane-services/helm-reval/scripts/.bazel-remote-probe
🚧 Files skipped from review as they are similar to previous changes (4)
- src/compute-plane-services/nvca/scripts/.bazel-remote-probe
- src/control-plane-services/helm-reval/scripts/.bazel-remote-probe
- src/compute-plane-services/ess-agent/scripts/.bazel-remote-probe
- src/control-plane-services/function-autoscaler/scripts/.bazel-remote-probe
Rebasing this branch onto main replayed a byoo README edit that was written before #444 reworked that paragraph. On the old base the edit was a no-op cleanup; on the new base it reads as -Bazel hermeticity + remote-cache reuse. +Bazel hermeticity + nvcfbarn remote-cache reuse. which re-adds a reference to the decommissioned cache in the very branch whose purpose is removing them. main already has the correct wording, so take main's version. This also clears the byoo VERSION check. That check requires byoo-otel-collector/VERSION to be bumped whenever any file under byoo-otel-collector/ changes, and the README was the only such file here. With it reverted the branch touches no byoo files, so the check passes without a version bump the change never warranted. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cache-writer decision was inline shell in two places in bazel.yml, so nothing could exercise it. actionlint covers workflow syntax and shellchecks run: blocks, but nothing in this repo tests what the shell inside them decides. That decision is a correctness boundary rather than a preference. A pull_request that gains write access can poison a cache every other build trusts, and a merge_group that loses it puts warming back on main pushes, where cancel-in-progress can kill the run before it uploads. Extract it to tools/ci/bazel-cache-upload-mode, call that from both gates, and assert the full event matrix in tools/ci/test-bazel-cache-upload-mode: merge_group and push-to-main write; pull_request, non-main push, tag push, workflow_dispatch and schedule do not; unset and unknown events fail closed. Verified the test fails when the behavior regresses, not just that it passes today. Dropping merge_group as a writer fails the merge_group case; letting unknown events write fails six cases. Also run both this and the existing test-bazel-remote-probe from the detect job. The probe test was added without a caller, so it never ran. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressing the CodeRabbit note on cache-writer coverage (its thread auto-resolved when the lines moved, so recording the response here). The gate was inline shell in two places in Extracted it to
I also verified the test fails when the behavior regresses rather than only passing today. Removing Separately: Two notes for review, since the branch changed under you:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/ci/test-bazel-cache-upload-mode`:
- Around line 43-45: Add separate test invocations in the read-only cases around
“unset event is read-only” and “push with unset ref is read-only” that remove
the variables with env -u GITHUB_EVENT_NAME and env -u GITHUB_REF, rather than
passing empty strings. Keep the existing empty-value coverage and expected
read-only results unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f3683ec7-cc3a-4d37-bf41-3e25e17acfbf
📒 Files selected for processing (3)
.github/workflows/bazel.ymltools/ci/bazel-cache-upload-modetools/ci/test-bazel-cache-upload-mode
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/bazel.yml
Contributor
Author
|
🎉 This PR is included in version nvcf-function-autoscaler-v1.18.8 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Contributor
Author
|
🎉 This PR is included in version nvcf-helm-reval-api-v0.4.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Contributor
Author
|
🎉 This PR is included in version nvcf-grpc-proxy-v1.31.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Contributor
Author
|
🎉 This PR is included in version nvcf-invocation-service-v0.9.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Contributor
Author
|
🎉 This PR is included in version nvcf-llm-api-gateway-v0.11.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Contributor
Author
|
🎉 This PR is included in version nvcf-ess-agent-v1.4.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The internal Buildbarn host was retired, but nine files still referenced it. The important one is not cosmetic: the per-service
.bazel-remote-probescripts defaultedNVCF_BAZEL_REMOTE_HOSTto that host, so an unset variable pointed the probe at a dead endpoint instead of cleanly disabling the cache.What changed
NVCF_BAZEL_REMOTE_HOSTnow disables the cache and builds local-only, which is the safe fallback these scripts already document. No replacement endpoint is hardcoded; the caller supplies it, matching the root.bazelrc.nvsnap/.bazelrc: drop the dead endpoint from the:remoteprofile, leaving only policy and tuning flags.byoo-otel-collector/README.mdandgrpc-proxy/proxy/geo/BUILD.bazel: comment references.There are now zero references to the retired host in the repository.
While editing the geo
BUILD.bazelcomment, corrected a factual error. It claimedlocal = True"does NOT bypass result caching". It does:localis the union ofno-sandbox,no-remote-exec, andno-cache. That same misconception is what kept byoo-otel-collector recompiling on every CI run instead of hitting the remote cache. Only the comment changed here; the target's tags are untouched.Customer Release Notes
Not customer visible.
Testing
All six probe scripts pass
bash -n. The behavior change is fail-safe by construction: previously an unset host produced a probe against a dead endpoint, now it produces a local-only build.Notes
Not included, deliberately. This branch originally also synced every subtree
.bazelversionto the root's 9.1.1. Local validation showed that is a migration, not a file edit, so it was dropped:nats-auth-callout(Go) on 9.1.1:name 'sh_test' is not defined. Bazel 9 removedsh_testfrom the built-in globals; it needs an explicitload()plus arules_shelldependency.ratelimiter(Rust) on 9.1.1:protoc version does not match protobuf Bazel module.Two of two sampled subtrees fail, for two different reasons, so converging the 15 subtrees currently on 8.6.0 up to the root's 9.1.1 needs per-subtree work and belongs in its own tracked effort. The separate float bug (four worker subtrees carrying no
.bazelversionat all, so their release builds resolve whatever bazelisk considers latest) is fixed in #433.References
Follows the root-level cleanup in #399.
Related Merge Requests/Pull Requests
#433 (pins the four worker subtrees that ship no
.bazelversion).Dependencies
None.
Summary by CodeRabbit
Bug Fixes
CI Improvements