diff --git a/tools/ci/postgres-partition.mjs b/tools/ci/postgres-partition.mjs index 0287dc7f9..e96dac0e2 100644 --- a/tools/ci/postgres-partition.mjs +++ b/tools/ci/postgres-partition.mjs @@ -88,9 +88,27 @@ export function packageWeights(entries) { } weights.set(pkg, (weights.get(pkg) ?? 0) + entry.measured_seconds); } + // Packages where EVERY entry is unmeasured. Imputing these from the global + // mean is unbounded: the estimate borrows from unrelated packages, and the + // error is invisible because the imputed value -- not the true one -- is what + // the share guard weighs. Measured: dropping the single 318.8s + // `writer-ownership-canonical-census-pg` (sole member of its package) imputes + // it at 14.3s, 1/209 = 0.48% of entries, and reports `partition ok` while its + // shard really runs 904.1s against a planned 599.6s. + const packagesWithMeasurement = new Set(measured.map((entry) => String(entry.package))); + const unmeasuredPackages = [ + ...new Set( + workflow + .filter((entry) => !isMeasured(entry)) + .map((entry) => String(entry.package)) + .filter((pkg) => !packagesWithMeasurement.has(pkg)), + ), + ].sort(); + return { weights, unmeasured: [...unmeasured].sort(), + unmeasuredPackages, imputedSeconds, measuredCount: measured.length, }; @@ -154,13 +172,14 @@ export function packPackages(weights, shardCount) { * assignment maps package -> shard index. */ export function partitionByDuration(entries, shardCount) { - const { weights, unmeasured, imputedSeconds, measuredCount } = packageWeights(entries); + const { weights, unmeasured, unmeasuredPackages, imputedSeconds, measuredCount } = + packageWeights(entries); const bins = packPackages(weights, shardCount); const assignment = new Map(); for (const bin of bins) { for (const pkg of bin.packages) assignment.set(pkg, bin.index); } - return { assignment, bins, unmeasured, imputedSeconds, measuredCount }; + return { assignment, bins, unmeasured, unmeasuredPackages, imputedSeconds, measuredCount }; } /** @@ -235,6 +254,21 @@ export function partitionFailures(entries, shardCount) { "no workflow entry has a measured_seconds, so there is no basis to impute from", ); } + // The share guard is denominated in ENTRIES while the cost is SECONDS, and + // the map's seconds are wildly non-uniform -- one entry is worth 318.8s + // against a 15.8s mean. A share of entries therefore cannot bound the error + // a lost measurement introduces. What CAN bound it is refusing to impute + // across a package that retains no measurement of its own: within a package + // the sibling mean is a real estimate, and outside one it is a guess with no + // ceiling. + if (result.unmeasuredPackages?.length) { + failures.push( + `${result.unmeasuredPackages.length} package(s) have no measured entry at all, so their ` + + "weight would be guessed from unrelated packages with no bound on the error; " + + "regenerate weights before trusting the balance:\n " + + result.unmeasuredPackages.slice(0, 10).join("\n "), + ); + } } return failures; } diff --git a/tools/ci/postgres-partition.test.mjs b/tools/ci/postgres-partition.test.mjs index c46f334d7..0f2fe20fc 100644 --- a/tools/ci/postgres-partition.test.mjs +++ b/tools/ci/postgres-partition.test.mjs @@ -157,6 +157,43 @@ test("postgres-partition", async (t) => { assert.deepEqual(partitionFailures(entries, 5), []); }); + await t.test("a package that keeps NO measurement fails closed", () => { + // The hole the entry-count share guard cannot close. Cost is seconds and the + // map's seconds are wildly non-uniform, so a share of ENTRIES cannot bound + // the error a lost measurement introduces: on the real map, dropping the + // single 318.8s writer-ownership-canonical-census-pg (sole member of its + // package, 0.48% of entries) imputed it at 14.3s from the global mean and + // reported "partition ok" while its shard really ran 904.1s against a + // planned 599.6s. Within a package the sibling mean is a real estimate; + // across packages it is a guess with no ceiling, so that is what is refused. + const entries = Array.from({ length: 40 }, (_, i) => e(`m${i}`, `pkg${i % 5}`, 10)); + entries.push({ name: "lone", package: "solo-pkg", in_workflow_postgres_job: true }); + const failures = partitionFailures(entries, 5); + assert.ok( + failures.some((f) => /no measured entry at all/.test(f)), + `expected an unmeasured-package failure, got ${JSON.stringify(failures)}`, + ); + assert.ok(failures.some((f) => /solo-pkg/.test(f)), "the offending package must be named"); + }); + + await t.test("a new test in a package that still has measured siblings passes", () => { + // The case imputation exists to allow must keep working: this is exactly + // #802's shape, one new suite joining a package full of measured ones. + const entries = Array.from({ length: 40 }, (_, i) => e(`m${i}`, `pkg${i % 5}`, 10)); + entries.push({ name: "brand-new", package: "pkg0", in_workflow_postgres_job: true }); + assert.deepEqual(partitionFailures(entries, 5), []); + }); + + await t.test("packageWeights names the packages that kept no measurement", () => { + const { unmeasuredPackages } = packageWeights([ + e("a", "measured-pkg", 10), + { name: "n", package: "measured-pkg", in_workflow_postgres_job: true }, + { name: "x", package: "blind-pkg", in_workflow_postgres_job: true }, + ]); + assert.deepEqual(unmeasuredPackages, ["blind-pkg"], + "a package with a measured sibling is bounded; one without is not"); + }); + await t.test("a drifted map still fails closed", () => { // The case the guard was actually for: entries that silently lost weights // they once had. Distinguished from "new test" by share, not by kind.