Skip to content

fix: bound stale running turn recovery - #83

Closed
guanbear wants to merge 1 commit into
william0wang:mainfrom
guanbear:fix/bounded-stale-running-recovery
Closed

fix: bound stale running turn recovery#83
guanbear wants to merge 1 commit into
william0wang:mainfrom
guanbear:fix/bounded-stale-running-recovery

Conversation

@guanbear

Copy link
Copy Markdown
Contributor

Summary

  • separate real ACP/ZCode protocol progress from projection-based liveness probes
  • stop refreshing the 120-second deadline when session/read only repeats status: running
  • at the no-progress deadline, probe the authoritative prompt lock with session/goal show
  • preserve long-running model/tool work when the prompt lock is explicitly held
  • keep the existing bounded max_turn_requests outcome when the lock is released or indeterminate
  • consume already-queued events before making a deadline decision

Why this is a Draft

This implements the narrow, conservative case described in #80: a stale running projection can no longer keep a turn alive after the real prompt lock has been released.

An explicitly held prompt lock defers the next terminal decision by another 120 seconds. That can repeat while the backend continues to prove ownership of the turn. This avoids imposing an arbitrary hard limit on a legitimate long-running model or tool operation, but deliberately does not define policy for a genuinely stuck prompt lock. Maintainer feedback on that boundary is welcome before marking this ready.

Tests

  • added a deterministic public-boundary regression test through prompt() for stale running plus released lock
  • added a long-running active-tool regression test proving an explicitly held lock is not cancelled and later completion still returns end_turn
  • eslint on changed TypeScript files
  • tsc --noEmit
  • tsc
  • full test suite: 56 files, 794 tests passed

Refs #80

@william0wang

Copy link
Copy Markdown
Owner

Thanks for the thorough writeup and the clean minimal fix — the approach is correct and has been carried forward.

I've rebased your work onto current main (main moved substantially since 8/27: the cancel drain gate, v4/command stop, and steer-report rework landed in the same loop) as #85, with three adaptations:

  1. probePromptLock now matches the lock-busy error by code 1308 first (message text kept as fallback) — same rationale as the send-retry loop; message drift between backend releases would otherwise kill a live turn.
  2. Dropped the deadline else if (turn.cancelled) branch — main's in-loop cancel check already covers it; keeping it would misreport a user cancel as max_turn_requests.
  3. Regression tests' microtask pump raised 20 → 80 (main's prompt() gained more async preamble).

On your open policy question: no hard cap on lock-held deferral. Backend-owned long work is legitimate and user cancel remains the escape hatch; a follow-up will surface deferral to the client as a visible session/update note instead of a cap.

Closing this in favor of #85 — your commit is preserved there (rebased, credited in the commit chain).

@guanbear

Copy link
Copy Markdown
Contributor Author

Thanks for carrying this forward and preserving the attribution. The adaptations make sense, especially matching code 1308 and avoiding the redundant cancelled branch.

I agree with keeping lock-held deferral uncapped while user cancellation remains available. Surfacing the deferral through a visible session/update note sounds like the right follow-up.

Glad to see this merged in #85. Thanks again!

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.

2 participants