Add safe R2 audio maintenance tools - #544
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
Claude finished @gianpaj's task in 6m 59s —— View job Review: Add safe R2 audio maintenance toolsI read the full diff and cross-checked the cleanup allowlist and key formats against the web app's upload paths. Overall this is careful, defensive work — the safety layering is genuinely good and I found no bug that would cause incorrect deletion. My findings are about structure, robustness on the destructive path, and efficiency, plus one behavioral property that should be documented before it gets "fixed" by a future change.
What's well done
Findings1. The destructive script can't be imported, so its orchestration is untested —
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2b922c1e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The cleanup tool's reference check misses the Redis URL cache, which pins the exact R2 storage key with no TTL — deleting those objects leaves live cache entries pointing at 404s that the app will keep serving at zero credits. Separately, the single line that decides which objects get deleted has no test behind it, and runAction isn't exported so no test can reach it.
Reviewed changes
- Two new operator CLIs —
cleanup-orphaned-r2-audio.mtsinventories the allowlisted free-audio prefixes and permanently deletes from a reviewed manifest;backup-r2-audio.mtsmirrors buckets or exact prefixes to local disk and never deletes. - Shared R2 layer —
lib/r2-client.mtswraps the AWS SDK,lib/r2-transfer.mtsholds pagination, size parsing, safe path resolution, checksum verification, and no-overwrite download finalization. - Cleanup policy module —
lib/r2-orphan-audio-cleanup.mtsowns the bucket/prefix allowlist, the 45-day cutoff, manifest validation, and the pre-delete recheck. - Helper extraction — five existing
.mtsscripts now shareloadScriptEnv()andcreateScriptAdminClient();reset-freeloader-credits.mtscorrectly preserves its uniquepersistSession: true. - Deps and wiring —
effect@4.0.0-rc.111andeffective-progress@0.12.0pinned in the workspace catalog, plus a@sexyvoice/scriptstest target that Turbo does pick up from rootpnpm test.
I verified the Effect 4 RC usage against the pinned versions: mode: 'result', Result.isSuccess, .success/.failure, and the AbortSignal in Effect.tryPromise are all correct, and effective-progress forwards its options to Effect.all unchanged so result ordering is preserved. The flag matrix, the prefix allowlist against every write site, and the per-object pre-delete rechecks all hold up.
⚠️ Nothing has exercised the delete path — not a test, and not a real run
This is a scope question only you can answer, so I'm raising it rather than guessing.
Your implementation note records that the production inventory found zero orphan candidates (.agents/notes/implemented/operations/2026-08-22-r2-orphan-audio-cleanup.md:38 — the free prefixes contained only objects younger than 45 days), and the backup note ends with "Do not run a full backup." That's the right call for validation. But it means --delete has never executed against real R2, and as noted inline it has no test coverage either. The careful parts of this design — the manifest validation, the recheck, the backup gate — are exactly the parts that have never been observed working end to end.
That's defensible for a tool that ships dormant. It's worth being explicit about what closes the loop before the first destructive run: a test around runAction, a dry-run mode for --delete that reports what would be deleted without calling deleteObjects, or a first run scoped to a hand-written single-candidate manifest.
ℹ️ Nitpicks
backup-r2-audio.mts:461-466— theprogressAll/Ink branch only runs whenprocess.stdout.isTTYis true, which is what every real operator hits. The test atr2-audio-backup.test.mts:617passesinteractive: false, and a dry run never mounts the renderer, so per the verification record this branch has not been executed. A crash here is recoverable, but it is the default interactive path.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏
There was a problem hiding this comment.
ℹ️ The cache-eviction fix is correct and well-scoped. One minor sequencing issue noted inline.
Reviewed changes (incremental, f2b922c..cd0c271)
- Cache eviction added —
evictDeletedAudioCache()inlib/r2-orphan-audio-cleanup.mtsdeletes the Redis key after R2 confirms deletion, gated oncandidate.bucket === mainBucketso the API speech bucket is correctly left alone. - Eviction failures surface — a failed
redis.delrecordsdeletion-failurewith a reason that states the R2 object was deleted, which drives the nonzero exit. Reporting a successful delete as a failure is slightly odd on its face, but it's the right call: a stale cache key is the outcome an operator needs to act on. - Two new tests — both are exact rather than loose.
assert.deepEqual(deletedKeys, [mainCandidate.key])genuinely pins the bucket gate (removing theifturns it red), and the rejection test pins error propagation. - Dependency and docs —
@upstash/redismoved to the workspace catalog at1.37.0with no version change forapps/web, andAGENTS.md,docs/devops.md,scripts/README.md, and the plan's env-var list all record the new requirement and theKV_REST_API_*variables.
I re-checked the cache-key equality that this fix depends on: clone-voice/route.ts:1133-1134 uploads to R2 and caches under the same filename, so candidate.key is the correct eviction key for cloned-audio-free/ as well as generated-audio-free/.
The other findings from my previous review are unaddressed and their threads remain open, so this is a comment review rather than an approval.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏
|
Zed - GPT-5.6 Sol Addressed the actionable review findings in separate commits:
I kept deletion serial because batching would widen the race between the final database check and R2 deletion. I also kept Effect and effective-progress in the workspace catalog because the approved design explicitly requires catalog pins. Serial local hashing and timestamp parsing remain performance follow-ups; the measured production scope is 2.1 GiB, and neither affects correctness. A report-directory write failure cannot be recovered by another write to the same unavailable destination, but recoverable mid-delete errors are now recorded and covered by tests. Validation after these fixes: 45 scripts tests pass, pnpm fixall passes with the five existing Sentry warnings, and pnpm type-check passes. The local full suite still has the existing closed-Redis failure in the 35 Stripe webhook tests; 62 other web files pass. |
There was a problem hiding this comment.
ℹ️ The three prior findings these commits target are genuinely addressed. One gap on the newly-exported
runActionnoted inline, plus two corrections to my own earlier review.
Reviewed changes — nine commits since the last pullfrog review (cd0c271..48ee744), touching the cleanup command, both test files, the backup cap, and docs.
- Made the cleanup entry point import-safe —
runActionis exported and takes injected R2, database, cache, clock, log, and report dependencies, returning{exitCode, report, reportPath}instead of mutatingprocess.exitCode.loadScriptEnv()moved intomain(), which now sits behind animport.meta.urlentry-point guard matching the patternbackup-r2-audio.mtsalready used. - Added three orchestration tests over the destructive path —
assert.deepEqual(events, ['list','head','has','head','delete','cache','report'])is exact and load-bearing: I confirmed by mutation that removing theevictDeletedAudioCachecall, or moving thehasStorageKeycheck after the R2 recheck, each turn it red. - Gave each destructive candidate an outer error boundary — an unexpected throw now records
deletion-failurefor that candidate and continues, so earlier confirmed deletions survive in the report instead of dying with the run. - Moved Redis client construction into
main()— missingKV_REST_API_*now fails in the first second rather than after a multi-gigabyte download. - Stabilized database pagination —
listStorageKeysorders bystorage_keythenid. The tiebreak is total, sinceaudio_files.idis the uuid primary key. - Enforced the backup cap for direct callers —
runBackupbranches onmaxDownloadBytes === undefinedinstead of truthiness, so0reachesselectByDownloadLimitand throws instead of silently meaning "uncapped". Mutation-confirmed: removing that guard turns the suite red. - Stopped double-reading the manifest —
resolveExistingInputPathusesaccessinstead ofreadFile. - Documented the invariants — soft-deleted rows are references by design, manifests are approval artifacts rather than editable candidate lists, and deletion stays sequential to keep the check-to-delete gap small. I checked
scripts/README.md's "validates the candidate list and all derived totals" claim againstvalidateManifest, which does recompute the summary, validate inventory and bucket totals, and reject duplicate candidates.
All 45 scripts tests pass locally in ~1.8s.
ℹ️ Two corrections to my own earlier review
Both are things I got wrong or can now state with evidence, so they belong here rather than buried in a thread.
The :402 mutation claim was overstated. My initial review said that changing const requestedDeletes = options.download ? backedUp : eligible to = eligible would delete unverified objects with all tests green. The second half is true, the first half is not: the in-loop re-run of verifyLocalFile at :414-456 independently refuses anything that isn't verified-checksum, so the property is guarded twice and unverified objects are still not deleted. The new deletes only checksum-backed objects in download mode test does pin the property end to end — it just cannot distinguish which of the two guards is holding it. I've resolved that thread.
The MD5-mismatch branch is still unexercised, and I can now show it. Forcing the comparison in lib/r2-transfer.mts:345-351 to always report a match leaves all 45 tests green. The new test's "opaque" fixture uses a multipart-style ETag, so simpleEtagMd5 returns null and the code short-circuits to the size path before ever reaching the comparison. That thread and the runDownloadEffects ordering thread both stay open.
On the broader point from the first review: --delete still has not run against real R2, but it now has end-to-end coverage of the composition, which was the half of that concern code could close. The rollout question remains yours.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found. All three open findings are closed, and each fix is mutation-confirmed rather than merely present.
Reviewed changes — four commits since the last pullfrog review (48ee744..a7fe596), 102 lines across one implementation note, cleanup-orphaned-r2-audio.mts, and both test files. This delta is pure test-hardening plus a one-line guard; no production behavior changes.
- Reached the MD5 mismatch branch for the first time —
r2-audio-backup.test.mts:287-322writes a 5-byte wrong-content file against a 5-byte simple-ETag object, so the size check atr2-transfer.mts:330passes and control reaches the comparison at:346. I confirmed by mutation thatif (false && actualMd5 !== expectedMd5)now turns exactly that test red. The test also pinsgetCalls === 0and that the local file is byte-preserved, so it covers the "don't re-download, don't clobber" half too. - Pinned the
runDownloadEffectsordering invariant — each of the 7 items now carries its key as a distinctreason, and the descending delay(items.length - index) * 2forces item 3 to complete first and item 0 last.assert.deepEqual(outcomes.map(reason), items.map(key))is exact, not a.some(...). Mutation-confirmed: appending.reverse()to the return inbackup-r2-audio.mts:470-472turns it red. The invariantrunBackup:390-397depends on is now held by a test rather than by an RC dependency's implementation detail. - Validated options at the exported
runActionboundary —validateCleanupCliOptions(options)is now the first statement (cleanup-orphaned-r2-audio.mts:239). This closes both halves of the finding: the{delete: true, force: false, yes: false}bypass, and the{manifest, delete: false, download: false}report-losing throw out of the reconciliation.map(), which the--manifest requires --download or --deleterule (lib/r2-orphan-audio-cleanup.mts:202-204) now also blocks sincerunActionunconditionally requires a manifest. The new test assertsstorageReads === 0andreportWrites === 0, so it pins "fails before any work", not just "fails". - Recorded the invariants behind the tests — the implementation note now states why Effect result order is load-bearing, why the checksum tests use equal-size wrong-content fixtures, and why
runAction()re-validates. That is the right place for reasoning a future editor would otherwise have to reconstruct from the assertions.
All 47 scripts tests pass locally in ~1.3s, and every prior pullfrog thread on this PR is now resolved.
The one thing these commits could not close remains open and unchanged: --delete still has not executed against real R2, per .agents/notes/implemented/operations/2026-08-22-r2-orphan-audio-cleanup.md:38. The code half of that concern is now as covered as tests can make it, so what is left is purely a rollout decision — I raised it in the first review and am not re-raising it as a blocker here.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏
There was a problem hiding this comment.
ℹ️ The startup guard and the
NODE_ENVplacement are both correct. Two rough edges inline, plus one question about what closes the loop on the OOM fix.
Reviewed changes — one commit since the last pullfrog review (a7fe596..99e79fd), touching the backup implementation note, scripts/package.json, scripts/backup-r2-audio.mts, and scripts/r2-audio-backup.test.mts.
- Added an eager download-directory check to
runBackup—backup-r2-audio.mts:225-229creates the destination and checksW_OK/X_OKbefore listing R2, skipped entirely when--dry-runso dry runs stay non-mutating. It sits after the--download-dir is requiredthrow, and the per-objectmkdirindownloadWithoutOverwritestill exists, so this is a fail-fast check rather than a new prerequisite. - Pinned
NODE_ENV=productionon the backup command —scripts/package.json:9, reached by rootpackage.json:10, so both invocationsscripts/README.mddocuments pick it up. I verified this placement is necessary, not stylistic:backup-r2-audio.mts:7statically importseffective-progress, whosedist/index.mjsstatically importsink, so assigningprocess.env.NODE_ENVinsidemain()would run afterreact-reconcilerhas already picked its build. Installed versions confirm the mechanism —react-reconciler@0.33.0/index.js:3gates onprocess.env.NODE_ENV === 'production'at require time. - Added three tests around the new guard — two are load-bearing. Mutation-confirmed: deleting the block turns
creates the download directory before listingandrejects an unusable download directory before listingred, and flippingif (!options.dryRun)toif (true)turnsdoes not create the download directory during a dry runred.
I also checked the blast radius of the env var beyond React. Every package in this script's graph is clean: effect@4.0.0-rc.111, @aws-sdk/client-s3@3.1011.0 with all its @smithy/* transport, retry, and checksum middleware, dotenv@16.6.1, and tsx contain zero process.env.NODE_ENV references, so env loading, signing, retries, checksum validation, and stack traces are untouched. Ink routes render errors through its own ErrorBoundary.onError → unmount(error) rather than React's console.error defaults, so the production reconciler does not silently swallow renderer failures either.
All 50 scripts tests pass locally, matching the note's updated count.
ℹ️ The OOM fix targets the one path that has never been observed working
The interactive Ink branch at backup-r2-audio.mts:391 is still the only path with no test coverage and no recorded successful run — every test passes interactive: false, and the recorded production dry run never mounts the renderer. The note now describes a concrete failure there (dev reconciler retaining performance measurements until the V8 heap is exhausted), which reads like something you actually hit rather than something you anticipated.
If so, that is genuinely useful: it means the interactive path has now been exercised. What the verification record does not say is whether it was re-run under NODE_ENV=production and observed to complete. .agents/notes/implemented/operations/2026-08-23-r2-audio-backup.md:48-53 lists tests, type-check, Biome, and the earlier dry run, but no interactive run under the new env var. Worth recording one way or the other, since the fix is currently supported by a mechanism argument rather than an observation.
Technical details
# Interactive backup path under NODE_ENV=production is unverified
## Affected sites
- `scripts/backup-r2-audio.mts:391` — `dependencies.interactive ?? process.stdout.isTTY === true` selects `progressAll` (Ink) over `Effect.all`. No test reaches the Ink branch.
- `.agents/notes/implemented/operations/2026-08-23-r2-audio-backup.md:48-53` — verification record has no entry for an interactive run under the new env var.
## Required outcome
- The record should state whether the dev-reconciler OOM was observed in a real run, and whether an interactive run under `NODE_ENV=production` was then observed to complete.
## Suggested approach (optional)
- A short bounded interactive run (`--max-download-size` set small, against a prefix already fully backed up so nothing transfers) would exercise the renderer without a large transfer, and is consistent with the note's standing "Do not run a full backup."
## Open questions for the human
- Was the heap exhaustion observed, or inferred from Ink/React's dev-build behavior?@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏
There was a problem hiding this comment.
Important
Both prior findings are properly closed, and the W_OK fix is mutation-confirmed rather than merely present. But this delta hardened the destination check on the command that never deletes, and the same check is still absent from the one that does.
Reviewed changes — two commits since the last pullfrog review (99e79fd..8493838), touching the backup implementation note, scripts/backup-r2-audio.mts (-1 line), and scripts/r2-audio-backup.test.mts.
- Required the backup destination root to pre-exist — dropping
mkdir(options.downloadDir, { recursive: true })fromrunBackupleaves only the twoaccesschecks, so an unmounted removable volume now fails withENOENTinstead of being shadowed by a fresh directory on the internal disk. I confirmed the removal breaks nothing else:mkdiris still used at:730for the report directory, whose default (scripts/backups) is independent of--download-dir, anddownloadWithoutOverwritestill creates per-object subdirectories, so only the root is a prerequisite. - Replaced the mkdir-failure fixture with a real permission test —
rejects a read-only download directory before listingchmods a temp directory to0o555, asserts/EACCES/andlisted === false, restores0o755in afinally, and skips under root. This is the test the previous review asked for, and it works: mutation-confirmed that deletingaccess(options.downloadDir, constants.W_OK)atbackup-r2-audio.mts:226turns exactly that test red. Deleting bothaccesslines turns two tests red, where before it left the whole suite green. - Recorded the rationale in the note —
2026-08-23-r2-audio-backup.md:43now states the unmounted-volume reason rather than just the mechanic, which is the part a future editor would otherwise delete as a redundant check.
All 50 scripts tests pass locally in ~1.7s.
⚠️ The guard landed on the command that cannot lose data, not the one that can
This delta establishes that an unvalidated --download-dir is a hazard worth failing on. That reasoning applies with more force to cleanup-orphaned-r2-audio.mts, where the local copy is not the point of the run — it is the precondition for permanent R2 deletion — and where scripts/README.md:66 documents the same /Volumes/ExternalHD/sexyvoice-r2-bucket destination. I traced the delete path and had it independently re-traced; details are inline on runAction.
Worth being explicit about the scope question underneath it, since it is yours to answer: --download-dir is currently a "write here" instruction for both commands, but for cleanup it is really an assertion that a specific prepared volume is present. Those want different handling — the second argues for something stronger than an existence check, such as requiring a marker file the operator places on the drive once, or recording the resolved destination in the manifest at inventory time and refusing to delete if the action run resolves somewhere else. An existence check is the cheap version and closes the accidental-typo case; the marker-file version closes the wrong-volume case too.
ℹ️ Nitpicks
backup-r2-audio.mts:227—access(downloadDir, X_OK)is still unexercised. Mutation-confirmed: deleting that one line leaves all 50 tests green, because a0o555directory is still traversable. A0o666fixture would reach it, or collapsing both calls intoaccess(downloadDir, constants.W_OK | constants.X_OK)would shrink the untested surface to zero. Not worth a test on its own if you prefer the collapse.scripts/README.md:10-13— the backup destination must now already exist, but the README still shows--download-dir /Volumes/ExternalHD/sexyvoice-r2-bucketwith no note, so a first run against a fresh path fails with a bareENOENTthat reads like a bug. The implementation note records the new requirement; the operator-facing doc does not.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏
| ); | ||
| const locations = buildAllowedLocations(config); | ||
|
|
||
| if (options.download) { |
There was a problem hiding this comment.
runAction never checks that options.downloadDir exists or is writable — validateCleanupCliOptions only checks flag combinations, and resolveLocalObjectPath is pure path math with no filesystem access. So a mistyped or not-yet-mounted destination is created on the fly by downloadWithoutOverwrite's recursive mkdir (lib/r2-transfer.mts:389), the candidates verify as verified-download, enter backedUp, pass the in-loop verifyLocalFile re-check, and reach deleteCandidatesInBatches at :503.
The net effect is that the --download gate reports a verified backup while the only copy sits somewhere the operator did not intend, and the R2 objects are then permanently gone. 9522920 added exactly this guard to backup-r2-audio.mts:225-228 — three lines here would mirror it, and this is the command where getting it wrong is irreversible.
Technical details
# The destructive command does not validate its backup destination
## Affected sites
- `scripts/cleanup-orphaned-r2-audio.mts:252-260` — the first use of `options.downloadDir`, with no preceding existence or permission check.
- `scripts/lib/r2-orphan-audio-cleanup.mts:167-205` — `validateCleanupCliOptions` enforces that `--download` requires `--download-dir`, but never touches the filesystem.
- `scripts/lib/r2-transfer.mts:278-311` — `resolveLocalObjectPath` performs path-traversal safety checks only; it does not require the root to exist.
- `scripts/lib/r2-transfer.mts:389` — `mkdir(path.dirname(destination), { recursive: true })` creates the entire tree including the destination root.
- `scripts/cleanup-orphaned-r2-audio.mts:404-405` and the in-loop `verifyLocalFile` re-check — both compare size/MD5 at a resolved path and have no notion of which volume that path lands on, so neither constrains *where* the backup lives.
- `scripts/README.md:60-68` and `docs/plans/2026-08-22-r2-orphan-audio-cleanup.md:78,121` — both document `/Volumes/ExternalHD/sexyvoice-r2-bucket` for cleanup with no pre-existence caveat.
## Required outcome
- A cleanup run that would download to a destination root the operator has not prepared should fail before any R2 deletion, rather than creating the root and proceeding.
## Suggested approach (optional)
- Mirror `backup-r2-audio.mts:225-228` inside `runAction`, after `validateCleanupCliOptions(options)` and gated on `options.download`, so it fails before the manifest read and before any listing.
- A test in the existing `describe('cleanup action orchestration')` block asserting `storageReads === 0` and `deletes === 0` for a non-existent `downloadDir` would pin "fails before any work", matching the pattern the `validates destructive options for direct callers before work` test already uses.
- Note the honest limit of an existence check: it catches a typo or an unprepared path, but not a volume mounted at the expected path with the wrong contents. If the stronger property matters, requiring an operator-placed marker file on the drive, or pinning the resolved destination into the manifest at inventory time, would cover the wrong-volume case too.1 participant
This adds guarded tools for two R2 maintenance jobs: inventorying and cleaning old orphaned free-user audio, and backing up complete buckets or exact prefixes to a local drive without overwriting files. The split matters because backup must not inherit the cleanup command's Supabase, age, allowlist, or deletion behavior.
The cleanup command uses reviewed manifests, database-reference checks, a 45-day cutoff, fresh R2 metadata checks, bounded downloads, and explicit deletion confirmation. The backup command lists every requested source before transfer, verifies simple ETags with MD5 and opaque ETags by size, selects missing objects oldest-first under an optional cap, streams four downloads concurrently, and writes a complete JSON report. Interactive transfers use effective-progress with Effect 4 RC; redirected runs avoid Ink rendering.
Validation:
Documentation updates:
No migrations, billing, credits, generation behavior, storage deletion policy outside the cleanup command, or public API contracts change.