Skip to content

fix: add retry logic for resume race condition with pause persistence #3577

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
guoyangzhen wants to merge 2 commits into
base: main
Choose a base branch
from
+30 −5
Open

fix: add retry logic for resume race condition with pause persistence #3577

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
278daf6
fix: add retry for resume race condition (#3081)
guoyangzhen Mar 14, 2026
d16d7bd
fix: correct off-by-one in retry attempts (addressing review feedback)
guoyangzhen Mar 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 30 additions & 5 deletions apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,20 @@ export class PauseResumeManager {
static async enqueueOrStartResume(args: EnqueueResumeArgs): Promise<EnqueueResumeResult> {
const { executionId, contextId, resumeInput, userId } = args

return await db.transaction(async (tx) => {
const pausedExecution = await tx
.select()
.from(pausedExecutions)
// Retry to handle race condition where resume request arrives
// before persistPauseResult commits the paused execution row.
// The INSERT in persistPauseResult is awaited, so the race window
// is only between the method call and the await returning (~10-50ms).
const MAX_RETRIES = 3
const RETRY_DELAY_MS = 200
let lastError: Error | null = null

for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) {
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off-by-one: 4 attempts, not 3

The loop condition attempt <= MAX_RETRIES with MAX_RETRIES = 3 results in 4 total attempts (attempts 0, 1, 2, 3), not 3 as stated in the PR description and comments. The variable name MAX_RETRIES typically means the number of retries after the initial try, so 3 retries would mean 4 total attempts. However, the comment says "3 attempts" which implies only 3 total. This discrepancy can lead to confusion and unexpected behavior (an extra 200ms delay on the third retry).

If the intent is 3 total attempts:

Suggested change
const MAX_RETRIES = 3
const RETRY_DELAY_MS = 200
let lastError: Error | null = null
for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) {
const MAX_ATTEMPTS = 3
const RETRY_DELAY_MS = 200
let lastError: Error | null = null
for (let attempt = 0; attempt < MAX_ATTEMPTS; attempt++) {

If the intent is 3 retries (4 total attempts), update the comment to say "4 attempts (1 initial + 3 retries)".

try {
return await db.transaction(async (tx) => {
const pausedExecution = await tx
.select()
.from(pausedExecutions)
.where(eq(pausedExecutions.executionId, executionId))
.for('update')
.limit(1)
Expand Down Expand Up @@ -277,7 +287,22 @@ export class PauseResumeManager {
resumeInput,
userId,
}
})
})
} catch (err: any) {
lastError = err
const isNotFound = err.message?.includes('Paused execution not found')
const isLastAttempt = attempt === MAX_RETRIES

if (!isNotFound || isLastAttempt) {
throw err
}
Comment on lines +291 to +298
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fragile string-based error matching

The retry selector err.message?.includes('Paused execution not found') couples the retry logic to the exact wording of the error message. If the message on line 187 is ever changed, refactored, or translated, the retry silently stops working — without any compile-time or test-time warning.

A more robust approach is to use a typed custom error class so the check is structural rather than textual:

class PausedExecutionNotFoundError extends Error {
  constructor() {
    super('Paused execution not found or already resumed')
    this.name = 'PausedExecutionNotFoundError'
  }
}

// At throw site (line 187):
throw new PausedExecutionNotFoundError()

// In the catch block:
const isNotFound = err instanceof PausedExecutionNotFoundError

This makes the intent explicit and refactor-safe.


await new Promise((resolve) => setTimeout(resolve, RETRY_DELAY_MS))
}
}

// This should never be reached due to the for loop logic, but TypeScript needs it
throw lastError ?? new Error('enqueueOrStartResume failed after retries')
}

static async startResumeExecution(args: StartResumeExecutionArgs): Promise<void> {
Expand Down