Skip to content

fix(server): stop kills lingering Claude work - #5891

Merged
t3dotgg merged 4 commits into
mainfrom
t3code/fix-thread-kill-handling
Aug 23, 2026
Merged

fix(server): stop kills lingering Claude work#5891
t3dotgg merged 4 commits into
mainfrom
t3code/fix-thread-kill-handling

Conversation

@t3dotgg

@t3dotgg t3dotgg commented Aug 9, 2026

Copy link
Copy Markdown
Member

Claude's interrupt request could succeed while resumed background tasks kept the CLI process alive. Repeated Stop clicks left the thread working until the process was killed manually.

Claude Stop now closes the provider query before cleanup. The SDK escalates to SIGKILL if the CLI ignores graceful exit. Cleanup also keeps a replacement session safe when the next message resumes the thread.

Tests: vp test run apps/server/src/provider/Layers/ClaudeAdapter.test.ts

Made with GPT-5.6 in T3 Code through the Codex harness.


Note

Medium Risk
Changes core Claude session lifecycle and Stop semantics; close failures now fail operations instead of being logged and ignored, which can surface new errors during interrupt or session replacement.

Overview
Claude Stop now tears down the provider session instead of calling SDK interrupt() and per-task stopTask. interruptTurn routes through stopSessionInternal, which closes the query first (SDK can escalate to SIGKILL) and emits task.completed with status stopped for every live task.

Session teardown only removes the map entry and emits session.exited when the context is still the active one, so a replacement session started while slow cleanup runs is not wiped. getContextUsage is capped at 1 second so stop does not hang on a stuck provider call.

stopAll still tries every session and surfaces the first close() failure as ProviderAdapterProcessError while leaving failed sessions registered. Replacement startSession no longer swallows stop errors from the prior session. Tests cover close failures, multi-session stop, and resume-during-interrupt races.

Reviewed by Cursor Bugbot for commit e6f78be. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix lingering Claude work by closing provider sessions on stop and interrupt

  • interruptTurn and session replacement now close the Claude provider process via close() rather than issuing interrupt()/stopTask() SDK calls, which were leaving work running after a stop.
  • stopSessionInternal immediately closes the process, emits task.completed (stopped) for all live tasks, resolves pending approvals with cancel, and shuts down the prompt queue and stream fiber.
  • stopAll and the finalizer now attempt every session stop and surface aggregated failures instead of swallowing errors.
  • getContextUsage is wrapped in a 1-second timeout to prevent indefinite blocking during cleanup.
  • Risk: interruptTurn and startSession (replacement path) now fail with ProviderAdapterProcessError if the provider process close throws, instead of continuing silently.

Macroscope summarized e6f78be.

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 88771e71-63e6-41e5-9a56-bb38a15f1e44

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added size:L 100-499 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. labels Aug 9, 2026
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 11.3 KiB 11.3 KiB +33 B (+0.3%) 15.1 KiB
Codex Thread snapshot wire 5.5 KiB 5.5 KiB −2 B (−0.0%) 7.3 KiB
Codex Live turn WebSocket wire 5.9 KiB 5.9 KiB +35 B (+0.6%) 7.8 KiB
Codex Live turn WebSocket decoded 49.7 KiB 49.7 KiB 0 B (0.0%) 66.4 KiB
Codex Live turn messages 16 16 0 (0.0%) 21
Claude Total thread wire 11.3 KiB 11.3 KiB −21 B (−0.2%) 15.1 KiB
Claude Thread snapshot wire 5.5 KiB 5.5 KiB −1 B (−0.0%) 7.3 KiB
Claude Live turn WebSocket wire 5.9 KiB 5.8 KiB −20 B (−0.3%) 7.8 KiB
Claude Live turn WebSocket decoded 50.6 KiB 50.6 KiB 0 B (0.0%) 66.4 KiB
Claude Live turn messages 16 16 0 (0.0%) 21

Baseline: 83d769f · PR result: e6f78be · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 94.6 KiB
  • Claude decoded thread snapshot: 95.4 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

Comment thread apps/server/src/provider/Layers/ClaudeAdapter.ts
@macroscopeapp

macroscopeapp Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This PR changes the fundamental approach to stopping Claude sessions - replacing SDK-level interrupt/stopTask calls with process termination via close(). It also changes error propagation behavior in stopAll and session replacement. These are significant runtime behavior changes to session lifecycle management that warrant human review.

You can customize Macroscope's approvability policy. Learn more.

Comment thread apps/server/src/provider/Layers/ClaudeAdapter.ts

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 50906db. Configure here.

Comment thread apps/server/src/provider/Layers/ClaudeAdapter.ts
@t3dotgg
t3dotgg merged commit 4d12e52 into main Aug 23, 2026
18 checks passed
@t3dotgg
t3dotgg deleted the t3code/fix-thread-kill-handling branch August 23, 2026 07:02
github-actions Bot added a commit to omarcresp/t3code-flake that referenced this pull request Aug 23, 2026
## What's Changed
* fix(clients): reconnect after credentials fail during remote server updates by @t3dotgg in pingdotgg/t3code#7953
* feat(codex): submit thread feedback to OpenAI by @t3dotgg in pingdotgg/t3code#7949
* fix(server): stop kills lingering Claude work by @t3dotgg in pingdotgg/t3code#5891
* fix(ci): let Macroscope approve pull requests again by @t3dotgg in pingdotgg/t3code#7970


**Full Changelog**: pingdotgg/t3code@v0.0.34-nightly.20260823.1166...v0.0.34-nightly.20260823.1167

Upstream release: https://github.com/pingdotgg/t3code/releases/tag/v0.0.34-nightly.20260823.1167
pandec added a commit to pandec/t3code that referenced this pull request Aug 23, 2026
Fast-forwards main from 30be311 to f70eeee (6 commits, 47 files) and
merges it into dev. Small in commits, heavy in fork contact: 19 files
carried fork delta, giving 19 conflicts across 42 hunks.

Upstream lands Codex /feedback upload to OpenAI (pingdotgg#7949), a Claude Stop
rewrite that kills lingering work instead of only interrupting the turn
(pingdotgg#5891), settled pinned threads moving into the settled shelf (pingdotgg#7969),
and a credentials-failure reconnect during remote server updates (pingdotgg#7953).

Notable resolutions. Classification on both clients is now snoozed,
settled, pinned, Older, with the fork's Older grouping still last so it
never claims a lifecycle-owned row. ProviderService.test.ts and the mobile
send callback both had misaligned hunks and were reconstructed from each
side rather than resolved marker by marker; taking upstream there would
have dropped deliveryIntent and the bounded message window, both of which
compile. uploadFeedback takes the fork's per-thread lock. Claude's
session.exited keeps its sessionGenerationId and the usage-fiber
cancellation survives upstream's close-first teardown.

Two fixes a clean merge would not have caught: the fork's running composer
left Queue and Steer enabled while submitComposer refused them, and card
rows keep the passive pin marker because upstream's now-focusable
indicator hid itself inside the fork's focus-hide wrapper.

vp check, typecheck, and focused tests pass. Full suite green except the
documented PortScanner pair caused by a foreign dev server on [::1]:3000.

Reviewed and merged by Claude Opus 5 in Claude Code, with gpt-5.6-sol
subagents for the behavioral reviews and post-merge audits.
frankdavidcorona pushed a commit to frankdavidcorona/t3code that referenced this pull request Aug 23, 2026
cloudbridgeuy pushed a commit to cloudbridgeuy/t3code that referenced this pull request Aug 23, 2026
longtngo added a commit to longtngo/t3code that referenced this pull request Aug 24, 2026
Range 2274444..f035a0f. 80 files touched by both sides; 20 conflicted,
33 hunks. Four of upstream's commits landed in subsystems this fork has
rewritten, and three of the conflicts were git splicing two unrelated
blocks onto shared scaffolding.

Competing implementations, resolved toward a hybrid superset:

- pingdotgg#5891 (stop kills lingering Claude work) rewrote `interruptTurn` into a
  single hard `stopSessionInternal`. This fork's Stop is a two-rung client
  ladder (cooperative interrupt, then a deliberate second press escalating
  to `session.stop`), so collapsing the rungs would make that band
  vestigial for Claude and charge every stop-to-redirect a cold restart.
  Kept the fork's cooperative `interruptTurn` (bounded `stopTask` sweep +
  bounded `query.interrupt()`); took upstream's hardened
  `stopSessionInternal` (`close()` at the very top, `task.completed` sweep
  over `liveTaskIds`, `stopSessions` failure collection) and kept the
  fork's bounded `Fiber.interrupt` on top. Upstream removed
  `ClaudeQueryRuntime.interrupt`/`.stopTask` OUTSIDE any conflict marker;
  both restored. Upstream's four new tests retargeted to `stopSession`,
  not deleted.
- pingdotgg#7412 (recover from provider interrupt failures) adopted on top of the
  fork's live-session gate, which settles to `stopped` rather than
  erroring when nothing is live. Its three tests only projected a session,
  so each needed a `runtimeSessions.push` to reach the path it tests.
- pingdotgg#7969 (settled pinned threads) extracted the pin markup into
  `pinIndicator` byte-identically; took the extraction, kept
  `ThreadProviderChip`.
- pingdotgg#7949 (Codex feedback) added a send-abort condition; folded into the
  fork's `shouldAbortSendBeforeOfflineQueue` helper so every abort reason
  stays in one tested place, with a case for it.
- pingdotgg#8048 (attachment uploads) and pingdotgg#7999/pingdotgg#7971 merged additively; the
  offline-outbox branch stays ahead of the new feedback branch in
  `handleSend`, since it must run before anything mutates the draft.

Three conflicts were splices, resolved by reset-and-reapply rather than
hunk surgery: `ClaudeAdapter.test.ts` and
`ProviderCommandReactor.test.ts` (two unrelated tests sharing
`makeHarness()` scaffolding) and `http.ts` (two route layers sharing an
`Effect.gen` preamble; `attachmentUploadRouteLayer` re-added whole).

Invariants: all ten re-probed against the merged tree and holding.
Migrations 47 entries, ids unique and monotonic, max 48, 34 still burned;
patches now 16 files with exactly one fork-owned and 15 byte-identical to
upstream (upstream added `@legendapp/list`); `Sidebar.tsx` still the
default; the three MessagesTimeline helpers, the Collapsible wrapper, the
steering test and `serverRuntimeStartup.reconcile.test.ts` all still
absent; four `isSendBlocked=` sites; both footer panels inside
`SidebarUtilityMenu`; both project-row entry points. Registry gains
invariants 7-9 for the decisions above.

Sweeps: resurrected 1, dropped 11, fork-loss 17, every entry named. The
one resurrection is a text-level false positive (a bare `if (` the fork
restructured, matching upstream's new attachment block verbatim). Two
real findings the sweep caught that the gate could not: the fork's
deliberate deletion of `shouldShowBranchMismatchBanner` was re-imported
in two files by an additive resolution, and `ChatMarkdown.tsx` carried a
`markdown-file-link-tooltip-scroll` class that was never defined in any
stylesheet — a pre-existing fork defect, so the tooltip's thin scrollbar
had silently fallen back to the browser default. Restored upstream's
working inline utilities.

Gate (`pnpm verify`, the fork's only gate — CI never runs here): format
clean, typecheck 0 errors, lint 23 warnings and 0 errors (all
pre-existing), tests 9873 passed / 7 skipped / 0 failed across 14
projects. Three failures were fixed en route: the third pingdotgg#7412 test above,
an invalid `status: "starting"` in the runtime-session fixture it needed,
and upstream's `entrypoint.test.ts` symlink case, which cannot pass on
macOS because `os.tmpdir()` is itself a symlink that `realpathSync`
resolves along with the fixture's own link — proven with a live probe,
fixed in the fixture only, and worth sending upstream.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bcotrim pushed a commit to bcotrim/mognet that referenced this pull request Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant