Repository navigation
fix(cli): retire old snapshot run folders - #5635
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at 5b6d4593. I found no blocker in the cleanup boundaries or the concurrency, and required CI on this head is green. 445499f3..5b6d4593 only drops one doc comment on finishSnapshotRun, so the review below, done at 445499f3, still holds.
What can be deleted. Only UUID-named directories directly under .hyperframes/artifacts/snapshots are candidates. A path is skipped if any component from the project root down is a symlink (ordinaryPath). Removal is rmSync on the run directory, which does not follow symlinks inside it. History, renders, checks and any --output folder outside a managed UUID run are never candidates. That matches the claim that project history is kept.
What is kept, on top of the newest 10 runs by directory mtime:
- the newest non-cleaned record for the current revision and for each retained rendered revision;
snapshots.latest;- anything in
pendingArtifactCleanup; - any run younger than 24 hours;
- any run with
.user-output. Any lstat error other than ENOENT also counts as kept. - any run whose
.capture-owner.jsonnames a live pid, is a symlink, or can't be parsed. EPERM fromkill(pid, 0)counts as alive.
Every uncertain case resolves to keeping the run.
Races.
- Planning and removal. Cleanup is planned inside
updateState, under the video-state lock, and references are retired there first. Removal happens after the lock is released. It re-checks dev, ino and mtime (unchangedRun) and the owner marker beforermSync. A run that changed in between is skipped, and the retired references never point at a missing folder. Two processes removing the same run is harmless: the secondrmSynchits ENOENT and is caught. - Live captures. The snapshot command creates the owner marker when the directory is allocated (
artifactDirectory), beforewithProjectLockand the capture. It releases the marker infinally, after contact sheets and descriptions are written.checkshares one leased directory per project through a counter, and only the lastfinishCheckSnapshotsreleases it.checkSnapshotDirectory.test.tscovers this. - CLIs from before this PR. They write runs with no marker. Such a run reads as not live, but it is fresh, so the 24-hour grace and newest-10 still protect it.
Ran here with borrowed workspace deps:
studio-serversrc/history: 191 pass, 2 skipped, including the newsnapshotRuns.test.ts.checkSnapshotDirectory.test.ts: passes.- Two
checkPipeline.*files couldn't collect, because the borrowed deps are missing@hyperframes/parsers/slideshow. That is the same dependency gap the body discloses. CI covers them.
Non-blocking:
- Liveness is pid-only. A capture running in another pid namespace, such as a container writing to a bind-mounted project, looks dead from the host. The 24-hour grace and newest-10 make this practically unreachable, but a marker that is old and has a dead pid would be a stronger test than a dead pid alone.
- Retire-then-skip loses links. If removal skips a run because it changed after planning (for example, an
--outputreused into it), its records are alreadycleanedwhile the files remain. That is safe, but history loses those image links. A narrow window. - Kept runs count toward the 10. Runs kept for
.user-outputor an owner count toward the newest 10. Heavy--output-into-managed use would then keep fewer ordinary runs than the skill text implies. - The
Commentscheck, red at445499f3, passes at this head.
The skill text matches the code: newest 10, current/latest/rendered evidence, live captures, 24 hours, whole UUID folders, --output untouched, and never rm -rf .hyperframes.
No live Studio or render run was used.
— Rames
Edit accuracy: accurate 271 (base branch 271), smooth 173 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
jrusso1020
left a comment
There was a problem hiding this comment.
Holding my approval at da0b9061. The code reads right to me, but CodeQL raised one new high alert, and main's ruleset includes a code_scanning rule, so the alert blocks the merge.
snapshotRuns.ts:107 is reported as js/file-system-race: an lstatSync(marker) check followed by openSync(marker, …). The handle is in fact verified after the open (O_NOFOLLOW, then fstat dev/ino must match the pre-check), so there's no real race. CodeQL still keys on the lstat-then-open pattern.
Two ways through:
- Drop the pre-open
lstat. Open withO_NOFOLLOW | O_NONBLOCKfirst, then requirefstat(fd).isFile() && nlink === 1. O_NOFOLLOW already refuses a symlink, O_NONBLOCK keeps a FIFO from hanging, and checking the handle itself is the stronger form. This should clear the alert without changing behaviour. The swap tests should still pass: a replaced owner is a different inode by the time it's opened. - Or dismiss the alert as a false positive with that reasoning, if a maintainer prefers.
Everything else from my 14132b3a read still holds. da0b9061 is test-only: the Node test environment and the lease released only after the last check. All other checks pass (58).
— Rames
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at 5bb6b7db. The CodeQL js/file-system-race alert from da0b9061 is gone, and required CI on this head is green.
da0b9061..5bb6b7db touches only snapshotRuns.ts and its test:
openOwnernow opens first, withO_NOFOLLOW | O_NONBLOCK. It then requires both the open handle and a freshlstatof the name to be a regular file withnlink === 1and the same dev/ino. A swap at any point before that check fails closed.ownSnapshotRunwrites the lease through its own descriptor and records that file's identity.releaseSnapshotRunonly truncates a lease whose open handle matches the recorded identity. So a marker that was replaced, or hard-linked out after creation, is never touched. The map entry is cleared infinally.artifactDirectorybuilds the path withresolve(), andfinishSnapshotRungets the same path back, so the map lookup matches.
The two new tests cover a replacement after open and a hard link after creation.
Non-blocking: a run that is owned but never reaches finishSnapshotRun (a capture that throws first) leaves one small map entry for the life of the process. The lease then expires through the dead-pid path as before.
Not run locally. This is from reading the diff, plus CI.
— Rames
Repeated CLI snapshots left old UUID folders, contact sheets and descriptions under
.hyperframes/artifacts/snapshots. Completed snapshot/check runs now reclaim whole old folders while keeping the newest 10, current/latest and retained-render evidence, active captures, and a 24-hour grace period. Explicit--outputdestinations remain user-owned, including nested reuse of a managed folder.Owner metadata is opened with no-follow/nonblocking flags, then the descriptor and current pathname are checked for regular-file, device/inode and single-link identity before reading. Lease creation records its exclusive descriptor identity; completion also checks that trusted identity before marking the opened descriptor released. Swapping the pathname cannot remove a replacement owner, and hard-linked markers are refused. Races, malformed markers and failed writes keep the run. No CodeQL suppression or dismissal is used.
Cleanup retires recorded image references durably before deleting files, revalidates directory identity and activity, refuses symlink traversal, and retries failed removals on a later capture. Capture leases last through contact-sheet/description writes and overlapping checks. Project history is preserved.
The CLI skill now asks agents to inspect full-resolution frames and decoded exports at matching times, zoom into WebGL geometry, and clean only owned disposable output. The reported Crescendo Mac graphics corruption is still being investigated; this PR does not claim to fix it.
Validation: 37 current focused tests pass under CI-matching Vitest 4.1.11 (36 history/retention plus the concurrent-check helper); the 74 CLI snapshot tests also executed successfully in hosted CI on the security follow-up. This includes disk-full persistence failure/retry, derived-file removal, active/crashed captures, explicit/nested outputs, symlink/hardlink boundaries and marker swaps before open and during completion. Guard-removal mutation fails the swap regressions; an independent reviewer also passed all 37 current focused tests, changed-file lint and formatting. The original-head studio-server typecheck, changed-file lint/format, skill lint/mirror/manifest, package subpaths and the Fallow audit pass (no new findings). Independent code review found no normal capture/release blocker. A deliberate unlink followed by inode reuse remains a local filesystem identity limitation; failed releases conservatively preserve their lease until process exit.
Local limits: reused existing dependencies and cached Vitest 4.1.11, using exact package-source aliases because borrowed exports predate the branch. Filesystem race tests explicitly use the Node environment so happy-dom does not bypass their builtin mocks. Its local studio-server typecheck cannot resolve current parser/core exports; fresh hosted typecheck is required (the previous security head passed hosted Typecheck). Two broader CLI check suites cannot collect because workspace exports/Ajv are missing, and full CLI/monorepo typecheck has missing cloud/WebGPU/Ajv dependencies. Only that local pre-commit typecheck hook was excluded; fresh hosted tests/typecheck remain required. No dependency or lockfile change.