Skip to content

fix(acp-bridge): Disable permission timeout by default #9933

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
doudouOUC merged 4 commits into from
Aug 24, 2026
Merged

fix(acp-bridge): Disable permission timeout by default #9933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5697d2f
fix(acp-bridge): disable permission timeout by default
doudouOUC Aug 24, 2026
ecb06d2
Merge branch 'main' into fix/daemon-permission-timeout-default
wenshao Aug 24, 2026
6de94b0
chore: regenerate settings schema
doudouOUC Aug 24, 2026
07c2342
docs(acp-bridge): fix stale timeout comment
doudouOUC Aug 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions docs/design/2026-08-24-daemon-ask-user-question-timeout.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Daemon permission-response timeout default

## Goal

Let daemon interactions that require a human response wait indefinitely by default. Operators that require a wall-clock cap continue to use the existing `permissionResponseTimeoutMs` option or `qwen serve --permission-response-timeout-ms` flag.

## Current boundary

`BridgeClient` sends ordinary permissions and ACP tool calls marked with `_meta.qwenInteractionKind = "user_question"` through the same permission mediator. The mediator owns the single per-request timer, so the shared bridge option remains the correct configuration boundary.

## Behavior

- When `permissionResponseTimeoutMs` is omitted or `0`, neither ordinary permissions nor `ask_user_question` requests install a wall-clock timer.
- A positive `permissionResponseTimeoutMs`, including one supplied by the existing CLI flag, applies to both interaction kinds.
- No environment variable, wire field, or question-specific option is added.
- Voter cancellation, session cancellation, prompt cancellation, disconnect cleanup, idle reaping, daemon shutdown, and pending-interaction caps are unchanged.

## Non-goals

This change does not alter permission policy, pending interaction snapshots, daemon restart restoration, or the optional prompt-wide deadline.

## Verification

Focused bridge tests cover both interaction kinds with the default disabled timer and with an explicit finite timeout. Existing CLI tests cover option parsing and validation; package build and type checks cover the shared option wiring.
6 changes: 3 additions & 3 deletions docs/developers/daemon/03-acp-bridge.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ Each active `WorkspaceRuntime` owns one `HttpAcpBridge` instance. Production att
| `defaultEntry` | `SessionEntry \| null` | The "single" session used when `sessionScope: 'single'`. |
| `defaultPolicy` | `PermissionPolicy` | Configured via `BridgeOptions.permissionPolicy`. |
| `mediator` | `MultiClientPermissionMediator` | One per bridge instance. |
| Constants | — | `DEFAULT_INIT_TIMEOUT_MS = 10_000`, `MCP_RESTART_TIMEOUT_MS = 300_000`, `DEFAULT_MAX_SESSIONS = 32`, `MAX_EVENT_RING_SIZE = 1_000_000`, `DEFAULT_PERMISSION_TIMEOUT_MS = 5min`, `DEFAULT_MAX_PENDING_PER_SESSION = 64`. |
| Constants | — | `DEFAULT_INIT_TIMEOUT_MS = 10_000`, `MCP_RESTART_TIMEOUT_MS = 300_000`, `DEFAULT_MAX_SESSIONS = 32`, `MAX_EVENT_RING_SIZE = 1_000_000`, `DEFAULT_PERMISSION_TIMEOUT_MS = 0`, `DEFAULT_MAX_PENDING_PER_SESSION = 64`. |

**`isDying` invariant**: any teardown path must set `ChannelInfo.isDying = true` synchronously **before** awaiting `channel.kill()`. `ensureChannel` treats a dying channel as absent and spawns a fresh one. Without this flag a concurrent `spawnOrAttach` arriving during the SIGTERM grace window (up to 10s) would attach to a transport about to close and the caller's sessionId would 404 on every follow-up. **Set sites** (must keep in sync): `ensureChannel` (initialize failure + late-shutdown re-check), `doSpawn` (newSession failure on empty channel), `killSession` (last session leaving), `shutdown` (bulk).

Expand Down Expand Up @@ -179,7 +179,7 @@ sequenceDiagram
- Bridge construction is synchronous. A caller may preheat the channel before the first session; otherwise the first `spawnOrAttach` cold-starts the ACP child. A failed preheat leaves first use free to retry.
- `defaultEntry` lives for the lifetime of the bridge under `sessionScope: 'single'`; the channel reaps when `sessionIds.size === 0` (after `killSession`) AND `isDying` flips true.
- `MAX_EVENT_RING_SIZE = 1_000_000` is a soft upper bound on `BridgeOptions.eventRingSize` to catch operator typos before ~500 MB per-session OOMs.
- `DEFAULT_PERMISSION_TIMEOUT_MS = 5 * 60 * 1000` keeps a wedged permission request from blocking the per-session `promptQueue` forever.
- `DEFAULT_PERMISSION_TIMEOUT_MS = 0` lets human permissions and questions wait indefinitely by default. `permissionResponseTimeoutMs` enables a wall-clock cap when operators need one; voter cancellation, session cancellation, and shutdown remain available without it.
- `DEFAULT_MAX_PENDING_PER_SESSION = 64` mirrors `DEFAULT_MAX_SUBSCRIBERS`; excess `requestPermission` calls resolve as cancelled with a stderr warning.

## Dependencies
Expand All @@ -203,7 +203,7 @@ sequenceDiagram
| `sessionRestoreTimeoutMs` | `60_000` | ACP `loadSession` / `unstable_resumeSession` timeout; defaults to 60s, and an explicitly configured initialize timeout can raise it but never lower it. |
| `maxSessions` | `DEFAULT_MAX_SESSIONS = 32` | Cap on `byId.size`. `0` / `Infinity` = unlimited; NaN/negative throws. |
| `eventRingSize` | `DEFAULT_RING_SIZE` (from `eventBus.ts`) | Per-session event ring; soft-capped at `MAX_EVENT_RING_SIZE`. |
| `permissionResponseTimeoutMs` | `DEFAULT_PERMISSION_TIMEOUT_MS = 5 min` | Per-request wallclock for the mediator. |
| `permissionResponseTimeoutMs` | `DEFAULT_PERMISSION_TIMEOUT_MS = 0` | Per-request wallclock for the mediator; `0` disables it. |
| `maxPendingPermissionsPerSession` | `DEFAULT_MAX_PENDING_PER_SESSION = 64` | Backpressure on high-volume agents. |
| `childEnvOverrides` | `{}` | Per-handle env additions / scrubs for the ACP child. |
| `externalToolGuard` | (none) | Optional handler for the private child-to-parent pre-execution decision. The bridge accepts it only from the owning channel for the currently active Prompt. |
Expand Down
9 changes: 5 additions & 4 deletions docs/developers/daemon/04-permission-mediation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,10 +203,11 @@ Math.max(1, Math.floor(m / 2) + 1);
| 6 | 4 | More than half. |

For **M = 2**, split votes (A selects X, B selects Y) can only be resolved by
the per-permission timeout: no option reaches unanimity, so the request waits
until `permissionResponseTimeoutMs` (default 5 min) and resolves as
`{cancelled, timeout}`. The vote-advance path logs this "unanimity means split
votes time out" behavior to stderr for operators.
voter cancellation, session cancellation, or the optional interaction timeout:
no option reaches unanimity. `permissionResponseTimeoutMs` is disabled by
default; when configured, an unresolved split resolves as
Comment on lines +206 to +208

@wenshao wenshao Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The request()/N1 workflow section ~90 lines above this paragraph still documents unconditional timer arming — the flowchart node at :111 (arm setTimeout(timeoutMs) → resolve {cancelled, timeout}), the Responsibilities bullet at :27, the timeoutHandle bullet at :94, and the :117 rationale ("Without this … the entry pending with no timeout — the bridge's per-session promptQueue would hang forever"). At this commit request() arms nothing when timeoutMs is 0, and 0 is what every default-configured daemon forwards — so the documented workflow step never executes by default and this page contradicts itself (:207 says disabled by default; :111 says request() always arms). Worth making the flowchart's arm step conditional and scoping the N1 sentence and bullets to "when a timeout is configured (default 0 = no timer)" — while editing node F, its F→G order (arm before pending.set) also doesn't match the code's actual order (pending.set at permissionMediator.ts:446 precedes the setTimeout at :509).

中文说明

[Suggestion] 本段落上方约 90 行的 request()/N1 工作流小节仍把计时器装配描述为无条件行为——:111 的流程图节点（arm setTimeout(timeoutMs) → resolve {cancelled, timeout}）、:27 的 Responsibilities 条目、:94 的 timeoutHandle 条目，以及 :117 的理由（"Without this … the entry pending with no timeout — the bridge's per-session promptQueue would hang forever"）。而在当前提交，timeoutMs 为 0 时 request() 不会装配任何计时器，且每个默认配置的 daemon 转发的正是 0——被文档化的工作流步骤默认情况下从不执行，本页自相矛盾（:207 说默认禁用；:111 说 request() 总是装配）。建议把流程图的装配步骤改为条件分支，并将 N1 句子和两个条目限定为"配置了超时时（默认 0 = 无计时器）"——顺便一提，编辑节点 F 时可一并纠正其 F→G 顺序（先装配后 pending.set）与代码实际顺序（permissionMediator.ts:446 的 pending.set 先于 :509 的 setTimeout）不符的问题。

— glm-5.3 via Qwen Code /review (v0.22.0)

`{cancelled, timeout}` at that deadline. The vote-advance path logs the
applicable behavior to stderr for operators.
Comment on lines +207 to +210

@qwen-code-ci-bot qwen-code-ci-bot Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The timer default this paragraph documents was flipped by this PR, but the same page's unchanged request() Workflow section (lines 107–117) still asserts the old behaviour: the flowchart node E --> F["arm setTimeout(timeoutMs) → resolve {cancelled, timeout}"] arms the timer unconditionally, and the rationale sentence claims an entry left "pending with no timeout" hangs the per-session promptQueue forever. Under the new default (DEFAULT_PERMISSION_TIMEOUT_MS = 0 → mediator gate if (timeoutMs > 0) at permissionMediator.ts:509) no timer is armed at all, and requests resolve via voter/session cancellation or forgetSession — exactly as this paragraph and the sibling docs updated here now say, so the page contradicts itself and the code.

A maintainer modifying MultiClientPermissionMediator.request() or debugging a wedged prompt reads the Workflow section and concludes every pending request has an armed wall-clock backstop — e.g. writes code assuming pending.timer is always set (it is undefined under the default config; resolveEntry at permissionMediator.ts:1015 explicitly guards that), or tells an operator the request auto-cancels at a deadline that is never armed.

Branch the flowchart on the gate that exists in code and fix the rationale sentence (in the Workflow section, lines 107–117):

E --> T{"timeoutMs > 0?"}
T -- yes --> F["arm setTimeout(timeoutMs) → resolve {cancelled, timeout}"]
T -- no --> G["no timer — resolves only via voter/session cancellation or forgetSession"]
Loading

and state that with the timeout disabled (the default since this PR) entries legitimately carry no timer and the wait is bounded by cancellation paths, not a wall-clock deadline.

中文说明

本段描述的计时器默认值已被此 PR 翻转，但同一页面未修改的 request() Workflow 小节（107–117 行）仍在断言旧行为：流程图节点 E --> F["arm setTimeout(timeoutMs) → resolve {cancelled, timeout}"] 无条件安装计时器，理由句声称“没有超时而 pending”的条目会让 per-session promptQueue 永远挂起。在新默认值下（DEFAULT_PERMISSION_TIMEOUT_MS = 0 → mediator 门控 if (timeoutMs > 0)，permissionMediator.ts:509），根本不会安装计时器，请求通过 voter/session 取消或 forgetSession 解决——正如本段和此 PR 更新的姊妹文档所述，因此该页面与代码自相矛盾。

修改 MultiClientPermissionMediator.request() 或调试卡住的 prompt 的维护者读到 Workflow 小节，会以为每个 pending 请求都有墙钟兜底——例如写出假设 pending.timer 恒已设置的代码（默认配置下它是 undefined；permissionMediator.ts:1015 的 resolveEntry 明确做了防御），或告诉运维请求会在一个从未安装的截止时间后自动取消。

建议在 Workflow 小节（107–117 行）将流程图按代码中实际存在的门控分支，并调整理由句说明：超时禁用（本 PR 起为默认）时条目可以合法地不携带计时器，等待由取消路径兜底，而非墙钟截止时间。

— qwen3.8-max via Qwen Code /review (v0.22.0)


Operators who want first-vote-wins behavior for M = 2 can explicitly set
`policy.consensusQuorum: 1`. Stricter configurations, such as requiring
Expand Down
5 changes: 3 additions & 2 deletions docs/developers/daemon/17-configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ This page collects every setting that affects the `qwen serve` daemon and its ad
| `--channel-idle-timeout-ms <n>` | non-negative integer | `0` | How long to keep the ACP child alive after the last session closes. `0` means reclaim immediately. |
| `--initialize-timeout-ms <n>` | positive integer | `10000` | ACP child request timeout, including the initialize handshake (ms). |
| `--session-restore-timeout-ms <n>` | positive integer | `60000` | ACP session load/resume timeout (ms). When this flag is omitted, an explicitly supplied initialize timeout raises the budget but never lowers it below the default. |
| `--permission-response-timeout-ms <n>` | non-negative integer | `0` | Shared wall-clock timeout for ordinary permission and `ask_user_question` responses. `0` or an omitted flag waits indefinitely; a positive value enables the timer. |
| `--session-reap-interval-ms <n>` | non-negative integer | `60000` | Session reaper scan interval; `0` disables it. |
| `--session-idle-timeout-ms <n>` | non-negative integer | `1800000` | Disconnected-session idle reaping time; `0` disables it. |
| `--rate-limit` / `--no-rate-limit` | boolean | env / off | Enables per-tier HTTP rate limiting for prompt, mutation, and read routes. |
Expand Down Expand Up @@ -142,7 +143,7 @@ The daemon constructs each workspace runtime from that workspace's merged settin
| -------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `boundWorkspace` | Required canonical workspace. |
| `sessionScope` | `'single'` (default) vs `'thread'`. |
| `initializeTimeoutMs`, `sessionRestoreTimeoutMs`, `maxSessions`, `eventRingSize`, `permissionResponseTimeoutMs`, `maxPendingPermissionsPerSession` | Bounded resource caps. |
| `initializeTimeoutMs`, `sessionRestoreTimeoutMs`, `maxSessions`, `eventRingSize`, `permissionResponseTimeoutMs`, `maxPendingPermissionsPerSession` | Bounded resource caps. `permissionResponseTimeoutMs` is shared by ordinary permissions and questions; `0` or omission disables its timer. |
| `channelFactory` | Pluggable ACP child factory; default is `defaultSpawnChannelFactory`. |
| `fileSystem` | `BridgeFileSystem` adapter. See [`07-workspace-filesystem.md`](./07-workspace-filesystem.md). |
| `permissionPolicy`, `permissionConsensusQuorum`, `permissionAudit` | Mediator wiring. |
Expand All @@ -165,7 +166,7 @@ The daemon constructs each workspace runtime from that workspace's merged settin
| `WARN_RESET_RATIO` | `eventBus.ts` | `0.375` | Hysteresis re-arm threshold. |
| `DEFAULT_INIT_TIMEOUT_MS` | `bridge.ts` | `10_000` | ACP `initialize` handshake timeout. |
| `MCP_RESTART_TIMEOUT_MS` | `bridge.ts` | `300_000` | Bridge timeout for `/workspace/mcp/:server/restart`. |
| `DEFAULT_PERMISSION_TIMEOUT_MS` | `bridge.ts` | `5 * 60_000` | Per-permission request wallclock. |
| `DEFAULT_PERMISSION_TIMEOUT_MS` | `bridge.ts` | `0` | Shared permission and question wallclock; `0` disables the timer. |
| `DEFAULT_MAX_PENDING_PER_SESSION` | `bridge.ts` | `64` | Aligned with `DEFAULT_MAX_SUBSCRIBERS`. |
| `MAX_RESOLVED_PERMISSION_RECORDS` | `permissionMediator.ts` | `512` | FIFO for recently resolved permissions. |
| `KILL_HARD_DEADLINE_MS` | `spawnChannel.ts` | `10_000` | Per-channel graceful shutdown window. |
Expand Down
14 changes: 9 additions & 5 deletions docs/developers/qwen-serve-protocol.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3074,19 +3074,23 @@ The active policy is configured in `settings.json` under `policy.permissionStrat

> **F3 (#4175): multi-client permission coordination.** F3 added the four policies above. Pre-F3 daemons hardcoded first-responder; the wire shape stays bit-for-bit unchanged when the configured policy is `first-responder`. New events (`permission_partial_vote`, `permission_forbidden`) are additive — old SDKs see them as `unrecognized_known_event` and gracefully ignore.

> **Permission timeout (default 5 minutes).** A `permission_request`
> **Permission timeout (disabled by default).** A `permission_request`
> stays pending until: (a) some client votes here, (b) `POST /session/:id/cancel`
> fires, (c) the HTTP client driving the prompt disconnects
> (mid-prompt cancel resolves outstanding permissions as `cancelled`),
> (d) the session is killed, (e) the daemon shuts down, **or
> (f) the per-session permission timeout fires** (`DEFAULT_PERMISSION_TIMEOUT_MS`,
> 5 minutes). On timeout fire the agent's `requestPermission` resolves
> (f) its configured timeout fires**. On timeout fire the agent's
> `requestPermission` resolves
> as `{outcome: 'cancelled'}`, the audit ring records a
> `permission.timeout` entry, daemon stderr emits a one-line
> breadcrumb, and the SSE bus fans out the standard
> `permission_resolved` cancelled frame so subscribers clean up. The
> timeout is configurable via `BridgeOptions.permissionResponseTimeoutMs`;
> headless callers running long-form prompts may want to extend it.
> shared timeout is configurable via
> `BridgeOptions.permissionResponseTimeoutMs` or
> `qwen serve --permission-response-timeout-ms`. Its default is `0`, so both
> ordinary permissions and `ask_user_question` wait indefinitely for a human
> decision. Voter cancellation, session cancellation, disconnect cleanup, and
> daemon shutdown still resolve pending interactions as cancelled.
Comment on lines +3092 to +3093

@wenshao wenshao Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This sentence (and retained item (c) above) lists "disconnect cleanup" as a resolution path — but no disconnect resolves a pending interaction on the REST/SSE surface this protocol documents: the prompt POST's close listener is removed in the same synchronous block as admission (routes/session.ts:4851), an SSE subscriber's disconnect runs telemetry-only finalize, and the real disconnect cleanup (abandonPendingForSession, promptAbort/graceTimer) belongs to the /acp JSON-RPC transport, which this doc doesn't cover. A REST integrator who drops their connection as their cleanup step gets a silently-never-firing cleanup — and with the default now 0, the pin is indefinite instead of 5-minute-bounded. The same enumeration in the new design doc inherits the issue.

Suggested change
> decision. Voter cancellation, session cancellation, disconnect cleanup, and
> daemon shutdown still resolve pending interactions as cancelled.
> decision. Voter cancellation, session cancellation, and daemon shutdown
> still resolve pending interactions as cancelled (on the /acp JSON-RPC
> transport, a driver disconnect also cancels after the reconnect grace
> window; a REST prompt driver gets no disconnect cleanup once the 202 is
> returned — cancel via POST /session/:id/cancel).
中文说明

[Suggestion] 这句话（以及上方保留的条目 (c)）把 "disconnect cleanup" 列为解决路径——但在本文档所描述的 REST/SSE 表面上，任何断连都不会解决 pending 交互：prompt POST 的 close 监听在准入的同一同步块内被移除（routes/session.ts:4851），SSE 订阅者断连只执行遥测 finalize，而真正的断连清理（abandonPendingForSession、promptAbort/graceTimer）属于本文档未覆盖的 /acp JSON-RPC 传输。以断开连接作为清理步骤的 REST 集成方会得到一个永远不会触发的清理——且默认值改为 0 后，钉死从 5 分钟有界变为无限期。新设计文档中的相同枚举同样继承此问题。

— glm-5.3 via Qwen Code /review (v0.22.0)


Request:

Expand Down
Loading
Loading