Skip to content

fix(runtime): resume terminal failed pending workflows - #273

Closed
aviggiano wants to merge 3 commits into
mainfrom
fix/resume-terminal-pending-work
Closed

aviggiano wants to merge 3 commits into
mainfrom
fix/resume-terminal-pending-work

Conversation

@aviggiano

@aviggiano aviggiano commented Aug 6, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • fixes Resume terminal failed workflows with pending ready nodes #272
  • rewinds terminal failed workflow runs that still expose pending/runnable work and no failed tasks before issuing the detached resume
  • adds regression coverage for the R43 shape: run-level failed, one completed task, one pending task, no failed task rows

Validation

  • pnpm --filter @ultrafuzz/runtime test -- runtime.test.ts
  • pnpm --filter @ultrafuzz/runtime build
  • pnpm --filter @ultrafuzz/runtime exec prettier --check src/smithers.ts test/runtime.test.ts
  • git diff --check

R43 Evidence

R43 is preserved on the old immutable image. It reached all property-lens outputs, then repeated terminal-run-non-resumable with property-specification-fanin still pending/ready. A fresh image/run will be launched after this merges.

Greptile Summary

The PR extends failed-workflow resume recovery to detect pending tasks, rewind to an earlier usable frame, and transfer non-rewindable runs to a deterministic replacement lineage.

  • Broadens Smithers snapshot parsing across run-state and task-output variants.
  • Adopts an orphaned replacement lineage when fresh submission reports RUN_EXISTS.
  • Adds regression coverage for pending-work recovery, timeline edge cases, task phases, and lineage adoption.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/runtime/src/smithers.ts Adds pending-task detection, preceding-frame recovery, non-rewindable lineage replacement, and orphaned replacement adoption without an accepted blocking finding.
packages/runtime/test/runtime.test.ts Adds focused regression tests for task-phase parsing, failed runs with pending work, timeline edge cases, and replacement-lineage adoption.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Inspect failed workflow] --> B{Failed task rows?}
  B -->|Yes| C[Retry failed tasks]
  B -->|No| D{Pending workflow work?}
  D -->|No| E[Normal resume handling]
  D -->|Yes| F{Earlier timeline frame?}
  F -->|Yes| G[Rewind to preceding frame]
  F -->|No| H[Submit replacement lineage]
  H --> I{RUN_EXISTS?}
  I -->|Yes| J[Adopt replacement with resume]
  I -->|No| K[Use fresh replacement]
  C --> L[Detached resume]
  G --> L
  J --> M[Persist replacement run ID]
  K --> M
Loading

Reviews (4): Last reviewed commit: "fix(runtime): target a real frame and ad..." | Re-trigger Greptile

Context used:

@aviggiano
aviggiano force-pushed the fix/resume-terminal-pending-work branch from 7a76b53 to c4b71bf Compare August 6, 2026 09:48
A terminal failed workflow that still exposes pending ready work cannot be
resumed in place: the backend re-finalizes it as failed without dispatching.
The preceding-frame rewind only helps when the timeline has an earlier frame.

- read the run state from top-level `data.state` / `data.status` as well, so a
  terminal failed run reported in that shape is detected instead of resumed
  blindly
- extract the replacement-lineage submission into one helper and reuse it for a
  new `terminal-pending-non-rewindable` recovery when there is no frame to
  rewind to

Regression coverage: top-level `state`/`status` run snapshots, single-frame and
empty timelines transferring to a fresh `ufz-recovery-*` lineage, and a guard
that a render-failed run still rewinds to the latest frame and resumes in place.

Refs #272

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@aviggiano

Copy link
Copy Markdown
Collaborator Author

Second commit: 98190fb — non-rewindable terminal pending runs

c4b71bf rewinds to the preceding timeline frame, which only helps when the timeline has an earlier frame. Two gaps remained.

  1. smithersSnapshotRunState only read data.runState.state and data.run.status. A terminal failed run reported with the state at the top level of data was never classified as failed, so resume proceeded in place with no timeline read and no rewind. Added a top-level data.state then data.status fallback after the two existing lookups.
  2. With fewer than two timeline frames previousSmithersTimelineFrame returns undefined, no rewind happened, and the resume went straight back into the same terminal-run-non-resumable dead end that blocked R43. The replacement-lineage submission is now extracted into one helper and reused for a new terminal-pending-non-rewindable recovery, taken only when the run is terminal failed, has pending ready work, has no render failure, and has no frame to rewind to.

Regression coverage added

  • resume detects terminal pending work from top-level inspect run states — both state and status shapes, asserts timeline read plus rewind to the preceding frame.
  • resume transfers a single-frame terminal pending workflow to a valid durable lineage — asserts the fresh ufz-recovery-* run id, the updated run.json lineage, recovery: terminal-pending-non-rewindable in recovery-submission.json, and no rewind/timetravel/in-place resume.
  • resume transfers an empty-timeline terminal pending workflow to a valid durable lineage — zero-frame timelines take the same path.
  • resume still rewinds a render-failed workflow to the latest timeline frame — guards that render failures keep the pre-existing latest-frame rewind and in-place resume rather than transferring lineage.

Validation

  • pnpm --filter @ultrafuzz/runtime test — pass
  • pnpm --filter @ultrafuzz/runtime typecheck — pass
  • npx eslint packages/runtime/src/smithers.ts packages/runtime/test/runtime.test.ts — clean
  • npx prettier --check on both files — clean
  • git diff --check — clean

Note: compiled Smithers workflow passes a real non-executing graph smoke takes ~12s and trips bun's 5s default timeout when the file is run under bun test directly. The canonical runner is node --test via scripts/run-tests.mjs, which does not impose that limit. Pre-existing and unrelated.

R43 remains preserved and was never manually stopped or restarted.

Two review findings, both verified against the vendored smithers 0.31 sources.

Rewinding to the latest timeline frame is a no-op: jumpToFrame returns early
without touching the run row when the target equals the latest frame, and only
the non-latest path calls updateRun({status:"running", errorJson:null}). So
frame selection must key on pending work, not on whether the failure was also
reported as a render failure, or a render failure that leaves pending work
rewinds to latest, changes nothing, and re-sticks.

The replacement run id is a deterministic hash of the original, and `up`
without --resume exits 4 with RUN_EXISTS. A generation that created the
replacement run and died before the new lineage was persisted therefore wedged
every later generation on the same id forever. Adopt the orphan by resuming it.

Regression coverage: preceding-frame rewind for a render failure that still has
pending work, latest-frame rewind when it does not, and RUN_EXISTS adoption.

Refs #272

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@aviggiano

Copy link
Copy Markdown
Collaborator Author

Converting to draft — the premise of this PR does not survive review

Two independent reviews ran on 98190fb. One found no blocking defect; the other argued the fix cannot work. I verified the disputed claims myself against the vendored smithers-orchestrator@0.31.0 sources, and the skeptical review is right on the decisive point.

Verified, and fatal to this approach

  • activateRunForResume (engine/src/engine.js:7152-7175) already patches the claimed run with status: "running", finishedAtMs: null, errorJson: null, and "failed" is in RESUMABLE_RUN_STATUSES (engine.js:2968-2978). up --resume already un-terminalizes a failed run, so the rewind this PR is built around contributes nothing at the run-row level.
  • jumpToFrame returns early with zero mutation when the target equals the latest frame (time-travel/src/jumpToFrame.js:811-824). The only differential effect of a preceding-frame rewind is to un-complete the newest frame's nodes — it destroys completed work rather than unblocking anything.
  • DEPENDENCY_DEADLOCK reproduces R43's signature exactly — run failed, zero failed nodes, deferred tasks left pending (driver/src/WorkflowDriver.js:510-518) — and a rewind does not repair it.

R43 evidence checked directly

WORKFLOW_RENDER_FAILED does not appear anywhere in R43's preserved evidence, so the render-failure branch was never involved. Attempt 1 exited dependency-unreachable, which is what waitForTerminalRun raises when it never observes advancement (modal/src/worker.ts:281) — not a network condition. Attempts 2-4 ran ~108 s each and exited terminal-run-non-resumable with counts pinned at 21 / 0 / 11.

Also credited from review: the fresh-lineage path is unreachable for R43 (frames are persisted on every driver render, so a 2-hour run has a long history and previousSmithersTimelineFrame always returns a value), and where it does fire, cloud node checkpoints are namespaced by smithers run id (modal/src/node-provider.ts:341-343), so a new lineage means new volume, no checkpoint, full re-execution — then republishing into the same durable run root risks tripping the immutable-publication guard.

What I have kept

01b9aee lands the two findings that are genuinely real and independently correct: frame selection now keys on pending work rather than on renderFailed (because a latest-frame rewind is a proven no-op), and an orphaned replacement lineage is adopted via --resume instead of wedging forever on the deterministic id's RUN_EXISTS. Tests, typecheck, eslint and prettier are clean. But these fix defects in a recovery path whose value is now in doubt, so this stays a draft rather than merging.

The real mechanism and the fix direction are now recorded in #275, with the verified facts separated from the remaining hypothesis and the exact measurement that would confirm it. Recommend #272 be resolved via #275.

R43 remains preserved and was never stopped or restarted.

@aviggiano

Copy link
Copy Markdown
Collaborator Author

Closing unmerged. The premise is refuted by the author's own withdrawal: activateRunForResume already un-terminalizes, and jumpToFrame returns early when the target frame is the latest — so the rewind this PR adds is a no-op. release-gates is red on a single assertion (expects rewind … 4, gets 2) and the branch is 17 commits behind main.

Three pieces are independently correct and are now tracked in #324 for salvage off current main: the RUN_EXISTS replacement-lineage adoption, removal of the no-op rewind, and the top-level fallback in smithersSnapshotRunState.

Tracked in #327.

@aviggiano aviggiano closed this Aug 7, 2026
aviggiano added a commit that referenced this pull request Aug 7, 2026
…and #278 (#338)

Recovers the four pieces of #273/#278 that survived review, onto current
main, without their refuted premises.

- Adopt the deterministic replacement lineage when a fresh submission
  reports RUN_EXISTS. An earlier recovery generation can create the
  replacement run and die before the new lineage is persisted; every
  later generation then recomputes the same id and wedges on exit 4.
- Drop the rewind to the latest timeline frame. jumpToFrame returns
  early with zero mutation when the target equals the latest frame
  (time-travel/src/jumpToFrame.js:812-824) and up --resume already
  un-terminalizes a failed run, so the timeline read and the rewind
  were two subprocesses per generation that changed nothing.
- Fall back to a top-level data.state / data.status in
  smithersSnapshotRunState, for snapshot shape variants that carry no
  runState or run wrapper.
- Add smithersSnapshotUnverifiedDependencies, which reads a dependent
  prepare: failure off the run row. Proven against R43's real
  error_json; the recovery built on it is #288's job.

The refuted premises are deliberately left out: no rewind to an earlier
frame, no verifier reset, no artifact quarantine.

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resume terminal failed workflows with pending ready nodes

1 participant