Skip to content

fix: await background load futures during cancellation and fix loadingTimeoutMs default#48855

Open
sparknack wants to merge 3 commits intomilvus-io:masterfrom
sparknack:cl-fix
Open

fix: await background load futures during cancellation and fix loadingTimeoutMs default#48855
sparknack wants to merge 3 commits intomilvus-io:masterfrom
sparknack:cl-fix

Conversation

@sparknack
Copy link
Copy Markdown
Contributor

Summary

  • Await background load futures in GroupChunkTranslator and ManifestGroupTranslator cancellation cleanup paths to prevent use-after-free / dangling futures
  • Add CheckCancellation calls in LoadCellBatchAsync loop for prompt cancellation
  • Fix tieredStorage.loadingTimeoutMs default from 0 (immediate failure) to -1 (no timeout)

issue: #48854

Test plan

  • Added unit test CancellationStopsMidBatchPush verifying mid-batch cancellation stops loading and raises FollyCancel
  • CI passes

…nslators

Ensure background load futures are properly awaited during cancellation
cleanup in both GroupChunkTranslator and ManifestGroupTranslator to
prevent dangling async operations.

Also add CheckCancellation calls in LoadCellBatchAsync to support early
termination during cell loading.

Signed-off-by: Shawn Wang <shawn.wang@zilliz.com>
Signed-off-by: Shawn Wang <shawn.wang@zilliz.com>
@sre-ci-robot sre-ci-robot added the size/M Denotes a PR that changes 30-99 lines. label Apr 8, 2026
@sre-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sparknack
To complete the pull request process, please assign czs007 after the PR has been reviewed.
You can assign the PR to them by writing /assign @czs007 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mergify mergify bot added dco-passed DCO check passed. kind/bug Issues or changes related a bug labels Apr 8, 2026
@sre-ci-robot
Copy link
Copy Markdown
Contributor

[ci-v2-notice]
Notice: New ci-v2 system is enabled for this PR.

To rerun ci-v2 checks, comment with:

  • /ci-rerun-code-check // for ci-v2/code-check
  • /ci-rerun-build // for ci-v2/build
  • /ci-rerun-build-all // for ci-v2/build-all (multi-arch builds)
  • /ci-rerun-buildenv // for ci-v2/build-env (build milvus-env builder images)
  • /ci-rerun-ut-integration // for ci-v2/ut-integration, will rerun ci-v2/build
  • /ci-rerun-ut-go // for ci-v2/ut-go, will rerun ci-v2/build
  • /ci-rerun-ut-cpp // for ci-v2/ut-cpp
  • /ci-rerun-ut // for all ci-v2/ut-integration, ci-v2/ut-go, ci-v2/ut-cpp, will rerun ci-v2/build
  • /ci-rerun-e2e-default // for ci-v2/e2e-default
  • /ci-rerun-e2e-amd // for ci-v2/e2e-amd (e2e pool dispatcher)
  • /ci-rerun-build-ut-cov // for ci-v2/build-ut-cov (build + unit tests in one pipeline)
  • /ci-rerun-gosdk // for ci-v2/go-sdk (Go SDK E2E tests, ARM)

If you have any questions or requests, please contact @zhikunyao.

@sre-ci-robot
Copy link
Copy Markdown
Contributor

✅ CI Loop Results a29ee05

Stage Result Duration Tests
✅ Build SUCCESS 13.2min -

Total: 18min | Pipeline | Artifacts

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 8, 2026

Codecov Report

❌ Patch coverage is 67.50000% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.49%. Comparing base (91cf2cc) to head (a29ee05).
⚠️ Report is 7 commits behind head on master.

⚠️ Current head a29ee05 differs from pull request most recent head 339021d

Please upload reports for the commit 339021d to get more accurate results.

Files with missing lines Patch % Lines
...gcore/storagev2translator/GroupChunkTranslator.cpp 0.00% 6 Missing ⚠️
...re/storagev2translator/ManifestGroupTranslator.cpp 0.00% 6 Missing ⚠️
internal/core/src/segcore/MemoryPlannerTest.cpp 96.15% 1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (67.50%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #48855      +/-   ##
==========================================
- Coverage   84.29%   83.49%   -0.81%     
==========================================
  Files         633      645      +12     
  Lines       99080   101186    +2106     
==========================================
+ Hits        83516    84481     +965     
- Misses      15564    16653    +1089     
- Partials        0       52      +52     
Components Coverage Δ
Client ∅ <ø> (∅)
Core 84.28% <67.50%> (-0.01%) ⬇️
Go ∅ <ø> (∅)
Files with missing lines Coverage Δ
internal/core/src/segcore/memory_planner.cpp 95.65% <100.00%> (+0.04%) ⬆️
internal/core/src/segcore/MemoryPlannerTest.cpp 99.50% <96.15%> (-0.50%) ⬇️
...gcore/storagev2translator/GroupChunkTranslator.cpp 87.61% <0.00%> (-2.39%) ⬇️
...re/storagev2translator/ManifestGroupTranslator.cpp 77.31% <0.00%> (-2.47%) ⬇️

... and 12 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sre-ci-robot sre-ci-robot added the low-code-coverage add test-label from zhikun, diff coverage > 80% label Apr 8, 2026
Signed-off-by: Shawn Wang <shawn.wang@zilliz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dco-passed DCO check passed. kind/bug Issues or changes related a bug low-code-coverage add test-label from zhikun, diff coverage > 80% size/M Denotes a PR that changes 30-99 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants