fix(storage): reclaim the bytes the v1 upgrade orphaned

[Astro-Han]

Summary

#4808 left artifact bytes on disk with nothing pointing at them, and this reclaims them. It is the part of #4808 that PR could not finish, not a new mechanism.

Its v1 upgrade (migrateSqliteArtifactDatabase) rebuilds artifact_records by dropping the table and re-inserting whatever survives its own checks and decodeArtifactRecordJsons. Five kinds of row fail that gate, and each leaves its file behind:

dropped because	where those files came from
source is no longer an artifact source	provider_request_capture, history_compact_source, history_compact_block, synthesis_cache_block — writers removed by #4722 and #3545, records never cleaned up
the record carries no source at all	the CLI /recap snapshot never passed one
status is not live	a user-deleted upload, whose bytes v1 deliberately kept for a sweep that no longer exists
record_json does not parse	corruption
the record disagrees with its own columns	inconsistency

#4808 also removed the sweep that had been reclaiming the first group, which was self-consistent within that PR (the sweep found its work through the very records the upgrade drops, so it would have found nothing) but turns those bytes from being reclaimed into staying forever.

Dropping a row is the last moment anything knows where its bytes are — after the rebuild, no catalog can enumerate them, and a user's file has a user's name that no pattern could pick out of a directory. So the upgrade now records every path it does not carry over, in the same transaction that drops it. It cannot unlink there: it runs inside the BEGIN IMMEDIATE transaction migrateOperationalStateDatabaseInternal opens, where a rollback after a delete would be unrecoverable. ArtifactStore — still the only thing that deletes artifact bytes — drains the list on recovery, and a path some record has since claimed keeps its bytes.

Recording what the upgrade drops is exact where matching file names could only be a guess. Nothing scans the artifact tree, nothing is stat'd or resolved, so this does not go near the cold-start realpath/lstat walk of #4027; and nothing has to be kept in step with the writers that produced these files. It reaches malformed and inconsistent rows too, which a name-based pass could not have identified at all.

There is no sweeper and no timer. Steady state produces no orphans — completePurgeUnlocked removes bytes before metadata, so an interrupted purge leaves a record without bytes, never bytes without a record — and this residue has exactly one source, which is one-time. When the list is empty the reclamation returns immediately, and that is the whole record of having finished.

Review focus

• Capability given up: a store converted by an earlier build keeps its residue. Its rows are already gone, so nothing anywhere still knows which files they named. That is the argument for landing this in the same release as the conversion; the conversion is not on the nightly channel.
• A backup taken after the upgrade but before recovery carries the orphan bytes, and restoring it brings them back with the list already drained. operational-state-backup.ts copies artifacts/ wholesale. Those bytes are then unreclaimable, for the same reason as above.

Verification

• npm --workspace @maka/storage run build, then node --test --test-concurrency=4 "dist/**/*.test.js" in packages/storage — 1109 pass, 8 skipped, 0 fail.
• npm run format, npm run lint — clean. @maka/runtime-host typecheck is clean for this change (it reports only the pre-existing systeminformation and host-resource-collector.ts errors present on main).
• On a copy of a real workspace that had never been upgraded, opening the store runs the upgrade and then the reclamation:

before: 3 rows | provider_request_capture=3
before: 3 files, 95K
after : 0 rows | -
after : 0 files, 0K

One test, verified to fail without the change, builds a real v1 table with one row per reason the upgrade drops one — retired source, absent source, tombstoned upload, unparseable JSON, record/column mismatch — and pins that each one's bytes go while a live upload's stay. It also re-creates a dropped record's exact path before the reclamation runs, to pin that the new bytes win over the note, and asserts the list is drained and a second call is a no-op.

AI use

Select exactly one:

• No generative tool made a substantive contribution
• Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code — traced the residue to the upgrade's decode gate, enumerated the dropped kinds against each retired writer, wrote the reclamation, its test and this description, and ran the verification above. Reviewed and accepted by me.

Checklist

• Tests cover the change and fail without it
• Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

• Yes — described under Summary above
• No

[jackwener]

Approving at exact head 2a12d0ad. This does what it says, and the parts that could have destroyed live data are closed. One [P2] about what it does not cover, which is a scope statement rather than a defect in the code.

The two ways this could have deleted the wrong bytes are both shut

The name pattern alone would match a user's file. ^[A-Za-z0-9_-]+-provider-request-step-\d+-[A-Za-z0-9_-]+\.json$ matches an upload a user happened to name provider-request-step-7-mine.json, because the stored path is <artifactId>-<name>. The catalog is what saves it — claimed is built from metadataRepository.readAll(), the full record set across every Session, not a page or one Session's snapshot, and any claimed path is skipped. The test pins exactly this case. Reading the name to find candidates and the catalog to protect them is the right split, and it is also what lets this run either side of the conversion: on a store that has not migrated yet, the capture records still claim their paths and nothing is touched.

The session/name comparison is not a Windows hazard. relativePath is built with a literal / at artifact-store.ts:290 and :417, and artifact-metadata-codec.ts:122 rejects any record whose relativePath is not exactly ${sessionId}/${id}-${name}. So the string built from directory entries can be compared directly on every platform.

A symlinked Session directory is skipped rather than followed, since Dirent.isDirectory() is false for a symlink; the reclaim is idempotent; and a failure at startup is logged instead of refusing to start.

The pattern really does match what the retired writer produced

This is the part that usually breaks in a fix keyed on historical file names, so it was checked against the writer rather than inferred. provider-request-capture-artifact.ts set name: \provider-request-step-${input.step}-${input.captureId}.json`when captures were introduced in35998f2(#1277), and the same line was still there atd7235ac^, the commit before the writer was retired. stepis typednumberthroughout, so\d+is exact and thestep-none` file in the test is something the writer could never have emitted. The naming was stable across the writer's whole life, so the reclaim will find the real orphans.

[P2] The upgrade orphaned two kinds of file, and this reclaims one

The v1 conversion skips rows for two independent reasons: their source is no longer supported, and their status is not live. Both leave bytes behind. This PR addresses the first.

The second is a user-deleted upload. In v1, deleteUserArtifactInSession wrote a tombstone — status: 'deleted', record retained, bytes deliberately left on disk for the sweep to reclaim later. The conversion drops the tombstone with the table, and this reclaim cannot pick the bytes up afterwards, because a user's file has a user's name and no pattern can identify it. So on a v1 workspace, content a user explicitly asked to delete still persists after the upgrade, still survives purgeSessionArtifacts(), and is still copied into a backup.

This is not a criticism of the approach here — by volume, captures were the overwhelming majority (in the case measured for the removed sweep, 772.7 MB of an 814 MB installation), and this recovers essentially all of it. It is worth saying plainly because the remaining residue is the privacy-relevant half, and after this PR it will look closed.

If it is worth closing, the place is the conversion itself and only for stores that have not converted yet: unlink the bytes of a tombstoned row whose identity and path verify, then drop the metadata, leaving malformed or inconsistent rows untouched. For stores already converted, the tombstone is gone and the information needed to identify those files no longer exists anywhere — which is an argument for doing it in the same release as the conversion, if at all.

State

This PR is a draft and GitHub reports MERGEABLE / BLOCKED. The approval binds to 2a12d0ad and, per this repository's rule, stays valid across later pushes — but the required review and checks still have to be satisfied, and an automated review is not the independent human judgment CONTRIBUTING.md requires.

简体中文

在 exact head 2a12d0ad 上批准。它做到了它所声称的事,而且两条本可能删掉活数据的路都被堵死了。 有一条 [P2],讲的是它没有覆盖什么 —— 那是范围陈述,不是代码缺陷。

两条可能删错字节的路都已封闭

单靠文件名模式会命中用户自己的文件。 ^[A-Za-z0-9_-]+-provider-request-step-\d+-[A-Za-z0-9_-]+\.json$ 会匹配一个用户恰好命名为 provider-request-step-7-mine.json 的上传,因为落盘路径是 <artifactId>-<name>。救下它的是目录 —— claimed 由 metadataRepository.readAll() 构建,是跨所有 Session 的全量记录集,不是某一页、也不是单个 Session 的快照,任何被声明的路径都会跳过。测试正好钉住了这个用例。用名字找候选、用目录做保护,这个分工是对的;它同时也使得这段代码在转换前后都能安全运行:尚未迁移的库里,capture 记录仍然声明着自己的路径,于是什么都不会动。

session/name 的比较不是 Windows 陷阱。 relativePath 在 artifact-store.ts:290 与 :417 用字面 / 构造,而 artifact-metadata-codec.ts:122 会拒绝任何 relativePath 不严格等于 ${sessionId}/${id}-${name} 的记录。所以用目录项拼出的字符串可以在任何平台上直接比较。

被 symlink 的 Session 目录是跳过而不是跟随(Dirent.isDirectory() 对 symlink 为 false);回收是幂等的;启动时失败只记录日志,不拒绝启动。

这个模式确实匹配退役写入方当年产出的东西

以历史文件名为键的修复,通常就坏在这一步,所以这里是拿写入方去核而不是推断。provider-request-capture-artifact.ts 在 capture 引入时(35998f234,#1277)写的是 name: provider-request-step-${input.step}-${input.captureId}.json,而在写入方退役前的那个提交 d7235acef^ 上,这一行仍然一模一样。step 全程类型为 number,所以 \d+ 是精确的,测试里那个 step-none 文件是写入方根本不可能产出的。命名在写入方的整个生命周期里都是稳定的,所以这次回收会找到真正的孤儿。

[P2] 升级遗留了两类文件,这里回收了其中一类

v1 转换基于两个彼此独立的原因跳过行:source 已不受支持,以及 status 不是 live。两者都会留下字节。本 PR 处理的是前者。

后者是用户删除的上传。在 v1 里,deleteUserArtifactInSession 写的是 tombstone —— status: 'deleted'、记录保留、字节被刻意留在盘上,等待后续 sweep 回收。转换把 tombstone 连表一起丢掉,而这次回收之后也捡不起那些字节,因为用户的文件用的是用户起的名字,没有任何模式能识别它。所以在一个 v1 工作区上,用户明确要求删除的内容在升级后依然留存,依然熬过 purgeSessionArtifacts(),依然被复制进备份。

这不是对本 PR 做法的批评 —— 按体量,capture 是压倒性的大头(在为被移除的那个 sweep 实测过的案例里,是 814 MB 中的 772.7 MB),本 PR 基本上把它全收了。之所以要明说,是因为剩下的那一半才是与隐私相关的那一半,而本 PR 之后它看起来会像是已经关闭了。

若要关掉它,位置在转换本身,而且只对尚未转换的库有效:对身份与路径可验证的 tombstone 行,先 unlink 其字节,再丢掉 metadata,格式损坏或不一致的行保持不动。对已经转换过的库,tombstone 已经没了,识别那些文件所需的信息在任何地方都不复存在 —— 这本身就是一个理由:要做就该和转换放在同一个发布里。

状态

本 PR 是草稿,GitHub 报告 MERGEABLE / BLOCKED。批准绑定 2a12d0ad,按本仓库规则在后续 push 后仍然有效 —— 但必需的评审与检查仍需满足,而自动化评审不是 CONTRIBUTING.md 要求的独立人类判断。

---

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

[Astro-Han]

Reviewed current head 2a12d0adc9edac1631368e452be67bdcb4a8ca67 (OPEN, MERGEABLE/CLEAN). Technical GO — no P0–P2, three P3s below. Checks green on this head (test, label, windows_recovery).

P3 — new test calls symlink directly, bypassing the file's own Windows-skip wrapper

artifact-store.test.ts:931 calls bare symlink(...) while the other five symlink-creating tests in the same file all go through createSymlinkOrSkip() (:1034, :1073, :1178, :1342, :1393), which catches Windows EPERM/EACCES and skips (:1487-1505). It also omits the 'dir' type argument, which on Windows builds a file-type link for a directory target. (Caveat: this severity reading assumes no other required check runs the full storage suite — derived from the workflow step list; if one does, this should be raised.)

P3 — one undeletable file stops reclamation for every later session

The try/finally at :496-507 only covers inside a single session directory; an rm throw escapes the outer session loop and is caught at execution-composition.ts:1753-1759 with a console.error. So one permission- or lock-held file blocks all later sessions' leftovers — and with stable traversal order, every subsequent boot stalls on the same file. The comment's "retry next boot" holds for that file itself, not for the sessions behind it, which never get their turn. Suggest swallowing per-file rm failures with a count and continuing the loop.

P3 — the startup cost is permanent, with no termination condition

recovery.state awaits the scan every boot at "1 + session count" readdirs for a one-time leftover. The author explicitly accepted this (getdents only, no stat/realpath; a marker bit would add a new consistency problem) — a defensible call, recorded so humans know: with many session directories or a network filesystem, this is a permanent startup-path cost. Product trade-off, not a defect.

What I could not judge

Tests/build not run locally; the PR's "before 3 files 95K / after 0 files 0K" workspace numbers were read, not reproduced. The author-declared out-of-scope residue (deleted/retired/malformed/inconsistent records with no common name pattern) was not independently sized.

---

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

简体中文

评审结论来自自动化审查流程；发布者没有读这份 diff，核的是当前 head 有没有漂移、以及 exact-head 的门禁状态。当前 head 是 2a12d0a，未关闭。技术上无阻断问题，三条 P3：新测试裸调 symlink、单个删不掉文件挡住后面所有回收、启动扫描开销永久存在。等人类拍板。

[Astro-Han]

One clarification to my review above (grade unchanged: technical GO, three P3s).

On the first P3 (bare symlink() at artifact-store.test.ts:931): I noted it conditionally, but the fact is now confirmed — windows_recovery on this PR does not execute artifact-store.test.js at all (verified against its steps), so that green proves nothing about this test on Windows. What would catch it is the windows-baseline full_storage nightly line and local Windows development. The P3's severity note should read as this statement rather than a conditional.

---

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

[Astro-Han]

Thanks — reviewed against 2a12d0ad, and the head has moved twice since (rebase onto current main, then a rework). Where the three P3s stand at 8c523c413b:

P3 "one undeletable file stops reclamation for every later session" — real, and fixed. The session loop is gone, but the defect survived the rework in a new place: a non-ENOENT unlink failure escaped the loop, so one stuck file blocked every path behind it in the list and, with a stable order, blocked the same ones on every later boot. It now swallows the per-file failure, keeps only that path's own note for a later attempt, and discharges the rest. The v1-fixture test pins it: a leftover that cannot be unlinked (a directory at the recorded path, sorted first) leaves everything behind it reclaimed and exactly its own row in the list.

P3 "bare symlink() at artifact-store.test.ts:931" — obsolete. That test is deleted. Nothing in this PR walks the artifact tree any more, so there is no symlink behaviour left to pin, and the file's createSymlinkOrSkip() users are all pre-existing. The follow-up note about windows_recovery not running artifact-store.test.js is worth keeping on the record independently of this PR.

P3 "permanent startup cost with no termination condition" — obsolete, and the trade-off it recorded is withdrawn. The rework replaced file-name matching with an exact list: the upgrade records every path it does not carry over, in the same transaction that drops the row, and recovery drains that list. There is no directory scan at any boot; with an empty list the reclamation returns immediately. That also removed the pattern regex, and it now reaches malformed and self-inconsistent rows, which no name-based pass could have identified.

Re-reviewing at the current head would be useful — the mechanism under review is not the one that shipped in the diff you read.

[Astro-Han]

A process note, not a technical verdict: this branch has been force-pushed several times in quick succession (the head has moved four times since the last review, most recently to 8c523c41, with checks re-running each time). Reviewing each intermediate head wastes everyone's time.

Could the author please give a heads-up in a comment when the iteration settles? Happy to do a full review — including the new SQLite schema files and their migration — once the head is stable and CI is green.

---

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

[Astro-Han]

A note for the record, not a new verdict: this PR merged while under re-review. My earlier review (5122620715 plus a clarification) describes the filename-scanning mechanism at head 2a12d0ad, which the final merged head superseded (SQLite-backed metadata with migration). Read those comments as bound to that superseded head, not to the merged result. No approve expressed here; the merge decision was the author's and maintainers'.

---

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.