fix(server): replace reassignable readyLatch with a fixed monitor to close TOCTOU race #90

Merged

fred merged 1 commit from fix/48-readylatch-race into main

2026-09-07 18:39:39 +00:00

hauschel.fred commented

2026-09-07 18:27:48 +00:00

Collaborator

Problem

HeadlessApplication.readyLatch was a volatile CountDownLatch reassigned in
reloadWorkspace(). A thread calling awaitReady() could read the field, get
a stale (already counted-down) latch instance right as reloadWorkspace()
swapped in a fresh one, and return immediately — even though import/build
was still in progress. reloadWorkspace() itself was also not guarded
against concurrent invocations.

Fix

Replaced the swappable CountDownLatch with a single, never-reassigned
monitor guarding a plain ready boolean. isImporting(), awaitReady()
and the new private setReady() all synchronize on the same monitor, so
there is no stale reference to read — a thread always observes the
current state.
Serialized reloadWorkspace() via a ReentrantLock: Eclipse's workspace
does not tolerate overlapping project remove/import/build sequences, so
concurrent reload calls now block on each other instead of interleaving.
Public signatures of isImporting(), awaitReady(long, TimeUnit) and
reloadWorkspace() are unchanged — no changes needed in
McpProtocolHandler or ProjectInfoTools.

Verification

mvn -pl org.naturzukunft.jdt.mcp -am compile — clean.
mvn clean package — product build succeeds.
tests/smoke-test.sh against the built product: 9/9 passed.
tests/lifecycle-test.sh against the built product: 5/5 passed.
Manual check against tests/fixtures/fixture-parent over stdio: fired
jdt_reload_workspace and immediately fired a non-import-safe tool call
(jdt_get_compilation_errors); the second call's response only arrived
after "Reload workspace finished — ready for requests" was logged.
Note: stdio uses a single reader thread that processes requests strictly
sequentially, so this confirms readiness-gating still works end-to-end but
cannot reproduce the original two-thread race (background import daemon vs.
a concurrent reload) in a deterministic test; the fix's correctness for that
case rests on there no longer being a swappable reference to read stale,
not on this timing-dependent reproduction.

Closes #48

## Problem `HeadlessApplication.readyLatch` was a `volatile CountDownLatch` reassigned in `reloadWorkspace()`. A thread calling `awaitReady()` could read the field, get a stale (already counted-down) latch instance right as `reloadWorkspace()` swapped in a fresh one, and return immediately — even though import/build was still in progress. `reloadWorkspace()` itself was also not guarded against concurrent invocations. ## Fix - Replaced the swappable `CountDownLatch` with a single, never-reassigned monitor guarding a plain `ready` boolean. `isImporting()`, `awaitReady()` and the new private `setReady()` all synchronize on the same monitor, so there is no stale reference to read — a thread always observes the current state. - Serialized `reloadWorkspace()` via a `ReentrantLock`: Eclipse's workspace does not tolerate overlapping project remove/import/build sequences, so concurrent reload calls now block on each other instead of interleaving. - Public signatures of `isImporting()`, `awaitReady(long, TimeUnit)` and `reloadWorkspace()` are unchanged — no changes needed in `McpProtocolHandler` or `ProjectInfoTools`. ## Verification - `mvn -pl org.naturzukunft.jdt.mcp -am compile` — clean. - `mvn clean package` — product build succeeds. - `tests/smoke-test.sh` against the built product: 9/9 passed. - `tests/lifecycle-test.sh` against the built product: 5/5 passed. - Manual check against `tests/fixtures/fixture-parent` over stdio: fired `jdt_reload_workspace` and immediately fired a non-import-safe tool call (`jdt_get_compilation_errors`); the second call's response only arrived after `"Reload workspace finished — ready for requests"` was logged. Note: stdio uses a single reader thread that processes requests strictly sequentially, so this confirms readiness-gating still works end-to-end but cannot reproduce the original two-thread race (background import daemon vs. a concurrent reload) in a deterministic test; the fix's correctness for that case rests on there no longer being a swappable reference to read stale, not on this timing-dependent reproduction. Closes #48

hauschel.fred added 1 commit

2026-09-07 18:27:48 +00:00

fix(server): replace reassignable readyLatch with a fixed monitor to close TOCTOU race e4417aa242

HeadlessApplication.readyLatch was a volatile CountDownLatch reassigned in
reloadWorkspace(). A thread reading the field and a thread reassigning it
could interleave: awaitReady() could grab a stale, already counted-down
CountDownLatch instance right as reloadWorkspace() swapped in a fresh one,
returning immediately although import/build was still in progress.
Concretely reachable via the stdio transport's single reader thread racing
the background project-import daemon thread at startup against a fast
jdt_reload_workspace call (import-safe, so allowed to run while importing).

Replace the swappable latch with a single, never-reassigned monitor guarding
a plain `ready` boolean (isImporting()/awaitReady()/setReady() all
synchronize on it), so there is no reference left to go stale. Also
serialize reloadWorkspace() itself via a ReentrantLock — concurrent reloads
must not interleave their remove/import/build sequence.

isImporting(), awaitReady(long, TimeUnit) and reloadWorkspace() keep their
signatures; McpProtocolHandler and ProjectInfoTools are unaffected.

Verified: mvn -pl org.naturzukunft.jdt.mcp -am compile, mvn clean package,
tests/smoke-test.sh (9/9) and tests/lifecycle-test.sh (5/5) against the
built product. Manually confirmed against fixture-parent via stdio that a
tool call fired immediately after jdt_reload_workspace only gets its
response after "Reload workspace finished" is logged.

Closes #48

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016tHV1dzUTdTwMLgx6yotVd