[Test] Wait for cancellation before assertions

[ywangd]

Resolves: #134277

[elasticsearchmachine]

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

[ywangd]

It turns out we still need the latch. It was actually part of the original issue, i.e. the cancellation can comes in after all node responses are collected and right before the final response is sent. In this case, the final response is short circuited to be an exception while the cancellation listener is still doing work.

[DaveCTurner]

Hmm ok I think I see, so you mean the final response can be sent in between these two lines:

elasticsearch/server/src/main/java/org/elasticsearch/action/support/CancellableFanOut.java

Lines 78 to 80 in a59c182

	semaphore.release();
	// finally, release refs to all the per-item listeners (without calling onItemFailure, so this is also fast)
	cancellableTask.notifyIfCancelled(itemCancellationListener);

On reflection this seems kinda surprising if not an outright bug: we generally prefer to delay sending the response until everything is released.

[ywangd]

Not the above lines. The final response (cancellation) can be sent at this line

elasticsearch/server/src/main/java/org/elasticsearch/action/support/nodes/TransportNodesAction.java

Line 179 in 6884dbb

cancellableTask.notifyIfCancelled(l);

before the node responses are released by this line

elasticsearch/server/src/main/java/org/elasticsearch/action/support/nodes/TransportNodesAction.java

Line 120 in 6884dbb

Releasables.wrap(Iterators.map(drainedResponses.iterator(), r -> r::decRef)).close();

A potential alternative fix is to add a similar synchronized block to drain node responses right before the first line linked above. What do you think?

[DaveCTurner]

Ah yeah sorry this is all quite a tangle. I don't think it'll work to drain responses in onCompletion - we could already have drained them in the listener within addReleaseOnCancellationListener but still not quite released them yet.

On reflection it looks like there's other ways we can complete the final listener before releasing all the node-level responses, e.g. here:

elasticsearch/server/src/main/java/org/elasticsearch/action/support/nodes/TransportNodesAction.java

Lines 171 to 173 in 74fd66c

	try (var ignored = Releasables.wrap(Iterators.map(responses.iterator(), r -> r::decRef))) {
	newResponseAsync(task, request, actionContext, responses, exceptions, l);
	}

Let's leave this alone then; I appreciate the comment in the test calling out that this is slightly odd.

[DaveCTurner]

LGTM

[ywangd]

@elasticmachine update branch

[ywangd]

@elasticmachine update branch

[elasticmachine]

There are no new commits on the base branch.

[elasticsearchmachine]

💔 Backport failed

Status	Branch	Result
❌	9.0	Commit could not be cherrypicked due to conflicts
❌	8.19	Commit could not be cherrypicked due to conflicts
❌	9.1	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 134532

[ywangd]

💚 All backports created successfully

Status	Branch	Result
✅	9.1
✅	9.0
✅	8.19

Questions ?

Please refer to the Backport tool documentation