Repository navigation
Conversation
leepokai
left a comment
There was a problem hiding this comment.
Thanks for the deep work here! The process-ownership half is excellent and I'd like to land it, but the lifecycle policy needs changes first.
Great, ready to merge on their own
- Process tree ownership:
waitid(WNOWAIT)keeping the leader unreaped until the group is killed, and the suspended launch +KILL_ON_JOB_CLOSEjob on Windows. Much more reliable thantaskkill /T. Answer'sDropremoving abandoned pending requests.allocation_uncertainand persisting the first-promptPreludein the snapshot.
Needs changes
- Idle retirement kills user-started background processes. Retiring after
IDLE_GRACE(2 min) SIGKILLs the whole process group, including anything the agent launched in the background, e.g. a dev server Claude started with a background Bash.retire_if_idleonly looks at host-side state (turn, queue, approvals, requests), not at work the agent still has running. Users will see their servers vanish two minutes after the reply. - Two minutes means a cold start on most phone messages. Gaps over 2 min are the norm in chat, so nearly every message would pay launch +
session/loadreplay (your own Cursor probe measured ~11–12 s). Please make it much longer (e.g. 15–30 min) or pressure-based (only under memory pressure / over the live-session bound). - A failed
session/loadnow leaves the conversation stuck. Previously a failed load started a fresh session with a divider notice; now it returns "retry to resume it". When the saved session can't be loaded (bot cwd changed, which keeps the main session; session file deleted; adapter changed), retrying never succeeds. Main can recover through New session, but threads have no reset path, so a thread errors forever. Same for agents withoutloadSessionafter a host restart or crash: they used to start fresh automatically and now error on every message. Please keep the fresh-session fallback (with the notice) for unrecoverable load failures.
Minor
- Forking now also requires
sessions.load, so fork-only adapters lose the parent context for threads. fork_threadloads the main session twice; the first load can be released immediately bymake_session_room.
Suggestion: split into (a) process ownership + pending-answer cleanup, which I'm happy to merge as-is, and (b) the lifecycle policy, revised per the points above. The CI failure was the MIN_HOST floor test on dev; that's fixed now (dev includes 2.12.0), so a rebase should go green.
86020e0 to
a998e1b
Compare
4edd205 to
9326c09
Compare
1230571 to
280dc5d
Compare
leepokai
left a comment
There was a problem hiding this comment.
Thanks for the continued work on this, @fergusean. The process-ownership half is still excellent, and I'd like to land it. The lifecycle half has changed shape since my last review, and dev has moved on underneath it, so here's where things stand.
dev now releases idle agents (f5d6350, host/src/agent/bot/lifecycle.rs). A session unused for 30 minutes is closed through session/close when the agent advertises it. Once all of a bot's sessions are idle, its agent stops with its process group, and the next message starts it again and loads the session. Nothing is released mid-turn or with a queue, approval or request waiting. Agents that can't loadSession keep everything. While the agent has a descendant in a process group of its own, such as a dev server from Claude Code's run_in_background, the agent stays. That replaces the per-process live-session cap here, so please drop lifecycle.rs's cap/recycling from this PR.
What I'd merge now, as its own PR: the process ownership. That's waitid(WNOWAIT) keeping the leader unreaped, the suspended launch plus KILL_ON_JOB_CLOSE job on Windows, Answer's Drop, allocation_uncertain and the persisted Prelude. Splitting it out would let it land quickly.
Problems in the current head's lifecycle and recovery changes, if you keep any of them:
- Claude's missing session is never recovered. When the transcript is gone,
claude-agent-acpanswerssession/loadwith-32002"Resource not found: ".unavailable_saved_sessionmatches neither the code nor the message (every phrase contains "session"), so main and threads stay stuck on "retry to resume it" forever.devcurrently starts a fresh session with a divider on any failed load. - The cap breaks agents without
loadSession/session/close. With main plus one thread live, New session is refused and a second thread (or any routine run) fails with "reached its live session limit" until the host restarts (lifecycle.rs:66-70,:138). - The session to release is effectively random. It's the first key of a
HashMap(lifecycle.rs:75), so a routine run can close main's session and, withoutsession/close, kill the whole process group. - A failed
session/loadkills the whole agent first (session_setup.rs:124), taking the live main session and its tools with it, even when closing the one session would do. - Forking from a parent that can't be reloaded silently replaces main's session. The "starting a fresh one" divider then lands in the thread, not in main.
agent_lifecycle_e2e.rs:319sleeps 125 s in the default suite to prove a timer that no longer exists. Please drop or#[ignore]it. The pre-commit hook now stops host tests after 5 minutes.
Rebase notes: besides the textual conflicts (Cargo.toml, bot/{mod,session,turn}.rs, tests/common/mod.rs), dev's Fast mode switches the live session through session_request. With this PR's allocation_uncertain, that would make every Fast mode switch recycle the agent, so it needs its own request path.
For the next round, please include a real-host run with Claude Code (and Codex if you can) under Validation, covering the paths you change. The fixtures don't reproduce the real adapters' errors, which is where 1–3 come from.
280dc5d to
93fe2fd
Compare
Keep the upstream thirty-minute idle policy and fresh-session recovery. Own process trees through leader exit, actor drop and graceful shutdown, remove abandoned pending answers, and retain initial instructions across interrupted session setup. Co-authored-by: Codex <codex@openai.com>
93fe2fd to
d3f5a50
Compare
|
Thanks for the detailed review, @leepokai. I've narrowed the PR at
Validation: CI is green on Linux, macOS and Windows, including native-memory checks: run 38899877091. On this exact head, real Claude main/thread prompts, graceful restart, same-ID continuation and missing-transcript recovery all pass. Real Codex main, restart, same-ID continuation and missing-main recovery also pass. Full Codex thread acceptance remains incomplete. An earlier-build probe reached its 180-second harness deadline waiting for a thread reply in Codync. Checking its retained native Codex transcript shows the requested reply and turn completion within six seconds, while Codync's store has no thread reply. That establishes a delivery/completion gap, but not its precise adapter/host cause; the complete thread scenario has not been rerun on this head. The PR description now makes that scope explicit. Marking as draft until that's resolved. |
|
The Codex thread blocker is resolved, @leepokai. The same Updating only the probe adapter to 2.2.2 makes all six real-host checks pass on this exact PR head:
The initial fork reply completes in 4.754 seconds, with no intervening The PR description records the results, and Linux, macOS and Windows CI remain green. Thanks again for the review. Marking this ready for another look. |
Summary
Agent cleanup now retains ownership through leader exit and actor drop. Unix keeps the leader unreaped with
waitid(WNOWAIT)until its group is terminated; Windows assigns the suspended shell to aKILL_ON_JOB_CLOSEjob before resuming it. Intentional shutdown preserves dev's ten-second stdin-close grace period so CLI login refreshes can finish.Abandoned ACP answers remove their pending requests. Interrupted allocations remain marked uncertain until a session identity is confirmed, and session snapshots retain first-prompt profiles and thread introductions across setup failure or restart. Fast mode and idle closes have a separate request path. Confirmed new/fork identities are recorded before fallible model setup so retry uses the same conversation.
Reconstructed on current
dev(acd8998a), retaining its thirty-minute idle policy, background-descendant guard and fresh-session fallback. The previous per-process cap, recycling policy, cap-specific recovery code and 125-second timer test are removed. All clients keep their existing API and notice handling; there are no client UI or version changes.Review follow-up
Resource not foundresponse.loadSessioncan keep multiple threads and start a new main session without a session cap. Fork-only support remains.Validation
cargo fmt --check; all-target Clippy with-D warnings; 294 binary tests passed, two existing tests ignored; ten focused actor end-to-end checks and five upstream turn/Fast mode checks passed.d3f5a50f: real local host with Claude Code /claude-agent-acp0.85.0: exact replies in main and thread; graceful host restart and same-ID continuation in both; deliberately nonexistent saved IDs recover main and thread with divider notices. Existing user bots, credentials and saved conversations were untouched.d3f5a50f: real local host with releasedcodex-acp2.2.2 passes main and independent thread prompts, graceful host restart with same-ID continuation in both, and deliberately missing saved IDs recovering fresh with divider notices in both. All six replies match exactly. The first fork reply takes 4.754 seconds; the first prompt follows fork directly without an intervening load.codex-acp2.1.1 unsubscribed newly forked native threads, so Codex completed internally while ACP emitted no reply chunks or prompt completion. This reproduces on the same host binary; released 2.2.2 retains the subscription through upstream's merged fix. Updating only the probe adapter makes all six checks pass. No host workaround, timeout extension, assertion change or product source change was needed.What's New
zh-Hant
en-US