perf(benchmark): bound terminal run/batch registries - #738
Draft
beruro wants to merge 1 commit into
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
BENCHMARK_RUNSandBENCHMARK_AGENT_BATCHESinsrc-tauri/src/benchmark/mod.rsare process-wide staticHashMaps that only ever grow. Individual run logs are capped at 1,000 lines, but entries for finished (terminal) runs and batches are never evicted, so repeatedly running benchmarks in a long session grows memory without bound. Additionally, evaluator subprocesses (Python/Docker) still running at app exit were orphaned: theExitRequestedhandler kills PTY shells but never terminated benchmark evaluators.Solution
New
src-tauri/src/benchmark/retention.rsimplements a pure, unit-tested eviction policy:runningand batches inqueued/running(and any unknown status, conservatively) always stay resident.finished_atRFC3339 timestamps (all writers useUtc::now().to_rfc3339(), so lexicographic order is chronological); ties break by key so repeated application is idempotent.terminal_keys_to_evict(entries: &[RetentionEntry], budget) -> Vec<String>(input: terminal flag + timestamp + budget; output: keys to delete).prune_terminal_runs/prune_terminal_agent_batchesapply it under the registry lock at every site where an entry is inserted in or transitions to a terminal state (finish_run_with_result, run cancel, patch-only insert, batch item updates, batch persist/reload/refresh, batch cancel).benchmark_get_agent_batch_statusandload_agent_batch_for_updatealready fall back toload_agent_batch_historyand re-insert into the registry, so evicted batches reload on demand.benchmark::terminate_running_evaluators_sync()is called from theExitRequestedhandler (next to the existing PTY cleanup) and best-effort SIGTERMs evaluator PIDs still marked running. Cancel and error paths already release the child handle today (benchmark_cancel_runSIGTERMs the PID andrun_swe_bench_processreaps it viachild.wait(); error paths mark the run failed and drop the handle) — verified, unchanged.Unit tests cover: all-active never evicted, oldest terminal evicted beyond budget, active entries not counted toward the budget, idempotency on repeated application, missing-timestamp ordering, key tie-breaks, zero budget, status classification, and map-level pruning for both registries.
Potential risks
junyu/fix-benchmark-personal-paths: a parallel branch is cleaning personal-path defaults in the samesrc-tauri/src/benchmark/directory (it extractedpaths.rs). This PR does not touch path defaults, but import blocks inmod.rs/commands.rsmay conflict textually. Resolve at review/merge time.BENCHMARK_RUNShas no persistence layer, sobenchmark_get_run_statusreturns "not found" for terminal runs older than the 50 most recent. Scope is limited: the frontend (useBenchmarkRun.ts) polls only while a run isrunning(never evicted) and stops at terminal; batch evaluation outcomes are mirrored into batch items and persisted with the batch, andrefresh_agent_batch_evaluationsalready tolerates missing runs.running(e.g. its spawned task panics before finishing) is never evicted, per the "active entries are never evicted" requirement. This matches pre-existing behavior; entries are still bounded by the 1,000-line log cap.persist_agent_batch_statusfails (already only logged today) and the batch is later evicted, its latest in-memory state is lost. Same failure mode existed before for app restarts; eviction slightly widens the window.try_lockand SIGTERM; a hard kill of the app still orphans evaluators (as before).Verification
cargo check -p org2: NOT RUN — full cold compile of the app crate in a fresh worktree exceeded the local time budget on a machine shared with parallel build tasks; CI runs the full check suite for this PR (Draft until green).cargo test -p org2 benchmark::retention: NOT RUN — same reason (test harness requires the same full crate compile). The 11 unit tests inretention.rsare pure-function tests with no I/O and will run in CI.rustfmton all touched files: run, clean.src-tauri/src/benchmark/(8 files incl. newretention.rs) + 3-linelib.rsexit-handler hook; no path-default code touched; unrelated pre-existing rustfmt churn in other modules explicitly reverted.