Skip to content

fix: graceful shutdown of debug adapter on stop #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AlmogBaku merged 2 commits into from
Mar 13, 2026
Merged

fix: graceful shutdown of debug adapter on stop #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e90748
fix: graceful shutdown of debug adapter on stop
AlmogBaku Mar 13, 2026
1f04997
fix: capture adapterCmd in local var to prevent race
AlmogBaku Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1267,10 +1267,21 @@ func (d *Daemon) stopSession() {
_ = c.DisconnectRequest(true)
c.Close()
}
if d.adapterCmd != nil && d.adapterCmd.Process != nil {
_ = d.adapterCmd.Process.Kill()
_ = d.adapterCmd.Wait()
d.adapterCmd = nil
cmd := d.adapterCmd
d.adapterCmd = nil
if cmd != nil && cmd.Process != nil {
// Give the adapter time to shut down gracefully before hard-killing
done := make(chan struct{})
go func() {
_ = cmd.Wait()
close(done)
}()
select {
case <-done:
case <-time.After(3 * time.Second):
_ = cmd.Process.Kill()
<-done
Comment on lines +1273 to +1283
Copy link
Copy Markdown

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot Mar 13, 2026

Choose a reason for hiding this comment

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

Action required

1. Racy adaptercmd access 🐞 Bug ⛯ Reliability

stopSession launches a goroutine that calls d.adapterCmd.Wait() and later dereferences
d.adapterCmd.Process.Kill() without synchronizing access to adapterCmd. Because dispatch explicitly
allows stop to run without cmdMu while startAdapter assigns d.adapterCmd concurrently, this can
race, wait/kill the wrong process, or panic on nil dereference.
Agent Prompt 
## Issue description
`stopSession()` spawns a goroutine that reads `d.adapterCmd` (`Wait()`) and later dereferences `d.adapterCmd.Process` (`Kill()`) without any synchronization, while `startAdapter()` can concurrently assign `d.adapterCmd` and `dispatch()` intentionally allows `stop` to run without `cmdMu`. This creates a data race and can cause waiting/killing the wrong process or panics.

## Issue Context
- `stop`/`pause` bypass `cmdMu`, so `stopSession()` can run concurrently with `handleDebug()` / `startAdapter()`.
- `stopSession()` currently references `d.adapterCmd` from inside a goroutine, which can observe a different value than the one checked.

## Fix Focus Areas
- daemon.go[39-92]
- daemon.go[441-454]
- daemon.go[1261-1284]
- daemon.go[1301-1316]

## Suggested approach
1. Add a dedicated mutex (e.g., `adapterMu sync.Mutex`) to `Daemon` to guard `adapterCmd` (and optionally `adapterAddr`).
2. In `startAdapter()`, lock `adapterMu` when assigning `d.adapterCmd`.
3. In `stopSession()`, lock `adapterMu`, copy `cmd := d.adapterCmd`, set `d.adapterCmd = nil`, unlock, then operate on `cmd` only.
4. In the goroutine, call `cmd.Wait()` (not `d.adapterCmd.Wait()`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

}
}
if d.cleanupFn != nil {
d.cleanupFn()
Expand Down
46 changes: 46 additions & 0 deletions daemon_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ package dap
import (
"encoding/json"
"fmt"
"os/exec"
"strings"
"testing"
"time"
)

func TestOutputBufferBoundedAtWrite(t *testing.T) {
Expand Down Expand Up @@ -87,6 +89,50 @@ func TestTempBinaryCleanup_NilSafe(t *testing.T) {
d.stopSession() // should not panic
}

func TestStopSessionGracefulShutdown(t *testing.T) {
// Process that exits quickly on its own — should not need SIGKILL
cmd := exec.Command("sleep", "0")
if err := cmd.Start(); err != nil {
t.Fatal(err)
}
d := &Daemon{adapterCmd: cmd}

start := time.Now()
d.stopSession()
elapsed := time.Since(start)

if elapsed > 2*time.Second {
t.Errorf("graceful shutdown took too long: %v (expected < 2s)", elapsed)
}
if d.adapterCmd != nil {
t.Error("adapterCmd should be nil after stopSession")
}
}

func TestStopSessionKillsHangingProcess(t *testing.T) {
// Process that ignores signals and hangs — should be killed after timeout
cmd := exec.Command("sleep", "60")
if err := cmd.Start(); err != nil {
t.Fatal(err)
}
d := &Daemon{adapterCmd: cmd}

start := time.Now()
d.stopSession()
elapsed := time.Since(start)

// Should take ~3s (the timeout), not 60s
if elapsed > 5*time.Second {
t.Errorf("expected kill after ~3s, took %v", elapsed)
}
if elapsed < 2*time.Second {
t.Errorf("expected to wait for timeout, but finished in %v", elapsed)
}
if d.adapterCmd != nil {
t.Error("adapterCmd should be nil after stopSession")
}
}

func TestParseBreakpointSpec(t *testing.T) {
tests := []struct {
name string
Expand Down
Loading