Skip to content

Fix deadlocks on the GhostTableMigrated changelog signal - #1758

Open
ggilder wants to merge 3 commits into
github:masterfrom
ggilder:handle-ctx-cancellation-ghost-table-migrated
Open

ggilder wants to merge 3 commits into
github:masterfrom
ggilder:handle-ctx-cancellation-ghost-table-migrated

Conversation

@ggilder

@ggilder ggilder commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Description

This PR fixes two related deadlocks in the GhostTableMigrated changelog signal.

1. Abort during the normal wait

Migrate() blocks on an unbuffered receive from ghostTableMigrated while waiting for the ghost table to be created:

mgtr.migrationContext.Log.Infof("Waiting for ghost table to be migrated. Current lag is %+v", initialLag)
<-mgtr.ghostTableMigrated

The only sender is onChangelogStateEvent(), which publishes via base.SendWithContext(). If the migration aborts during this window, abort() cancels the migration context, so SendWithContext() takes its ctx.Done() branch and returns without ever sending. Nothing else writes to the channel, so Migrate() blocks forever:

  • Migrate() never returns, so its deferred teardown() never runs
  • finishedMigrating is therefore never set
  • the status and throttler tickers exit on finishedMigrating rather than on the context, so they keep looping

The result is a process that has already decided to abort but stays alive indefinitely, logging a frozen status line, until it is killed externally. This is the same type of deadlock, and the same fix, as #1677 applied to consumeRowCopyComplete()ghostTableMigrated is its remaining sibling.

2. Instant DDL success never receives the signal at all (#1735, #1736)

initiateApplier emits the GhostTableMigrated signal whenever !Revert && !Resume, regardless of whether instant DDL succeeds. The instant-DDL success path returns early right after finalCleanup() and never received the signal, so the publisher (onChangelogStateEvent, via base.SendWithContext) blocked forever, holding EventsStreamer.listenersMutex for the whole synchronous callback. finalCleanup() then closes the binlog reader, whose rows-event decode callback (shouldDecodeRowsEvent) needs the same mutex, so BinlogSyncer.Close() waits for that goroutine forever too — a permanent deadlock whenever --attempt-instant-ddl succeeds.

#1736 proposed fixing this with a bare <-mgtr.ghostTableMigrated receive, but that reintroduces deadlock 1: it would block forever if the context is cancelled during the instant-DDL attempt (e.g. the primary becomes unreachable during AttemptInstantDDL()'s retries and the heartbeat's PanicAbort fires).

Change

Extract the wait into waitForGhostTableMigrated() and select on the migration context alongside the channel, returning checkAbort() so the original abort error is surfaced rather than a bare context error. The extraction mirrors consumeRowCopyComplete() and makes the behaviour testable without a database.

The instant-DDL success path now calls this same waitForGhostTableMigrated() before finalCleanup(), guarded by !Resume (resume migrations never emit the signal), so both deadlocks share one context-aware wait instead of two divergent receives.

TestAbort_DuringGhostTableWait follows the existing TestAbort_* convention and blocks until its timeout (i.e. fails) without this change; TestWaitForGhostTableMigrated covers the normal path. TestEventsStreamerInstantDDLDeadlockIsResolvedByDraining reproduces the exact instant-DDL deadlock mechanism (listener blocked on the send while holding listenersMutex, shouldDecodeRowsEvent blocked on the same mutex) and proves that receiving the signal resolves it.

Possibly related

Not claiming these are fixed by this change — their root causes aren't established, and this only addresses hangs caused by an abort cancelling the context during the wait, plus the instant-DDL early return — but they are reports of hanging at this exact point, so noting them for reference: #884, #380.

In case this PR introduced Go code changes:

  • contributed code is using same conventions as original code
  • script/cibuild returns with no formatting errors, build errors or unit test errors.

On script/cibuild: formatting, build, and all unit tests pass. The three testcontainers-based suites (TestApplier, TestMigrator, TestEventsStreamer) cannot run in my environment — no rootless Docker provider — and fail identically on unmodified master, so they are unaffected by this change and left to CI.

Migrate() blocks on an unbuffered receive from ghostTableMigrated while
waiting for the ghost table to be created. The only sender is
onChangelogStateEvent(), which publishes via base.SendWithContext().

If the migration aborts during this window, abort() cancels the
migration context, so SendWithContext() takes its ctx.Done() branch and
returns without ever sending. Nothing else writes to the channel, so
Migrate() blocks forever: it never returns, its deferred teardown()
never runs, finishedMigrating is never set, and the status and throttler
tickers -- which exit on finishedMigrating rather than on the context --
keep looping. The process stays alive indefinitely, logging a frozen
status line, until it is killed externally.

Extract the wait into waitForGhostTableMigrated() and select on the
migration context alongside the channel, returning checkAbort() so the
original abort error is surfaced rather than a bare context error. The
extraction mirrors consumeRowCopyComplete() and makes the behaviour
testable without a database.

This is the same deadlock, and the same fix, as github#1677 applied to
consumeRowCopyComplete; ghostTableMigrated is its remaining sibling.

TestAbort_DuringGhostTableWait follows the existing TestAbort_* pattern
and fails (blocking until its timeout) without this change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings August 20, 2026 20:06

Copilot AI 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.

Pull request overview

Prevents migration shutdown deadlocks when cancellation occurs while awaiting ghost-table migration.

Changes:

  • Adds a context-aware ghost-table wait that preserves the abort error.
  • Adds regression tests for cancellation and successful signaling.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
go/logic/migrator.go Replaces the blocking receive with a cancellation-aware helper.
go/logic/migrator_test.go Tests abort and normal completion paths.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@ggilder

ggilder commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Passing CI on my fork ggilder#8

@ggilder

ggilder commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

@meiji163 any thoughts on this?

ggilder and others added 2 commits September 21, 2026 16:10
initiateApplier emits the GhostTableMigrated changelog signal whenever
!Revert && !Resume, regardless of whether instant DDL succeeds. The
instant-DDL success path returned early without ever receiving it, so
the publisher (onChangelogStateEvent) blocked forever holding
EventsStreamer.listenersMutex, and finalCleanup then deadlocked
closing the binlog reader, which needs the same mutex (github#1735, github#1736).

Reuse waitForGhostTableMigrated() here instead of a bare receive, so
this doesn't trade the instant-DDL deadlock for the abort-path
deadlock on the same channel.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ggilder ggilder changed the title Handle context cancellation while waiting for ghost table migration Fix deadlocks on the GhostTableMigrated changelog signal Sep 22, 2026
@ggilder

ggilder commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

@timvaillancourt @meiji163 @ericyan I've updated this PR to handle the two remaining deadlock issues in gh-ost that I've been able to identify. Build is passing on my fork: ggilder#8

Please take a look when you get a chance, thanks!

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants