Skip to content

Commit 13fcd99

Browse files
voidstackloopclaude
andcommitted
Make prune decide retention from filenames, not file content
Same anti-pattern as the last commit (ac7cf22), one function over: history::prune read and fully deserialized EVERY run's JSON just to sort by run.timestamp - a value save_run already writes straight into the filename ({ts}.json / {ts}-{suffix}.json). prune is the one command whose entire job is dealing with a lot of accumulated history, so this was the worst-affected function of the three fixed this week - a deployment that's been running monitor nightly for months, with runs full of embedded screenshots, paid for parsing all of that on every single prune. Fix: reuse the timestamp_from_filename helper added in ac7cf22 for all_known_forms. prune now never opens or reads a run file's content at all, only its name - retention is pure filename bookkeeping. Verified the same way as the last two rounds: added a test that corrupts every run file's content and confirms prune still counts and removes the right ones (it never needed to read them), then verified that test actually catches the regression by temporarily reverting to the old "parse every file, sort by parsed timestamp" implementation - it failed on the corrupted content exactly as predicted - then restored the fix. cargo fmt/clippy -D warnings clean, full suite green (157 tests). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent ac7cf22 commit 13fcd99

3 files changed

Lines changed: 52 additions & 3 deletions

File tree

CHANGELOG.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,15 @@ project doesn't have a release yet, so everything below is grouped under
2424

2525
### Performance
2626

27+
- **`prune` no longer opens or parses any run file.** Same anti-pattern
28+
as `all_known_forms` below, in the one command whose entire job is
29+
handling a *lot* of accumulated history: `history::prune` read and
30+
fully deserialized every run's JSON just to sort by `run.timestamp`,
31+
which `save_run` already writes into the filename. Retention
32+
decisions are now made from filenames alone — `prune` never touches a
33+
run's content at all, so a deployment that's been running `monitor`
34+
nightly for months prunes exactly as fast as one just getting
35+
started.
2736
- **`all_known_forms` no longer parses every historical run to find the
2837
newest one.** `history::all_known_forms` — behind `formwatch report`,
2938
`serve`'s `/metrics`/`/api/forms`, and `baseline --write` — picked each

README.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -525,6 +525,11 @@ newest runs. Defaults can live in config (`keep_last:` / `keep_days:`) or
525525
`*.json` run files and leaves anything else in the history directory
526526
alone. Run it from the same cron/Action that runs `monitor`.
527527
528+
Retention decisions come entirely from filenames (each run's timestamp
529+
is already encoded there) — `prune` never opens or parses a run's
530+
content, so it stays cheap no matter how much history has accumulated
531+
or how much screenshot data those runs carry.
532+
528533
## Container
529534
530535
A multi-stage `Dockerfile` builds a small Debian image with Chromium and

src/history.rs

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,13 @@ pub struct PruneReport {
346346
/// Deletes history runs per `retain`, per form. `now` is passed in so the
347347
/// `Days` policy is testable; `dry_run` computes the outcome without
348348
/// touching disk. Other files in a form directory are left alone.
349+
///
350+
/// Retention is decided from filename timestamps alone (see
351+
/// [`timestamp_from_filename`]) — a run file is never opened or parsed,
352+
/// so pruning a long-lived history stays cheap regardless of how many
353+
/// runs (or how much embedded screenshot data) it holds. A `.json` file
354+
/// whose name doesn't carry a recognizable timestamp is left alone,
355+
/// consistent with "other files in a form directory are left alone."
349356
pub fn prune(base: &Path, retain: Retain, dry_run: bool, now: i64) -> Result<PruneReport> {
350357
let mut report = PruneReport::default();
351358
if !base.exists() {
@@ -364,9 +371,10 @@ pub fn prune(base: &Path, retain: Retain, dry_run: bool, now: i64) -> Result<Pru
364371
if path.extension().and_then(|e| e.to_str()) != Some("json") {
365372
continue;
366373
}
367-
let run: RunResult = serde_json::from_str(&fs::read_to_string(&path)?)
368-
.with_context(|| format!("parsing {}", path.display()))?;
369-
runs.push((run.timestamp, path));
374+
let Some(ts) = timestamp_from_filename(&path) else {
375+
continue;
376+
};
377+
runs.push((ts, path));
370378
}
371379
// Oldest first; filename breaks ties so same-second runs (with a
372380
// `-N` suffix) stay a stable, total order.
@@ -738,6 +746,33 @@ mod tests {
738746
let _ = fs::remove_dir_all(&dir);
739747
}
740748

749+
#[test]
750+
fn prune_does_not_need_to_parse_any_run_file() {
751+
// Retention decisions come from filename timestamps alone, never
752+
// by parsing run content — proven by corrupting every file:
753+
// prune must still count and remove the right ones.
754+
let dir = std::env::temp_dir().join("formwatch-test-prune-corrupt");
755+
let _ = fs::remove_dir_all(&dir);
756+
let url = "https://city.gov/a";
757+
for ts in 1..=4 {
758+
save_run(&dir, &run_at(url, ts)).expect("save");
759+
}
760+
for ts in 1..=4 {
761+
fs::write(
762+
dir_for(&dir, url).join(format!("{ts}.json")),
763+
"{not valid json",
764+
)
765+
.expect("corrupt");
766+
}
767+
768+
let report = prune(&dir, Retain::Last(2), false, 0)
769+
.expect("prune should never need to parse run content");
770+
assert_eq!(report.removed, 2);
771+
assert_eq!(report.kept, 2);
772+
773+
let _ = fs::remove_dir_all(&dir);
774+
}
775+
741776
fn run_status(timestamp: i64, status: Status) -> RunResult {
742777
RunResult {
743778
schema_version: SCHEMA_VERSION,

0 commit comments

Comments
 (0)