Skip to content

feat(api): shared abort-signal cancellation-scope helpers - #1651

Open
easonLiangWorldedtech wants to merge 1 commit into
Zoo-Code-Org:mainfrom
easonLiangWorldedtech:feat/abort-r1-u1-abort-signal-util
Open

easonLiangWorldedtech wants to merge 1 commit into
Zoo-Code-Org:mainfrom
easonLiangWorldedtech:feat/abort-r1-u1-abort-signal-util

Conversation

@easonLiangWorldedtech

Copy link
Copy Markdown
Contributor

Shared-util piece of the abort-signal series (round 1): extends src/api/providers/utils/abort-signal.ts with the helpers the gateway providers use to run request phases inside a cancellation scope.

  • resolveModelWithAbort(fetchModel, abortSignal, providerName): runs model resolution inside a cancellation scope - a pre-aborted signal fast-fails before any catalog/SDK work, a rejectOnAbort race settles the lookup with the provider AbortError when the signal fires mid-resolution, and abort-flavored lookup failures are normalized; any other resolution failure propagates unchanged.
  • rejectOnAbort(pending, signal, providerName): the race primitive - rejects with createAbortError(providerName) when the signal fires before pending settles; the abort listener detaches when pending settles, so a task-scoped signal does not accumulate listeners.
  • createAbortError(providerName): fresh error satisfying the Task.ts abort contract (name === "AbortError", message ending in "aborted").
  • isRequestAborted(error, signal?): wider abort detection - an aborted signal, a DOM AbortError, the OpenAI/Anthropic SDK APIUserAbortError (name check), or the exact "Request was aborted." message. The name/message checks now require a real Error instance, so a plain object that merely looks like an abort propagates unchanged.
  • Removed throwIfAborted - dead code: nothing on main (or in the sibling abort PRs) calls it; it was exercised only by its own unit tests.

Tests: unit specs for all four helpers plus the new settle-guard test utility.

Series and unit

Unit 1/3 of the #1295 split (content source: 62f596c5d). Merge order: this unit first, then the opencode-go unit, then the unbound/vercel/zoo unit. No provider behavior changes in this unit.

Part of the abort-signal series (round 1). Builds on #674, #901, #1008. Addresses #404.

Review response (maintainer review of #1295)

  • The shared util keeps the existing surface (mergeAbortSignalAndTimeout, mergeAbortSignals untouched) plus the new helpers; throwIfAborted was removed as dead code and isRequestAborted was tightened with the instanceof Error requirement (kill-tested: plain-object abort look-alikes propagate unchanged).
  • The guard, the race, and the abort normalization that the gateway providers need now live once here as resolveModelWithAbort; each provider unit exercises it end-to-end (pre-aborted fast-fail, mid-resolution race, normalization, no-signal pass-through).

Evidence

  • vitest: 29/29 passing in the util suite (35 changed executable lines, all covered)
  • Local Stryker mutation gate (unit delta vs own base): 43 valid mutants (≤400), 41 killed, 2 directive-ignored (the rejectOnAbort settle-handler pair, with an event-fires-once equivalence proof), 0 Survived / 0 NoCoverage / 0 Timeout

…utils

Extend src/api/providers/utils/abort-signal.ts with the abort-signal
series helpers used by the gateway providers:

- isRequestAborted(error, signal): wider abort detection - an aborted
  signal, a DOM AbortError, the OpenAI/Anthropic SDK APIUserAbortError
  (name check), or the exact SDK abort message "Request was aborted." -
  trusting name/message only on real Error instances so a plain object
  that merely looks like an abort propagates unchanged
- createAbortError(providerName): fresh error satisfying the Task.ts
  abort contract (name "AbortError", message ending in "aborted")
- rejectOnAbort(pending, signal, providerName): settle a signal-less
  async phase (model discovery) on the provider AbortError when the
  signal fires first; the abort listener detaches when pending settles
- resolveModelWithAbort(fetchModel, signal, providerName): run model
  resolution inside a cancellation scope - entry fast-fail for a
  pre-aborted signal, the rejectOnAbort race while the lookup is
  pending, and normalization of abort-flavored lookup failures; any
  other resolution failure propagates unchanged

Includes direct unit tests for the resolveModelWithAbort cancellation
scope (pre-aborted fast-fail, no-signal pass-through, mid-resolution
race, abort normalization, non-abort propagation), the
isRequestAborted instanceof tightening tests, and the settle-guard
test utility.

Unit 1/3 of the Zoo-Code-Org#1295 split (content source: 62f596c5d).
Part of the abort-signal series (round 1). Builds on Zoo-Code-Org#674, Zoo-Code-Org#901, Zoo-Code-Org#1008.
Addresses Zoo-Code-Org#404.
@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 2fc2a52f-41f3-4c30-8603-ee4fa7b61cf1

📥 Commits

Reviewing files that changed from the base of the PR and between 9973630 and a3fb762.

📒 Files selected for processing (3)
  • src/api/providers/utils/__tests__/abort-signal.spec.ts
  • src/api/providers/utils/abort-signal.ts
  • src/test-utils/settle-guard.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (5)
Treat model, provider, MCP, path, command, and tool data as untrusted.

⚙️ CodeRabbit configuration file

Files:

  • src/api/providers/utils/abort-signal.ts
  • src/api/providers/utils/__tests__/abort-signal.spec.ts
Require regression coverage at the lowest valid harness with behavior-focused assertions, including relevant negative, error, false/unset, and boundary cases.

⚙️ CodeRabbit configuration file

Files:

  • src/api/providers/utils/__tests__/abort-signal.spec.ts
Check strict typing and exhaustive behavior across normal, boundary, error, cancellation, retry, and compatibility paths.

⚙️ CodeRabbit configuration file

Files:

  • src/test-utils/settle-guard.ts
  • src/api/providers/utils/abort-signal.ts
  • src/api/providers/utils/__tests__/abort-signal.spec.ts
Verify extension/webview contracts, cancellation and error propagation, VS Code lifecycle correctness, and behavior under retries and partial failure.

⚙️ CodeRabbit configuration file

Files:

  • src/test-utils/settle-guard.ts
  • src/api/providers/utils/abort-signal.ts
  • src/api/providers/utils/__tests__/abort-signal.spec.ts
Act as an adversarial second-opinion reviewer.

⚙️ CodeRabbit configuration file

Files:

  • src/test-utils/settle-guard.ts
  • src/api/providers/utils/abort-signal.ts
  • src/api/providers/utils/__tests__/abort-signal.spec.ts
🔇 Additional comments (3)
src/api/providers/utils/abort-signal.ts (1)

52-65: LGTM!

Also applies to: 80-146

src/api/providers/utils/__tests__/abort-signal.spec.ts (1)

6-195: LGTM!

Also applies to: 330-339

src/test-utils/settle-guard.ts (1)

1-26: LGTM!


📝 Summary

Summary by CodeRabbit

  • Bug Fixes

    • Improved cancellation handling for provider model requests, including requests aborted before or during loading.
    • Standardized abort errors and preserved other request failures.
    • Prevented non-error values that resemble abort errors from being incorrectly classified as cancellations.
  • Tests

    • Added comprehensive coverage for cancellation races, pre-aborted requests, error propagation, listener cleanup, and model lookup behavior.
    • Added safeguards so tests fail quickly when asynchronous operations do not settle.

Walkthrough

The provider utilities now support abort-aware promise and model resolution. Abort errors are normalized and classified only from Error instances. Tests cover abort races, listener cleanup, error propagation, and settlement timeouts.

Changes

Abort-aware resolution

Layer / File(s) Summary
Abort-aware provider resolution
src/api/providers/utils/abort-signal.ts
Replaces throwIfAborted with rejectOnAbort and resolveModelWithAbort. The utilities handle pre-aborted signals, abort races, listener cleanup, and abort-error normalization. isRequestAborted now requires an Error instance.
Abort resolution validation
src/api/providers/utils/__tests__/abort-signal.spec.ts, src/test-utils/settle-guard.ts
Tests cover settlement, abort races, lookup behavior, listener removal, error propagation, and stricter abort classification. withSettleGuard fails pending promises after a timeout.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Feature

Merge Risk: ⚪ Minimal · up to a3fb7

The new shared cancellation helpers are covered for their intended abort and settlement paths, with no current integration risk identified. The change is ready to merge.

🚥 Pre-merge checks | ✅ 7 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Regression Evidence ⚠️ Warning The new withSettleGuard helper has an uncovered timeout branch. Its setTimeout callback at src/test-utils/settle-guard.ts:12-14 is never exercised by a focused test. All current uses wrap promis… Add a focused test for withSettleGuard with a never-settling promise and a short timeout. Assert rejection with Error("settle guard timed out after ${ms}ms"). Keep coverage for the existing resolve and reject paths, and verify that sett…
✅ Passed checks (7 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Security Boundaries ✅ Passed No changed path meets a stated security failure condition. The diff adds cancellation and test utilities, tightens abort-error classification, and removes an unused helper. The new helpers only create…
Persistence Integrity ✅ Passed No changed persistence path exists. The PR changes only abort-signal helpers, their unit tests, and a test-only settlement guard. The reviewed head has no production callers of resolveModelWithAbort
Lifecycle Resource Cleanup ✅ Passed No concrete lifecycle leak is introduced by the changed paths. rejectOnAbort registers one abort listener with { once: true }, removes it when the pending promise resolves or rejects, and relies o…
Title check ✅ Passed The title clearly identifies the main change: shared API abort-signal cancellation helpers.
Description check ✅ Passed The description explains the helper behavior, removed code, scope, issue references, series context, test coverage, and mutation results. It does not reproduce the full checklist or template headings,…
Full details: Regression Evidence

Explanation

The new withSettleGuard helper has an uncovered timeout branch. Its setTimeout callback at src/test-utils/settle-guard.ts:12-14 is never exercised by a focused test. All current uses wrap promises that settle through success, rejection, or abort paths, so they do not verify that a never-settling promise rejects with the guard error. A regression that removes or breaks the timeout could therefore survive.

Resolution

Add a focused test for withSettleGuard with a never-settling promise and a short timeout. Assert rejection with Error("settle guard timed out after ${ms}ms"). Keep coverage for the existing resolve and reject paths, and verify that settlement clears the timer so a normal completion does not later trigger the guard.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Review status

Thanks for contributing. This comment tracks the review sequence and the next action.

Current step: Awaiting fresh human maintainer or CODEOWNER approval.

Automated review is complete for the latest commit but does not replace human approval.

Review-state labels are managed by this workflow; do not edit them manually.

@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/test-utils/settle-guard.ts 88.88% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@github-actions github-actions Bot added coderabbit-review-active Required CI passed; CodeRabbit review is active awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit labels Sep 16, 2026
@github-actions github-actions Bot added awaiting-maintainer CodeRabbit approved; waiting for a human maintainer and removed coderabbit-review-active Required CI passed; CodeRabbit review is active awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit labels Sep 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-maintainer CodeRabbit approved; waiting for a human maintainer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants