Skip to content

Fix ISCP index consumer cancellation during DROP INDEX#25733

Open
jiangxinmeng1 wants to merge 8 commits into
matrixorigin:mainfrom
jiangxinmeng1:fix-iscp-index-consumer-cancel
Open

Fix ISCP index consumer cancellation during DROP INDEX#25733
jiangxinmeng1 wants to merge 8 commits into
matrixorigin:mainfrom
jiangxinmeng1:fix-iscp-index-consumer-cancel

Conversation

@jiangxinmeng1

@jiangxinmeng1 jiangxinmeng1 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #25711

What this PR does / why we need it:

This PR fixes DROP INDEX for ISCP-backed index jobs by canceling and draining the
active index consumer before index metadata and hidden tables are removed.

Changes:

  • Add ISCP runtime tracking for running job consumers.

  • Fence dropped index jobs so new iterations skip them.

  • Cancel the consumer context and data retriever, then wait for the running consumer to
    exit.

  • Wire DROP INDEX and ALTER TABLE DROP INDEX through the ISCP drain path before
    deleting metadata.

  • Make data retrievers reject/cleanup queued data after cancellation.

  • Avoid updating ISCP job status after a job has been fenced/canceled.

  • Add ctx-aware fault injection for WAIT and SLEEP.

  • Add ISCP cancel failpoints for different consumer phases, including per-index-name
    injection.

  • Fix WAIT fault removal so blocked waiters are notified.

  • Add unit tests for runtime cancel, retriever cleanup, fault cancellation, and DDL
    drain behavior.

  • Added an intermediate ISCP job state, Canceling, while keeping the existing
    Canceled state as the durable final state.

  • Changed ISCP unregister/drop paths to mark dropped jobs as Canceling first instead
    of relying on a foreground, process-local drain.

  • Added an executor-side cancel queue so the ISCP runner CN asynchronously drains any
    unregistered job and then marks it as Canceled in mo_catalog.mo_iscp_log.

  • Added startup/recovery handling so existing dropped jobs that are not yet Canceled
    are converged to Canceled.

  • Prevented normal watermark/status flushes from overwriting terminal or canceling
    states: Error, Canceled, and Canceling.

  • Added a standalone system-table state update SQL helper for finalizing jobs as
    Canceled.

  • Kept DROP TABLE / drop-all ISCP paths able to proceed when the local CN is not the
    ISCP runner; final cancellation is confirmed through the system table state instead of
    a process-local executor.

  • Added fault points for cancel-finalization testing:

    • fj/iscp/cancel/before-mark-canceled
    • fj/iscp/cancel/mark-canceled-error
  • Fixed the final mark-canceled transaction context by adding a timeout/deadline,
    avoiding the context deadline set fatal observed during local multi-CN testing.

  • Added focused unit coverage for dropped-job queueing, recovered dropped-job
    finalization, and drop-all behavior when the local executor is absent.

Test Matrix

Scenario Injection / Result
Coverage
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ━━━━━━━━━━━━━━━━━━━━━ ━━━━━━━━━━━━━━━━━━━━━━
Reproduce original DROP INDEX issue Documented ISCP Fixed, DROP INDEX no
index cancel repro longer leaves
background consumer
running
──────────────────────────────────────── ───────────────────── ──────────────────────
Cancel after consumer registration fj/iscp/cancel/ DROP waits for
after-register- drain, then succeeds
consumer
──────────────────────────────────────── ───────────────────── ──────────────────────
Cancel after worker submit fj/iscp/cancel/ Fenced job is
after-submit skipped, DROP
succeeds
──────────────────────────────────────── ───────────────────── ──────────────────────
Cancel during fanout fj/iscp/cancel/ Canceled retriever
fanout-before-send rejects data, DROP
succeeds
──────────────────────────────────────── ───────────────────── ──────────────────────
Cancel before HNSW update fj/iscp/cancel/ DROP returns quickly
long/hnsw-before-
update
──────────────────────────────────────── ───────────────────── ──────────────────────
Cancel before HNSW save fj/iscp/cancel/ DROP returns quickly
long/hnsw-before-
save
──────────────────────────────────────── ───────────────────── ──────────────────────
Cancel before watermark update fj/iscp/cancel/ DROP returns quickly
long/before-
watermark
──────────────────────────────────────── ───────────────────── ──────────────────────
Cancel before SQL execution fj/iscp/cancel/ DROP returns quickly
long/before-exec
with fulltext ASYNC
──────────────────────────────────────── ───────────────────── ──────────────────────
Long wait via sleep fj/iscp/cancel/ DROP returns in
long/before- ~498ms
send:idx01 with
sleep 30
──────────────────────────────────────── ───────────────────── ──────────────────────
Iteration already failed before cancel injected Job records
changesNext error ErrorCode=1; DROP
still succeeds and
sets drop_at
──────────────────────────────────────── ───────────────────── ──────────────────────
Multiple jobs in one iteration two HNSW indexes on idx01 removed; idx02
different columns, preserved
cancel only idx01
──────────────────────────────────────── ───────────────────── ──────────────────────
Fault cleanup list fault points no remaining fault
after tests points
──────────────────────────────────────── ───────────────────── ──────────────────────
Goroutine cleanup pprof goroutine no stuck cancel/
scan fault/ISCP
goroutines

Area Regression reproduction
Scenario Started a local multi-CN cluster with the old binary and executed DROP TABLE
from a CN that was not the ISCP runner.
Result Reproduced the BVT failure: cannot confirm ISCP consumer quiescence ....
───────────────────────────────────────────────────────────────────────────────────────
Area Multi-CN lifecycle
Scenario Rebuilt the new branch, started a local multi-CN cluster, created an async
HNSW index on the ISCP runner CN, and executed DROP TABLE from another CN.
Result DROP TABLE succeeded. The ISCP job moved from Canceling(6) to Canceled(5).
───────────────────────────────────────────────────────────────────────────────────────
Area Long cancel finalization
Scenario Injected fj/iscp/cancel/before-mark-canceled with sleep 5 before marking the
job canceled.
Result Foreground DDL returned successfully; the system table stayed at
Canceling(6) during the sleep and then became Canceled(5).
───────────────────────────────────────────────────────────────────────────────────────
Area Mark-canceled failure retry
Scenario Injected fj/iscp/cancel/mark-canceled-error once before updating the final
canceled state.
Result The job temporarily stayed at Canceling(6) and was later retried and
finalized as Canceled(5).
───────────────────────────────────────────────────────────────────────────────────────
Area Restart recovery
Scenario Injected sleep 30 before mark-canceled, stopped the cluster while the job
was still Canceling(6), then restarted with the same data directory.
Result After executor startup/recovery, the dropped job converged to Canceled(5).
───────────────────────────────────────────────────────────────────────────────────────
Area Final state check
Scenario Queried mo_catalog.mo_iscp_log after all local multi-CN fault-injection
runs.
Result All tested dropped ISCP jobs ended in Canceled(5).
───────────────────────────────────────────────────────────────────────────────────────
Area Unit tests
Scenario go test -count=1 ./pkg/iscp ./pkg/cdc ./pkg/sql/compile with the local CGo
flags.
Result Passed.
───────────────────────────────────────────────────────────────────────────────────────
Area Static checks
Scenario go vet ./pkg/iscp ./pkg/cdc ./pkg/sql/compile; git diff --check.
Result Passed.
───────────────────────────────────────────────────────────────────────────────────────
Area Build/package graph
Scenario make build.
Result Passed, including the Go package graph check.

@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 →

@mergify mergify Bot added the kind/bug Something isn't working label Jul 15, 2026
@matrix-meow matrix-meow added the size/XL Denotes a PR that changes [1000, 1999] lines label Jul 15, 2026

@XuPeng-SH XuPeng-SH 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.

Re-reviewed the latest head 4c546a95994f95023d458249634c14008446c728. The earlier correctness blockers around wrong-CN silent success, permanent name-scoped fences, and drop-all paths look materially improved: wrong-CN drain now fails closed, fences are cleaned on commit/rollback and keyed by jobID, and DropAllIndexCdcTasks now routes through drain. The focused pkg/iscp, pkg/sql/compile, and pkg/util/fault tests also pass on this head.

I still see one remaining blocker in the fault-injection wait path. faultEntry.do() now checks removed before registering a waiter, but the context-aware path used by TriggerFaultWithContext still does not. doWithContext(WAIT) always increments nWaiters and waits without first consulting removed, so REMOVE can still run after TRIGGER dispatch but before waiter registration, broadcast while nobody is waiting, delete the entry, and leave the context-aware waiter blocked until its context is canceled. This path is not just theoretical: objectio.WaitInjectedCtx uses TriggerFaultWithContext, and pkg/iscp/index_consumer.go calls it in the injected wait hooks. The new tests cover entry.do() and the cancel-after-registration shape, but they still do not exercise remove-after-dispatch-before-wait on the context-aware trigger path.

Please apply the same removed/registration handshake to doWithContext(WAIT) and add a deterministic regression around TriggerFaultWithContext + RemoveFaultPoint for that interleaving.

Comment thread pkg/util/fault/fault.go
Comment thread pkg/util/fault/fault_test.go

@XuPeng-SH XuPeng-SH 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.

Re-reviewed the latest head 5264206547, focusing on the context-aware WAIT path I blocked on 4c546a9599. The added removed check in doWithContext(WAIT) closes the remove-after-dispatch-before-wait window, and the new regression covers exactly that interleaving. I reran the focused pkg/util/fault tests on this head and do not see a remaining high-confidence correctness or unhappy-path blocker in the latest delta.

@LeftHandCold LeftHandCold 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.

Reviewed the current head (76ddc86). The local cancellation path is substantially safer now, and the targeted fault/ISCP/compile tests pass, but the cross-CN path still does not establish quiescence before destructive DDL. That leaves the original lock cycle reachable. I also found a deterministic lifetime leak in the generation-scoped fence map. Please address the two inline findings and add a two-CN regression that pauses an already-running consumer after it has touched the hidden table, then verifies DROP completes without waiting for the DDL transaction to commit first.

jobName,
jobID,
)
return nil

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.

[P1] Do not continue destructive DDL without quiescing the runner CN

This is still the original deadlock when the ISCP task runs on a different CN. DropIndexCdcTask writes Canceling/drop_at in the DROP transaction, so the runner cannot observe or enqueue that cancellation until this transaction commits; however the hidden tables are dropped later in this same transaction. If an existing consumer already holds a hidden-table write lock and then performs its normal mo_iscp_log result update, the DDL owns/waits on the log row and then waits on the hidden table while the consumer waits on the log row. The transaction therefore cannot reach the commit that would trigger the async cancellation. Returning success here makes the two-CN path fail open. Please route cancel/drain to the actual runner CN, or provide an out-of-transaction durable quiescence handshake and wait for it before dropping the hidden tables. The regression test needs an already-running remote consumer paused after touching the hidden table, not merely a two-CN DROP with no in-flight consumer.


exec.runtimeMu.Lock()
exec.ensureRuntimeMapsLocked()
exec.fencedJobs[key] = struct{}{}

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.

[P2] Reclaim generation fences after terminal state is installed

Every async cancellation reaches finishCanceledJob, which calls this method and inserts a generation key into fencedJobs; after markJobCanceled succeeds it only clears cancelingJobs. The cross-CN path has no compile callback that removes this fence, and the local path can re-add it after its commit callback already ran. GCInMemoryJob also does not remove these keys. Since every drop/recreate gets a new JobID, a long-lived runner retains one map entry and job-name string per historical generation without bound. Please remove the fence at a safe point after the canceled/drop state has been applied to in-memory scheduling state, or from the corresponding GC path, so cleanup does not reopen the pre-logtail admission window.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants