fix(#489): make JDI bridge pause idempotent on an already-suspended VM - #490
Merged
Conversation
A JVM launched with JDWP suspend=y already holds every thread at JDI suspend count 1. The #465 post-attach pause called vm.suspend() unconditionally, raising the count to 2, while continue's single vm.resume() only took it back to 1 - so the first continue succeeded but the VM never ran again: breakpoints never fired and the session was permanently stuck. Regression from #483, which was only verified live against suspend=n. handlePause now skips vm.suspend() when every thread is already suspended (and t.suspend() for an already-suspended single thread) but still emits the stopped event so the session layer's post-attach stoppedSeen wait resolves and the client state machine settles. Pausing a genuinely running VM is unchanged, which the #352 e2e test guards. No new unit test: the bridge has no JVM-free test harness and the behavior depends on live JDI suspend-count state. The four previously failing e2e tests (mcp-server-smoke-java-attach tests 1-2, mcp-server-smoke-java-redefine both) encode the exact double-suspend sequence and now pass, alongside the #352 counter-case. Fixes #489 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
2 participants
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.
Summary
Fixes #489 — a regression from #483: attaching to a JVM launched with JDWP
suspend=yleft the target permanently frozen.A
suspend=yJVM already holds every thread at JDI suspend count 1. The #465 post-attach pause calledvm.suspend()unconditionally, raising the count to 2, whilecontinue's singlevm.resume()only took it back to 1 — so the first continue "succeeded" but nothing ever ran: breakpoints never fired,waitForPausedStatetimed out, and 4 e2e tests failed (mcp-server-smoke-java-attachtests 1–2,mcp-server-smoke-java-redefineboth). #483 was verified live only againstsuspend=n, and these stdio e2e smokes don't run in CI, so it landed green.Fix
handlePausein the JDI bridge is now idempotent:vm.suspend()when every thread is already suspended (logged: "Pause: VM already fully suspended; reporting stopped without deepening suspend count"), but still emit the stopped event — that resolves the session layer's post-attachstoppedSeenwait and keeps the client state machine settled.t.suspend()only when the thread isn't already suspended.Pausing a genuinely running VM is unchanged — its threads are unsuspended, so the real suspend path executes; the #352 e2e test guards this.
Also a comment-only note on
JavaAdapterPolicy.getAttachBehaviordocumenting the idempotency contract.Testing
No new unit test: the bridge has no JVM-free test harness and the behavior depends on live JDI suspend-count state. The regression proof is the four previously-failing e2e tests, which encode the exact double-suspend sequence:
Log verification:
[SessionManager] Sent post-attach pause (threadId=0)fires for both attach tests, and theNo 'stopped' event within …mswarning no longer appears.🤖 Generated with Claude Code