Fix out-of-bounds crash in ObjectStorageQueueSource::FileIterator::next - #108977

Can we reproduce the same in a more realistic way?

Reworked the regression test to reproduce this through real S3Queue operation instead of a fabricated keeper failure.

The two crash preconditions are now produced the way they happen when several consumers share one keeper path:

• The failed keeper multi is real: the test pre-creates a real processing node in keeper for one batch file (node name = sipHash64(path), same as getNodeName), so the engine's own tryMulti fails against it with a real Node exists (real code, real responses, real getFailedOpIndex). No faked Coordination::Responses.
• The non-processable file is produced by the engine's own std::nullopt reject path (the same branch taken when another table on the path already grabbed the file). The only test-only hook is a small failpoint that flips one batch entry onto that existing path; it changes no keeper or vector state itself.

To keep it deterministic on slow CI the test waits for the ObjectStorageQueueFailedToBatchSetProcessing profile event before continuing, so the failing batch is always exercised.

Verified both directions locally: without the fix the server aborts in FileIterator::next (libc++ ... index out of bounds); with the fix it survives and the queue drains.

This assertion is hiding a real recovery bug in the fallback path that this PR exercises. After the batch tryMulti fails and file_metadatas is cleared, the conflicted file is replayed through next(size_t) with file_metadata == nullptr, so it falls back to trySetProcessing (ObjectStorageQueueSource.cpp:551-555). If the foreign processing node still exists, ObjectStorageQueueUnorderedFileMetadata::setProcessingImpl returns {false, Processing} and afterSetProcessing stores Processing in the shared FileStatus cache (ObjectStorageQueueIFileMetadata.cpp:313-372).

From that point on, subsequent attempts short-circuit on state == Processing and never go back to Keeper, even after this test deletes the conflict node. In other words, the queue can leave one file permanently stuck in the local metadata cache instead of draining once the conflict disappears. I think the fix needs to keep that file retryable (or clear the cached Processing state on the individual fallback failure) rather than teaching the regression test to accept >= files_to_generate - 1.

Verified against the code, the mechanism is correct. After the batch tryMulti fails, file_metadatas is cleared (ObjectStorageQueueSource.cpp:400); the conflicted file is replayed through next(size_t) with file_metadata == nullptr -> trySetProcessing (551-555); with the foreign processing node present, ObjectStorageQueueUnorderedFileMetadata::setProcessingImpl hits the create_processing_node_idx failure branch and returns {false, Processing}, so afterSetProcessing caches Processing via updateState (ObjectStorageQueueIFileMetadata.cpp:369-372). Subsequent trySetProcessing then short-circuits on state == Processing (294-304), and reset() runs only from resetProcessing(), which needs created_processing_node == true (never set on a foreign-node conflict). So the file can stay in in-memory Processing after the conflict node is gone. Confirmed.

Two scope points:

1. It is pre-existing and independent of this PR. It needs only the failed batch multi (cond2), not the out-of-bounds compaction path this PR fixes (cond1). With a failed multi and no skipped file, file_metadatas.clear() still runs, num_successful_objects == new_batch.size() so the compaction block is skipped, and every file including the conflicted one is replayed with null metadata (454-457). The afterSetProcessing / trySetProcessing / updateState logic that produces the stuck state is untouched here, so it already reproduces on master whenever a batch multi loses a race against a foreign processing node.

2. In normal operation the stale cache is harmless: the owning consumer creates a processed node when it finishes, and filterOutProcessedAndFailed then drops the file from future listings so it never returns to this consumer. The stuck case needs the processing node to vanish without the file being processed (a crashed/abandoned consumer, or the manual zk.delete here), a real but narrow recovery gap.

So this PR stays scoped to the crash: it asserts that the abort is gone and the queue survives and drains, and >= files_to_generate - 1 states that faithfully. Clearing or retrying the cached Processing on an individual fallback failure changes shared FileStatus cache semantics and deserves its own review, so I am tracking it as a separate follow-up rather than folding it into this crash fix.

Fixed in a separate pull request: #112313

The mechanism you described is exactly right, and it is pre-existing on master and independent of this crash: after trySetProcessing loses the race for the processing node, afterSetProcessing caches the local FileStatus as Processing, the two non-processable checks treat Processing as terminal, and only the processed/failed node cleanup evicts local_file_statuses - never the stale processing node cleanup.

#112313 makes the cached Processing state a hint instead of a terminal state (FileStatus::onProcessingByAnotherProcessor), so the file is picked up again once the foreign processing node is gone, with an integration test that emulates a foreign processor releasing a file without committing it. It is kept out of this pull request so that the out-of-bounds fix here stays a minimal crash fix; the >= files_to_generate - 1 assertion can be tightened to equality once #112313 is merged.

The Changelog category is also changed to Critical Bug Fix as requested.

Closing the loop on the follow-up I committed to track above: this recovery gap is now being fixed by #112313, so I am not opening a PR of my own for it.

That fix keeps the observation of a foreign processing node in the cache but records it as a timestamped hint with a TTL, and relaxes the two non-processable checks to Processing && !isProcessingRetryable(). That is a superset of what I had planned: it also avoids probing Keeper for the file on every polling pass, and it keeps the case where the Processing state belongs to a concurrent local processor distinguishable from a foreign one, which a plain "do not cache it" approach collapses. Since it changes the same block of afterSetProcessing, a second PR on those lines would only conflict with it.

One thing that survives it and is unrelated to the behaviour: the status column of system.s3queue_metadata_cache and system.azure_queue_metadata_cache documents Processed, Processing, Failed, but the value is printed straight from FileStatus::State, which also has None (the initial value of a cache entry, and what reset writes). I am tracking that one-line description fix separately, after #112313 lands.

This code path is still unrealistic, need inject a thrown error in needed place if you say this happens in case of error

The crash needs two conditions in one batch, and both are non-throwing paths, which is exactly why the pre-fix compaction mishandled them:

• cond2, the real error, already injected: the keeper multi at ObjectStorageQueueSource.cpp:373 returns a non-ZOK code (not a thrown exception), which triggers file_metadatas.clear() at :399. The test makes this real by pre-creating an actual processing node in keeper for one batch file, so the engine's own tryMulti fails against it with a real code and real responses.
• cond1, not an error: another file in the same batch is non-processable, so num_successful_objects < new_batch.size() and the compaction at :402 runs over the cleared vector. In production this is the benign std::nullopt skip from prepareSetProcessingRequests when another consumer on the same server already holds the file (lock at ObjectStorageQueueIFileMetadata.cpp:324, or state already Processing/Processed/exhausted-Failed at :334). It never throws.

A literal throw at either site would not reproduce this: there is no try/catch between the batch loop and the compaction (the nearest is generate() at :1015), so a throw escapes next() before the compaction runs, while the out-of-bounds is inside that compaction.

The only artificial part left is how cond1's file is made non-processable (the force_skip failpoint). I can make that realistic by having a genuine second consumer mark the file Processing first and dropping the failpoint, but that turns the non-processable window into a race, so the test would need retries and risks being flaky. Which do you prefer: the two-consumer version, or keeping the failpoint for cond1 together with the real keeper error for cond2?

The unrealistic bit is gone now: the failpoint no longer sits in FileIterator::next forcing a std::nullopt. It was moved into ObjectStorageQueueIFileMetadata::prepareSetProcessingRequests, where it makes file_status->processing_lock.try_lock report failure — i.e. it takes exactly the production path of a file already grabbed by another consumer on the same server, and returns std::nullopt from the real code, not from a test-only branch in the iterator.

So both batch preconditions are now produced by real code paths:

• one file returns std::nullopt from the genuine processing-lock conflict branch (failpoint only flips the try_lock result, ONCE);
• another file makes the keeper tryMulti fail for real, against a processing node that the test pre-creates in keeper (real code, real responses, real getFailedOpIndex), so file_metadatas.clear() happens through the engine's own error handling.

No exception is thrown on either path, which is why the pre-fix compaction indexed past the end of the cleared vector.