feat: graceful stream cancellation on 403#513
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR ports Teams stream-cancellation handling to TypeScript by detecting “stream stopped” via HTTP 403 and short-circuiting future streaming work (no retries, no noisy logs), while ensuring the inbound request still returns 200 to prevent Teams retries.
Changes:
- Introduces
StreamCancelledErrorand adds acanceledflag toIStreamer. - Updates
HttpStreamto mark streams canceled on 403, blockemit/send/closewhen canceled, and suppress expected cancellation logs. - Updates
retry()and$process()to avoid retrying/handling cancellations as failures.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/apps/src/utils/promises/retry.ts | Skips retries by rethrowing StreamCancelledError immediately. |
| packages/apps/src/types/streamer.ts | Adds StreamCancelledError and extends IStreamer with canceled. |
| packages/apps/src/http/http-stream.ts | Detects 403 cancellations, sets _canceled, bails early in emit/close/send, and suppresses cancellation logging. |
| packages/apps/src/http/http-stream.spec.ts | Adds test coverage around 403-driven cancellation and post-cancel behavior. |
| packages/apps/src/app.process.ts | Treats StreamCancelledError as a successful (200) outcome so Teams won’t retry. |
Comments suppressed due to low confidence (1)
packages/apps/src/http/http-stream.ts:1
- Given the
while ((this.queue.length || !this.id) && !this._canceled)loop condition, control flow shouldn’t reach theif (!this.id)block unless the earlier timeout/cancel paths return (and cancel already returns just above). This makes theno stream id setwarning effectively unreachable/redundant; consider removing it or restructuring the loop/conditions so the warning is emitted on a reachable path.
import {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Detect when Teams stops a stream (user presses Stop or 2-minute timeout) via 403 response and cancel the stream immediately instead of silently retrying until timeout. - Add StreamCancelledError class and canceled property to IStreamer - HttpStream.send() catches 403 and sets canceled flag - emit() and close() bail out immediately when canceled - retry() skips retries for StreamCancelledError - flush() suppresses error log for expected cancellation - app.process catches StreamCancelledError and returns 200 Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
- Standardize on 'canceled' spelling and use default error message - Update StreamCancelledError docstring to cover both 403 and post-cancel cases - Add err.name fallback for instanceof checks in retry and process - Keep !this.id guard in close() for type narrowing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
545f427 to
10f8669
Compare
|
This change is part of the following stack: Change managed by git-spice. |
|
Reviewed this with a few folks who have streaming context. Looks good — clean design. Architecture trace works end-to-end: 403 in 403 assumption is safe. Mid-stream, auth has already succeeded (first Minor notes (non-blocking):
LGTM |
## Summary - Merges all changes from `main` since v2.0.7 into `release` - Sets `version.json` to stable `2.0.8` ### What's included - **feat:** Sovereign cloud support (GCCH, DoD, China) (#500) - **feat:** Add missing endpoints — Paged Members, Meeting Notifications & client gaps (#516) - **feat:** Graceful stream cancellation on 403 (#513) - **feat:** GitHub issue analysis → Teams notification workflow (#517) - **fix:** Improve error message when app credentials are missing (#527) - **fix:** Surface Graph API error body in GraphError (#524) - **fix:** Resolve missing deps, broken JSON, type errors, and typos in CLI templates (#521) - **fix:** Drain entire queue per flush cycle (#520) - **fix:** Merge User-Agent headers when cloning HTTP client (#508) - Dependency bumps: hono, axios, vite, @hono/node-server ## Post-merge 1. Trigger the [release pipeline](https://dev.azure.com/DomoreexpGithub/Github_Pipelines/_build?definitionId=52&_a=summary) for `release` with **Public** publish type 2. Bump `version.json` on `main` to `2.0.9-preview.{height}` 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Summary
StreamCancelledErrorclass andcanceledproperty toIStreamerinterfaceHttpStream.send()catches 403, setscanceledflag, and throwsStreamCancelledErroremit()andclose()bail out immediately when canceledretry()skips retries forStreamCancelledError(no point retrying a permanent cancellation)flush()suppresses error log for expected cancellationapp.processcatchesStreamCancelledErrorand returns 200 so Teams doesn't retryPorts teams.py#337 to TypeScript.
To test, I ran a streaming bot, press Stop mid-stream — verify no held connection, verify future messages weren't impacted, verified that stream didn't restart b/c of a 500.