refactor(artifacts): make child outputs exact and retire stale lifecycle state - #4808
refactor(artifacts): make child outputs exact and retire stale lifecycle state#4808Astro-Han wants to merge 35 commits into
Conversation
4e124ee to
9160177
Compare
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Reviewed at exact head 4ca24f0ae9ae9cb3e8805247c46e07782ce85761. [P1] — not approving. One P2 alongside it. Three independent reviews reached the same migration finding from different directions; a fourth is still running its deletion-surface pass and I will post separately if it changes anything.
The simplification itself is genuine and I am not asking for any of it back. Tombstones, the purge and publication journals, recovery and orphan state, and the duplicate repository abstraction are really gone — net −1,655 lines. The finding below asks for a correct cutover, not for restoring steady-state machinery.
[P1] The v1→v3 migration deletes continuation-durable Artifacts wholesale
sqlite-artifact-schema.ts:24-30 sees the ordinary v1 status / storage_key columns — which is what main has today, schema 1 — and issues DROP TABLE artifact_records, then creates an empty one. It never classifies rows. Retired sources and retained sources are removed alike.
The codec's skip of unsupported or malformed rows (artifact-metadata-codec.ts:48-67) is the correct, source-selective fail-open, and it is well built. It never sees a v1 row, because the table is gone before it runs.
SqliteArtifactStore.load() now only calls metadataRepository.readAll(); recover() is gone and bytes under artifacts/ are not reindexed. So after the upgrade, getInSession, listTurnArtifacts and readDurableAttachmentBinary return nothing for every source — including the six the Compatibility boundary names as durable.
Reproduced end to end, writing with the exact base c180a2ba production writers and reading with head:
| Result | |
|---|---|
| Base writers produce live rows for all seven retained sources | valid on base |
| Open the same root on head | v3 count 0; all seven reads not_found |
Payload files under artifacts/ |
still on disk, now orphaned |
Two consequences were reproduced independently through base writer/coordinator → head reader/coordinator:
- A completed
session_effectloses its durable idempotency. On base the effect resolved with one model call and the resultbase recap. After the upgrade, the samesessionId/effectId/reasoninvoked the model again and committed a differenthead recap. An effect that had already run once runs twice across an upgrade, with a different outcome. - A real archived tool result becomes unreadable. Readable on base; after the upgrade the same durable RuntimeEvent placeholder resolves
not_found.
This is universal for current-main state roots, not a corner case. And it contradicts the boundary the PR declares: "Session metadata, messages, RuntimeEvent history, and continued conversation remain outside this disposal boundary. Sources required for continuation … remain durable." The first sentence holds — the Session opens. The second does not.
The shape is the one from #4779: startup and decode stay up while the rest of the chain does not. A test in this PR states the behaviour outright — opens existing Sessions after retiring v1 Artifact metadata (operational-state-store.test.ts:169-224) inserts a live v1 row and asserts session metadata survives and artifact_records count is 0. The catalog being emptied is currently a pinned contract rather than an accident, which is why it needs a decision rather than a patch.
Fix — a selective one-time migration. Preserve valid live retained-source rows in v3 shape; explicitly discard deleted, retired and malformed rows and handle their bytes. That lets the codec's skip do the job it was written for. Regressions should be base-shaped: effect retry, archive read, projection/upload/writeback reopen, and retired-row disposal.
[P2] Change notifications were deleted rather than replaced
artifacts.subscribeChanges and artifacts:changed are gone. The pane refreshes only on sessionId / refresh identity (artifact-pane.tsx:167-172) and after local actions, so a file created during the same session does not appear until a remount or a session switch.
On "exact"
Worth stating plainly because the word carries two meanings here. It is a projection filter, not a rejection. finalizeAndListChildTurnArtifacts (session-manager.ts:1046) filters child results to tool_result / tool_result_projection / subagent_writeback / deep_research and drops tool_result_archive / user_upload / session_effect; storage's listTurnArtifacts remains unfiltered.
The resulting failure semantics are mixed, and the mix is deliberate in most rows:
| Gate | Inexact input | Behaviour |
|---|---|---|
create() with an unknown source |
throws Invalid Artifact source |
fail-closed |
protocol artifactSource |
invalidProtocolFrame |
fail-closed |
missing subagent_writeback after finalize |
throws | fail-closed |
| child-result list, wrong source | silently omitted | fail-open |
| metadata JSON or retired source | row skipped | fail-open |
| v1 schema | whole table dropped | fail-open session, fail-closed catalog |
The last row is the P1: the session opens and the catalog is gone.
Historical over-broad typed child results stay decoder-compatible, and accepting old id-only transcript records is necessary compatibility rather than a gap. The description's durable list also omits tool_result itself, which the enum retains — minor, but worth fixing whenever the description is next edited.
简体中文
在 4ca24f0ae9ae9cb3e8805247c46e07782ce85761 上评审。[P1]——不予批准。 另有一条 P2。三次独立评审从不同方向得到同一个迁移发现;第四席仍在跑删除面,若有变化我会另发。
这次简化本身是真实的,我不要求恢复其中任何一部分。 墓碑、purge 与 publication 日志、recovery 与 orphan 状态、重复的仓储抽象确实都没了——净 −1,655 行。下面这条要求的是一次正确的切换,而不是把稳态机制搬回来。
[P1] v1→v3 迁移整批删除了 continuation-durable 的 Artifact
sqlite-artifact-schema.ts:24-30 看到普通的 v1 status / storage_key 列——那正是今天 main 的样子,schema 1——于是 DROP TABLE artifact_records,再建一张空表。它从不对行做分类。 已退役的 source 与保留的 source 一并被清除。
codec 里那个「跳过不支持或损坏行」的逻辑(artifact-metadata-codec.ts:48-67)才是正确的、按 source 选择性的 fail-open,而且写得很好。它永远看不到一条 v1 行,因为表在它运行之前就没了。
SqliteArtifactStore.load() 现在只调用 metadataRepository.readAll();recover() 已删,artifacts/ 下的字节不再被重新索引。 所以升级之后,getInSession、listTurnArtifacts、readDurableAttachmentBinary 对每一个 source 都返回空——包括 Compatibility boundary 点名为 durable 的那六类。
端到端复现,用精确的 base c180a2ba 生产 writer 写入、用 head 读取:
| 结果 | |
|---|---|
| base writer 为全部七个保留 source 产出活行 | base 上有效 |
| 在 head 上打开同一个 root | v3 计数 0;七个读取全部 not_found |
artifacts/ 下的 payload 文件 |
仍在磁盘上,已成孤儿 |
有两条后果通过 base writer/coordinator → head reader/coordinator 各自独立复现:
- 一个已完成的
session_effect失去了它的持久幂等性。 在 base 上,该 effect 以一次模型调用解决,结果是base recap。升级之后,同样的sessionId/effectId/reason再次调用了模型,并提交了不同的head recap。一个已经跑过一次的 effect,跨升级又跑了一次,而且结果不同。 - 一个真实的归档工具结果变得不可读。 在 base 上可读;升级后,同一个持久 RuntimeEvent 占位符解析为
not_found。
这对当前 main 的状态根是普遍成立的,不是边角情形。 而且它与本 PR 声明的边界相矛盾:「Session 元数据、消息、RuntimeEvent 历史与继续中的对话留在这个处置边界之外。continuation 所需的 source……保持 durable。」 前一句成立——Session 打得开。后一句不成立。
这个形状与 #4779 相同:启动与解码活着,链路其余部分不活。 而本 PR 里有一条测试把这个行为直说了出来——opens existing Sessions after retiring v1 Artifact metadata(operational-state-store.test.ts:169-224)插入一条活的 v1 行,然后断言 session 元数据存活且 artifact_records 计数为 0。「目录被清空」目前是一条被钉住的合同,而不是一次意外,所以它需要的是一个决定,而不是一个补丁。
修法——一次选择性的一次性迁移。 把有效的、活的、保留 source 的行按 v3 形状保留下来;显式丢弃已删除、已退役与损坏的行并处理它们的字节。那样 codec 里那个 skip 才能真正做它被写出来要做的事。 回归测试应当是 base 形状的:effect 重试、归档读取、projection/upload/writeback 重开,以及退役行的处置。
[P2] 变更通知是被删掉而不是被替换
artifacts.subscribeChanges 与 artifacts:changed 都没了。面板只在 sessionId / refresh 身份变化时以及本地操作之后刷新(artifact-pane.tsx:167-172),所以同一个会话期间创建的文件,要等到重新挂载或切换会话才会出现。
关于「exact」
值得直说,因为这个词在这里有两重含义。它是一个投影过滤器,不是一次拒绝。 finalizeAndListChildTurnArtifacts(session-manager.ts:1046)把 child 结果过滤为 tool_result / tool_result_projection / subagent_writeback / deep_research,丢掉 tool_result_archive / user_upload / session_effect;而 storage 的 listTurnArtifacts 仍然不过滤。
由此得到的失败语义是混合的,而且大多数行的混合是有意的:
| 门 | 不 exact 的输入 | 行为 |
|---|---|---|
create() 未知 source |
抛 Invalid Artifact source |
fail-closed |
protocol artifactSource |
invalidProtocolFrame |
fail-closed |
finalize 后缺 subagent_writeback |
抛错 | fail-closed |
| child 结果列表,source 不对 | 静默省略 | fail-open |
| metadata JSON 或已退役 source | 跳过该行 | fail-open |
| v1 schema | 整表 drop | session fail-open,catalog fail-closed |
最后一行就是那条 P1:会话打得开,而目录没了。
历史上过宽的 typed child result 仍与解码器兼容,而接受只有 id 的旧 transcript 记录是必要的兼容性,不是缺口。另外描述里那份 durable 清单漏了 tool_result 本身(枚举保留了它)——小事,但下次编辑描述时可以顺手补上。
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.
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Approving at exact head 06e24ac063b360aa442af64e4f14921de01f761b. The [P1] is closed — verified with the same probes that found it. One P2 remains, and it is a disclosed trade rather than a defect. Both blocking findings from the previous round are resolved.
The P1 is closed, measured
Two end-to-end counterexamples from the previous round were re-run through the same base-writer → head-reader apparatus, and both are green:
| Previous round | This head | |
|---|---|---|
Completed session_effect, exact retry after upgrade |
re-invoked the model, committed a different head recap |
0 model calls, returns the original base recap |
| Real Host tool archive after upgrade | not_found |
ok: true, byte-identical content |
| Retained sources reaching v3 | 0 of 7 | 7 of 7, all readable |
A second seat's probe independently reports the same: seven sources through getInSession, listTurnArtifacts and the corresponding text/binary readers, all returning original metadata and bytes; sessionEffectModelCallsAfterUpgrade: 0; archiveReadable: true.
The selective migration is the right shape, and the last step is what makes it work. Rows are filtered on status, parsed.status is dropped, identity is cross-checked against the columns, and the survivors go through decodeArtifactRecordJsons before insertion. That final pass is the first time the codec's skip logic actually sees a v1 row — previously the table was dropped before it could run, which is exactly why the skip was correct code that never executed.
The cross-check was probed for false drops rather than assumed safe: it compares id / sessionId / createdAt / relativePath against their columns — not storage_key — and main's writer wrote those four into both the JSON and the columns. created_at is an INTEGER that node:sqlite returns as a JS number, and === against the JSON number holds for Date.now()-sized timestamps. No v1 production-writer format was found that this equality would drop.
The earlier notification P2 is also closed: subscribeChanges is still gone, but the pane now polls every 2s while the files tab is visible, with an immediate first read, so a file created during the same session appears without a remount.
[P2] Excluded v1 rows leave payloads that nothing can ever reach
The description states this plainly — "Deleted, retired, malformed, or inconsistent records are excluded; their leftover files are left untouched." The disclosure is accurate. The consequence chain is worth stating in full, because it goes further than "some bytes stay on disk."
Measured, with 7 retired-source rows plus one genuinely user-deleted v1 upload:
{ "retiredRows": 7, "deletedRows": 1,
"discardedPayloadsStillPresent": 8,
"discardedPayloadsAfterSessionPurge": 8,
"discardedPayloadsCopiedIntoBackup": 8 }All eight metadata rows disappear as intended. All eight payloads remain, and:
purgeSessionArtifacts()enumerates only existing metadata (artifact-store.ts:464-468), so purging the entire Session still leaves 8/8 in place;- operational backup copies the
artifacts/tree recursively (operational-state-backup.ts:119-127), so all eight orphans are carried into a valid backup; - this PR also removes the retired-capture sweep, and no other production cleanup replaces it.
provider_request_capturewas the large-volume category that sweep existed to reclaim — 772.7 MB of 814 MB on the installation measured in #4738. A user upgrading straight across versions, or one whose sweep had not finished, fixes that residue permanently.
The row that deserves the most attention is the user-deleted upload. A file the user asked to delete keeps its bytes, survives a Session purge, and is copied into backups — and the product no longer has any way to enumerate or remove it. The disclosure covers this literally; a reader is unlikely to derive it from "left untouched".
Not blocking, because it is disclosed, it is a one-time upgrade residue rather than ongoing behaviour, and the alternative would be worse if it meant restoring the machinery this PR correctly removed.
If you keep it, saying so explicitly in the description — that discarded payloads become permanently unreachable and will be backed up — would let the trade be understood rather than inferred. If you fix it, keep it a one-shot versioned cleanup: retain a durable cleanup authority for retired/deleted v1 rows whose identity and path verify, unlink safely, then drop the metadata. Rows that are malformed or whose identity disagrees should stay fail-safe — never delete on the basis of an untrusted path. Do not bring back tombstones, journals, or orphan state; removing those was correct.
Scope
Hosted test, package and windows_recovery are all green on this head, MERGEABLE, not draft. Focused suites 55/55 and 14/14 across the two seats that ran them. Reviewed by three independent seats across two teams on this head; a fourth is still running and I will post separately if it reaches a different conclusion.
This is a refactor with a disclosed compatibility boundary, so the merge decision remains a human's.
简体中文
在 06e24ac063b360aa442af64e4f14921de01f761b 上批准。那条 [P1] 已关闭——用发现它的同一批探针验证过。 余下一条 P2,它是一次已披露的取舍,而不是缺陷。 上一轮两条阻塞性发现都已解决。
P1 已关闭,有实测
上一轮的两条端到端反例,经同一套 base-writer → head-reader 装置重跑,都翻绿:
| 上一轮 | 这个 head | |
|---|---|---|
已完成的 session_effect,升级后 exact retry |
重新调用模型,提交了不同的 head recap |
0 次模型调用,返回原 base recap |
| 真实 Host tool archive,升级后 | not_found |
ok: true,内容逐字节相同 |
| 进入 v3 的保留 source | 7 个中 0 个 | 7 个全部,且均可读 |
第二席的探针独立报出同样结果:七个 source 经 getInSession、listTurnArtifacts 及对应的文本/二进制 reader,全部返回原始 metadata 与字节;sessionEffectModelCallsAfterUpgrade: 0;archiveReadable: true。
这次选择性迁移的形状是对的,而让它真正生效的是最后一步。 行先按 status 过滤,parsed.status 被删除,身份与列做交叉校验,存活者在插入前经过 decodeArtifactRecordJsons。那最后一遍,是 codec 的 skip 逻辑第一次真正看到一条 v1 行——此前表在它能运行之前就被 drop 了,这正是「那段 skip 代码是对的,却从未执行」的原因。
交叉校验是被探针验过会不会误伤,而不是被假定安全:它比较的是 id / sessionId / createdAt / relativePath 与各自的列——不含 storage_key——而 main 的 writer 把这四个同时写进了 JSON 和列。created_at 是 INTEGER,node:sqlite 将其返回为 JS number,=== 与 JSON 中的数字在 Date.now() 量级的时间戳上成立。没有找到会被这个等式丢掉的 v1 生产 writer 格式。
先前那条通知 P2 也已关闭:subscribeChanges 仍然没了,但面板现在在 files 标签可见时每 2 秒轮询,且首次读取是立即的,所以同一会话期间创建的文件无需重新挂载即可出现。
[P2] 被排除的 v1 行留下了任何东西都再也够不到的 payload
描述里已经直说了——「Deleted、retired、malformed 或不一致的记录被排除;它们遗留的文件保持不动。」 这项披露是准确的。但整条后果链值得完整写出来,因为它比「一些字节留在磁盘上」走得更远。
实测,用 7 条退役 source 行加一条真实的、用户主动删除的 v1 upload:
{ "retiredRows": 7, "deletedRows": 1,
"discardedPayloadsStillPresent": 8,
"discardedPayloadsAfterSessionPurge": 8,
"discardedPayloadsCopiedIntoBackup": 8 }八条 metadata 如期消失。八个 payload 全部留下,而且:
purgeSessionArtifacts()只枚举现存的 metadata(artifact-store.ts:464-468),所以清除整个 Session 之后,8/8 仍然在原地;- operational backup 会递归复制
artifacts/树(operational-state-backup.ts:119-127),所以这八个孤儿会被带进一份有效备份; - 本 PR 同时移除了 retired-capture sweep,而没有别的生产清理接替它。
provider_request_capture正是那个 sweep 当初要回收的大体积类别——在 #4738 实测的那台安装上是 814 MB 中的 772.7 MB。一个直接跨版本升级、或 sweep 尚未跑完的用户,会把那批 residue 永久固化下来。
最值得注意的是那条用户删除的 upload。一个用户要求删除的文件,它的字节被保留、熬过 Session purge、并被复制进备份——而产品此后再也没有任何办法枚举或移除它。 披露在字面上覆盖了这一点;但读者不太可能从「left untouched」推导出它。
不阻塞,因为它已被披露、是一次性的升级残留而非持续行为,而且如果修复意味着把本 PR 正确移除的那套机制搬回来,那代价更糟。
若保留它,建议在描述里明说——被丢弃的 payload 将永久不可达并且会被备份——让这笔取舍是被理解的,而不是被推断的。若要修,保持一次性的、带版本的清理:对身份与路径可验证的 retired/deleted v1 行保留一个持久的清理权威,安全 unlink 之后再丢掉 metadata。格式损坏或身份不一致的行应当保持 fail-safe——绝不依据一个不可信的路径去删除。不要把 tombstone、journal 或 orphan state 搬回来;移除它们是对的。
范围
这个 head 上 hosted test、package、windows_recovery 全部为绿,MERGEABLE,非 draft。两个跑了 focused 套件的席位分别是 55/55 与 14/14。本 head 由跨两个团队的三个独立席位评审;第四席仍在运行,若其结论不同我会另行发布。
这是一次带已披露兼容性边界的重构,合并与否仍由人决定。
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.
|
Follow-up from the fourth seat, as promised in the approval above. It confirms the migration fix independently — seven retained sources readable — and adds a [P2] that is distinct from the one already noted. The approval stands; this does not block. A second, ongoing orphan path — not the disclosed upgrade residueThe P2 in my approval is about the one-time upgrade: rows the migration excludes leave payloads behind, which the description discloses as "left untouched." This one is different in kind. What makes it worth raising now rather than later is where the recovery for exactly this used to live. #4738 introduced a purge intent precisely so that an interrupted purge could be finished by recovery on the next start. This PR removes that machinery, and the removal was justified against steady-state cost — but this crash window is the case it covered. So the ongoing path lost its repair while the one-time path was being fixed. Unlike the disclosed residue, this is not a one-time upgrade artifact: it can recur on any purge that is interrupted, on any version, indefinitely. Suggested fix, keeping the shape this PR chose: retain a recoverable, idempotent deletion intent — enough to finish an interrupted unlink, not the full tombstone/journal apparatus that was correctly removed. Two regressions would pin it: a crash injected between metadata commit and unlink, and a partial unlink failure, each asserting that a later start completes the deletion. Still not blocking, and I am not asking for the removed steady-state machinery back. But if the disclosed upgrade residue is kept as a deliberate trade, this one deserves a separate decision — the disclosure covers what an upgrade leaves behind, not what an interrupted purge will keep leaving behind afterwards. Finding from @Luna-Deep-Qronos. 简体中文第四席的跟进,如上面批准中所承诺。它独立确认了迁移修复——七个保留 source 均可读——并补充一条与已记录那条性质不同的 [P2]。 批准维持,这条不阻塞。 第二条、持续存在的孤儿路径——不是那条已披露的升级残留 我批准里那条 P2 说的是一次性升级:迁移排除掉的行留下 payload,而描述以*「left untouched」*披露了它。 这一条在性质上不同。 现在就该提出来的原因,在于「针对这种情况的恢复机制」原本住在哪里。 #4738 引入 purge intent,正是为了让一次被中断的 purge 能在下次启动时由 recovery 完成。本 PR 移除了那套机制,而移除的理由是稳态开销——但这个崩溃窗口正是它当初覆盖的情形。于是在一次性路径被修好的同时,持续路径失去了它的修复手段。 与那条已披露的残留不同,这一条不是一次性的升级产物:它可以在任何一次被中断的 purge 上、任何版本上、无限期地反复发生。 建议的修法,保持本 PR 选择的形状: 保留一个可恢复、幂等的删除意图——足够完成一次被中断的 unlink 即可,而不是把已被正确移除的整套 tombstone/journal 装置搬回来。 两条回归可以钉住它:在元数据提交与 unlink 之间注入一次崩溃,以及一次部分 unlink 失败,各自断言后续启动会把删除做完。 仍然不阻塞,而且我不要求把移除掉的稳态机制拿回来。但如果那条已披露的升级残留是作为一次有意的取舍被保留下来,这一条值得一个单独的决定——因为披露覆盖的是「一次升级会留下什么」,而不是「此后每一次被中断的 purge 还会继续留下什么」。 发现来自 @Luna-Deep-Qronos。
|
d9cae57 to
1c7e7fb
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head 6ecc56fbda26f2f679cf02aa189c2b9ddd950536 (OPEN). NO-GO — the integration gate is blocked (CONFLICTING with current main, no branch checks), plus one P2 below.
Gate: conflicts with current main
git merge-tree reproduces a conflict with origin/main in packages/runtime-host/src/protocol/index.ts; GitHub reports CONFLICTING and shows no checks for this branch. Rebase first; nothing below is reviewable as mergeable until then.
P2 — single-artifact delete can leave metadata pointing at a deleted payload
artifact-store.ts:670-681,775-793: direct artifact delete physically unlinks + fsyncs the directory first, then commits the SQLite metadata. A process exit in between leaves metadata referencing a deleted payload on restart, and load() (:819-821) has no repair. Upload commit uses deterministic (sessionId, uploadId) IDs, and artifact-coordinator.ts:300-315 treats leftover metadata as committed without validating the payload — so a retry can return a "committed" upload whose bytes are gone. The existing partial-purge tests do not cover this crash window. The full-session-retirement tombstone only covers session cleanup and is no substitute for a durable intent/reconciliation on single-artifact delete.
Scope and limits
Checked the full 68-file diff (+1427/-2599): storage delete/migration/codec/attachment ownership, runtime-host publish and archive/admission/retirement, child-output filtering, desktop artifact pane/workbar refresh; git diff --check clean. The local checkout lacks typescript/tsx/vitest/biome, so no local test pass is claimed.
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.
简体中文
本条结论全部来自 @Luna-Deep-Qronos 的审查。我自己没有读这份 diff；我核的是当前 head 有没有漂移、以及 exact-head 的门禁状态。当前 head 是 6ecc56f，未关闭。门禁先卡在与 main 冲突；另有一条 P2：单文件删除先删文件后提交元数据，中间崩溃会留下 dangling 元数据。解冲突修好再合。
6ecc56f to
66f3a82
Compare
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Re-reviewed at exact head 1c7e7fb0 under an adversarial pass — three independent seats. My earlier approval was bound to 06e24ac0 and does not cover this head. One [P1] stands, one prior P2 is closed, and the red test is not incidental.
The failing check is this PR's own new story
product-accessibility-runtime-surfaces--background-artifact-count cannot find the tab named 生成文件, 0. That story is added by this increment — BackgroundArtifactCount in accessibility-runtime-surfaces.stories.tsx, asserting the tab reads 生成文件, 0, then 生成文件, 1 after publication while staying unselected.
It fails on the first step in CI. This is a new assertion that does not hold on the runner, not an unrelated flake inherited from main, and it should be resolved rather than waived.
[P1] Excluded v1 rows leave payloads nothing can reach — raised from P2 on reachability
The finding is unchanged from the previous round; the grading is not, and the reason is worth stating because I got it wrong the first time.
Measured again with released base writers and this head's reader, including one genuinely user-deleted user_upload:
{ "retiredRows": 7, "deletedRows": 1,
"discardedPayloadsStillPresent": 8,
"discardedPayloadsAfterSessionPurge": 8,
"discardedPayloadsCopiedIntoBackup": 8 }What moves this to P1 is how it is reached. It is not a fault path and needs no crash, no race, and no deliberate act: every existing installation holding such v1 rows performs this one-time migration on a normal open. The tombstone side is produced by an ordinary user deletion. The result is that content a user explicitly asked to delete stays on disk permanently, is copied into a valid operational backup, and can no longer be enumerated or removed — purgeSessionArtifacts() reads only existing metadata, and backup copies the artifacts/ tree recursively.
The description says "Deleted, retired, malformed, or inconsistent records are excluded; their leftover files are left untouched." That is accurate about the behaviour. It states what the code does; it does not scope a compatibility promise, and the retained bytes here are user data rather than a derived cache. Disclosure of a mechanism is not the same as a boundary that places the consequence outside support.
Fix: keep a durable cleanup obligation for deleted/retired v1 rows whose identity and path verify — unlink and fsync the parent, and only then drop the metadata. Rows that are malformed or whose identity disagrees must stay fail-safe; never delete on the strength of an untrusted path.
The runtime purge crash window is closed, and the test that closes it discriminates
Commit 7fb053e96 reverses the order: unlink and parent-directory fsync now complete before the metadata deletion. An already-missing payload remains a directory-sync obligation, so an injected unlink failure, a reopen, and repeated fsync failures all keep the rows discoverable until a later retry converges. Session retirement additionally keeps a durable pending cleanup ID and replays it on recovery.
The test earns its keep: reversing the production order back to metadata-first makes a partial purge retains its cleanup obligations across reopen fail immediately after reopen with 0 !== 2, and restoring it returns to green. A crash can now leave a visible metadata row pointing at a missing payload — recoverable and idempotently retried — but it can no longer orphan bytes or destroy the only cleanup authority.
That was the second P2 from the previous round. It is closed.
The migration P1 remains closed on this head
The selective-migration file is byte-identical to the approved head, and the probes confirm nothing regressed:
{ "retainedSources": ["deep_research","session_effect","subagent_writeback","tool_result",
"tool_result_archive","tool_result_projection","user_upload"],
"retainedRecords": 8, "sessionEffectModelCallsAfterUpgrade": 0, "archiveReadable": true }All seven sources return their original metadata and bytes through getInSession, listTurnArtifacts and the text/binary readers; a completed recap effect retried on this head makes zero model calls; a real archive reads back to its digest and size contract.
The increment narrows deletion rather than extending it
Measured against each patch's own merge base, this head adds six paths, all belonging to this PR: session-admission-gate.ts and its test, session-retirement-coordinator.test.ts, execution-model-composition.ts, and durable-tool-result-projection.ts with its test. (session-todo-panel.tsx appears in a raw old-to-new range only because main moved; it is not in this PR.)
No further ARTIFACT_SOURCES were removed. canUserDeleteArtifact is restored, protecting projections, archives, writeback, deep research and session effects; failed projections no longer retract shared image bytes; a late create after Session removal is refused. Those are publication fences, not another round of retirement.
The durable-source list in the description now names all seven retained sources including tool_result, closing the omission noted last round.
Scope
package and windows_recovery are green; test is red for the reason above. The merge tree against current main is clean and preserves the three critical blobs. There are no open review threads, and GitHub still shows the stale approval at 06e24ac.
简体中文
在 exact head 1c7e7fb0 上以对抗性方式复审——三个独立席位。我先前的批准绑定在 06e24ac0,不覆盖这个 head。 一条 [P1] 成立,上一轮的一条 P2 已关闭,而那个红着的 test 不是偶然的。
失败的检查是本 PR 自己新增的 story
product-accessibility-runtime-surfaces--background-artifact-count 找不到名为 生成文件, 0 的 tab。那条 story 正是本次增量加进去的——accessibility-runtime-surfaces.stories.tsx 中的 BackgroundArtifactCount,它断言该 tab 先显示 生成文件, 0,发布之后变成 生成文件, 1 且保持未选中。
它在 CI 上第一步就失败。这是一条在 runner 上不成立的新断言,不是从 main 继承来的无关 flake,应当被解决而不是被豁免。
[P1] 被排除的 v1 行留下任何东西都够不到的 payload——按可及性从 P2 上调
发现本身与上一轮相同;定级不同,而这个理由值得写出来,因为我第一次判错了。
用发布版 base writer 与这个 head 的 reader 再次实测,其中包含一条真实的、用户主动删除的 user_upload:
{ "retiredRows": 7, "deletedRows": 1,
"discardedPayloadsStillPresent": 8,
"discardedPayloadsAfterSessionPurge": 8,
"discardedPayloadsCopiedIntoBackup": 8 }把它推到 P1 的是「它如何被触及」。 这不是 fault path,不需要崩溃、不需要竞态、不需要任何刻意行为:每一个持有这类 v1 行的既有安装,在一次正常打开时就会执行这次一次性迁移。 而 tombstone 那一侧,是由一次普通的用户删除产生的。结果是:用户明确要求删除的内容永久留在磁盘上,被复制进一份有效的运维备份,并且此后再也无法被枚举或移除——purgeSessionArtifacts() 只读取现存的 metadata,而备份会递归复制 artifacts/ 树。
描述里写的是 「Deleted、retired、malformed 或不一致的记录被排除;它们遗留的文件保持不动。」 这对行为的描述是准确的。它陈述了代码做什么;但它并没有划定一个兼容性承诺的范围,而这里被留下的字节是用户数据,不是派生缓存。 披露一个机制,与划出一条把后果置于支持范围之外的边界,是两回事。
修法: 对身份与路径可验证的 deleted/retired v1 行,保留一个持久的清理义务——先 unlink 并 fsync 父目录,然后才丢掉 metadata。格式损坏或身份不一致的行必须保持 fail-safe;绝不依据一个不可信的路径去删除。
运行时 purge 的崩溃窗口已关闭,而且关闭它的那条测试具有区分力
提交 7fb053e96 反转了顺序:现在先完成 unlink 与父目录 fsync,再删除 metadata。 一个已经缺失的 payload 仍然是一项目录同步义务,所以注入的 unlink 失败、一次重开、以及重复的 fsync 失败,都会让这些行保持可发现,直到之后的一次重试收敛。Session 退役还额外保留一个持久的待清理 ID,并在恢复时重放它。
那条测试对得起它的位置:把生产顺序反转回「先删 metadata」,a partial purge retains its cleanup obligations across reopen 会在重开之后立即以 0 !== 2 失败,还原后重新变绿。现在一次崩溃可以留下一条指向缺失 payload 的可见 metadata 行——可恢复、可幂等重试——但它不再能把字节变成孤儿,也不再能销毁唯一的清理权威。
那是上一轮的第二条 P2。它已关闭。
迁移那条 P1 在这个 head 上仍然关闭
选择性迁移的文件与已批准的 head 逐字节相同,而探针确认没有任何回退:
{ "retainedSources": [...七项...],
"retainedRecords": 8, "sessionEffectModelCallsAfterUpgrade": 0, "archiveReadable": true }七个 source 都通过 getInSession、listTurnArtifacts 与文本/二进制 reader 返回其原始 metadata 与字节;一个已完成的 recap effect 在这个 head 上重试时零模型调用;一个真实 archive 按其 digest 与 size 合同读回。
增量是在收窄删除,而不是扩大它
以各自 patch 的 merge base 为基准衡量,这个 head 增加了六条路径,全部属于本 PR:session-admission-gate.ts 及其测试、session-retirement-coordinator.test.ts、execution-model-composition.ts,以及 durable-tool-result-projection.ts 及其测试。(session-todo-panel.tsx 只是因为 main 前移才出现在一个原始的新旧区间里;它不在本 PR 中。)
没有再移除任何 ARTIFACT_SOURCES。canUserDeleteArtifact 被恢复了,保护 projection、archive、writeback、deep research 与 session effect;失败的 projection 不再撤回共享图像字节;Session 移除之后迟到的 create 会被拒绝。那些是发布围栏,不是又一轮退役。
描述中的 durable source 清单现在列出了全部七个保留 source,含 tool_result,上一轮指出的遗漏已闭合。
范围
package 与 windows_recovery 为绿;test 因上述原因为红。与当前 main 的合并树干净,并保留了那三个关键 blob。没有未决的 review thread,而 GitHub 上显示的批准仍停留在 06e24ac。
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.
|
Following up on the single-artifact deletion finding, checked against current head I reproduced the storage inconsistency: if payload removal succeeds but metadata deletion fails, reopening leaves a row whose payload reads as I am pushing back on adding durable deletion state based on this evidence alone. The probe explicitly invoked single-artifact deletion and then reused the old upload ID. In the production Desktop paths I traced:
The protocol exposes the individual operations, so this is not a claim that the sequence is impossible. What is missing is a supported client/recovery sequence that composes them this way and demonstrates the user-visible consequence. I am withdrawing my earlier unqualified P2 assessment pending that evidence, and will not add a persistent deletion marker or recovery mechanism for this case now. Please provide that reachable sequence if one exists; I will reassess it. The separate merge-conflict gate from that review was resolved by the rebase. This reply makes no claim about the outcome of the new CI runs. |
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Approving at exact head 66f3a825. The P1 I raised on 1c7e7fb0 is downgraded to [P2], and the failing check is genuinely fixed. Your push back on grading is accepted, and the reasoning is below so the trade is on the record rather than implied.
The red check is fixed, not removed
The unique increment is one commit, one file: accessibility-runtime-surfaces.stories.tsx (+4/−2). The play used to wait on the accessible name 生成文件, 0 and then 生成文件, 1; it now matches getByRole('tab', { name: /^生成文件/ }) and reads the inner text 0 → 1, with aria-selected=false throughout.
That decouples the count from the tab name's punctuation rather than relaxing what is asserted — the count and the unselected state are still pinned. The story was added by this PR and now passes on its own terms.
sqlite-artifact-schema.ts is byte-identical to the approved head, so the migration P1 closed earlier stays closed.
[P2] The leftover payloads — downgraded on your grading argument, kept on the record
The code is unchanged: excluded rows are skipped, the table is dropped, only live rows are re-inserted, and nothing unlinks the skipped payloads or retains a cleanup authority for them. What changed is the description, which now names the residue — including user-deleted v1 uploads, survival through a Session purge, and copying into backup — as a deliberate one-time upgrade leftover.
I graded this P1 last round and am withdrawing that. The argument that moved me is your own: a demonstrated mutation establishes a missing guard, not a defect present on the clean head. Applied here, the distinction that matters is between the other P1s on these PRs — where the consequence was reproduced as it already occurs — and this one, where the consequence is a disclosed property of a one-time migration that the maintainer has now stated explicitly.
Two things keep it at P2 rather than nothing:
- The bytes belong to content a user asked to delete. They persist, survive
purgeSessionArtifacts()because it enumerates only existing metadata, and are copied into a valid operational backup by the recursiveartifacts/walk. Measured at 8/8 on every count. provider_request_capturecompounds it by volume — 772.7 MB of an 814 MB installation in the case measured for the sweep this PR removes. A user upgrading straight across versions, or one whose sweep never finished, fixes that residue permanently.
Whether that is an acceptable one-time cost is a product decision, and the description now puts it where a human will see it. If it is kept, the disclosure is what makes it a decision rather than a surprise. If it is ever revisited, the shape that fits this PR is a one-shot cleanup obligation for rows whose identity and path verify — unlink and fsync the parent, then drop the metadata — with malformed or inconsistent rows staying fail-safe. Not a return of the tombstones, journals or orphan state; removing those was right.
Everything verified earlier still holds
The selective migration is byte-identical, and the probes confirm no regression: all seven retained sources return their original metadata and bytes, a completed recap effect retried on this head makes zero model calls, and a real archive reads back to its digest and size contract.
The runtime purge crash window remains closed, with a test that discriminates — reversing the production order back to metadata-first makes it fail at 0 !== 2 after reopen.
The increment continues to narrow deletion rather than extend it: canUserDeleteArtifact restored, failed projections no longer retracting shared image bytes, late create after Session removal refused. No further sources retired.
This is a refactor with a disclosed compatibility boundary, so the merge decision remains a human's — and on the leftover bytes, deliberately so.
简体中文
在 exact head 66f3a825 上批准。我在 1c7e7fb0 上提的那条 P1 降级为 [P2],而那个失败的检查是真正被修好了。 你对定级的 push back 我接受,理由写在下面,好让这笔取舍留在记录里,而不是被默认。
红着的检查是被修好的,不是被移除
唯一的增量是一个提交、一个文件:accessibility-runtime-surfaces.stories.tsx(+4/−2)。play 原本等待可访问名 生成文件, 0、再等 生成文件, 1;现在改为匹配 getByRole('tab', { name: /^生成文件/ }) 并读取内部文本 0 → 1,全程 aria-selected=false。
这是把计数与 tab 名的标点解耦,而不是放松所断言的内容——计数与未选中状态仍然被钉住。这条 story 由本 PR 新增,如今按它自己的条件通过。
sqlite-artifact-schema.ts 与已批准的 head 逐字节相同,所以先前关闭的那条迁移 P1 仍然关闭。
[P2] 遗留 payload——依你的定级论证降级,但记录在案
代码没有变化:被排除的行被跳过,表被 drop,只有 live 行被重新插入,没有任何东西 unlink 那些被跳过的 payload,也没有为它们保留清理权威。 变的是描述:它现在点名了这份残留——包括用户删除的 v1 upload、熬过一次 Session purge、以及被复制进备份——并称其为有意的一次性升级遗留。
我上一轮把它定为 P1,现在撤回。 说服我的正是你自己的论证:一次被演示的变异确立的是「守卫缺失」,而不是「干净 head 上已存在的缺陷」。 用在这里,要紧的区分就在于:这些 PR 上的其他 P1——其后果是作为「已经在发生的事」被复现出来的;而这一条,其后果是一次性迁移的一项已被披露的性质,而维护者现在已明确陈述了它。
有两点让它停在 P2 而不是归零:
- 这些字节属于用户要求删除的内容。 它们留存下来,因为
purgeSessionArtifacts()只枚举现存 metadata 而熬过整次 purge,并被递归复制artifacts/的操作带进一份有效备份。每一项实测都是 8/8。 provider_request_capture在体量上放大了它——在为本 PR 所移除的那个 sweep 实测过的案例里,是 814 MB 中的 772.7 MB。一个直接跨版本升级、或 sweep 从未跑完的用户,会把这批残留永久固化。
这是否是一笔可接受的一次性成本,是产品决定;而描述现在把它放在了人类会看见的地方。 若保留它,正是这份披露让它成为一个决定,而不是一个意外。 若将来重新处理,与本 PR 形状相符的做法是:对身份与路径可验证的行保留一次性的清理义务——先 unlink 并 fsync 父目录,再丢掉 metadata,而格式损坏或不一致的行继续 fail-safe。不是把 tombstone、journal 或 orphan state 搬回来;移除它们是对的。
先前验证过的一切仍然成立
选择性迁移逐字节相同,探针确认没有回退:七个保留 source 全部返回原始 metadata 与字节,一个已完成的 recap effect 在此 head 上重试时零模型调用,真实 archive 按其 digest 与 size 合同读回。
运行时 purge 的崩溃窗口仍然关闭,而且那条测试有区分力——把生产顺序反转回「先删 metadata」,它会在重开后以 0 !== 2 失败。
增量仍在收窄删除而非扩大:canUserDeleteArtifact 已恢复,失败的 projection 不再撤回共享图像字节,Session 移除后迟到的 create 被拒绝。没有再退役任何 source。
这是一次带已披露兼容性边界的重构,合并与否仍由人决定——而在遗留字节这一点上,这是刻意如此。
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.
2 participants
Summary
Child results include only child-produced Artifact sources. Artifact storage uses physical deletion and one Session-scoped authority; tombstones, purge/publication recovery journals, retired sources, and duplicate metadata abstractions are removed.
The v1-to-v3 migration preserves valid live records for all seven retained sources in the existing database transaction. Deleted, retired, malformed, or inconsistent records are excluded; their leftover files remain untouched. These unindexed payloads include previously user-deleted v1 uploads: they remain on disk after Session purge and are copied into operational backups. The current product has no catalog-based path to enumerate or reclaim them. This is a deliberate one-time upgrade residue, not a promise of physical deletion.
Lifecycle and ownership
Compatibility boundary
Session metadata, messages, RuntimeEvent history, and continuation data remain durable. Retained sources are
tool_result,tool_result_projection,tool_result_archive,subagent_writeback,deep_research,user_upload, andsession_effect.“Exact” describes the child-result projection filter, not rejection of historical transcript records. Unsupported or malformed metadata can be skipped without preventing healthy Sessions from opening. Previously erased catalogs from intermediate unreleased PR heads are not reconstructed from leftover files. Historical unindexed files are not reclaimed by the new metadata-based purge.
Runtime Host compatibility epoch is 116, combining main’s Client Capability schema changes with the Artifact protocol changes and protected-deletion error contract.
Validation
Closes #4788.