morluto/rea

Fork 0

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

[GH-ISSUE #5284] [Bug] SQLite provider replaces owned worker failures with late cancellation #3225

New issue

Closed

opened 2026-10-31 00:38:02 +00:00 by bsakaguchi · 3 comments

bsakaguchi commented

2026-10-31 00:38:02 +00:00

Owner

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

Before submitting

Searched open and closed issues for SQLite cancellation and late physical-failure replacement. #4227 is the broad lifecycle audit; no SQLite-specific duplicate was found.
No credentials, private target paths, or sensitive data included.

Area

Process capture or runtime observation

Observed behavior

At source revision e84e17e30, SqliteDatabaseProvider.inspect can return AnalysisCancelledError when its owned worker has already exited with a physical failure and the caller signal is aborted during final command cleanup. The cancellation result omits the worker's exit status and diagnostic output, despite the command owner having recorded them.

Reproduction

On Node 25.1.0 / macOS 25.3.0 arm64, add a focused provider-boundary case that:

Creates a small SQLite fixture and calls SqliteDatabaseProvider.inspect with an AbortController signal.
Injects a launcher that starts an owned Node child writing authored SQLite worker failure to stderr and exiting 7.
Wraps the child's actual owned cleanup and aborts the caller signal with a selected object after cleanup completes.

The new regression failed once on the unmodified source: it received AnalysisCancelledError with no diagnostics. The physical control expected a provider error with failure_kind: process, exit_code: 7, and the child's stderr. After the local fix, the same focused test passed.

Expected behavior

Keep the owned command's tagged physical result primary. The caller's late abort reason must not erase the process failure or its captured diagnostics. This follows the lifecycle rule to preserve existing primary failures across cancellation during finalization in docs/testing.md#lifecycle-regressions.

Impact and scope

Confirmed at the SQLite provider boundary with a controlled owned child. The caller receives a cancellation classification instead of a worker failure and loses its exit/output diagnostics. Frequency with ordinary SQLite worker failures and effect on public CLI/MCP responses: NOT_ENOUGH_INFO; those workflows and other platforms were not tested.

REA version

Source revision e84e17e30f7de41ad9ac801eda235574fa5d4dad (package 6.7.0).

Runtime and platform

Node 25.1.0; macOS 25.3.0 arm64.

Target and artifact context

A temporary local SQLite fixture; the failing child was a controlled Node process, not the SQLite worker or an external database.

Interface

Other

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

Baseline focused run:

npm run test:focused -- tests/boundary/sqlite/sqliteDatabaseProvider.test.ts

The added case failed with AnalysisCancelledError and diagnostics: undefined; the child's exit 7 and stderr were not projected. The same test command after the local fix passed: 9 passed, 14 skipped (the existing native-limits-gated cases).

Source at the reproduced revision: SqliteDatabaseProvider.ts first projects the caught owner failure, then selectedFailure rereads the signal and replaces any non-cancellation error (lines 371-383). SqliteDatabaseFailures.ts already maps OwnedCommandFailure by its tagged reason.

Typed outcome or recovery expected

A provider failure retaining exit status and captured output; cancellation should be returned only when the owner reports cancellation.

Analysis or suspected cause

The provider's selectedFailure helper reinterprets an already-projected failure using ambient signal state. A narrow local fix removes that resampling and lets the command/artifact owner supply cancellation facts. The source and regression test are currently modified in the local checkout; they are uncommitted.

Originally created by @morluto on GitHub (Oct 19, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/5284 ### Before submitting - [x] Searched open and closed issues for SQLite cancellation and late physical-failure replacement. #4227 is the broad lifecycle audit; no SQLite-specific duplicate was found. - [x] No credentials, private target paths, or sensitive data included. ### Area Process capture or runtime observation ### Observed behavior At source revision `e84e17e30`, `SqliteDatabaseProvider.inspect` can return `AnalysisCancelledError` when its owned worker has already exited with a physical failure and the caller signal is aborted during final command cleanup. The cancellation result omits the worker's exit status and diagnostic output, despite the command owner having recorded them. ### Reproduction On Node 25.1.0 / macOS 25.3.0 arm64, add a focused provider-boundary case that: 1. Creates a small SQLite fixture and calls `SqliteDatabaseProvider.inspect` with an `AbortController` signal. 2. Injects a launcher that starts an owned Node child writing `authored SQLite worker failure` to stderr and exiting 7. 3. Wraps the child's actual owned cleanup and aborts the caller signal with a selected object after cleanup completes. The new regression failed once on the unmodified source: it received `AnalysisCancelledError` with no diagnostics. The physical control expected a provider error with `failure_kind: process`, `exit_code: 7`, and the child's stderr. After the local fix, the same focused test passed. ### Expected behavior Keep the owned command's tagged physical result primary. The caller's late abort reason must not erase the process failure or its captured diagnostics. This follows the lifecycle rule to preserve existing primary failures across cancellation during finalization in [`docs/testing.md#lifecycle-regressions`](https://github.com/morluto/rea/blob/e84e17e30f7de41ad9ac801eda235574fa5d4dad/docs/testing.md#lifecycle-regressions). ### Impact and scope Confirmed at the SQLite provider boundary with a controlled owned child. The caller receives a cancellation classification instead of a worker failure and loses its exit/output diagnostics. Frequency with ordinary SQLite worker failures and effect on public CLI/MCP responses: NOT_ENOUGH_INFO; those workflows and other platforms were not tested. ### REA version Source revision `e84e17e30f7de41ad9ac801eda235574fa5d4dad` (package 6.7.0). ### Runtime and platform Node 25.1.0; macOS 25.3.0 arm64. ### Target and artifact context A temporary local SQLite fixture; the failing child was a controlled Node process, not the SQLite worker or an external database. ### Interface Other ### Earliest failure stage Cleanup, cancellation, or lifecycle ### Relevant output or evidence Baseline focused run: ```text npm run test:focused -- tests/boundary/sqlite/sqliteDatabaseProvider.test.ts ``` The added case failed with `AnalysisCancelledError` and `diagnostics: undefined`; the child's exit 7 and stderr were not projected. The same test command after the local fix passed: 9 passed, 14 skipped (the existing native-limits-gated cases). Source at the reproduced revision: [`SqliteDatabaseProvider.ts`](https://github.com/morluto/rea/blob/e84e17e30f7de41ad9ac801eda235574fa5d4dad/src/sqlite/SqliteDatabaseProvider.ts#L298-L305) first projects the caught owner failure, then `selectedFailure` rereads the signal and replaces any non-cancellation error ([lines 371-383](https://github.com/morluto/rea/blob/e84e17e30f7de41ad9ac801eda235574fa5d4dad/src/sqlite/SqliteDatabaseProvider.ts#L371-L383)). `SqliteDatabaseFailures.ts` already maps `OwnedCommandFailure` by its tagged reason. ### Typed outcome or recovery expected A provider failure retaining exit status and captured output; cancellation should be returned only when the owner reports cancellation. ### Analysis or suspected cause The provider's `selectedFailure` helper reinterprets an already-projected failure using ambient signal state. A narrow local fix removes that resampling and lets the command/artifact owner supply cancellation facts. The source and regression test are currently modified in the local checkout; they are uncommitted.

bsakaguchi

2026-10-31 00:38:02 +00:00

closed this issue
added the
triage

bug
labels

bsakaguchi commented

2026-10-31 00:38:03 +00:00

Author

Owner

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

Follow-up source-owner review also fixed exact-reason losses at SQLite caller admission / post-retry checkpoints and snapshot read entry/final verification. The snapshot producer now emits a typed ArtifactReaderFailure("cancelled") with the exact signal reason; provider-close-only cancellation remains reason-unknown. OwnedCommandFailure.cancellation.reason is carried to the projected AnalysisCancelledError, while its tagged physical failure and cleanup owner remain separately represented.

The late successful-inspection case is a distinct trigger and is tracked in #5286: cancellation from root.close() after worker success previously returned ok: true. The provider now checks after successful final cleanup and returns typed cancellation with exact reason and completed partial Evidence. Physical failure and incomplete cleanup remain primary.

Executed after these changes: provider, cleanup, descriptor cleanup, snapshot boundary, and snapshot limits focused suites; 51 passed, 16 skipped. Logs: .cache/codebase-audit/sqlite5284-red.log (baseline failures) and .cache/codebase-audit/sqlite5284-green.log (green suite). No CLI/MCP or cross-platform claim.

@morluto commented on GitHub (Oct 19, 2026): Follow-up source-owner review also fixed exact-reason losses at SQLite caller admission / post-retry checkpoints and snapshot read entry/final verification. The snapshot producer now emits a typed `ArtifactReaderFailure("cancelled")` with the exact signal reason; provider-close-only cancellation remains reason-unknown. `OwnedCommandFailure.cancellation.reason` is carried to the projected `AnalysisCancelledError`, while its tagged physical failure and cleanup owner remain separately represented. The late successful-inspection case is a distinct trigger and is tracked in #5286: cancellation from `root.close()` after worker success previously returned `ok: true`. The provider now checks after successful final cleanup and returns typed cancellation with exact reason and completed partial Evidence. Physical failure and incomplete cleanup remain primary. Executed after these changes: provider, cleanup, descriptor cleanup, snapshot boundary, and snapshot limits focused suites; 51 passed, 16 skipped. Logs: `.cache/codebase-audit/sqlite5284-red.log` (baseline failures) and `.cache/codebase-audit/sqlite5284-green.log` (green suite). No CLI/MCP or cross-platform claim.

bsakaguchi commented

2026-10-31 00:38:03 +00:00

Author

Owner

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

Runtime attribution clarification: the original local baseline/full-suite logs used the ambient PATH runtime Node 25.1.0 with npm 11.8.0, not the repository-pinned toolchain. Baseline was rerun in an isolated worktree using pinned Node 24.18.0/npm 11.16.0 and the same five provider regressions failed. The full five-suite SQLite set was subsequently run on pinned Node 24.18.0/npm 11.16.0: 67/67 passed, no skips (.cache/codebase-audit/sqlite5284-pinned-final.log). After a directly related artifact-checkpoint cleanup, provider plus snapshot-limit tests passed 29/29 pinned (.cache/codebase-audit/sqlite5284-owner-check-node24.log). The earlier 51 passed/16 skipped count belongs to Node 25.1.0. Detailed log filenames were normalized to sqlite5284-baseline-node25-red.log, sqlite5284-baseline-node24-red.log, and sqlite5284-node25-green.log.

@morluto commented on GitHub (Oct 19, 2026): Runtime attribution clarification: the original local baseline/full-suite logs used the ambient PATH runtime Node 25.1.0 with npm 11.8.0, not the repository-pinned toolchain. Baseline was rerun in an isolated worktree using pinned Node 24.18.0/npm 11.16.0 and the same five provider regressions failed. The full five-suite SQLite set was subsequently run on pinned Node 24.18.0/npm 11.16.0: 67/67 passed, no skips (`.cache/codebase-audit/sqlite5284-pinned-final.log`). After a directly related artifact-checkpoint cleanup, provider plus snapshot-limit tests passed 29/29 pinned (`.cache/codebase-audit/sqlite5284-owner-check-node24.log`). The earlier 51 passed/16 skipped count belongs to Node 25.1.0. Detailed log filenames were normalized to `sqlite5284-baseline-node25-red.log`, `sqlite5284-baseline-node24-red.log`, and `sqlite5284-node25-green.log`.

bsakaguchi commented

2026-10-31 00:38:03 +00:00

Author

Owner

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

Closing as completed: verified during REA Bot open-bug audit that the described SQLite provider late-cancellation fix is already on current main with regression coverage. No further PR needed for this report.

@morluto commented on GitHub (Oct 20, 2026): Closing as completed: verified during REA Bot open-bug audit that the described SQLite provider late-cancellation fix is already on current main with regression coverage. No further PR needed for this report.

bsakaguchi referenced this issue

2026-10-31 00:40:20 +00:00

[GH-ISSUE #5654] [Tracker] Open PRs that can't merge yet: conflicts and red CI (October 2026) #3425

bsakaguchi referenced this issue

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

[PR #3225] [CLOSED] fix(managed): report InitLocals comparison changes #5655