Repository navigation
feat(chat): guard the stream — idle deadline, pre-token retry, configurable ceiling - #152
Merged
Merged
Conversation
…urable ceiling Closes #150, the first half of #143. Three things the turn lifecycle could not do, all of them about a turn that goes wrong rather than about what a turn *is*. **A stream that goes quiet is cancelled.** Nothing noticed a provider that stopped producing while holding the connection open: the turn sat in `streaming` forever, the composer stayed locked, and a hung provider looked exactly like a slow one. There is a deadline now, reset by every event — reasoning deltas included, so a model that thinks for a long time is not cut off mid-thought. It *cancels* rather than settling, because whether a silent provider is worth another attempt is the retry policy's call, so `ChatExecution` learned to cancel without deciding anything. **A transient failure before the first token is retried.** Retrying is the wrong answer for a deterministic failure — same wait, same error, plus tokens — so only recognisably transient failures qualify (rate limit, 429, 5xx, overloaded, timeout, the transport's own errors), and anything unrecognised is reported as it is. Two more conditions: nothing the reader would see may have been sent, and the attempts are bounded with backoff. Structural events (`start`, `start-step`) do not count as content, which is what makes the retry reachable at all: verified against the pinned SDK, a second attempt's parts append to an empty message, so the reader sees one answer. **The output ceiling stops being a hard-coded constant.** `DEFAULT_MAX_TOKENS = 2048` was sent on every call and was not configurable anywhere. On a reasoning model the thinking tokens share that budget with the answer, so the reader saw `truncated` early even with a perfectly healthy stream. The connection now carries `maxOutputTokens`, and absent means no ceiling is sent at all — the model's own default applies. It is a per-connection field for the same reason the model id is: the right value belongs to the model, not to KnowNote. Settings shows Automatic or a custom value, chat connections only. Each attempt gets its own signal (`ChatExecution.beginAttempt`), so a retry after a timeout or a cancel does not inherit an aborted one, and the outcome a cancelled attempt reports is translated to `failed(timeout)` however the cancellation surfaced — a rejected read, or the SDK turning it into an `abort` chunk. Verified: `npm test` 336 pass (6 new in `test/retryPolicy.test.ts`; five in `test/chatStreamManager.test.ts` covering a retry that succeeds, a transient failure after content that is *not* retried, a deterministic failure that is not retried, bounded attempts, and a silent stream that is cancelled and fails as a timeout; plus the ceiling's two cases in `test/streamChat.test.ts`), `npm run typecheck`, `npm run check:design`, `npm run build`, `npm run build:unpack` + `smoke:packaged` (23 checks). Based on #142's branch: #149 is not merged yet, and this work is independent of it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #150, the first half of #143.
Three things the turn lifecycle could not do, all of them about a turn that goes wrong rather than about what a turn is.
A stream that goes quiet is cancelled
Nothing noticed a provider that stopped producing while holding the connection open: the turn sat in
streamingfor as long as the socket lived, the composer stayed locked, and a hung provider was indistinguishable from a slow one — the only way out was Stop.ChatExecutiongained a cancel that decides nothing about the turn.failed(timeout)however the cancellation surfaces — a rejected read, or the SDK turning the abort into anabortchunk. Both happen in practice, and the first version of the test only covered one of them.A transient failure before the first token is retried
Retrying is the wrong answer for a deterministic failure: same wait, same error, plus tokens spent. So the rule is narrow, and the default is not to retry:
ECONNRESET,socket hang up,fetch failed, …). Anything unrecognised, and anything like a policy refusal or a bad key, is reported as it is.start,start-step) do not count: verified against the pinned SDK, a second attempt's parts append to an empty message, so the reader still sees exactly one answer.Each attempt gets its own signal (
ChatExecution.beginAttempt), so a retry after a timeout or a cancel does not inherit an aborted one.The output ceiling stops being a hard-coded constant
DEFAULT_MAX_TOKENS = 2048was sent on every call and was not configurable anywhere. On a reasoning model the thinking tokens share that budget with the answer, so a reader using one sawtruncatedearly even with a perfectly healthy stream — the ceiling was the defect, not the stream.ModelConnectionnow carriesmaxOutputTokens, and absent means no ceiling is sent at all: the model's own default applies. It is a per-connection field for the same reason the model id is — the right number belongs to the model, not to KnowNote. Settings shows Automatic (default) or a custom value, for chat connections only.Verification
npm testnpm run typechecknpm run check:designnpm run buildnpm run build:unpack+smoke:packagednpx eslint <changed files>no-explicit-anywarnings are pre-existing lines invalidation.ts)New coverage:
test/retryPolicy.test.ts(what counts as transient, what does not, the attempt bound, the content rule, backoff) and five cases intest/chatStreamManager.test.ts— a retry that succeeds and delivers the second attempt's answer, a transient failure after content that is deliberately not retried, a deterministic failure that is not retried, bounded attempts against a provider that is down, and a silent stream that is cancelled and fails as a timeout while keeping the part that had arrived.test/streamChat.test.tspins both ceiling cases against the provider call args.What is left
#151 — Continue vs Retry, with sibling attempts and the
attempt_ofcolumn — is the last child of epic #138.