Assert collect task is not completed before fall-back to resume

[jeeminso]

Summary of the changes / Why this improves CrateDB

crate/server/src/main/java/io/crate/execution/engine/collect/sources/ShardCollectSource.java

Lines 433 to 446 in 571d6c6

	try {
	ShardCollectorProvider shardCollectorProvider = getCollectorProviderSafe(shardId);
	CompletableFuture<BatchIterator<Row>> iterator = shardCollectorProvider
	.awaitShardSearchActive()
	.thenApply(batchIteratorFactory -> batchIteratorFactory.getIterator(
	collectPhase,
	requiresScroll,
	collectTask
	))
	.exceptionallyCompose(err -> shardFailureFallbackOrRaise(err, shardId, collectPhase, collectTask, requiresScroll));
	iterators.add(iterator);
	} catch (Throwable e) {
	iterators.add(shardFailureFallbackOrRaise(e, shardId, collectPhase, collectTask, requiresScroll));
	}

batchIteratorFactory.getIterator() and shardFailureFallbackOrRaise() both eventually call collectTask.addSearcher() with the same searcherId (without checking if the searcherId is already added) causing the ShardCollectContext already added exception.

Checklist

• Added an entry in the latest docs/appendices/release-notes/<x.y.0>.rst for user facing changes
• Updated documentation & sql_features table for user facing changes
• Touched code is covered by tests
• CLA is signed
• This does not contain breaking changes, or if it does:
  • It is released within a major release
  • It is recorded in the latest docs/appendices/release-notes/<x.y.0>.rst
  • It was marked as deprecated in an earlier release if possible
  • You've thought about the consequences and other components are adapted
    (E.g. AdminUI)

[jeeminso]

I think the fall back function must check if the collect task is resume-able before resuming.

[jeeminso]

Seems like no concrete scenario that will cause this, but could add an assert.

[jeeminso]

Hi @matriv could you have a quick look if it is beneficial to add an assert here? It was my former attempt to #15520 before Moll reported steps to reproduce.

[matriv]

The assertions are only checked when running tests, and the issue was spotted with running cratedb as "blackbox", so I don't think it will benefit, but on the other hand, the assertion seems logical, so it doesn't harm to add it. Although, the collect task is used only in the else part, shouldn't we have the assertion there?

[jeeminso]

Thank you moved it.