morluto/rea

Fork 0

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

[PR #6210] [MERGED] fix(ghidra): retry retained language catalog closes before admission #7207

New issue

Closed

opened 2026-11-01 21:46:09 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:46:09 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6210
Author: @kaoru0822-kitauji
Created: 10/22/2026
Status: ✅ Merged
Merged: 10/22/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/6133-ghidra-catalog-close-recovery

📝 Commits (1)

4c2dfa8 fix(ghidra): retry retained language catalog closes before admission

📊 Changes

7 files changed (+487 additions, -192 deletions)

View changed files

📝 src/ghidra/GhidraLanguageCatalog.ts (+60 -110)
📝 src/ghidra/GhidraLanguageOverride.test.ts (+13 -5)
📝 src/ghidra/GhidraProvider.ts (+8 -25)
📝 tests/boundary/filesystem/ghidraLanguageCatalogBound.test.ts (+39 -7)
📝 tests/boundary/filesystem/ghidraLanguageCatalogCleanup.test.ts (+98 -41)
📝 tests/boundary/filesystem/ghidraLanguageCatalogEncoding.test.ts (+7 -4)
➕ tests/boundary/filesystem/ghidraLanguageCatalogRecovery.test.ts (+262 -0)

📄 Description

What broke: A failed close of a Ghidra .ldefs handle during configured-language validation left the registry returning cleanup_incomplete for every later open_binary. Nothing retried the retained handle, so the same registry could not admit new work even after the fault cleared.
Fix: The language catalog now reads definitions through the provider's caller-owned inspection scope with the shared regular-file owner that the seed and extension readers already use. A failed close stays in that scope as an exact lease. Provider close and the next catalog admission retry it, and the translated error is bound to the lease, so a confirmed release reopens the same registry.
Risk: Low to moderate. Catalog failures now use the shared Ghidra file-failure projection, so the user message is the underlying reason rather than a fixed sentence. .ldefs entries also go through regular-file admission: symlinks are still followed, and a non-regular entry such as a FIFO is refused instead of blocking.

Summary

Fixes #6133.

GhidraLanguageCatalog kept unconfirmed closes in a module-global set that only a later catalog read retried. GhidraProvider turned the catalog's cleanup error into a ProviderCleanupError that was bound to no owner. Registry admission retries provider close before it resolves another profile, but provider close only retried its inspection scope, which never held the catalog handle. The original error therefore stayed cleanupIncomplete forever, and a fresh provider/registry was the only way to recover.

Problem and expected behavior

Repro: the issue's MCP test. It rejects x86.ldefs close with a synthetic EIO, then clears the fault. On base 719d56ebf every assertion that characterizes the defect held. After the fault cleared, open_binary/close_binary/open_binary still returned cleanup_incomplete with close calls stuck at 1, and the provider reported no pending resources while the registry stayed blocked.

Expected behavior (docs/tool-design.md ownership and provider-selection lifecycle): retained cleanup is retried before new work, the originating error is bound to its exact owner, and admission reopens after a confirmed retry. While the retry keeps failing, admission stays fail-closed and names the resource.

Change and scope

src/ghidra/GhidraLanguageCatalog.ts: readGhidraLanguageCatalog(installDir, resources, signal?) runs inside ArtifactResourceScope.run, which retries retained owners before it admits a read. Each .ldefs file goes through withArtifactRegularFile(..., "follow"), which retains a failed close as an AnalysisCleanupLease in the caller's scope. The 4 MiB bound, the oversized-file skip and the XML byte decoding are unchanged. Definitions are now parsed before the final close, as the seed reader does.
Removed the superseded module-global pendingLanguageFileCleanup set, its retry helpers and GhidraLanguageCatalogCleanupError.
src/ghidra/GhidraProvider.ts: passes #inspectionResources and the caller signal, and translates failures with the shared ghidraFileFailure, which binds the error to the retained leases. closeOwnedResources/hasPendingOwnedResources now cover the catalog handle with no provider-specific code.
Sibling readers: the seed and extension profile readers already use this scope and lease binding (ghidraProfileArtifactOwnership.test.ts). The catalog was the only Ghidra profile reader with a separate global retention path. No registry change was needed.

Contribution scope: not applicable. This is an ordinary fix within existing boundaries.

Contract and boundary impact

Semantic owner: the provider's inspection ArtifactResourceScope now owns catalog descriptors whose close is unconfirmed. Before, a module-global set shared by every provider in the process owned them.
CLI and MCP/tool-catalog contract: no schema change. Error codes are unchanged (cleanup_incomplete while retained). For a non-cleanup catalog failure, diagnostics.reason is now the underlying reason, and diagnostics.setting is REA_GHIDRA_LANGUAGE_ID.
Provider/platform compatibility: Ghidra profile selection only. No engine or launcher change.
Evidence/provenance: unchanged. cleanup.resources still names the exact .ldefs path.
Target selection, authority and cleanup: a cleanup failure no longer leaks across providers in the process, because each provider retains its own owners.
Generated metadata, package and installation impact: none.

Evidence and regression coverage

tests/boundary/filesystem/ghidraLanguageCatalogRecovery.test.ts (new; uses the production config, provider, registry, session and MCP server over in-memory transport; only installation readiness and the close fault are simulated):
- MCP: a healthy open/close passes. A failed close returns cleanup_incomplete naming the .ldefs path, with the provider reporting pending resources. A second open_binary while the close still fails retries the close (close calls 1 to 2) without reopening the catalog. After the fault clears, open_binary succeeds on the same registry and nothing is left pending.
- Registry admission cancelled during the cleanup retry: the in-flight close is not abandoned, and admission is still pending after the abort while the close is held. If the retry releases, the result is AnalysisCancelledError with the caller's reason and the original error resolves. If it still fails, the result stays cleanup_incomplete naming the path. Both cases recover on the next admission.
- Cancellation during the final catalog close: the result is AnalysisCancelledError with the caller's reason and the catalog lease retained, and the error resolves after provider close confirms release.
tests/boundary/filesystem/ghidraLanguageCatalogCleanup.test.ts (rewritten for the caller-owned scope) covers:
- a successful retry
- a retry that still fails, where no new definition is opened
- isolation between scopes
- several retained owners retried together
- the read failure staying primary when close also fails
- an empty catalog still being blocked
- a sticky invalidated descriptor
ghidraLanguageCatalogBound.test.ts adds two counterexamples: a symlinked .ldefs file is still followed, and a FIFO .ldefs file is refused with ENOTFILE without waiting for a writer and without retaining anything.
Without the source change, all 11 new or rewritten recovery and cleanup tests fail. The key MCP assertion that fails is the provider reporting no pending owner after the failed close.
The issue's repro file was run unchanged on base and reproduced the bug. It is not committed, and it cannot run unchanged after the fix because it calls the old one-argument catalog signature. The new MCP boundary test covers the same sequence.
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

Issue MCP repro on base 719d56ebf: reproduced.
vitest run src/ghidra tests/boundary/filesystem/ghidra tests/boundary/providers/ghidra tests/boundary/mcp/ghidra tests/process-global/ghidra plus the 22 test files that reference AnalysisProviderRegistry (--maxWorkers=4): 75 files. All passed except 4 files that need dist/. After npm run build:cached, those 4 passed (25 tests).
vitest run tests/boundary/filesystem/ghidra src/ghidra/GhidraLanguageOverride.test.ts: 11 files, 161 passed.
npm run check:fast: passed.
npm run format:check: passed.

Compatibility, safety, and release

Breaking changes: none for CLI/MCP. The internal readGhidraLanguageCatalog signature now requires a resource scope, and GhidraLanguageCatalogCleanupError was removed. Neither has external consumers.
Real Hopper/Ghidra coverage: none. No Ghidra or Java process ran. Close faults were injected, and installation readiness came from the repository fixture host.
Performance: each definition read adds one fstat for regular-file admission. Not measured, because these are a handful of small files read once per profile resolution, and agent output size is unchanged.
Package or release metadata impact: none.

Author readiness

Human owner: @kaoru0822-kitauji

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.

🤖 Generated with Claude Code

_{🔄 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/6210 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/22/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/6133-ghidra-catalog-close-recovery` --- ### 📝 Commits (1) - [`4c2dfa8`](https://github.com/morluto/rea/commit/4c2dfa878144cafb2d1e938f382efa5bc4457f9f) fix(ghidra): retry retained language catalog closes before admission ### 📊 Changes **7 files changed** (+487 additions, -192 deletions) <details> <summary>View changed files</summary> 📝 `src/ghidra/GhidraLanguageCatalog.ts` (+60 -110) 📝 `src/ghidra/GhidraLanguageOverride.test.ts` (+13 -5) 📝 `src/ghidra/GhidraProvider.ts` (+8 -25) 📝 `tests/boundary/filesystem/ghidraLanguageCatalogBound.test.ts` (+39 -7) 📝 `tests/boundary/filesystem/ghidraLanguageCatalogCleanup.test.ts` (+98 -41) 📝 `tests/boundary/filesystem/ghidraLanguageCatalogEncoding.test.ts` (+7 -4) ➕ `tests/boundary/filesystem/ghidraLanguageCatalogRecovery.test.ts` (+262 -0) </details> ### 📄 Description **What broke:** A failed close of a Ghidra `.ldefs` handle during configured-language validation left the registry returning `cleanup_incomplete` for every later `open_binary`. Nothing retried the retained handle, so the same registry could not admit new work even after the fault cleared. **Fix:** The language catalog now reads definitions through the provider's caller-owned inspection scope with the shared regular-file owner that the seed and extension readers already use. A failed close stays in that scope as an exact lease. Provider close and the next catalog admission retry it, and the translated error is bound to the lease, so a confirmed release reopens the same registry. **Risk:** Low to moderate. Catalog failures now use the shared Ghidra file-failure projection, so the user message is the underlying reason rather than a fixed sentence. `.ldefs` entries also go through regular-file admission: symlinks are still followed, and a non-regular entry such as a FIFO is refused instead of blocking. ## Summary Fixes #6133. `GhidraLanguageCatalog` kept unconfirmed closes in a module-global set that only a later catalog read retried. `GhidraProvider` turned the catalog's cleanup error into a `ProviderCleanupError` that was bound to no owner. Registry admission retries provider close before it resolves another profile, but provider close only retried its inspection scope, which never held the catalog handle. The original error therefore stayed `cleanupIncomplete` forever, and a fresh provider/registry was the only way to recover. ## Problem and expected behavior Repro: the issue's MCP test. It rejects `x86.ldefs` close with a synthetic `EIO`, then clears the fault. On base `719d56ebf` every assertion that characterizes the defect held. After the fault cleared, `open_binary`/`close_binary`/`open_binary` still returned `cleanup_incomplete` with close calls stuck at 1, and the provider reported no pending resources while the registry stayed blocked. Expected behavior (docs/tool-design.md ownership and provider-selection lifecycle): retained cleanup is retried before new work, the originating error is bound to its exact owner, and admission reopens after a confirmed retry. While the retry keeps failing, admission stays fail-closed and names the resource. ## Change and scope - `src/ghidra/GhidraLanguageCatalog.ts`: `readGhidraLanguageCatalog(installDir, resources, signal?)` runs inside `ArtifactResourceScope.run`, which retries retained owners before it admits a read. Each `.ldefs` file goes through `withArtifactRegularFile(..., "follow")`, which retains a failed close as an `AnalysisCleanupLease` in the caller's scope. The 4 MiB bound, the oversized-file skip and the XML byte decoding are unchanged. Definitions are now parsed before the final close, as the seed reader does. - Removed the superseded module-global `pendingLanguageFileCleanup` set, its retry helpers and `GhidraLanguageCatalogCleanupError`. - `src/ghidra/GhidraProvider.ts`: passes `#inspectionResources` and the caller signal, and translates failures with the shared `ghidraFileFailure`, which binds the error to the retained leases. `closeOwnedResources`/`hasPendingOwnedResources` now cover the catalog handle with no provider-specific code. - Sibling readers: the seed and extension profile readers already use this scope and lease binding (`ghidraProfileArtifactOwnership.test.ts`). The catalog was the only Ghidra profile reader with a separate global retention path. No registry change was needed. Contribution scope: not applicable. This is an ordinary fix within existing boundaries. ## Contract and boundary impact - Semantic owner: the provider's inspection `ArtifactResourceScope` now owns catalog descriptors whose close is unconfirmed. Before, a module-global set shared by every provider in the process owned them. - CLI and MCP/tool-catalog contract: no schema change. Error codes are unchanged (`cleanup_incomplete` while retained). For a non-cleanup catalog failure, `diagnostics.reason` is now the underlying reason, and `diagnostics.setting` is `REA_GHIDRA_LANGUAGE_ID`. - Provider/platform compatibility: Ghidra profile selection only. No engine or launcher change. - Evidence/provenance: unchanged. `cleanup.resources` still names the exact `.ldefs` path. - Target selection, authority and cleanup: a cleanup failure no longer leaks across providers in the process, because each provider retains its own owners. - Generated metadata, package and installation impact: none. ## Evidence and regression coverage - `tests/boundary/filesystem/ghidraLanguageCatalogRecovery.test.ts` (new; uses the production config, provider, registry, session and MCP server over in-memory transport; only installation readiness and the close fault are simulated): - MCP: a healthy open/close passes. A failed close returns `cleanup_incomplete` naming the `.ldefs` path, with the provider reporting pending resources. A second `open_binary` while the close still fails retries the close (close calls 1 to 2) without reopening the catalog. After the fault clears, `open_binary` succeeds on the same registry and nothing is left pending. - Registry admission cancelled during the cleanup retry: the in-flight close is not abandoned, and admission is still pending after the abort while the close is held. If the retry releases, the result is `AnalysisCancelledError` with the caller's reason and the original error resolves. If it still fails, the result stays `cleanup_incomplete` naming the path. Both cases recover on the next admission. - Cancellation during the final catalog close: the result is `AnalysisCancelledError` with the caller's reason and the catalog lease retained, and the error resolves after provider close confirms release. - `tests/boundary/filesystem/ghidraLanguageCatalogCleanup.test.ts` (rewritten for the caller-owned scope) covers: - a successful retry - a retry that still fails, where no new definition is opened - isolation between scopes - several retained owners retried together - the read failure staying primary when close also fails - an empty catalog still being blocked - a sticky invalidated descriptor - `ghidraLanguageCatalogBound.test.ts` adds two counterexamples: a symlinked `.ldefs` file is still followed, and a FIFO `.ldefs` file is refused with `ENOTFILE` without waiting for a writer and without retaining anything. - Without the source change, all 11 new or rewritten recovery and cleanup tests fail. The key MCP assertion that fails is the provider reporting no pending owner after the failed close. - The issue's repro file was run unchanged on base and reproduced the bug. It is not committed, and it cannot run unchanged after the fix because it calls the old one-argument catalog signature. The new MCP boundary test covers the same sequence. - [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 - Issue MCP repro on base `719d56ebf`: reproduced. - `vitest run src/ghidra tests/boundary/filesystem/ghidra tests/boundary/providers/ghidra tests/boundary/mcp/ghidra tests/process-global/ghidra` plus the 22 test files that reference `AnalysisProviderRegistry` (`--maxWorkers=4`): 75 files. All passed except 4 files that need `dist/`. After `npm run build:cached`, those 4 passed (25 tests). - `vitest run tests/boundary/filesystem/ghidra src/ghidra/GhidraLanguageOverride.test.ts`: 11 files, 161 passed. - `npm run check:fast`: passed. - `npm run format:check`: passed. ## Compatibility, safety, and release - Breaking changes: none for CLI/MCP. The internal `readGhidraLanguageCatalog` signature now requires a resource scope, and `GhidraLanguageCatalogCleanupError` was removed. Neither has external consumers. - Real Hopper/Ghidra coverage: none. No Ghidra or Java process ran. Close faults were injected, and installation readiness came from the repository fixture host. - Performance: each definition read adds one `fstat` for regular-file admission. Not measured, because these are a handful of small files read once per profile resolution, and agent output size is unchanged. - Package or release metadata impact: none. ## Author readiness Human owner: @kaoru0822-kitauji - [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. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-11-01 21:46:09 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-03 13:39:51 +00:00

[PR #7207] [MERGED] fix(setup): report Kuna cache aliases consistently #7717

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

No description provided.

Rows
Columns