fix(#500): report js attach/child-session breakpoint verification truthfully - #503
Merged
Merged
Conversation
…thfully
js-debug binds breakpoints in a child session while the parent answers
with provisional stubs (verified:false, "Unbound breakpoint", parent-space
ids). Nothing reconciled the two on the attach path: a breakpoint set
after attach_to_process raced a fire-and-forget child mirror, and one set
before attach stayed unbound-with-no-adapterId forever - even after it
fired - because js-debug answers a no-change re-send with an empty echo
(BreakpointManager returns {breakpoints:[]} when the diff is a no-op) and
emits no late bind event for it.
- MinimalDapClient hands the child's authoritative setBreakpoints
response back through the sync path (marked child-sourced via a dunder
key that survives the worker IPC, bounded 3s wait), so
syncBreakpointsForFile full-stamps verified/adapterId synchronously -
set_breakpoint on a live js attach session now returns verified:true
in its own response.
- attachToProcess gains the post-launch belt-and-braces re-sync
(+ function breakpoints and their launch-style warning); the mirror
clears and re-sets when asked for a fresh echo (forceFreshEcho, sent
only by the attach re-sync) so already-registered sets can still be
verified.
- Child-origin breakpoint events are tagged at the proxy choke point;
handleBreakpoint ignores non-child downgrades of verified records on
ANY match type (a late parent stub with a colliding integer id could
previously downgrade by id-match - the #495 signature) and only stamps
adapterId from child-origin VERIFIED events for mirroring policies, so
parent-space and provisional-stub ids never poison the store.
- Every previously-silent mirror failure mode (no active child, child
rejection, short/absent echo, unmatched synthesized event) now logs.
- syncBreakpointsForFile normalizes raw l10n keys before storing
(issue #471's last unnormalized stamp site).
Non-mirroring adapters see byte-identical behavior; all changes are
gated on mirrorBreakpointsToChild.
Fixes #500
Closes #495
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
js-debug is the only adapter that binds breakpoints in a child session (reverse
startDebugging); the parent session answerssetBreakpointswith provisional stubs —verified:false,breakpoint.provisionalBreakpoint, parent-space ids. Three structural facts combined into #500:breakpoints_synced(list_breakpoints: logpoints (and any never-paused launch) stay verified:false with no adapterId after firing #439) never fires for js — the worker's command-queueing branch skipshandleInitializedEvent, launch and attach. What heals js launch is the post-launch belt-and-braces re-sync;attachToProcesshad no equivalent, and the js handshake discards its ownsetBreakpointsresponses.storeBreakpointsswallowed rejections,emitBreakpointResultsreturned silently on an empty body, and an unmatched synthesized event was dropped at debug level.BreakpointManager.setBreakpointsreturns{breakpoints:[]}when the diff is a no-op (verified in the vendored bundle). A pre-attach breakpoint is registered inside the child via js-debug's pending-target queue during adoption, so every later identical re-send echoes[]— and js-debug emits no late bind event for it. Its verified state was unrecoverable by construction.Diagnosis was run live against the repro in issue #500 with
DAP_TRACE=1; the trace shows the child answering the adoption-time replay with its own provisional stub{id:1, verified:false}(which also stamped the parent/stub id asadapterId— the exact poisoning behind #495) and answering the post-attach re-mirror with[].#495's mechanism (proven from a launch trace): parent and child ids share one integer space (both
id:1observed in one session). The downgrade guard only protected fallback matches, so once a child id was stamped, a late parent stub with the same integer matched by id, bypassed the guard, and permanently downgraded a verified record —verified:false+ "Unbound breakpoint" +adapterIdpresent, exactly #495's signature, order-dependent under load.The fix
All behavior changes are gated on
mirrorBreakpointsToChild(js-only); non-mirroring adapters see byte-identical behavior.MinimalDapClient.sendRequest('setBreakpoints')awaits the child mirror (bounded 3s; a hung child only delays, never fails) and, when a fully-adopted child echoes the complete set, returns the child's body marked child-sourced (dunder key, survives worker IPC).syncBreakpointsForFilefull-stamps from a child-sourced response.set_breakpointon a live js attach session now returnsverified:truein its own response.attachToProcessre-sends stored breakpoints (+ function breakpoints and their launch-style warning) after the state settles, passingforceFreshEcho: on a short no-change echo the mirror clears the path and re-sets, forcing js-debug to answer with real records.breakpointevents are marked at the proxy choke point;handleBreakpointnow ignores non-child downgrades of verified records on any match type, and for mirroring policies stampsadapterIdonly from child-origin verified events — parent-space and provisional-stub ids never enter the store.syncBreakpointsForFilerunsnormalizeBreakpointMessagebefore storing (the last stamp site missing Verified JS breakpoints keep the raw provisional message 'breakpoint.provisionalBreakpoint' (untranslated key + contradicts verified:true) #471's normalization).Verification
set_breakpointreturnsverified:trueimmediately;list_breakpointsshowsverified:true+adapterIdbefore, during, and after the hit.verified:true+adapterIdright afterattach_to_processreturns; the breakpoint still fires after the clear+re-set.handleBreakpointorder permutations (P1 Docker/js: logpoint fires but list_breakpoints reports verified:false + 'Unbound breakpoint' (intermittent, did not reproduce; suspected child-session bind race) #495 id-collision regression, parent-stub-first, child unbind still applies, non-mirroring unchanged); attach re-sync (files +forceFreshEcho+ failure tolerance); child-sourced full stamp + l10n normalization.mcp-server-smoke-javascript-attach.test.tsnow pollslist_breakpointsforverified:true+adapterIdafter the hit (the blind spot js attach: breakpoints that bind and fire are reported "Unbound breakpoint" / verified:false, permanently #500 fell through), plus a new pre-attach-breakpoint test.Case study: mcp-debugger debugging mcp-debugger
The fix was verified by attaching one mcp-debugger to another mcp-debugger's proxy worker while a third client drove the #500 repro through it:
node dist/index.js http -p 3111) debugged anode --inspecttick target on behalf of an MCP driver client.node -e "process._debugProcess(<pid>)"switched one on in the running process.verified:trueat set time on a js attach session, i.e. the fix vouching for itself.set_breakpoint, the worker froze atChildSessionManager.storeBreakpoints(stack:handleCommand → handleDapCommand → sendRequest → storeBreakpoints), showing the mirror's live state (mirrorTargetIsChild:true,adoptionInProgress:false), then at the merge, where oneevaluate_expressioncaptured both truths side by side:{ "parentAnswer": { "breakpoints": [{ "id": 1, "verified": false, "message": "Unbound breakpoint" }] }, "childAnswer": { "breakpoints": [{ "id": 0, "verified": true, "line": 11 }] } }verified:true+adapterId:0— with an honest timeout warning, since the inspection had held the worker past the 35s DAP deadline.(Two side findings from the exercise are filed separately: attaching js-debug to a server that forks node children strands those children, and a worker that outlived its reaped HTTP session.)
Fixes #500
Closes #495
🤖 Generated with Claude Code