Skip to content

fix(explicit-user-abort): separate explicit user abort semantics#3776

Merged
icecrasher321 merged 3 commits intostagingfrom
fix/user-intent-cancel
Mar 26, 2026
Merged

fix(explicit-user-abort): separate explicit user abort semantics#3776
icecrasher321 merged 3 commits intostagingfrom
fix/user-intent-cancel

Conversation

@icecrasher321
Copy link
Collaborator

Summary

Explicit user abort signal must be distinguished to not kill reconnection chance.

Type of Change

  • Bug fix

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link

vercel bot commented Mar 26, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Mar 26, 2026 2:07am

Request Review

@cursor
Copy link

cursor bot commented Mar 26, 2026

PR Summary

Medium Risk
Touches chat streaming abort/reconnect paths and tool cancellation logic; mistakes could leave streams running, prevent reconnects, or fail to stop tool mutations when users explicitly cancel.

Overview
Separates explicit user stop from passive disconnect/abort across copilot streaming. Server-side chat streaming now tracks both a transport abortController and a new userStopController, propagating userStopSignal through the orchestrator/tool execution stack so only explicit stops trigger tool cancellation and mutation guards.

Improves client resiliency for interrupted streams. useChat refactors resume/reconnect into resumeOrFinalize() and adds exponential-backoff reconnect retries after network errors, while ensuring reconnection UI state (isReconnecting) is cleared on finalize.

Written by Cursor Bugbot for commit bc00454. Configure here.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 26, 2026

Greptile Summary

This PR separates "explicit user stop" semantics from passive transport-disconnect semantics by introducing a dedicated userStopController / userStopSignal signal that only fires when the user actively cancels a generation — not when the SSE connection drops. It also adds a 10-attempt exponential-backoff reconnect loop on the client for network errors, and extracts a resumeOrFinalize helper to deduplicate recovery logic.\n\nKey changes:\n- chat-streaming.ts: activeStreams now stores an ActiveStreamEntry { abortController, userStopController }. Both the explicit-abort HTTP endpoint and the Redis-polling abort path fire both controllers together; a plain transport disconnect only fires abortController.\n- tool-execution.ts / base-tool.ts: abortRequested() and assertServerToolNotAborted now check userStopSignal instead of abortSignal, so in-flight tool mutations survive a transport blip and remain available for reconnection.\n- use-chat.ts: New resumeOrFinalize callback and a retry loop (MAX_RECONNECT_ATTEMPTS = 10, base 1 s, max 30 s backoff) that attempts to re-attach to the buffered stream after a network error.\n- P1 bug: After all 10 reconnect attempts are exhausted, isReconnecting is never reset to false. finalize({ error: true }) does not call setIsReconnecting(false), so the UI will be stuck in a reconnecting spinner indefinitely after a full failure.\n- P2: The resumeOrFinalize call inside the reconnect loop does not forward abortController.signal, leaving fetchStreamBatch un-cancellable during reconnect attempts.

Confidence Score: 4/5

Safe to merge after fixing the isReconnecting state leak; the server-side signal separation is correct and well-scoped.

The core server-side change (separating userStopSignal from abortSignal) is logically sound and the propagation chain is complete. The P1 bug in the client-side reconnect loop (isReconnecting not reset on failure) is a visible UI regression but does not affect data integrity or server behavior. One targeted fix resolves it.

apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts — reconnect loop error path needs setIsReconnecting(false) before setError.

Important Files Changed

Filename Overview
apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts Adds resumeOrFinalize helper to deduplicate reconnect logic and introduces a 10-attempt exponential-backoff reconnect loop on network errors; has a P1 bug where isReconnecting is never reset to false after all attempts are exhausted, and a P2 issue where the abort signal is not forwarded in the reconnect loop.
apps/sim/lib/copilot/chat-streaming.ts Introduces ActiveStreamEntry with a separate userStopController, correctly wiring it through both the explicit-abort endpoint and the Redis-polling path so only user-initiated stops fire userStopSignal.
apps/sim/lib/copilot/orchestrator/sse/handlers/tool-execution.ts Changes abortRequested to check only userStopSignal (plus wasAborted), not abortSignal, so transport disconnects no longer halt tool execution — intentional for reconnection support.
apps/sim/lib/copilot/tools/server/base-tool.ts Switches assertServerToolNotAborted to check userStopSignal instead of abortSignal, preventing transport disconnects from blocking server-side tool mutations.
apps/sim/lib/copilot/orchestrator/types.ts Adds optional userStopSignal to both OrchestratorOptions and ExecutionContext with a clear JSDoc comment distinguishing its semantics.
apps/sim/lib/copilot/orchestrator/index.ts Threads userStopSignal from options into execContext, straightforward plumbing change.
apps/sim/lib/copilot/orchestrator/tool-executor/index.ts Forwards userStopSignal from ExecutionContext into ServerToolContext, completing the signal propagation chain.

Sequence Diagram

sequenceDiagram
    participant UI as Client (use-chat)
    participant Server as SSE Server
    participant Redis as Redis

    Note over UI,Server: Happy path — explicit user stop
    UI->>Server: POST /abort (streamId)
    Server->>Server: userStopController.abort()
    Server->>Server: abortController.abort()
    Server->>Server: assertServerToolNotAborted → throws (userStopSignal fired)
    Server-->>UI: stream closed

    Note over UI,Server: Transport disconnect → reconnect
    UI->>Server: fetch SSE stream
    Server-->>UI: connection drops (network error)
    Note over UI: catch block — NOT an AbortError
    UI->>UI: reconnect loop (attempt 1..10, exponential backoff)
    UI->>Server: fetchStreamBatch (replay buffered events)
    Server-->>UI: buffered events + status
    UI->>Server: attachToExistingStream (live tail)
    Server-->>UI: stream resumes
    Note over Server: abortController NOT fired → tools kept running
    Note over Server: userStopSignal NOT fired → assertServerToolNotAborted passes

    Note over UI,Redis: Redis-triggered abort (e.g. from another tab)
    Redis->>Server: set streamAbortKey
    Server->>Server: poll detects key
    Server->>Server: userStopController.abort()
    Server->>Server: abortController.abort()
    Server->>Redis: del streamAbortKey
Loading

Reviews (1): Last reviewed commit: "fix(explicit-user-abort): separate expli..." | Re-trigger Greptile

@icecrasher321
Copy link
Collaborator Author

bugbot run

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

@icecrasher321 icecrasher321 merged commit 794d5ea into staging Mar 26, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant