Skip to content

Commit bea811a

Browse files
matt-aitkenclaude
andcommitted
fix(webapp): don't count dropped batches as successful inserts in metrics
Devin caught this: when #insertWithJsonParseRecovery drops a batch (sanitizer no-op, or sanitize-retry still hit a parse error), #insertTaskRunInserts was previously converting `{kind: "dropped"}` to `undefined`, so #insertWithRetry saw `[null, undefined]` (no error) and #flushBatch ticked `_taskRunsInsertedCounter` / `_payloadsInsertedCounter` for rows that never landed in ClickHouse. Fix: return the recovery wrapper's outcome straight through. #flushBatch now reads the outcome and only increments the success counter when both `!err` AND `outcome?.kind !== "dropped"`. Matches the pattern in ClickhouseEventRepository where the caller explicitly bails on `outcome.kind === "dropped"` before downstream success work. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 8957a01 commit bea811a

1 file changed

Lines changed: 10 additions & 8 deletions

File tree

apps/webapp/app/services/runsReplicationService.server.ts

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -677,7 +677,7 @@ export class RunsReplicationService {
677677
combinedTaskRunInserts.push(...group.taskRunInserts);
678678
combinedPayloadInserts.push(...group.payloadInserts);
679679

680-
const [trErr] = await this.#insertWithRetry(
680+
const [trErr, trOutcome] = await this.#insertWithRetry(
681681
(attempt) => this.#insertTaskRunInserts(clickhouse, group.taskRunInserts, attempt),
682682
"task run inserts",
683683
flushId
@@ -686,7 +686,7 @@ export class RunsReplicationService {
686686
taskRunError = trErr;
687687
}
688688

689-
const [plErr] = await this.#insertWithRetry(
689+
const [plErr, plOutcome] = await this.#insertWithRetry(
690690
(attempt) => this.#insertPayloadInserts(clickhouse, group.payloadInserts, attempt),
691691
"payload inserts",
692692
flushId
@@ -695,10 +695,14 @@ export class RunsReplicationService {
695695
payloadError = plErr;
696696
}
697697

698-
if (!trErr) {
698+
// Only count rows that actually landed in ClickHouse. `kind: "dropped"`
699+
// means the recovery wrapper bailed (sanitizer no-op or sanitize-retry
700+
// still failed) — those rows never made it, so they must not show up
701+
// as successful inserts in the per-batch counter.
702+
if (!trErr && trOutcome?.kind !== "dropped") {
699703
this._taskRunsInsertedCounter.add(group.taskRunInserts.length);
700704
}
701-
if (!plErr) {
705+
if (!plErr && plOutcome?.kind !== "dropped") {
702706
this._payloadsInsertedCounter.add(group.payloadInserts.length);
703707
}
704708
}
@@ -872,13 +876,12 @@ export class RunsReplicationService {
872876
return insertResult;
873877
};
874878

875-
const outcome = await this.#insertWithJsonParseRecovery(
879+
return await this.#insertWithJsonParseRecovery(
876880
taskRunInserts,
877881
doInsert,
878882
"task_runs_v2",
879883
attempt
880884
);
881-
return outcome.kind === "dropped" ? undefined : outcome.insertResult;
882885
});
883886
}
884887

@@ -907,13 +910,12 @@ export class RunsReplicationService {
907910
return insertResult;
908911
};
909912

910-
const outcome = await this.#insertWithJsonParseRecovery(
913+
return await this.#insertWithJsonParseRecovery(
911914
payloadInserts,
912915
doInsert,
913916
"raw_task_runs_payload_v1",
914917
attempt
915918
);
916-
return outcome.kind === "dropped" ? undefined : outcome.insertResult;
917919
});
918920
}
919921

0 commit comments

Comments
 (0)