[PR #5301] [CLOSED] fix(sqlite): bind returned cleanup errors to their exact root and worker owners #6867

Closed
opened 2026-11-01 21:43:40 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5301
Author: @charan-rathore
Created: 10/19/2026
Status: ❌ Closed

Base: main ← Head: fix/5294-sqlite-cleanup-owner-binding


📝 Commits (10+)

  • 509b8f1 fix(sqlite): bind returned cleanup errors to their exact root and worker owners
  • 2e85d6c test(sqlite): cover cleanup-error owner binding through release
  • 7f20e30 fix(sqlite): bind close-time and root-wrapper failures to every cleanup owner
  • c1f921a test(sqlite): pin descriptor-close failure resolution through its leases
  • e5e8736 fix(sqlite): keep prior cleanup failures as dependencies of root-retry wrappers
  • cefe0e2 style(sqlite): drop the trailing blank line oxfmt rejects
  • aa96e45 test(sqlite): type the caught aggregate in the close-descriptor cleanup test
  • 5f4329a test(sqlite): compare the aggregate owner list order-insensitively
  • b708c5a test(sqlite): prove a root-retry wrapper waits on its embedded read failure
  • 0606858 fix(sqlite): keep a root-retry wrapper pending until its retained owner releases

📊 Changes

5 files changed (+593 additions, -74 deletions)

View changed files

📝 src/artifacts/AnalysisCleanupLease.test.ts (+22 -0)
📝 src/artifacts/AnalysisCleanupLease.ts (+5 -0)
📝 src/domain/analysisErrorBase.ts (+18 -3)
📝 src/sqlite/SqliteDatabaseProvider.ts (+167 -70)
📝 tests/boundary/sqlite/sqliteDatabaseCleanup.test.ts (+381 -1)

📄 Description

Summary

Fixes #5294. A cleanup error returned from the SQLite provider kept reporting cleanupIncomplete: true and the removed runtime root after provider.close() had already confirmed that root's release, so a caller retaining the original error saw a cleanup blocker for a resource the owner no longer held.

Problem and expected behavior

Smallest trigger: the worker launch fails (or the first snapshot removal is denied), so inspect returns a ProviderCleanupError; a later provider.close() removes the actual root successfully. The returned error still reports cleanupIncomplete: true and keeps the deleted path in cleanupResources. A returned cleanup error must stay bound to the exact worker and root owners it reports, resolve only when every one of them has confirmed release, and never keep listing a released owner's resources.

Change and scope

src/sqlite/SqliteDatabaseProvider.ts now gives every retained root one AnalysisCleanupLease whose release is staged (confirm the retained worker has stopped, then remove the snapshot) and whose live resource attribution comes from the pending map — worker resources while the worker is retained, then the root path, then nothing. Returned inspection errors, retry failures, and close aggregates are bound to their exact leases through AnalysisCleanupLeaseGroup, and multi-failure aggregates use bindCleanupDependencies, the same ownership pattern ArtifactAnalysisResources already uses. The artifact-descriptor scope keeps owning its own leases; no API, admission, or evidence behavior changes.

Contribution scope: not applicable — ordinary fix within the existing SQLite provider boundary.

Contract and boundary impact

  • Semantic owner and earliest changed stage: SqliteDatabaseProvider; cleanup, cancellation, and lifecycle stage only.
  • Contribution scope fit and linked maintainer decision: not applicable (ordinary fix within existing boundaries).
  • CLI and MCP/tool-catalog contract: unchanged.
  • Provider, bridge, target-format, or platform compatibility: unchanged; boundary tests run on Linux.
  • Evidence, artifact, provenance, or reconstruction contract: unchanged; partial-Evidence retention across continued cleanup refusal is preserved.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): returned cleanup errors now resolve (cleanupIncomplete: false, empty cleanupResources) only after every owner they report has confirmed release; a still-owned worker or root continues to block replacement admission exactly as before.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in .agents/skills/reverse-engineer-anything/ and never commit build outputs): none.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

  • 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.

No new effects: the diff changes only how returned errors track the cleanup owners the provider already retains.

Evidence and regression coverage

  • Tests added or updated: tests/boundary/sqlite/sqliteDatabaseCleanup.test.ts — three new regressions: a returned error resolves and empties once its retained root's close is confirmed; a worker-plus-root error drops only the worker's resources when the worker's stop is confirmed, then resolves when the root is removed; a close aggregate tracks every unresolved owner until each retained root releases.
  • Base reproduction or other evidence: each new test fails on base revision 71ae02c without the provider change (the kept error stays cleanupIncomplete with the removed path still listed) and passes with it.
  • User-visible CLI/MCP output (if applicable): none.
  • Remaining proof gaps: macOS worker paths are exercised through the same seams but were not run on Darwin; two SQLite boundary failures on Linux (cancels an acquired SQLite worker and removes its private snapshot, retains reply-read cancellation and retries the failed reply descriptor close) reproduce identically with and without this change.

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 tests/boundary/sqlite/sqliteDatabaseCleanup.test.ts — 8 passed (5 existing + 3 new).
  • Same file with the provider change reverted — the 3 new tests fail (RED), the 5 existing pass.
  • npx vitest run tests/boundary/sqlite/ — 48 passed, 42 skipped, 2 failed; both failures reproduce identically on clean 71ae02c.

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable; Linux boundary suite only.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no new process launches, target interaction, or external writes; the change binds returned errors to cleanup owners the provider already retains.

Author readiness

Human owner: @charan-rathore

  • 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.

AI-assisted; Charan Rathore is the human driver.


🔄 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/5301 **Author:** [@charan-rathore](https://github.com/charan-rathore) **Created:** 10/19/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/5294-sqlite-cleanup-owner-binding` --- ### 📝 Commits (10+) - [`509b8f1`](https://github.com/morluto/rea/commit/509b8f1f6aad886b45f0c3991c826d46bec4a42f) fix(sqlite): bind returned cleanup errors to their exact root and worker owners - [`2e85d6c`](https://github.com/morluto/rea/commit/2e85d6ce08e66472e637e6a5a7bde01c37f1bbca) test(sqlite): cover cleanup-error owner binding through release - [`7f20e30`](https://github.com/morluto/rea/commit/7f20e30f1a8b7ff18af0f4ea285e2bf267d3ce84) fix(sqlite): bind close-time and root-wrapper failures to every cleanup owner - [`c1f921a`](https://github.com/morluto/rea/commit/c1f921a39d09f9e0ca446628941f4e63d5b04b41) test(sqlite): pin descriptor-close failure resolution through its leases - [`e5e8736`](https://github.com/morluto/rea/commit/e5e873649b20c3bf4bcdd1ba1e0d8e84dd97e664) fix(sqlite): keep prior cleanup failures as dependencies of root-retry wrappers - [`cefe0e2`](https://github.com/morluto/rea/commit/cefe0e27365b09111e8ecf2e3e8cfc2f4f6b8f2e) style(sqlite): drop the trailing blank line oxfmt rejects - [`aa96e45`](https://github.com/morluto/rea/commit/aa96e452329d51140dafb382bd40403bfe8642a0) test(sqlite): type the caught aggregate in the close-descriptor cleanup test - [`5f4329a`](https://github.com/morluto/rea/commit/5f4329ab75f361602b2ce391648c40cee5a0b4c0) test(sqlite): compare the aggregate owner list order-insensitively - [`b708c5a`](https://github.com/morluto/rea/commit/b708c5a2cee91d63e7bb74ca31743cdd7534d3c1) test(sqlite): prove a root-retry wrapper waits on its embedded read failure - [`0606858`](https://github.com/morluto/rea/commit/060685819361ecfb1416f547a0c60d6b03885d39) fix(sqlite): keep a root-retry wrapper pending until its retained owner releases ### 📊 Changes **5 files changed** (+593 additions, -74 deletions) <details> <summary>View changed files</summary> 📝 `src/artifacts/AnalysisCleanupLease.test.ts` (+22 -0) 📝 `src/artifacts/AnalysisCleanupLease.ts` (+5 -0) 📝 `src/domain/analysisErrorBase.ts` (+18 -3) 📝 `src/sqlite/SqliteDatabaseProvider.ts` (+167 -70) 📝 `tests/boundary/sqlite/sqliteDatabaseCleanup.test.ts` (+381 -1) </details> ### 📄 Description ## Summary Fixes #5294. A cleanup error returned from the SQLite provider kept reporting `cleanupIncomplete: true` and the removed runtime root after `provider.close()` had already confirmed that root's release, so a caller retaining the original error saw a cleanup blocker for a resource the owner no longer held. ## Problem and expected behavior Smallest trigger: the worker launch fails (or the first snapshot removal is denied), so `inspect` returns a `ProviderCleanupError`; a later `provider.close()` removes the actual root successfully. The returned error still reports `cleanupIncomplete: true` and keeps the deleted path in `cleanupResources`. A returned cleanup error must stay bound to the exact worker and root owners it reports, resolve only when every one of them has confirmed release, and never keep listing a released owner's resources. ## Change and scope `src/sqlite/SqliteDatabaseProvider.ts` now gives every retained root one `AnalysisCleanupLease` whose release is staged (confirm the retained worker has stopped, then remove the snapshot) and whose live resource attribution comes from the pending map — worker resources while the worker is retained, then the root path, then nothing. Returned inspection errors, retry failures, and close aggregates are bound to their exact leases through `AnalysisCleanupLeaseGroup`, and multi-failure aggregates use `bindCleanupDependencies`, the same ownership pattern `ArtifactAnalysisResources` already uses. The artifact-descriptor scope keeps owning its own leases; no API, admission, or evidence behavior changes. Contribution scope: not applicable — ordinary fix within the existing SQLite provider boundary. ## Contract and boundary impact - Semantic owner and earliest changed stage: `SqliteDatabaseProvider`; cleanup, cancellation, and lifecycle stage only. - Contribution scope fit and linked maintainer decision: not applicable (ordinary fix within existing boundaries). - CLI and MCP/tool-catalog contract: unchanged. - Provider, bridge, target-format, or platform compatibility: unchanged; boundary tests run on Linux. - Evidence, artifact, provenance, or reconstruction contract: unchanged; partial-Evidence retention across continued cleanup refusal is preserved. - Target selection, authority, upstream behavior, and cleanup details (if applicable): returned cleanup errors now resolve (`cleanupIncomplete: false`, empty `cleanupResources`) only after every owner they report has confirmed release; a still-owned worker or root continues to block replacement admission exactly as before. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `.agents/skills/reverse-engineer-anything/` and never commit build outputs): none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [ ] 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. No new effects: the diff changes only how returned errors track the cleanup owners the provider already retains. ## Evidence and regression coverage - Tests added or updated: `tests/boundary/sqlite/sqliteDatabaseCleanup.test.ts` — three new regressions: a returned error resolves and empties once its retained root's close is confirmed; a worker-plus-root error drops only the worker's resources when the worker's stop is confirmed, then resolves when the root is removed; a close aggregate tracks every unresolved owner until each retained root releases. - Base reproduction or other evidence: each new test fails on base revision 71ae02c without the provider change (the kept error stays `cleanupIncomplete` with the removed path still listed) and passes with it. - User-visible CLI/MCP output (if applicable): none. - Remaining proof gaps: macOS worker paths are exercised through the same seams but were not run on Darwin; two SQLite boundary failures on Linux (`cancels an acquired SQLite worker and removes its private snapshot`, `retains reply-read cancellation and retries the failed reply descriptor close`) reproduce identically with and without this change. 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 tests/boundary/sqlite/sqliteDatabaseCleanup.test.ts` — 8 passed (5 existing + 3 new). - Same file with the provider change reverted — the 3 new tests fail (RED), the 5 existing pass. - `npx vitest run tests/boundary/sqlite/` — 48 passed, 42 skipped, 2 failed; both failures reproduce identically on clean 71ae02c. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; Linux boundary suite only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new process launches, target interaction, or external writes; the change binds returned errors to cleanup owners the provider already retains. ## Author readiness Human owner: @charan-rathore - [ ] 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. - [ ] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. AI-assisted; Charan Rathore is the human driver. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-11-01 21:43:40 +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#6867
No description provided.