fix(core): always write decider-queue postpone - #1492
Conversation
…he default offset WorkflowExecutorOps.decide() only called queueDAO.setUnackTimeout() when the computed postpone duration differed from workflowOffsetTimeout. A workflow whose only active task is a WAIT with no explicit timeout (or a HUMAN/ active-SUB_WORKFLOW task, per ExecutorUtils.computePostpone) always computes exactly workflowOffsetTimeout, so the write was skipped on every decide() call for that workflow. Because the write never happened, the queue_message row's deliver_on stayed frozen at whatever value preceded it. A frozen deliver_on in the past is always "due", so the workflow got re-swept roughly every ~60 seconds forever (paced by the unrelated queue-unack recovery cycle) instead of once per workflowOffsetTimeout as intended - a permanent, silent busy-loop with no error logged, for every workflow that ever lands in this exact state. Fix: drop the equality guard and always write the postpone for non-terminal workflows. Added a regression test that reproduces the bug via a WAIT task with waitTimeout=0 and asserts setUnackTimeout still fires.
| queueDAO.setUnackTimeout( | ||
| DECIDER_QUEUE, | ||
| workflow.getWorkflowId(), | ||
| updatedOffset.getSeconds() * 1000); |
There was a problem hiding this comment.
Context
Thanks, the busy loop this targets is real, a workflow whose deliver_on is stuck in the past does need the postpone to advance. My concern with dropping the equality guard is that decide now writes setUnackTimeout with deliver_on = now + offset unconditionally on every decide, and that write is last writer wins on the deliver_on column with no guard (PostgresQueueDAO.java:212 to 227), while expediteLazyWorkflowEvaluation sets deliver_on to now under a different lock.
▎ Example: Parent P has an active SUB_WORKFLOW task, so computePostpone returns exactly workflowOffsetTimeout. A decide(P) holds P's lock and reads P with the sub workflow still in progress. At the same time child C completes, and completeWorkflow(C) runs under C's lock, never takes P's lock, and calls expediteLazyWorkflowEvaluation(P) which does postpone(DECIDER_QUEUE, P, EXPEDITED_PRIORITY, 0) to force immediate re-evaluation (WorkflowExecutorOps.java:2578 to 2586). If the decide write lands after that expedite, deliver_on becomes now + 30s and the nudge is overwritten, so the parent does not observe the child completion or satisfy the JOIN for up to workflowOffsetTimeout. Before this change the write was skipped in this offset equals default case, so the expedite always won.
Suggestion
setUnackTimeoutIfShorter alone does not fix it either, because when deliver_on is frozen in the past now + 30s is later, so the shorter guard declines and the busy loop stays. The write probably needs to advance deliver_on only when the existing value is at or behind now or absent, and otherwise take the min of the existing value and now + offset. A regression test that enqueues an expedite with deliver_on 0 before a decide whose offset equals workflowOffsetTimeout and asserts deliver_on is not pushed back would lock this down.
NOTE
It would also be worth confirming the extra per decide write on the hot path is acceptable churn on the queue table.
Pull Request type
Changes in this PR
WorkflowExecutorOps.decide()only calledqueueDAO.setUnackTimeout()when the postpone duration computed byExecutorUtils.computePostpone()differed fromworkflowOffsetTimeout:Several common cases in computePostpone() always return exactly workflowOffsetTimeout — a WAIT task with no explicit timeout, an in-progress HUMAN task, and an active SUB_WORKFLOW task all fall back to this default. For any workflow currently in one of these states, the != check is false on every single decide() call, so the write is skipped
every time, indefinitely.
Because the write never happens, the queue_message row's deliver_on stays frozen at whatever value preceded it. Since a deliver_on in the past is always "due", the decider-queue sweeper picks the workflow up again on its next pass — but since the write is skipped again, deliver_on never advances. In practice this settles into a permanent busy-loop: the workflow is repeatedly popped and re-swept roughly every ~60 seconds forever (paced by the queue implementation's own unrelated unack-recovery cycle, since nothing else is releasing/rescheduling it), instead of once per workflowOffsetTimeout as intended. No error is ever logged — decide() completes successfully every time — so this is silent.
At scale (e.g. a fleet with many long-lived workflows idling on WAIT/HUMAN tasks), this produces a large population of workflows that occupy sweeper capacity every minute without ever needing attention, which crowds out genuine work and inflates decider-queue depth and processing latency fleet-wide.
Fix: drop the equality guard and always write the postpone for non-terminal workflows. The computed duration is always meaningful — it's either a real task-driven deadline or the configured default — so there's no case where skipping the write is correct.