ACP: recover hung bound turns by dutifulbob · Pull Request #51816 · openclaw/openclaw

dutifulbob · 2026-03-21T18:44:59Z

Summary

Problem: a bound ACP session could stop replying indefinitely if one runtime.runTurn() never completed, because later turns on that session stayed queued behind the stuck turn.
Why it matters: Discord ACP bindings like #codex-1 could look dead until the gateway or session was manually reset.
What changed: AcpSessionManager.runTurn() now applies a manager-side watchdog, aborts/cancels/closes timed-out turns, drops the stale cached runtime handle, and lets queued turns proceed.
What did NOT change (scope boundary): this PR does not change ACP projection or Discord readback behavior beyond unsticking the bound session turn queue.

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Related ACP: recover hung bound turns #51816

User-visible / Behavior Changes

Bound ACP sessions no longer stay wedged behind a single hung turn forever.
When a turn exceeds its watchdog timeout, the session now returns an ACP turn failure instead of starving all later turns on that binding.

Security Impact (required)

New permissions/capabilities? (No)
Secrets/tokens handling changed? (No)
New/changed network calls? (No)
Command/tool execution surface changed? (No)
Data access scope changed? (No)
If any Yes, explain risk + mitigation:

Repro + Verification

Environment

OS: Linux
Runtime/container: Node 22, pnpm test wrapper
Model/provider: openai-codex/gpt-5.4 via acpx/Codex ACP session
Integration/channel (if any): persistent Discord ACP binding (#codex-1)
Relevant config (redacted): Discord ACP thread binding on agent:codex:acp:binding:discord:default:f56b4624dd829241

Steps

Start one ACP turn whose runtime never completes.
Enqueue a second turn on the same ACP session.
Observe that the manager watchdog times out the hung turn, cancels/closes the stale runtime, and allows the queued turn to proceed.

Expected

A single hung turn should not be able to starve all later turns on the same ACP binding indefinitely.

Actual

Before this change, AcpSessionManager.runTurn() waited forever on the hung turn and kept the session actor occupied.
After this change, the manager times out the stuck turn and frees the session actor queue.

Evidence

Attach at least one:

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)

Evidence used:

unit regression in src/acp/control-plane/manager.test.ts
live gateway logs showing prior Discord inbound worker timeouts on #codex-1
live verification after the fix: sending new messages via LOOPER_BOT_TOKEN to #codex-1 now causes the bound ACP session state to return to idle promptly and Bob emits a fresh reply message instead of leaving the worker hung

Human Verification (required)

What you personally verified (not just CI), and how:

Verified scenarios:
- pnpm test -- src/acp/control-plane/manager.test.ts
- pnpm build
- direct acpx prompt against the bound session returned visible text after the session was reset
- live Discord probes sent to #codex-1 via LOOPER_BOT_TOKEN produced fresh Bob replies and advanced the bound ACP session back to idle
Edge cases checked:
- queued second turn runs after the first timed-out turn is cleaned up
- timeout path calls ACP cancel/close and drops the stale cached runtime handle
- service-managed gateway (systemd --user) also round-trips after restarting on the built fix
What you did not verify:
- full pnpm check on this workstation is currently blocked by unrelated untracked plugin-sdk docs formatting work outside this PR diff

Review Conversations

I replied to or resolved every bot review conversation I addressed in this PR.
I left unresolved only the conversations that still need reviewer or maintainer judgment.

If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.

Compatibility / Migration

Backward compatible? (Yes)
Config/env changes? (No)
Migration needed? (No)
If yes, exact upgrade steps:

Failure Recovery (if this breaks)

How to disable/revert this change quickly: revert commit 5cb9f5eb49
Files/config to restore: src/acp/control-plane/manager.core.ts, src/acp/control-plane/manager.test.ts
Known bad symptoms reviewers should watch for: unexpected ACP turn timeout errors on legitimately long-running sessions if operators want a larger timeout than the current manager fallback

Risks and Mitigations

Risk: the new watchdog could end legitimately long turns earlier than some operators expect.
- Mitigation: session-level ACP runtime timeouts already exist; the manager uses the session timeout when configured and otherwise falls back to the existing agent timeout baseline instead of a new tiny default.
Risk: cleanup of a timed-out runtime turn could itself hang.
- Mitigation: cancel/close cleanup is bounded and late cleanup failures are logged without re-blocking the session actor queue.

greptile-apps · 2026-03-21T19:11:09Z

Greptile Summary

This PR adds a manager-side watchdog to AcpSessionManager.runTurn() so that a single hung ACP turn can no longer starve all queued turns on the same binding indefinitely. When the watchdog fires it aborts the turn's signal, cancels and closes the stale runtime handle with bounded cleanup grace periods, evicts the cached runtime state, and throws ACP_TURN_FAILED — freeing the session actor queue for the next turn. The PR also ships a focused regression test that validates the full timeout → cleanup → queued-turn-proceeds flow under fake timers.

Key observations:

The eventGate + IIFE design correctly decouples event-stream consumption from the timeout race without introducing shared-state hazards.
skipPostTurnCleanup correctly prevents the finally block from attempting a redundant reconcile or oneshot-close on a handle that cleanupTimedOutTurn already cleaned up.
The if (streamError) { throw streamError; } check at line 726 is dead code after the refactor: if eventGate.open is true when the IIFE finishes, any streamError is thrown from inside the IIFE and propagates through awaitTurnWithTimeout before reaching this line; if the timeout fired, awaitTurnWithTimeout already throws and this line is never reached. Safe to remove.
Timeout resolution correctly prefers the session's configured timeoutSeconds, falling back to the agent-level default — no new hard-coded short defaults are introduced.

Confidence Score: 4/5

Safe to merge; the watchdog correctly unblocks stuck sessions with no logic errors found.
The core timeout/cleanup logic is sound: eventGate, skipPostTurnCleanup, bounded cleanup grace, and cache eviction all interact correctly. The regression test validates the primary scenario under fake timers. The only finding is one dead if (streamError) check left over from the refactor — harmless but worth cleaning up, keeping this at 4 rather than 5.
No files require special attention; the dead-code line in manager.core.ts:726 is the only non-blocking nit.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/acp/control-plane/manager.core.ts
Line: 726-728

Comment:
**Dead `streamError` re-throw after `awaitTurnWithTimeout`**

After the refactoring, the `if (streamError) { throw streamError; }` check on this line is unreachable and can be removed:

- **Normal path** (`eventGate.open` remains `true`): If the for-await loop emits an error event, the IIFE throws `streamError` internally — that rejection propagates out of `awaitTurnWithTimeout` before this line is ever reached.
- **Timeout path** (`eventGate.open = false`): `awaitTurnWithTimeout` itself throws `AcpRuntimeError("ACP_TURN_FAILED", ...)`, so control never reaches this check.

In neither case can `awaitTurnWithTimeout` return without throwing while `streamError` is truthy.

```suggestion
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "ACP: recover hung bo..."}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a8facdd077

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-03-21T19:52:44Z

+    void Promise.allSettled([params.activeTurn.cancelPromise, closePromise]).then(() => {
+      this.clearCachedRuntimeStateIfHandleMatches({
+        sessionKey: params.sessionKey,
+        handle: params.activeTurn.handle,
+      });


Clear oneshot cache immediately after successful timeout close

In the oneshot timeout path, if cancel exceeds the grace period but close succeeds, this branch defers cache eviction until Promise.allSettled([cancelPromise, closePromise]) resolves; if cancelPromise never settles, eviction never happens even though the runtime session is already closed. That leaves a dead handle permanently counted in runtimeCache, which can exhaust maxConcurrentSessions and block new sessions after a single hung cancel call.

Useful? React with 👍 / 👎.

* ACP: add hung-turn starvation repro * ACP: recover hung bound turns * ACP: preserve timed-out session handles --------- Co-authored-by: Onur <[email protected]>

* ACP: add hung-turn starvation repro * ACP: recover hung bound turns * ACP: preserve timed-out session handles --------- Co-authored-by: Onur <[email protected]> (cherry picked from commit 8cac327)

* ACP: add hung-turn starvation repro * ACP: recover hung bound turns * ACP: preserve timed-out session handles --------- Co-authored-by: Onur <[email protected]>

ACP: add hung-turn starvation repro

b45ffc1

openclaw-barnacle Bot added the size: S label Mar 21, 2026

ACP: recover hung bound turns

5cb9f5e

openclaw-barnacle Bot added size: M and removed size: S labels Mar 21, 2026

dutifulbob changed the title ~~ACP: add hung-turn starvation repro~~ ACP: recover hung bound turns Mar 21, 2026

dutifulbob marked this pull request as ready for review March 21, 2026 19:07

greptile-apps Bot reviewed Mar 21, 2026

View reviewed changes

Comment thread src/acp/control-plane/manager.core.ts

ACP: preserve timed-out session handles

a8facdd

chatgpt-codex-connector Bot reviewed Mar 21, 2026

View reviewed changes

osolmaz merged commit 8cac327 into openclaw:main Mar 21, 2026
37 checks passed

osolmaz deleted the fix/acp-runturn-starvation-repro branch March 21, 2026 19:54

github-actions Bot mentioned this pull request Mar 21, 2026

📡 Upstream Digest — 2026-03-21 20:17 UTC curtismercier/openclaw-mods#325

Open

dutifulbob mentioned this pull request Mar 21, 2026

Agents: raise default timeout to 48h #51874

Merged

20 tasks

alexey-pelykh mentioned this pull request Mar 27, 2026

Cherry-pick: security hardening — ACP test dedupe (1 of 5) remoteclaw/remoteclaw#2013

Merged

clawsweeper Bot mentioned this pull request May 24, 2026

[Bug]: ACP concurrent session spawns — first agent fails to launch CC process #53319

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

ACP: recover hung bound turns#51816

ACP: recover hung bound turns#51816
osolmaz merged 3 commits into
openclaw:mainfrom
osolmaz:fix/acp-runturn-starvation-repro

dutifulbob commented Mar 21, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot commented Mar 21, 2026

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Mar 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

dutifulbob commented Mar 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

User-visible / Behavior Changes

Security Impact (required)

Repro + Verification

Environment

Steps

Expected

Actual

Evidence

Human Verification (required)

Review Conversations

Compatibility / Migration

Failure Recovery (if this breaks)

Risks and Mitigations

Uh oh!

greptile-apps Bot commented Mar 21, 2026

Greptile Summary

Confidence Score: 4/5

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dutifulbob commented Mar 21, 2026 •

edited

Loading