Skip to content

fix: detach polled cloud handoffs at session end instead of cancelling them - #18

Open
hessabi wants to merge 1 commit into
TheOnlyFusionCube:mainfrom
hessabi:fix/session-end-handoff-cancel
Open

hessabi wants to merge 1 commit into
TheOnlyFusionCube:mainfrom
hessabi:fix/session-end-handoff-cancel

Conversation

@hessabi

@hessabi hessabi commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

Follow-up requested on #15: the session-end hook cancelled --wait handoffs 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 --wait and --background the pid is the local polling companion. So the job was recorded as cancelled while the remote Devin session kept running and billing, and status stopped refreshing it. Plain detached handoffs (no pid) were already left alone on purpose.

What changed

  • For a handoff with a cloudSessionId, the hook now kills only the local poller and moves the job to the state a plain handoff already has: status detached, pid null, phase "poller stopped at session end".
    • It never claims a stop that didn't happen.
    • The job stays active and status keeps refreshing it.
    • /devin:cancel still stops the remote session (cancel_pending, then cancelled once confirmed).
  • The hook makes no network calls, since SessionEnd has a short timeout.
  • Local jobs, and a handoff with no cloud session yet, keep the current behavior: killed and cancelled.
  • Race safety: the hook waits up to 1s for the killed poller to exit, then re-reads state through updateState, and only detaches a job that is still running. A terminal state the poller wrote while shutting down is kept.
  • The README SessionEnd bullet is updated.

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.mjs spawns a real stand-in poller and runs the hook with a SessionEnd payload. It checks:

  • the poller is killed and the job is detached and active, in both state and the job file, with zero API requests;
  • a local task is still cancelled;
  • handoffs from another session, and handoffs already detached, are untouched;
  • a handoff without a cloud session is still cancelled;
  • a terminal state written during the kill is preserved.

Full suite: 113/113.

@TheOnlyFusionCube TheOnlyFusionCube left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants