Fix file identifier in rescheduleTasksFromReplica

[ianton-ru]

Changelog category (leave one):

• Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Solved #1486
File identifier for distributed tasks was changed between 25.8 and 26.1
But in frontport #1414 it was missed and rescheduleTasksFromReplica continued to use old variant.

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

• Fast test
• Integration Tests
• Stateless tests
• Stateful tests
• Performance tests
• All with ASAN
• All with TSAN
• All with MSAN
• All with UBSAN
• All with Coverage
• All with Aarch64
• All Regression
• Disable CI Cache

Regression jobs to run:

• Fast suites (mostly <1h)
• Aggregate Functions (2h)
• Alter (1.5h)
• Benchmark (30m)
• ClickHouse Keeper (1h)
• Iceberg (2h)
• LDAP (1h)
• Parquet (1.5h)
• RBAC (1.5h)
• SSL Server (1h)
• S3 (2h)
• Tiered Storage (2h)

[ianton-ru]

@codex review

[github-actions]

Workflow [PR], commit [06b4ede]

[chatgpt-codex-connector]

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

We were unable to download your code in a timely manner.

ℹ️ 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".

[ianton-ru]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 👍

ℹ️ 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".

[vzakaznikov]

Audit Review

No confirmed defects in the PR. The fix correctly centralizes file identifier logic and resolves the key mismatch in rescheduleTasksFromReplica. It also fixes a secondary inconsistency in getPreQueuedFile where isArchive() was not checked (only send_over_whole_archive was), which could cause lookup mismatches when bucket info is present.

Pre-existing risk: data race on replica_to_files_to_be_processed

getReplicaForFile() iterates replica_to_files_to_be_processed without holding the mutex when called from getMatchingFileFromIterator:

// line 197, StorageObjectStorageStableTaskDistributor.cpp — no lock held
size_t file_replica_idx = getReplicaForFile(file_identifier);

Meanwhile rescheduleTasksFromReplica can concurrently erase() from the same map under the lock:

// line 319, StorageObjectStorageStableTaskDistributor.cpp — lock held
replica_to_files_to_be_processed.erase(number_of_current_replica);

Concurrent read iteration + erase on std::unordered_map is UB. Not introduced by this PR, but worth noting.

Minor style nit

getFileIdentifier takes ObjectInfoPtr by value (unnecessary atomic ref-count bump). const ObjectInfoPtr & would be slightly more efficient.

Confidence: High (9/10) — a test exercising rescheduleTasksFromReplica with archive or bucket-info files would raise it further.

[vzakaznikov]

Follow-up audit note: additional pre-existing UB/crash risks in StorageObjectStorageStableTaskDistributor (not introduced by this PR).

1) Unsynchronized unordered_map iteration/read vs erase

Container: replica_to_files_to_be_processed

Read path (no lock):

• getReplicaForFile() iterates map:
  • src/Storages/ObjectStorage/StorageObjectStorageStableTaskDistributor.cpp lines ~84-106
• Called from getMatchingFileFromIterator() without holding mutex:
  • same file line ~197

Write path (with lock):

• rescheduleTasksFromReplica() erases from map:
  • same file line ~319 (replica_to_files_to_be_processed.erase(...))

Why this is a defect: concurrent iteration/read + erase on std::unordered_map is undefined behavior and may crash/corrupt state.

2) Unsynchronized unordered_map pointer/iterator lifetime hazard in getNextTask

Container: replica_to_files_to_be_processed

Read path (no lock):

• getNextTask() obtains iterator/pointer-like handle from find():
  • same file lines ~52-53
• Later uses it to mutate list:
  • line ~70 (processed_file_list_ptr->second.push_back(file))

Concurrent write path (with lock):

• rescheduleTasksFromReplica() may erase same key:
  • line ~319

Why this is a defect: if another thread erases the entry between find() and later push_back(), processed_file_list_ptr becomes invalid (UB/use-after-erase class behavior).

Minimal interleaving that can trigger UB

1. T1 enters getNextTask(replica=X), reads processed_file_list_ptr = find(X) (no lock).
2. T2 detects connection loss for same replica and enters rescheduleTasksFromReplica(X), acquires lock, erases map entry for X.
3. T1 later executes processed_file_list_ptr->second.push_back(file) using invalid iterator/handle.

Suggested fix direction

• Guard all accesses (find/iterate/read/write/erase) to replica_to_files_to_be_processed with the same mutex, or
• Snapshot active replica ids under lock before hashing/iteration, and never keep iterators/refs across unlocks.

Given both issues are same shared-state root cause, fixing together would likely be safest.