Stop drained backlog manager goroutines - #12047
chaptersix wants to merge 1 commit into
Conversation
|
Claude finished @chaptersix's task in 4m 7s —— View job Reviewed 4 files, 3 findings. nit —
|
|
I'm not an expert in the matching logic, so I'm leaning on Codex quite a bit. It noticed this issue while debugging some unrelated functionality. |
| s.Require().NoError(backlogCtx.Err()) | ||
| s.blm.Stop() | ||
| s.Require().ErrorIs(backlogCtx.Err(), context.Canceled) | ||
| s.Require().NoError(s.tqCtx.Err()) |
There was a problem hiding this comment.
med — The test only exercises the uninitialized early-return path, not the path the PR is actually about.
SetupTest never calls Start(), so at Stop():
priBacklogManagerImpl/fairBacklogManagerImplhitif !c.initializedError.Ready() ... { return }and go straight to the deferred cancel;backlogManagerImplhasackLevel == -1, so it skips the final update too.
The behavior this PR is careful about is the other branch: a running manager flushes ack levels through c.tqCtx and only then cancels, which is exactly what a drained manager does in FinishedDraining. If someone changed defer c.tqCtxCancel() to a plain c.tqCtxCancel() at the top of Stop, this test would still pass while the final db.SyncState/OldUpdateState would start failing on a canceled context.
Every other test in this suite already does s.blm.Start() + WaitUntilInitialized, so covering the initialized path is cheap.
Suggestion:
| s.Require().NoError(backlogCtx.Err()) | |
| s.blm.Stop() | |
| s.Require().ErrorIs(backlogCtx.Err(), context.Canceled) | |
| s.Require().NoError(s.tqCtx.Err()) | |
| s.Require().NoError(backlogCtx.Err()) | |
| s.blm.Start() | |
| s.Require().NoError(s.blm.WaitUntilInitialized(context.Background())) | |
| s.blm.Stop() | |
| s.Require().ErrorIs(backlogCtx.Err(), context.Canceled) | |
| s.Require().NoError(s.tqCtx.Err()) |
To also pin the ordering (flush happens before cancel), assert the final ack level landed in s.taskMgr after Stop, the way dbAckLevel does.
| tqCtx context.Context | ||
| tqCtxCancel context.CancelFunc |
There was a problem hiding this comment.
small — tqCtx is no longer the task queue context, and the name now misleads at its call sites.
The whole point of the change is that this context is scoped to one backlog manager, not to the physical task queue. Readers and writers reach it as tr.backlogMgr.tqCtx / w.backlogMgr.tqCtx, and comments written against the old meaning are now wrong — pri_task_reader.go:337 and fair_task_reader.go:349 both say "if tqCtx is closing, addTaskToMatcher will give up", which now happens when a drained manager stops while the queue keeps running. physicalTaskQueueManagerImpl also has its own tqCtx/tqCtxCancel pair, so two different lifetimes now share one name.
Suggestion: rename to bmCtx/bmCtxCancel in all three managers and their readers/writers, and drop the two stale "if tqCtx is closing" comments to match. Mechanical, but it keeps the new ownership boundary legible at the ~20 use sites that only see the field name.
What changed?
Give each backlog manager its own cancellable child context and cancel it when the backlog manager stops.
Cancellation happens after the final ack-level update and garbage-collection pass, preserving the existing shutdown ordering. Canceling a draining backlog manager therefore stops its writer, readers, timers, and retry work without canceling the parent physical task queue or its active backlog manager.
Add coverage for the classic, priority, and fairness backlog managers that verifies
Stopcancels the manager context while leaving the physical task queue context active.Why?
When a priority/fairness migration is in progress, a physical task queue owns two backlog managers:
Once draining finishes,
FinishedDrainingremoves the draining manager from the physical task queue, performs its final garbage collection, and callsStop. Before this change,Stoponly flushed the final ack state. The manager's background work used the physical task queue's context, which remains active after the draining manager is detached.The periodic-sync goroutine that detects completion returns normally, but the detached manager's writer and priority subqueue readers remain blocked on the still-live physical task queue context. Fairness readers and retry work can also retain the manager while active. This keeps the manager and its associated state reachable until the entire physical task queue unloads.
Physical task queues normally unload after
MaxTaskQueueIdleTime, but task additions, polls, and matching activity reset that timer. A busy queue may therefore remain loaded for the lifetime of the Matching process, turning a completed migration into long-lived goroutine and memory retention.Broader context
The lifecycle evolved in several steps:
FinishedDrainingbegan detaching the old manager and calling its existingStopmethod, which had been designed for physical-queue shutdown where the shared context was canceled immediately afterward.The intentional part of the old ordering is the final state flush before context cancellation: persistence operations use the manager context and would fail if it were canceled first. The unintended part is that drain completion is a component-level shutdown, not a physical-queue shutdown, so no cancellation followed the flush.
A child context restores that ownership boundary:
Stop, including uninitialized managers and lost ownership, still cancel background work.Potential risks
The change makes
Stopfully stop a backlog manager instead of relying on a later physical-queue cancellation. Final persistence and garbage collection still run before cancellation, and tests verify that the parent physical task queue context remains active. No task-routing or persistence-format behavior changes.