Skip to content

Refactor DAP attach: transport + connector strategies (replaces #10)#11

Merged
waltoss merged 2 commits intomainfrom
refactor/dap-transport-connector
Apr 24, 2026
Merged

Refactor DAP attach: transport + connector strategies (replaces #10)#11
waltoss merged 2 commits intomainfrom
refactor/dap-transport-connector

Conversation

@waltoss
Copy link
Copy Markdown
Contributor

@waltoss waltoss commented Apr 23, 2026

Summary

  • Replaces #10 with a design that separates transport (stdio vs TCP) from provisioning (spawn adapter vs attach to running server), eliminating the useTcpAttach boolean and the dual-mode DapClient.
  • Collapses DapSession.launch and DapSession.attach into a single runHandshake() template; the debugpy-fires-initialized-during-initialize race is now fixed structurally (listener armed before the initialize request is sent) instead of via a separate _initializedPromise field.
  • Adds a RED→GREEN integration test that spawns python -m debugpy --listen <port> --wait-for-client, attaches, and verifies a breakpoint hits inside the running loop.

Why reject #10 as-is

#10 is right about the bug but the fix bolted a second mode onto DapClient:

  • proc: Subprocess | null + tcpSocket: net.Socket | null with if (this.tcpSocket) ... else if (this.proc) branches in the constructor, writeMessage, and disconnect.
  • get pid() returns 0 for TCP (a lie — should be "no pid").
  • DapSession.attach() forks into two near-duplicated attach sequences switched on config.useTcpAttach.
  • The useTcpAttach boolean collapses two orthogonal concerns — transport vs provisioning — onto one axis.

Design

Two small interfaces, each with two implementations today:

DapTransport  (interface)         ← axis 1: wire
  ├── StdioTransport(subprocess)
  └── TcpTransport(socket)

DapConnector  (interface)         ← axis 2: provisioning
  ├── SpawnAdapterConnector(cmd)        → returns StdioTransport
  └── TcpAttachConnector(host, port)    → returns TcpTransport

DapRuntimeConfig
  launch(input):  DapConnectPlan
  attach?(target): DapConnectPlan       // undefined = no attach support

DapClient(transport)              ← no more nullables, no branched I/O
DapSession.runHandshake(plan, \"launch\"|\"attach\", args)   ← single handshake

debugpyConfig.attach() returns TcpAttachConnector(host, port) — exactly PR #10's behaviour, but declarative. javaConfig.attach() keeps SpawnAdapterConnector (the java adapter bridges DAP→JDWP). Adding Go (delve, TCP) or Ruby (rdbg, both) is now a config-file change.

RED → GREEN

New test: tests/integration/python/python-attach.test.ts

  • RED (pre-refactor main): times out with `this test timed out after 15000ms`. Pre-refactor `DapSession.attach("64596", "debugpy")` spawned `debugpy.adapter` and sent `{ pid: 64596 }` — there's no such pid, so the adapter hangs waiting.
  • GREEN (this PR): passes in ~6s (1 pass / 0 fail).

Two subtleties worth calling out

  • `python -m debugpy --listen --wait-for-client` treats the first TCP connection as THE client. You can't probe-connect to check readiness — the test watches debugpy's stderr for its "waiting for client" message instead.
  • `DapSession.waitUntilStopped()` doesn't fetch the stack when a waiter is set (the `stopped` handler skips the eager fetch, expecting the caller to do it). The test uses `buildState({ stack: true })` which calls `ensureStack()`.

Test plan

  • `bun run typecheck` clean
  • `bun run lint` clean
  • `bun test tests/integration/python/` — 17 pass / 0 fail (includes the new attach test)
  • `bun test` — 576 pass / 0 fail on a clean run (full suite is flaky across repeated invocations when leftover debugger processes accumulate, unrelated to this change)
  • RED verified against pre-refactor main (test times out)

🤖 Generated with Claude Code

Replaces the `useTcpAttach` boolean and dual-mode DapClient with two
orthogonal abstractions:

  DapTransport  — byte-level I/O (Stdio/Tcp)
  DapConnector  — transport provisioning (SpawnAdapter/TcpAttach)

Each DapRuntimeConfig now returns a DapConnectPlan (connector + request
args) from launch() and optional attach(). DapSession.launch/attach
collapse into a single runHandshake() template, and the
debugpy-fires-`initialized`-during-initialize race is now fixed
structurally (listener armed before the initialize request is sent) instead
of via a separate one-shot promise field.

Fixes PR #10's issue (adapter-on-adapter when attaching to debugpy):
debugpyConfig.attach() returns a TcpAttachConnector, so dbg speaks DAP
directly over the socket to the running debugpy — no subprocess in between.

Adds an integration test (tests/integration/python/python-attach.test.ts)
that spawns `python -m debugpy --listen <port> --wait-for-client`, attaches,
and verifies a breakpoint hits. RED on pre-refactor main (times out with
the adapter-on-adapter bug), GREEN here.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review feedback on the initial refactor:

- DapClient JSDoc: explain wire format, responsibilities, and link to the
  canonical DAP spec at microsoft.github.io/debug-adapter-protocol/overview.
  JSDoc on `pid` explaining why it's undefined for TCP transports.

- Zod-validated message envelopes: `handleMessage` now parses via
  schemas in `src/dap/protocol.ts` (ProtocolMessage / Response / Event)
  instead of casting. JSON parse and schema errors are logged via the
  existing dap logger (warn level) instead of silently swallowed.

- `handleIncomingData` extracted from the inline transport callback.

- Strongly-typed `DapClient.send`: new `DapCommandMap` / `DapEventMap` in
  `src/dap/protocol.ts` give typed args and return the response body
  directly (drops `response.body as X` at ~13 call sites in session.ts).
  Optional-args commands (`configurationDone`, `disconnect`) get a
  rest-tuple conditional type so they remain callable without `undefined`.
  Vendor extensions like Java's `redefineClasses` fall through to the
  untyped string overload.

- `DapClient.waitForEvent(name, timeoutMs)` helper replaces the
  hand-rolled `armInitializedWaiter` — listener + timer cleanup live in
  one place.

- Session features moved to a strategy pattern:
  `SessionCapabilities` → `SessionFeatures` (rename avoids collision
  with DAP's `DebugProtocol.Capabilities` on the adapter side).
  `DapRuntimeConfig.features` is now `Partial<SessionFeatures>` — runtime
  configs only list overrides on top of `DEFAULT_DAP_FEATURES`.
  `DapSession` no longer has the `isJava` conditional.

- Transport description in `wsUrl`: `dap://python/tcp(127.0.0.1:5678)` vs
  `dap://lldb/spawn(lldb-dap)` makes `dbg status` clear about whether
  the adapter is spawned or attached.

- `_runtime` narrowed via zod: new `CanonicalRuntimeSchema` derives a
  `CanonicalRuntime` union from a single const tuple. `RUNTIME_CONFIGS`
  is typed `Record<CanonicalRuntime, DapRuntimeConfig>` so adding a new
  runtime without a config is a compile error. `resolveRuntime` now
  narrows `string` → `CanonicalRuntime`.

- `getDap()` → `client` getter on `DapSession`. All ~20 call sites read
  `this.client.send(...)`. Throw semantics preserved.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@waltoss waltoss merged commit 9464da2 into main Apr 24, 2026
5 checks passed
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.

1 participant