Conversation
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>
Contributor
There was a problem hiding this comment.
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.
Contributor
Author
|
Passing CI on my fork ggilder#8 |
2 tasks
Contributor
Author
|
@meiji163 any thoughts on this? |
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>
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes two related deadlocks in the
GhostTableMigratedchangelog signal.1. Abort during the normal wait
Migrate()blocks on an unbuffered receive fromghostTableMigratedwhile waiting for the ghost table to be created:The only sender is
onChangelogStateEvent(), which publishes viabase.SendWithContext(). If the migration aborts during this window,abort()cancels the migration context, soSendWithContext()takes itsctx.Done()branch and returns without ever sending. Nothing else writes to the channel, soMigrate()blocks forever:Migrate()never returns, so its deferredteardown()never runsfinishedMigratingis therefore never setfinishedMigratingrather than on the context, so they keep loopingThe 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()—ghostTableMigratedis its remaining sibling.2. Instant DDL success never receives the signal at all (#1735, #1736)
initiateApplieremits theGhostTableMigratedsignal whenever!Revert && !Resume, regardless of whether instant DDL succeeds. The instant-DDL success path returns early right afterfinalCleanup()and never received the signal, so the publisher (onChangelogStateEvent, viabase.SendWithContext) blocked forever, holdingEventsStreamer.listenersMutexfor the whole synchronous callback.finalCleanup()then closes the binlog reader, whose rows-event decode callback (shouldDecodeRowsEvent) needs the same mutex, soBinlogSyncer.Close()waits for that goroutine forever too — a permanent deadlock whenever--attempt-instant-ddlsucceeds.#1736 proposed fixing this with a bare
<-mgtr.ghostTableMigratedreceive, 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 duringAttemptInstantDDL()'s retries and the heartbeat'sPanicAbortfires).Change
Extract the wait into
waitForGhostTableMigrated()and select on the migration context alongside the channel, returningcheckAbort()so the original abort error is surfaced rather than a bare context error. The extraction mirrorsconsumeRowCopyComplete()and makes the behaviour testable without a database.The instant-DDL success path now calls this same
waitForGhostTableMigrated()beforefinalCleanup(), guarded by!Resume(resume migrations never emit the signal), so both deadlocks share one context-aware wait instead of two divergent receives.TestAbort_DuringGhostTableWaitfollows the existingTestAbort_*convention and blocks until its timeout (i.e. fails) without this change;TestWaitForGhostTableMigratedcovers the normal path.TestEventsStreamerInstantDDLDeadlockIsResolvedByDrainingreproduces the exact instant-DDL deadlock mechanism (listener blocked on the send while holdinglistenersMutex,shouldDecodeRowsEventblocked 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.
script/cibuildreturns 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 unmodifiedmaster, so they are unaffected by this change and left to CI.