Workflow [PR], commit [1c740ad]

Here is an example of new, shorter output, for one of the recent PRs:

AI audit note: This review comment was generated by AI (gpt-5.3-codex).

Audit update for PR #1402 (Improvements to partition export):

Confirmed defects:

• High: Backward-incompatible manifest parsing for in-flight exports
• Impact: Upgraded nodes can fail to parse older metadata.json records, breaking updater/system-table processing for those exports.
• Anchor: src/Storages/ExportReplicatedMergeTreePartitionManifest.h / ExportReplicatedMergeTreePartitionManifest::fromJsonString
• Trigger: Parse metadata created before lock_inside_the_task existed.
• Why defect: New key is read unconditionally; persisted control-plane state requires backward compatibility.
• Fix direction (short): Treat lock_inside_the_task as optional and default missing key to false.
• Regression test direction (short): Mixed-version metadata fixture (without key) must parse and remain queryable.

manifest.lock_inside_the_task = json->getValue<bool>("lock_inside_the_task");

• Medium: Scheduler slot counter increments after failed schedule attempt
• Impact: Available background slots can be underutilized in the same scheduling cycle, delaying export throughput.
• Anchor: src/Storages/MergeTree/ExportPartitionTaskScheduler.cpp / non-lock_inside_the_task branch in run()
• Trigger: storage.exportPartToTable(...) throws for a candidate part while slots remain.
• Why defect: scheduled_exports_count increments even when no task was actually scheduled.
• Fix direction (short): Increment counter only on confirmed successful scheduling.
• Regression test direction (short): Inject first-attempt scheduling failure and assert remaining eligible parts still fill available slots.

try
{
    storage.exportPartToTable(...);
}
catch (const Exception &)
{
    zk->tryRemove(fs::path(storage.zookeeper_path) / "exports" / key / "locks" / zk_part_name);
}
scheduled_exports_count++;

• Medium: Local system-table fallback can omit non-pending exports
• Impact: system.replicated_partition_exports can miss completed/failed entries when local path is used (no MULTI_READ or remote not preferred).
• Anchor: src/Storages/MergeTree/ExportPartitionManifestUpdatingTask.cpp / poll() and local info path
• Trigger: Export is already non-pending when poll runs and local cache path is used for query.
• Why defect: Non-pending records are skipped from task-entry tracking, but local query path depends on those entries.
• Fix direction (short): Include required non-pending records in local cache or preserve robust remote fallback when MULTI_READ is unavailable.
• Regression test direction (short): No-MULTI_READ test verifying completed/failed exports appear in system table.

if (!is_pending)
{
    LOG_INFO(storage.log, "Skipping {}: status is not PENDING", key);
    continue;
}

Coverage summary:

• Scope reviewed: Full merged diff of PR Improvements to partition export #1402 (664f99e^1..664f99e^2), including scheduler/task execution, metadata/updater, system table path, settings, and integration tests.
• Categories failed: Metadata schema compatibility; scheduling-accounting correctness; local-vs-remote observability parity.
• Categories passed: Locking/collision handling; exception/partial-update safety in touched paths; static concurrency interleaving checks; C++ lifetime/UB checks in changed code.
• Assumptions/limits: Static audit only (no runtime Keeper fault-injection or stress execution); conclusions for timing-sensitive paths are code-path based.