[PR #4668] [MERGED] fix(electron): retain fuse descriptor ownership through failed final close #6351

Closed
opened 2026-10-31 01:13:48 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4668
Author: @nosmile99
Created: 10/17/2026
Status: ✅ Merged
Merged: 10/17/2026
Merged by: @morluto

Base: main ← Head: fix/electron-fuse-final-close-ownership


📝 Commits (3)

  • 3136977 fix(electron): retain fuse descriptor ownership through failed final close
  • 1d88a75 fix(electron): keep cleanup report and partial evidence through cancel and successful reads
  • 6bb9491 fix(electron): resolve fuse cleanup cancellation conflicts

📊 Changes

3 files changed (+471 additions, -35 deletions)

View changed files

📝 src/electron/ElectronFuseProvider.ts (+122 -0)
📝 src/electron/ElectronFuseReader.ts (+46 -35)
➕ src/electron/ElectronFuseReaderCleanup.test.ts (+303 -0)

📄 Description

Summary

Fixes #4658. Static Electron fuse inspection admitted a file descriptor and closed it in a bare finally: a rejecting final close() replaced the in-flight read error, was classified by errno (e.g. EACCES) as a read permission failure, and silently dropped ownership of the still-live descriptor.

AI-assisted contribution (no human owner available): Author readiness declarations are left unchecked per the repo's AGENTS.md. @morluto marked this PR ready for review on 2026-10-17; the automated-review findings below were addressed after that.

Problem and expected behavior

Smallest trigger: fault close() on the admitted descriptor (reject with EACCES) while a read fails with EIO. Violated invariants:

  1. The close rejection replaced the primary EIO read error (throw in finally).
  2. The response classified the close error as a filesystem read permission failure (AnalysisAccessDeniedError, "Selected artifact could not be read (EACCES)").
  3. Ownership of the still-live descriptor was lost with no incomplete-cleanup report.

Expected: the primary error keeps its classification, the close failure is reported as incomplete cleanup, and the descriptor owner is retained.

Change and scope

  • src/electron/ElectronFuseReader.ts: route the admitted read through the existing withOwnedRegularFile primitive (same one used by the adb/jadx/linux-hopper providers) instead of a bare try/finally. A failed close now throws RegularFileCleanupFailure carrying the read outcome (primary error preserved), the cleanup cause, and the exact owner. New optional retainFailedClose passthrough on readElectronFuseImage options; no caller changes required.
  • src/electron/ElectronFuseProvider.ts: map RegularFileCleanupFailure in failureFor — the primary outcome keeps its own classification (recursed through failureFor), combined with a ProviderCleanupError (cleanupIncomplete=true) via analysisErrorWithCleanupFailure, mirroring JadxProvider.executionError. A close failure can no longer surface as a read permission denial.
  • Intentionally not included: no retry of the failed close, no new CLI/MCP surface, no retention registry — the error retains the owner for the caller/operator, and incomplete cleanup is reported rather than silently recovered.

Contribution scope: ordinary fix within existing boundaries (no new provider, no target execution/injection/mutation, no protection recovery) — maintainer scope decision not applicable.

Contract and boundary impact

  • Semantic owner and earliest changed stage: filesystem ownership primitive, at the final-close stage of readElectronFuseImage.
  • Contribution scope fit and linked maintainer decision: ordinary fix; not applicable.
  • CLI and MCP/tool-catalog contract: no signature or schema change; error classification changes only in the failed-close path.
  • Provider, bridge, target-format, or platform compatibility: none.
  • Evidence, artifact, provenance, or reconstruction contract: none.
  • Target selection, authority, upstream behavior, and cleanup details: cleanup is now reported incomplete instead of silently dropped.

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/electron/ElectronFuseReaderCleanup.test.ts (6 tests) — faults close() with EACCES with and without a primary read error (EIO), via the same vi.mock("node:fs/promises") open-wrapper convention as RegularFileAdmission.test.ts.

  • Base reproduction or other evidence: executed. The two fault tests fail on the pre-fix revision (stashed fix, tests kept): close rejection replaced the primary error, and the provider mapped EACCES to AnalysisAccessDeniedError. All 3 pass with the fix. Fault-assertion details: outcome.cause is the original EIO error (identity), cleanupCause is the EACCES error, owner.handle is the admitted handle, and fstatSync(fd) still succeeds on the retained descriptor.

  • User-visible CLI/MCP output: none (error-path only).

  • Remaining proof gaps: Windows taskkill-style abrupt termination and real Chrome-less CI coverage of the close path are unavailable here; the fault injection is controlled and local-only.

  • Observed, derived, and inferred claims remain distinguishable.

  • Artifact identity, source provenance, and failed attempts remain preserved.

  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Current merge-resolution validation

Merged main at f85a932d7 in 6bb94914e. Keep failed-close handling before cancellation mapping so the cleanup report and descriptor owner survive. Use main's regularFileCancellation facts for cancellation recorded by the operation/final close. A read failure that precedes cancellation during close retains its own classification, alongside incomplete cleanup. Completed fuse observations remain attached as partial Evidence.

Added a synchronized read-failure + abort-during-close + close-failure regression; reused the fault helper for close-boundary cancellation. Executed on macOS with Node 24.18.0:

  • npm run test:focused -- src/electron/ElectronFuseProvider.test.ts src/electron/ElectronFuseReaderCleanup.test.ts tests/boundary/filesystem/regularFileCancellation.test.ts tests/boundary/mcp/electronFusesMcp.test.ts: passed, 63 tests across four files, including the public MCP fuse workflow.
  • After splitting the new regression into a separate describe block to satisfy the function line limit, npm run test:local -- src/electron/ElectronFuseReaderCleanup.test.ts: passed, 7 tests.
  • npm run check:fast: passed (typecheck and lint).
  • git diff origin/main...HEAD --check: passed; pre-commit formatting/lint passed.

Close/cancellation faults are controlled filesystem-boundary injections. No real failing OS close, Windows runtime, abrupt process termination, or retained-owner retry registry was verified or added by this resolution. Existing limitations concerning provider/session retry ownership remain.

Original validation (before merge resolution)

  • npx vitest run src/electron/ElectronFuseReaderCleanup.test.ts — 3/3 pass with fix; 2/3 fail pre-fix (fault tests), 1/3 pass pre-fix (control).
  • npx vitest run src/electron/ src/application/electron/ src/domain/electron/ src/filesystem/ — 36/36 pass.
  • npm run check:fast — typecheck + lint clean.

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: Linux only (Node 24); the fault is injected at the node:fs/promises boundary and is platform-independent by construction.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: the retained failed-close owner keeps a live fd until process exit; this is now reported via cleanupIncomplete instead of being silently dropped. No target interaction changes.

Automated-review follow-up

Greptile/Codex findings addressed in commit 1d88a75 ("fix(electron): keep cleanup report and partial evidence through cancel and successful reads"):

  • Cancellation hid failed cleanup (P1, both bots): confirmed by code reading — failureFor returned a bare AnalysisCancelledError when signal.aborted fired before the RegularFileCleanupFailure branch, silently dropping the descriptor owner. The cleanup-failure branch now runs first; on abort it composes AnalysisCancelledError (cause = signal.reason) with the incomplete-cleanup error, keeping cleanupIncomplete, cleanupResources, and the owner in the cause chain.
  • Completed fuse evidence discarded (Codex P1): confirmed — the completed branch discarded the successful read. The completed ElectronFuseImage is now attached as explicitly-partial Evidence (confidence observed, sha256/bytes/format + slices, limitations marking cleanup incomplete) on the primary error; the abort-after-completion path attaches the same evidence to the composed cancellation error, mirroring PcapProvider.
  • Combined-fault coverage (Greptile P2): added a provider-level test with faulted read (EIO) + faulted close (EACCES) asserting the primary diagnostic keeps the read classification, cleanupIncomplete, and the retained owner in the cause chain.
  • Retry lease for failed closes (Codex P1): acknowledged, not implemented here. Verified the descriptor owner is retained in the error cause chain (RegularFileCleanupFailure.owner carried by ProviderCleanupError.cause); retention.kind === "not_requested" only means no retainFailedClose callback was registered. A session/provider-scoped retry lease is a design decision for the long-lived MCP server — left for the maintainer.

Validation: the abort fires inside the faulted close (synchronized with the finalization boundary, no fixed delay). The abort-during-close and partial-evidence tests fail pre-fix (stashed provider) and pass post-fix; the combined-fault test guards the already-working failed-branch mapping. 29/29 electron tests green; npm run check:fast clean.

Author readiness

Human owner:

  • 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/4668 **Author:** [@nosmile99](https://github.com/nosmile99) **Created:** 10/17/2026 **Status:** ✅ Merged **Merged:** 10/17/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/electron-fuse-final-close-ownership` --- ### 📝 Commits (3) - [`3136977`](https://github.com/morluto/rea/commit/313697791634f732b8a2826e52916d91977d3f03) fix(electron): retain fuse descriptor ownership through failed final close - [`1d88a75`](https://github.com/morluto/rea/commit/1d88a753d8bffb8db7cfbd96edd84652f6fe9942) fix(electron): keep cleanup report and partial evidence through cancel and successful reads - [`6bb9491`](https://github.com/morluto/rea/commit/6bb94914eb6270e0bc1f43f63c7247a7aa330508) fix(electron): resolve fuse cleanup cancellation conflicts ### 📊 Changes **3 files changed** (+471 additions, -35 deletions) <details> <summary>View changed files</summary> 📝 `src/electron/ElectronFuseProvider.ts` (+122 -0) 📝 `src/electron/ElectronFuseReader.ts` (+46 -35) ➕ `src/electron/ElectronFuseReaderCleanup.test.ts` (+303 -0) </details> ### 📄 Description ## Summary Fixes #4658. Static Electron fuse inspection admitted a file descriptor and closed it in a bare `finally`: a rejecting final `close()` replaced the in-flight read error, was classified by errno (e.g. `EACCES`) as a read permission failure, and silently dropped ownership of the still-live descriptor. AI-assisted contribution (no human owner available): Author readiness declarations are left unchecked per the repo's AGENTS.md. @morluto marked this PR ready for review on 2026-10-17; the automated-review findings below were addressed after that. ## Problem and expected behavior Smallest trigger: fault `close()` on the admitted descriptor (reject with `EACCES`) while a read fails with `EIO`. Violated invariants: 1. The close rejection replaced the primary `EIO` read error (throw in `finally`). 2. The response classified the close error as a filesystem **read** permission failure (`AnalysisAccessDeniedError`, "Selected artifact could not be read (EACCES)"). 3. Ownership of the still-live descriptor was lost with no incomplete-cleanup report. Expected: the primary error keeps its classification, the close failure is reported as incomplete cleanup, and the descriptor owner is retained. ## Change and scope - `src/electron/ElectronFuseReader.ts`: route the admitted read through the existing `withOwnedRegularFile` primitive (same one used by the adb/jadx/linux-hopper providers) instead of a bare `try/finally`. A failed close now throws `RegularFileCleanupFailure` carrying the read outcome (primary error preserved), the cleanup cause, and the exact owner. New optional `retainFailedClose` passthrough on `readElectronFuseImage` options; no caller changes required. - `src/electron/ElectronFuseProvider.ts`: map `RegularFileCleanupFailure` in `failureFor` — the primary outcome keeps its own classification (recursed through `failureFor`), combined with a `ProviderCleanupError` (`cleanupIncomplete=true`) via `analysisErrorWithCleanupFailure`, mirroring `JadxProvider.executionError`. A close failure can no longer surface as a read permission denial. - Intentionally not included: no retry of the failed close, no new CLI/MCP surface, no retention registry — the error retains the owner for the caller/operator, and incomplete cleanup is reported rather than silently recovered. Contribution scope: ordinary fix within existing boundaries (no new provider, no target execution/injection/mutation, no protection recovery) — maintainer scope decision not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: filesystem ownership primitive, at the final-close stage of `readElectronFuseImage`. - Contribution scope fit and linked maintainer decision: ordinary fix; not applicable. - CLI and MCP/tool-catalog contract: no signature or schema change; error classification changes only in the failed-close path. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details: cleanup is now reported incomplete instead of silently dropped. 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/electron/ElectronFuseReaderCleanup.test.ts` (6 tests) — faults `close()` with `EACCES` with and without a primary read error (`EIO`), via the same `vi.mock("node:fs/promises")` open-wrapper convention as `RegularFileAdmission.test.ts`. - Base reproduction or other evidence: executed. The two fault tests **fail on the pre-fix revision** (stashed fix, tests kept): close rejection replaced the primary error, and the provider mapped `EACCES` to `AnalysisAccessDeniedError`. All 3 pass with the fix. Fault-assertion details: `outcome.cause` is the original `EIO` error (identity), `cleanupCause` is the `EACCES` error, `owner.handle` is the admitted handle, and `fstatSync(fd)` still succeeds on the retained descriptor. - User-visible CLI/MCP output: none (error-path only). - Remaining proof gaps: Windows `taskkill`-style abrupt termination and real Chrome-less CI coverage of the close path are unavailable here; the fault injection is controlled and local-only. - [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. ## Current merge-resolution validation Merged main at `f85a932d7` in `6bb94914e`. Keep failed-close handling before cancellation mapping so the cleanup report and descriptor owner survive. Use main's `regularFileCancellation` facts for cancellation recorded by the operation/final close. A read failure that precedes cancellation during close retains its own classification, alongside incomplete cleanup. Completed fuse observations remain attached as partial Evidence. Added a synchronized read-failure + abort-during-close + close-failure regression; reused the fault helper for close-boundary cancellation. Executed on macOS with Node 24.18.0: - `npm run test:focused -- src/electron/ElectronFuseProvider.test.ts src/electron/ElectronFuseReaderCleanup.test.ts tests/boundary/filesystem/regularFileCancellation.test.ts tests/boundary/mcp/electronFusesMcp.test.ts`: passed, 63 tests across four files, including the public MCP fuse workflow. - After splitting the new regression into a separate describe block to satisfy the function line limit, `npm run test:local -- src/electron/ElectronFuseReaderCleanup.test.ts`: passed, 7 tests. - `npm run check:fast`: passed (typecheck and lint). - `git diff origin/main...HEAD --check`: passed; pre-commit formatting/lint passed. Close/cancellation faults are controlled filesystem-boundary injections. No real failing OS close, Windows runtime, abrupt process termination, or retained-owner retry registry was verified or added by this resolution. Existing limitations concerning provider/session retry ownership remain. ## Original validation (before merge resolution) - `npx vitest run src/electron/ElectronFuseReaderCleanup.test.ts` — 3/3 pass with fix; 2/3 fail pre-fix (fault tests), 1/3 pass pre-fix (control). - `npx vitest run src/electron/ src/application/electron/ src/domain/electron/ src/filesystem/` — 36/36 pass. - `npm run check:fast` — typecheck + lint clean. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: Linux only (Node 24); the fault is injected at the `node:fs/promises` boundary and is platform-independent by construction. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the retained failed-close owner keeps a live fd until process exit; this is now reported via `cleanupIncomplete` instead of being silently dropped. No target interaction changes. ## Automated-review follow-up Greptile/Codex findings addressed in commit 1d88a75 ("fix(electron): keep cleanup report and partial evidence through cancel and successful reads"): - **Cancellation hid failed cleanup (P1, both bots):** confirmed by code reading — `failureFor` returned a bare `AnalysisCancelledError` when `signal.aborted` fired before the `RegularFileCleanupFailure` branch, silently dropping the descriptor owner. The cleanup-failure branch now runs first; on abort it composes `AnalysisCancelledError` (cause = `signal.reason`) with the incomplete-cleanup error, keeping `cleanupIncomplete`, `cleanupResources`, and the owner in the cause chain. - **Completed fuse evidence discarded (Codex P1):** confirmed — the completed branch discarded the successful read. The completed `ElectronFuseImage` is now attached as explicitly-partial `Evidence` (confidence `observed`, sha256/bytes/format + slices, limitations marking cleanup incomplete) on the primary error; the abort-after-completion path attaches the same evidence to the composed cancellation error, mirroring `PcapProvider`. - **Combined-fault coverage (Greptile P2):** added a provider-level test with faulted read (`EIO`) + faulted close (`EACCES`) asserting the primary diagnostic keeps the read classification, `cleanupIncomplete`, and the retained owner in the cause chain. - **Retry lease for failed closes (Codex P1):** acknowledged, not implemented here. Verified the descriptor owner is retained in the error cause chain (`RegularFileCleanupFailure.owner` carried by `ProviderCleanupError.cause`); `retention.kind === "not_requested"` only means no `retainFailedClose` callback was registered. A session/provider-scoped retry lease is a design decision for the long-lived MCP server — left for the maintainer. Validation: the abort fires inside the faulted close (synchronized with the finalization boundary, no fixed delay). The abort-during-close and partial-evidence tests fail pre-fix (stashed provider) and pass post-fix; the combined-fault test guards the already-working failed-branch mapping. 29/29 electron tests green; `npm run check:fast` clean. ## Author readiness Human owner: - [ ] 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 - [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. - [ ] Owning docs, contracts, and generated metadata are updated where needed. - [ ] 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:13:48 +00:00
Sign in to join this conversation.
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#6351
No description provided.