morluto/rea

Fork 0

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

[PR #3491] [MERGED] fix(managed): avoid duplicate decode budget charging on cached metadata strings #5778

New issue

Closed

opened 2026-10-31 01:09:22 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:09:22 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3491
Author: @keeponlight
Created: 10/14/2026
Status: ✅ Merged
Merged: 10/17/2026
Merged by: @N0zoM1z0

Base: main ← Head: fix/managed-representation-budget-3449

📝 Commits (6)

ca1fa79 fix(managed): avoid duplicate decode budget charging on cached metadata strings
f1e2fba Merge branch 'main' into fix/managed-representation-budget-3449
2eecab0 Merge branch 'main' into fix/managed-representation-budget-3449
9ebeed5 Merge branch 'main' into fix/managed-representation-budget-3449
89aac64 Merge branch 'main' into fix/managed-representation-budget-3449
44b03c9 Merge branch 'main' into fix/managed-representation-budget-3449

📊 Changes

2 files changed (+33 additions, -1 deletions)

View changed files

📝 src/dotnet/ManagedMetadataHeaps.test.ts (+33 -0)
📝 src/dotnet/ManagedMetadataHeaps.ts (+0 -1)

📄 Description

Summary

Prevents readMetadataString from repeatedly deducting encodedBytes * 6 + 2 from ManagedDecodeBudget on #Strings heap cache hits (cached !== undefined).

Fixes #3449

Problem and expected behavior

Running rea inspect-managed-members against ordinary .NET assemblies (such as the 2.2 MB HP.Omen.Core.Common.dll in #3449) failed with resource-limit: Managed metadata exceeds the 67108864 byte representation budget at a non-null #Strings file offset ("offset": 938934).

While #3487 (bddeae5) resolved duplicate admitManagedProjection charges (offset: null) via #prepaid, readMetadataString in src/dotnet/ManagedMetadataHeaps.ts:126-130 still called managedDecodeBudget(layout).reserveString(cached.encodedBytes, start) on every cache hit (cached !== undefined). During inspectManagedMembersBytes, repeated lookups of the same #Strings entries across readManagedMetadataInventory, typeRanges, parseTypes, typeNameLookup, CustomAttribute, and MemberRef permanently charged encodedBytes * 6 + 2 on every lookup before admitManagedProjection charged the retained JSON output.

Change and scope

Removed the duplicate reserveString call on #Strings cache hits in readMetadataString (src/dotnet/ManagedMetadataHeaps.ts); the heap decode reservation is charged once when the string is first decoded and cached at line 144, and any retained JSON occurrences are charged by admitManagedProjection.
Added unit regression test does not re-charge the decode budget on metadata string cache hits in src/dotnet/ManagedMetadataHeaps.test.ts.
Rebased onto main (89faa863e), dropping the ManagedDecodeBudget.ts projection changes that were already landed by #3487.

Contribution scope: not applicable (ordinary fix within existing boundaries).

Contract and boundary impact

Semantic owner and earliest changed stage: managed metadata string heap reader (src/dotnet/ManagedMetadataHeaps.ts)
Contribution scope fit and linked maintainer decision: not applicable
CLI and MCP/tool-catalog contract: none
Provider, bridge, target-format, or platform compatibility: none; the 64 MiB per-inspection representation limit remains enforced for unique #Strings heap decodes and retained JSON projections
Evidence, artifact, provenance, or reconstruction contract: none
Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable; static file-backed inspection only
Generated metadata (docs/public/product-catalog.json), package, or installation impact: none

Expected effects:

Read existing files or captured data.
Launch or attach to a process.
Inject instrumentation or run scripts/code.
Change process memory, registers, files, or configuration.
Contact network services.
Recover protected code or work around protections.
Documentation or usability only; no target interaction.
Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

Tests added or updated: src/dotnet/ManagedMetadataHeaps.test.ts (does not re-charge the decode budget on metadata string cache hits).
Base reproduction or other evidence: executed. The new test fails with ManagedReaderFailure: Managed metadata exceeds the 67108864 byte representation budget on 89faa863e and passes with this fix.
User-visible CLI/MCP output (if applicable): assemblies that repeatedly reference cached #Strings heap entries across metadata tables no longer fail prematurely at a #Strings file offset with resource-limit on metadata.representation.
Remaining proof gaps: proprietary HP OMEN binaries from #3449 were not attached to the issue; verified via synthetic 10 MiB #Strings cache-hit fixture and the managed test suite.

For evidence-bearing changes:

Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

npx vitest run src/dotnet/ManagedMetadataHeaps.test.ts src/dotnet/ManagedDecodeBudget.test.ts src/dotnet/ManagedMemberInspector.test.ts — 3 files, 142 passed (1 new)
npm run check:fast — typecheck and lint passed with 0 errors
node scripts/scan-todos.mjs — No tech-debt markers found.

Compatibility, safety, and release

Breaking changes or migration steps: none
Real Hopper/Ghidra, browser, or OS coverage: macOS Darwin arm64 (Node v24.14.1)
Package or release metadata impact: none
Security, privacy, process, or containment review: preserves the 64 MiB representation cap against uncached heap allocations and repeated string expansions in JSON projections.

Maintainer review update — Linux (17 October 2026)

Reviewed head: 2eecab0e698c80e4fad98fe13223d4971b6f140e, merged with main 150e996a86f2bec4982ba87f37bf502a8d5e8214. Its whole tree matches the independently computed integration tree; its stable patch ID matches the previously reviewed change, and it does not modify workflows.

142 focused metadata heap/decode-budget/member tests pass; compiled public workflows over a real Microsoft.Extensions.Primitives 8 DLL pass through 5 CLI and 4 MCP calls (comparison, graph, inference-only reconstruction and connection reuse). Source bytes stay unchanged and owned-process cleanup verifies.

Historical baseline: the former firmware and three async BinarySessionStatus failures were independently reproduced earlier; both are now repaired upstream. The current-head baseline is recorded below.

Measurement follow-up for the cache-hit review: the missing evidence was a before/after workload comparison, not a changed output contract. I inspected the owner and consumers: cached decoded strings belong to one immutable metadata layout; physical-row/unique-string reservations and per-occurrence admitManagedProjection accounting are retained, including repeated values at JSON serialization. The 142 focused tests retain distinct/transient/projection-capacity rejection coverage. No budget increase or downstream repair was added.

Executed the same completed real Microsoft.Extensions.Primitives 8 task twice on clean main 150e996a and twice on PR 2eecab0: 5 CLI calls, 4 MCP calls plus initialization/catalog, member comparison, graph, inference-only reconstruction and same-connection reuse. All four runs pass with identical semantic summaries and artifact SHA; source bytes remain unchanged and owned-process cleanup verifies. Each run uses fresh CLI/MCP processes and private data directories. OS/dependency caches were not cleared; these are process-cold runs, not controlled machine-cold measurements. Repeated comparison within each MCP connection exercises a warm reuse step but its costs are included in the complete task.

Revision	Repeat	Wall s	Reaped CPU s	Sampled peak family GiB
main	1	14.60	18.73	0.816
fix	1	14.24	18.08	0.797
main	2	14.66	18.77	0.805
fix	2	14.54	18.37	0.809

Whole MCP protocol traffic (including initialize/catalog, inputs and replies) is 13 messages / approximately 5.23 MB JSON per task; the five CLI replies total approximately 0.79 MB. Those byte sizes are not token counts. No LLM agent task/model-token accounting was run and model/context tokens remain unmeasured; no token reduction or speedup is claimed. CPU excludes unreaped descendants; memory is 200 ms sampled job-family RSS, excluding the assistant/shared services, on a shared host with two-core affinity and a 3 GiB Node heap. The tiny between-run differences do not establish a performance improvement. This real DLL does not replace the separate synthetic repeated-string/capacity regressions, and no all-assemblies bound is inferred from its measured RSS. Raw artifacts are retained in /tmp/rea-ci-retry-20261017/3491/. No related code cleanup remains for this evidence-only finding.

Baseline follow-up (2026-10-17)

Current head 44b03c9072bd36245c6cf566dcda0f7997919ec9 incorporates main 5b243993872c2bcc70cb00060ffb79b347321370, including the normally merged #4691/#4703 fixture repairs. Exact merge-tree verification and stable patch IDs confirm that this PR's patch is unchanged by the update. Current-head CI: All five built-dependent shards, Source/static checks and required gates passed.

Fresh exact-head production build and the real Microsoft.Extensions.Primitives 8 DLL workflow pass: 5 CLI and 4 MCP calls, matching artifact identity, comparison/graph/inference-only reconstruction, connection reuse, unchanged input bytes and verified owned cleanup. Completed task: 15.00 s wall, 18.91 s reaped CPU, 0.80 GiB sampled process-family RSS. The earlier pinned before/after measurements above remain the performance comparison; this rerun makes no additional speedup claim.

Local heavy jobs ran serially on two CPU cores with a 3 GiB Node heap. RSS is sampled process-family memory, may double-count shared pages and excludes assistant/shared services; CPU covers reaped descendants. OS/dependency caches were not controlled. Model token use, controlled cold/warm costs and remote resource costs remain unmeasured; no token or speedup claim is inferred. Prior observations above remain tied to their stated revisions. Initial human-owner readiness remains valid and was not supplied on the author's behalf. Available automated reviews have no unresolved findings; Codex explicitly reports repository review-credit exhaustion, so no fresh Codex review is claimed.

Author readiness

Human owner: @keeponlight

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/3491 **Author:** [@keeponlight](https://github.com/keeponlight) **Created:** 10/14/2026 **Status:** ✅ Merged **Merged:** 10/17/2026 **Merged by:** [@N0zoM1z0](https://github.com/N0zoM1z0) **Base:** `main` ← **Head:** `fix/managed-representation-budget-3449` --- ### 📝 Commits (6) - [`ca1fa79`](https://github.com/morluto/rea/commit/ca1fa79a7dc22fcf9538f91cc5567a2ee3b278d6) fix(managed): avoid duplicate decode budget charging on cached metadata strings - [`f1e2fba`](https://github.com/morluto/rea/commit/f1e2fba980b34b9128f02db3c5c53e19f880e71a) Merge branch 'main' into fix/managed-representation-budget-3449 - [`2eecab0`](https://github.com/morluto/rea/commit/2eecab0e698c80e4fad98fe13223d4971b6f140e) Merge branch 'main' into fix/managed-representation-budget-3449 - [`9ebeed5`](https://github.com/morluto/rea/commit/9ebeed5888d1d9b445678a5656c7fddb2f1823c0) Merge branch 'main' into fix/managed-representation-budget-3449 - [`89aac64`](https://github.com/morluto/rea/commit/89aac64ef9f8eebde96e91c698ae58a4b2d65413) Merge branch 'main' into fix/managed-representation-budget-3449 - [`44b03c9`](https://github.com/morluto/rea/commit/44b03c9072bd36245c6cf566dcda0f7997919ec9) Merge branch 'main' into fix/managed-representation-budget-3449 ### 📊 Changes **2 files changed** (+33 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/dotnet/ManagedMetadataHeaps.test.ts` (+33 -0) 📝 `src/dotnet/ManagedMetadataHeaps.ts` (+0 -1) </details> ### 📄 Description ## Summary Prevents `readMetadataString` from repeatedly deducting `encodedBytes * 6 + 2` from `ManagedDecodeBudget` on `#Strings` heap cache hits (`cached !== undefined`). Fixes #3449 ## Problem and expected behavior Running `rea inspect-managed-members` against ordinary .NET assemblies (such as the 2.2 MB `HP.Omen.Core.Common.dll` in #3449) failed with `resource-limit: Managed metadata exceeds the 67108864 byte representation budget` at a non-null `#Strings` file offset (`"offset": 938934`). While #3487 (`bddeae5`) resolved duplicate `admitManagedProjection` charges (`offset: null`) via `#prepaid`, `readMetadataString` in `src/dotnet/ManagedMetadataHeaps.ts:126-130` still called `managedDecodeBudget(layout).reserveString(cached.encodedBytes, start)` on every cache hit (`cached !== undefined`). During `inspectManagedMembersBytes`, repeated lookups of the same `#Strings` entries across `readManagedMetadataInventory`, `typeRanges`, `parseTypes`, `typeNameLookup`, `CustomAttribute`, and `MemberRef` permanently charged `encodedBytes * 6 + 2` on every lookup before `admitManagedProjection` charged the retained JSON output. ## Change and scope - Removed the duplicate `reserveString` call on `#Strings` cache hits in `readMetadataString` (`src/dotnet/ManagedMetadataHeaps.ts`); the heap decode reservation is charged once when the string is first decoded and cached at line 144, and any retained JSON occurrences are charged by `admitManagedProjection`. - Added unit regression test `does not re-charge the decode budget on metadata string cache hits` in `src/dotnet/ManagedMetadataHeaps.test.ts`. - Rebased onto `main` (`89faa863e`), dropping the `ManagedDecodeBudget.ts` projection changes that were already landed by #3487. Contribution scope: not applicable (ordinary fix within existing boundaries). ## Contract and boundary impact - Semantic owner and earliest changed stage: managed metadata string heap reader (`src/dotnet/ManagedMetadataHeaps.ts`) - Contribution scope fit and linked maintainer decision: not applicable - CLI and MCP/tool-catalog contract: none - Provider, bridge, target-format, or platform compatibility: none; the 64 MiB per-inspection representation limit remains enforced for unique `#Strings` heap decodes and retained JSON projections - Evidence, artifact, provenance, or reconstruction contract: none - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable; static file-backed inspection only - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none Expected effects: - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: `src/dotnet/ManagedMetadataHeaps.test.ts` (`does not re-charge the decode budget on metadata string cache hits`). - Base reproduction or other evidence: executed. The new test fails with `ManagedReaderFailure: Managed metadata exceeds the 67108864 byte representation budget` on `89faa863e` and passes with this fix. - User-visible CLI/MCP output (if applicable): assemblies that repeatedly reference cached `#Strings` heap entries across metadata tables no longer fail prematurely at a `#Strings` file offset with `resource-limit` on `metadata.representation`. - Remaining proof gaps: proprietary HP OMEN binaries from #3449 were not attached to the issue; verified via synthetic 10 MiB `#Strings` cache-hit fixture and the managed test suite. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npx vitest run src/dotnet/ManagedMetadataHeaps.test.ts src/dotnet/ManagedDecodeBudget.test.ts src/dotnet/ManagedMemberInspector.test.ts` — 3 files, 142 passed (1 new) - `npm run check:fast` — typecheck and lint passed with 0 errors - `node scripts/scan-todos.mjs` — `No tech-debt markers found.` ## Compatibility, safety, and release - Breaking changes or migration steps: none - Real Hopper/Ghidra, browser, or OS coverage: macOS Darwin arm64 (Node v24.14.1) - Package or release metadata impact: none - Security, privacy, process, or containment review: preserves the 64 MiB representation cap against uncached heap allocations and repeated string expansions in JSON projections. ### Maintainer review update — Linux (17 October 2026) Reviewed head: `2eecab0e698c80e4fad98fe13223d4971b6f140e`, merged with main `150e996a86f2bec4982ba87f37bf502a8d5e8214`. Its whole tree matches the independently computed integration tree; its stable patch ID matches the previously reviewed change, and it does not modify workflows. 142 focused metadata heap/decode-budget/member tests pass; compiled public workflows over a real Microsoft.Extensions.Primitives 8 DLL pass through 5 CLI and 4 MCP calls (comparison, graph, inference-only reconstruction and connection reuse). Source bytes stay unchanged and owned-process cleanup verifies. Historical baseline: the former firmware and three async BinarySessionStatus failures were independently reproduced earlier; both are now repaired upstream. The current-head baseline is recorded below. Measurement follow-up for the cache-hit review: the missing evidence was a before/after workload comparison, not a changed output contract. I inspected the owner and consumers: cached decoded strings belong to one immutable metadata layout; physical-row/unique-string reservations and per-occurrence `admitManagedProjection` accounting are retained, including repeated values at JSON serialization. The 142 focused tests retain distinct/transient/projection-capacity rejection coverage. No budget increase or downstream repair was added. Executed the same completed real Microsoft.Extensions.Primitives 8 task twice on clean main `150e996a` and twice on PR `2eecab0`: 5 CLI calls, 4 MCP calls plus initialization/catalog, member comparison, graph, inference-only reconstruction and same-connection reuse. All four runs pass with identical semantic summaries and artifact SHA; source bytes remain unchanged and owned-process cleanup verifies. Each run uses fresh CLI/MCP processes and private data directories. OS/dependency caches were not cleared; these are process-cold runs, not controlled machine-cold measurements. Repeated comparison within each MCP connection exercises a warm reuse step but its costs are included in the complete task. | Revision | Repeat | Wall s | Reaped CPU s | Sampled peak family GiB | |---|---:|---:|---:|---:| | main | 1 | 14.60 | 18.73 | 0.816 | | fix | 1 | 14.24 | 18.08 | 0.797 | | main | 2 | 14.66 | 18.77 | 0.805 | | fix | 2 | 14.54 | 18.37 | 0.809 | Whole MCP protocol traffic (including initialize/catalog, inputs and replies) is 13 messages / approximately 5.23 MB JSON per task; the five CLI replies total approximately 0.79 MB. Those byte sizes are not token counts. No LLM agent task/model-token accounting was run and model/context tokens remain unmeasured; no token reduction or speedup is claimed. CPU excludes unreaped descendants; memory is 200 ms sampled job-family RSS, excluding the assistant/shared services, on a shared host with two-core affinity and a 3 GiB Node heap. The tiny between-run differences do not establish a performance improvement. This real DLL does not replace the separate synthetic repeated-string/capacity regressions, and no all-assemblies bound is inferred from its measured RSS. Raw artifacts are retained in `/tmp/rea-ci-retry-20261017/3491/`. No related code cleanup remains for this evidence-only finding. ## Baseline follow-up (2026-10-17) Current head `44b03c9072bd36245c6cf566dcda0f7997919ec9` incorporates main `5b243993872c2bcc70cb00060ffb79b347321370`, including the normally merged #4691/#4703 fixture repairs. Exact merge-tree verification and stable patch IDs confirm that this PR's patch is unchanged by the update. [Current-head CI](https://github.com/morluto/rea/actions/runs/39011222965): All five built-dependent shards, Source/static checks and required gates passed. Fresh exact-head production build and the real Microsoft.Extensions.Primitives 8 DLL workflow pass: 5 CLI and 4 MCP calls, matching artifact identity, comparison/graph/inference-only reconstruction, connection reuse, unchanged input bytes and verified owned cleanup. Completed task: 15.00 s wall, 18.91 s reaped CPU, 0.80 GiB sampled process-family RSS. The earlier pinned before/after measurements above remain the performance comparison; this rerun makes no additional speedup claim. Local heavy jobs ran serially on two CPU cores with a 3 GiB Node heap. RSS is sampled process-family memory, may double-count shared pages and excludes assistant/shared services; CPU covers reaped descendants. OS/dependency caches were not controlled. Model token use, controlled cold/warm costs and remote resource costs remain unmeasured; no token or speedup claim is inferred. Prior observations above remain tied to their stated revisions. Initial human-owner readiness remains valid and was not supplied on the author's behalf. Available automated reviews have no unresolved findings; Codex explicitly reports repository review-credit exhaustion, so no fresh Codex review is claimed. ## Author readiness Human owner: @keeponlight - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:09:22 +00:00