Skip to content

Commit eb9868a

Browse files
committed
fix(powerx): harden window epochs, sources footer, and zoomed-out dim overlay | PowerX:加固窗口时间戳校验、来源脚注与缩放后的置灰遮罩
Review fixes for #931: - reject implausible unix-second window bounds (garbage/ms-scale epochs) in windowFromUnixPair and format window bounds defensively, so a corrupt sidecar can never crash the page with a Date RangeError - credit an artifact in the power block's sources footer only when its parsed content actually contributed (no-op bmk agg rows fall through to the Tier-2 bundle agg fallback) - keep the warmup/post-benchmark dim overlay when the chart is zoomed wholly outside the measurement window instead of dropping all shading - stop mapping the validation sidecar's own schema_version into published.power_metric_schema_version (different versioning axis)
1 parent fea3670 commit eb9868a

7 files changed

Lines changed: 137 additions & 33 deletions

File tree

packages/app/src/app/api/gpu-metrics/route.test.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -530,6 +530,37 @@ describe('GET /api/gpu-metrics', () => {
530530
});
531531
});
532532

533+
it('keeps a no-op bmk artifact out of sources and falls through to the bundle', async () => {
534+
zipRegistry.set('zip:bmk-noop', [
535+
// Agg row with no power fields at all (e.g. an eval-style row).
536+
{ entryName: 'agg_dsr1_h200.json', contents: JSON.stringify({ output_toks_per_sec: 1 }) },
537+
]);
538+
zipRegistry.set('zip:audit-sidecar-only', [
539+
{ entryName: 'power_validation_dsr1_h200.json', contents: SIDECAR_JSON },
540+
]);
541+
installFetch([
542+
{ id: 1, name: 'gpu_metrics_dsr1_h200' },
543+
{ id: 2, name: 'bmk_dsr1_h200', zipKey: 'zip:bmk-noop' },
544+
{ id: 3, name: 'power_audit_dsr1_h200', zipKey: 'zip:audit-sidecar-only' },
545+
]);
546+
547+
const res = await GET(req('/api/gpu-metrics?runId=12345'));
548+
expect(res.status).toBe(200);
549+
const body = await res.json();
550+
// The bmk artifact contributed nothing, so only the bundle is credited.
551+
expect(body.artifacts[0].power.sources).toEqual(['power_audit_dsr1_h200']);
552+
expect(body.artifacts[0].power.published).toEqual({
553+
avg_power_w: 401.5,
554+
avg_total_gpu_power_w: 3212,
555+
power_metric_schema_version: null,
556+
source: 'validation_metrics',
557+
});
558+
expect(body.artifacts[0].power.window).toEqual({
559+
start_unix: 1755000020,
560+
end_unix: 1755000080,
561+
});
562+
});
563+
533564
it('omits power but keeps the CSV view when the sidecar JSON is malformed', async () => {
534565
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
535566
try {

packages/app/src/app/api/gpu-metrics/route.ts

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616
type GithubWorkflowRun,
1717
} from '@/lib/github-artifacts';
1818
import {
19+
hasPowerContent,
1920
mergeArtifactPower,
2021
powerFromAggRow,
2122
powerFromValidationSidecar,
@@ -94,8 +95,13 @@ async function resolveArtifactPower(
9495
const aggRow =
9596
findNamedAggEntry(entries, suffix) ?? (entries.length === 1 ? entries[0].json : null);
9697
if (aggRow) {
97-
fromAgg = powerFromAggRow(aggRow, 'bmk_artifact');
98-
sources.push(bmkArtifact.name);
98+
const parsed = powerFromAggRow(aggRow, 'bmk_artifact');
99+
// A row with no power fields contributes nothing: keep it out of the
100+
// sources footer and let Tier 2's agg fallback take over.
101+
if (hasPowerContent(parsed)) {
102+
fromAgg = parsed;
103+
sources.push(bmkArtifact.name);
104+
}
99105
}
100106
}
101107
}
@@ -107,14 +113,20 @@ async function resolveArtifactPower(
107113
const entries = await downloadJsonEntries(auditArtifact, githubToken);
108114
if (entries) {
109115
const sidecar = selectValidationEntry(entries, suffix);
110-
if (sidecar) fromSidecar = powerFromValidationSidecar(sidecar);
116+
const parsedSidecar = sidecar ? powerFromValidationSidecar(sidecar) : null;
117+
if (hasPowerContent(parsedSidecar)) fromSidecar = parsedSidecar;
118+
let bundleAggContributed = false;
111119
if (!fromAgg) {
112120
const aggRow = findNamedAggEntry(entries, suffix);
113-
if (aggRow) fromAgg = powerFromAggRow(aggRow, 'power_audit_agg');
114-
}
115-
if (sidecar || fromAgg?.published?.source === 'power_audit_agg') {
116-
sources.push(auditArtifact.name);
121+
if (aggRow) {
122+
const parsed = powerFromAggRow(aggRow, 'power_audit_agg');
123+
if (hasPowerContent(parsed)) {
124+
fromAgg = parsed;
125+
bundleAggContributed = true;
126+
}
127+
}
117128
}
129+
if (fromSidecar || bundleAggContributed) sources.push(auditArtifact.name);
118130
}
119131
}
120132

packages/app/src/components/gpu-power/GpuPowerChart.tsx

Lines changed: 21 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -113,9 +113,10 @@ function drawMeasurementWindow(
113113
if (!aligned || width <= 0) return;
114114

115115
const clampX = (x: number) => Math.max(0, Math.min(width, x));
116-
const xStart = clampX(xScale(aligned.startSeconds));
117-
const xEnd = clampX(xScale(aligned.endSeconds));
118-
if (xEnd <= xStart) return;
116+
const rawStart = xScale(aligned.startSeconds);
117+
const rawEnd = xScale(aligned.endSeconds);
118+
const xStart = clampX(rawStart);
119+
const xEnd = clampX(rawEnd);
119120

120121
const windowGroup = group
121122
.insert('g', ':first-child')
@@ -135,30 +136,31 @@ function drawMeasurementWindow(
135136
.text(text);
136137
};
137138

138-
// Dimmed warmup/post regions outside the formal window
139-
if (xStart > 0) {
140-
windowGroup
141-
.append('rect')
142-
.attr('x', 0)
143-
.attr('y', 0)
144-
.attr('width', xStart)
145-
.attr('height', height)
146-
.attr('fill', DIM_COLOR)
147-
.attr('opacity', 0.05);
148-
addRegionLabel(0, xStart, labels.warmup, DIM_COLOR);
149-
}
150-
if (xEnd < width) {
139+
const addDimRegion = (x0: number, x1: number, text: string) => {
151140
windowGroup
152141
.append('rect')
153-
.attr('x', xEnd)
142+
.attr('x', x0)
154143
.attr('y', 0)
155-
.attr('width', width - xEnd)
144+
.attr('width', x1 - x0)
156145
.attr('height', height)
157146
.attr('fill', DIM_COLOR)
158147
.attr('opacity', 0.05);
159-
addRegionLabel(xEnd, width, labels.after, DIM_COLOR);
148+
addRegionLabel(x0, x1, text, DIM_COLOR);
149+
};
150+
151+
if (xEnd <= xStart) {
152+
// Visible x-range lies wholly on one side of the window (deep zoom into
153+
// warmup or post-benchmark): keep the dim overlay so the region is not
154+
// mistaken for measured data.
155+
if (rawEnd <= 0) addDimRegion(0, width, labels.after);
156+
else if (rawStart >= width) addDimRegion(0, width, labels.warmup);
157+
return;
160158
}
161159

160+
// Dimmed warmup/post regions outside the formal window
161+
if (xStart > 0) addDimRegion(0, xStart, labels.warmup);
162+
if (xEnd < width) addDimRegion(xEnd, width, labels.after);
163+
162164
// Measurement window band + dashed boundary lines
163165
windowGroup
164166
.append('rect')

packages/app/src/components/gpu-power/GpuPowerDisplay.tsx

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,12 @@ const DELTA_LEVEL_CLASSES = {
152152
alert: 'text-red-700 dark:text-red-300',
153153
} as const;
154154

155+
/** ISO-format a unix-seconds window bound; Date throws outside ±8.64e15 ms. */
156+
function formatWindowBound(unixSeconds: number): string {
157+
const ms = unixSeconds * 1000;
158+
return Math.abs(ms) <= 8.64e15 ? new Date(ms).toISOString() : String(unixSeconds);
159+
}
160+
155161
type GpuMetricsView = 'chart' | 'correlation';
156162

157163
const GPU_METRICS_VIEW_OPTIONS: SegmentedToggleOption<GpuMetricsView>[] = [
@@ -562,8 +568,8 @@ export default function GpuMetricsDisplay() {
562568
<div className="flex flex-wrap gap-x-6 gap-y-1 text-sm mb-2">
563569
<span>
564570
<span className="text-muted-foreground">{t.windowLabel}</span>{' '}
565-
{new Date(window.start_unix * 1000).toISOString()}{' '}
566-
{new Date(window.end_unix * 1000).toISOString()}
571+
{formatWindowBound(window.start_unix)}{' '}
572+
{formatWindowBound(window.end_unix)}
567573
</span>
568574
<span>
569575
<span className="text-muted-foreground">{t.windowDurationLabel}</span>{' '}

packages/app/src/lib/api-route-catalog.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ export const apiRouteCatalog = [
5656
},
5757
// 2026-08: additive optional `power` block assembled from same-suffix
5858
// bmk_*/power_audit_* sibling artifacts; still a UI-only unstable shape.
59-
sourceSha256: 'e2e860cca7365a49fa56e39657a2e7d272a061544415f3adcf445a5a24f94e3c',
59+
sourceSha256: 'a752682622d9685ab7f76a1bd0d49d94df4dcc659fde36e62d56681f6146df7a',
6060
},
6161
{
6262
source: 'src/app/api/openapi.json/route.ts',

packages/app/src/lib/power-audit-artifacts.test.ts

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { describe, expect, it } from 'vitest';
22

33
import {
4+
hasPowerContent,
45
mergeArtifactPower,
56
powerFromAggRow,
67
powerFromValidationSidecar,
@@ -108,6 +109,25 @@ describe('powerFromAggRow', () => {
108109
it('returns an empty partial for a row with no power fields', () => {
109110
expect(powerFromAggRow({ output_toks_per_sec: 1000 })).toEqual({});
110111
});
112+
113+
it('rejects implausible window epochs (garbage, ms-scale, non-positive)', () => {
114+
// 1e16 s → new Date(1e19 ms) would throw RangeError in the client render.
115+
expect(
116+
powerFromAggRow({
117+
power_audit: { window_start_unix: 1e16, window_end_unix: 1e16 + 60 },
118+
}).window,
119+
).toBeUndefined();
120+
expect(
121+
powerFromAggRow({
122+
power_audit: { window_start_unix: 1_755_000_020_000, window_end_unix: 1_755_000_080_000 },
123+
}).window,
124+
).toBeUndefined();
125+
expect(
126+
powerFromAggRow({
127+
power_audit: { window_start_unix: -5, window_end_unix: 1_755_000_080 },
128+
}).window,
129+
).toBeUndefined();
130+
});
111131
});
112132

113133
// ---------------------------------------------------------------------------
@@ -138,11 +158,21 @@ describe('powerFromValidationSidecar', () => {
138158
expect(result.published).toEqual({
139159
avg_power_w: 401.25,
140160
avg_total_gpu_power_w: 3210,
141-
power_metric_schema_version: 1,
161+
// The sidecar's own schema_version is a different versioning axis than
162+
// the agg row's power_metric_schema_version — never mapped through.
163+
power_metric_schema_version: null,
142164
source: 'validation_metrics',
143165
});
144166
});
145167

168+
it('rejects implausible window epochs in the sidecar', () => {
169+
expect(
170+
powerFromValidationSidecar({
171+
benchmark_window: { start_time_unix: 1_755_000_020_000, end_time_unix: 1_755_000_080_000 },
172+
}).window,
173+
).toBeUndefined();
174+
});
175+
146176
it('maps an invalid verdict with reasons', () => {
147177
const result = powerFromValidationSidecar({
148178
power_valid: false,
@@ -252,6 +282,19 @@ describe('selectValidationEntry', () => {
252282
});
253283
});
254284

285+
// ---------------------------------------------------------------------------
286+
// hasPowerContent
287+
// ---------------------------------------------------------------------------
288+
289+
describe('hasPowerContent', () => {
290+
it('distinguishes empty partials from ones carrying a power field', () => {
291+
expect(hasPowerContent(null)).toBe(false);
292+
expect(hasPowerContent({})).toBe(false);
293+
expect(hasPowerContent({ power_valid: 1 })).toBe(true);
294+
expect(hasPowerContent(powerFromAggRow({ output_toks_per_sec: 1000 }))).toBe(false);
295+
});
296+
});
297+
255298
// ---------------------------------------------------------------------------
256299
// mergeArtifactPower
257300
// ---------------------------------------------------------------------------

packages/app/src/lib/power-audit-artifacts.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,17 @@ function asRecord(value: unknown): Record<string, unknown> | null {
4646
: null;
4747
}
4848

49+
// Upper bound on plausible unix-second epochs (2100-01-01T00:00:00Z). Rejects
50+
// ms-scale and garbage values, and keeps bound*1000 inside Date's ±8.64e15 ms
51+
// representable range so the client can always ISO-format the window.
52+
const MAX_UNIX_SECONDS = 4_102_444_800;
53+
4954
function windowFromUnixPair(start: unknown, end: unknown): PowerWindow | null {
5055
const startUnix = asFiniteNumber(start);
5156
const endUnix = asFiniteNumber(end);
5257
if (startUnix === null || endUnix === null) return null;
58+
if (startUnix <= 0 || endUnix <= 0) return null;
59+
if (startUnix > MAX_UNIX_SECONDS || endUnix > MAX_UNIX_SECONDS) return null;
5360
return { start_unix: startUnix, end_unix: endUnix };
5461
}
5562

@@ -141,7 +148,9 @@ export function powerFromValidationSidecar(
141148
result.published = {
142149
avg_power_w: avgPowerW,
143150
avg_total_gpu_power_w: avgTotalGpuPowerW,
144-
power_metric_schema_version: asFiniteNumber(sidecar.schema_version),
151+
// The sidecar's schema_version versions the sidecar itself, a
152+
// different axis than the agg row's power_metric_schema_version.
153+
power_metric_schema_version: null,
145154
source: 'validation_metrics',
146155
};
147156
}
@@ -195,7 +204,8 @@ export function selectValidationEntry(
195204
return null;
196205
}
197206

198-
function hasContent(
207+
/** True when a mapped partial carries at least one power field. */
208+
export function hasPowerContent(
199209
partial: Partial<GpuArtifactPower> | null,
200210
): partial is Partial<GpuArtifactPower> {
201211
return partial !== null && Object.keys(partial).length > 0;
@@ -212,7 +222,7 @@ export function mergeArtifactPower(
212222
fromSidecar: Partial<GpuArtifactPower> | null,
213223
sources: string[],
214224
): GpuArtifactPower | null {
215-
if (!hasContent(fromAgg) && !hasContent(fromSidecar)) return null;
225+
if (!hasPowerContent(fromAgg) && !hasPowerContent(fromSidecar)) return null;
216226
const agg = fromAgg ?? {};
217227
const sidecar = fromSidecar ?? {};
218228

0 commit comments

Comments
 (0)