fix(sweeper): re-queue on lock miss to eliminate multi-minute workflow pauses#1259
Open
NicholasDCole wants to merge 3 commits into
Open
fix(sweeper): re-queue on lock miss to eliminate multi-minute workflow pauses#1259NicholasDCole wants to merge 3 commits into
NicholasDCole wants to merge 3 commits into
Conversation
…w pauses The active WorkflowSweeper.sweep() bare-returned when it couldn't acquire the workflow lock, doing nothing to advance or reschedule the workflow. A lock-contended workflow (e.g. at a JOIN boundary) then fell back to its decider-queue entry, which a polled task postpones out to responseTimeoutSeconds via ExecutionService.adjustDeciderQueuePostpone(). With responseTimeoutSeconds=600 this surfaces as an intermittent ~10-minute pause per contended transition (reported on 3.30.2 with Spanner persistence and Redis queue/locks). Fix: on a lock miss, re-queue the workflow to the decider queue with a bounded backoff (lockLeaseTime/2, capped by maxPostponeDurationSeconds) so it is retried in seconds instead of parking. The identical backoff math previously inlined in the decide()==null path is factored into a shared lockContentionBackoffSeconds() helper. The existing "Couldn't acquire lock to sweep workflow" error log is kept verbatim so log-based diagnostics still fire. Tests: - WorkflowSweeperTest.sweepReQueuesOnLockMissWithBoundedBackoffInsteadOfParking regression: fails before the fix (no push), passes after (30s re-queue). - WorkflowSweeperTest.sweepReQueuesWith30sBackoffWhenDecideCannotAcquireLock pins the decide()==null backoff at lockLeaseTime/2. - ExecutionServiceTest.testPollSetsDeciderQueuePostponeToResponseTimeout_reproducesTenMinutePause documents the 600s decider postpone that is the source of the pause duration. - TestWorkflowSweeper: legacy-sweeper repro of the same responseTimeout-driven fallback, retained as a diagnostic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pause Adds an integration spec that drives the real DynamicFanInOutTest workflow (integration_task_1 -> FORK_JOIN_DYNAMIC -> JOIN -> integration_task_4) through real fork/JOIN execution and validates the sweeper lock-miss re-queue fix. - Regression feature: holds the workflow lock from a foreign thread, executes the JOIN (completes, but the post-JOIN decide can't get the lock so integration_task_4 is not scheduled -> the stall), sweeps under contention and asserts the workflow was re-queued to DECIDER_QUEUE (containsMessage), then releases the lock and confirms it runs to COMPLETED. Fails against the pre-fix bare-return (containsMessage == false); passes with the fix. - Control feature: with the lock free, the same JOIN schedules the next task in the same decide (no stall) - isolates the lock as the sole variable. Uses LocalOnlyLock (the harness lock; the lock-miss branch is implementation- agnostic) with the real Redis-backed decider queue the harness already provisions via Testcontainers. Determinism comes from the fix's lockLeaseTime/2 (30s) re-queue backoff: the re-pushed message is not due, so the background sweeper cannot pop it during the containsMessage assertion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Problem
Reported on Conductor 3.30.2 (Spanner persistence, Redis queue + locks): a workflow with many JOIN boundaries stalls for an intermittent ~10 minutes per run after upgrading from 3.21.4.
Root cause
The active sweeper (
org.conductoross.conductor.core.execution.WorkflowSweeper, default viaconductor.app.sweeper.enabled) bare-returned when it couldn't acquire the workflow lock — nodecide(), no re-queue:A lock-contended workflow (e.g. the post-completion
decide()at a FORK/JOIN boundary) then had no near-term wake-up. Its only remaining decider-queue entry is postponed byExecutionService.adjustDeciderQueuePostpone()toresponseTimeoutSeconds * 1000when a worker polls a task. With the affected task defs carryingresponseTimeoutSeconds=600, that fallback is 600 s ≈ 10 minutes — the observed pause. It's intermittent ("sometimes") because it only surfaces when the event-driven decide is missed.Fix
On a lock miss, re-queue the workflow to the decider queue with a bounded backoff (
lockLeaseTime/2, capped bymaxPostponeDurationSeconds) so a contended workflow is retried in seconds instead of parking:decide()==nullpath is factored into a sharedlockContentionBackoffSeconds()helper."Couldn't acquire lock to sweep workflow"error log is kept verbatim so existing log-based diagnostics/alerts still fire.responseTimeout-based postpone for genuinely long-running tasks.Tests
WorkflowSweeperTest.sweepReQueuesOnLockMissWithBoundedBackoffInsteadOfParkingWorkflowSweeperTest.sweepReQueuesWith30sBackoffWhenDecideCannotAcquireLockdecide()==nullbackoff atlockLeaseTime/2ExecutionServiceTest.testPollSetsDeciderQueuePostponeToResponseTimeout_reproducesTenMinutePauseTestWorkflowSweeper(legacy path)responseTimeout-driven fallback, retained as a diagnosticAll green; spotless clean.
Note for affected deployments
As an immediate config mitigation before adopting a build with this fix, lowering
responseTimeoutSecondson the task defs (e.g. 600 → 60) shrinks the fallback pause proportionally.Test evidence (validated with / without the fix)
All tests were run both with the fix in place and with the lock-miss re-queue reverted (call site A in
WorkflowSweeper.sweep()restored to the original barereturn, i.e. deletequeueDAO.push(DECIDER_QUEUE, workflowId, 0, backoffSeconds)), on the same commit otherwise. The two tests that assert the fix flip from pass → fail when the fix is removed, confirming they are genuine regression gates (not vacuous). Environment:local_onlyexecution lock; the e2e spec runs against the real Redis-backed decider queue the harness provisions via Testcontainers (redis:6.2-alpine).✅ With the fix (current branch)
❌ With the fix reverted (bare
returnon lock miss)Unit regression —
WorkflowSweeperTest.sweepReQueuesOnLockMissWithBoundedBackoffInsteadOfParking(tests=6, failures=1):The contended
sweep()never re-queued the workflow — exactly the bug (the 30s =lockLeaseTime/2backoff push is absent).E2E —
DynamicForkJoinLockContentionSpec› "sweeper re-queues a lock-contended workflow…" (tests=2, failures=1; the control still passes):After the JOIN completes under a held workflow lock (post-JOIN
decide()returns null,integration_task_4never scheduled) and the decider entry is cleared, a contendedsweep()leaves nothing in_deciderQueuefor the workflow — it is parked with no near-term wake-up (in production, until theresponseTimeoutSeconds=600 postpone fires). The control case (no contention) schedulesintegration_task_4immediately in both variants, isolating the lock as the sole cause.Restore check
Restoring the fix returns
WorkflowSweeper.javato this PR's committed content (git diffempty) and both suites go green again.🤖 Generated with Claude Code