Skip to content

Commit 6f7cf13

Browse files
author
Drew Stone
committed
fix(trace-store): merge _update span patches on FileSystemTraceStore.load
When a fresh FileSystemTraceStore opens a directory written by a prior process, load() calls appendSpan() for every NDJSON row — including the { ...patch, _update: true } fragments that updateSpan writes. Those fragments lack kind / runId / name / model, so the reader ended up with two spans per id (full row + fragment), breaking any consumer that re-opens the store cross-process (e.g. tax-agent's canonical eval OTLP converter, which then reported 0 spans to the trace analyst). The runs branch already handled this via appendRun-then-updateRun fallback. Mirror the same pattern for spans: detect \_update and route to updateSpan, merging into the prior span. New regression test reads the dir from a second FileSystemTraceStore instance and asserts the patch fields are applied without losing kind / runId / name.
1 parent 544fa69 commit 6f7cf13

2 files changed

Lines changed: 50 additions & 1 deletion

File tree

src/trace/store.ts

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,25 @@ export class FileSystemTraceStore implements TraceStore {
268268
await store.updateRun(record.runId, record)
269269
}
270270
} else if (base === 'spans') {
271-
await store.appendSpan(record)
271+
// `updateSpan` appends an `_update: true` patch row instead of
272+
// rewriting the original span. On reload we must collapse those
273+
// patches into the prior span — otherwise a fresh
274+
// FileSystemTraceStore reading the same dir reports duplicate
275+
// spans (one full, one fragment with no runId/kind/name), which
276+
// breaks any downstream consumer that re-opens the store
277+
// cross-process (e.g. the canonical eval's OTLP converter).
278+
if (record?._update) {
279+
try {
280+
await store.updateSpan(record.spanId, record)
281+
} catch {
282+
// Patch row arrived before the original — should not happen
283+
// with locked append order, but fall through to append so we
284+
// don't lose data.
285+
await store.appendSpan(record)
286+
}
287+
} else {
288+
await store.appendSpan(record)
289+
}
272290
} else if (base === 'events') {
273291
await store.appendEvent(record)
274292
} else if (base === 'artifacts') {

tests/trace-store.test.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,37 @@ describe('FileSystemTraceStore', () => {
139139
expect(spans).toHaveLength(1)
140140
expect(spans[0].endedAt).toBe(2)
141141
})
142+
143+
it('cross-process reload merges _update span patches into the original span — regression: a fresh FileSystemTraceStore opened on a written dir reported each span twice (the original row + the patch fragment with no runId/kind/name), which broke OTLP conversion downstream (canonical eval analyst saw 0 spans)', async () => {
144+
const dir = await makeDir()
145+
146+
// First process: write run, append a full LLM span, then patch it via updateSpan.
147+
const writer = new FileSystemTraceStore({ dir })
148+
await writer.appendRun(makeRun('r1'))
149+
await writer.appendSpan({
150+
runId: 'r1',
151+
spanId: 's1',
152+
kind: 'llm',
153+
model: 'claude',
154+
messages: [{ role: 'user', content: 'hi' }],
155+
name: 'turn-1',
156+
startedAt: 1,
157+
})
158+
await writer.updateSpan('s1', { endedAt: 5, status: 'ok', output: 'merged-output' } as Partial<Span>)
159+
160+
// Second process: fresh store reads the dir from scratch (forces load()).
161+
const reader = new FileSystemTraceStore({ dir })
162+
const spans = await reader.spans({ runId: 'r1' })
163+
expect(spans).toHaveLength(1)
164+
const [s] = spans
165+
expect(s.spanId).toBe('s1')
166+
expect(s.runId).toBe('r1') // must survive the merge (patch row has no runId)
167+
expect(s.kind).toBe('llm') // must survive the merge (patch row has no kind)
168+
expect(s.name).toBe('turn-1') // must survive the merge (patch row has no name)
169+
expect(s.endedAt).toBe(5) // applied from patch
170+
expect(s.status).toBe('ok') // applied from patch
171+
expect((s as { output?: string }).output).toBe('merged-output')
172+
})
142173
})
143174

144175
describe('TraceEmitter', () => {

0 commit comments

Comments
 (0)