morluto/rea

Fork 0

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

[PR #2710] [MERGED] fix(application): preserve a cancellation that races a completed provider run #5334

New issue

Closed

opened 2026-10-31 01:05:48 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:05:48 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2710
Author: @XiaoBinGan
Created: 10/13/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/provider-cancellation-after-observation

📝 Commits (2)

4fe3cd8 fix(application): preserve a cancellation that races a completed provider run
8007627 fix(application): retain completed Evidence on raced cancellation

📊 Changes

2 files changed (+50 additions, -4 deletions)

View changed files

📝 src/application/ProviderOperationExecution.ts (+16 -4)
📝 src/application/apktool/ApktoolResourceAnalysisService.test.ts (+34 -0)

📄 Description

executeProviderOperation honours a cancellation that arrives before the provider starts, but not one that arrives while the provider is running.

Summary

executeProviderOperation checks the caller's signal once, before dispatching to the provider:

if (options?.signal?.aborted === true)
  return err(new AnalysisCancelledError(operation));
…
const executed = await provider.execute(request.data, options);
if (!executed.ok) return executed;

A provider that observes the signal returns a cancellation failure of its own, and the second check correctly passes that through. But a provider that cannot observe the signal — one that finishes its work regardless — returns success, and the helper reports a completed observation to a caller that has already cancelled. In a cancellation contract, a successful result is not by itself proof that the caller still wants it; the signal has to be re-read after the await.

The repository already applies that rule at the sibling seam: EvidenceBundleFiles.ts checks the signal at line 60 before readJsonFile and again at line 76 after it returns. executeProviderOperation is the outlier.

Problem and expected behavior

For a provider whose execute completes successfully and aborts the caller's controller part-way through, the operation reports success:

current:  { ok: true,  value: <evidence> }
expected: { ok: false, error: AnalysisCancelledError }

The cancellation is delivered during the operation, so the operation must not report a completed observation.

Change and scope

ProviderOperationExecution.ts re-reads options?.signal?.aborted after the await and returns AnalysisCancelledError when it is set. The existing pre-dispatch check is kept, so a cancellation that arrives before any work is still rejected without calling the provider.

The abort read is spelled options?.signal?.aborted rather than === true, matching the sibling seam. Both are equivalent for AbortSignal#aborted, which is a boolean; no branch depends on the difference and the narrow === true form was not protecting anything here.

Both callers of the helper (ApktoolResourceAnalysisService, AdbDeviceAnalysisService, FlutterBuildAnalysisService, JebAnalysisService all share it) gain the same behaviour from the single change. No wrapper, flag or second helper is introduced.

Contract and boundary impact

Semantic owner and earliest changed stage: cancellation settlement in the shared provider-operation adapter, after the provider returns. The provider ports, their cleanup, the schema validation and the Evidence payload are untouched.
CLI and MCP/tool-catalog contract: no tool name, command, input field, output schema or error code changes. A cancelled operation now reports AnalysisCancelledError, which is the same error the pre-dispatch check and every signal-aware provider already produce; callers that never cancel are unaffected.
Provider, target-format or platform compatibility: none — the shared adapter is provider-neutral and platform-independent.
Evidence, artifact, provenance or reconstruction contract: no Evidence is created for a cancelled operation, which is the intended outcome. An operation that completes without cancellation produces byte-identical Evidence.
Process execution, authorization, cleanup or containment impact: the check adds no cleanup step and does not alter provider-owned cleanup. The error carries the same operation as the pre-dispatch check. A provider that still holds work after the abort settles it through its own existing path; the change only stops the result from being reported as wanted.
Generated metadata, package or installation impact: none.

Evidence and regression coverage

A new case in ApktoolResourceAnalysisService.test.ts, "preserves a cancellation that races a completed observation", uses a port whose execute aborts the caller's controller and then returns a successful execution, and asserts the service reports AnalysisCancelledError. The existing case that pins a cancellation before execution still passes, so the new check does not replace the old one.

I re-ran them myself on macOS 26.5.1 arm64 with Node.js 24.18.0 and npm 11.16.0, based on bb54082:

npm run test:focused -- src/application/apktool/ApktoolResourceAnalysisService.test.ts: 6 passed with the change.
Reverting only src/application/ProviderOperationExecution.ts to bb54082 fails exactly the new case with AssertionError: expected true to be false, with 1 failed and 5 passed.

Unavailable coverage: no Windows or Linux run, and no real apktool binary — the racing provider is a constructed port implementing the existing ApktoolResourceAnalysisPort. No external tool was launched.

_{🔄 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/2710 **Author:** [@XiaoBinGan](https://github.com/XiaoBinGan) **Created:** 10/13/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/provider-cancellation-after-observation` --- ### 📝 Commits (2) - [`4fe3cd8`](https://github.com/morluto/rea/commit/4fe3cd8209908e01b9bd6df701ddba6f5b6ccd1b) fix(application): preserve a cancellation that races a completed provider run - [`8007627`](https://github.com/morluto/rea/commit/80076277887072ab2aae9c0b510f9fbbfb9cf4c4) fix(application): retain completed Evidence on raced cancellation ### 📊 Changes **2 files changed** (+50 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `src/application/ProviderOperationExecution.ts` (+16 -4) 📝 `src/application/apktool/ApktoolResourceAnalysisService.test.ts` (+34 -0) </details> ### 📄 Description `executeProviderOperation` honours a cancellation that arrives before the provider starts, but not one that arrives while the provider is running. ## Summary [`executeProviderOperation`](https://github.com/morluto/rea/blob/bb54082/src/application/ProviderOperationExecution.ts) checks the caller's signal once, before dispatching to the provider: ```ts if (options?.signal?.aborted === true) return err(new AnalysisCancelledError(operation)); … const executed = await provider.execute(request.data, options); if (!executed.ok) return executed; ``` A provider that observes the signal returns a cancellation failure of its own, and the second check correctly passes that through. But a provider that cannot observe the signal — one that finishes its work regardless — returns success, and the helper reports a completed observation to a caller that has already cancelled. In a cancellation contract, a successful result is not by itself proof that the caller still wants it; the signal has to be re-read after the await. The repository already applies that rule at the sibling seam: [`EvidenceBundleFiles.ts`](https://github.com/morluto/rea/blob/bb54082/src/application/EvidenceBundleFiles.ts) checks the signal at line 60 before `readJsonFile` and again at line 76 after it returns. `executeProviderOperation` is the outlier. ## Problem and expected behavior For a provider whose `execute` completes successfully and aborts the caller's controller part-way through, the operation reports success: ``` current: { ok: true, value: <evidence> } expected: { ok: false, error: AnalysisCancelledError } ``` The cancellation is delivered during the operation, so the operation must not report a completed observation. ## Change and scope [`ProviderOperationExecution.ts`](https://github.com/XiaoBinGan/rea/blob/HEAD/src/application/ProviderOperationExecution.ts) re-reads `options?.signal?.aborted` after the await and returns `AnalysisCancelledError` when it is set. The existing pre-dispatch check is kept, so a cancellation that arrives before any work is still rejected without calling the provider. The abort read is spelled `options?.signal?.aborted` rather than `=== true`, matching the sibling seam. Both are equivalent for `AbortSignal#aborted`, which is a boolean; no branch depends on the difference and the narrow `=== true` form was not protecting anything here. Both callers of the helper (`ApktoolResourceAnalysisService`, `AdbDeviceAnalysisService`, `FlutterBuildAnalysisService`, `JebAnalysisService` all share it) gain the same behaviour from the single change. No wrapper, flag or second helper is introduced. ## Contract and boundary impact - **Semantic owner and earliest changed stage:** cancellation settlement in the shared provider-operation adapter, after the provider returns. The provider ports, their cleanup, the schema validation and the Evidence payload are untouched. - **CLI and MCP/tool-catalog contract:** no tool name, command, input field, output schema or error code changes. A cancelled operation now reports `AnalysisCancelledError`, which is the same error the pre-dispatch check and every signal-aware provider already produce; callers that never cancel are unaffected. - **Provider, target-format or platform compatibility:** none — the shared adapter is provider-neutral and platform-independent. - **Evidence, artifact, provenance or reconstruction contract:** no Evidence is created for a cancelled operation, which is the intended outcome. An operation that completes without cancellation produces byte-identical Evidence. - **Process execution, authorization, cleanup or containment impact:** the check adds no cleanup step and does not alter provider-owned cleanup. The error carries the same `operation` as the pre-dispatch check. A provider that still holds work after the abort settles it through its own existing path; the change only stops the result from being reported as wanted. - **Generated metadata, package or installation impact:** none. ## Evidence and regression coverage A new case in [`ApktoolResourceAnalysisService.test.ts`](https://github.com/XiaoBinGan/rea/blob/HEAD/src/application/apktool/ApktoolResourceAnalysisService.test.ts), "preserves a cancellation that races a completed observation", uses a port whose `execute` aborts the caller's controller and then returns a successful execution, and asserts the service reports `AnalysisCancelledError`. The existing case that pins a cancellation *before* execution still passes, so the new check does not replace the old one. I re-ran them myself on macOS 26.5.1 arm64 with Node.js 24.18.0 and npm 11.16.0, based on `bb54082`: - `npm run test:focused -- src/application/apktool/ApktoolResourceAnalysisService.test.ts`: 6 passed with the change. - Reverting only `src/application/ProviderOperationExecution.ts` to `bb54082` fails exactly the new case with `AssertionError: expected true to be false`, with 1 failed and 5 passed. Unavailable coverage: no Windows or Linux run, and no real `apktool` binary — the racing provider is a constructed port implementing the existing `ApktoolResourceAnalysisPort`. No external tool was launched. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:05:48 +00:00

closed this issue
added the
pull-request
label

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

No description provided.

Rows
Columns