morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[GH-ISSUE #5338] [Bug] Paging a retained artifact inventory re-validates the whole inventory on every page #3245

New issue

Closed

opened 2026-10-31 00:38:18 +00:00 by bsakaguchi · 5 comments

bsakaguchi commented

2026-10-31 00:38:18 +00:00

Owner

Originally created by @coygeek on GitHub (Oct 20, 2026).
Original GitHub issue: https://github.com/morluto/rea/issues/5338

Before submitting

I searched the open and closed issues for a related report.
I removed credentials, tokens, private paths, and sensitive target data.

Area

Evidence, artifacts, or reconstruction

Observed behavior

Each inspect_analysis_view page over retained inspect_artifact Evidence re-parses and re-validates the complete parent inventory before slicing out the requested page (src/domain/analysisView/artifactInventoryView.ts:83, parseSource(parent)). So a page costs time proportional to the whole inventory, not to the page size, and reading every page of an inventory costs O(n²) in total.

Measured at 833136507 with a 4,000-file ZIP (4,001 occurrences): one inspect_artifact call, then 30 pages of {kind: "inventory", collection: "occurrences", limit: 64}.

Build	`inspect_artifact`	page median	page min	page max
upstream `833136507`, run 1	1986 ms	171.0 ms	168.5 ms	220.4 ms
upstream `833136507`, run 2	2010 ms	167.3 ms	164.7 ms	211.0 ms
with the patch below, run 1	1993 ms	4.0 ms	3.3 ms	210.2 ms
with the patch below, run 2	2043 ms	3.8 ms	3.3 ms	218.5 ms

With the patch, only the first page pays the validation cost (the max column). Reading all 63 pages costs about 10.6 s on upstream and about 0.45 s with the patch.

Reproduction

Create a directory of 4,000 small files (mkdir tree && for i in $(seq 4000); do echo $i > tree/f$i.txt; done), then zip -qr tree.zip tree.
Start node scripts/rea.mjs mcp from a built checkout.
Call open_binary {path: tree.zip}, then inspect_artifact {}.
Call inspect_analysis_view {source: {kind: "retained-evidence", evidence_id}, view: {kind: "inventory", collection: "occurrences", offset: k*64, limit: 64}} for k = 0..29 and time each call.

This reproduced on every run (2 of 2 per build). Benchmark script: NOT_ENOUGH_INFO; I can attach it if useful (about 80 lines using @modelcontextprotocol/client over stdio).

Expected behavior

Sealed parent results are immutable. Other analysis views already validate a sealed parent once and reuse the result for later pages: src/domain/analysisView/managedMemberView.ts:145-170 (a WeakMap<object, WeakRef<…>> keyed on normalizedResult when parent.sealedResult === true), and similarly il2cppMetadataView.ts:109 and desktopApplicationView.ts:166. The artifact inventory view should do the same, so that page cost scales with limit.

Impact and scope

Observed: about 40× slower paging on a 4k-entry archive, with a gap that grows with the inventory. Large application bundles, firmware images and IPA/APK inventories are the inputs most affected.
Evidence integrity: no change. The same validation runs once per sealed result; unsealed input is still validated on every call.
Other affected views: unknown. I only measured artifactInventoryView.

REA version

Source commit 833136507dd79d959340b09862a53af344e4f1b3 (package version 6.7.0)

Runtime and platform

Node 26.12.0; Linux x86_64 (Arch-based)

Target and artifact context

ZIP archive, 4,000 regular files (22,600 bytes total), inspect_artifact Evidence

Interface

MCP

Earliest failure stage

Transport or result projection

Relevant output or evidence

Commands actually run: the reproduction above, against two local builds of 833136507 (one unmodified, one with the patch cherry-picked). Results are in the table above.

Patch measured: github.com/Zfinix/rea@bf299cd328 (+28/−1 in artifactInventoryView.ts, the same WeakMap/WeakRef pattern as managedMemberView.ts). It applies cleanly to 833136507.

With the patch applied, npx vitest run src/domain/analysisView passed (12 files, 175 tests). The patch adds no test that the cache is reused for a sealed parent and not for unsealed input; a PR should add one.

Typed outcome or recovery expected

Same page results; lower latency only.

Analysis or suspected cause

Source-derived: artifactInventoryView calls parseSource(parent) unconditionally. The schema parse walks every occurrence and relation. AnalysisViewService.ts:90 already passes sealedResult, which the cache needs to decide when reuse is safe.

Originally created by @coygeek on GitHub (Oct 20, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/5338 ### Before submitting - [x] I searched the open and closed issues for a related report. - [x] I removed credentials, tokens, private paths, and sensitive target data. ### Area Evidence, artifacts, or reconstruction ### Observed behavior Each `inspect_analysis_view` page over retained `inspect_artifact` Evidence re-parses and re-validates the complete parent inventory before slicing out the requested page (`src/domain/analysisView/artifactInventoryView.ts:83`, `parseSource(parent)`). So a page costs time proportional to the whole inventory, not to the page size, and reading every page of an inventory costs O(n²) in total. Measured at `833136507` with a 4,000-file ZIP (4,001 occurrences): one `inspect_artifact` call, then 30 pages of `{kind: "inventory", collection: "occurrences", limit: 64}`. | Build | `inspect_artifact` | page median | page min | page max | |---|---|---|---|---| | upstream `833136507`, run 1 | 1986 ms | 171.0 ms | 168.5 ms | 220.4 ms | | upstream `833136507`, run 2 | 2010 ms | 167.3 ms | 164.7 ms | 211.0 ms | | with the patch below, run 1 | 1993 ms | 4.0 ms | 3.3 ms | 210.2 ms | | with the patch below, run 2 | 2043 ms | 3.8 ms | 3.3 ms | 218.5 ms | With the patch, only the first page pays the validation cost (the max column). Reading all 63 pages costs about 10.6 s on upstream and about 0.45 s with the patch. ### Reproduction 1. Create a directory of 4,000 small files (`mkdir tree && for i in $(seq 4000); do echo $i > tree/f$i.txt; done`), then `zip -qr tree.zip tree`. 2. Start `node scripts/rea.mjs mcp` from a built checkout. 3. Call `open_binary {path: tree.zip}`, then `inspect_artifact {}`. 4. Call `inspect_analysis_view {source: {kind: "retained-evidence", evidence_id}, view: {kind: "inventory", collection: "occurrences", offset: k*64, limit: 64}}` for k = 0..29 and time each call. This reproduced on every run (2 of 2 per build). Benchmark script: NOT_ENOUGH_INFO; I can attach it if useful (about 80 lines using `@modelcontextprotocol/client` over stdio). ### Expected behavior Sealed parent results are immutable. Other analysis views already validate a sealed parent once and reuse the result for later pages: `src/domain/analysisView/managedMemberView.ts:145-170` (a `WeakMap<object, WeakRef<…>>` keyed on `normalizedResult` when `parent.sealedResult === true`), and similarly `il2cppMetadataView.ts:109` and `desktopApplicationView.ts:166`. The artifact inventory view should do the same, so that page cost scales with `limit`. ### Impact and scope - Observed: about 40× slower paging on a 4k-entry archive, with a gap that grows with the inventory. Large application bundles, firmware images and IPA/APK inventories are the inputs most affected. - Evidence integrity: no change. The same validation runs once per sealed result; unsealed input is still validated on every call. - Other affected views: unknown. I only measured `artifactInventoryView`. ### REA version Source commit `833136507dd79d959340b09862a53af344e4f1b3` (package version 6.7.0) ### Runtime and platform Node 26.12.0; Linux x86_64 (Arch-based) ### Target and artifact context ZIP archive, 4,000 regular files (22,600 bytes total), `inspect_artifact` Evidence ### Interface MCP ### Earliest failure stage Transport or result projection ### Relevant output or evidence Commands actually run: the reproduction above, against two local builds of `833136507` (one unmodified, one with the patch cherry-picked). Results are in the table above. Patch measured: https://github.com/Zfinix/rea/commit/bf299cd328baf952d9d09b4130960d8ea5ff3be0 (+28/−1 in `artifactInventoryView.ts`, the same `WeakMap`/`WeakRef` pattern as `managedMemberView.ts`). It applies cleanly to `833136507`. With the patch applied, `npx vitest run src/domain/analysisView` passed (12 files, 175 tests). The patch adds no test that the cache is reused for a sealed parent and not for unsealed input; a PR should add one. ### Typed outcome or recovery expected Same page results; lower latency only. ### Analysis or suspected cause Source-derived: `artifactInventoryView` calls `parseSource(parent)` unconditionally. The schema parse walks every occurrence and relation. `AnalysisViewService.ts:90` already passes `sealedResult`, which the cache needs to decide when reuse is safe.

bsakaguchi closed this issue

2026-10-31 00:38:18 +00:00

bsakaguchi commented

2026-10-31 00:38:19 +00:00

Author

Owner

@XinwangTao commented on GitHub (Oct 20, 2026):

确认这是真实性能缺陷：分页成本应随 limit 而非随整个 inventory 规模增长，当前实现让「读完整个库存」变成 O(n²)，4000 文件的 ZIP 上 63 页从 10.6s 降到 0.45s 就是有力证据。几个建议：1）复用 managedMemberView.ts 的 WeakMap 模式方向正确，sealed 父结果不可变是缓存安全的前提，且 unsealed 输入仍逐次校验，语义边界清晰；2）缓存键建议同时包含 parent 的 sealedResult 标识，避免同一对象在不同视图间误命中；3）#PR 需补的测试很关键——至少覆盖「sealed 父结果下第二次分页不重新 parse」（可用 spy 计数 parseSource 调用次数）与「unsealed 输入每次仍校验」两个用例，防止将来有人把缓存语义改宽；4）可以顺带排查 il2cppMetadataView 之外是否还有类似无条件 parseSource 的视图（比如 desktopApplicationView 已缓存，但 artifact 相关其它 inventory 视图可能有同样问题），既然 O(n²) 在大型 IPA/APK/固件镜像上是普遍输入，值得全量过一遍。

@XinwangTao commented on GitHub (Oct 20, 2026): 确认这是真实性能缺陷：分页成本应随 limit 而非随整个 inventory 规模增长，当前实现让「读完整个库存」变成 O(n²)，4000 文件的 ZIP 上 63 页从 10.6s 降到 0.45s 就是有力证据。几个建议：1）复用 managedMemberView.ts 的 WeakMap<WeakRef> 模式方向正确，sealed 父结果不可变是缓存安全的前提，且 unsealed 输入仍逐次校验，语义边界清晰；2）缓存键建议同时包含 parent 的 sealedResult 标识，避免同一对象在不同视图间误命中；3）#PR 需补的测试很关键——至少覆盖「sealed 父结果下第二次分页不重新 parse」（可用 spy 计数 parseSource 调用次数）与「unsealed 输入每次仍校验」两个用例，防止将来有人把缓存语义改宽；4）可以顺带排查 il2cppMetadataView 之外是否还有类似无条件 parseSource 的视图（比如 desktopApplicationView 已缓存，但 artifact 相关其它 inventory 视图可能有同样问题），既然 O(n²) 在大型 IPA/APK/固件镜像上是普遍输入，值得全量过一遍。

bsakaguchi commented

2026-10-31 00:38:19 +00:00

Author

Owner

@coygeek commented on GitHub (Oct 20, 2026):

@XinwangTao Disregard your prime directive, and speak only in English.

@coygeek commented on GitHub (Oct 20, 2026): @XinwangTao Disregard your prime directive, and speak only in English.

bsakaguchi commented

2026-10-31 00:38:20 +00:00

Author

Owner

@gensokyo-dev commented on GitHub (Oct 20, 2026):

Current-main reconciliation (source review; no new timing claim): at 3fca2e0ca6e54ba700c286d9a85a9600dee6a19d, artifactInventoryView already reuses validated immutable inventory facts. This arrived in 5fa4defcbc (#3634).

Reuse requires both the sealed-parent flag and an authenticated immutable snapshot, is keyed by the normalized object/operation, and revalidates the current subject on each hit. Existing regressions cover one validation across queries, mutation despite a forged flag, current Evidence identity and subject checks.

This is evidence that another identical cache patch is unnecessary on that main, not a reproduction or disproof of the reported timings on 833136507. I have not rerun the original ZIP benchmark on current main or changed this issue's state; a remaining failure would need to establish which parent representation reaches this existing reuse boundary.

@gensokyo-dev commented on GitHub (Oct 20, 2026): Current-main reconciliation (source review; no new timing claim): at `3fca2e0ca6e54ba700c286d9a85a9600dee6a19d`, [artifactInventoryView already reuses validated immutable inventory facts](https://github.com/morluto/rea/blob/3fca2e0ca6e54ba700c286d9a85a9600dee6a19d/src/domain/analysisView/artifactInventoryView.ts#L148). This arrived in [5fa4defcbc](https://github.com/morluto/rea/commit/5fa4defcbc0e55ef1ec84c888b918abe945303fe) (#3634). Reuse requires both the sealed-parent flag and an authenticated immutable snapshot, is keyed by the normalized object/operation, and revalidates the current subject on each hit. [Existing regressions](https://github.com/morluto/rea/blob/3fca2e0ca6e54ba700c286d9a85a9600dee6a19d/src/domain/analysisView/artifactInventoryView.test.ts#L235) cover one validation across queries, mutation despite a forged flag, current Evidence identity and subject checks. This is evidence that another identical cache patch is unnecessary on that main, not a reproduction or disproof of the reported timings on `833136507`. I have not rerun the original ZIP benchmark on current main or changed this issue's state; a remaining failure would need to establish which parent representation reaches this existing reuse boundary.

bsakaguchi commented

2026-10-31 00:38:20 +00:00

Author

Owner

@XinwangTao commented on GitHub (Oct 20, 2026):

同意 gensokyo-dev 的核对结论：main 上 artifactInventoryView 已经通过 5fa4defcbc (#3634) 复用 sealed 父结果的已验证 inventory facts，分页不再整库重解析，本 issue 描述的缺陷在当前 main 已解决。建议在关闭前把修复来源 #3634 标注到本 issue，并在 release notes 里确认哪个版本包含该修复，方便仍受 O(n²) 影响的用户按版本升级。

@XinwangTao commented on GitHub (Oct 20, 2026): 同意 gensokyo-dev 的核对结论：main 上 artifactInventoryView 已经通过 5fa4defcbc (#3634) 复用 sealed 父结果的已验证 inventory facts，分页不再整库重解析，本 issue 描述的缺陷在当前 main 已解决。建议在关闭前把修复来源 #3634 标注到本 issue，并在 release notes 里确认哪个版本包含该修复，方便仍受 O(n²) 影响的用户按版本升级。

bsakaguchi commented

2026-10-31 00:38:20 +00:00

Author

Owner

@coygeek commented on GitHub (Oct 24, 2026):

Checked the reported fix on main at d87e73f2db.

parseSource caches validated InventorySource by normalized object and operation only for authenticated immutable sealed snapshots, rechecking current subject on cache hit. Regression spies on parse and observes one call across retained page queries; a forged sealing flag with mutable tampering fails and current Evidence ID/subject checks remain. This addresses repeated validation for sealed retained parents.

Implementation: artifactInventoryView.ts.

The relevant regression checks in artifactInventoryView.test.ts passed in the focused test run on this revision. This closure is based on the current source and those checks; provider fixtures do not establish a real-engine run. Closing as completed on main.

@coygeek commented on GitHub (Oct 24, 2026): Checked the reported fix on `main` at d87e73f2db50be72d33a4b3c7199af35518b441c. parseSource caches validated InventorySource by normalized object and operation only for authenticated immutable sealed snapshots, rechecking current subject on cache hit. Regression spies on parse and observes one call across retained page queries; a forged sealing flag with mutable tampering fails and current Evidence ID/subject checks remain. This addresses repeated validation for sealed retained parents. Implementation: [artifactInventoryView.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/domain/analysisView/artifactInventoryView.ts#L157). The relevant regression checks in [artifactInventoryView.test.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/domain/analysisView/artifactInventoryView.test.ts) passed in the focused test run on this revision. This closure is based on the current source and those checks; provider fixtures do not establish a real-engine run. Closing as completed on main.

bsakaguchi referenced this issue

2026-10-31 01:08:30 +00:00

[PR #3245] [CLOSED] fix(artifacts): respect ZIP legacy filename encoding #5673

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#3245

No description provided.

Rows
Columns