Repository navigation
Delete idle session files after the retention window (OPT-1317) - #22
Optale-ops wants to merge 4 commits into
Conversation
|
Review status at 028b08d (GPT-6 Astra medium). Passes 1 and 2 found and closed: deletion before the final listing completed (P1); a size cap that made large prefixes permanently exempt (P1; keys now spill to a temp file instead); the approval wording in .env.example (moved to the deploy note); and leftover spill directories after a hard kill (now reclaimed after 24 quiet hours). Fresh-context pass 3 is in progress. Real Redis + MinIO proof on this head (artifact /home/thor/optale-artifacts/security-engine-retention-1317-2026-10-06/proof/final-head-028b08d9/): dry-run counted the idle prefix and a 10,205-object prefix and deleted nothing. Enabled, it deleted both completely and kept the live, recent and excluded prefixes. The spill directory existed during deletion and was gone afterwards, and an abandoned 25-hour spill was reclaimed while a fresh one stayed. Method: only the sweep's Date.now was shifted forward by a preload; MinIO, Redis and S3 signing ran on the real clock. Accepted known limits, by PRIMARY decision:
Deploy: engine PR #21 (OPT-1251) also changes service/src/file-server.ts on the same base, so expect a rebase after #21 merges. Deploy with CODEAPI_FILE_RETENTION_ENABLED=false, let one complete pass report its counts, and enable after Thor approves those counts (his rollout decision). |
Optale-ops
left a comment
There was a problem hiding this comment.
Fresh-context third pass for OPT-1317
Reviewed head 028b08d against base b68e85e. Both refs resolve, the local checkout matches the assigned head, and the complete three-dot diff is nonempty: six files. Read every changed test and all changed code.
Findings: no material blocking or should-fix findings. No hard violation or judgement-call finding to report.
Spec alignment, static analysis:
- The cutoff adds the configured retention period to max(24 hours, SESSION_CACHE_TTL), using a fixed cutoff for the pass. Registration sites mint storage session IDs and set the Redis mapping; ordinary authorization and reads do not refresh it. The sweep checks the exact session mapping before eligibility, after each re-list, and before every delete page. See service/src/file-retention.ts:173-260; service/src/service/router.ts:250-258,425-531,629-655; service/src/service/programmatic-router.ts:572-580,1326-1330; service/src/middleware/auth.ts:312-331.
- Checkpoint, runner/ and configured exclusions reach the sweep. Nested/non-session objects are preserved. Console references and read-only metadata do not create an exemption. See service/src/file-server.ts:813-818 and service/src/file-retention.ts:183-203.
- The final complete listing and mapping/age checks precede every deletion. Collected exact keys spill above 10,000 to a private temporary directory; pages contain at most 100 keys. Dry-run branches before object deletion. Spill cleanup covers completion, errors and close; abandoned spill reclamation checks directory and file mtimes. See service/src/file-retention.ts:34-118,228-268.
- Startup schedules non-overlapping bounded ticks, preserves the generator between ticks, publishes metrics through the existing /metrics route, and closes the sweep before Redis shutdown. Restarts begin a new pass. See service/src/file-server.ts:311,809-896.
Applied the rulebook checks for auth/scope, malformed configuration, pagination, security boundaries, cross-actor state, replicas/restarts, deployment requirements, liveness and scope. Inspected the file-server upload/download/delete routes, public file authorization, replay registrations, gateway forwarding, runner transfers, pushed runtime inputs, checkpoint object naming, build artifact upload paths, environment injection and the installed MinIO listing/delete implementation. No out-of-scope code changes identified.
Boundaries: this was static-only review as assigned. No tests, builds, linters, formatters or runtime probes were executed. LSP reference and definition requests returned empty results even for visible imports; repository text searches and direct caller/consumer reads supplied the fallback coverage. These results do not establish runtime behavior or production configuration. Deployment must supply the intended retention settings and the same session TTL/checkpoint configuration as the writers; the unchanged Helm template does not expose the new retention settings directly.
The declared post-final-listing writer race and mtime-only spill-reclamation limit remain accepted limits, not new findings. I did not treat OPT-1321 as a blocker or propose new permission gates.
Verdict: no blocking findings at 028b08d; static review passes against the assigned contract.
OPT-1317. The engine store kept every code session's files forever (production, 2026-10-06: 83,334 session folders, 146.5 GB, oldest 2026-06-24).
Thor's decision: delete a session's files 48 hours after its last activity; files the Console points to get no exception, because the Console keeps master copies and re-uploads when missing.
How: sessions end a fixed 24 h (SESSION_CACHE_TTL) after registration and use does not extend them. So the sweep deletes a storage-session prefix only when its newest object is older than CODEAPI_FILE_RETENTION_HOURS + max(24 h, SESSION_CACHE_TTL), which is 72 h by default and guarantees at least 48 h since any activity. It skips any prefix that still has a live Redis session mapping, the checkpoint prefix, runner/ build artifacts, and CODEAPI_FILE_RETENTION_EXCLUDED_PREFIXES. Before deleting it re-lists the full prefix and aborts if anything is recent, then deletes only the exact aged keys, in pages of up to 100.
Default is dry-run (CODEAPI_FILE_RETENTION_ENABLED=false): it only counts. Metrics on /metrics: codeapi_file_retention_prefixes / _bytes {outcome, dry_run}, plus _complete. It runs bounded work per tick and is safe across replicas.
Known limit: an existing key overwritten after the final listing can lose its fresh bytes. Conditional DeleteObject (If-Match) is not available: minio-js 8.0.6 has no such option and server RELEASE.2025-09-07T16-13-09Z ignores it on DELETE, by source reading.
Deploy: deploy with ENABLED=false, let a full pass finish, report the complete=1 counts to Thor, and enable only after his yes.
Verification: bun test src/file-retention.test.ts src/file-server-download.test.ts, 13 pass. Real MinIO proof and Astra medium review pending.