fix: scope model-switch/reset/error process kills to the invoking topic - #166
Merged
PleasePrompto merged 1 commit intoJul 12, 2026
Merged
Conversation
Model switch, the /new and /reset command handlers, and the session error/timeout/recovery flows killed every registered CLI process for the chat via kill_all(chat_id). In multi-topic groups this aborted other topics' in-flight turns and background task subprocesses (exit=143, "CLI returned empty output"), losing their results. Switch these call sites to kill_by_chat_topic(chat_id, topic_id) -- the primitive /stop already uses -- and teach kill_by_chat_topic to preserve processes whose lifecycle is owned elsewhere (task:, task_result:, ns: labels), matching the contract documented on Orchestrator.abort() (the /stop handler). Preserved processes stay registered so TaskHub.cancel() can still terminate them. kill_all() semantics are unchanged for /stop_all and shutdown teardown.
ryuhaneul
marked this pull request as ready for review
July 9, 2026 02:21
PleasePrompto
added a commit
that referenced
this pull request
Jul 12, 2026
Post-merge fixes threading the Slack transport through main's newer subsystems (#164 per-session effort, #157 append_system_prompt_files, #166 topic-scoped kills, #137/#138 task changes): - Delete fork-only .github/workflows/upstream-release-watch.yml - Remove dead _is_message_addressed() in messenger/slack/bot.py - Fix trailing comma in config.example.json Slack section (invalid JSON) - Drop fork-only "ductor-slack" brand strings from en/wizard.toml - Harden _normalized_transport_list() in __main__.py: honor a non-empty transports list whenever the primary is a member (primary-first, others preserved) instead of collapsing to [primary] on any ordering mismatch, so telegram+matrix multi-transport setups are never silently dropped; add regression tests - named.py create(): keep both reasoning_effort and key params (# noqa: PLR0913, matching the codebase convention e.g. tasks/registry.py) - Update #164 create() test stubs for the new key kwarg - mypy: guard the optional slack_bolt import behind TYPE_CHECKING/else to satisfy strict no-redef - uv lock: add slack-bolt/slack-sdk (also picks up pre-existing lock drift) - ruff format integration-touched files Note: orch._sessions.list_active_for_chat() in slack/bot.py follows the existing private-access convention (telegram/startup.py does the same); no public accessor exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PleasePrompto
added a commit
that referenced
this pull request
Jul 12, 2026
Reconcile #169 with main's #164 (per-session reasoning effort) and #166 (topic-scoped kills): - keep #166's kill_by_chat_topic(chat_id, topic_id) instead of #169's kill_all - keep #164's per-session effort resolution and sync_session_target(reasoning_effort=) - route the topic branch of switch_model through #169's resolve_session_target Review fixes for effort persistence, per-provider bucket staleness, and DRY follow in a subsequent commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PleasePrompto
added a commit
that referenced
this pull request
Jul 12, 2026
…olve_session_target Review fixes on top of the #169 merge: 1. Effort persistence (reconcile with #164): resolve_session_target now takes reasoning_effort and writes it onto the created/retargeted session, so a topic /model that also picks an effort survives to the next message. The topic branch of switch_model passes the resolved effort through. 2. Per-provider staleness: when a session is stale only because the TARGET provider bucket hit max_session_messages, reset ONLY that bucket and keep the other providers' history instead of wiping the whole shell. Session-wide staleness (idle timeout / daily reset) still replaces the full shell. 3. DRY: factor the freshness primitives (_within_message_limit, _time_fresh) shared by resolve_session and resolve_session_target; drop the provider- mutating _is_fresh_for_target hack. 4. switch_model: extract _persist_switch_target/_SwitchContext to stay within complexity limits (no lint suppressions); keep #166's topic-scoped kills. Tests: update the bucket-wipe assertions to per-bucket reset, add a genuine idle-stale whole-shell test, add the codex-fresh + claude-count-stale -> /model claude keeps codex case, and switch the topic mocks to kill_by_chat_topic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Six call sites kill every registered CLI process for the whole chat via
ProcessRegistry.kill_all(chat_id):switch_model(model selector) —/modelconfirmationcmd_reset/cmd_reset_current—/new,/resetcommand-registry handlersIn a multi-topic group every topic shares one
chat_id, so any of theseevents in one topic aborts other topics' in-flight turns and background
task subprocesses. The killed CLI exits 143 and surfaces as
CLI returned empty output (exit=143); a background task hit this way ismarked failed/cancelled and its (already billed) work is lost.
This also contradicts the contract documented on
Orchestrator.abort()(the
/stophandler): a topic-scoped stop deliberately leaves backgroundtasks and named sessions alone because they have their own management
surfaces (
/tasks,/sessions), yet task/named-session subprocesses areregistered under the same chat bucket and get swept by the chat-wide kills.
Reproduce: in a group with topics, start a background task from topic A,
then run
/model(pick a different model) in topic B → the task'ssubprocess dies mid-run.
Fix
kill_by_chat_topic(chat_id, topic_id)—the primitive
/stopalready uses. The callers all have the fullSessionKeyin hand; they now use it.kill_by_chat_topicto preserve processes whose lifecycle isowned elsewhere (
task:,task_result:,ns:labels), matching theOrchestrator.abort()contract. Preserved entries stay registeredso
TaskHub.cancel()→kill_for_task()can still terminate them.(
clear_topic_abort) instead of the chat-level one.kill_all()itself is unchanged:/stop_alland shutdown teardownstill sweep everything, tasks included.
Behavior notes:
topic_id=None: private chats, non-topic groups) behave asbefore, minus the protected labels.
/stopin a topic where only protected processes are running nowreports "nothing to stop" — the task keeps running and is cancellable
via
/tasks.Tests
kill_by_chat_topicpreserves protected labels, keeps them registered,and
kill_for_taskstill kills them; protected-only topics return 0and set no abort flag; cross-topic survival;
topic_id=Noneflat-chatbehavior;
kill_allstill sweeps protected labels.clear_topic_abort(chat, topic)./stopzero-kill UX pinned at the Telegram handler.envelope origin, named-session flows).
13 files changed, +310/−74. Rollback = revert (no config/schema impact).