Skip to content

restore: stop bulk insert polling on context cancellation - #1158

Open
huanghaoyuanhhy wants to merge 1 commit into
zilliztech:mainfrom
huanghaoyuanhhy:restore-bulkinsert-poll-ctx
Open

restore: stop bulk insert polling on context cancellation#1158
huanghaoyuanhhy wants to merge 1 commit into
zilliztech:mainfrom
huanghaoyuanhhy:restore-bulkinsert-poll-ctx

Conversation

@huanghaoyuanhhy

@huanghaoyuanhhy huanghaoyuanhhy commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Fixes #1157

Summary

The bulk insert state polling loops in checkBulkInsertViaGrpc and checkBulkInsertViaRestful (core/restore/coll_task.go) used for range time.Tick(...) and never checked ctx.Done() in the loop itself. Cancellation only surfaced indirectly, when the next GetBulkInsertState RPC failed with a context error, so a cancelled restore task kept polling for up to another poll interval (and, in a stale-success race at the moment of cancellation, longer). The loops also leaked their tickers.

Changes

  • Both loops now use time.NewTicker with select { <-ctx.Done(); <-ticker.C } between polls, mirroring the secondary restore implementation in core/restore/secondary/coll_dml_task.go
  • Drop the trailing return errors.New(...) lines, which became unreachable (govet) once the loops were rewritten as unbounded for {}
  • Polling cadence and no-progress watchdog behavior are unchanged

Test plan

  • go test ./core/restore/ passes
  • golangci-lint clean on the changed package

@sre-ci-robot

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: huanghaoyuanhhy

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

The pull request process is described 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

@codecov-commenter

codecov-commenter commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.71%. Comparing base (2309945) to head (aead0c3).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
core/restore/coll_task.go 0.00% 14 Missing ⚠️

❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.
❌ Your project status has failed because the head coverage (42.71%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1158      +/-   ##
==========================================
+ Coverage   42.50%   42.71%   +0.20%     
==========================================
  Files         136      134       -2     
  Lines       12181    12229      +48     
==========================================
+ Hits         5178     5224      +46     
+ Misses       6624     6611      -13     
- Partials      379      394      +15     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

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

@mergify mergify Bot added the ci-passed label Aug 14, 2026
@huanghaoyuanhhy
huanghaoyuanhhy force-pushed the restore-bulkinsert-poll-ctx branch from 08c7478 to 0c1f221 Compare August 17, 2026 02:54
@mergify mergify Bot removed the ci-passed label Aug 17, 2026
Signed-off-by: huanghaoyuanhhy <haoyuan.huang@zilliz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

restore: bulk insert polling loop ignores context cancellation

3 participants