MemberJunction / MemberJunction/MJ
BaseLLM: absorb the duplicated per-driver cancellation plumbing (and add AIErrorType.Cancelled)
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 323
Description
Follow-up from wiring `ChatParams.cancellationToken` through all 19 LLM providers.
Every driver now forwards the abort signal to its SDK, but getting there required each one to copy the same three pieces. That duplication belongs in `BaseLLM`.
## What's duplicated across 12+ drivers
1. **`buildCancelledChatResult(startTime)`** — identical apart from the `context.provider` string.
2. **`isCancellationError(error, signal)`** — identical except the SDK-specific abort error class. A base version checking `signal.aborted` + `name === 'AbortError'`, with an overridable `isProviderAbortError(error)` hook, would cover everyone.
3. **The stream-cancellation dance** — a `streamCancelled` flag (or stashed token), a `resetStreamingState()` override, and an early-return in `finalizeStreamingResponse`.
With those in the base class, a driver's entire cancellation story collapses to "pass `{ signal }` to the SDK."
## `AIErrorType` has no `Cancelled` member
This is the root of an ugly workaround. `ErrorAnalyzer` classifies an abort as `Unknown`, which maps to **Transient / `canFailover: true`** — meaning a driver that simply let the abort error propagate would have the prompt runner **retry a request the user just cancelled**. Every driver therefore hand-builds `AIErrorInfo` with `errorType: 'Unknown'`, `severity: 'Fatal'`, `canFailover: false`, `providerErrorCode: 'request_cancelled'`.
Adding a first-class `'Cancelled'` member to `AIErrorType` (Fatal, non-failover by construction) removes the hand-rolled `AIErrorInfo` from every driver and makes the retry semantics correct by default rather than by convention.
## Per-request cancellation state
The stashed token / `streamCancelled` flag is **instance-level state on what may be a shared provider singleton**. That's tolerable today only because `_streamingState` already makes the same concurrency assumption — but both are latent bugs under concurrent streaming on one driver instance. A Core-level fix should carry cancellation state **per request**, not per instance, and ideally fix `_streamingState` the same way.
## Related
- The mid-stream error swallow in `handleStreamingChatCompletion` was fixed separately (genuine failures now surface instead of finalizing as truncated successes).
- #3064 (the prompt-timeout work that motivated all of this).
Contributor guide
Research direction
Start by tracing BaseLLM, AIErrorType, ErrorAnalyzer, ChatParams, and handleStreamingChatCompletion, then compare the cancellation paths in the 12+ drivers. Review the existing _streamingState and per-request cancellation behavior under concurrent streaming. Done means shared cancellation handling, a first-class Cancelled error with non-failover semantics, and no duplicated driver plumbing.
Written by the indexing model from the issue text.
Assessment
- Domain
- backend-api-design
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100