Ensure JDI test VM cleanup after failed startup by carstenartur · Pull Request #992 · eclipse-jdt/eclipse.jdt.debug

carstenartur · 2026-09-05T11:51:09Z

What it does

Ensure that the shared JDI test harness cleans up owned VM/proxy processes when launch, attach or program startup fails.

The concrete lifecycle gap is a failure in startProgram() after a VM has been launched and attached. Such a failure escapes setUp(), and JUnit TestCase.runBare() does not invoke tearDown() when setup fails. Cleanup therefore also belongs at the shared startup entry points.

The remaining diff is limited to two files in org.eclipse.jdt.debug.jdi.tests:

AbstractJDITest attempts cleanup after failed startup, tolerates partially initialized readers, and preserves the original exception if cleanup also fails. Shutdown attempts owned-process termination in a finally block and retains handles to any processes that remain alive.
TestProcessCleanup requests normal termination, waits up to five seconds, then requests forced termination and waits with a separate five-second deadline. It continues cleanup across interruption, restores the interrupt flag, and attempts both owned processes even if one cleanup fails.

The connector receives the remaining nominal five-second connection budget, so an individual attach attempt cannot wait indefinitely for an unanswered JDWP handshake. Connection failures retain their cause and pre-cleanup process diagnostics. The limits apply to individual connection/termination operations, not to every action in the complete startup/shutdown sequence.

Following review, both added infrastructure test classes, their suite registrations and the temporary Jenkins diagnostics have been removed. The configurable termination-timeout overload used only by those tests is also removed. The existing test suite and Jenkinsfile are unchanged by the PR.

Related: #990, which contains the separate breakpoint-removal change. This PR has no product-side debugger or breakpoint changes and does not claim to explain historical CI failures or demonstrated cascades of follow-up test failures.

How to test

Run the existing JDI suite with a JDK compatible with the current Eclipse target platform (Java 27 was used locally):

mvn -Pbuild-individual-bundles -pl org.eclipse.jdt.debug.jdi.tests -am verify -Dmaven.test.failure.ignore=false

Local validation on Java 27: 256 existing JDI tests passed, with 0 failures, 0 errors and 0 skipped tests, using the Maven/Tycho command above. The initial Java-21 attempt could not initialize the suite because the current Eclipse target platform contains Java-25 class files; using Java 27 resolved that environment mismatch.

A temporary JUnit reproducer, kept outside the repository, was also run against the unchanged base harness and the updated harness. It launches the real target JVM, attaches through the real Eclipse JDI connector, and makes the existing startProgram() path time out waiting for a class-prepare event. Without the fix, the check fails because the child JVM remains alive; with the fix, it passes, verifies that the original exception is preserved, and verifies that JUnit teardown was not called. Independent final cleanup terminates the child even in the failing baseline run.

Author checklist

I have thoroughly tested my changes
The change is following the coding conventions
I have signed the Eclipse Contributor Agreement (ECA)

Assisted-by: OpenAI ChatGPT

Compare the unchanged PR eclipse-jdt#992 commit with its upstream parent using three complete JDI AutomatedSuite runs on GitHub-hosted Ubuntu/JDK 21. Preserve Maven output, XML reports and process/socket observations. Fail when no real JDI tests ran; do not alter the upstream PR branch.

Integrate df207ce as a separately reviewable prerequisite of the breakpoint-removal change in PR eclipse-jdt#990. The infrastructure commit passed upstream Jenkins PR-992 build 2, including 1474 tests and the overall build. Its JDI test bundle is copied byte-for-byte; the original seven breakpoint-change files are unchanged. Both changes share upstream base af15c26. This does not assert that the original connection-refused failures had the silent-peer cause. The combined change now needs its own upstream CI run. Merge eclipse-jdt#992 first; its commits remain distinguishable from the feature change, rather than being squashed into it. Signed-off-by: Carsten Hammer <carsten.hammer@t-online.de>

Copilot

🟡 Changes recommended

A small robustness bug in the new process-termination helper can throw UnsupportedOperationException from Process.pid() and mask the intended cleanup failure signal.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens the JDI test harness against failed VM startup/attach/program-start scenarios by ensuring owned processes and reader threads are cleaned up even when setUp() aborts, and by bounding attach/termination waits to avoid hangs.

Changes:

Add failure-safe startup cleanup in AbstractJDITest (including interrupt preservation and diagnostic retention on connect timeouts).
Introduce bounded process termination helper and new regression tests covering injected failures and a real Java subprocess.
Register the new regression tests in the JDI AutomatedSuite.

File summaries

File	Description
org.eclipse.jdt.debug.jdi.tests/tests/org/eclipse/debug/jdi/tests/VMStartupCleanupTest.java	Adds regression coverage for startup failure cleanup paths (processes, readers, ports, idempotent shutdown).
org.eclipse.jdt.debug.jdi.tests/tests/org/eclipse/debug/jdi/tests/VMConnectionTimeoutTest.java	Adds regression coverage ensuring a stalled JDWP handshake can’t hang startup and cleanup preserves diagnostics.
org.eclipse.jdt.debug.jdi.tests/tests/org/eclipse/debug/jdi/tests/TestProcessCleanup.java	Adds bounded termination utility used by the test harness to reliably stop owned processes.
org.eclipse.jdt.debug.jdi.tests/tests/org/eclipse/debug/jdi/tests/AutomatedSuite.java	Includes the new regression test classes in the automated suite.
org.eclipse.jdt.debug.jdi.tests/tests/org/eclipse/debug/jdi/tests/AbstractJDITest.java	Implements failure-safe cleanup on startup errors, bounded attach timeouts, and safer shutdown when readers/event reader may be absent.

Review details

Files reviewed: 5/5 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Keep the JDI test-infrastructure changes needed by this branch as one separately reviewable dependency snapshot. The shared harness cleans up VM/proxy processes and readers when launch, attach or program startup fails, uses bounded process termination, and bounds individual JDI attach attempts so startup cleanup remains reachable if a JDWP peer stalls during the handshake. These changes belong to eclipse-jdt#992 and are not part of the breakpoint-removal fix. After eclipse-jdt#992 is merged, eclipse-jdt#990 should be rebased onto the resulting upstream state so this dependency commit disappears from the final breakpoint-specific history. Signed-off-by: Carsten Hammer <carsten.hammer@t-online.de>

SougandhS · 2026-09-29T06:11:53Z

Hi @carstenartur, can you squash all commits to one and rebase on master ?

Breakpoint removal notifications can arrive after the associated marker has been deleted or detached. Avoid marker-dependent support and equality checks while removing installed breakpoints, guard install-count updates for both regular and class-prepare requests, and always clear target and thread references even when request cleanup reports an error. Add regression coverage for marker deletion, detached line breakpoints, and detached class-prepare breakpoints. Verify listener notification, absence of marker errors, target and thread cleanup, and successful installation of a replacement breakpoint on a repeatable loop line. Keep JDI test-harness startup cleanup separate in PR eclipse-jdt#992; it is not part of this breakpoint-specific change. Addresses eclipse-jdt#55 Signed-off-by: Carsten Hammer <carsten.hammer@t-online.de>

SougandhS · 2026-09-29T07:06:11Z

Hi @trancexpress can you have a look at this ?

trancexpress · 2026-09-29T08:01:21Z

Which particular problem in the tests is this change trying to solve? I'm aware of some problems with JDI tests but nothing where one test is causing fails in subsequent tests?

carstenartur · 2026-09-29T08:28:24Z

Which particular problem in the tests is this change trying to solve? I'm aware of some problems with JDI tests but nothing where one test is causing fails in subsequent tests?

@trancexpress I have not established that a failed JDI test is causing subsequent tests to fail, so I should not present this PR as a demonstrated fix for that.

The narrower issue I can point to in the code is cleanup after a program-start failure. After launch and attach have succeeded, startProgram() can throw, for example when the expected ClassPrepareEvent does not arrive. That exception escapes from setUp(), and JUnit's TestCase.runBare() does not call tearDown() when setUp() throws. On the current base, there is no cleanup around that startup call.

The ordinary attach-failure path already calls killVM(), so the justification should not be that every failed attach leaves its process behind. Nor have I established that the startup-cleanup gap explains the earlier CI failures.

I would propose narrowing this PR to:

Ensure cleanup is attempted when startup fails after resources have been acquired, tolerate partially initialized state, and preserve the original startup exception.
Keep the existing attach/retry and normal-shutdown behavior, rather than introducing the new connector timeout and general graceful/forced-termination helper in this change.
Reduce the tests to focused regression coverage, starting with the existing program-start failure path after a VM has actually been launched and attached. The missing-cleanup assertion should be verified to fail on the base revision and pass with the reduced fix.

That would make the cleanup defect and its evidence the focus, without requiring agreement on the additional timeout and termination policies.

Would this narrower scope be a more suitable direction for the PR?

Copilot

Copilot review overview

🟢 Approval recommended

The lifecycle and timeout changes are coherent and covered by focused regression tests without identified unresolved defects.

Review effort: Balanced
Findings: None

Resolved since last review (1)

terminationFailure(...) calls process.pid() unguarded. Process.pid() can throw…

Breakpoint removal notifications can arrive after the associated marker has been deleted or detached. Avoid marker-dependent support and equality checks while removing installed breakpoints, guard install-count updates for both regular and class-prepare requests, and always clear target and thread references even when request cleanup reports an error. Add regression coverage for marker deletion, detached line breakpoints, and detached class-prepare breakpoints. Verify listener notification, absence of marker errors, target and thread cleanup, and successful installation of a replacement breakpoint on a repeatable loop line. Keep JDI test-harness startup cleanup separate in PR eclipse-jdt#992; it is not part of this breakpoint-specific change. Addresses eclipse-jdt#55 Signed-off-by: Carsten Hammer <carsten.hammer@t-online.de>

trancexpress · 2026-09-29T18:14:34Z

Would this narrower scope be a more suitable direction for the PR?

Generally it doesn't hurt when clean-up code can handle also problems in setup, I'm just not sure its worth having the more complicated code.

I'd say the current clean-ups are OK, as they also allow more flexible calls from tests. E.g. if a test would call the launch and shutdown routines in a loop.

IMO tests for test infrastructure should be kept to a minimum, only for problems we really don't want to see. I don't think an exception in setup (highly likely caused by infrastructure problems) not having proper clean-up fits this. I'd drop the tests entirely - unless there really are cases where one failed setup causes tens if not hundreds of follow-up failures (which I expect is not the case here).

Breakpoint removal notifications can arrive after the associated marker has been deleted or detached. Avoid marker-dependent support and equality checks while removing installed breakpoints, guard install-count updates for both regular and class-prepare requests, and always clear target and thread references even when request cleanup reports an error. Add regression coverage for marker deletion, detached line breakpoints, and detached class-prepare breakpoints. Verify listener notification, absence of marker errors, target and thread cleanup, and successful installation of a replacement breakpoint on a repeatable loop line. Keep JDI test-harness startup cleanup separate in PR eclipse-jdt#992; it is not part of this breakpoint-specific change. Addresses eclipse-jdt#55 Signed-off-by: Carsten Hammer <carsten.hammer@t-online.de>

Clean up owned VM and proxy processes when launch, attach or program startup fails, including when failed JUnit setup bypasses teardown. Preserve the original failure and interruption state, tolerate missing readers, and bound normal and forced process termination. Bound individual JDI attach attempts by the remaining connection budget so that a stalled JDWP handshake cannot prevent startup cleanup. Retain connection failures and process diagnostics before cleanup changes state. Add regression coverage for startup failures, process termination, repeated shutdown, unavailable process IDs and stalled JDWP handshakes. Mark intentionally unused invocation-handler parameters explicitly. Keep these changes confined to the JDI test harness. They are reviewed separately in eclipse-jdt#992 and are not a prerequisite for the breakpoint regression tests in eclipse-jdt#990. Signed-off-by: Carsten Hammer <carsten.hammer@t-online.de>

Remove tracked breakpoints by identity after their marker is deleted or detached. Guard installation-count updates with markerExists() and clear target and current-thread references even when request cleanup fails. Continue removing remaining JDI requests after a deregistration CoreException. Report the original exception afterwards and preserve additional failures as suppressed exceptions, rather than silently leaving active requests behind. Keep the marker-deletion and both detached-marker regression tests. Record removals by identity in the global test listener and reject unexpected error-log entries during removal. Add a real-VM regression that injects deregistration failures and checks all captured requests, target/thread cleanup, preserved errors, repeat removal and a working replacement breakpoint. The separate JDI test-harness change is reviewed in PR eclipse-jdt#992 and is not part of this breakpoint-specific commit. Addresses eclipse-jdt#55 Signed-off-by: Carsten Hammer <carsten.hammer@t-online.de>

Copilot

🟢 Approval recommended

The lifecycle changes are coherent, exception-safe, and comprehensively covered by focused regression tests.

0 open findings

🧠 Review effort: Balanced

Remove tracked breakpoints by identity after their marker is deleted or detached. Guard installation-count updates with markerExists() and clear target and current-thread references even when request cleanup fails. Continue removing remaining JDI requests after a deregistration CoreException. Complete breakpoint-side filter and installation cleanup before reporting the first failure, preserving later cleanup errors as suppressed exceptions. Clear installed-target state independently of plugin notification and remove all matching instance filters. Retain the marker-deletion and detached-marker regression tests. Record removals by identity in the global test listener and reject unexpected error-log entries. Real-VM failure-path tests verify request deletion, state on both sides, removal and installation notifications, repeated removal, and reinstalling and hitting the very same breakpoint. Also verify that a change-notification failure cannot mask the original request-deregistration failure or skip the removal notification. The separate JDI test-harness change is reviewed in PR eclipse-jdt#992 and is not part of this breakpoint-specific commit. Addresses eclipse-jdt#55 Signed-off-by: Carsten Hammer <carsten.hammer@t-online.de>

trancexpress

Please remove the added tests.

The actual test infrastructure change handles a lot of corner cases. I'm not sure how valuable that is for test code, assuming the test code runs in a CI environment that is anyway cleaned up after a test/build job. But I guess some extra clean-ups won't hurt.

Clean up owned VM and proxy processes when launch, attach or program startup fails, including when failed JUnit setup bypasses teardown. Preserve the original failure and interruption state, tolerate missing readers, and bound normal and forced process termination. Bound individual JDI attach attempts by the remaining connection budget so a stalled JDWP handshake cannot prevent startup cleanup. Retain the connection failure and process diagnostics before cleanup changes state. Follow review by keeping the existing JDI suite unchanged, omitting the added infrastructure tests and temporary Jenkins diagnostics, and removing the configurable process timeout entry point used only by those tests. Explain why the forced-exit wait keeps its deadline across interruptions. The breakpoint cleanup implementation is reviewed separately in eclipse-jdt#990. Signed-off-by: Carsten Hammer <carsten.hammer@t-online.de> Assisted-by: OpenAI ChatGPT

trancexpress · 2026-10-21T05:59:05Z

I'll be merging this in a few hours if there are no objections.

carstenartur mentioned this pull request Sep 5, 2026

Bound JDI test attach attempts by the startup deadline carstenartur/eclipse.jdt.debug#60

Closed

2 of 3 tasks

carstenartur marked this pull request as ready for review September 5, 2026 20:36

Copilot AI lite review requested due to automatic review settings September 5, 2026 20:36

Copilot started reviewing on behalf of carstenartur September 5, 2026 20:37

View session

Copilot AI reviewed Sep 5, 2026

View reviewed changes

Comment thread org.eclipse.jdt.debug.jdi.tests/tests/org/eclipse/debug/jdi/tests/TestProcessCleanup.java Resolved

carstenartur mentioned this pull request Sep 5, 2026

Clean up breakpoints after marker loss #990

Open

2 of 3 tasks

carstenartur mentioned this pull request Sep 11, 2026

Stale Breakpoints and marker mismatch #55

Open

carstenartur force-pushed the fix-jdi-test-startup-cleanup branch from bbf02c1 to 7b0eba7 Compare September 29, 2026 06:29

SougandhS requested a review from trancexpress September 29, 2026 07:03

trancexpress reviewed Sep 29, 2026

View reviewed changes

Comment thread org.eclipse.jdt.debug.jdi.tests/tests/org/eclipse/debug/jdi/tests/AbstractJDITest.java Resolved

trancexpress reviewed Sep 29, 2026

View reviewed changes

Comment thread org.eclipse.jdt.debug.jdi.tests/tests/org/eclipse/debug/jdi/tests/TestProcessCleanup.java Resolved

trancexpress reviewed Sep 29, 2026

View reviewed changes

Comment thread org.eclipse.jdt.debug.jdi.tests/tests/org/eclipse/debug/jdi/tests/VMStartupCleanupTest.java Resolved

iloveeclipse requested a balanced review from Copilot September 29, 2026 11:26

Copilot started reviewing on behalf of iloveeclipse September 29, 2026 11:27

View session

Copilot AI reviewed Sep 29, 2026

View reviewed changes

carstenartur force-pushed the fix-jdi-test-startup-cleanup branch 2 times, most recently from 7668015 to abe5f9f Compare October 9, 2026 08:03

SougandhS requested a balanced review from Copilot October 9, 2026 08:25

Copilot started reviewing on behalf of SougandhS October 9, 2026 08:26

View session

Copilot AI reviewed Oct 9, 2026

View reviewed changes

SougandhS requested a review from trancexpress October 9, 2026 08:35

carstenartur force-pushed the fix-jdi-test-startup-cleanup branch from abe5f9f to a6a7cd5 Compare October 9, 2026 09:19

trancexpress requested changes Oct 15, 2026

View reviewed changes

carstenartur force-pushed the fix-jdi-test-startup-cleanup branch from a6a7cd5 to 8a9f1a0 Compare October 20, 2026 01:36

trancexpress reviewed Oct 20, 2026

View reviewed changes

Comment thread org.eclipse.jdt.debug.jdi.tests/tests/org/eclipse/debug/jdi/tests/TestProcessCleanup.java Resolved

trancexpress force-pushed the fix-jdi-test-startup-cleanup branch from 8a9f1a0 to abfa743 Compare October 20, 2026 18:15

trancexpress approved these changes Oct 20, 2026

View reviewed changes

trancexpress merged commit bd20019 into eclipse-jdt:master Oct 21, 2026
13 checks passed

Conversation

carstenartur commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What it does

How to test

Author checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

SougandhS commented Sep 29, 2026

Uh oh!

SougandhS commented Sep 29, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trancexpress commented Sep 29, 2026

Uh oh!

carstenartur commented Sep 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

trancexpress commented Sep 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

trancexpress left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

trancexpress commented Oct 21, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carstenartur commented Sep 5, 2026 •

edited

Loading