Skip to content

fix(reconciler): ms-truncate standalone CAS guard; stop auto-retrying cancelled runs - #567

Open
jonwiggins wants to merge 1 commit into
mainfrom
fix/reconciler-cas-and-cancel-retry
Open

fix(reconciler): ms-truncate standalone CAS guard; stop auto-retrying cancelled runs#567
jonwiggins wants to merge 1 commit into
mainfrom
fix/reconciler-cas-and-cancel-retry

Conversation

@jonwiggins

Copy link
Copy Markdown
Owner

Fixes two confirmed reconciler bugs in the standalone (workflow_runs) path.

Bug 1 — standalone CAS comparison was microsecond-fragile

applyStandaloneTransition in apps/api/src/services/reconcile-executor.ts guarded its write with a plain eq(workflowRuns.updatedAt, version), while the file's casUpdate helper compares with millisecond truncation (date_trunc('milliseconds', ...)).

Failure scenario: Postgres timestamptz stores microseconds; JS Date versions carry only milliseconds. Any run whose updated_at was last stamped by PG's now() / defaultNow() (rather than a JS-originated write) could never match the snapshot version — every executor transition from that row returned stale, permanently. Auto-retries, cancel transitions, and finish transitions from such rows silently never applied until some other writer happened to stamp a ms-precision updated_at.

Fix: extracted the ms-truncating comparison into one updatedAtMatches() helper and made it the single comparison used by every CAS guard — casUpdate (tasks / pr_reviews / persistent_agents / workflow_runs) and applyStandaloneTransition. The sibling repo / pr-review / persistent-agent transition applicators already went through casUpdate and were audited to confirm no other raw-eq guard exists.

Bug 2 — the reconciler auto-retried user-cancelled runs

workflowService.cancelWorkflowRun (and the reconciler's own control_intent=cancel path in reconcile-standalone.ts) transitions a run to FAILED with "Cancelled by user". decideFailed cannot distinguish a user cancellation from an agent failure, so while retryCount < maxRetries the next reconcile pass flipped the cancelled run back to QUEUEDa cancelled run silently reran.

Fix: both cancel paths now structurally exhaust the retry budget at cancel time — retryCount = max(retryCount, workflow.maxRetries) — so decideFailed sees no budget and leaves the run alone. No DB migration, no fragile error-message matching. An explicit user retry via retryWorkflowRun still works, since it does not consult maxRetries.

How the tests prove it

  • apps/api/src/services/reconcile-executor.int.test.ts (real Postgres + Redis):
    • The pinned µs-seam test that previously asserted the stale behavior (row stamped date_trunc('milliseconds', now()) + interval '456 microseconds') is flipped: the auto-retry transition now applies from the µs-stamped row, and replaying the same action from the stale snapshot is still refused — CAS integrity preserved.
    • The control-intent cancel test now executes a full follow-up reconcile pass and asserts the cancelled run stays FAILED (noop failed_no_retry_intent, no write, no agent job) — previously it pinned the surprising auto_retry decision.
  • apps/api/src/services/workflow-service.int.test.ts: service-level cancelWorkflowRun stamps retryCount = maxRetries, and feeding the cancelled run's real snapshot into reconcileStandalone decides noop, not auto_retry; plus a test that cancel never lowers a retryCount already above maxRetries.
  • packages/shared/src/reconcile/reconcile-standalone.test.ts: cancel action's statusPatch carries the exhausted retryCount; the post-cancel row shape is left alone by decideFailed.

Verification

  • cd apps/api && npx tsc --noEmit — clean
  • Unit tests: apps/api 123 files / 2167 passed; packages/shared 19 files / 442 passed
  • Both touched integration files run twice (flake check): 24/24 passed both runs
  • pnpm format:check — clean

… cancelled runs

Two reconciler bugs in the standalone (workflow_runs) path:

1. applyStandaloneTransition CAS-guarded with a plain
   eq(workflowRuns.updatedAt, version). Postgres timestamptz stores
   microseconds while JS Dates carry milliseconds, so a row whose
   updated_at was last written by PG now()/defaultNow() could NEVER
   match the snapshot version — every executor transition from such a
   row was permanently stale. All CAS guards now share one
   ms-truncating updatedAtMatches() comparison (extracted from
   casUpdate, which already did this correctly).

2. Cancelling a run left it FAILED with retry budget remaining, and
   decideFailed cannot tell a user cancellation from an agent failure —
   the next reconcile pass silently flipped the cancelled run back to
   QUEUED and reran it. Both cancel paths (workflowService.
   cancelWorkflowRun and the control_intent=cancel decision) now stamp
   retryCount = max(retryCount, maxRetries) at cancel time so the
   auto-retry budget is structurally exhausted. Explicit user retry via
   retryWorkflowRun still works — it does not consult maxRetries.
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.

1 participant