Skip to content

fix(claude-sdk): recover sessions after user interrupt - #3995

Open
SabhyaC26 wants to merge 1 commit into
mainfrom
codex/fix-claude-sdk-interrupt-poisoning
Open

fix(claude-sdk): recover sessions after user interrupt#3995
SabhyaC26 wants to merge 1 commit into
mainfrom
codex/fix-claude-sdk-interrupt-poisoning

Conversation

@SabhyaC26

Copy link
Copy Markdown
Contributor

Related issue

N/A

Summary

  • Keep intentional Claude SDK interrupt teardown from marking the session as crashed when the terminated CLI reports ProcessError.
  • Add a regression test proving a stopped turn can be followed by a successful turn in the same Omnigent session.

ELI5: Stop removes the active client on purpose. If that client then reports that it was terminated, Omnigent now recognizes the termination as part of Stop instead of treating the conversation as permanently broken.

Stop -> remove client -> CLI exits with SIGTERM -> do not set crash marker
                                                -> next turn creates a client -> succeeds

Test Plan

  • python -m pytest tests/inner/test_claude_sdk_executor.py (121 passed)
  • Ruff checks for both changed Python files passed.
  • Pyrefly passed.
  • Live E2E with a local Omnigent server and host in the Browser: started a sleep 30 command, clicked Interrupt, observed the Claude runner report ProcessError with exit code 143, then sent a follow-up in the same session and received SECOND_OK.

Demo

N/A

Type of change

  • Bug fix
  • Feature
  • UI / frontend change
  • Refactor / chore
  • Docs
  • Test / CI
  • Breaking change

Test coverage

  • Unit tests added / updated
  • Integration tests added / updated
  • E2E tests added / updated
  • Manual verification completed
  • Existing tests cover this change
  • Not applicable

Coverage notes

The regression test deterministically reproduces interrupt teardown followed by a process error and asserts that a second turn recovers. Manual verification exercised the same sequence through the live Browser UI against a local Omnigent server and host.

Changelog

Stopping a Claude SDK turn no longer prevents follow-up messages in the same session.

@github-actions github-actions Bot added the size/M Pull request size: M label Aug 3, 2026
interrupted_turn = asyncio.create_task(_consume_turn())
await asyncio.wait_for(response_started.wait(), timeout=5)
self.assertTrue(await executor.interrupt_session("session-a"))
await interrupted_turn
@omnigent-ci

omnigent-ci Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Missing visual demonstration

This PR fixes a backend flow that was previously broken/stuck — a Stop poisoned the session so no follow-up message could succeed — which is exactly the "backend bug that was stuck and is now fixed" case that warrants a before/after demonstration. The Demo section is N/A and no image/video was found. The Test Plan already describes the exact reproduction through the live Browser (start sleep 30 → Interrupt → observe exit 143 → follow-up returns SECOND_OK); please attach a short screen recording of that sequence so a reviewer can see recovery without checking out the branch. (The deterministic regression test partially mitigates this, so treat it as a request, not a hard blocker.)

Blocking issues

None. The change is correct and well-scoped:

  • On interrupt, interrupt_sessionclose_session_close_live_client pops session_key from self._clients before the in-flight turn's receive_response() raises the SIGTERM-induced ProcessError. Guarding the crash marker on session_key in self._clients therefore correctly distinguishes intentional teardown (client already removed → no crash mark) from a genuine mid-turn crash (client still present → crash mark set).
  • Genuine crashes are unaffected: the client is added in _get_or_create_client and is only removed via deliberate close/interrupt/cancel paths, so a real fault still leaves session_key in self._clients and still poisons the session as before.
  • The subsequent _close_live_client(session_key) call is a harmless no-op when the client was already popped.

Security vulnerabilities

None. No auth, deserialization, injection, or boundary changes; no dependency/lockfile changes.

Non-blocking notes

  • The interrupted turn still yields an ExecutorError (with the ProcessError stderr/diagnostics) to the caller after teardown — only the persistent crash marker is suppressed. That matches prior behavior, but if the runner surfaces that ExecutorError to the user it will read like a failure even though the Stop was intentional. Worth confirming the runner treats a post-interrupt ExecutorError as expected teardown rather than a hard error.
  • The test relies on ordering between interrupt_session popping the client and the turn's error propagating. It's deterministic here via the process_terminated event, but the guard's correctness depends on close_session always completing the pop before the boundary runs on the real path — which holds given interrupt_session awaits close_session before returning.

Summary

A minimal, correct fix that stops intentional interrupt teardown from permanently poisoning a Claude SDK session, backed by a solid deterministic regression test that reproduces "stop → ProcessError → recover in a fresh client." Logic and semantics check out and there are no security concerns. The only ask is a short screen recording of the interrupt-then-recover flow in the Browser to satisfy the visual-demonstration expectation for a previously-broken backend behavior.


Automated review by Polly · workflow run

@SabhyaC26
SabhyaC26 force-pushed the codex/fix-claude-sdk-interrupt-poisoning branch from e65225d to d43778a Compare August 11, 2026 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Pull request size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant