Skip to content

Commit 6b32405

Browse files
committed
fix(webapp): don't notify trace subscribers when nothing landed
Swallowing an unrecoverable batch instead of throwing lost the one thing the throw was still doing: skipping the Redis publish. The flush therefore told live trace and log views that new events had arrived, and logged the batch as inserted, when ClickHouse had rejected every row. Subscribers refetched and found nothing. Both event flush paths now check the outcome and return before publishing when it reports the whole batch dropped. The check requires an exact dropped count, so a partial drop on a table whose count is only a floor still publishes the rows that did land.
1 parent a20aed2 commit 6b32405

3 files changed

Lines changed: 73 additions & 0 deletions

File tree

apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ import type {
6969
import {
7070
insertWithBadRowSkip,
7171
type JsonParseRecoveryOutcome,
72+
landedNothing,
7273
} from "./sanitizeRowsOnParseError.server";
7374

7475
export type ClickhouseEventRepositoryConfig = {
@@ -330,6 +331,10 @@ export class ClickhouseEventRepository implements IEventRepository {
330331
});
331332
this.#recordRecoveryOutcome(outcome, contextLabel, events.length);
332333

334+
if (landedNothing(outcome, events.length)) {
335+
return;
336+
}
337+
333338
logger.debug("ClickhouseEventRepository.flushBatch Inserted batch into clickhouse", {
334339
events: events.length,
335340
outcome: outcome.kind,
@@ -367,6 +372,10 @@ export class ClickhouseEventRepository implements IEventRepository {
367372
});
368373
this.#recordRecoveryOutcome(outcome, "llm_metrics_v1", rows.length);
369374

375+
if (landedNothing(outcome, rows.length)) {
376+
return;
377+
}
378+
370379
logger.debug("ClickhouseEventRepository.flushLlmMetricsBatch Inserted LLM metrics batch", {
371380
rows: rows.length,
372381
outcome: outcome.kind,

apps/webapp/app/v3/eventRepository/sanitizeRowsOnParseError.server.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,20 @@ export type JsonParseRecoveryOutcome =
232232
bailReason?: RecoveryBailReason;
233233
};
234234

235+
/**
236+
* True when a recovery landed no rows at all, so nothing reached ClickHouse.
237+
*
238+
* Callers must not treat such a flush as a successful insert: there is no new
239+
* data for subscribers to read, so notifying them only causes refetches that
240+
* find nothing. Requires an exact dropped count, so a batch whose partial-drop
241+
* count is only a floor is never mistaken for a total loss.
242+
*/
243+
export function landedNothing(outcome: JsonParseRecoveryOutcome, batchSize: number): boolean {
244+
return (
245+
outcome.kind === "recovered" && outcome.rowsDroppedExact && outcome.rowsDropped >= batchSize
246+
);
247+
}
248+
235249
/**
236250
* Default number of poison rows to isolate-and-strip precisely before bailing
237251
* to a single `allow_errors` skip insert. One covers the common case (a single

apps/webapp/test/sanitizeRowsOnParseError.test.ts

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
insertWithBadRowSkip,
55
insertWithLimitedStrip,
66
isClickHouseJsonParseError,
7+
landedNothing,
78
parseRowNumberFromError,
89
parseStrippableRowNumber,
910
sanitizeRows,
@@ -107,6 +108,55 @@ describe("parseRowNumberFromError", () => {
107108
});
108109
});
109110

111+
describe("landedNothing", () => {
112+
it("is true only when an exact dropped count covers the whole batch", () => {
113+
expect(
114+
landedNothing(
115+
{
116+
kind: "recovered",
117+
rowsStripped: 0,
118+
rowsDropped: 3,
119+
rowsDroppedExact: true,
120+
capped: true,
121+
},
122+
3
123+
)
124+
).toBe(true);
125+
expect(
126+
landedNothing(
127+
{
128+
kind: "recovered",
129+
rowsStripped: 0,
130+
rowsDropped: 2,
131+
rowsDroppedExact: true,
132+
capped: false,
133+
},
134+
3
135+
)
136+
).toBe(false);
137+
});
138+
139+
it("is false when the dropped count is only a floor, so a partial drop is never read as total loss", () => {
140+
expect(
141+
landedNothing(
142+
{
143+
kind: "recovered",
144+
rowsStripped: 0,
145+
rowsDropped: 1,
146+
rowsDroppedExact: false,
147+
capped: false,
148+
},
149+
1
150+
)
151+
).toBe(false);
152+
});
153+
154+
it("is false for a healthy or sanitized insert", () => {
155+
expect(landedNothing({ kind: "inserted", insertResult: {} }, 5)).toBe(false);
156+
expect(landedNothing({ kind: "sanitized", insertResult: {} }, 5)).toBe(false);
157+
});
158+
});
159+
110160
describe("parseStrippableRowNumber", () => {
111161
it("reads the parenthesised position ClickHouse appends", () => {
112162
expect(

0 commit comments

Comments
 (0)