fix(proxy): thread per-request DAP timeout through parent/worker/socket for evaluate and redefine (fixes #142)#148
Merged
Conversation
…et for evaluate and redefine (fixes #142) evaluate_expression and redefine_classes previously rode three stacked hard timeouts (parent 35s, worker request-tracker 30s, socket 30s), so a long-running expression spuriously failed even though nothing was wrong. Unlike step/pause there is no async completion path - the DAP response IS the result - so this threads a real timeout override end to end: - tool arg `timeout` (ms) on evaluate_expression + redefine_classes - SessionManagerOperations validates (positive, finite) and clamps to 600000ms - ProxyManager.sendDapRequest gains options.timeoutMs; the parent timer is now derived ((override ?? 30000) + 5000 margin) instead of hardcoded 35000, preserving the invariant that the worker/socket timeout fires first - DapCommandPayload.timeoutMs carries the override over IPC; the message parser drops non-finite/non-positive values defensively - the worker forwards it to the request tracker and the DAP socket (which already supported per-request timeouts), and timeout failures now report the actual deadline ("timed out after Ns") - timeout failures on these two tools append a hint naming the `timeout` arg Defaults are unchanged when the argument is omitted (30s worker/socket, 35s parent). Launch-request slowness is explicitly out of scope, and DAP has no cancel - an expired evaluate keeps executing in the debuggee. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 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.
Fixes #142.
Problem
evaluate_expression(andredefine_classes) rode three stacked hard timeouts — parent 35s, worker request-tracker 30s, socket 30s — so a long-running expression spuriously failed even though nothing was wrong. Unlike the step/pause case (#144) there is no async completion path: the DAP response is the result, so this needs a real threaded timeout override, not a "pending" marker.Change
Thread a per-request
timeoutMsend to end, preserving layer ordering (worker/socket fire first, parent last):timeoutis omitted (30s worker/socket, 35s parent) — the parent timer is now derived instead of the hardcoded35000, and its error reports the actual deadline.Request 'evaluate' timed out after 30s.timeoutargument (ErrorMessages.dapRequestTimeoutHint).IRequestTracker.track'stimeoutMsis now optional, matching both implementations.Verification
timeoutMsout of the IPC payload when unset; worker forwards to tracker + socket incl. queued js-debug commands; parser normalization; session-manager validation/clamp/hint and stackTrace pre-request keeping the default; server schema + forwarding for both tools).evaluate_expressionof a 45s-sleeping expression withtimeout: 120000→ succeeds with the correct result where the old stack failed at 30s; the same expression with defaults still fails truthfully at 30s, now with the hint:Request 'evaluate' timed out after 30s. If the operation is expected to take this long, retry with a larger 'timeout' (ms) argument. Note the operation may still be running in the debuggee.Out of scope (noting on #142)
cancelsupport — an expired evaluate keeps running in the debuggee after the timeout fires.Note for merging
PR #147 (#143, attach
verifyTimeout) is still open and also touchessrc/utils/error-messages.ts,src/server.ts, andsrc/session/session-manager-operations.tsin different regions — whichever merges second may need a trivial conflict resolution (both append newErrorMessagesfactories).🤖 Generated with Claude Code