Stop recovery adopting workflows this executor is already running

[devhawk]

Fixes #488, fixes #491. Very likely fixes #477 (see caveat below). Fixes #490, which is unrelated to the rest.

One defect behind three reports

#477, #488 and #491 are the same bug: the launch-time recovery sweep adopting a workflow that this process started microseconds earlier, and running its body a second time.

start() bounds the sweep with withEndTime(Instant.now()), read at the end of launch, and the filter is created_at <= endTime at millisecond granularity. There is nothing between that read and the caller's first startWorkflow, so the two land in the same millisecond and the just-inserted row is <= the cutoff. From the #488 job:

20:35:25.577  DBOS Executor started
20:35:25.577  startWorkflow ... insertWorkflowStatus outcome-ownership-failed-deleted-...
20:35:25.579  executeWorkflow task throwingWorkflow ...        <- run A (the test's)
20:35:25.580  recoverWorkflow execute outcome-ownership-...
20:35:25.581  insertWorkflowStatus outcome-ownership-...       <- row re-inserted
20:35:25.583  executeWorkflow task throwingWorkflow ...        <- run B (recovery's)
20:35:25.583  updateWorkflowOutcome ... status ERROR           <- run A
20:35:25.584  updateWorkflowOutcome ... status ERROR           <- run B

Both copies run to completion and race to record the outcome. What each test noticed differed:

• Launch-time recovery can re-execute a workflow that is already running in-process (duplicate execution) #491 (AsyncWorkflowTest.sameWorkflowId) — the body simply ran twice, so executionCount was 2.
• WorkflowOutcomeOwnershipTest.failedRunFailsFastWhenItsRowIsDeleted is flaky (reported its own error instead of DBOSNonExistentWorkflowException) #488 (WorkflowOutcomeOwnershipTest.failedRunFailsFastWhenItsRowIsDeleted) — recovery's re-insert resurrected the row the test had deleted, so the live run's UPDATE ... WHERE status = 'PENDING' matched after all, it owned the outcome, and it reported its own IllegalStateException instead of failing fast. That is why WorkflowOutcomeOwnershipTest.failedRunFailsFastWhenItsRowIsDeleted is flaky (reported its own error instead of DBOSNonExistentWorkflowException) #488 read as a visibility anomaly; it wasn't one.
• CancelResumeRaceTest.activeIdReleasedBeforeOutcomeWriteTest is flaky (expected 2 but was 3) #477 (CancelResumeRaceTest) — a recovery duplicate of run 1 makes the resumed dispatch the third run, which is the expected: <2> but was: <3> reported.

ActiveWorkflowGuard does not stop this. It is released just before the terminal outcome write, so a recovery landing in that window acquires the ID and re-runs the body.

The change

The sweep's cutoff moves to the top of start(), before the isRunning gate. Everything start() goes on to do — the app-version hash over every registered workflow, the executor service, migrations, queue and scheduler start, listener callbacks — now lands between the read and the earliest a caller can start a workflow.

The one-millisecond step-back stays, doing a different job: placement buys margin, the step-back makes the bound exclusive. created_at is written from System.currentTimeMillis() (the same clock this reads), so <= cutoff - 1ms is exactly < cutoff on a millisecond-granular column. Without it a start() that finishes inside a millisecond — already-migrated database, no queues, no listeners — reopens the same hole.

recoverWorkflow skips a workflow whose ID is active on this executor. recoverPendingWorkflows(), the Conductor/AdminServer entry point, carries no time bound at all, so this is the only thing standing between an operator-triggered recovery and a duplicate execution.

What this does not fix

The launch path is now closed deterministically: the sweep can only see workflows created strictly before this process started, and none of those is running in it.

The operator-triggered path is narrowed, not closed. The active-ID check is a sample, not a lock, and during the ActiveWorkflowGuard release window the ID is absent from the map — so a recovery landing exactly there still passes the check. Closing that means changing the release-before-write ordering, which is the invariant CancelResumeRaceTest exists to pin down, and is a design call rather than a flaky-test fix. Left alone deliberately.

Separately: #490

DBOSExecutorTest.sleep asserted a 2s sleep returns within 2400ms. That measures the round trip around the sleep — launch, checkpoint, poll, resume, outcome write — not the sleep, so it reddens on whichever job is slowest. Widened to a bound only a repeated sleep could break, the same correction #482 made to ChaosTest and the queue tests, which missed this one.

Testing

RecoveryServiceTest.recoveryLeavesAWorkflowThisExecutorIsRunningAlone drives recovery directly, since the millisecond collision that produced these failures is not something a test can schedule. Confirmed it fails without the recoverWorkflow guard — recovery re-inserts the deleted row — and passes with it.

33 tests pass across RecoveryServiceTest, WorkflowOutcomeOwnershipTest, AsyncWorkflowTest, CancelResumeRaceTest and DBOSExecutorTest; spotlessCheck, compileTestJava and javadoc clean.

I have left #477 for CI to confirm rather than closing it on reasoning: its original run logs have expired, so the link to this root cause is inferred from the test's shape (setUp() launches, runCancelAndPark() starts a workflow immediately) rather than observed. Worth a few green runs before trusting it.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Moves launch recovery cutoff before executor startup and skips active in-process workflows; also relaxes the sleep timing test.

Changes:

• Prevents startup recovery from adopting newly created workflows.
• Adds an active-workflow recovery guard and regression test.
• Replaces the overly strict sleep timing bound.

File summaries

File	Description
transact/src/test/java/dev/dbos/transact/execution/RecoveryServiceTest.java	Updated as part of this pull request.
transact/src/test/java/dev/dbos/transact/execution/DBOSExecutorTest.java	Updated as part of this pull request.
transact/src/main/java/dev/dbos/transact/execution/DBOSExecutor.java	Updated as part of this pull request.

Review details

Suppressed comments (2)

transact/src/main/java/dev/dbos/transact/execution/DBOSExecutor.java:1220

• An active entry guarantees this workflow already inserted a row, so this branch should use the same fail-if-missing behavior as the adoption path. If recovery races deletion (as this test does), retrieveWorkflow returns a polling handle with failIfMissing=false, so getResult() waits forever for a row that recovery deliberately did not recreate instead of reporting DBOSNonExistentWorkflowException. Return a fail-if-missing handle here.

      return retrieveWorkflow(workflowId);

transact/src/main/java/dev/dbos/transact/execution/DBOSExecutor.java:1220

• This is only a sample check, not an ownership handoff. ActiveWorkflowGuard.release() removes the ID before the terminal outcome write, so recovery arriving in that interval sees no entry, re-inserts/executes the workflow, and can still duplicate its side effects. Since recoverPendingWorkflows has no cutoff, this path still violates the no-duplicate guarantee; recovery must coordinate with outcome ownership or the active entry must cover that window.

    if (activeWorkflows.containsKey(workflowId)) {
      logger.debug("recoverWorkflow skip active {}", workflowId);
      return retrieveWorkflow(workflowId);

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.