Repository navigation
Commit d3d9cd4
feat(api): abort signal support for requesty (completePrompt + shared helpers) (#1537)
* feat(api): abort signal support for requesty (completePrompt + shared helpers)
* test(api): strengthen requesty abort assertions and share settle guard helper
* test(api): assert pre-aborted Requesty completePrompt skips model lookup (CodeRabbit finding)
* chore: re-trigger CodeRabbit review
* chore: re-trigger CodeRabbit re-review and label reconcile
* chore: trigger CodeRabbit re-review (findings already addressed)
* chore: trigger CodeRabbit re-review / label reconcile
* chore: trigger CodeRabbit re-review at current head (all findings already addressed; stale review commit needs a fresh pass)
* fix(abort): consume the pending rejection when the signal is already aborted
CodeRabbit finding at 2d12c01: rejectOnAbort returned early on an already-aborted signal without attaching a handler to pending, so a later rejection of the in-flight work surfaced as an unhandled rejection. The early return now consumes it; the test asserts no unhandled rejection is reported.
* Re-trigger review at the current head
Every review thread on this PR is resolved and CI is green at this head; the
review decision still points at an older commit. This empty commit re-runs the
review so the decision and the label reflect the current head.
* test(requesty): pin the provider abort message in the timeout test
The name assertion alone only proves that a TimeoutError did not reach handleOpenAIError; it does not pin the complete abort contract. Assert the provider-specific message the sibling cancellation tests use.
* test(requesty): pin ordinary SDK failures with a live signal/timeout
Review follow-up: an ordinary API error while a live external signal or per-request timeout is configured must stay a provider failure, not an AbortError. requesty.spec 36 passed.
* fix(requesty): forward a validated SDK timeout via the shared helpers
Lint follow-up: the per-request timeout is validated with hasUsableTimeout and rounded/capped with toSdkTimeoutMs instead of forwarding the raw value (SDKs treat 0 as an immediate abort and reject fractional/non-finite values). Added the two helpers to the branch abort-signal utils. requesty + abort-signal specs 69 passed; eslint 0; tsc clean.
* chore: drop the duplicated timeout helpers after the upstream merge
upstream/main already provides hasUsableTimeout/toSdkTimeoutMs/MAX_TIMEOUT_MS, so the branch-local copies are removed; requesty keeps using the shared helpers. requesty + abort-signal specs 80 passed.
* fix(merge): restore the upstream hunks dropped by the main merge
restore the upstream requesty.spec imports (providerIdentifiers, requestyDefaultModelInfo, getModels) alongside the branch abort tests
* fix(requesty): detach the SDK abort signal and race the completion
Two cancellation gaps in completePrompt:
1. The SDK signal was not detachable. mergeAbortSignalAndTimeout() returns the
caller's signal unchanged when timeoutMs is not usable, and openai@5.23.2
attaches its abort listener to whichever signal it receives and leaves it
attached after the request completes, so reusing one caller signal retained
one listener and one per-request controller per completed request. The caller
signal is now bridged with bridgeExternalAbortSignal, the SDK receives the
per-request signal, and dispose() runs in a finally covering model lookup and
completion alike - completion, failure, abort, and late result.
2. The completion was not raced against the cancellation scope. openai@5.23.2
does not observe the signal inside retryRequest: it sleeps for the backoff (or
a server retry-after-ms, up to about a minute) before issuing the next
attempt, so awaiting the SDK call alone could hold completePrompt open long
after the caller cancelled. The create() call now goes through rejectOnAbort,
the same shape the model lookup above already uses.
Tests (requesty.spec.ts, completePrompt):
- settles on abort while the SDK is sleeping between retries instead of waiting
for the completion
- detaches the abort listener from the caller's signal when the completion finishes
- detaches the abort listener from the caller's signal when the completion fails
- does not leave the SDK's abort listener on the caller's signal after a
completed request (the mock emulates the SDK leaving its listener attached)
"should pass abort signal through to client" now asserts the contract the bridge
provides - the SDK receives a per-request signal that tracks the caller's abort -
instead of identity with the caller's own signal.
Negative controls (each restored byte-identically afterwards):
- dropping the race fails exactly the retry-sleep test:
"expected Error: settle guard timed out after 500ms to match object { name: 'AbortError', ... (1) }".
- dropping the bridge fails the pass-through test ("expected AbortSignal { aborted: false }
not to be AbortSignal { aborted: false }") and the leak test ("expected true to be false").
- keeping the bridge but emptying the finally fails the two detach tests
("expected \"removeEventListener\" to be called with arguments: [ 'abort', [Function onExternalAbort] ]")
plus the leak test.
- requesty.spec.ts 43/43 green; src tsc --noEmit, eslint --max-warnings=0 and
prettier --check clean; no new suppression, no new "as any", no Stryker directive.
* test(requesty): pin the late-completion discard and the empty-content fallback
Rerunning the unit mutation preflight after the completePrompt fix left three
mutants on lines the rewrite touches, none of them reachable by the previous
tests:
- ConditionalExpression at requesty.ts:333 (the post-completion abort check,
replaced by false) survived because the new race settles at the abort, so no
test still reached that check. "discards a completion that resolves in the same
turn the caller aborts" makes the SDK win the race outright - it returns an
already-resolved response and aborts from a handler queued one step ahead of the
provider's continuation - and asserts the value is still discarded. Control:
deleting the check fails exactly that test with
"promise resolved \"'late'\" instead of rejecting".
- StringLiteral at requesty.ts:337 (the || "" fallback) was NoCoverage.
- OptionalChaining at requesty.ts:337 (choices[0]?.message) survived.
"returns an empty string when the completion carries an empty message content /
no choices at all" covers both: the string mutant fails both cases with
"expected 'Stryker was here!' to be ''", the optional-chain mutant fails the
no-choices case with "Cannot read properties of undefined (reading 'message')".
requesty.spec.ts 46/46 green; eslint --max-warnings=0, prettier --check and src
tsc --noEmit clean; no suppression and no Stryker directive added.
* test(requesty): pin timeout normalisation, timeout-covered lookup and the three cancel paths
Three review asks on this head, all in the spec so no production behaviour moved.
createOptions timeout: an it.each now asserts what the SDK is actually handed for two
values that whole numbers cannot distinguish - 1500.5 must be ceiled to 1501 and 3e9 must be
clamped to 2147483647. Measured: with toSdkTimeoutMs replaced by a pass-through of
options.timeoutMs both cases fail (2 failed) and the received value is 1500.5, which is the
sentence that turns the coverage ask into a proof: dropping the conversion is observable.
Model lookup under timeoutMs: a new case runs completePrompt with { timeoutMs: 20 } and no
caller signal at all against a getModels that never settles, so the only cancellation source
is the timeout this method configures; it asserts the provider AbortError and that create was
never reached. The control that drives the lookup race from bridgedCallerSignal instead was
attempted twice and the vitest process was killed both times, so that half is reported as
unmeasured rather than claimed.
Detach lifecycle: the resolve test and the failure test are folded into one it.each with a
third case that aborts mid-flight. The fold asserts nothing new about detaching for the two
original outcomes; the new ground is that dispose also runs on the cancellation exit, which
neither original pinned.
The comment that said it mirrored the Requesty test inside the Requesty spec no longer claims
a sibling: git grep for withSettleGuard/notifyCreateStarted across src/api/providers/__tests__
returns matches only in this file (28), so there was no other provider test to name and the
comparison was dropped rather than re-pointed.
Verified: tsc -p tsconfig.json --noEmit clean; spec 50 passed (was 45 - the fold nets one
extra case and the timeout it.each adds two, the lookup case one); prettier --check clean;
eslint over the touched file clean after formatting; pre-commit hook run by hand before the
commit. src/eslint-suppressions.json untouched, no changeset, no CHANGELOG.
* test(lint): flag bare abort substring in toThrow assertions
* fix(eslint): tighten abort assertion guards and test lane coverage
* fix(requesty): pass the request signal to model lookup, widen abort lint
* test(opencode-go,requesty): pin the abort assertions the new lint rule exposed
The compile arm failed on three lint errors after main was absorbed (merge commit
801a672, 602 files, no conflicts). Two of them sit in
src/api/providers/__tests__/opencode-go.spec.ts, a file this pull request never
touched: the branch adds abortSubstringSelector to the spec lint configs, and main's
already-merged assertions then fail that new rule on the merge ref. A head-only lint
run reports the same two rows one line earlier, at 1134 and 1485, so the failure is
the merge-tree class a head-only run cannot see. In the merged tree eslint reports
exactly the arm's three rows - 1135:10, 1486:10 and 964:36 - and exits 1.
opencode-go.spec.ts:1137 - the double is now one object shared by the two listeners
that race to reject the next() promise, and the test asserts rejects.toBe(abortDouble).
The stream path propagates the SDK-level rejection unchanged, so identity is the
strongest true statement; asserting an AbortError name and a provider message there
would assert a contract this handler does not have.
opencode-go.spec.ts:1495 - the Responses completion path wraps every failure
(opencode-go.ts:439 throws "Opencode Go completion error: <message>"), so the row
pins name "Error" and that whole message instead of a bare substring that a raw SDK
error would match too.
requesty.spec.ts:966 - the fixed sleep becomes the shared flushMacrotasks() helper
(src/test-utils/timers.ts): a rejection that ends up handled has no observable
condition to poll, which is what that helper is for, and setTimeout is what the rule
bans in specs.
Controls in this repo, on the merged tree: eslint over src exits 1 with those three
errors before the change and 0 after; the two spec files report 177 passed after it;
and mutating the two pinned details - asserting a fresh but equal error object, and
dropping the provider prefix from the message - fails exactly those two tests
("expected Error: request aborted to be Error: request aborted", and the mismatch on
{ name: 'Error', message: 'Opencode Go completion error: request aborted' }) and
nothing else. Test-only: no implementation file is touched, eslint-suppressions
counts are unchanged for every entry (opencode-go.spec.ts stays 3).
* fix(config-eslint): flag an escaped dot or anchor in a bare abort assertion
The row on this pull request's own rule says a regex with an escaped dot slips past
abortSubstringSelector: /^Request was aborted\.$/ is not flagged while the same message
written as a string is. Measured against the rule as it stood, through the extension's
real config (calculateConfigForFile on core/task/__tests__/example.spec.ts, then
linter.verify):
toThrow(/^aborted$/) 1 (already flagged)
toThrow(/^Request was aborted\.$/) 0 <- the hole
toThrowError(/request aborted\.$/i) 0 <- the hole
toThrow(/^request aborted\$/) 0 <- same hole, escaped anchor
toThrow(/^Request was aborted because the peer …\.$/) 0 (a full message stays allowed)
The tail [.$]?$ cannot consume the backslash that an escaped dot or anchor leaves in the
pattern text, so the match dies one character short. The tail becomes [\\.$]*$ in both
branches of the selector (the string-value branch and the regex-pattern branch), which
consumes \\., \$, . or $ before the end and nothing else.
Red before, green after, in packages/config-eslint/api-provider-hygiene.test.js: the four
new rows fail with 'expected: 1' against the old tail (# tests 89, # fail 2 - the loop runs
for two lane paths) and pass against the new one (# tests 89, # pass 89, # fail 0). After
the change the probe reports 13 of 13 cases as wanted, including toThrow(/aborted\./) -> 1,
rejects.not.toThrow(/^Request was aborted\.$/) -> 0 and the long-message cases -> 0.
No spec in the repository is newly caught: eslint over src (with main merged at
a326612) still exits 0 with --max-warnings=0, root pnpm run lint is 12/12, and
pnpm lint in packages/config-eslint exits 0. No eslint-suppression entry changes
(pnpm exec eslint --prune-suppressions on both files leaves src/eslint-suppressions.json
byte-identical). Rule source and its test are the only files touched.
* config-eslint,api: close the escaped-group hole and reach the ignored workflow tests
abortSubstringSelector consumed escaped dots and anchors but stopped at an inline
non-capturing group, so toThrow(/^Request was aborted\.$/) and toThrow(/abort(?:ed)?/i)
fell through the rule while the same message written as a string was flagged. The pattern
branch now consumes a (?:ed|ion|al)? group verbatim, and the four workflow tests that are
exempt from the path-traversal selectors get the abort rule back instead of losing the
whole block.
Assertions tightened where they were blind: Requesty's normalised completion message
instead of a substring that the unwrapped SDK error also satisfies, the model-lookup
rejection by object identity, the ask abort message by task and instance id, and a case
where only the per-request timeout may abort while the caller signal stays live.
Controls, each run and reverted: rule suite 90/90 with the new tail, 89/90 with the old
one and with the workflow mapping removed; requesty spec fails on the exact message when
handleOpenAIError is bypassed at both catch sites, fails on Object.is when the lookup
assertion takes a copy, and times out in the settle guard at timeoutMs 100_000; the ask
assertion fails against foreign task ids. eslint over src is 0 findings before and after,
the repo-wide abort-rule count stays 11, the four workflow files score 0 under the
selector alone, and the mutation selection is 46 executable lines against main tip / 0
packages against head^1 (cap 500).
* test(api): prove the Requesty lookup timeout aborts with a live caller signal
The bot row asked for the pending model-lookup case with both options present. The existing lookup-deadline case supplies no caller signal, so nothing proved the deadline still ends a lookup when a caller signal is live and that the caller signal is not the thing that changed. New case asserts the provider AbortError, controller.signal.aborted false, and that no completion was issued. Control: dropping the merged timeout whenever a caller signal exists turns this case and the completion-timeout case red (2 failed / 52 passed) while the no-caller-signal lookup case stays green; flipping the caller-signal assertion gives expected false to be true // Object.is equality.
* fix: address CodeRabbit abort assertion findings
---------
Co-authored-by: Eason Liang <easonliang28@gmail.com>
Co-authored-by: easonLiangWorldedtech <eason.liang@worldedtech.com>
Co-authored-by: dsh-agent <dsh@local>
Co-authored-by: lead <lead@local>
Co-authored-by: Elliott de Launay <edelauna@gmail.com>
Co-authored-by: f0-1528 <f0-1528@local>
Co-authored-by: easonLiangWorldedtech <eason.liang@worlded.tech>1 parent ba105e6 commit d3d9cd4
8 files changed
Lines changed: 1020 additions & 54 deletions
File tree
- packages/config-eslint
- src
- api/providers
- __tests__
- core/task/__tests__
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
63 | 63 | | |
64 | 64 | | |
65 | 65 | | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
66 | 76 | | |
67 | 77 | | |
68 | 78 | | |
69 | 79 | | |
70 | 80 | | |
71 | 81 | | |
72 | 82 | | |
| 83 | + | |
73 | 84 | | |
74 | 85 | | |
75 | 86 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
| 3 | + | |
3 | 4 | | |
4 | | - | |
| 5 | + | |
5 | 6 | | |
6 | 7 | | |
7 | 8 | | |
| |||
173 | 174 | | |
174 | 175 | | |
175 | 176 | | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
176 | 208 | | |
177 | 209 | | |
178 | 210 | | |
| |||
245 | 277 | | |
246 | 278 | | |
247 | 279 | | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
| 314 | + | |
| 315 | + | |
| 316 | + | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
| 322 | + | |
| 323 | + | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
| 332 | + | |
| 333 | + | |
| 334 | + | |
| 335 | + | |
| 336 | + | |
| 337 | + | |
| 338 | + | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
| 342 | + | |
| 343 | + | |
| 344 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1100 | 1100 | | |
1101 | 1101 | | |
1102 | 1102 | | |
| 1103 | + | |
| 1104 | + | |
| 1105 | + | |
| 1106 | + | |
| 1107 | + | |
| 1108 | + | |
1103 | 1109 | | |
1104 | 1110 | | |
1105 | 1111 | | |
1106 | 1112 | | |
1107 | 1113 | | |
1108 | 1114 | | |
1109 | | - | |
| 1115 | + | |
1110 | 1116 | | |
1111 | 1117 | | |
1112 | 1118 | | |
| |||
1117 | 1123 | | |
1118 | 1124 | | |
1119 | 1125 | | |
1120 | | - | |
| 1126 | + | |
1121 | 1127 | | |
1122 | 1128 | | |
1123 | 1129 | | |
| |||
1132 | 1138 | | |
1133 | 1139 | | |
1134 | 1140 | | |
1135 | | - | |
| 1141 | + | |
1136 | 1142 | | |
1137 | 1143 | | |
1138 | 1144 | | |
| |||
1473 | 1479 | | |
1474 | 1480 | | |
1475 | 1481 | | |
| 1482 | + | |
| 1483 | + | |
| 1484 | + | |
| 1485 | + | |
| 1486 | + | |
| 1487 | + | |
1476 | 1488 | | |
1477 | | - | |
| 1489 | + | |
1478 | 1490 | | |
1479 | 1491 | | |
1480 | 1492 | | |
| |||
1483 | 1495 | | |
1484 | 1496 | | |
1485 | 1497 | | |
1486 | | - | |
| 1498 | + | |
| 1499 | + | |
| 1500 | + | |
| 1501 | + | |
1487 | 1502 | | |
1488 | 1503 | | |
1489 | 1504 | | |
| |||
0 commit comments