feat(acp): recover from an unresponsive agent without restarting Neovim#267
Open
schmoelder wants to merge 6 commits into
Open
feat(acp): recover from an unresponsive agent without restarting Neovim#267schmoelder wants to merge 6 commits into
schmoelder wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a libuv-based inactivity watchdog to Sequence Diagram(s)sequenceDiagram
participant User
participant Agentic
participant SessionRegistry
participant SessionManager
participant ACPClient
participant Transport
User->>Agentic: reconnect()
Agentic->>SessionRegistry: get_session_for_tab_page()
SessionRegistry-->>Agentic: session
Agentic->>SessionManager: session:reconnect()
SessionManager->>ACPClient: agent:reconnect()
ACPClient->>Transport: stop()
ACPClient->>Transport: _connect()
ACPClient-->>SessionManager: agent:when_ready
alt loadSession supported
SessionManager->>SessionManager: load_acp_session(session_id)
else
SessionManager->>SessionManager: new_session()
end
sequenceDiagram
participant Caller
participant ACPClient
participant Timer
participant Transport
Caller->>ACPClient: _send_request()
ACPClient->>ACPClient: record watched request
ACPClient->>Timer: arm watchdog
ACPClient->>Transport: send(request)
Transport-->>ACPClient: inbound message
ACPClient->>ACPClient: update last activity
Timer->>ACPClient: _check_watchdog()
alt idle past timeout
ACPClient->>ACPClient: reject watched callbacks
else response received
ACPClient->>ACPClient: clear watched request
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lua/agentic/session_manager.lua`:
- Around line 1406-1412: In the callback passed to self.agent:when_ready(), the
code captures session_id and uses it without re-validating if it's still the
current session. Before calling self:load_acp_session(session_id), add a check
to ensure the captured session_id matches the current session state. If the
session has changed since the callback was registered, skip the load_acp_session
call and proceed with new_session instead to prevent overwriting newer session
state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2581e8f-346b-4e18-ab57-ab14fb4508f5
📒 Files selected for processing (6)
lua/agentic/acp/acp_client.lualua/agentic/acp/acp_client.test.lualua/agentic/acp/acp_transport.test.lualua/agentic/init.lualua/agentic/session_manager.lualua/agentic/session_manager.test.lua
Contributor
Author
|
full transparency, I vibecoded all of this (using agentic.nvim! ;-)) |
schmoelder
force-pushed
the
fix/agent-reconnect-on-hang
branch
from
June 26, 2026 07:39
7a5ae01 to
b087b6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lua/agentic/acp/acp_client_types.lua`:
- Around line 329-330: `ACPProviderConfig` currently exposes both `timeout` and
`request_timeout`, but only `request_timeout` is used by the watchdog in
`acp_client.lua` via the active request handling path. Update the config
definition and related consumers to remove the unused `timeout` field or wire it
into real logic if it is meant to control a separate deadline; if keeping the
watchdog setting, rename `request_timeout` to a clearer symbol such as
`watchdog_timeout` or `request_silence_timeout` and update all references
consistently. Make sure the behavior matches the documented field names so users
are not configuring a no-op.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2a66f9b1-ee41-41f1-a316-1036046c43fc
📒 Files selected for processing (7)
lua/agentic/acp/acp_client.lualua/agentic/acp/acp_client.test.lualua/agentic/acp/acp_client_types.lualua/agentic/acp/acp_transport.test.lualua/agentic/init.lualua/agentic/session_manager.lualua/agentic/session_manager.test.lua
After a suspend or hang the agent child keeps its stdio pipes open and never exits, so the process-exit path never drains pending callbacks and a control request hangs forever. A connection-level timer, armed only while a control request is in flight and reset by any inbound message, rejects those requests with TIMEOUT_ERROR after sustained silence. session/prompt is deliberately excluded: it can run silently for minutes during a tool call, so silence is not a death signal there and a long generation must never be cancelled; the prompt path is guarded separately by a liveness probe. Explicit transport send failures are rejected immediately for every method.
When the agent child hangs (e.g. the machine resumes from suspend with a dead connection) the only recovery was to quit Neovim and restore, since quitting is what actually killed the child. Agentic.reconnect() does that in place: stop the child, which drains the stuck request callbacks and resets the generating UI, respawn and re-initialize, then reload the current session so the conversation continues. The agent process is shared per provider, so this also resets other tabs on that provider; they re-establish on next use.
Re-initialization after a reconnect is async; capturing session_id up front could reload a session that was replaced during the window, clobbering newer state. Read it inside the when_ready callback so the current session is reloaded instead.
schmoelder
force-pushed
the
fix/agent-reconnect-on-hang
branch
from
June 26, 2026 07:56
b087b6f to
77bb52e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lua/agentic/acp/acp_client.lua`:
- Around line 280-286: The watchdog flow in `_check_watchdog()` and
`_on_request_stall()` can reject fresh watched requests after activity has
already resumed. Re-check the timeout condition right at the start of
`_on_request_stall()` using `_last_activity` and the same `timeout` before
touching `_watched_pending`, and return early if the request is no longer stale.
Keep the existing disarm/schedule behavior, but ensure `_on_request_stall()`
only rejects requests when the inactivity window is still valid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 72ebcb2d-9051-4453-a74b-c6f8963272f3
📒 Files selected for processing (7)
lua/agentic/acp/acp_client.lualua/agentic/acp/acp_client.test.lualua/agentic/acp/acp_client_types.lualua/agentic/acp/acp_transport.test.lualua/agentic/init.lualua/agentic/session_manager.lualua/agentic/session_manager.test.lua
Between _check_watchdog disarming and _on_request_stall running, a response can arrive and update _last_activity. Without a re-check, the stall handler rejects a request that is no longer stale.
uv.now() counts ms since the event loop started, not wall time. On a fresh CI runner it can be only a few seconds, so _last_activity = 0 no longer reliably falls outside the 60s timeout window. Pin the stale timestamp as uv.now() - 70000 instead.
Contributor
Author
|
The only typecheck fail seems to be fixed upstream (a37f19f). I rebased, hopefully this now passes. |
Owner
|
Hey @schmoelder, I'm doing a deeper review of this PR, and it seems way more complex than initially assessed. Which OS, and how often do you face this unresponsive issue? This PR will introduce some hard-to-debug edge cases, such as:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After the machine resumes from suspend (or the agent child otherwise hangs), a prompt gets no reply and the connection can't recover; the only fix was to quit Neovim and restore, since quitting is what actually kills the child. This adds in-place recovery, split out of #266 per your request.
initialize/session/load/session/list/set_mode/set_model) is rejected with a timeout instead of hanging forever.session/promptis deliberately excluded, since it can legitimately run silently for minutes during a tool call.Agentic.reconnect(): kills the hung child (which drains the stuck request and clears the spinner), respawns and re-initializes, then reloads the current session so the conversation continues, with no Neovim restart needed.Follow-up to #266.