morluto/rea

Fork 0

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

[GH-ISSUE #4475] [Bug] Firmware read-handle close failures discard outcomes and lose cleanup ownership #1771

New issue

Closed

opened 2026-10-16 21:10:06 +00:00 by bsakaguchi · 6 comments

bsakaguchi commented

2026-10-16 21:10:06 +00:00

Owner

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

Before submitting

I searched related open and closed reports (#3887, #3923, #1501, #3935, #4240).
This report contains no credentials or sensitive target data.

Area

Evidence, artifacts, or reconstruction

Observed behavior

Source-only: firmware hash, snapshot and report readers own raw FileHandles only in local variables. Each closes in finally; a failed close replaces a successful read or its original error and loses the exact descriptor owner. The provider can retry process/workspace/tree cleanup, but retains no failed read-handle owner. Removing a workspace does not prove its open descriptors closed.

Reproduction

NOT_ENOUGH_INFO: no injected-close runtime reproduction was performed for this report. Source inspection at main fe2a398dd9; proposed check: inject one real descriptor close failure after successful hash/report parsing and after a primary parse/read failure, then retry provider.close(). Assert exact-handle retry and preservation of the original outcome.

Expected behavior

AGENTS.md requires retaining resources whose cleanup failed and preserving producing-boundary ownership. Reuse the canonical owned regular-file lifecycle; transfer the exact owner into provider cleanup and preserve primary errors or completed observations through close/retry.

Impact and scope

Source-derived: persistent or transient close failure can leak a readable descriptor and erase the meaningful read outcome. Natural failure frequency and real-provider impact are unknown. This is distinct from the blocking acquisition in #3887 and the final cancellation translation in #3923.

REA version

6.5.0; main fe2a398dd9, source inspection.

Runtime and platform

Source review on macOS arm64; Node 24.18.0. Native Linux firmware producer not run.

Target and artifact context

Firmware snapshot, configured tool hash and producer JSON report.

Interface

CLI and MCP (shared provider; reachability is source-derived).

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

src/firmware/FirmwareFiles.ts: hashFirmwareFile, snapshotFirmware and readFirmwareReport each acquire a raw handle and close in finally.
src/firmware/FirmwareProvider.ts: PendingFirmwareCleanup retains supervisor/tree/root only; #retryCleanup cannot retry a failed read descriptor.
src/firmware/FirmwareCommand.ts: command hash/report wrappers reclassify the thrown close error without transferring an owner.
src/firmware/FirmwarePublication.ts: hashFirmwareFile consumer shares the same read-handle gap.

Evidence provenance: source-derived. Local runtime verification: not performed. Exact descriptor-close regression and full native Linux producer behavior remain unverified.

Typed outcome or recovery expected

A typed failure retains the primary outcome and exact cleanup capability; confirmed retry clears cleanup uncertainty, failed retry keeps that same owner.

Analysis or suspected cause

The helper's local finally block is the only descriptor owner. Existing workspace and process owners cannot substitute for descriptor ownership. A narrow nonblocking-open repair for #3887 does not itself resolve this lifecycle gap.

Additional context

AI-assisted source audit; no runtime reproduction or performance claim.

Originally created by @morluto on GitHub (Oct 16, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/4475 ### Before submitting - [x] I searched related open and closed reports (#3887, #3923, #1501, #3935, #4240). - [x] This report contains no credentials or sensitive target data. ### Area Evidence, artifacts, or reconstruction ### Observed behavior Source-only: firmware hash, snapshot and report readers own raw FileHandles only in local variables. Each closes in `finally`; a failed close replaces a successful read or its original error and loses the exact descriptor owner. The provider can retry process/workspace/tree cleanup, but retains no failed read-handle owner. Removing a workspace does not prove its open descriptors closed. ### Reproduction NOT_ENOUGH_INFO: no injected-close runtime reproduction was performed for this report. Source inspection at main fe2a398dd9ab290ba6cb3411403d319cec8d07a2; proposed check: inject one real descriptor close failure after successful hash/report parsing and after a primary parse/read failure, then retry provider.close(). Assert exact-handle retry and preservation of the original outcome. ### Expected behavior AGENTS.md requires retaining resources whose cleanup failed and preserving producing-boundary ownership. Reuse the canonical owned regular-file lifecycle; transfer the exact owner into provider cleanup and preserve primary errors or completed observations through close/retry. ### Impact and scope Source-derived: persistent or transient close failure can leak a readable descriptor and erase the meaningful read outcome. Natural failure frequency and real-provider impact are unknown. This is distinct from the blocking acquisition in #3887 and the final cancellation translation in #3923. ### REA version 6.5.0; main fe2a398dd9ab290ba6cb3411403d319cec8d07a2, source inspection. ### Runtime and platform Source review on macOS arm64; Node 24.18.0. Native Linux firmware producer not run. ### Target and artifact context Firmware snapshot, configured tool hash and producer JSON report. ### Interface CLI and MCP (shared provider; reachability is source-derived). ### Earliest failure stage Cleanup, cancellation, or lifecycle ### Relevant output or evidence - src/firmware/FirmwareFiles.ts: hashFirmwareFile, snapshotFirmware and readFirmwareReport each acquire a raw handle and close in finally. - src/firmware/FirmwareProvider.ts: PendingFirmwareCleanup retains supervisor/tree/root only; #retryCleanup cannot retry a failed read descriptor. - src/firmware/FirmwareCommand.ts: command hash/report wrappers reclassify the thrown close error without transferring an owner. - src/firmware/FirmwarePublication.ts: hashFirmwareFile consumer shares the same read-handle gap. Evidence provenance: source-derived. Local runtime verification: not performed. Exact descriptor-close regression and full native Linux producer behavior remain unverified. ### Typed outcome or recovery expected A typed failure retains the primary outcome and exact cleanup capability; confirmed retry clears cleanup uncertainty, failed retry keeps that same owner. ### Analysis or suspected cause The helper's local finally block is the only descriptor owner. Existing workspace and process owners cannot substitute for descriptor ownership. A narrow nonblocking-open repair for #3887 does not itself resolve this lifecycle gap. ### Additional context AI-assisted source audit; no runtime reproduction or performance claim.

bsakaguchi added the

bug

label

2026-10-16 21:10:06 +00:00

bsakaguchi commented

2026-10-16 21:10:07 +00:00

Author

Owner

@AayushJB03 commented on GitHub (Oct 16, 2026):

Hi @morluto I want to work on this bug #4475.
Could you assign it to me.
Its new to me but I'd like to understand how this works

@AayushJB03 commented on GitHub (Oct 16, 2026): Hi @morluto I want to work on this bug #4475. Could you assign it to me. Its new to me but I'd like to understand how this works

bsakaguchi commented

2026-10-16 21:10:07 +00:00

Author

Owner

@qorvr commented on GitHub (Oct 16, 2026):

Resolved in #4493 (qorvr:fix/firmware-read-handle-cleanup).

Root Cause

Admitted file handles opened via openRegularFile / withOwnedRegularFile in readFirmwareReport, snapshotFirmware, and hashFirmwareFile did not supply failure retention callbacks or intercept RegularFileCleanupFailure. When descriptor close failed:

Completed reports were discarded and unhandled rejections were thrown instead of preserving partial observations or composing with primary failures.
Ownership of the descriptor was lost, leaking the handle and preventing retries during provider.close().
Descriptor file paths were omitted from cleanupResources on ProviderCleanupError.

Resolution

Added retainFailedClose options and callbacks to hashFirmwareFile, snapshotFirmware, and readFirmwareReport.
Wired readFirmwareCommandReport to catch RegularFileCleanupFailure, generating ProviderCleanupError that attaches completedReport and records descriptor paths in cleanupResources.
Extended PendingFirmwareCleanup and FirmwareProvider with descriptor tracking (descriptors: Map<OwnedFileHandle, string> and #unrootedDescriptors), retrying descriptor cleanup during #retryCleanup() upon close().
Preserved completed observations when descriptors fail close in finishFirmwareWorkspace.
Added unit and boundary regression tests covering descriptor close failure recovery and resource recording.

@qorvr commented on GitHub (Oct 16, 2026): Resolved in #4493 (`qorvr:fix/firmware-read-handle-cleanup`). ### Root Cause Admitted file handles opened via `openRegularFile` / `withOwnedRegularFile` in `readFirmwareReport`, `snapshotFirmware`, and `hashFirmwareFile` did not supply failure retention callbacks or intercept `RegularFileCleanupFailure`. When descriptor close failed: 1. Completed reports were discarded and unhandled rejections were thrown instead of preserving partial observations or composing with primary failures. 2. Ownership of the descriptor was lost, leaking the handle and preventing retries during `provider.close()`. 3. Descriptor file paths were omitted from `cleanupResources` on `ProviderCleanupError`. ### Resolution - Added `retainFailedClose` options and callbacks to `hashFirmwareFile`, `snapshotFirmware`, and `readFirmwareReport`. - Wired `readFirmwareCommandReport` to catch `RegularFileCleanupFailure`, generating `ProviderCleanupError` that attaches `completedReport` and records descriptor paths in `cleanupResources`. - Extended `PendingFirmwareCleanup` and `FirmwareProvider` with descriptor tracking (`descriptors: Map<OwnedFileHandle, string>` and `#unrootedDescriptors`), retrying descriptor cleanup during `#retryCleanup()` upon `close()`. - Preserved completed observations when descriptors fail close in `finishFirmwareWorkspace`. - Added unit and boundary regression tests covering descriptor close failure recovery and resource recording.

bsakaguchi commented

2026-10-17 21:03:31 +00:00

Author

Owner

@N0zoM1z0 commented on GitHub (Oct 16, 2026):

Additional runtime test evidence for this lifecycle follow-up: the two firmware failures reproduce on the unmodified main base 9d940c2f284cc6f75b27900336e52ceeba4aa2af.

A fresh detached worktree, locked npm ci, then npm run test:focused -- tests/boundary/firmware/firmwareIntegration.test.ts gives 33 passed / 2 failed, with a clean worktree:

retains a setup-failed publication tree for provider cleanup retry
retains the complete firmware result when published output cleanup stays uncertain

The first receives ProviderAdapterError with nested primary/cleanup diagnostics; the expected shape has the earlier flat cleanup diagnostics. The second expects ProviderCleanupError but receives an adapter failure. Whether the producing contract or expectations need correction remains undiagnosed.

CI failure and exact assertions. CI checked merge eedae92d406015895a6d0eced5035da1b9ead07a (Electron head 84d9cf5db + that main base). This is boundary-test reproduction using the existing fake extractor fixtures, not a real firmware-provider claim. It isolates the failures outside the Electron diff; #4475 is the related lifecycle/cleanup ownership discussion.

@N0zoM1z0 commented on GitHub (Oct 16, 2026): Additional runtime test evidence for this lifecycle follow-up: the two firmware failures reproduce on the **unmodified main base** `9d940c2f284cc6f75b27900336e52ceeba4aa2af`. A fresh detached worktree, locked `npm ci`, then `npm run test:focused -- tests/boundary/firmware/firmwareIntegration.test.ts` gives **33 passed / 2 failed**, with a clean worktree: - `retains a setup-failed publication tree for provider cleanup retry` - `retains the complete firmware result when published output cleanup stays uncertain` The first receives `ProviderAdapterError` with nested primary/cleanup diagnostics; the expected shape has the earlier flat cleanup diagnostics. The second expects `ProviderCleanupError` but receives an adapter failure. Whether the producing contract or expectations need correction remains undiagnosed. [CI failure and exact assertions](https://github.com/morluto/rea/actions/runs/38935598884/job/117090615652). CI checked merge `eedae92d406015895a6d0eced5035da1b9ead07a` (Electron head `84d9cf5db` + that main base). This is boundary-test reproduction using the existing fake extractor fixtures, not a real firmware-provider claim. It isolates the failures outside the Electron diff; #4475 is the related lifecycle/cleanup ownership discussion.

bsakaguchi commented

2026-10-17 21:03:31 +00:00

Author

Owner

@qorvr commented on GitHub (Oct 16, 2026):

Thanks @N0zoM1z0 for isolating and detailing the reproduction on main base 9d940c2f.

Both failures have been diagnosed and resolved in #4493:

1. `retains a setup-failed publication tree for provider cleanup retry`

Root Cause: Test expectation staleness.
Diagnosis: Under REA's error model (analysisErrorWithCleanupFailure), when both a primary analysis setup error and a cleanup failure occur, the resulting ProviderAdapterError carries the cleanup failure inside details.cleanup_error. The test expectation on main was asserting the older flat structure (details.diagnostics.previous_error).
Correction: The expectation in tests/boundary/firmware/firmwareIntegration.test.ts was updated to assert the nested cleanup_error.details.diagnostics.previous_error, matching the standardized composite error contract.

2. `retains the complete firmware result when published output cleanup stays uncertain`

Root Cause: Producing contract bug in src/firmware/FirmwarePublication.ts.
Diagnosis: When extraction successfully completes and publishes its result (publishedResult !== undefined), subsequent descriptor closure failures occurred with no primary extraction error. FirmwarePublication.ts was mistakenly wrapping this in an adapter failure (analysisErrorWithCleanupFailure) instead of surfacing the pure cleanup failure.
Correction: In FirmwarePublication.ts, when publishedResult !== undefined and descriptor closure fails without a primary extraction error, the method now directly re-throws cleanup (ProviderCleanupError) while preserving publishedResult on the error.

Linux Fixture Portability Note

Additionally, tests/fixtures/firmware/provider.ts had a path assertion endsWith("/true") which failed on Debian/Ubuntu environments where realpath("/usr/bin/true") resolves to /usr/bin/gnutrue. This was updated to .endsWith("true").

All 35 tests in tests/boundary/firmware/firmwareIntegration.test.ts pass cleanly in #4493.

@qorvr commented on GitHub (Oct 16, 2026): Thanks @N0zoM1z0 for isolating and detailing the reproduction on main base `9d940c2f`. Both failures have been diagnosed and resolved in #4493: ### 1. `retains a setup-failed publication tree for provider cleanup retry` - **Root Cause:** Test expectation staleness. - **Diagnosis:** Under REA's error model (`analysisErrorWithCleanupFailure`), when both a primary analysis setup error and a cleanup failure occur, the resulting `ProviderAdapterError` carries the cleanup failure inside `details.cleanup_error`. The test expectation on `main` was asserting the older flat structure (`details.diagnostics.previous_error`). - **Correction:** The expectation in `tests/boundary/firmware/firmwareIntegration.test.ts` was updated to assert the nested `cleanup_error.details.diagnostics.previous_error`, matching the standardized composite error contract. ### 2. `retains the complete firmware result when published output cleanup stays uncertain` - **Root Cause:** Producing contract bug in `src/firmware/FirmwarePublication.ts`. - **Diagnosis:** When extraction successfully completes and publishes its result (`publishedResult !== undefined`), subsequent descriptor closure failures occurred with no primary extraction error. `FirmwarePublication.ts` was mistakenly wrapping this in an adapter failure (`analysisErrorWithCleanupFailure`) instead of surfacing the pure cleanup failure. - **Correction:** In `FirmwarePublication.ts`, when `publishedResult !== undefined` and descriptor closure fails without a primary extraction error, the method now directly re-throws `cleanup` (`ProviderCleanupError`) while preserving `publishedResult` on the error. ### Linux Fixture Portability Note Additionally, `tests/fixtures/firmware/provider.ts` had a path assertion `endsWith("/true")` which failed on Debian/Ubuntu environments where `realpath("/usr/bin/true")` resolves to `/usr/bin/gnutrue`. This was updated to `.endsWith("true")`. All 35 tests in `tests/boundary/firmware/firmwareIntegration.test.ts` pass cleanly in #4493.

bsakaguchi commented

2026-10-17 21:03:31 +00:00

Author

Owner

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

Additional related ownership repair in 0a51b246c: failure/cancellation of a firmware stream no longer bypasses its descriptor owner, including failed-close retry.

Firmware now uses the existing owner-preserving readArtifactFileChunks for hashing, snapshot/range copying and publication. Destroying a pipeline no longer closes the source descriptor outside its owner. The canonical source also owns cancellation: competing pipeline signals and obsolete stream-error reinterpretation were removed after snapshot/range controls reproduced EIO being replaced by cancellation (2 failed, 29 passed in that intermediate candidate).

Final validation on Node 24.18.0: 46 focused tests across four files passed, including actual native file growth from 128 MiB to one byte above the unchanged allowance, successful and denied close for both operations, exact-owner retry, a stable file exactly at the allowance, same-read EIO/cancellation, and existing finalization/mixed-owner/publication controls. Docs checks and five conformance tests passed. Native file IO and controlled faults on Darwin; no Linux firmware engines or public CLI growth reproduction claimed. Two separately selected Linux workspace checks were skipped on this host.

Bounded completed snapshot comparison against immediate parent 8451d2839: 32 MiB sparse source, identical size/digest, two alternating warm repeats/build plus separate first processes. Warm medians baseline/candidate: elapsed 0.07248/0.06984 s, CPU 0.06910/0.06844 s, sampled process-family RSS 64790528/57565184 bytes, result JSON tokens 51/51 (cl100k_base). No speed/memory improvement is established by these repeats; true cold, CLI/MCP/whole-agent/schema/request/retry and native-engine costs are unmeasured. Heavy Qodo tests remained stopped.

Related remaining shared error-model defect: #4628. After a composite cleanup dependency resolves, its old wrapper still projects execution_failure while the typed primary survives in diagnostics. The firmware patch preserves the corrected primary and exact cleanup ownership; it does not claim to repair that shared post-recovery projection.

@morluto commented on GitHub (Oct 17, 2026): Additional related ownership repair in `0a51b246c`: failure/cancellation of a firmware stream no longer bypasses its descriptor owner, including failed-close retry. Firmware now uses the existing owner-preserving `readArtifactFileChunks` for hashing, snapshot/range copying and publication. Destroying a pipeline no longer closes the source descriptor outside its owner. The canonical source also owns cancellation: competing pipeline signals and obsolete stream-error reinterpretation were removed after snapshot/range controls reproduced EIO being replaced by cancellation (2 failed, 29 passed in that intermediate candidate). Final validation on Node 24.18.0: 46 focused tests across four files passed, including actual native file growth from 128 MiB to one byte above the unchanged allowance, successful and denied close for both operations, exact-owner retry, a stable file exactly at the allowance, same-read EIO/cancellation, and existing finalization/mixed-owner/publication controls. Docs checks and five conformance tests passed. Native file IO and controlled faults on Darwin; no Linux firmware engines or public CLI growth reproduction claimed. Two separately selected Linux workspace checks were skipped on this host. Bounded completed snapshot comparison against immediate parent 8451d2839: 32 MiB sparse source, identical size/digest, two alternating warm repeats/build plus separate first processes. Warm medians baseline/candidate: elapsed 0.07248/0.06984 s, CPU 0.06910/0.06844 s, sampled process-family RSS 64790528/57565184 bytes, result JSON tokens 51/51 (cl100k_base). No speed/memory improvement is established by these repeats; true cold, CLI/MCP/whole-agent/schema/request/retry and native-engine costs are unmeasured. Heavy Qodo tests remained stopped. Related remaining shared error-model defect: #4628. After a composite cleanup dependency resolves, its old wrapper still projects execution_failure while the typed primary survives in diagnostics. The firmware patch preserves the corrected primary and exact cleanup ownership; it does not claim to repair that shared post-recovery projection.

bsakaguchi closed this issue

2026-10-31 00:32:44 +00:00

bsakaguchi commented

2026-10-31 00:32:45 +00:00

Author

Owner

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

Closing as fixed on main after reconciling this report with the landed owner/consumer changes.

0a51b246c: fix(firmware): preserve snapshot input failures and descriptor ownership. Validation: 46 focused tests across four files, docs checks and five conformance tests. Native file IO with controlled faults; no Linux firmware engines verified. Shared post-cleanup error projection remains tracked in #4628. Refs #4619, #4475
5eaef92e5: test(firmware): assert cleanup provenance through composed failures. Check public cleanup projection and retained primary diagnostics instead of a superseded error subclass and flat diagnostic location. Require the caller's retained error to resolve after the exact cleanup owner succeeds on retry.

Evidence scope: this closure uses the landed source, regression coverage and validation already recorded in the linked commits/thread. No new local tests or native-provider reproduction were run during this backlog pass; previously documented platform and real-provider gaps still apply.

@morluto commented on GitHub (Oct 18, 2026):  Closing as fixed on `main` after reconciling this report with the landed owner/consumer changes. - [0a51b246c](https://github.com/morluto/rea/commit/0a51b246c632a36b24c121282aeb51fda1ddb461): fix(firmware): preserve snapshot input failures and descriptor ownership. Validation: 46 focused tests across four files, docs checks and five conformance tests. Native file IO with controlled faults; no Linux firmware engines verified. Shared post-cleanup error projection remains tracked in #4628. Refs #4619, #4475 - [5eaef92e5](https://github.com/morluto/rea/commit/5eaef92e5b3f9b15b23f2e4aea6a32a867b89dd0): test(firmware): assert cleanup provenance through composed failures. Check public cleanup projection and retained primary diagnostics instead of a superseded error subclass and flat diagnostic location. Require the caller's retained error to resolve after the exact cleanup owner succeeds on retry. Evidence scope: this closure uses the landed source, regression coverage and validation already recorded in the linked commits/thread. No new local tests or native-provider reproduction were run during this backlog pass; previously documented platform and real-provider gaps still apply.

bsakaguchi referenced this issue

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

[PR #1773] [MERGED] fix(cli): retry retained browser scenario cleanup #4806

bsakaguchi referenced this issue

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

[PR #1780] [MERGED] fix(cli): await browser scenario cleanup on interrupts #4812

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#1771

No description provided.

Rows
Columns

[GH-ISSUE #4475] [Bug] Firmware read-handle close failures discard outcomes and lose cleanup ownership #1771

Before submitting

Area

Observed behavior

Reproduction

Expected behavior

Impact and scope

REA version

Runtime and platform

Target and artifact context

Interface

Earliest failure stage

Relevant output or evidence

Typed outcome or recovery expected

Analysis or suspected cause

Additional context

Root Cause

Resolution

1. retains a setup-failed publication tree for provider cleanup retry

2. retains the complete firmware result when published output cleanup stays uncertain

Linux Fixture Portability Note

1. `retains a setup-failed publication tree for provider cleanup retry`

2. `retains the complete firmware result when published output cleanup stays uncertain`