aws / aws/aws-durable-execution-sdk-js

Checkpoint failure during queue drain is swallowed; execution can be reported SUCCEEDED

Open
#869 3 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
84
Forks
28
Avg merge
1d 17h
Merged PRs (30d)
43

Description

## Summary

If a checkpoint batch fails while `runHandler` is draining the checkpoint queue **after** the handler promise has already won the race at L211, the failure is never observed and the invocation returns the handler's result as **SUCCEEDED**. The operation's checkpoint was discarded (`clearQueue()`), so it is neither persisted nor retried.

The root of it is that `waitForQueueCompletion()` has no way to report a failure to its caller, and the one signal that *is* raised — a termination-promise resolution — arrives after `runHandler` has already committed to the race outcome.

## Mechanism

**1. `waitForQueueCompletion()` can only resolve.** It captures `resolve` and never `reject` (`src/utils/checkpoint/checkpoint-manager.ts`):

```ts
async waitForQueueCompletion(): Promise {
if (this.queue.length === 0 && !this.isProcessing) return;
return new Promise((resolve) => { this.queueCompletionResolver = resolve; });
}
```

**2. It is resolved on the failure path too.** `processQueue`'s `catch` classifies the error, discards the queue, and raises termination; the `finally` then resolves the waiter regardless of outcome:

```ts
} catch (error) {
const checkpointError = this.classifyCheckpointError(error);
this.clearQueue(); // pending checkpoints discarded
this.terminationManager.terminate({
reason: TerminationReason.CHECKPOINT_FAILED,
message: checkpointError.message,
error: checkpointError,
});
} finally {
this.isProcessing = false;
if (this.queue.length > 0) { setImmediate(() => this.processQueue()); }
else { this.notifyQueueCompletion(); ... } // resolves waitForQueueCompletion()
}
```

**3. So the `try/catch` around the drain in `runHandler` cannot catch this** (`src/with-durable-execution.ts` L221-226). There is no rejection to catch; the `catch` is reachable only for a synchronous throw from the call itself:

```ts
try {
await durableExecution.checkpointManager.waitForQueueCompletion();
} catch (error) {
log("⚠️", "Error waiting for checkpoint completion:", error);
}
```

**4. The only failure signal is the termination promise, and it arrives too late.** `runHandler` commits to the outcome at L211, before the drain:

```ts
const [resultType, result] = await Promise.race([handlerPromise, terminationPromise]);
```

If the handler resolved first, `resultType === "handler"`. `TerminationManager.terminate()` then resolves `terminationPromise`, but `Promise.race` has already settled and nothing observes the late resolution.

**5. Every post-drain branch is gated on `resultType === "termination"`** — `CHECKPOINT_FAILED` (L230), `SERDES_FAILED` (L240), `CONTEXT_VALIDATION_ERROR` (L249), `CONFIG_VALIDATION_ERROR` (L277), and the generic `PENDING` branch (L298). With `resultType === "handler"` none match, so control reaches the normal completion path at L302+ and the execution is reported successful.

## Impact

An operation checkpoint that failed and was discarded is reported as a successful execution. The comments at the sibling drain sites read *"Continue anyway - the checkpoint will be retried on next invocation"*, which does not hold here: returning a result means there is no next invocation, so the discarded checkpoint is never retried. The classified error (`CheckpointUnrecoverableInvocationError` / `CheckpointUnrecoverableExecutionError`) is dropped, no error appears in the response, and the only trace is a debug-level log line from `processQueue`.

Note the *execution result* checkpoint (L346-L380) does rethrow its own failures. The gap is the earlier operation checkpoints drained at L222.

## The termination path is protected

For contrast, when termination wins the race this works correctly. `shouldTerminate()` refuses to terminate while the queue is non-empty (Rule 1), while a checkpoint is processing (Rule 2), or while force-checkpoint promises are outstanding (Rule 3), and `scheduleTermination`'s cooldown timer re-checks `shouldTerminate()` before firing. A checkpoint failing during active processing therefore resolves the race *as* `CHECKPOINT_FAILED` and is correctly rethrown at L230. The problem is specific to the handler winning the race while a checkpoint is still in flight.

## Expected behaviour

A drain failure should be observable by the caller regardless of which promise won the race. Either:

- have `waitForQueueCompletion()` reject (or resolve with a status) on failure, which would make the existing `try/catch` meaningful; and/or
- have `runHandler` consult the termination manager's state after the drain — `terminate()` already sets `isTerminated` and records `terminationDetails` — rather than relying solely on the `result` captured at L211.

## Related: completions are only learned from the checkpoint response

`processBatch` ingests operation updates solely from `response.NewExecutionState.Operations`, and `getExecutionState` is called only during initialization (`src/context/execution-context/execution-context.ts`), so there is no fallback re-read of full state within an invocation. If a checkpoint request is applied server-side but its response is lost, the terminal statuses it carried are never reconciled into `stepData`; the awaited operation stays `IDLE_AWAITED` and the invocation suspends with `PENDING` while the service considers that operation complete.

## Relationship to #865

Same family. `runHandler`'s post-race dispatch enumerates conditions individually and defaults to success on the handler path and to `PENDING` on the termination path, so an unhandled or late-arriving condition is silently reported as progress. #865 covers `TerminationReason.CUSTOM` having no case. A default-deny structure would prevent future reasons inheriting the same masking.

## Version

Observed in `2.3.0`; the same code is present on `main` at `5cf7f692`.

Contributor guide

Open the contributing guide

Research direction

Start in src/utils/checkpoint/checkpoint-manager.ts by tracing waitForQueueCompletion(), processQueue(), and notifyQueueCompletion(), then follow the drain handling in src/with-durable-execution.ts around L211 and L221-226. Verify that a checkpoint failure after the handler wins the race remains observable, the discarded checkpoint is not reported as successful, and existing termination-path behavior stays correct.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
backend-api-design, distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.