Skip to content

Commit 2104e57

Browse files
committed
fix(daemon): stop moving a published metric, and abstain when tools are unattributable
Two findings from the seventh review round. Passing the producer's start into `markFirstModelEvent` moved `time_to_first_model_event_ms`, which this PR promised not to touch. That field means "when we first saw a model event"; for an ACP tool starting at 4s and emitted at terminal 20s it began reporting 4s instead of 20s. My own invariance fixture did not catch it because it only pins the four first-token metrics. The mark is now split. `firstModelEventAt` goes back to first-write-wins on arrival and keeps feeding the published metric unchanged. A new `firstModelResponseAt` holds the earliest producer start (clamped to arrival, so a fast producer clock cannot claim the model responded in the future) and is what phase boundaries anchor on. The new phase field is renamed to `runtime_init_to_first_model_response_ms` to match what it measures; it is introduced by this PR and has never shipped. Second, a tool id shared by two attempts cannot be attributed. When a retry reuses an id whose previous opener is still outstanding, a later `tool_result` could belong to either, and the two readings differ by seconds of occupancy. Earlier rounds tried to pick a winner; nothing in the event log supports one. The summarizer now records that an opener was displaced, marks the ledger ambiguous when a result arrives for such an id, and withholds `bottleneck_phase` and a `complete` phase status -- the same treatment truncation already gets, and for the same reason. Timings derived from lifecycle marks keep reporting. Fixing this properly rather than abstaining needs an attempt or generation id carried on the tool events themselves, which is a producer and event-schema change; noted in the review thread rather than smuggled in here.
1 parent 1e55ab7 commit 2104e57

7 files changed

Lines changed: 192 additions & 34 deletions

File tree

apps/daemon/src/run-analytics-observability.ts

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,10 @@ export interface RunTelemetryTimestamps {
104104
stdinWriteStartAt?: number;
105105
stdinWriteEndAt?: number;
106106
firstModelEventAt?: number;
107+
// When the model began responding, as opposed to when we first saw evidence
108+
// of it. Phase boundaries anchor here; `firstModelEventAt` keeps feeding the
109+
// published `time_to_first_model_event_ms`.
110+
firstModelResponseAt?: number;
107111
firstModelEventType?: TrackingFirstModelEventType;
108112
firstTokenAt?: number;
109113
firstVisibleOutputAt?: number;
@@ -172,7 +176,7 @@ export interface RunTimingAnalytics {
172176
// thinking, text, artifact) rather than to the first text token. On a
173177
// tool-first run `runtime_init_to_first_token_ms` swallows the entire tool
174178
// loop; this one stops the moment the model starts responding.
175-
runtime_init_to_first_model_event_ms?: number;
179+
runtime_init_to_first_model_response_ms?: number;
176180
spawn_to_first_token_ms?: number;
177181
time_to_first_artifact_ms?: number;
178182
// `spawn_to_first_token_ms` split into auditable subsegments. By construction
@@ -1009,6 +1013,12 @@ export function summarizeRunTimingAnalytics(args: {
10091013
// separate from its start, which may be a producer-supplied `startedAt` from
10101014
// a different clock and so cannot be compared against our own marks.
10111015
const openToolObservedAt = new Map<string, number>();
1016+
// Tool ids whose opener was replaced by a same-id call from a later attempt.
1017+
const displacedToolUseIds = new Set<string>();
1018+
// Set when a `tool_result` arrives for such an id: the two candidate openers
1019+
// differ by seconds of occupancy and the log cannot say which one closed, so
1020+
// phase attribution for this run is not a measurement.
1021+
let toolLedgerAmbiguous = false;
10121022
const openToolNames = new Map<string, string>();
10131023
// Count unique tool_use ids so historical double-emits (or retries) do not
10141024
// inflate tool_call_count.
@@ -1068,6 +1078,10 @@ export function summarizeRunTimingAnalytics(args: {
10681078
attemptStartAt !== undefined &&
10691079
priorObservedAt < attemptStartAt &&
10701080
ts >= attemptStartAt;
1081+
// Remember that a previous attempt's opener was pushed aside. Its result
1082+
// may still be in flight, and once two calls have shared an id nothing in
1083+
// the event log says which of them a later `tool_result` closes.
1084+
if (reusesDeadAttemptId) displacedToolUseIds.add(data.id);
10711085
if (!openTools.has(data.id) || reusesDeadAttemptId) {
10721086
openTools.set(data.id, toolStartedAt);
10731087
openToolObservedAt.set(data.id, ts);
@@ -1095,6 +1109,7 @@ export function summarizeRunTimingAnalytics(args: {
10951109
const startedAt = openTools.get(data.toolUseId);
10961110
if (startedAt !== undefined && ts >= startedAt) {
10971111
toolDurationMs += ts - startedAt;
1112+
if (displacedToolUseIds.has(data.toolUseId)) toolLedgerAmbiguous = true;
10981113
if (openedInCurrentAttempt(openToolObservedAt.get(data.toolUseId))) {
10991114
toolIntervals.push({ start: startedAt, end: ts });
11001115
}
@@ -1133,6 +1148,7 @@ export function summarizeRunTimingAnalytics(args: {
11331148
// daemon-generated finalizer event, a producer clock offset), and this keeps
11341149
// one from dragging every boundary to the end of the run.
11351150
const phaseAnchorCandidates = [
1151+
telemetry.firstModelResponseAt,
11361152
telemetry.firstModelEventAt,
11371153
telemetry.firstTokenAt,
11381154
].filter((value): value is number => value !== undefined && Number.isFinite(value));
@@ -1196,7 +1212,7 @@ export function summarizeRunTimingAnalytics(args: {
11961212
if (runtimeInitToFirstToken !== undefined) {
11971213
result.runtime_init_to_first_token_ms = runtimeInitToFirstToken;
11981214
}
1199-
setMeasuredDuration(result, 'runtime_init_to_first_model_event_ms', phaseDurations, 'runtime_init', runtimeInitStartAt, phaseAnchorAt);
1215+
setMeasuredDuration(result, 'runtime_init_to_first_model_response_ms', phaseDurations, 'runtime_init', runtimeInitStartAt, phaseAnchorAt);
12001216
const spawnToFirstToken = durationBetween(telemetry.processSpawnedAt, telemetry.firstTokenAt);
12011217
if (spawnToFirstToken !== undefined) result.spawn_to_first_token_ms = spawnToFirstToken;
12021218
const timeToFirstArtifact = durationBetween(startAt, firstArtifactWriteAt);
@@ -1281,7 +1297,12 @@ export function summarizeRunTimingAnalytics(args: {
12811297
// built from lifecycle marks are still sound, but we cannot know whether an
12821298
// evicted tool would have outweighed them, so naming a winner would report
12831299
// an artefact of what the buffer happened to keep.
1284-
if (bottleneckPhase !== undefined && eventStreamComplete) {
1300+
// The ledger phases are reconstructed from tool frames. Truncation removes
1301+
// frames; an id shared by two attempts makes the surviving ones
1302+
// unattributable. Either way the winner would describe the log rather than
1303+
// the run.
1304+
const phaseLedgerReliable = eventStreamComplete && !toolLedgerAmbiguous;
1305+
if (bottleneckPhase !== undefined && phaseLedgerReliable) {
12851306
result.bottleneck_phase = bottleneckPhase;
12861307
}
12871308
result.phase_schema_version = RUN_PHASE_SCHEMA_VERSION;
@@ -1302,7 +1323,7 @@ export function summarizeRunTimingAnalytics(args: {
13021323
// Truncation can only downgrade a `complete` claim; it never upgrades a
13031324
// bundle whose boundaries were genuinely missing.
13041325
result.phase_timing_status =
1305-
!eventStreamComplete && phaseTimingStatus === 'complete'
1326+
!phaseLedgerReliable && phaseTimingStatus === 'complete'
13061327
? 'partial'
13071328
: phaseTimingStatus;
13081329

apps/daemon/src/run-lifecycle-tracer.ts

Lines changed: 42 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,10 @@ export function runLifecycleMarkersForStreamEvent(
103103

104104
export function createRunLifecycleTracer(run: RunWithLifecycleTelemetry): {
105105
mark(mark: RunLifecycleMark, timestamp?: number): void;
106-
markFirstModelEvent(type: TrackingFirstModelEventType, timestamp?: number): void;
106+
markFirstModelEvent(
107+
type: TrackingFirstModelEventType,
108+
producerStartedAt?: number,
109+
): void;
107110
resetForAttempt(attemptIndex: number, timestamp?: number): void;
108111
} {
109112
const mark = (lifecycleMark: RunLifecycleMark, timestamp = Date.now()) => {
@@ -118,22 +121,45 @@ export function createRunLifecycleTracer(run: RunWithLifecycleTelemetry): {
118121

119122
return {
120123
mark,
121-
markFirstModelEvent(type: TrackingFirstModelEventType, timestamp = Date.now()) {
122-
if (!Number.isFinite(timestamp)) return;
124+
markFirstModelEvent(
125+
type: TrackingFirstModelEventType,
126+
producerStartedAt?: number,
127+
) {
128+
const arrivedAt = Date.now();
123129
const current = run.analyticsTelemetry ?? {};
124-
const existing = current.firstModelEventAt;
125-
// Earliest wins, not first observed. ACP holds each toolCallId until it
126-
// is terminal, so two parallel calls can complete in the opposite order
127-
// they started: the call that began later can be the first one we hear
128-
// about. First-write-wins would anchor on it and lose the real head
129-
// start. Marks without a producer timestamp default to arrival, which is
130-
// monotonic, so this only ever moves the anchor earlier.
131-
if (existing !== undefined && timestamp >= existing) return;
132-
run.analyticsTelemetry = {
133-
...current,
134-
firstModelEventAt: timestamp,
135-
firstModelEventType: type,
136-
};
130+
const next = { ...current };
131+
let changed = false;
132+
133+
// `firstModelEventAt` is when we SAW the first model event. It is already
134+
// published as `time_to_first_model_event_ms`, so it stays first-write-
135+
// wins on arrival -- a producer-supplied start must not silently move it.
136+
if (current.firstModelEventAt === undefined) {
137+
next.firstModelEventAt = arrivedAt;
138+
next.firstModelEventType = type;
139+
changed = true;
140+
}
141+
142+
// `firstModelResponseAt` is when the model actually began responding, and
143+
// is what phase boundaries anchor on. Two reasons it differs from
144+
// arrival: ACP holds each toolCallId until terminal status, so the
145+
// canonical `tool_use` arrives when the tool ENDS while its payload
146+
// carries the real start; and parallel calls can terminate in the
147+
// opposite order they began, so earliest-wins rather than first-wins.
148+
// Clamped to arrival so a producer clock running ahead cannot claim the
149+
// model responded in the future.
150+
const responseAt =
151+
typeof producerStartedAt === 'number' && Number.isFinite(producerStartedAt)
152+
? Math.min(producerStartedAt, arrivedAt)
153+
: arrivedAt;
154+
if (
155+
current.firstModelResponseAt === undefined ||
156+
responseAt < current.firstModelResponseAt
157+
) {
158+
next.firstModelResponseAt = responseAt;
159+
changed = true;
160+
}
161+
162+
if (changed) run.analyticsTelemetry = next;
137163
},
138164
resetForAttempt(attemptIndex: number, timestamp = Date.now()) {
139165
run.analyticsTelemetry = {

apps/daemon/src/server.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10688,9 +10688,10 @@ export async function startServer({
1068810688
const send = (event, data) => {
1068910689
const lifecycleMarkers = runLifecycleMarkersForStreamEvent(event, data);
1069010690
if (lifecycleMarkers.firstModelEventType) {
10691-
// `firstModelEventAt` is present when the producer told us when the
10692-
// work began. ACP emits `tool_use` at terminal status, so without this
10693-
// the anchor lands at tool completion.
10691+
// Second argument is the PRODUCER's start, used only for the phase
10692+
// anchor. ACP emits `tool_use` at terminal status, so without it the
10693+
// anchor lands at tool completion. `time_to_first_model_event_ms`
10694+
// still measures to arrival and is unaffected.
1069410695
lifecycle.markFirstModelEvent(
1069510696
lifecycleMarkers.firstModelEventType,
1069610697
lifecycleMarkers.firstModelEventAt,

apps/daemon/tests/run-analytics-observability.test.ts

Lines changed: 79 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1148,7 +1148,7 @@ describe('summarizeRunTimingAnalytics', () => {
11481148
time_to_first_token_ms: 1300,
11491149
time_to_first_visible_output_ms: 1300,
11501150
runtime_init_to_first_token_ms: 650,
1151-
runtime_init_to_first_model_event_ms: 150,
1151+
runtime_init_to_first_model_response_ms: 150,
11521152
spawn_to_first_token_ms: 740,
11531153
time_to_first_artifact_ms: 3050,
11541154
// No subsegment markers were observed, so the whole spawn->first-token
@@ -1550,7 +1550,7 @@ describe('summarizeRunTimingAnalytics phase anchoring', () => {
15501550
const result = summarizeRunTimingAnalytics(toolFirstRun);
15511551

15521552
// stdin closed at 3.2s and the model responded at 4s.
1553-
expect(result.runtime_init_to_first_model_event_ms).toBe(800);
1553+
expect(result.runtime_init_to_first_model_response_ms).toBe(800);
15541554
});
15551555

15561556
it('counts the whole tool loop as model-active time', () => {
@@ -1614,7 +1614,7 @@ describe('summarizeRunTimingAnalytics phase anchoring', () => {
16141614

16151615
// Text-first runs already had a truthful anchor, so the new fields must
16161616
// land on exactly the old numbers -- no drift for Claude Code-shaped runs.
1617-
expect(result.runtime_init_to_first_model_event_ms).toBe(result.runtime_init_to_first_token_ms);
1617+
expect(result.runtime_init_to_first_model_response_ms).toBe(result.runtime_init_to_first_token_ms);
16181618
expect(result.model_active_duration_ms).toBe(result.generation_duration_ms);
16191619
});
16201620

@@ -1644,7 +1644,7 @@ describe('summarizeRunTimingAnalytics phase anchoring', () => {
16441644
// are not reset per retry attempt. `time_to_first_model_event_ms` keeps
16451645
// its existing scan-based fallback and is unaffected.
16461646
expect(result.time_to_first_model_event_ms).toBe(2_000);
1647-
expect(result.runtime_init_to_first_model_event_ms).toBe(20_800);
1647+
expect(result.runtime_init_to_first_model_response_ms).toBe(20_800);
16481648
expect(result.model_active_duration_ms).toBe(1_000);
16491649
});
16501650
});
@@ -1788,7 +1788,7 @@ describe('summarizeRunTimingAnalytics anchor and completeness edges', () => {
17881788
// reports a 1s active window and a 16s runtime init for a run that was
17891789
// streaming the whole time.
17901790
expect(result.model_active_duration_ms).toBe(15_000);
1791-
expect(result.runtime_init_to_first_model_event_ms).toBe(2_000);
1791+
expect(result.runtime_init_to_first_model_response_ms).toBe(2_000);
17921792
expect(result.bottleneck_phase).toBe('stream_output');
17931793
});
17941794

@@ -2068,3 +2068,77 @@ describe('summarizeRunTimingAnalytics late tool results from a dead attempt', ()
20682068
expect(result.tool_duration_ms).toBe(26_900);
20692069
});
20702070
});
2071+
2072+
describe('summarizeRunTimingAnalytics separates the response anchor from the event mark', () => {
2073+
it('anchors phases on the response while the published metric keeps arrival', () => {
2074+
const result = summarizeRunTimingAnalytics({
2075+
runCreatedAt: 1_000,
2076+
runUpdatedAt: 30_000,
2077+
analyticsCapturedAt: 30_050,
2078+
telemetry: {
2079+
startRequestedAt: 1_100,
2080+
startChatRunStartedAt: 2_000,
2081+
stdinWriteEndAt: 3_000,
2082+
// ACP: the canonical tool_use arrived at 20s, but the tool began at 4s.
2083+
firstModelEventAt: 20_000,
2084+
firstModelResponseAt: 4_000,
2085+
firstModelEventType: 'tool_use' as const,
2086+
attemptStartedAt: 2_000,
2087+
attemptIndex: 1,
2088+
},
2089+
events: [],
2090+
});
2091+
2092+
// Already published, and this PR promised not to move it.
2093+
expect(result.time_to_first_model_event_ms).toBe(18_000);
2094+
// New, and measured from when the model actually started working.
2095+
expect(result.runtime_init_to_first_model_response_ms).toBe(1_000);
2096+
expect(result.model_active_duration_ms).toBe(26_000);
2097+
});
2098+
});
2099+
2100+
describe('summarizeRunTimingAnalytics with an unattributable tool ledger', () => {
2101+
const ambiguousRun = {
2102+
runCreatedAt: 1_000,
2103+
runUpdatedAt: 30_000,
2104+
analyticsCapturedAt: 30_050,
2105+
telemetry: {
2106+
startRequestedAt: 1_100,
2107+
attemptStartedAt: 20_000,
2108+
attemptIndex: 2,
2109+
stdinWriteEndAt: 20_500,
2110+
firstModelEventAt: 21_000,
2111+
firstModelResponseAt: 21_000,
2112+
firstModelEventType: 'text_delta' as const,
2113+
firstTokenAt: 21_000,
2114+
},
2115+
events: [
2116+
// Attempt 1 opened `call_0` and was killed before it returned.
2117+
{ id: 1, event: 'agent', timestamp: 3_000, data: { type: 'tool_use', id: 'call_0', name: 'Bash' } },
2118+
// Attempt 2's fresh session restarts sequential ids and opens the same
2119+
// one for a different call.
2120+
{ id: 2, event: 'agent', timestamp: 21_500, data: { type: 'tool_use', id: 'call_0', name: 'Read' } },
2121+
// A result arrives. Nothing in the event log says which of the two it
2122+
// belongs to, and the two readings differ by 8s of occupancy.
2123+
{ id: 3, event: 'agent', timestamp: 22_000, data: { type: 'tool_result', toolUseId: 'call_0' } },
2124+
],
2125+
};
2126+
2127+
it('withholds the bottleneck when a result cannot be attributed to an attempt', () => {
2128+
const result = summarizeRunTimingAnalytics(ambiguousRun);
2129+
2130+
expect(result.bottleneck_phase).toBeUndefined();
2131+
});
2132+
2133+
it('marks phase timing partial rather than complete', () => {
2134+
const result = summarizeRunTimingAnalytics(ambiguousRun);
2135+
2136+
expect(result.phase_timing_status).toBe('partial');
2137+
});
2138+
2139+
it('still reports mark-derived timings', () => {
2140+
const result = summarizeRunTimingAnalytics(ambiguousRun);
2141+
2142+
expect(result.model_active_duration_ms).toBe(9_000);
2143+
});
2144+
});

apps/daemon/tests/run-lifecycle-tracer.test.ts

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { describe, expect, it } from 'vitest';
1+
import { afterEach, describe, expect, it, vi } from 'vitest';
22
import {
33
createRunLifecycleTracer,
44
runLifecycleMarkersForStreamEvent,
@@ -27,21 +27,30 @@ describe('runLifecycleMarkersForStreamEvent', () => {
2727

2828
describe('createRunLifecycleTracer', () => {
2929
it('only records first timestamps for repeated lifecycle marks', () => {
30+
vi.useFakeTimers();
31+
vi.setSystemTime(new Date('2026-08-20T00:00:10.000Z'));
32+
const arrivedAt = Date.now();
3033
const run = {};
3134
const lifecycle = createRunLifecycleTracer(run);
3235

3336
lifecycle.mark('first_artifact_write', 1_000);
3437
lifecycle.mark('first_artifact_write', 2_000);
38+
// Second argument is the producer's start, not the mark's timestamp.
3539
lifecycle.markFirstModelEvent('tool_use', 3_000);
3640
lifecycle.markFirstModelEvent('text_delta', 4_000);
3741

3842
expect(run).toEqual({
3943
analyticsTelemetry: {
4044
firstArtifactWriteAt: 1_000,
41-
firstModelEventAt: 3_000,
45+
// Arrival, first-write-wins -- the repeat does not overwrite it.
46+
firstModelEventAt: arrivedAt,
4247
firstModelEventType: 'tool_use',
48+
// Earliest producer start wins, so the 4_000 repeat does not win here
49+
// either.
50+
firstModelResponseAt: 3_000,
4351
},
4452
});
53+
vi.useRealTimers();
4554
});
4655
});
4756

@@ -132,7 +141,7 @@ describe('createRunLifecycleTracer first model event ordering', () => {
132141

133142
// First-write-wins would anchor at 200 and lose the 100ms head start,
134143
// pushing every phase boundary later.
135-
expect(run.analyticsTelemetry?.firstModelEventAt).toBe(100);
144+
expect(run.analyticsTelemetry?.firstModelResponseAt).toBe(100);
136145
expect(run.analyticsTelemetry?.firstModelEventType).toBe('tool_use');
137146
});
138147

@@ -143,7 +152,34 @@ describe('createRunLifecycleTracer first model event ordering', () => {
143152
tracer.markFirstModelEvent('tool_use', 100);
144153
tracer.markFirstModelEvent('text_delta', 300);
145154

146-
expect(run.analyticsTelemetry?.firstModelEventAt).toBe(100);
155+
expect(run.analyticsTelemetry?.firstModelResponseAt).toBe(100);
147156
expect(run.analyticsTelemetry?.firstModelEventType).toBe('tool_use');
148157
});
149158
})
159+
160+
describe('createRunLifecycleTracer keeps the legacy model-event mark intact', () => {
161+
afterEach(() => {
162+
vi.useRealTimers();
163+
});
164+
165+
it('records arrival for firstModelEventAt and the producer start separately', () => {
166+
vi.useFakeTimers();
167+
vi.setSystemTime(new Date('2026-08-20T00:00:20.000Z'));
168+
const arrivedAt = Date.now();
169+
const producerStartedAt = arrivedAt - 16_000;
170+
const run: { analyticsTelemetry?: Record<string, unknown> | null } = {};
171+
const tracer = createRunLifecycleTracer(run as never);
172+
173+
// ACP emits the canonical tool_use at terminal status, so this arrives at
174+
// 20s carrying a first-frame time of 4s.
175+
tracer.markFirstModelEvent('tool_use', producerStartedAt);
176+
177+
// `time_to_first_model_event_ms` is built from this field and is already
178+
// published. It must keep meaning "when we saw the first model event",
179+
// or every dashboard reading it silently shifts.
180+
expect(run.analyticsTelemetry?.firstModelEventAt).toBe(arrivedAt);
181+
expect(run.analyticsTelemetry?.firstModelEventType).toBe('tool_use');
182+
// The phase anchor is a separate mark: when the model actually began.
183+
expect(run.analyticsTelemetry?.firstModelResponseAt).toBe(producerStartedAt);
184+
});
185+
});

packages/contracts/src/analytics/events/result-events.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -517,7 +517,7 @@ export interface RunFinishedProps extends Omit<RunCreatedProps, 'area'> {
517517
// Runtime init measured to the first model event of any kind rather than to
518518
// the first text token. On a tool-first run the first-token variant absorbs
519519
// the whole tool loop and reads as slow startup.
520-
runtime_init_to_first_model_event_ms?: number;
520+
runtime_init_to_first_model_response_ms?: number;
521521
spawn_to_first_token_ms?: number;
522522
time_to_first_artifact_ms?: number;
523523
// `spawn_to_first_token_ms` split into auditable subsegments so dashboards

0 commit comments

Comments
 (0)