Skip to content

fix(incrservice): keep lazy cleanup cancelable - #26883

Merged
XuPeng-SH merged 4 commits into
mainfrom
fix/incrservice-shutdown-logtail-wait
Aug 10, 2026
Merged

fix(incrservice): keep lazy cleanup cancelable#26883
XuPeng-SH merged 4 commits into
mainfrom
fix/incrservice-shutdown-logtail-wait

Conversation

@XuPeng-SH

@XuPeng-SH XuPeng-SH commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • stop dropped-table auto-increment metadata GC from requesting a committed-logtail visibility wait
  • keep the fail-closed wait unchanged for allocation and offset update paths
  • release the account lookup result before executing the metadata delete
  • assert transaction/nested SQL options and mpool release in regression tests

Root cause

TestHashBuildSharedBudgetRecoverySQL completed its main workload, but CN shutdown joined the auto-increment cleanup worker while it was deleting metadata for a dropped table. The delete inherited WithWaitCommittedLogApplied into the nested SQL execution. After the transaction committed, SyncLatestCommitTS waited with its own five-minute background timeout and did not observe the cleanup worker context, so teardown ended in the five-minute fatal timeout seen in both PR #26850 and PR #26806.

A second deep-review pass found that the successful account lookup result was overwritten by the subsequent delete result when the tenant existed. Its batches therefore never reached Result.Close, retaining their mpool allocation for every cleaned table. The lookup result is now closed immediately after reading it, and the test uses a real allocated result to verify that CurrNB returns to zero.

Safety

This delete is lazy GC for a globally allocated table ID that is not reused. Commit durability is sufficient; no later operation requires the same CN to observe the deletion immediately. Failed deletes remain in the destroyed-table set and are retried. Allocation and offset mutation paths retain their committed-logtail waits.

Validation

  • focused delete tests plus the background TestDelete
  • go test ./pkg/incrservice
  • go test -race ./pkg/incrservice
  • each of the three delete tests with adaptive race stress: T=0/0/0.06s, B=30s, N=100
  • TestHashBuildSharedBudgetRecoverySQL normally and under race; race T=18.28s, B=30s, N=1
  • go build ./pkg/incrservice
  • go vet ./pkg/incrservice
  • golangci-lint run ./pkg/incrservice/... (0 issues)

Failure examples:

@XuPeng-SH
XuPeng-SH requested a review from gouhongshen as a code owner August 9, 2026 16:42
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@gouhongshen gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex automated review

No blocking or non-blocking defects found. Existing discussion contained no substantive reviews or inline findings; focused tests were blocked by missing native headers.

@mergify

mergify Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@XuPeng-SH
XuPeng-SH merged commit 0b20dd4 into main Aug 10, 2026
30 checks passed
@XuPeng-SH
XuPeng-SH deleted the fix/incrservice-shutdown-logtail-wait branch August 10, 2026 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Denotes a PR that changes [10,99] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants