fix(coding-agent): trust supervisor-approved session renames in worker mode - #1814
Closed
snimu wants to merge 17 commits into
Closed
fix(coding-agent): trust supervisor-approved session renames in worker mode#1814snimu wants to merge 17 commits into
snimu wants to merge 17 commits into
Conversation
…-blanket-timeouts
…d-rename-authority
…-blanket-timeouts
…d-rename-authority
| }, 1000); | ||
|
|
||
| this.process?.on("exit", () => { | ||
| const timeout = setTimeout(() => child.kill("SIGKILL"), 1000); |
Contributor
There was a problem hiding this comment.
🟠 High rpc/rpc-client.ts:158
stop() can remain pending indefinitely when a descendant keeps the child’s stdout or stderr pipe open: after the 1-second timer sends SIGKILL, it still waits for close, which is delayed by those inherited pipes. Resolve the shutdown promise from the timeout callback after sending SIGKILL, preserving the previous bounded shutdown behavior.
- const timeout = setTimeout(() => child.kill("SIGKILL"), 1000);
+ const timeout = setTimeout(() => {
+ child.kill("SIGKILL");
+ resolve();
+ }, 1000);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/modes/rpc/rpc-client.ts around line 158:
`stop()` can remain pending indefinitely when a descendant keeps the child’s stdout or stderr pipe open: after the 1-second timer sends `SIGKILL`, it still waits for `close`, which is delayed by those inherited pipes. Resolve the shutdown promise from the timeout callback after sending `SIGKILL`, preserving the previous bounded shutdown behavior.
Evidence trail:
packages/coding-agent/src/modes/rpc/rpc-client.ts:111-115, 150-164 @ REVIEWED_COMMIT
git diff MERGE_BASE REVIEWED_COMMIT -- packages/coding-agent/src/modes/rpc/rpc-client.ts
https://nodejs.org/api/child_process.html#event-close
Contributor
Author
|
Superseded: the chained stack was restructured into independent PRs (byte-identical combined tree). A fresh standalone PR for this change follows on the same branch name. |
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
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.
What was wrong
Renaming a session could fail even though the rename was valid. In worker mode the supervisor already reserves and validates the new name, but the worker then validated it a second time against its own copy of the peer roster — which can be stale (sync failures are suppressed). So a name the supervisor had correctly approved could be rejected by the worker holding outdated data.
The fix
rename, activerename_saved_session,set_session_name) now go through a mutation-only path: the supervisor's reservation is trusted, the worker just applies the name and appends the RLM ledger rename.18 production lines, deletion-first (the second validation layer is gone, not worked around).
How it's verified
tsgo clean, biome clean, focused rename suites pass (daemon + supervisor), full-suite failures reproduce identically on main (pre-existing, unrelated). Implemented and independently reviewed by two different models; the reviewer traced all worker command paths and the authenticated-socket gate.
Follow-up candidates noted in review (not in this PR, same bug class, pre-existing): the non-active
rename_saved_sessionbranch and worker-modecreate-with-name paths still double-validate.Note: intentionally no Linear ticket for this cleanup stack, so that check stays red.
Note
Medium Risk
RPC calls and agent waits can hang indefinitely if the child stays alive without responding; supervised renames and single-send messaging change failure and delivery semantics in multi-process setups.
Overview
This PR fixes three daemon/RPC reliability issues and adds regression tests.
Supervised session renames: In worker mode,
rename,rename_saved_session(active session), andset_session_namenow callsetStateSessionNameForCommand, which applies the name viaapplyStateSessionNamewithout a second local availability check. Standalone daemons still usesetStateSessionNamewith reservation and validation.Remote agent messages:
sendRemoteAgentSessionMessageonly retries connect/hello; once connected it sends once. If the connection drops after the supervisor accepted the message, the error surfaces instead of resending (avoids duplicate delivery).RPC client:
RpcClientremoves default timeouts onsend,refine,waitForIdle, andcollectEvents. Transport failures (spawn error, stdout close,stop) reject pending requests and event waiters throughfailPendingOperations.promptvalidates the RPC response;promptAndWaituses cancellable event collection so a failed prompt does not leave waiters hanging.Reviewed by Cursor Bugbot for commit 1cb0f22. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Trust supervisor-approved session renames in worker mode and remove RPC client timeouts
AgentDaemonnow applies supervisor-approved session names directly viaapplyStateSessionNamewithout re-validating; non-worker mode keeps the existing reservation flowRpcClientfor long-running RPC commands and agent turns; requests now resolve on response or reject on transport failure/stopRpcClientlatchestransportError, fails all pending requests and event waiters on spawn/pipe/close failures, and includes stderr contextsendRemoteAgentSessionMessageto send remote agent messages exactly once; if the connection drops after the supervisor receives the message but before the response, the call fails without retryingRpcClient.sendmeans hung child processes (stdin open, no response) will block indefinitely instead of timing out; verify callers handle the new failure-on-transport-error behavior and thatprompt()now throws on command-level failures viagetData(...)📊 Macroscope summarized 1cb0f22. 5 files reviewed, 2 issues evaluated, 1 issue filtered, 1 comment posted
🗂️ Filtered Issues
packages/coding-agent/src/modes/daemon/daemon-mode.ts — 0 comments posted, 1 evaluated, 1 filtered
rename_saved_sessionvalidatescommand.sessionPathin the supervisor but, for an active session, forwards the rename to the worker selected byactiveSessionId. A client can supply the path of a different session, so the supervisor checks the wrong family/name scope; this new worker-mode path then skips local validation and renames the active session to a duplicate sibling name. Require the supplied path to match the routed active session or validate using that session's actual file. [ Failed validation ]Linear: ENG-5648
Supersedes #1702 (recreated as a plain PR against
main; GitHub's stack lock prevented retargeting the stacked PR).