drain() future completes true even when the drain timed out

[scottf]

Subscription.drain(timeout) returns a future that completes with whether the drain succeeded. For subscriptions it always completes true, including when the drain timed out with messages still queued. Dispatchers are not affected.

Mechanism

NatsConsumer.drain(Duration) runs its wait loop, then cleans up, then completes the future:

while (NatsSystemClock.nanoTime() - startTime < timeoutNanos && !Thread.interrupted()) {
    if (this.isDrained()) {
        break;
    }
    Thread.sleep(1);
}

this.cleanUpAfterDrain();
...
finally {
    tracker.complete(this.isDrained());
}

NatsSubscription.cleanUpAfterDrain() calls connection.invalidate(this), which nulls incoming. So the isDrained() inside the finally is always evaluated against a subscription whose queue is already gone, and:

boolean isDrained() {
    return isDraining() && this.getPendingMessageCount() <= 0;
}

getPendingMessageCount() returns -1 once the queue is gone, so the comparison is true no matter how much was left unread. The loop's own isDrained() calls are fine — those run before the cleanup — but their result is discarded.

This is long-standing rather than new. Before #1615 the accessor returned 0 for a missing queue instead of -1, so the comparison was 0 == 0 and the outcome was identical. #1615 changed the sentinel and kept the behaviour deliberately, to avoid flipping the contract as a side effect of a race fix.

Dispatchers are unaffected because NatsDispatcher.incoming is final and never nulled, so their count stays real.

Suggested fix

Capture the answer while the queue still exists, and complete the future with that:

boolean drained = false;
try {
    while (...) {
        if (this.isDrained()) {
            drained = true;
            break;
        }
        Thread.sleep(1);
    }
    this.cleanUpAfterDrain();
}
...
finally {
    tracker.complete(drained);
}

An explicit flag is better than re-deriving it from queue state, since the queue is deliberately torn down before the future completes.

Why this needs its own change

It flips a public contract. Callers who today see true from every drain will start seeing false when a drain genuinely times out — which is the point, but it can surface as new failures in code that was silently relying on the old answer. It wants its own release note, and DrainTests should grow a case that drains with an unreachable server or a too-short timeout and asserts false.