Ensure JDI test VM cleanup after failed startup

[carstenartur]

What it does

Make the shared JDI test harness clean up owned VM/proxy processes after
failed launch, attach or program startup, and prevent a stalled JDWP
handshake from blocking the attach operation indefinitely.

The changes are confined to org.eclipse.jdt.debug.jdi.tests. They
affect the test harness, not product-side debugger connection settings
or breakpoint behavior.

Related: #990.
This infrastructure change is reviewed separately from the
breakpoint-removal fix.

Why the shared infrastructure needs to change

Failed startup can bypass normal teardown

The harness can create processes and readers before VM startup has
completed. With the JUnit TestCase lifecycle used here, an exception
from setUp() prevents tearDown() from running: setUp() is called
before the try/finally that invokes teardown.

See JUnit's TestCase.runBare() implementation.

Consequently, cleanup cannot rely exclusively on normal test teardown.
It must also run when the startup operation itself fails.

The existing shutdown path additionally assumes that the event reader
has already been created. That assumption does not hold when attaching
to the VM fails. Process destruction also previously returned without
waiting for process termination, so requesting cleanup did not establish
that the owned process had actually exited.

The cleanup belongs at the shared startup entry points rather than in
individual tests. This covers both the usual launch-and-start path and
callers of launchTargetAndConnectToVM(), including VirtualMachineTest,
without duplicating failure handling across test classes.

A retry deadline does not bound a blocked attach call

The existing connection loop checks elapsed time between failed attach
attempts, but does not supply a timeout to the connector. A peer that
accepts the socket without completing the JDWP handshake can therefore
keep an individual attach attempt blocked, preventing both the retry
check and startup cleanup from being reached.

Cleanup alone cannot address that case. The connector call also needs
a finite timeout.

These are specific lifecycle and timeout defects. This PR does not
claim that they explain every earlier CI startup failure, or that
earlier connection-refused failures had the same cause as the
deliberately simulated silent-peer scenario.

Implementation

Exception-safe startup and shutdown

launchTargetAndConnectToVM() and launchTargetAndStartProgram() invoke
cleanup when their launch, attach or program-start operations throw a
RuntimeException or Error. The original failure is rethrown, and a
cleanup failure is attached as a suppressed exception rather than
replacing it.

Shutdown tolerates an absent event reader and attempts owned-process
cleanup in a finally block, including when stopping readers or
requesting VM exit throws.

After cleanup, VM and reader references are cleared. Handles to
processes that are still alive are retained rather than discarded.
Repeated shutdown after successful cleanup does not terminate the
processes again or select another port.

The existing automatic port-selection mechanism is reused after
cleanup when target state was present. A user-supplied VM command keeps
its configured port.

Bounded process termination

The package-private TestProcessCleanup helper centralizes termination
of processes owned by the test harness.

For each live process, it requests normal termination and waits up to
five seconds. If necessary, it requests forced termination and waits
with a separate five-second deadline. Missing or already exited
processes are ignored.

Interruption does not abandon cleanup: the helper attempts forced
termination and restores the interrupt flag afterward. A failure for
one process does not prevent attempts to terminate the remaining
processes; additional failures are retained as suppressed exceptions.

A process that remains alive after forced termination produces an
IllegalStateException. Diagnostic formatting tolerates
Process.pid() throwing UnsupportedOperationException, reporting
the PID as unavailable without masking the intended cleanup failure.

Keeping this logic in a small helper allows the termination sequence,
failure aggregation and interrupt handling to be tested using process
test doubles, without depending on an overloaded or unresponsive JVM.

Attach timeout and diagnostic retention

The connection loop passes its remaining nominal five-second budget
to the connector instead of leaving the connector timeout unbounded.
Transport timeouts participate in the connection-failure handling.
Interruption detected before an attach attempt or during the retry
sleep aborts startup while preserving the interrupt flag.

When connection attempts fail, the reported error retains the last
connection failure as its cause. Additional diagnostics record the
port, VM/proxy process state and test-runtime information before
cleanup changes that state.

Behavioral impact and boundaries

This is a shared test-harness behavior change, not merely additional
regression tests.

The attach policy is stricter than before: an attempt that previously
blocked beyond the nominal retry budget can now fail with a timeout.
A slow test VM that previously connected only after exceeding that
budget may therefore fail sooner.

Normal test shutdown also changes. Owned-process termination is now
attempted after the VM exit request, and shutdown may wait for process
exit or escalate to forced termination instead of returning immediately.
Failure to terminate an owned process is reported rather than silently
ignored.

The process timeouts apply separately to each termination phase and
each process. They are not a single five-second deadline for the whole
shutdown operation, nor does this PR establish an end-to-end deadline
for every startup or shutdown action.

Reader shutdown continues to use the existing reader stop mechanism.
This change adds null-safe invocation and reference cleanup; it does
not introduce reader-thread joins or redesign reader synchronization.

The startup cleanup covers the launch, attach and program-start paths
described above. It is not a general recovery mechanism for arbitrary
failures in every test-specific setup hook.

No product code, test exclusions or existing test assertions are
changed.

How to test

Run org.eclipse.debug.jdi.tests.AutomatedSuite using the repository's
existing JDI test configuration. Both new test classes are registered
in that suite, which also performs the shared harness initialization.

The current change adds 20 regression tests.

VMStartupCleanupTest — 18 tests

The lifecycle tests inject failures during launch, attach and program
startup. They exercise the JUnit runBare() path and verify that
cleanup occurs even though the fixture's normal tearDown() is not
called after failed setup.

Coverage includes cleanup before the event reader exists, reader stop
requests and reference clearing, preservation of the original startup
failure, automatic/custom port handling, repeated shutdown, and a
subsequent startup after successful cleanup.

Process test doubles cover normal and forced termination, bounded
waits, interruption before or during cleanup, missing/already exited
processes, continued cleanup after another process fails, and
diagnostics when a PID is unavailable.

A separate test starts a real Java subprocess, waits until it signals
readiness, injects an attach failure, and verifies that startup cleanup
terminates the subprocess. This supplements the deterministic
test-double coverage with an actual process-lifecycle check.

VMConnectionTimeoutTest — 2 tests

These tests use the real JDI connector and a local socket peer that
accepts the connection and reads the JDWP handshake without replying.

They verify that the connection attempt fails within the test's
bounded wait, retains the transport timeout and pre-cleanup diagnostics,
and reaches owned-process cleanup when invoked through the startup
entry point.

The socket peer is deliberately constructed to reproduce the stalled
handshake condition. These tests do not depend on reproducing an
intermittent CI failure.

CI status

For commit bbf02c17a90b2a23563bde8d02be9574201343a0, GitHub reports
continuous-integration/jenkins/pr-head as successful:

Jenkins PR-992, build 6

This records the reported CI status for that revision. It does not
establish that all historical startup failures share one cause or that
every platform-specific process-termination behavior has been tested.

Relationship to #990

This PR does not depend on the breakpoint-removal implementation in
#990 and
contains no breakpoint-specific changes.

The intended integration order is to review and merge this
infrastructure change first, then update #990 to the resulting upstream
state. This keeps the shared test-harness behavior changes separately
reviewable and out of the final breakpoint-specific diff.

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

[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.

[SougandhS]

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

[SougandhS]

Hi @trancexpress can you have a look at this ?

[trancexpress]

Do we actually need this loop? Considering process.waitFor will be waiting for the entire timeout time?

[carstenartur]

Not for an uninterrupted wait: in that case, waitFor(timeout, unit) already waits until the process exits or the timeout expires.

The loop is there for InterruptedException. The current helper deliberately continues waiting for forced termination after an interruption, using the remaining time from the same deadline, and restores the interrupt flag afterwards. Simply removing the loop would change that behavior: the method could leave the wait without establishing that the process had exited.

That purpose should have been made explicit next to the loop.

However, with the narrower startup-cleanup scope proposed in the general comment, I would leave this new termination helper out of the PR rather than make its interrupt and termination policy part of the lifecycle fix.

[trancexpress]

I'm not too sure if we want tests for test infrastructure. Stable and passing tests is usually enough proof that the infrastructure works.

[carstenartur]

My reason for adding regression coverage is that a passing run does not exercise the failed-startup path. Successful VM starts do not establish whether cleanup happens when startup aborts before normal teardown becomes available.

That said, the current test scaffolding is broader than necessary for a focused lifecycle fix. I would reduce it to a small reproducer for the startup-cleanup gap and, where needed, a focused check that cleanup does not replace the original exception.

The main reproducer should exercise the existing program-start failure path after a real launch and attach, rather than replace the entire attach operation with a throwing test double and bypass its existing cleanup.

The acceptance criterion would be a specific missing-cleanup assertion that fails without the fix and passes with it. The test itself would also need independent final cleanup so that running it against the unfixed code does not leave a subprocess behind.

This would retain evidence for the failure path without keeping a broad test framework for the additional termination policies.

[trancexpress]

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]

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:

1. Ensure cleanup is attempted when startup fails after resources have been acquired, tolerate partially initialized state, and preserve the original startup exception.
2. 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.
3. 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…

[trancexpress]

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).