wip - #509
Closed
jayshrivastava wants to merge 1 commit into
Closed
Conversation
Collaborator
|
cleaning up stale PRs, I think there's no follow up on this one, feel free to reopen if this is not right |
gabotechs
added a commit
that referenced
this pull request
Jul 3, 2026
Closes #521 The old lifecycle assumed every producer partition stream would be materialized and dropped. That still tends to hold in the current `main` branch because the WorkerConnectionPool owns eager demux/drop behavior, so there is no natural failing test here. In the following PR: - #512 The gRPC abstraction refactor changes stream ownership and returns already-demuxed streams through the protocol boundary, which exposes the latent issue: query output can finish while some producer partition streams never reach the final drop path, leaving the metrics oneshot unresolved. Coordinator-channel EOS is the better task-finalization signal because it represents query-scoped shutdown independently of whether every partition stream was consumed or dropped normally. Deadlock sequence (WARN: explained by Codex, I still don't understand it 100%) Why the bug does not reproduce in `main`: 1. Metrics are sent from the worker when the worker-side execution streams for a task finish or are dropped. 2. In `main`, the remote gRPC path owns the full execute-task request lifecycle internally: once `WorkerConnectionPool` initializes a remote connection, the request is driven by the connection/demux machinery. 3. Local self-calls are also effectively per-partition: the local path asks `execute_task` for one partition at a time instead of returning a batch of partition streams where some may never be polled. 4. Because of that, the streams that count toward task finalization are normally materialized and eventually dropped. 5. The metrics oneshot resolves before the coordinator needs the query-scoped coordinator->worker stream to close. 6. `drain_pending_tasks()` can complete, and only after that the query-end guard is dropped. There is no dependency cycle. Why the bug reproduces in #512: 1. Metrics are still tied to the old final stream-drop path. 2. The refactor makes `WorkerChannel::execute_task` return already-demuxed partition streams through the protocol boundary. 3. The gRPC implementation keeps those streams lazy: it does not send the actual `execute_task` RPC until one returned partition stream is first polled. 4. In shapes like `custom_routing_join`, DataFusion may construct some remote partition streams that are never polled. 5. For those streams, the first-poll gate never opens, so the worker never receives the corresponding `execute_task` request. 6. Since the worker never receives that request, it never materializes the worker-side execution streams whose drops would decrement the task-finalization counter. 7. The worker therefore never sends metrics for that task, leaving the worker->coordinator response stream open. 8. The coordinator is waiting in `drain_pending_tasks()` for that response stream to finish, while the query-end guard is still alive. 9. If metrics are moved to coordinator-channel EOS without also dropping the query-end guard before draining, the worker then waits for coordinator->worker EOS, but that EOS is gated by the same query-end guard. That creates the cycle. The following changes are made here: - Finalize worker task metrics when the coordinator channel reaches EOS instead of from the last partition stream drop. - Drop the query-end guard before draining coordinator background tasks so workers can observe the EOS being waited on. - Keep task invalidation tied to coordinator-channel shutdown. I think this is something #509 also noticed
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
2 participants
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.
No description provided.