fix(native): Do not discard all tasks in maybeStartNextQueuedTask() (#27548)#27548
Open
spershin wants to merge 1 commit intoprestodb:masterfrom
Open
fix(native): Do not discard all tasks in maybeStartNextQueuedTask() (#27548)#27548spershin wants to merge 1 commit intoprestodb:masterfrom
spershin wants to merge 1 commit intoprestodb:masterfrom
Conversation
|
Contributor
Reviewer's GuideAdjusts TaskManager's queued task handling so that aborted or invalid tasks are skipped instead of causing all sibling tasks in the same queue entry to be discarded, and adds a regression test to validate the new behavior when one queued task is aborted. Sequence diagram for updated queued task start behavior in TaskManagersequenceDiagram
participant TaskManager
participant TaskQueue
participant QueueEntry
participant TaskA
participant TaskB
TaskManager->>TaskQueue: maybeStartNextQueuedTask()
TaskQueue-->>TaskManager: front() returns QueueEntry
TaskManager->>TaskQueue: pop_front()
loop over queuedTasks in QueueEntry
TaskManager->>QueueEntry: lock(queuedTask A)
QueueEntry-->>TaskManager: TaskA
alt TaskA is null or TaskA.task is null
TaskManager->>TaskManager: log Skipping null task
Note over TaskManager: continue to next queuedTask
else TaskA is non-null
TaskManager->>TaskA: taskState()
TaskA-->>TaskManager: state != kPlanned
alt state != kPlanned
TaskManager->>TaskManager: log Discarding aborted task
Note over TaskManager: continue to next queuedTask
end
end
TaskManager->>QueueEntry: lock(queuedTask B)
QueueEntry-->>TaskManager: TaskB
TaskManager->>TaskB: taskState()
TaskB-->>TaskManager: state == kPlanned
TaskManager->>TaskManager: add TaskB to tasksToStart
end
alt tasksToStart is not empty
TaskManager->>TaskManager: proceed to start tasks in tasksToStart
TaskManager->>TaskB: start()
else tasksToStart is empty
TaskManager->>TaskManager: continue checking next QueueEntry
end
Class diagram for TaskManager queued task handling changesclassDiagram
class TaskManager {
+maybeStartNextQueuedTask() void
-taskQueue std::deque~std::vector~std::weak_ptr~QueuedTask~~>
}
class QueuedTask {
+task std::shared_ptr~VeloxTaskWrapper~
+info TaskInfo
+taskState() PrestoTaskState
}
class VeloxTaskWrapper {
+task void*
}
class TaskInfo {
+taskId std::string
}
class PrestoTaskState {
<<enumeration>>
kPlanned
kRunning
kAborted
kFinished
kFailed
}
TaskManager "1" --> "*" QueuedTask : manages
QueuedTask "1" --> "1" VeloxTaskWrapper : has
QueuedTask "1" --> "1" TaskInfo : has
QueuedTask "1" --> "1" PrestoTaskState : returns
note for TaskManager "maybeStartNextQueuedTask now skips aborted or null tasks within a queue entry and starts remaining valid tasks instead of discarding the whole entry"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
queuedTaskAbortDoesNotBlockSiblings, you modify the globalSystemConfig::kWorkerOverloadedTaskQueuingEnabledflag but never restore it, which can leak state into other tests; consider saving the previous value and resetting it in a scope guard or test teardown. - The new
maybeStartNextQueuedTasklogic now silently skips queue entries where all tasks are invalid; if this is expected, it might be useful to add a single INFO/WARNING log when an entire queue entry is discarded so operators can distinguish this from normal successful dequeues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `queuedTaskAbortDoesNotBlockSiblings`, you modify the global `SystemConfig::kWorkerOverloadedTaskQueuingEnabled` flag but never restore it, which can leak state into other tests; consider saving the previous value and resetting it in a scope guard or test teardown.
- The new `maybeStartNextQueuedTask` logic now silently skips queue entries where all tasks are invalid; if this is expected, it might be useful to add a single INFO/WARNING log when an entire queue entry is discarded so operators can distinguish this from normal successful dequeues.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
spershin
pushed a commit
to spershin/presto
that referenced
this pull request
Apr 9, 2026
…restodb#27548) Summary: Skip any tasks that have been aborted or are no longer valid in maybeStartNextQueuedTask(), but still start the remaining valid tasks. This avoids a bug where aborting one task (e.g. from a completed fragment) would silently discard other still-valid tasks from the same query. Example of the query that got stuck due to this: 20260404_172612_64504_mgsfz Differential Revision: D100103601
be00a1f to
26b90b7
Compare
spershin
pushed a commit
to spershin/presto
that referenced
this pull request
Apr 9, 2026
…restodb#27548) Summary: Skip any tasks that have been aborted or are no longer valid in maybeStartNextQueuedTask(), but still start the remaining valid tasks. This avoids a bug where aborting one task (e.g. from a completed fragment) would silently discard other still-valid tasks from the same query. Example of the query that got stuck due to this: 20260404_172612_64504_mgsfz ``` == NO RELEASE NOTE == ``` Differential Revision: D100103601
26b90b7 to
50ac510
Compare
…restodb#27548) Summary: Pull Request resolved: prestodb#27548 Skip any tasks that have been aborted or are no longer valid in maybeStartNextQueuedTask(), but still start the remaining valid tasks. This avoids a bug where aborting one task (e.g. from a completed fragment) would silently discard other still-valid tasks from the same query. Example of the query that got stuck due to this: 20260404_172612_64504_mgsfz ``` == NO RELEASE NOTE == ``` Differential Revision: D100103601
50ac510 to
7bbd0fb
Compare
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.
Summary:
Skip any tasks that have been aborted or are no longer valid in
maybeStartNextQueuedTask(), but still start the remaining valid tasks. This
avoids a bug where aborting one task (e.g. from a completed fragment) would
silently discard other still-valid tasks from the same query.
Example of the query that got stuck due to this: 20260404_172612_64504_mgsfz
Differential Revision: D100103601