Skip to content

fix(cli): retire old snapshot run folders - #5635

Merged
miguel-heygen merged 5 commits into
mainfrom
fix/snapshot-run-retention
Oct 15, 2026
Merged

miguel-heygen merged 5 commits into
mainfrom
fix/snapshot-run-retention

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

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 --output destinations 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.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.json names a live pid, is a symlink, or can't be parsed. EPERM from kill(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 before rmSync. 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 second rmSync hits ENOENT and is caught.
  • Live captures. The snapshot command creates the owner marker when the directory is allocated (artifactDirectory), before withProjectLock and the capture. It releases the marker in finally, after contact sheets and descriptions are written. check shares one leased directory per project through a counter, and only the last finishCheckSnapshots releases it. checkSnapshotDirectory.test.ts covers 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-server src/history: 191 pass, 2 skipped, including the new snapshotRuns.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:

  1. 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.
  2. Retire-then-skip loses links. If removal skips a run because it changed after planning (for example, an --output reused into it), its records are already cleaned while the files remain. That is safe, but history loses those image links. A narrow window.
  3. Kept runs count toward the 10. Runs kept for .user-output or an owner count toward the newest 10. Heavy --output-into-managed use would then keep fewer ordinary runs than the skill text implies.
  4. The Comments check, red at 445499f3, 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

Comment thread packages/studio-server/src/history/snapshotRuns.ts Fixed
Comment thread packages/studio-server/src/history/snapshotRuns.ts Fixed
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 173 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

Unstable (1)

  • textselect-none-px-r0-nested-z100: tracking 0, pressJump -, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0, pressJump -, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0, pressJump -, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true

Comment thread packages/studio-server/src/history/snapshotRuns.ts Fixed

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 with O_NOFOLLOW | O_NONBLOCK first, then require fstat(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 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • openOwner now opens first, with O_NOFOLLOW | O_NONBLOCK. It then requires both the open handle and a fresh lstat of the name to be a regular file with nlink === 1 and the same dev/ino. A swap at any point before that check fails closed.
  • ownSnapshotRun writes the lease through its own descriptor and records that file's identity. releaseSnapshotRun only 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 in finally.
  • artifactDirectory builds the path with resolve(), and finishSnapshotRun gets 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

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 15, 2026
Merged via the queue into main with commit db845d9 Oct 15, 2026
106 of 107 checks passed
@miguel-heygen
miguel-heygen deleted the fix/snapshot-run-retention branch October 15, 2026 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants