morluto/rea

Fork 0

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

[GH-ISSUE #4723] [Bug] Native plist reads drop failed descriptor cleanup ownership #2464

New issue

Closed

opened 2026-10-17 21:05:09 +00:00 by bsakaguchi · 4 comments

bsakaguchi commented

2026-10-17 21:05:09 +00:00

Owner

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

Before submitting

Searched open/closed issues and open plist PRs. Other plist decoding/format issues and #4658 (Electron fuse file ownership) do not cover this native plist read boundary.
Only authored inert data and controlled errors are included.

Area

Evidence, artifacts, or reconstruction

Observed behavior

A production NativeMacOSProvider client returns an ordinary ProviderAdapterError with cleanupIncomplete: false after its source-format prefix read and descriptor close both reject. A controlled boundary test opens a real tiny local plist descriptor, injects read and close failures, and expects retained cleanup; it fails on that flag. Native commands are fixture captures, not actual file/plutil executions. The test explicitly closes its captured handle during teardown.

Reproduction

At source 70de70210, create a tiny XML plist and instantiate NativeMacOSProvider with NativeFixtureRunner captures (file: ASCII text, plutil: "value"). Inject a FileHandle.read failure on the prefix probe and replace that exact handle's close method with a controlled rejection. Call client.execute('inspect_plist', {path}). One boundary run returned ProviderAdapterError with cleanupIncomplete false rather than a retained cleanup failure. Test: tests/boundary/providers/native/nativePlistFileCleanup.test.ts in the local audit diff.

Expected behavior

Preserve the physical read failure and cleanup failure independently, retain the exact descriptor owner, retry it on client cleanup, and update the original error when cleanup succeeds. The same rule applies to plist admission's separate open/close. Follow AGENTS.md ownership/completion rules and the existing OwnedRegularFile / ArtifactResourceScope primitives.

Impact and scope

Controlled real-descriptor fault on macOS arm64 / Node24.18.0. Source-derived: plistSourceFormat uses raw open/read/finally-close; a failing finally-close replaces the read error. requireRegularPlist has a second unsupervised descriptor close. The client retains signature resources but no plist descriptor. Ordinary OS close-failure incidence and live/public CLI failure reproduction: NOT_ENOUGH_INFO. No target execution or file mutation claim.

REA version / target

Source 70de702109fd492306d35d70a920616256804885; authored inert XML plist, synthetic Mach-O target metadata for the provider seam.

Interface / earliest failure stage

Other: production provider interface with controlled command captures / Cleanup, cancellation, or lifecycle.

Relevant output or evidence

Executed focused boundary regression: 1 failed. Actual ProviderAdapterError { cleanupIncomplete: false, operation: 'inspect_plist' }; expected retained cleanup. Source: src/native/NativeMacOSProvider.ts, plistSourceFormat and requireRegularPlist, introduced/affected by #4320. Baseline log retained locally; no real native utility or Windows qualification.

Proposed checks

Reuse shared file ownership through admission, prefix read, cancellation and final close. Cover mixed read/close outcomes, close retry, concurrent client close and opaque cancellation reasons. Preserve existing source-format and caller-selection behavior. Source freshness across separate native commands is a distinct audit question, not established by this fault control.

Originally created by @morluto on GitHub (Oct 17, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/4723 ### Before submitting - [x] Searched open/closed issues and open plist PRs. Other plist decoding/format issues and #4658 (Electron fuse file ownership) do not cover this native plist read boundary. - [x] Only authored inert data and controlled errors are included. ### Area Evidence, artifacts, or reconstruction ### Observed behavior A production NativeMacOSProvider client returns an ordinary ProviderAdapterError with `cleanupIncomplete: false` after its source-format prefix read and descriptor close both reject. A controlled boundary test opens a real tiny local plist descriptor, injects read and close failures, and expects retained cleanup; it fails on that flag. Native commands are fixture captures, not actual file/plutil executions. The test explicitly closes its captured handle during teardown. ### Reproduction At source `70de70210`, create a tiny XML plist and instantiate NativeMacOSProvider with NativeFixtureRunner captures (`file: ASCII text`, `plutil: "value"`). Inject a FileHandle.read failure on the prefix probe and replace that exact handle's close method with a controlled rejection. Call `client.execute('inspect_plist', {path})`. One boundary run returned ProviderAdapterError with cleanupIncomplete false rather than a retained cleanup failure. Test: `tests/boundary/providers/native/nativePlistFileCleanup.test.ts` in the local audit diff. ### Expected behavior Preserve the physical read failure and cleanup failure independently, retain the exact descriptor owner, retry it on client cleanup, and update the original error when cleanup succeeds. The same rule applies to plist admission's separate open/close. Follow AGENTS.md ownership/completion rules and the existing OwnedRegularFile / ArtifactResourceScope primitives. ### Impact and scope Controlled real-descriptor fault on macOS arm64 / Node24.18.0. Source-derived: `plistSourceFormat` uses raw open/read/finally-close; a failing finally-close replaces the read error. `requireRegularPlist` has a second unsupervised descriptor close. The client retains signature resources but no plist descriptor. Ordinary OS close-failure incidence and live/public CLI failure reproduction: NOT_ENOUGH_INFO. No target execution or file mutation claim. ### REA version / target Source `70de702109fd492306d35d70a920616256804885`; authored inert XML plist, synthetic Mach-O target metadata for the provider seam. ### Interface / earliest failure stage Other: production provider interface with controlled command captures / Cleanup, cancellation, or lifecycle. ### Relevant output or evidence Executed focused boundary regression: 1 failed. Actual `ProviderAdapterError { cleanupIncomplete: false, operation: 'inspect_plist' }`; expected retained cleanup. Source: `src/native/NativeMacOSProvider.ts`, `plistSourceFormat` and `requireRegularPlist`, introduced/affected by #4320. Baseline log retained locally; no real native utility or Windows qualification. ### Proposed checks Reuse shared file ownership through admission, prefix read, cancellation and final close. Cover mixed read/close outcomes, close retry, concurrent client close and opaque cancellation reasons. Preserve existing source-format and caller-selection behavior. Source freshness across separate native commands is a distinct audit question, not established by this fault control.

bsakaguchi added the

triage

bug

labels

2026-10-17 21:05:09 +00:00

bsakaguchi commented

2026-10-17 21:05:09 +00:00

Author

Owner

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

The gap is clear: when both the prefix read and the descriptor close reject, the close failure is swallowed and the error reports cleanupIncomplete: false, so callers cannot tell the descriptor was never safely released. The boundary-test expectation (retained cleanup) looks right — a close failure on a handle we already own should surface as retained cleanup even when the primary operation failed, otherwise native-provider resource leaks become invisible. It might also be worth asserting the close error is carried alongside the read error in the returned ProviderAdapterError, so the contract is explicit rather than just a boolean flag flip.

@XinwangTao commented on GitHub (Oct 17, 2026): The gap is clear: when both the prefix read and the descriptor close reject, the close failure is swallowed and the error reports `cleanupIncomplete: false`, so callers cannot tell the descriptor was never safely released. The boundary-test expectation (retained cleanup) looks right — a close failure on a handle we already own should surface as retained cleanup even when the primary operation failed, otherwise native-provider resource leaks become invisible. It might also be worth asserting the close error is carried alongside the read error in the returned ProviderAdapterError, so the contract is explicit rather than just a boolean flag flip.

bsakaguchi commented

2026-10-17 21:05:10 +00:00

Author

Owner

@morluto commented on GitHub (Oct 17, 2026):

Published in e09a503f44 on main, with shared dependency completion correction 26bb85c007 (#4578).

Root cause: raw admission and prefix descriptors had no retained cleanup owner; failing finally-close could replace the read failure. Both now use shared owned regular-file operations and a dedicated plist resource scope. Client close joins admitted work, retries exact retained leases, and updates earlier errors. Physical outcomes, opaque abort reasons and completed converter output survive cleanup. Command-thrown error objects confer no descriptor authority. Removed the superseded raw file implementations.

Validation: baseline controlled prefix fault failed cleanup retention; final post-rebase focused run passed 156 tests across eight files, including 11 cleanup controls, 14 source-format controls, 17 public native CLI checks, signature ownership and shared error/plist consumers. CLI checks used actual macOS file/plutil on generated inert files; injected cleanup faults use real descriptors with controlled read/close outcomes and simulated command captures. Static checks, docs checks (5 tests), commit and push hooks passed. No real OS close-failure, LLDB, Windows or target-execution qualification.

Small completed provider workflow comparison: six byte-identical complete replies, 228 raw cl100k_base tokens each. First-process baseline/candidate: elapsed 0.348/0.380 s, CPU 0.501/0.511 s, sampled process-family RSS 285,278,208/312,934,400 bytes. Two warm-repeat medians: elapsed 0.325/0.311 s, CPU 0.475/0.474 s, RSS 285,409,280/264,724,480 bytes. Real tiny file admission/probe/close; native commands simulated. These noisy tiny samples establish no performance improvement. Whole-agent, public CLI/MCP, real utility, large-file and injected-failure costs unmeasured. Heavy profiling stayed stopped.

Remaining separate question: freshness across the independent path-based native commands is not established by this change. Existing file ownership retains unknown closure states rather than assuming every OS failure is retryable.

@morluto commented on GitHub (Oct 17, 2026): Published in e09a503f44fba1639b71907a018d71161360f5ae on main, with shared dependency completion correction 26bb85c007109dd4eed11a5b9fcbd39f494f0b6e (#4578). Root cause: raw admission and prefix descriptors had no retained cleanup owner; failing finally-close could replace the read failure. Both now use shared owned regular-file operations and a dedicated plist resource scope. Client close joins admitted work, retries exact retained leases, and updates earlier errors. Physical outcomes, opaque abort reasons and completed converter output survive cleanup. Command-thrown error objects confer no descriptor authority. Removed the superseded raw file implementations. Validation: baseline controlled prefix fault failed cleanup retention; final post-rebase focused run passed 156 tests across eight files, including 11 cleanup controls, 14 source-format controls, 17 public native CLI checks, signature ownership and shared error/plist consumers. CLI checks used actual macOS file/plutil on generated inert files; injected cleanup faults use real descriptors with controlled read/close outcomes and simulated command captures. Static checks, docs checks (5 tests), commit and push hooks passed. No real OS close-failure, LLDB, Windows or target-execution qualification. Small completed provider workflow comparison: six byte-identical complete replies, 228 raw cl100k_base tokens each. First-process baseline/candidate: elapsed 0.348/0.380 s, CPU 0.501/0.511 s, sampled process-family RSS 285,278,208/312,934,400 bytes. Two warm-repeat medians: elapsed 0.325/0.311 s, CPU 0.475/0.474 s, RSS 285,409,280/264,724,480 bytes. Real tiny file admission/probe/close; native commands simulated. These noisy tiny samples establish no performance improvement. Whole-agent, public CLI/MCP, real utility, large-file and injected-failure costs unmeasured. Heavy profiling stayed stopped. Remaining separate question: freshness across the independent path-based native commands is not established by this change. Existing file ownership retains unknown closure states rather than assuming every OS failure is retryable.

bsakaguchi commented

2026-10-17 21:05:11 +00:00

Author

Owner

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

Thanks for the thorough fix — moving raw admission and prefix descriptors onto shared owned file operations with a dedicated plist resource scope directly addresses the swallowed close-failure I flagged: callers now get a retained-cleanup signal instead of cleanupIncomplete:false. Good to see client close joining admitted work and retrying exact retained leases, so the error contract is explicit rather than just a boolean flip. The controlled prefix fault failing before and passing after the fix confirms the regression is actually covered.

@XinwangTao commented on GitHub (Oct 17, 2026): Thanks for the thorough fix — moving raw admission and prefix descriptors onto shared owned file operations with a dedicated plist resource scope directly addresses the swallowed close-failure I flagged: callers now get a retained-cleanup signal instead of cleanupIncomplete:false. Good to see client close joining admitted work and retrying exact retained leases, so the error contract is explicit rather than just a boolean flip. The controlled prefix fault failing before and passing after the fix confirms the regression is actually covered.

bsakaguchi closed this issue

2026-10-31 00:34:16 +00:00

bsakaguchi commented

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

Author

Owner

@morluto commented on GitHub (Oct 18, 2026):

Reconciled against current main: this defect is already fixed by e09a503f4 (fix(native): retain plist descriptor cleanup ownership), with selection-intent follow-up e6c487799. NativePlistSource now routes admission and prefix reads through the shared owned-file boundary and retains cleanup at the client resource scope; the existing tests retain the exact failed handle, preserve primary/cancellation facts, block replacement admission and retry cleanup before releasing the owner.

Executed current nativePlistFileCleanup.test.ts and nativePlistSelection.test.ts: 18 passed; the final affected selection including these cases passed 162 tests in 13 files, with one unrelated pre-existing 8 MiB LLDB allocation case skipped. Full static gate passed. Read the current issue narrative and all three comments; the latest comment also acknowledges the owner-level fix. These are controlled native-command/file faults, not real OS close-failure incidence or Windows/provider qualification. No additional implementation needed for the reported defect.

@morluto commented on GitHub (Oct 18, 2026): Reconciled against current main: this defect is already fixed by e09a503f4 (`fix(native): retain plist descriptor cleanup ownership`), with selection-intent follow-up e6c487799. `NativePlistSource` now routes admission and prefix reads through the shared owned-file boundary and retains cleanup at the client resource scope; the existing tests retain the exact failed handle, preserve primary/cancellation facts, block replacement admission and retry cleanup before releasing the owner. Executed current `nativePlistFileCleanup.test.ts` and `nativePlistSelection.test.ts`: 18 passed; the final affected selection including these cases passed 162 tests in 13 files, with one unrelated pre-existing 8 MiB LLDB allocation case skipped. Full static gate passed. Read the current issue narrative and all three comments; the latest comment also acknowledges the owner-level fix. These are controlled native-command/file faults, not real OS close-failure incidence or Windows/provider qualification. No additional implementation needed for the reported defect.

bsakaguchi referenced this issue

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

[PR #2464] [MERGED] feat(wasm): compare retained core-module interfaces #5247

bsakaguchi referenced this issue

2026-10-31 01:05:42 +00:00

[PR #2694] fix(wasm): raise the WABT output budget to 128 MiB #5318