Skip to content

fix(stop): scope /stop to the current forum topic when present#136

Open
ryuhaneul wants to merge 2 commits into
PleasePrompto:mainfrom
ryuhaneul:fix/stop-topic-scoped
Open

fix(stop): scope /stop to the current forum topic when present#136
ryuhaneul wants to merge 2 commits into
PleasePrompto:mainfrom
ryuhaneul:fix/stop-topic-scoped

Conversation

@ryuhaneul
Copy link
Copy Markdown
Contributor

@ryuhaneul ryuhaneul commented May 19, 2026

Background

When using ductor in a Telegram supergroup with multiple forum topics, I sometimes have parallel sessions running across different topics (e.g. one topic for research, another for design review). Typing /stop in one topic currently ends every active CLI process in the entire chat, plus every background task and named session — including the work happening in the other topics that I did not intend to stop.

It would be more convenient if /stop only stopped the in-flight response in the topic where it was sent, leaving the other topics' work and background machinery (which already have /tasks and /sessions for management) untouched. /stop_all continues to cover the chat-wide case.

Proposed change

When the inbound message carries a message_thread_id, scope /stop to that thread; otherwise keep the current chat-wide behavior so non-forum chats and legacy callers see no change.

Implementation:

  • ProcessRegistry.kill_by_chat_topic(chat_id, topic_id) — filter tracked processes by topic. kill_all is left as-is so /stop_all and shutdown paths keep their full sweep.
  • Orchestrator.abort(chat_id, topic_id=None) — when topic_id is provided, run only the topic-scoped kill; otherwise the existing chat-wide branch runs (background tasks + named sessions included, exactly as today).
  • handle_abort reads thread_id from the message and passes it as topic_id. handle_abort_all is untouched.

If you'd prefer a different shape (e.g. a separate /stop_topic command, or an opt-in config flag), happy to adjust.

Reinforcement (commit 12494ca)

Downstream review found a gap in the original commit: kill_by_chat_topic did not set an abort marker the way other kill paths do (kill_all_aborted, kill_by_label_aborted_labels). Without a marker, flows.py's was_aborted(chat_id) check returned False, the SIGKILL response was treated as an error, and _reset_on_error() ran chat-wide kill_all — defeating the topic scope.

Added a topic-aware abort marker:

  • ProcessRegistry._aborted_topics: set[tuple[int, int | None]] + was_aborted_topic + clear_topic_abort
  • kill_by_chat_topic now sets the marker (only when targets actually exist, so empty stops don't leak markers) and runs under _kill_lock for parity with kill_by_label
  • 5 sites in orchestrator/flows.py (_maybe_recover_session, normal, normal_streaming, named_session_flow, named_session_streaming) and 2 sites in cli/service.py (execute_streaming, _handle_stream_fallback) now also check was_aborted_topic
  • Orchestrator._handle_message_impl clears the topic marker on the next message entry (parallel to existing chat-wide clear_abort at core.py:328)

Topic abort marker has no persistent state; it lives until the next message in the same topic clears it.

Out of scope

Background tasks and drain_pending(chat_id) remain chat-wide in this PR — background tasks are not topic-tagged in the current model, and the pending queue drain has its own follow-up scope. Topic-aware versions can be added in follow-up issues if useful.

Test plan

  • 7 new unit tests covering kill_by_chat_topic marker behavior, Topic A kill not killing Topic B, _maybe_recover_session skipping recovery on topic abort, handle_abort propagating topic_id, and named_session_flow topic abort
  • Related suite (tests/cli/test_process_registry.py + tests/orchestrator/test_flows.py + tests/messenger/telegram/test_handlers.py + tests/cli/test_service.py) — 108 passed (no regression on existing PR tests)
  • ruff check, ruff format --check, mypy clean

In a Telegram supergroup with multiple forum topics, `/stop` ends every
active CLI process in the chat plus every background task and named
session.  Users running parallel work across topics see unrelated work
interrupted.

This change scopes `/stop` to the message thread when one is present;
when no thread is set the existing chat-wide branch runs unchanged so
non-forum chats and legacy callers see no behavior change.  `/stop_all`
is untouched.

- `ProcessRegistry.kill_by_chat_topic(chat_id, topic_id)` filters
  tracked processes by topic. `kill_all` is unchanged so `/stop_all`
  and shutdown paths keep their chat-wide sweep.
- `Orchestrator.abort(chat_id, topic_id=None)` runs the topic-scoped
  kill when `topic_id` is provided; otherwise the existing branch
  (foreground CLIs + background tasks + named sessions) runs.
- `handle_abort` reads `thread_id` from the message and passes it as
  `topic_id`. `handle_abort_all` is untouched.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ryuhaneul ryuhaneul marked this pull request as draft May 20, 2026 04:00
@ryuhaneul
Copy link
Copy Markdown
Contributor Author

Self-flag: potential abort-state issue uncovered during downstream review.

After scoping /stop to the current topic via kill_by_chat_topic(), the killed CLI processes are not marked as aborted. If a killed CLI returns a non-zero / error path back into the normal/streaming flow, that flow does not recognize the result as an explicit abort and falls into the error handler. _reset_on_error() then calls kill_all(chat_id), which tears down CLI processes belonging to other topics in the same chat — defeating the topic-scoped intent of this PR.

Concretely: a user issuing /stop in topic A could still indirectly kill the CLI in topic B via the error-recovery sweep.

Converting to draft until the abort flow is reconciled with the new topic scope. Likely fix direction: have kill_by_chat_topic() (or a layer above it) tag the targeted entries as aborted, or propagate the topic-scoped abort intent into _reset_on_error so it does not chat-wide sweep on a topic-scoped stop.

Add a topic-aware abort marker for ProcessRegistry.kill_by_chat_topic so
/stop in a forum topic is recognized by flow and streaming abort checks
without setting the chat-wide abort marker.

Topic abort marker is cleared on the next message entry to the same topic
(parallel to existing chat-wide _aborted clear at core.py:328). No
persistent state.

Background tasks and pending queue drain remain chat-wide in this PR
(background tasks are not topic-tagged per the current model;
drain_pending is unchanged). Topic-aware versions can be added in
follow-up issues if needed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ryuhaneul ryuhaneul force-pushed the fix/stop-topic-scoped branch from 5af87a9 to 12494ca Compare May 26, 2026 02:50
@ryuhaneul ryuhaneul marked this pull request as ready for review May 26, 2026 03:02
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