fix(java): enforce immediate timeouts

[avifenesh]

Summary

• enforce immediate Java-side timeouts and drop timed-out callbacks before JNI conversion
• propagate request timeout to async registration and add native timeout markers with late-response cleanup
• add timeout integration coverage and avoid false blocking-command timeouts in existing tests

Performance

• batch/getset (100B/1KB) runs remained similar or better than baseline; 1KB at 65k/70k with timeout=20s + inflight=50k matched or improved vs 2.2.4

Additional fixes

• SpotBugs cleanup: thread-safe temp dir init in glide.ffi.resolvers.NativeUtils and safer concurrency handling in glide.managers.ConnectionManager

Closes #5263

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b1210a26b1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[avifenesh]

Addressed this: batches now pass the per-batch timeout into async registration, so Java-side orTimeout matches BatchOptions.timeout. CommandManager forwards BaseBatchOptions.timeout, and GlideCoreClient uses it when registering the batch future (fallbacks to client timeout if unset).

[avifenesh]

CI failures were due to batch timeout tests expecting while produced . Updated to schedule a timeout that completes with Glide's TimeoutException and only mark native timeout when we set that exception.

[avifenesh]

CI failures were due to batch timeout tests expecting glide.api.models.exceptions.TimeoutException while CompletableFuture.orTimeout produced java.util.concurrent.TimeoutException. Updated AsyncRegistry to schedule timeouts that complete with Glide's TimeoutException and only mark native timeout when we set that exception.

[avifenesh]

Addressed in 9a4db9e:

• @jduo: Done
• @xShinnRyuu on corrId: Leftovers, good catch
• @xShinnRyuu on test helper: Done

[avifenesh]

Done

[shohamazon]

Few comments, whats the most concerning is the blocking commands issue

[avifenesh]

@shohamazon All review comments have been addressed. Here's a summary of the changes:

Key Fixes

1. Cancellable Timeouts (Main Concern)

Replaced delayedExecutor with ScheduledExecutorService + ScheduledFuture:

• Timeout tasks are now properly tracked and cancelled when requests complete
• No more task accumulation - cancelled tasks are removed from the scheduler
• Added proper cleanup in shutdown() and reset() methods

2. Blocking Commands (Critical Bug Fix)

Fixed GlideClusterClient.wait() and waitaof() to use submitBlockingCommand instead of submitNewCommand:

• These commands have their own built-in timeouts that Rust handles
• The Java-side timeout was incorrectly being applied, causing premature timeouts
• This was the root cause of the wait_timeout_check cluster test failures

3. Exception Handling

Changed ConnectionManager to use GlideException hierarchy instead of generic RuntimeException

4. Dead Code Removal

Removed unused internal method overloads:

• 3-arg register() method in AsyncRegistry
• 2-arg executeBatchAsync() in GlideCoreClient

5. Reverted Unnecessary Changes

Reverted SharedCommandTests.java to align with main - the blocking command tests work correctly with the default 250ms timeout because submitBlockingCommand skips Java-side timeouts

Test Results

2743 tests passed, 1 infrastructure failure (cluster connection refused - unrelated to code), 12 skipped

[avifenesh]

Note: These comments were written by Claude Opus 4.5 and approved by Avi.

[shohamazon]

Minor comments, mostly doc related One question I have is do we want all those blocking commands to be handled by the core or do we want to extract the timeout?
@shohamazon The timeouts should be handled by the wrappers, since the timeouts should be returned on time, this is the contract with the user. It doesn't matter what the core says; until it returns to the user it may take time.

But if you're running blocking commands with no timeout (in java side), it is not handled by the wrappers
@avifenesh

[avifenesh]

Minor comments, mostly doc related One question I have is do we want all those blocking commands to be handled by the core or do we want to extract the timeout?
@shohamazon The timeouts should be handled by the wrappers, since the timeouts should be returned on time, this is the contract with the user. It doesn't matter what the core says; until it returns to the user it may take time.

But if you're running blocking commands with no timeout (in java side), it is not handled by the wrappers @avifenesh

@shohamazon by design, they should block forever.

[avifenesh]

Addressing Review Comments - Documentation & Design Decisions

AsyncRegistry Documentation (Commits 65b20d907, 9434535ee)

All documentation has been restored and enhanced:

1. Class-level javadoc - Restored responsibilities list, updated to include "Schedule optional Java-side timeouts with cancellable tasks"

2. Field documentation - All fields now documented:

   • activeFutures - Thread-safe storage for active futures
   • timeoutTasks - Scheduled timeout tasks mapped by correlation ID for cancellation
   • clientInflightCounts - Per-client inflight request counters
   • timeoutScheduler - Single-threaded scheduler with daemon thread

3. Method documentation - All methods documented including:

   • register() - Full javadoc with @param tags
   • scheduleTimeout() - Explains cancellable timeout behavior
   • setupCleanup() - Explains atomic cleanup to avoid races
   • completeCallback() / completeCallbackWithErrorCode() - Full javadoc

4. timeoutScheduler.shutdownNow() - Added to shutdown() method

5. cancel(true) vs cancel(false) - Added inline comments explaining:

   • cancel(false) for timeout tasks - don't interrupt scheduler thread
   • cancel(true) for user futures - may be blocked waiting

---

Blocking Commands Design Decision

Question: Should blocking commands use the command's timeout argument for Java-side timeout?

Answer: No, and here's why:

1. Server handles the timeout: Commands like BLPOP key 5 tell the server to block for 5 seconds. The server returns when:

   • Data becomes available, OR
   • The timeout expires

2. Java-side timeout would interfere: If we applied a Java-side 500ms timeout to BLPOP key 5, the Java client would timeout after 500ms even though the server was correctly waiting for 5 seconds.

3. BLOCK 0 means block forever: This is intentional behavior - the user wants to wait indefinitely until data arrives. Adding Java-side timeout would break this contract.

4. Rust core handles blocking correctly: The Rust core already has proper timeout handling for blocking commands based on the command arguments.

The isBlocking() check (Commit fbb33e3d7):

// We check block != null, NOT block != null && block != 0
// Because BLOCK 0 = "block indefinitely" (still a blocking command!)
public boolean isBlocking() {
    return this.block != null;
}

---

Summary of Documentation Changes vs Main

Change	Reason
Updated "Timeouts handled by Rust"	Now Java also handles timeouts
Removed "Provide batched completion helpers"	Not accurate for new implementation
Added timeout-related docs	New functionality
Removed "Rust handles all timeout logic" comment	No longer true

No documentation was unnecessarily removed - all changes reflect the new timeout functionality.

[avifenesh]

@shohamazon To clarify the blocking command timeout flow:

For blocking commands (BLPOP, BRPOP, XREAD BLOCK, etc.):

1. User calls client.blpop(keys, 5.0) → Server blocks for up to 5 seconds
2. User calls client.blpop(keys, 0) → Server blocks forever until data arrives
3. Java-side timeout is skipped intentionally via submitBlockingCommand()

Why skip Java-side timeout for blocking commands?

The timeout is in the command itself - the server knows when to return. If we added Java-side timeout:

• BLPOP key 30 with 5s Java timeout → Java times out at 5s, but user expected 30s wait
• BLPOP key 0 with any Java timeout → Breaks "block forever" behavior

The contract:

• Non-blocking commands: Java-side timeout ensures quick failure if server is slow
• Blocking commands: Server-side timeout (from command args) is the contract with the user

This matches how other Redis/Valkey clients handle blocking commands - they let the server control the timeout.

[shohamazon]

small comments, overall LGTM 🙂

[avifenesh]

Addressing Test Review Comments

Changes Made (Commit 721f04d98)

1. Removed unnecessary cleanup loop ✅

• Removed the manual key deletion loop - test teardown handles it

2. Added blocking command test ✅

• New blocking_command_uses_server_timeout_not_java_timeout test
• Creates client with 200ms Java timeout
• Runs BLPOP with 1s server timeout
• Verifies it waits ~1 second (server timeout), NOT 200ms (Java timeout)
• Asserts result is null (key doesn't exist)

3. Removed non-deterministic test ✅

• Removed zero_timeout_uses_rust_default (didn't test anything meaningful)
• Removed rapid_requests_do_not_leak_timeout_tasks (non-deterministic assumption)
• Removed different_clients_different_timeouts (redundant)

4. Added deterministic cleanup test ✅

• Added getPendingTimeoutCount() and getActiveFutureCount() to AsyncRegistry
• New timeout_tasks_cleaned_up_after_completion test:
  • Records initial counts before operations
  • Runs 50 SET/GET pairs
  • Waits 100ms for async cleanup
  • Asserts counts return to initial values (no leaks)

Final Test Suite (4 tests, each runs standalone + cluster mode = 8 total)

1. request_completes_before_timeout - Normal operation
2. request_exceeds_timeout_throws_exception - TimeoutException on slow commands
3. blocking_command_uses_server_timeout_not_java_timeout - BLPOP uses server timeout
4. timeout_tasks_cleaned_up_after_completion - Deterministic leak detection