Repository navigation
Conversation
TheOnlyFusionCube
left a comment
There was a problem hiding this comment.
Thanks for following up on #15. The successful shutdown path moves polled cloud handoffs to an active/refreshable detached state without making network calls, preserves terminal states during shutdown, and leaves local cancellation intact. I verified the Test workflow passed (113/113 on Node 18, 20 and 22). The HOL Scanner failure is the previously identified baseline finding addressed in separate PR #17, not evidence of a new issue here.
One correctness blocker remains: if local process termination fails or exceeds the 1-second wait, the hook still reports that the poller stopped and erases its PID. I reproduced this with a SIGTERM-ignoring stand-in process using the PR's termination/detach logic: after the timeout, the process remained alive while both job records said detached, pid: null, and poller stopped at session end. This is a focused reproduction of the logic, not a claim that the failure occurs in normal operation. The still-running poller may subsequently overwrite state; in particular, its polling error path can mark the cloud job failed, making /devin:status stop refreshing an active remote session. The lost PID also prevents /devin:cancel from killing that local poller, although its remote stop path is still available while the job remains active.
Please fix the unconfirmed-stop path and add focused failure/timeout regression coverage before merge. Keep remote cloud sessions active, visible and cancellable when the local poller cannot be confirmed dead, with consistent state.json/job-file persistence and truthful phase/log output; preserve confirmed remote-stop semantics from #15. Do not add network requests to SessionEnd. I'll re-review once those invariants are covered.
| // Ignore teardown failures during session shutdown. | ||
| } | ||
| if (isPolledCloudHandoff(job)) { | ||
| waitForExit(job.pid); |
There was a problem hiding this comment.
Blocking – unchecked termination outcome. terminateProcessTree(job.pid) can throw or report delivered: false; waitForExit(job.pid) returns false on timeout/EPERM. Neither result is used, so lines 125–136 unconditionally clear pid and claim poller stopped while a SIGTERM-resistant poller may still be running. It can continue upsertJob/writeJobFile (including recording failed after a polling error) and /devin:cancel loses the local PID. Please distinguish confirmed exit from unconfirmed exit, and only clear the PID/claim a stop after confirmation. For an unconfirmed stop, keep the cloud job active/refreshable, retain a means to kill the poller later and persist an accurate state/phase in both files. Simply retaining running is also insufficient if the PID subsequently exits: detectDeadJob would mark failed before cloud refresh. Add a SIGTERM-ignoring poller test (longer than 1s), plus a simulated termination error/undelivered kill; assert a bounded hook, no HTTP calls, state/job-file consistency, continued /devin:status refresh, and correct /devin:cancel remote-stop behavior.
Follow-up requested on #15: the session-end hook cancelled
--waithandoffs without stopping the remote session.What was wrong: SessionEnd killed every pid-bearing active job of the ending Claude session and marked it cancelled. For
handoff --waitand--backgroundthe pid is the local polling companion. So the job was recorded as cancelled while the remote Devin session kept running and billing, andstatusstopped refreshing it. Plain detached handoffs (no pid) were already left alone on purpose.What changed
cloudSessionId, the hook now kills only the local poller and moves the job to the state a plain handoff already has: statusdetached,pidnull, phase "poller stopped at session end".statuskeeps refreshing it./devin:cancelstill stops the remote session (cancel_pending, thencancelledonce confirmed).updateState, and only detaches a job that is stillrunning. A terminal state the poller wrote while shutting down is kept.Why detached rather than stopping the remote session: ending a Claude session shouldn't silently terminate cloud work, and the hook can't confirm a remote stop within its time budget. Detached matches how plain handoffs already behave, and keeps the job visible and cancellable.
Tests: new
tests/session-end-hook.test.mjsspawns a real stand-in poller and runs the hook with a SessionEnd payload. It checks:Full suite: 113/113.