Skip to content

fix(session): cancel/teardown correctness - #20102

Closed
ESRE-dev wants to merge 1 commit into
anomalyco:devfrom
ESRE-dev:pr/cancel-correctness
Closed

fix(session): cancel/teardown correctness#20102
ESRE-dev wants to merge 1 commit into
anomalyco:devfrom
ESRE-dev:pr/cancel-correctness

Conversation

@ESRE-dev

@ESRE-dev ESRE-dev commented Mar 30, 2026

Copy link
Copy Markdown

Issue for this PR

Closes #20095

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Fixes session-cancel race conditions that can cause lost cancels, stale aborts, and dangling waits:

  1. Pre-cancel handling

    • If cancel happens before start, the cancel is recorded and consumed on start so the session does not incorrectly run.
  2. Generation-guarded cancel

    • Cancels are tied to session generation, preventing stale cancel callbacks from aborting a newly-started run of the same session.
  3. Cancel-time rejection of pending waits

    • Adds rejectSession(sessionID) to permission and question services so pending asks are rejected when a session is cancelled, preventing indefinite blocking.
  4. Cancel propagation bridge

    • Adds prompt-layer initialization for cancel event propagation so cancellation is handled consistently across components.
  5. Explicit cancel error + test hooks

    • Uses a dedicated SessionCancelledError and exposes internal state hooks used by cancellation tests.

Related overlap (not duplicates):

How did you verify your code works?

  • Added/ran packages/opencode/test/session/cancel.test.ts (generation isolation, pre-cancel behavior, callback rejection, idempotency, event propagation).
  • Added/ran packages/opencode/test/question/nested-question.test.ts (session-scoped rejection behavior and nested question flows).
  • Verified pending permission/question waits are rejected on cancel in service-layer behavior.

Screenshots / recordings

Not a UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

If you do not follow this template your PR will be automatically rejected.

- Add pre-cancel map to prevent lost cancels when cancel() arrives before start()
- Add generation-guarded cancel to prevent stale cancel from aborting new sessions
- Add rejectSession() to permission and question services to unblock pending prompts
- Add SessionPrompt.init() to bridge CancelRequested events from processor
- Add SessionCancelledError for explicit cancel rejection
- Add SessionActivity module for per-session activity tracking
- Add comprehensive cancel test suite (17 tests)
- Add nested question propagation tests (6 tests)
@github-actions

Copy link
Copy Markdown
Contributor

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

  1. Open an issue describing the bug/feature (if one doesn't exist)
  2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

@github-actions github-actions Bot added needs:compliance This means the issue will auto-close after 2 hours. and removed needs:compliance This means the issue will auto-close after 2 hours. needs:issue labels Mar 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Thanks for updating your PR! It now meets our contributing guidelines. 👍

@ESRE-dev

ESRE-dev commented Apr 1, 2026

Copy link
Copy Markdown
Author

Closing: Obsoleted by upstream Effect/Runner migration

This PR is no longer needed. The upstream codebase has undergone a major architectural migration to Effect/Stream/Runner that addresses all the race conditions this PR targeted, plus the prompt_async fire-and-forget issue was independently fixed. Our PR branches forked from 0bbf26a1ce (Mar 20) and were pushed Mar 30 — the same day the final migration commit landed.

Upstream commits that supersede this PR

Commit Date Description
814a515a8a Mar 24 fix: improve plugin system robustness (#18280) — Adds .catch() + Bus.publish(Session.Event.Error) to the prompt_async route, fixing the fire-and-forget unhandled rejection. Includes a test.
860531c275 Mar 28 refactor(session): effectify session processor (#19485) — Effect/Stream replaces raw async iteration; interruption is native.
c5442d418d Mar 30 refactor(session): effectify SessionPrompt service (#19483) — Introduces the Runner state machine (Idle → Running → Shell → ShellThenRun) with SynchronizedRef-based atomic transitions. Runner.Cancelled replaces our SessionCancelledError. ensureRunning coalesces concurrent callers via Deferred. Fiber-based interruption propagates through Deferred.await in Permission/Question services, replacing our explicit rejectSession().

What this PR did vs what upstream now handles

Our fix Upstream solution
Generation-guarded cancel (prevent stale cancel hitting new generation) Runner state machine with fiber IDs — cancel only interrupts the current fiber
precancelled map (cancel before start) Runner.ensureRunning + SynchronizedRef — atomic state transitions prevent lost cancels
SessionCancelledError Runner.Cancelled error class
rejectSession() for Permission/Question on cancel Effect interruption propagates through Deferred.await — pending Permission/Question awaits are interrupted when the fiber is interrupted
prompt_async fire-and-forget fix (planned addition) Already fixed in 814a515a8a with .catch() + Bus.publish(Error)

Related: #20095, #20094, #14565, #19855
Superseded by: #19483, #19485, #18280

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant