Skip to content

change the audit report to short form#1489

Merged
zvonand merged 2 commits intoantalya-25.8from
update-audit-skill-to-return-shorter-reports
Mar 10, 2026
Merged

change the audit report to short form#1489
zvonand merged 2 commits intoantalya-25.8from
update-audit-skill-to-return-shorter-reports

Conversation

@Selfeer
Copy link
Collaborator

@Selfeer Selfeer commented Mar 9, 2026

Changelog category (leave one):

  • New Feature
  • Experimental Feature
  • Improvement
  • Performance Improvement
  • Backward Incompatible Change
  • Build/Testing/Packaging Improvement
  • Documentation (changelog entry is not required)
  • Critical Bug Fix (crash, data loss, RBAC) or LOGICAL_ERROR
  • Bug Fix (user-visible misbehavior in an official stable release)
  • CI Fix or Improvement (changelog entry is not required)
  • Not for changelog (changelog entry is not required)

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

...

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)

@github-actions
Copy link

github-actions bot commented Mar 9, 2026

Workflow [PR], commit [1c740ad]

@Selfeer
Copy link
Collaborator Author

Selfeer commented Mar 10, 2026

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.

@zvonand zvonand merged commit 2e097b9 into antalya-25.8 Mar 10, 2026
166 checks passed
@Selfeer Selfeer added the verified Verified by QA label Mar 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

verified Verified by QA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants