Repository navigation
Conversation
…able A dead server during the ClickBench Parquet lane's concurrency phase kept every task cycling the suite against a refused connection, producing millions of "engine unreachable" errors and one D7 per-statement violation for each of 39 statements. The phase now keeps the typed EngineError in each outcome. A connection level failure (EngineError::Unreachable: the request could not be sent, or the connection was refused or dropped) sets a shared stop that every task reads before starting a statement, so no statement starts after the first one. A statement in flight then finishes, answered or failed the same way, so at most one connection failure per task is counted. An HTTP error answer of any status, 422 and 503 included, stays a statement error and the phase runs its duration. The figures keep everything up to the stop and add engine_unreachable (time into the phase, task, statement and error text) and per-statement and total unreachable counts, which errors includes. The bench prints the line once on stderr and records it as concurrency_error, so the D7 check reports one ConcurrencyPhaseFailed violation through the existing path. The per-statement rule, errored_statements and first_error count statement errors only. qps is 0 when the phase ended at zero elapsed time. Tests use scripted clocks and no sleeps: a server refusing after N answers ends the phase at the first refusal with errors at most the task count and exactly one D7 violation, and a 503 counts as a statement error with no concurrency_error. Step 7 of the lane's runbook describes the early end and what the report carries. Fixes: #2632 Refs: #2040 Changelog: none Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
|
@claude-fleet review |
|
Review task queued. Task: 8b0c8507-9bed-4a4c-b2b3-0cfc554debd8 |
![claude-fleet[bot]](https://avatars.githubusercontent.com/in/4876296?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: two should_fix findings. The core mechanism is sound and the new tests are real regression tests (both fail on the unfixed code), but the "one violation for one event" property does not hold when the engine is already dead at the phase's start, and ADR-2040 still states the old rule 2 the diff narrows.
Verified:
EngineError::Unreachableon the HTTP path comes only from a request that could not be sent and a body that could not be read (engine.rs:322, 327); every status the server answers with goes throughcheck_statusintoQuery/Ddl(engine.rs:360-364), so "HTTP error answers are unchanged, 422 and 503 included" holds.- At most one connection-level failure per task: a task that records one re-enters the loop and breaks on
stop.get()(concurrency.rs:205-221), sounreachable <= tasksas the field doc claims. - All concurrency tasks share one
Arc<dyn Clock>(clickbench_parquet_bench.rs:322-327), soat_s = stop.at - phase_startis a difference on one clock, not a mix. - A connection-level failure leaves
errored_statementsandfirst_erroralone (concurrency.rs:318-325, 370), andstatement_errors()is what the per-statement D7 rule reads (report.rs:846). unreachableandengine_unreachablecarry#[serde(default)], so a report from before this change still parses.
Checks run:
cargo test -p ravel-bench --features sql-latency --lib clickbench_parquet::concurrency→ 11 passed.- Reverted
|| stop.get().is_some()at concurrency.rs:207, reran:a_refused_connection_stops_every_taskandrequests_in_flight_when_the_engine_dies_fail_the_same_wayboth fail withdriver kept starting statements(the 1,000-request cap). Restored;git status --porcelainclean. cargo test -p ravel-bench --features sql-latency --bin clickbench_parquet_bench -- engine_that_refuses http_error_from_a_serving→ 2 passed.cargo test -p ravel-bench --features sql-latency --lib clickbench_parquet::report→ 36 passed.
Smaller things:
- The
Stopdoc comment (concurrency.rs:171) says "The first connection-level failure any task saw". When several tasks fail in the same instant it is whichever task winsOnceLock::set, not necessarily the earliestended. The spread is sub-millisecond, so this is wording, not behaviour. - The summary line at clickbench_parquet_bench.rs:337-347 prints
error_ratioagainstRLOG_ERROR_RATIOfor comparison, and after an early-ended phase that ratio is mostly connection failures over a truncated window, so the comparison the runbook asks for has no meaning there. The unreachable line printed right after does say why, so this is only a presentation point.
Not checked:
- Whether the real server's 503/422 answers reach the bench as HTTP responses rather than as dropped connections under load; only the error mapping in engine.rs was read.
- The #2055 M1 run figures quoted in the description (52 to 61 million errors); no run artifact was available here.
- CI's full ravel-bench feature lanes (sql-latency, profiling, flight-lane) beyond the targeted tests listed above.
Task 8b0c8507-9bed-4a4c-b2b3-0cfc554debd8 on ci-16gb-fsn1-1, model claude-opus-5, effort medium.
| errors: error_total, | ||
| qps: queries_completed as f64 / elapsed.as_secs_f64(), | ||
| unreachable: figures.iter().map(|s| s.unreachable).sum(), | ||
| qps: if elapsed.is_zero() { |
There was a problem hiding this comment.
should_fix: The qps rule still judges a phase the engine ended, so a dead engine can produce two violations rather than the one the PR and the runbook claim.
Failure: the server dies during the serial phase, so every task's first concurrency request is refused. queries_completed is 0, qps is 0.0 (0 completed over any window, and the elapsed.is_zero() arm gives 0.0 as well), and check_against pushes ConcurrencyQpsBelowFloor { qps: 0.0, floor: 0.400 } on top of ConcurrencyPhaseFailed. The D7 output then reads as a throughput miss as well as a dead engine, which is the same shape as the #2632 noise, smaller. The same holds whenever the engine dies before the surviving completions over the truncated window reach 0.400 qps.
Evidence: concurrency.rs:356-362 computes qps over a window the stop can truncate; report.rs:834-840 applies the floor to report.concurrency unconditionally, before the concurrency_error arm at report.rs:855-859; docs/internal/clickbench.md:1186 states the line "the D7 check reports as one violation". The new an_unreachable_engine_is_one_violation_however_many_statements_it_failed test does not catch it because clean_report() keeps qps: 1.0 (report.rs:1127-1133) and only mutates the per-statement counts.
Fix: skip the qps rule when concurrency.engine_unreachable.is_some(), the way the per-statement rule now skips connection-level failures.
| D7's concurrency bar is two rules (issue #2055): `qps` at least | ||
| `concurrency_qps_floor` (0.400), and no error from any statement outside | ||
| `failures`. The error ratio is not judged: the five registered failures | ||
| `concurrency_qps_floor` (0.400), and no statement error from any statement |
There was a problem hiding this comment.
should_fix: The runbook now states D7's rule 2 as "no statement error from any statement outside failures", but the normative sources for that bar still state it as "zero errors" and are not touched by this PR.
Failure: a reader (or a later change) checking the bar against ADR-2040 finds rule 2 written as "zero errors from any statement outside the pre-registered failure set" (docs/adrs/2040-parquet-tables-queried-in-place.md:1208) and the same wording in benchmarks/clickbench/parquet/prereg.toml:35, both of which the code at report.rs:846 no longer implements. The ADR's supporting argument goes stale with it: "rule 2 already fails the run on the first error from any of them, so whenever rule 2 passes that ratio is 0" (ADR:1221-1222) is false once connection-level failures are excluded from rule 2, since rule 2 can now pass with a nonzero error count on statements outside the set.
Evidence: report.rs:846 (statement.statement_errors()); ADR 2040:1204-1224; prereg.toml:33-35.
Fix: amend ADR-2040's concurrency bar amendment (the normative doc per CLAUDE.md's doc map) to say rule 2 counts statement errors and that a connection-level failure is judged once through concurrency_error, and update the prereg.toml comment with it.
| } | ||
| let index = (offset + k) % len; | ||
| let result = task.engine.query(&statements[index].sql).await; | ||
| ended = task.clock.now(); |
There was a problem hiding this comment.
nit: One transient connection-level failure now ends the whole phase, with no retry anywhere under it.
Failure: a pooled keep-alive connection the server closes while a POST is in flight surfaces from client.execute as a send error, which engine.rs:322 maps to EngineError::Unreachable. The first such error sets stop, a 600 s phase can end seconds in, and D7 records ConcurrencyPhaseFailed for a server that is still serving. HttpEngine builds a plain Client::new() (engine.rs:308-314) with no retry, so nothing distinguishes a one-off drop from a dead server.
Evidence: engine.rs:318-328, concurrency.rs:212-221. The failure direction is loud rather than silent, which is why this is a nit and not more: it costs a rerun, not a wrong verdict. Worth saying in the runbook that a single dropped connection ends the phase, or retrying a send error once before setting the stop.
A server that died during the timed statements refused every task's first concurrency request, so the phase completed nothing and its qps was 0. The D7 check then reported a qps violation on top of ConcurrencyPhaseFailed, judging a phase the engine ended as if it had measured the engine. check_against now skips the qps rule when the concurrency figures carry engine_unreachable, the same way the per-statement rule already leaves connection-level failures to concurrency_error. Every other report keeps the qps rule, including one whose concurrency_error is set for another reason. A test builds the dead-before-the-phase report and asserts exactly one violation, ConcurrencyPhaseFailed; a sibling asserts that qps 0.1 with engine_unreachable unset is still a qps violation, with and without an unrelated concurrency_error. ADR-2040 gains an amendment: rule 2 counts statement errors (HTTP error answers) only, a connection-level failure ends the phase and is one ConcurrencyPhaseFailed violation with no qps or per-statement violation beside it, the bench does not retry so one dropped connection ends the phase, and the 2026-10-05 amendment's error-ratio argument holds for statement errors only. The 2026-10-05 section points to it beside rule 2 and the sentence it qualifies. prereg.toml's rule 2 comment and step 7 of the ClickBench runbook say the same, and the Stop doc comment says it holds the failure recorded first rather than the first one seen. Refs: #2632 Refs: #2040 Changelog: none Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
|
@claude-fleet review |
|
Review task queued. Task: 68898020-29ac-4145-8c2b-864e92e77432 |
There was a problem hiding this comment.
Restored the working tree after both revert checks (git status --porcelain shows only the untracked node_modules).
Approve
The fix does what the description says. The stop is set from the typed EngineError::Unreachable only, every task reads it before starting a statement, and in-flight statements still land, so the per-task bound on connection-level failures holds. HTTP error answers keep going through EngineError::Query and leave the phase running. The three findings from the previous round are addressed: the qps rule now skips a phase the engine ended, ADR-2040 carries the amendment with working integrity markers, and the no-retry consequence is written into both the ADR and the runbook.
Verified:
- Only
HttpEngine::sendcan produceUnreachablefrom aquerycall (engine.rs:322,engine.rs:327).InProcessEngine::querymaps every failure toQuery(engine.rs:242-258) andReferenceEngine'sUnreachableuses are all innew/create_view_statement(engine.rs:641-679), so no in-process run can end the phase by accident. - Any non-2xx status becomes
Call::Query's error, so 422 and 503 stay statement errors (engine.rs:360-364,engine.rs:412-416). - Each task records at most one connection-level failure: it sets
stopafter the failing request and the loop head breaks on the next iteration (concurrency.rs:205-221), which backs theat most tasksclaim onConcurrencyFigures::unreachable(concurrency.rs:136-139). statement_errors()cannot disagree withfirst_error:first_erroris only set on the non-Unreachablearm (concurrency.rs:324-329), so a statement withstatement_errors() > 0always has one, anderrored_statementsmatches (concurrency.rs:370).engine_unreachableandconcurrency_errorare always set together, from the sameDisplaystring (clickbench_parquet_bench.rs:349-353), so the qps skip atreport.rs:837never leaves a dead engine unjudged.- New serde fields all carry
#[serde(default)], so a report written by the previous bench still deserializes withstatement_errors() == errors. - No other code or script reads
concurrency.*from the report:grepovercrates/,scripts/,docs/,benchmarks/finds only the ADR, the runbook andprereg.toml.
Checks run:
cargo test -p ravel-bench --features sql-latency --lib clickbench_parquet::concurrency→ 11 passed.- Reverted the stop check at
concurrency.rs:208toif started >= deadline {→a_refused_connection_stops_every_taskandrequests_in_flight_when_the_engine_dies_fail_the_same_wayboth fail withdriver kept starting statements(the 1000-request cap), 9 others still pass. Restored. cargo test -p ravel-bench --features sql-latency --lib clickbench_parquet::reportwithengine_unreachable.is_none() &&dropped atreport.rs:837andstatement_errors()back toerrorsatreport.rs:849→ the three new tests fail (an_unreachable_engine_is_one_violation_however_many_statements_it_failedreports 11 violations where 1 is expected). Restored.cargo test -p ravel-bench --features sql-latency --bin clickbench_parquet_bench→ 11 passed, including both new end-to-end tests.scripts/guards/check-amendment-integrity.sh→ clean, 269 amendments over 175 files.scripts/guards/check-changelog-touched.sh a39e966 HEAD→ clean via theChangelog: nonetrailer.
Smaller things:
SharedServer/Connection/Requestinconcurrency.rs:601-699andScriptedServer/ServerConnection/Requestinclickbench_parquet_bench.rs:716-836are the same scripted server written twice, differing only indiesand theunavailableargument type. The bin cannot reach the lib's#[cfg(test)]items, so this is a cost of the target split rather than an oversight, but apubtest-support module behindsql-latencywould remove about 110 duplicated lines.REFUSED(clickbench_parquet_bench.rs:717-718) uses a\line continuation, which eats the leading whitespace of the next line, so the constant reads...for url(http://...)without the space reqwest actually emits. Harmless, since the test compares the constant with itself, but the doc comment above it claims it is the textHttpEngineproduces.- The qps skip keys on
engine_unreachablewhile the violation that replaces it comes fromconcurrency_error, and onlymeasureties the two together. Nothing callscheck/check_againston a report loaded from disk today, so there is no live path where the pair can come apart.
Not checked:
- Whether any ADR-2040 reader outside the two sections the amendment markers name relies on the pre-amendment wording of rule 2; only the markered sections were read.
- Behavior against a real ravel-server: both new tests drive a scripted in-process engine, so reqwest's actual mapping of a dropped keep-alive connection to a send error is taken from engine.rs:322 rather than observed.
Task 68898020-29ac-4145-8c2b-864e92e77432 on rp2, model claude-opus-5, effort medium.
| read) is not a statement error. It ends the phase: no task starts a | ||
| statement after it. D7 reports it once, as `ConcurrencyPhaseFailed`, | ||
| with no queries-per-second violation and no per-statement violation | ||
| beside it, since the phase's qps then measures the outage, not the |
There was a problem hiding this comment.
nit: The amendment's point 2 says a connection-level failure is reported "with no queries-per-second violation and no per-statement violation beside it". The first clause is unconditional in the code; the second is not.
Failure: a statement outside failures answers one HTTP 503 at 100 s and the server then dies at 300 s. statement_errors() for that statement is 1, so report.rs:850-856 pushes ConcurrencyUnregisteredError and report.rs:860-864 pushes ConcurrencyPhaseFailed. A reader holding the ADR to its word sees two violations where it promises one, and a later change made to match the sentence would drop a real statement error.
Evidence: report.rs:837-839 makes the qps skip unconditional on engine_unreachable, while report.rs:849-856 applies the per-statement rule regardless; the PR's own test a_statement_error_beside_connection_failures_is_still_named (report.rs:1559-1578) asserts that a statement with both error kinds still yields ConcurrencyUnregisteredError { number: 8, errors: 2 }.
Fix: scope the clause to the failure itself, e.g. "and no per-statement violation arising from it".
|
Review round 2 approves the fix and finds only nits, so this merges at The two should_fix comments shown against this head are the round-1 findings re-posted. Commit
On the nits:
|
1 participant
When the server died mid-way through the ClickBench Parquet lane's concurrency phase, the bench kept cycling statements against it with no backoff. The #2055 M1 run recorded 52 to 61 million "engine unreachable" errors, 1.2 to 1.4 million per statement. The report's
concurrency_errorstayed empty, and the D7 check listed 39 per-statement violations for what was one event (#2632).What changed
EngineError::Unreachable(a request that could not be sent, or a body that could not be read) is stored in a shared stop that every task checks before it starts a statement.concurrency_errornaming the time into the phase, the task and statement that saw it first, and the error.ConcurrencyPhaseFailedpath.Tests
an_engine_that_refuses_connections_ends_the_phase_for_every_task:a_refused_connection_stops_every_task: the same property at the library level (8 against 6).Fixes: #2632
Refs: #2040