From 174fe36845e1861c5cc470554ca81ff4d1255094 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Wed, 29 Jul 2026 22:08:09 +0900 Subject: [PATCH] fix(federated): use analytics.ts's median for the peer benchmark, not nearest-rank MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `refreshFederatedBenchmarkCache` computed the peer "median" with `percentile(sorted, 50)` — a nearest-rank estimator that, on an even-sized peer set, returns one middle element rather than the mean of the two. `computeFleetAnalytics` uses `median` (which averages the two middle values), so the peer benchmark and the fleet aggregation could disagree for the same data. Export `median` from analytics.ts and call it (it sorts internally, so the local `.sort()` is removed), making the two halves share one estimator by construction. Document the two contracts side by side: `median` sorts a copy and averages at even n; `percentile` requires a pre-sorted input and does not sort (its call sites, cycleP50Ms/cycleP95Ms, are named). Those percentiles are unchanged — they are percentiles, not medians. Closes #9645 --- src/orb/analytics.ts | 10 +++++++++- src/orb/federated-benchmark.ts | 15 ++++++++------- test/unit/federated-benchmark.test.ts | 26 ++++++++++++++++++++++---- 3 files changed, 39 insertions(+), 12 deletions(-) diff --git a/src/orb/analytics.ts b/src/orb/analytics.ts index 7ec3df5a25..35ffda61c9 100644 --- a/src/orb/analytics.ts +++ b/src/orb/analytics.ts @@ -174,13 +174,21 @@ export interface FleetAnalytics { cycleTimeObservable: boolean; } -function median(xs: number[]): number | null { +/** The fleet-aggregation median estimator (#9645) — the single one both computeFleetAnalytics and the federated + * peer benchmark (federated-benchmark.ts) share. Sorts a COPY internally (callers need not pre-sort) and, at + * even n, returns the MEAN of the two middle values — unlike {@link percentile}(…, 50)'s nearest-rank, which + * returns the upper-middle value. Null for an empty input. */ +export function median(xs: number[]): number | null { if (xs.length === 0) return null; const s = [...xs].sort((a, b) => a - b); const mid = Math.floor(s.length / 2); return s.length % 2 === 0 ? (s[mid - 1]! + s[mid]!) / 2 : s[mid]!; } +/** Nearest-rank percentile. PRECONDITION: `sorted` MUST already be ascending-sorted — this function does NOT + * sort. Satisfied by its two call sites, cycleP50Ms and cycleP95Ms below, which pass an already-sorted array. + * For a true median use {@link median} (which sorts and averages the two middle values at even n) — a p=50 + * call here is nearest-rank, not a median. */ export function percentile(sorted: number[], p: number): number | null { if (sorted.length === 0) return null; // Nearest-rank: the p-th percentile is the value at 1-based rank ceil(p/100 * N), i.e. index diff --git a/src/orb/federated-benchmark.ts b/src/orb/federated-benchmark.ts index 7f59402eec..e1ff37193b 100644 --- a/src/orb/federated-benchmark.ts +++ b/src/orb/federated-benchmark.ts @@ -22,7 +22,7 @@ import { buildFederatedBundle, isFederatedIntelligenceEnabled, resolveFederatedWindowDays } from "./federated-bundle"; import { applyFederatedPeerWatermarks, importPeerBundles } from "./federated-import"; import { pullPeerBundles, pushFederatedBundle, type CollectorOpts } from "./federated-collector"; -import { percentile } from "./analytics"; +import { median } from "./analytics"; import { loadRepoFocusManifest } from "../signals/focus-manifest-loader"; import { resolveLoopOverSelfRepoFullName } from "../config/loopover-repo-focus-manifest"; import type { FocusManifest } from "../signals/focus-manifest"; @@ -140,18 +140,19 @@ export async function refreshFederatedBenchmarkCache( const stateless = importPeerBundles(manifest, peerBundles, { now, localWindowDays }); const { accepted } = config ? await applyFederatedPeerWatermarks(db, stateless, config.peerKeys, { now }) : stateless; - // MEDIAN, NOT MEAN (mirrors analytics.ts's own fleet aggregation, see federated-import.ts's header comment): - // a bounded number of outliers cannot drag a median arbitrarily, so re-deriving a mean here would quietly - // weaken the same poisoning-resistance property the import side already relies on holding by construction. + // MEDIAN, NOT MEAN, and via analytics.ts's OWN `median` — the identical estimator computeFleetAnalytics uses + // for the fleet median (#9645). A nearest-rank percentile(…, 50) differed on an even-sized peer set (it + // returns the upper-middle value, not the mean of the two middle ones), so the peer benchmark and the fleet + // aggregation could disagree; sharing one function makes them agree by construction. A bounded number of + // outliers cannot drag a median arbitrarily, preserving the poisoning-resistance the import side relies on. // `accepted` is deduped one-bundle-per-instanceId by importPeerBundles (#9148), so this count is already a // count of distinct CONTRIBUTING INSTANCES, not raw bundles — the bug the peerCount doc used to warn about. const peerMergePrecisions = accepted .map((bundle) => bundle.mergePrecision) - .filter((value): value is number => value !== null) - .sort((a, b) => a - b); + .filter((value): value is number => value !== null); await writeFederatedBenchmarkCache(db, { - peerMedianMergePrecision: percentile(peerMergePrecisions, 50), + peerMedianMergePrecision: median(peerMergePrecisions), peerCount: peerMergePrecisions.length, refreshedAt: new Date(now).toISOString(), }); diff --git a/test/unit/federated-benchmark.test.ts b/test/unit/federated-benchmark.test.ts index a59d9a26d1..ea7d3ca85e 100644 --- a/test/unit/federated-benchmark.test.ts +++ b/test/unit/federated-benchmark.test.ts @@ -187,6 +187,24 @@ describe("buildFederatedBenchmark() — reads whatever refreshFederatedBenchmark expect(result?.peerCount).toBe(1); }); + it("uses analytics.ts's median (mean of the two middle values), so 0.4 + 0.8 → 0.6, not the nearest-rank 0.4 (#9645)", async () => { + const db = makeDb(); + for (let pr = 1; pr <= 5; pr++) await resolved(db, pr); + // Two DISTINCT peer instances (importPeerBundles dedupes by instanceId) with precisions 0.4 and 0.8. + const fetchFn = fetchReturning([ + signedWith(PEER_KEY_A, { instanceId: "peer-a-inst", mergePrecision: 0.4 }), + signedWith(PEER_KEY_B, { instanceId: "peer-b-inst", mergePrecision: 0.8 }), + ]); + + await refreshFederatedBenchmarkCache(manifest({ peerKeys: [PEER_KEY_A, PEER_KEY_B] }), db, { now: NOW, fetchFn }); + const result = await buildFederatedBenchmark(manifest({ peerKeys: [PEER_KEY_A, PEER_KEY_B] }), db, { now: NOW }); + + // A true median: (0.4 + 0.8) / 2 = 0.6 (float-close). The old nearest-rank percentile(…, 50) returned a + // single middle element (0.4 for this even-sized set), not the mean of the two — that was the bug. + expect(result?.peerMedianMergePrecision).toBeCloseTo(0.6, 10); + expect(result?.peerCount).toBe(2); + }); + it("reports a null local precision (below MIN_DECIDED) while still surfacing a cached peer median", async () => { const db = makeDb(); await resolved(db, 1); // only 1 decided PR, below MIN_DECIDED (5) @@ -245,12 +263,12 @@ describe("refreshFederatedBenchmarkCache() — the background-tick write path (# ]); await refreshFederatedBenchmarkCache(manifest({ peerKeys: [PEER_KEY_A, PEER_KEY_B] }), db, { now: NOW, fetchFn }); - // Median of [0.5, 0.9] (the untrusted 0.01 is rejected, not merely a low outlier) is 0.5 under this - // module's nearest-rank percentile(50) (analytics.ts: idx = ceil(0.5*2)-1 = 0) — pinning the real - // cross-module contract, not a re-derivation. + // Median of [0.5, 0.9] (the untrusted 0.01 is rejected, not merely a low outlier) is (0.5 + 0.9) / 2 = 0.7 + // via analytics.ts's shared `median` — the SAME estimator computeFleetAnalytics uses (#9645). This used to + // read 0.5 under the module's own nearest-rank percentile(50); the two halves now agree. const result = await buildFederatedBenchmark(manifest({ peerKeys: [PEER_KEY_A, PEER_KEY_B] }), db, { now: NOW }); expect(result?.peerCount).toBe(2); - expect(result?.peerMedianMergePrecision).toBe(0.5); + expect(result?.peerMedianMergePrecision).toBe(0.7); }); it("a second tick's median REPLACES the first's, rather than merging the two", async () => {