fix: graceful shutdown of debug adapter on stop

[AlmogBaku]

Summary

• On dap stop, wait up to 3s for the debug adapter to exit gracefully after DisconnectRequest(terminateDebuggee=true) before resorting to SIGKILL
• Previously the adapter was hard-killed immediately, which could prevent clean debuggee shutdown

Test plan

• TestStopSessionGracefulShutdown — verifies fast-exiting processes aren't unnecessarily delayed
• TestStopSessionKillsHangingProcess — verifies hanging processes get killed after 3s timeout
• All existing tests pass (make all)

🤖 Generated with Claude Code

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Graceful shutdown of debug adapter on stop

🐞 Bug fix

Walkthroughs

Description

• Implement graceful shutdown of debug adapter with 3-second timeout
• Send DisconnectRequest before terminating adapter process
• Add two test cases for graceful and forced shutdown scenarios

Diagram

flowchart LR
  A["stopSession called"] --> B["Send DisconnectRequest"]
  B --> C["Wait for adapter exit"]
  C --> D{"Exit within 3s?"}
  D -->|Yes| E["Clean shutdown"]
  D -->|No| F["Send SIGKILL"]
  F --> G["Wait for process"]
  E --> H["Clear adapterCmd"]
  G --> H

Loading

File Changes

1. daemon.go 🐞 Bug fix +12/-2

Implement graceful shutdown with timeout fallback

• Replace immediate SIGKILL with graceful shutdown mechanism
• Launch goroutine to wait for adapter process exit
• Use select with 3-second timeout to enforce hard kill if needed
• Ensure process cleanup happens in both graceful and timeout cases

daemon.go

---

2. daemon_test.go 🧪 Tests +46/-0

Add graceful and forced shutdown test cases

• Add imports for os/exec and time packages
• Add TestStopSessionGracefulShutdown to verify fast-exiting processes complete quickly
• Add TestStopSessionKillsHangingProcess to verify timeout-based killing of unresponsive processes
• Both tests verify adapterCmd is properly cleared after shutdown

daemon_test.go

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. Racy adapterCmd access 🐞 Bug ⛯ Reliability

Description

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.

Code

daemon.go[R1271-1281]

+		// Give the adapter time to shut down gracefully before hard-killing
+		done := make(chan struct{})
+		go func() {
+			_ = d.adapterCmd.Wait()
+			close(done)
+		}()
+		select {
+		case <-done:
+		case <-time.After(3 * time.Second):
+			_ = d.adapterCmd.Process.Kill()
+			<-done

Evidence

stopSession uses d.adapterCmd inside a goroutine and later again during the timeout path, but
adapterCmd is also assigned in startAdapter, and stop commands are dispatched without holding
cmdMu—so stopSession can run concurrently with startAdapter/debug command execution.

daemon.go[1261-1284]
daemon.go[1301-1314]
daemon.go[441-454]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

---

2. Tests depend on sleep 📘 Rule violation ⛯ Reliability

Description

The new tests spawn an external sleep binary, which may not exist or behave consistently across
test environments (e.g., Windows/minimal CI images), risking go test ./... failures. This makes
the added coverage less reliable for validating the shutdown behavior.

Code

daemon_test.go[R92-116]

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

Evidence

PR Compliance ID 3 requires appropriate automated tests and that go test ./... passes; the added
tests directly depend on invoking an external sleep command, which can break test execution
depending on the runtime environment.

CLAUDE.md
daemon_test.go[92-99]
daemon_test.go[112-118]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new tests rely on running the external `sleep` executable via `exec.Command("sleep", ...)`, which is not guaranteed to exist or behave consistently across environments and can cause `go test ./...` to fail.

## Issue Context
These tests are intended to validate `stopSession()` graceful shutdown vs timeout kill behavior. They should be self-contained and portable.

## Fix Focus Areas
- daemon_test.go[92-134]

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

---

ⓘ The new review experience is currently in Beta. Learn more

[qodo-free-for-open-source-projects]

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

[AlmogBaku]

Fixed in commit 1f04997. We now capture d.adapterCmd into a local cmd before the goroutine and immediately set d.adapterCmd = nil. The goroutine then operates only on the local cmd, preventing concurrent races with startAdapter(). This mirrors the existing pattern used for d.client at line 1263-1265.