Repository navigation
Prune rollback manifests by age, on read #102
Description
Question
A pass that moves files will write one manifest per pass. At a 15 minute interval that is 96 files a day, roughly 35,000 a year, and nothing currently deletes them. RollbackService reads and unlinks manifest files with no lock (rollback.ts:464, :729), and AGENTS.md already records that as a known gap.
Decision already taken during charting: prune by age, 30 days, and delete lazily on read.
Not a cap on count. Age, because 30 days is a promise a user can reason about ("I can undo anything from the last month") while "the most recent 50" is a number that silently means something different as the interval changes.
Not on a schedule, because that means a second cron job of our own, which is the thing #52 is trying to avoid. Prune inside an operation that already touches the directory, so the cleanup piggybacks on work already happening.
Where the prune hooks in. Candidates, in rough order of preference:
listManifests, which alreadyreaddirs the rollback directory on every call (rollback.ts:346). Any tool that lists runs hits it.ensureStorage/migrateLegacyStorage, already run once per process (rollback.ts:134,:146), so it happens on every MCP server start too.
The race, which is the whole reason this is a ticket and not a line of code. AGENTS.md is explicit:
Do not bolt the history lock onto
RollbackServiceto close it: that would serialize two different resources and make the two files contend. Separate the shared state first.
So this cannot ship before the manifest safety ticket closes the rollback lock gap. A unlink racing a read of the same manifest means an undo in flight loses its manifest mid-execution, and rollback unlinks on success only (rollback.ts:718). Pruning makes the file have a scheduled death, which turns a rare race into a scheduled one.
Decide here.
- Is 30 days fixed, or configurable, or per-source? A machine sweeping Downloads every 15 minutes and a machine sweeping monthly want different retention. A single
historyLogging-style config field is cheap; recommend making it configurable with a 30-day default. - Does a pruned manifest lose its history row too? The row in
operations.jsonlcarries amanifestIdthat would then point at nothing.view_historyshows that column. Decide whether to drop the row, null the field, or let it dangle. Recommend leaving the row and letting the id dangle, since history rotation is capped by size and a dangling id is honest. - Does pruning interact with the
[verify_integrity]tool?file_organizer_verify_integritytakes amanifest_id(src/tools/rollback.ts:128). Verified answer: it is a separate tool definition in the same file,verifyIntegrityToolDefinition, registered separately. A pruned manifest makes it error the same way a missing one does today. Confirm that is acceptable.
Implementation detail.
- The retention window belongs next to
DEFAULT_CONFIGin the rollback service, mirroringHISTORY_LOGGING_CONFIG(src/core/config/defaults.ts:37). Consistency with the history rotation, which already caps at 10MB times 5 (history-logger.service.ts:71). listManifestssorts bytimestampdescending (rollback.ts:373) after parsing. Pruning there is a filter over an already-parsed list, so it costs nothing extra.- Path portability. Per
AGENTS.mdrule 3, do not letpath.sepreach a value a human or a test reads. Manifest paths inside the file are real filesystem paths andpath.joinis correct. Just do not build a reported id or label withpath.join.
Tests that must fail without this change.
- A manifest older than the window is gone after a read; one inside it survives.
- An absent window field uses the 30-day default, tested by asserting the boundary.
- Pruning does not remove a manifest that an undo is currently reading. Hard to test directly; test it by holding the rollback lock during the prune attempt and asserting the prune refuses rather than unlinks.
- A history row whose manifest was pruned still renders in
view_history, per decision 2.
Proof. Seed a sandbox with a manifest whose timestamp is older than the window, drive a listing, and assert the file is gone from disk. Not a mock.
Depends on. The manifest safety ticket must land first. Interacts with the digest ticket, which may print manifest ids that are about to be pruned.
Activity
kridaydave commentedon Oct 11, 2026
Decision
Prune rollback manifests by age, lazily, on read. 30 days default, configurable.
Now unblocked. #97 gave
RollbackServiceits ownmanifests.lock, held for the whole undo. That is the gate this ticket was waiting on: without it, a prune unlinking a manifest that an undo is reading turns a rare race into a scheduled one.The shape
Age, not count. 30 days is a promise a user can reason about ("I can undo anything from the last month"). "The most recent 50" silently means something different as the interval changes.
Configurable with a 30-day default. A machine sweeping Downloads every 15 minutes and a machine sweeping monthly want different retention.
On
UserConfigatsrc/core/config/loader.ts:16. UpdateKNOWN_CONFIG_KEYS(src/core/config/effective-config.ts:62), which usessatisfiesand is therefore a compile error if you forget. Also updateconfig.schema.json.Where the prune hooks in
listManifests, under the manifest lock. It alreadyreaddirs the rollback directory on every call (rollback.ts:346) and the list is already parsed and sorted by timestamp descending at:373. Pruning there is a filter over an already-parsed list, so it costs nothing extra in I/O.Not on a schedule. A second cron job of our own is the thing #52 exists to avoid. Piggyback on work already happening.
The lock is not optional here. #97's whole point. Prune inside the same
manifests.lockacquisition that guardsrollback(). Without it the unlink races the read.History rows: leave them dangling
A pruned manifest leaves its
manifestIdinoperations.jsonlpointing at nothing.view_historyshows that column.Do not drop the row, do not null the field. Let it dangle. History rotation is already capped by size (10MB times 5,
history-logger.service.ts:71), so a dangling id is bounded, and a dangling id is honest: the manifest existed and has aged out.Cleaning up rows would mean rewriting
operations.jsonl, which is append-only JSON-lines behind a lock. That is a second writer on a resource that already has one, for a cosmetic gain.verify_integrityon a pruned manifestfile_organizer_verify_integritytakes amanifest_id(src/tools/rollback.ts:128), a separate tool definition in the same file. A pruned manifest makes it error exactly the way a missing one already does. That is acceptable, no change needed.Implementation
DEFAULT_CONFIGin the rollback service, mirroringHISTORY_LOGGING_CONFIG(src/core/config/defaults.ts:37). Consistency with the history rotation.listManifests: acquiremanifests.lock, prune manifests whosetimestampis older thanretention_days ?? 30, release, then return the list. The manifest is already parsed by the time we decide, so the check is arithmetic on a number we have.path.joinis correct there. Do not build a reported id or a label withpath.join. Rule 3 inAGENTS.md.Tests that must fail without this change
retention_daysuses the 30-day default, asserted at the boundary.view_history, with its dangling id.verify_integrityerrors the way a missing one does.Proof
Seed a sandbox with a manifest whose
timestampis older than the window, drive a listing through the real server, and assert the file is gone from disk. Not a mock.Depends on
#97. Ships with #42.
kridaydave commentedon Oct 19, 2026
Shipped in PR #117. Rollback retention is
RollbackService.pruneExpiredManifests()insrc/core/organize/rollback.ts(prune by age, default 30 days, run lazily on read), covered bytests/unit/core/organize/prune-expired.test.ts.kridaydave commentedon Oct 19, 2026
Shipped in PR #117. Rollback retention is
RollbackService.pruneExpiredManifests()insrc/core/organize/rollback.ts(prune by age, default 30 days, run lazily on read), covered bytests/unit/core/organize/prune-expired.test.ts.