Repository navigation
fix(cli): propagate benchmark failures and isolate benchmark runs - #294
Conversation
Three benchmark defects, all of which let a run report success it did not earn: Report I/O (matrixorigin#270): cmd_benchmark wrote --out with `unwrap_or_else(|e| eprintln!(..))` and then printed "Saved" unconditionally, returning unit, so a failed write still exited 0. Scripts that archive reports by exit status silently kept nothing. Propagate the error and only print "Saved" after the write succeeds. Error accounting (matrixorigin#271): store() parsed the body without error_for_status and turned a missing memory_id into a successful empty string, so a 400/500 seed or store step counted as a success — a scenario could score 100/S with its setup entirely broken. retrieve() collapsed transport, HTTP and parse failures into an empty vector, indistinguishable from a valid empty answer, and the retrieve step could never fail. Both now return errors; the retrieve step propagates them; run_assertion records them; and an errored assertion scores zero instead of passing on empty expectations (recall and noise default to 100 when a scenario declares no expected/excluded contents). Failures are also printed as diagnostics instead of being discarded into unread fields. Run isolation (matrixorigin#273): run_id came from epoch *seconds* with no random component, so independent runs started in the same second shared a user namespace and could purge or correct each other's memories. It now mixes in random bits. Separately, validation checked scenario_id uniqueness case-sensitively while the executor lowercases it to build the namespace, so `CaseA` and `casea` validated as distinct and then collided; uniqueness is now checked on the same normalized form. Fixes matrixorigin#270 Fixes matrixorigin#271 Fixes matrixorigin#273 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed head Two medium-severity findings remain:
Validation: all 21 CLI tests passed on this head. The above cases were reproduced using the built CLI and a local HTTP fixture. Controls also confirmed that retrieval HTTP 500 produces failed/zero-scored results with diagnostics, an unwritable output destination exits nonzero without reporting success, and four concurrent runs receive distinct run namespaces. These checks did not use a live MatrixOne database. Coverage: 5/5 changed files reviewed; 0 skipped (100%). |
…d items
Follow-up to review on this PR; both findings reproduced before fixing.
The case-insensitive scenario_id check only ran under --validate-only, so
an ordinary `memoria benchmark` still executed a dataset containing both
`CaseA` and `casea` under one namespace (verified: two scenarios, one
X-Impersonate-User, exit 0). The invariant is now a reusable
`colliding_scenario_namespaces` helper that cmd_benchmark enforces before
executing anything, so the isolation fix cannot be bypassed by omitting
the flag. Only this invariant is enforced pre-run, not all of
validate_dataset, to avoid failing datasets over unrelated warnings that
run today.
retrieve() checked the top-level response shape but still used filter_map
over each item's `content`, so an entry without a string content was
silently discarded: a 200 returning
`[{"content":"memory"},{"memory_id":"broken"}]` scored Overall 100.0 (S)
with no diagnostics. Every item is now required to carry a string
`content`, and a malformed one propagates through the failure path added
in this PR (same case now scores 17.5 (D) with a diagnostic).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
lgtm Re-reviewed Validation: Coverage: 5/5 changed files reviewed; 0 skipped (100%). |
1 participant
Summary
Three benchmark defects that all let a run report success it did not earn. Grouped into one PR because they overlap in
benchmark_executor.rs/main.rs.Report I/O — #270.
cmd_benchmarkwrote--outwithunwrap_or_else(|e| eprintln!(…))and then printedSavedunconditionally, returning unit, so a failed write still exited 0. Now the error propagates andSavedis only printed after a successful write.Error accounting — #271.
store()parsed the body withouterror_for_statusand turned a missingmemory_idinto a successful empty string, so a 400/500 seed or store step counted as a success.retrieve()collapsed transport/HTTP/parse failures into an empty vector — indistinguishable from a valid empty answer — and the retrieve step could never fail. Both now return errors, the retrieve step propagates them,run_assertionrecords them, and an errored assertion scores zero rather than passing on empty expectations (recallandnoise_rejectionboth default to 100 when a scenario declares no expected/excluded contents, so a service failure could previously pass outright). Failures are now printed as diagnostics instead of being discarded into never-read fields.Run isolation — #273.
run_idcame from epoch seconds with no random component, so independent runs started in the same second shared a user namespace and could purge/correct each other's memories. It now mixes in random bits. Separately, validation checkedscenario_iduniqueness case-sensitively while the executor lowercases it to build the namespace, soCaseA/caseavalidated as distinct and then collided; uniqueness is now checked on that same normalized form.Verification
Reran each issue's own reproduction against the built binary.
#271 — loopback server injecting the issue's status matrix:
✗ setup failed: … 400 Bad Request✗ setup failed: … 500✗ setup failed: … 500#273 —
CaseA+caseanow fails--validate-onlywith exit 1 (duplicate scenario_id (case-insensitive): casea); four concurrent runs of one scenario now produce 4 distinct namespaces instead of 1.#270 —
--out <dir>→ exit 1,Is a directory (os error 21), noSaved;--out <missing>/report.json→ exit 1,No such file or directory (os error 2), noSaved; writable file → exit 0,Saved, valid report JSON.Note the benchmark still exits 0 when scenarios fail against a live service — a bad score is a legitimate outcome — but the failures are now visible on stderr and scored correctly. Only the
--outwrite failure changes the exit status.Fixes #270
Fixes #271
Fixes #273
Test plan
cargo test -p memoria-clipasses (21/21 — 18 pre-existing plus 3 new: case-insensitive duplicate rejected, genuinely distinct ids still accepted, errored assertion scores 0 while a genuinely empty answer still scores 100)🤖 Generated with Claude Code