diff --git a/.agents/ci-caching.md b/.agents/ci-caching.md index 17cc6001d..26aba82e6 100644 --- a/.agents/ci-caching.md +++ b/.agents/ci-caching.md @@ -122,7 +122,7 @@ The per-backend prefix match only sees files under a backend's own directory, so | Changed path | Rebuilds | |---|---| -| `backend/backend.proto` | everything (all languages compile or copy it) | +| `backend/backend.proto` | nothing if the edit is additive-only, otherwise everything (see below) | | `backend/Dockerfile.` | the Linux entries whose `dockerfile:` names it | | `backend/python/common/` | Python, Linux + Darwin | | `scripts/build/package-gpu-libs.sh` | Python, Linux only | @@ -132,6 +132,17 @@ The per-backend prefix match only sees files under a backend's own directory, so Deliberately excluded: `backend/index.yaml` (gallery metadata, never enters an image), `.github/backend-matrix.yml` (adding a backend would rebuild all of them), `backend/Dockerfile.base-grpc-builder` (owned by `base-images.yml`), and the root `Makefile` (touched in ~11% of commits, and its backend-relevant edits arrive alongside the backend directory anyway). `make test-ci-scripts` pins all of this. +#### `backend/backend.proto` is content-filtered, not path-filtered + +Every language consumes the proto, so a path rule for it can only ever say "rebuild all 473 images". It changes in ~1.3% of commits, and that was enough to make it the single largest CI cost driver in the repo: on 2026-07-29 four runs totalling 935 queued jobs traced to nothing but a proto edit, one of which (#11158) was a six-line diff adding `bool cache_prompt = 8;`. + +An additive proto edit cannot change how a backend that never references the new symbol behaves, so `filterMatrix()` suppresses the rule for one. `changed-backends.js` fetches `backend/backend.proto` at the base revision (same contents-API pattern as `.github/backend-matrix.yml`) and hands both texts to `protoChangeIsAdditive()`, which compares them structurally rather than textually: + +- **Additive, rebuilds nothing**: a new field with an unused number, a new message, a new enum value, a new RPC. Comment, whitespace and ordering changes also land here. +- **Breaking, rebuilds everything**: a removed, renumbered, retyped or renamed field, a dropped RPC, a changed `option` or `package`. So does an unresolvable base revision, matching the run-all posture used for a truncated diff. + +Checked against every proto commit in the preceding six months, all nine resolvable ones classify as additive. Note the tradeoff this accepts: generated stubs do change for an additive edit, so image bytes would differ on a rebuild even though behavior does not. That is the same standard already applied when the filter declines to rebuild on unrelated `pkg/` changes, and the weekly cron remains the backstop. + The Sunday 06:00 UTC cron on `backend.yml` exists specifically because path filtering can leave Python backends frozen on stale wheels. `DEPS_REFRESH` (below) only fires when the build actually runs, so an untouched Python backend would never re-resolve its unpinned deps. The weekly cron is the safety net. ## The `DEPS_REFRESH` cache-buster (Python backends) diff --git a/scripts/changed-backends.js b/scripts/changed-backends.js index 1c87d0247..08a024fc6 100644 --- a/scripts/changed-backends.js +++ b/scripts/changed-backends.js @@ -6,6 +6,7 @@ import { getAllBackendPaths, filterMatrix, BACKEND_MATRIX_FILE, + BACKEND_PROTO_FILE, } from "./lib/backend-filter.mjs"; // Matrix data lives in a small data-only YAML so both backend.yml (master push) @@ -116,6 +117,42 @@ async function getPreviousMatrix(event) { } } +// backend.proto at the base revision plus the checked-out copy, so filterMatrix +// can tell an additive edit (a new field, message or RPC, which invalidates no +// existing image) from a breaking one. Returning null means "rebuild +// everything", the same posture as an unresolvable matrix diff. +// +// Only called when the changed-file list names the proto, so the common path +// costs no extra API request. +async function getProtoRevisions(event) { + const ref = event.pull_request ? event.pull_request.base.sha : event.before; + if (!ref || /^0+$/.test(ref)) return null; + const owner = event.repository.owner.login; + const repo = event.repository.name; + try { + const res = await octokit.request('GET /repos/{owner}/{repo}/contents/{path}', { + owner, + repo, + path: BACKEND_PROTO_FILE, + ref, + mediaType: { format: 'raw' }, + }); + const previous = typeof res.data === 'string' + ? res.data + : Buffer.from(res.data.content, 'base64').toString('utf8'); + return { + previous, + current: fs.readFileSync(BACKEND_PROTO_FILE, "utf8"), + }; + } catch (err) { + console.log( + `could not read ${BACKEND_PROTO_FILE} at ${ref}, falling back to run-all:`, + err.message + ); + return null; + } +} + // Group matrix entries by tag-suffix and emit a merge-matrix entry per group. // Both multi-leg groups (per-arch fan-out) and singletons get one entry each: // the build job pushes by digest only with no tags applied, so every backend @@ -240,7 +277,7 @@ function emitFullMatrix() { } } -function emitFilteredMatrix(changedFiles, previousMatrix) { +function emitFilteredMatrix(changedFiles, previousMatrix, protoRevisions) { console.log("Changed files:", changedFiles); const { filtered, filteredDarwin, changedBackends } = filterMatrix({ @@ -248,6 +285,7 @@ function emitFilteredMatrix(changedFiles, previousMatrix) { includesDarwin, changedFiles, previousMatrix, + protoRevisions, }); console.log("Filtered files:", filtered); @@ -306,5 +344,9 @@ function emitFilteredMatrix(changedFiles, previousMatrix) { ? await getPreviousMatrix(event) : null; - emitFilteredMatrix(changedFiles, previousMatrix); + const protoRevisions = changedFiles.includes(BACKEND_PROTO_FILE) + ? await getProtoRevisions(event) + : null; + + emitFilteredMatrix(changedFiles, previousMatrix, protoRevisions); })(); diff --git a/scripts/lib/backend-filter.mjs b/scripts/lib/backend-filter.mjs index 50edd1726..ccded8d53 100644 --- a/scripts/lib/backend-filter.mjs +++ b/scripts/lib/backend-filter.mjs @@ -178,6 +178,126 @@ const GO_BACKEND_PKG_PREFIXES = [ "pkg/utils/", ]; +export const BACKEND_PROTO_FILE = "backend/backend.proto"; +const PROTO_RULE_ID = "backend-proto"; + +// Split a .proto into a map of symbol -> fingerprint so two revisions can be +// compared structurally instead of textually. A comment reflow, a reindent or a +// reordered field must not read as a change, and a renumbered or retyped field +// must. +// +// The scanner is deliberately syntax-light: it tracks brace depth to build a +// container path and records one entry per declaration (`message`, `enum`, +// `service`, `oneof`, `rpc`) and one per statement (fields, enum values, +// `option`, `reserved`). It never needs to understand types, only to notice +// when the text describing one stops being identical. +function protoSymbols(text) { + const symbols = new Map(); + const stack = []; + const norm = s => s.trim().replace(/\s+/g, " "); + + // A declaration is identified by its kind and name, so that changing its body + // shows up as changed members rather than as a wholesale replacement. + const declKey = header => { + const rpc = header.match(/^rpc\s+([A-Za-z_]\w*)/); + if (rpc) return `rpc ${rpc[1]}`; + const decl = header.match(/^(message|enum|service|oneof|extend)\s+([A-Za-z_]\w*)/); + if (decl) return `${decl[1]} ${decl[2]}`; + return header; + }; + + // `bool cache_prompt = 8` is identified by `cache_prompt`, so renumbering or + // retyping it changes the fingerprint under a stable key, while renaming it + // reads as a removal plus an addition. Statements with no `=` (`reserved 4;`) + // are their own identity. + const stmtKey = stmt => { + const eq = stmt.indexOf("="); + if (eq === -1) return stmt; + const lhs = norm(stmt.slice(0, eq)).split(" "); + return lhs[lhs.length - 1] || stmt; + }; + + let buf = ""; + let i = 0; + while (i < text.length) { + const c = text[i]; + + // String literals first: `option go_package = "github.com/..."` contains a + // `//` that is not a comment. + if (c === '"' || c === "'") { + buf += c; + i++; + while (i < text.length) { + if (text[i] === "\\") { + buf += text.slice(i, i + 2); + i += 2; + continue; + } + buf += text[i]; + i++; + if (text[i - 1] === c) break; + } + continue; + } + if (c === "/" && text[i + 1] === "/") { + while (i < text.length && text[i] !== "\n") i++; + continue; + } + if (c === "/" && text[i + 1] === "*") { + i += 2; + while (i < text.length && !(text[i] === "*" && text[i + 1] === "/")) i++; + i += 2; + continue; + } + if (c === "{") { + const header = norm(buf); + buf = ""; + i++; + const key = header ? declKey(header) : ""; + if (header) symbols.set(`${stack.join("/")}|${key}`, header); + stack.push(key); + continue; + } + if (c === "}") { + stack.pop(); + buf = ""; + i++; + continue; + } + if (c === ";") { + const stmt = norm(buf); + buf = ""; + i++; + if (stmt) symbols.set(`${stack.join("/")}|${stmtKey(stmt)}`, stmt); + continue; + } + buf += c; + i++; + } + return symbols; +} + +// True when every symbol the previous revision declared survives unchanged into +// the current one. New symbols are free: a backend that never references a new +// field, message or RPC produces the same behavior with or without it, so its +// image does not need rebuilding. +// +// Anything else (a removed, renumbered, retyped or renamed field, a dropped +// RPC, a changed option) is treated as breaking and rebuilds the full matrix. +// Either text being unresolvable is breaking too, matching the run-all posture +// changed-backends.js takes for a diff it cannot compute. +export function protoChangeIsAdditive(previousText, currentText) { + if (typeof previousText !== "string" || typeof currentText !== "string") { + return false; + } + const before = protoSymbols(previousText); + const after = protoSymbols(currentText); + for (const [key, fingerprint] of before) { + if (after.get(key) !== fingerprint) return false; + } + return true; +} + // Shared build inputs: files that end up in, or decide the contents of, images // belonging to backends whose own directory they do not live under. The // per-backend prefix match in filterMatrix() structurally cannot see these, so @@ -200,7 +320,14 @@ export const SHARED_BUILD_INPUTS = [ // backends regenerate their stubs from it via `make protogen-go`, the C++ // CMakeLists compile backend.pb.cc from it, and the Rust crate Makefile // copies it in before build.rs runs. - matches: file => file === "backend/backend.proto", + // + // Which is why this rule is always/always, and why it is the single most + // expensive entry in the table: 417 Linux plus 56 Darwin builds. It fires + // on 1.3% of commits (10 of 767 over six months), and every one of those + // ten was purely additive. filterMatrix() suppresses this rule for an + // additive-only diff, so `id` exists to identify it there. + id: PROTO_RULE_ID, + matches: file => file === BACKEND_PROTO_FILE, linux: always, darwin: always, }, @@ -362,8 +489,19 @@ export function filterMatrix({ includesDarwin, changedFiles, previousMatrix, + protoRevisions, }) { - const sharedRules = matchedSharedRules(changedFiles); + // An additive-only backend.proto edit invalidates no existing image, so drop + // its always/always rule. Every other matched rule still applies: a PR that + // touches the proto and scripts/build/ is still a full rebuild. + const protoAdditiveOnly = + changedFiles.includes(BACKEND_PROTO_FILE) && + !!protoRevisions && + protoChangeIsAdditive(protoRevisions.previous, protoRevisions.current); + + const sharedRules = matchedSharedRules(changedFiles).filter( + rule => !(rule.id === PROTO_RULE_ID && protoAdditiveOnly) + ); const matrixFileChanged = changedFiles.includes(BACKEND_MATRIX_FILE); // The matrix file changed but we could not resolve what it used to say (API diff --git a/scripts/lib/backend-filter_test.mjs b/scripts/lib/backend-filter_test.mjs index 0d99e1e10..5b62807a7 100644 --- a/scripts/lib/backend-filter_test.mjs +++ b/scripts/lib/backend-filter_test.mjs @@ -385,3 +385,146 @@ test("an unavailable previous matrix conservatively rebuilds everything", () => assert.equal(filtered.length, includes.length); assert.equal(filteredDarwin.length, includesDarwin.length); }); + +// --- backend.proto: additive changes must not rebuild the world ------------- +// +// backend/backend.proto is consumed by every language, so the SHARED_BUILD_INPUTS +// rule for it is always/always: a full 417-entry Linux matrix plus all 56 Darwin +// entries. It fires on 1.3% of commits, and in practice every one of those has +// been purely additive (a new field with an unused number, a new message, a new +// RPC). Adding `bool cache_prompt = 8;` (PR #11158) cannot change the behavior of +// a backend that never reads it, yet it rebuilt all 473 images. +// +// So the rule becomes content-aware rather than path-aware, using the same shape +// as previousMatrix above: changed-backends.js resolves the base revision and +// hands the two texts in, and everything here stays pure. + +const protoWith = body => ` +syntax = "proto3"; + +package backend; + +service Backend { + rpc Health(HealthMessage) returns (Reply) {} + rpc Predict(PredictOptions) returns (Reply) {} +} + +message HealthMessage {} + +message PredictOptions { +${body} +} +`; + +const BASE_FIELDS = ` string Prompt = 1; + int32 Tokens = 2; + bool UseTokenizerTemplate = 3;`; + +const runProto = (previous, current) => + filterMatrix({ + includes, + includesDarwin, + changedFiles: ["backend/backend.proto"], + protoRevisions: { previous, current }, + }); + +test("an added proto field rebuilds nothing", () => { + // The real PR #11158 diff: six added lines, one new field, unused number. + const { filtered, filteredDarwin, changedBackends } = runProto( + protoWith(BASE_FIELDS), + protoWith(`${BASE_FIELDS}\n bool cache_prompt = 8;`) + ); + + assert.deepEqual(filtered, []); + assert.deepEqual(filteredDarwin, []); + assert.equal(changedBackends.size, 0); +}); + +test("an added proto message and RPC rebuild nothing", () => { + const current = protoWith(BASE_FIELDS).replace( + "message HealthMessage {}", + "message HealthMessage {}\n\nmessage ScoreRequest {\n string Text = 1;\n}" + ).replace( + " rpc Predict(PredictOptions) returns (Reply) {}", + " rpc Predict(PredictOptions) returns (Reply) {}\n rpc Score(ScoreRequest) returns (Reply) {}" + ); + + const { filtered, filteredDarwin } = runProto(protoWith(BASE_FIELDS), current); + + assert.deepEqual(filtered, []); + assert.deepEqual(filteredDarwin, []); +}); + +test("a removed proto field rebuilds every backend on every OS", () => { + const { filtered, filteredDarwin } = runProto( + protoWith(BASE_FIELDS), + protoWith(` string Prompt = 1;\n bool UseTokenizerTemplate = 3;`) + ); + + assert.equal(filtered.length, includes.length); + assert.equal(filteredDarwin.length, includesDarwin.length); +}); + +test("a renumbered proto field rebuilds every backend on every OS", () => { + // Wire-incompatible: an old backend reading field 2 gets nothing. + const { filtered, filteredDarwin } = runProto( + protoWith(BASE_FIELDS), + protoWith(` string Prompt = 1;\n int32 Tokens = 9;\n bool UseTokenizerTemplate = 3;`) + ); + + assert.equal(filtered.length, includes.length); + assert.equal(filteredDarwin.length, includesDarwin.length); +}); + +test("a retyped proto field rebuilds every backend on every OS", () => { + const { filtered, filteredDarwin } = runProto( + protoWith(BASE_FIELDS), + protoWith(` string Prompt = 1;\n int64 Tokens = 2;\n bool UseTokenizerTemplate = 3;`) + ); + + assert.equal(filtered.length, includes.length); + assert.equal(filteredDarwin.length, includesDarwin.length); +}); + +test("a renamed proto field rebuilds every backend on every OS", () => { + // Same number and type, but every generated accessor changes name. + const { filtered, filteredDarwin } = runProto( + protoWith(BASE_FIELDS), + protoWith(` string Prompt = 1;\n int32 MaxTokens = 2;\n bool UseTokenizerTemplate = 3;`) + ); + + assert.equal(filtered.length, includes.length); + assert.equal(filteredDarwin.length, includesDarwin.length); +}); + +test("a removed proto RPC rebuilds every backend on every OS", () => { + const { filtered, filteredDarwin } = runProto( + protoWith(BASE_FIELDS), + protoWith(BASE_FIELDS).replace( + " rpc Predict(PredictOptions) returns (Reply) {}\n", + "" + ) + ); + + assert.equal(filtered.length, includes.length); + assert.equal(filteredDarwin.length, includesDarwin.length); +}); + +test("a comment-only proto change rebuilds nothing", () => { + const { filtered, filteredDarwin } = runProto( + protoWith(BASE_FIELDS), + protoWith(` string Prompt = 1;\n // how many tokens to emit\n int32 Tokens = 2;\n bool UseTokenizerTemplate = 3;`) + ); + + assert.deepEqual(filtered, []); + assert.deepEqual(filteredDarwin, []); +}); + +test("unresolvable proto revisions conservatively rebuild everything", () => { + // Same posture as the previousMatrix fallback: if we cannot resolve what the + // proto used to say, we must not claim the change was additive. + const { filtered, filteredDarwin } = runProto(null, protoWith(BASE_FIELDS)); + + assert.equal(filtered.length, includes.length); + assert.equal(filteredDarwin.length, includesDarwin.length); +});