Skip to content

Feature/stream interrupt - #3185

Closed
tisnik wants to merge 5 commits into
mainfrom
feature/stream-interrupt
Closed

tisnik wants to merge 5 commits into
mainfrom
feature/stream-interrupt

Conversation

@tisnik

@tisnik tisnik commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Description

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library [pyproject.toml + uv.lock]
  • Bump-up dependent library [requirements.*.txt for Konflux]
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement
  • Benchmarks improvement

Tools used to create PR

Identify any AI code assistants used in this PR (for transparency and review context)

  • Assisted-by: (e.g., Claude, CodeRabbit, Ollama, etc., N/A if not used)
  • Generated by: (e.g., tool name and version; N/A if not used)

Related Tickets & Documents

  • Related Issue #
  • Closes #

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

max-svistunov and others added 5 commits October 23, 2026 15:20
register_interrupt_callback registered asyncio.current_task() in
StreamInterruptRegistry together with an on_interrupt closure that
persisted the interrupted turn, and returned a dual-persist guard. It
exists only for the ASGI task-cancel interrupt model, which the PAI
CancellationToken migration replaces (design sections 4.2, 4.3, 4.7).

Remove the function. generate_agent_response now starts with a local
persist_guard of [False]; its CancelledError handler, uncancel() and
the deregister_stream call in finally stay unchanged.

Tests: drop the unit test of the removed function, the patches that
stubbed it with [False], and the guard tests that simulated
on_interrupt persisting first (that scenario no longer exists). The
autouse stream_interrupt_mocks fixture no longer patches
get_stream_interrupt_registry: register_interrupt_callback was its
only caller, so the fixture now patches deregister_stream alone. The
turn persistence integration tests interrupt a stream by cancelling the
task that consumes it, as a client disconnect does, instead of through
the registry, so they keep checking that an interrupted turn is stored
once.

Nothing registers a stream any more, so on this feature branch the
interrupt endpoint returns 404 for live streams until LCORE-4733 mints
and registers a token per run. Removing persist_guard, uncancel() and
the CancelledError interrupt path is LCORE-4735.
StreamInterruptRegistry held the asyncio task serving the SSE response
and an interrupt cancelled that task. Per design sections 4.2-4.3 it
becomes an ownership and cancellation index: request_id -> owning
user + the agent run's CancellationToken (+ conversation_id).

ActiveStream holds user_id, token and conversation_id; register_stream
takes the token where the task was. Under the registry lock
cancel_stream returns NOT_FOUND, FORBIDDEN (warning log kept),
ALREADY_DONE when the token is already cancelled, or calls
token.cancel() and returns CANCELLED, so exactly one interrupt call
reports interrupted=true. No task cancel, callback, persistence or SSE.

Tests: a parametrized registry test covers all four outcomes, one of
them after deregister_stream; the endpoint unit tests and the
integration round trip use real tokens and assert token.cancelled.

Left to sibling stories: token per run (LCORE-4733), RunCancelled
finalize (LCORE-4734), CancelledError path removal (LCORE-4735),
endpoint wording (LCORE-4736). Live interrupt returns 404 until 4733.
…pt-cancellation-token

LCORE-4732: Cancel streams through a PAI CancellationToken in StreamInterruptRegistry
…errupt

LCORE-4731: merge main into feature/stream-interrupt
@tisnik tisnik closed this Nov 9, 2026
@coderabbitai

coderabbitai Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

Caution

Review failed

The pull request is closed.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@fullsend-ai-review

fullsend-ai-review Bot commented Nov 9, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ⏭️ Skipped (PR #3185 is already closed) · Started 1:26 PM UTC · Completed 1:26 PM UTC

Commit: c2c33ad · View workflow run →

Effort: high

@fullsend-ai-review

Copy link
Copy Markdown

Review skipped — this PR is already closed.

The /fs-review command only reviews open PRs/MRs.

Posted by fullsend pre-review check

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.

2 participants