Skip to content

fix(auto-routing-benchmark): isolate dead-lettered lanes so one model cannot wedge a run - #5135

Merged
iscekic merged 4 commits into
mainfrom
bench-lane-failure-2c4a
Aug 7, 2026
Merged

iscekic merged 4 commits into
mainfrom
bench-lane-failure-2c4a

Conversation

@iscekic

@iscekic iscekic commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

Today a decider run wedges when a queue message exhausts its 6 retries and dead-letters: the lane's chunk chain is gone, finalizeRunIfComplete waits forever for rows that can never exist, and the 6h stale sweep then fails the entire run — including every cleanly measured model. Observed in prod: profile run profile-2026-08-07T09-24-27-661Z stuck for hours with 32/33 entries fully measured; only cohere/north-mini-code:free dead-lettered.

The worker now consumes auto-routing-benchmark-dlq itself. Each dead message is recorded in a new run_lane_failures table, and run finalization becomes lane-aware: a run completes once every (model, variant, rep) lane has either all its case rows or a recorded lane death.

  • Profile runs complete per-entry: finished entries go ready; entries with a dead lane go failed (owner Retry re-admits, quota-charged). One failing model no longer fails the other owners' entries.
  • Platform runs fail fast with the dead lane named in the run error. No partial publish — a silently missing candidate would drop it from routing decisions; the previous routing table stays live.
  • The 6h stale sweep stays as backstop for deaths that never reach the DLQ (e.g. a throwing consumer); the DLQ handler never throws and always acks.

Verification

  • Automated only: 21 files / 249 tests pass, incl. new honest-SQLite tests for the idempotent insert and per-entry no-clobber guard, and dead-letter flow tests (per-entry profile completion, platform fail-fast, rows-win-over-failure-record, terminal-run skip).
  • Not manually tested against a live queue (requires a deploy); risk is contained to the new DLQ consumer and finalize accounting.

Visual Changes

N/A

Reviewer Notes

  • Deploy applies migration 0008_easy_doctor_faustus.sql via predeploy.
  • On deploy, the DLQ consumer will drain the current incident's 3 dead cohere messages (still in the DLQ, 4-day retention): the wedged profile run will then complete automatically with 32 entries ready and cohere failed.
  • Dead-lane resurrection (re-enqueue once before declaring death) was deliberately deferred to keep this change tight.

… cannot wedge a run

A benchmark queue message that exhausted its 6 retries dead-lettered
silently: its lane's chunk chain was gone, finalizeRunIfComplete waited
forever for rows that can never exist, and the 6h stale sweep then failed
the whole run — including every cleanly measured model.

Consume the DLQ in the worker itself. Each dead message is recorded in the
new run_lane_failures table and finalization becomes lane-aware: a run
completes once every (model, variant, rep) lane has either all its case
rows or a recorded lane death.

- Profile runs complete per-entry: finished entries go ready, entries with
  a dead lane go failed (retry re-admits them, quota-charged).
- Platform runs fail fast with the dead lane named; publishing a partial
  routing table would silently drop a candidate from routing decisions.
- The 6h stale sweep stays as backstop for deaths that never reach the DLQ.
@iscekic iscekic self-assigned this Aug 7, 2026
@iscekic
iscekic enabled auto-merge (squash) August 7, 2026 12:56
@iscekic
iscekic disabled auto-merge August 7, 2026 12:56
@kilo-code-bot

kilo-code-bot Bot commented Aug 7, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

Incremental review of the three follow-up commits since ed0198a: all previous findings are verifiably fixed — the DLQ handler now propagates processDeadLetter errors so queue retries re-attempt finalization (a5502bc), lane/entry identity keys are collision-safe JSON tuples (ff8b97a), and the per-entry fail statement builder throws on empty entries instead of degrading to the whole-run form (a42c9e7) — and no new issues were found in the changed code, including memory-leak checks (per-invocation structures only, no global state).

Files Reviewed (7 files)
  • services/auto-routing-benchmark/src/db.ts - previous finding resolved (builder throws on empty entries)
  • services/auto-routing-benchmark/src/index-queue.test.ts - no issues (new retry-contract tests match handler behavior)
  • services/auto-routing-benchmark/src/index.ts - previous finding resolved (errors propagate, ack only after success; formatError import cleanly removed)
  • services/auto-routing-benchmark/src/lane-failure-sql.test.ts - no issues
  • services/auto-routing-benchmark/src/run-dead-letter.test.ts - no issues (collision regression test is sound)
  • services/auto-routing-benchmark/src/run.ts - previous finding resolved (JSON tuple keys used consistently on both write and lookup sides)
  • services/auto-routing-benchmark/wrangler.jsonc - no issues (comment now accurately describes the retry contract)
Previous Review Summary (commit ed0198a)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit ed0198a)

Status: 3 Issues Found | Recommendation: Address before merge

Executive Summary

The DLQ consumer's catch-and-always-ack handler defeats its own max_retries: 2 config, so a transient failure after the lane-death record commits can still leave the run wedged until the 6h stale sweep fails the entire run — the incident shape this PR sets out to eliminate; two minor hardening suggestions accompany it. No memory leaks introduced (per-invocation Maps only, no global state).

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 2
Issue Details (click to expand)

WARNING

File Line Issue
services/auto-routing-benchmark/src/index.ts 42 Swallowed error + unconditional ack make DLQ max_retries ineffective; transient failure after recordLaneFailure but before finalization leaves the run to the 6h stale sweep, which fails the whole run despite the recorded lane death

SUGGESTION

File Line Issue
services/auto-routing-benchmark/src/run.ts 1222 Space-joined laneKey can collide; codebase convention is collision-safe poolEntryKey (JSON-encoded tuple)
services/auto-routing-benchmark/src/db.ts 667 Exported markProfilesFailedForEntriesStatement with empty entries degrades to failing all of the run's running entries; guard belongs in the builder, not only the wrapper
Files Reviewed (13 files)
  • services/auto-routing-benchmark/README.md - no issues (docs-only, no images added)
  • services/auto-routing-benchmark/migrations/0008_easy_doctor_faustus.sql - no issues (new table, no locking DDL on populated tables)
  • services/auto-routing-benchmark/migrations/meta/0008_snapshot.json - skipped (generated)
  • services/auto-routing-benchmark/migrations/meta/_journal.json - skipped (generated)
  • services/auto-routing-benchmark/src/db-schema.ts - no issues
  • services/auto-routing-benchmark/src/db.ts - 1 issue
  • services/auto-routing-benchmark/src/index.ts - 1 issue
  • services/auto-routing-benchmark/src/lane-failure-sql.test.ts - no issues
  • services/auto-routing-benchmark/src/profile-runs.test.ts - no issues
  • services/auto-routing-benchmark/src/run-dead-letter.test.ts - no issues
  • services/auto-routing-benchmark/src/run-process-job.test.ts - no issues
  • services/auto-routing-benchmark/src/run.ts - 1 issue
  • services/auto-routing-benchmark/wrangler.jsonc - no issues (comment on DLQ consumer retries is misleading, covered by the index.ts finding)

Fix these issues in Kilo Cloud


Reviewed by kimi-k3 · Input: 42.5K · Output: 10.3K · Cached: 313.6K

Review guidance: REVIEW.md from base branch main

iscekic added 3 commits August 7, 2026 15:04
…mpt finalization

The swallowed error + unconditional ack made the DLQ consumer's
max_retries ineffective: if recordLaneFailure committed but
finalizeRunIfComplete threw transiently, nothing re-attempted
finalization and the run wedged until the 6h stale sweep failed it
wholesale — the exact failure mode this PR removes. processDeadLetter now
propagates transient errors (same ack contract as the jobs branch); a
message still failing after max_retries is dropped and the stale sweep
remains the backstop.
…dentity

Space-joined keys collide when a model or variant contains a space (the
pool schema allows any trimmed non-empty variant). Match the codebase's
collision-safe key convention with JSON.stringify tuples, with a
regression test where space-joined keys would collapse two entries.
…fail statement builder

With entries = [], or(...[]) evaluates to undefined and the WHERE degrades
to run_id + status='running', which would fail every running entry of the
run. The wrapper already early-returns; the exported builder now refuses
the destructive form so a future direct caller cannot produce it silently.
@iscekic
iscekic merged commit 92633a3 into main Aug 7, 2026
13 of 15 checks passed
@iscekic
iscekic deleted the bench-lane-failure-2c4a branch August 7, 2026 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants