fix(flue-review): recover interrupted workflows#2053
Conversation
🦋 Changeset detectedLatest commit: 4f9a1d7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Scope checkThis PR changes 788 lines across 10 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | 4f9a1d7 | Jul 15 2026, 11:33 AM |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | 4f9a1d7 | Jul 15 2026, 11:32 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | 4f9a1d7 | Jul 15 2026, 11:32 AM |
There was a problem hiding this comment.
Pull request overview
This PR hardens the infra/flue-review automated review worker so interrupted Flue workflows can be recovered safely (without duplicate/concurrent reviews), while preserving the original GitHub Check Run and adding a two-step Durable Object class reset for an incompatible FlueRegistry schema.
Changes:
- Adds stage-aware staleness deadlines and extends
ReviewWatchdogto reconcile Flue run status, fence superseded runs, and re-admit stalled workflows with a bounded retry budget. - Prevents duplicate PR review posts by probing for an existing attempt marker across paginated GitHub review pages and failing closed when marker inspection is unavailable.
- Introduces a shared
admitReviewWorkflow()helper and wiresexpectedRunIdthrough the workflow payload to support fencing/ownership claims during recovery.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| infra/flue-review/wrangler.jsonc | Appends DO migration tags to reset the FlueRegistry class via drop+recreate. |
| infra/flue-review/test/review-watchdog.test.ts | Extends unit coverage for stage-specific staleness deadlines. |
| infra/flue-review/test/review-watchdog-do.test.ts | Adds DO-level tests for recovery, fencing, admission retries, and reconciliation behaviors. |
| infra/flue-review/test/github-checks.test.ts | Adds tests covering marker pagination and “fail closed” behavior when marker inspection is unavailable. |
| infra/flue-review/.flue/workflows/review.ts | Threads expectedRunId and runId through watchdog RPC calls to enforce run fencing. |
| infra/flue-review/.flue/lib/workflow-admission.ts | New shared workflow admission helper used by both webhook path and watchdog recovery. |
| infra/flue-review/.flue/lib/review-watchdog.ts | Introduces stage-based staleness thresholds and updates watchdog RPC signatures. |
| infra/flue-review/.flue/lib/github.ts | Implements paginated attempt-marker lookup and integrates it into postReview() to avoid duplicates. |
| infra/flue-review/.flue/cloudflare.ts | Implements watchdog recovery logic (run inspection, bounded re-admission, hard ceiling for “active” stalls) and enforces runId matching for heartbeat/finish. |
| infra/flue-review/.flue/app.ts | Persists workflow input for re-admission and routes initial admission through the new helper with expectedRunId. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a well-scoped bug fix for the Flue review deployment infrastructure, and the approach is sound. It layers two defenses against duplicate or concurrent reviews: (1) ReviewWatchdog now fences superseded workflow runs with expectedRunId/runId checks and re-admits interrupted runs only when Flue reports them as errored/missing, and (2) postReview now checks for an existing attempt marker before posting and paginates through review pages. The stage-aware stale timeouts and the active-run hard ceiling cover the common interruption modes, and the existing GitHub Check Run is preserved throughout recovery.
I traced the admission flow from app.ts through workflow-admission.ts, the DO state machine in cloudflare.ts, and the workflow in review.ts, and checked that every changed RPC signature (identify, heartbeat, finish) is called with the new runId parameter. The recovery budget is bounded (WORKFLOW_RETRY_LIMIT = 2), the alarm is re-armed on every branch that doesn't terminalize, and the re-read + consistency check in alarm() guards against races with concurrent terminalization. The added tests in review-watchdog-do.test.ts and github-checks.test.ts cover the retry, fencing, active/completed reconciliation, duplication suppression, and pagination paths.
No AGENTS.md violations: this is private deployment infrastructure so no changeset or i18n is required; there is no SQL or admin UI; migrations are append-only. No blocking issues or regressions found.
github-actions
Bot
added
review/approved
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This remains a well-scoped fix for the private Flue review deployment: it layers Flue-run status checks, expectedRunId/runId fencing, bounded retry admission, and attempt-marker reconciliation to prevent duplicate/concurrent reviews while recovering interrupted workflows. The approach still fits EmDash's architecture (trusted workflow DO owns GitHub posting; the public handler only admits; migrations are append-only; no admin UI/SQL changes).
As a re-review: my prior review had no findings, and the current diff is the same shape I approved. I re-traced the admission flow from app.ts/workflow-admission.ts into the watchdog DO and the review.ts workflow, confirmed the updated RPC signatures (identify, heartbeat, finish) are used consistently with the new runId parameter, and verified the tests cover retry/fencing/active/completed reconciliation and pagination.
No blockers or regressions. I have two low-priority suggestions: put a page ceiling on the marker pagination loop, and consider Flue’s isError flag when interpreting a "completed" run during recovery.
| for (let page = 1; ; page++) { | ||
| const suffix = page === 1 ? "" : `&page=${page}`; | ||
| const res = await githubFetch( | ||
| `${GITHUB_API}/repos/${owner}/${repo}/pulls/${prNumber}/reviews?per_page=100${suffix}`, | ||
| { headers: installationHeaders(token) }, | ||
| ); | ||
| if (!res.ok) { | ||
| return { | ||
| status: "unavailable", | ||
| error: new Error(`review marker inspection failed: ${res.status}`), | ||
| }; | ||
| } | ||
| const reviews = await res.json<Array<{ body?: string; commit_id?: string }>>(); | ||
| if ( | ||
| reviews.some( | ||
| (review) => review.commit_id === commitId && review.body?.includes(marker) === true, | ||
| ) | ||
| ) { | ||
| return { status: "present" }; | ||
| } | ||
| if (reviews.length < 100) return { status: "absent" }; |
There was a problem hiding this comment.
[suggestion] The new marker pagination loop is unbounded. It keeps fetching pages until it finds the marker or GitHub returns fewer than 100 reviews. For a pathological PR with many review pages this can issue a large, sequential chain of API calls, blowing the duplicate-check request budget or hitting the 30s fetch timeout before the function resolves.
Cap the loop at a reasonable page limit and return absent once that limit is reached.
| for (let page = 1; ; page++) { | |
| const suffix = page === 1 ? "" : `&page=${page}`; | |
| const res = await githubFetch( | |
| `${GITHUB_API}/repos/${owner}/${repo}/pulls/${prNumber}/reviews?per_page=100${suffix}`, | |
| { headers: installationHeaders(token) }, | |
| ); | |
| if (!res.ok) { | |
| return { | |
| status: "unavailable", | |
| error: new Error(`review marker inspection failed: ${res.status}`), | |
| }; | |
| } | |
| const reviews = await res.json<Array<{ body?: string; commit_id?: string }>>(); | |
| if ( | |
| reviews.some( | |
| (review) => review.commit_id === commitId && review.body?.includes(marker) === true, | |
| ) | |
| ) { | |
| return { status: "present" }; | |
| } | |
| if (reviews.length < 100) return { status: "absent" }; | |
| for (let page = 1; page <= 10; page++) { | |
| const suffix = page === 1 ? "" : `&page=${page}`; | |
| const res = await githubFetch( | |
| `${GITHUB_API}/repos/${owner}/${repo}/pulls/${prNumber}/reviews?per_page=100${suffix}`, | |
| { headers: installationHeaders(token) }, | |
| ); | |
| if (!res.ok) { | |
| return { | |
| status: "unavailable", | |
| error: new Error(`review marker inspection failed: ${res.status}`), | |
| }; | |
| } | |
| const reviews = await res.json<Array<{ body?: string; commit_id?: string }>>(); | |
| if ( | |
| reviews.some( | |
| (review) => review.commit_id === commitId && review.body?.includes(marker) === true, | |
| ) | |
| ) { | |
| return { status: "present" }; | |
| } | |
| if (reviews.length < 100) return { status: "absent" }; | |
| } | |
| return { status: "absent" }; |
| ): Promise<"active" | "completed" | "recoverable" | "unavailable"> { | ||
| try { | ||
| const run = await getRun(attempt.runId); | ||
| if (!run || run.status === "errored") return "recoverable"; |
There was a problem hiding this comment.
[suggestion] workflowStatus only looks at run.status when deciding that a run has completed successfully. The admin endpoint in app.ts exposes r.isError on the same getRun record, so Flue appears to track completion and error status independently. If a run can return status: "completed" with isError: true, this branch would mark the GitHub Check Run as successful even though the review did not complete cleanly.
Treat isError: true as recoverable to be safe.
| if (!run || run.status === "errored") return "recoverable"; | |
| if (!run || run.status === "errored" || run.isError === true) return "recoverable"; | |
| return run.status; |
github-actions
Bot
added
review/approved
ascorbic
added
bot:review
There was a problem hiding this comment.
This re-review confirms the Flue review recovery approach is sound: the watchdog DO inspects persisted run status, expectedRunId/runId fencing prevents superseded admissions from claiming ownership, bounded retry admission + attempt-marker reconciliation cover the interruption cases, and the migrations are the required forward-only two-step reset. The change stays inside the private infra/flue-review deployment, so the core EmDash conventions (Lingui, RTL, SQL safety, API envelopes, locale filtering) don't apply, and the empty changeset is appropriate.
However, this pass also validates that two concerns raised in the prior review are real:
workflowStatusignores Flue'sisErrorflag — the run record returned bygetRuncarriesisError(seeapp.tsline 93), so a workflow can end withstatus === "completed"andisError: true. The watchdog currently reports that as a successful review on GitHub. This is a correctness bug, not just a suggestion.- Unbounded pagination in
reviewWasPosted— the marker-inspection loop has no page ceiling, so a pathological many-page response could exhaust time/rate limits before posting.
The new tests otherwise cover retry/fencing/active/completed reconciliation and the pre-post marker check well. I recommend fixing the isError handling before merging and adding a page ceiling (with a closed fallback) as a follow-up hardening step.
| ): Promise<"active" | "completed" | "recoverable" | "unavailable"> { | ||
| try { | ||
| const run = await getRun(attempt.runId); | ||
| if (!run || run.status === "errored") return "recoverable"; |
There was a problem hiding this comment.
[needs fixing] workflowStatus treats any Flue run whose status is "completed" as a successful review and marks the GitHub check as success. But the run record returned by getRun also carries an isError flag (used in .flue/app.ts line 93), so a workflow can finish with status === "completed" and isError: true. In that case the watchdog reports success even though the review failed or was never posted.
Treat an errored-completed run as recoverable (so the bounded retry path can attempt recovery) rather than success, and add a test case for getRun returning { status: "completed", isError: true }.
| if (!run || run.status === "errored") return "recoverable"; | |
| if (!run || run.status === "errored" || run.isError) return "recoverable"; |
| ); | ||
| } catch { | ||
| return false; | ||
| for (let page = 1; ; page++) { |
There was a problem hiding this comment.
[suggestion] reviewWasPosted paginates through PR reviews with an unbounded for (let page = 1; ; page++) loop. If GitHub ever returns a pathological response where every page contains exactly 100 items, this will keep fetching until the surrounding timeout kills it, wasting tokens and time. Add a small page ceiling and fail closed when it is exceeded.
Change the loop header:
| for (let page = 1; ; page++) { | |
| const MAX_REVIEW_PAGES = 10; | |
| for (let page = 1; page <= MAX_REVIEW_PAGES; page++) { |
And return unavailable after the loop so duplicate posting is not risked:
| for (let page = 1; ; page++) { | |
| return { | |
| status: "unavailable", | |
| error: new Error("review marker inspection exceeded page limit"), | |
| }; |
github-actions
Bot
added
the
review/needs-rereview
ascorbic
added
bot:review
ascorbic
added
bot:review
There was a problem hiding this comment.
This re-review confirms the recovery approach is sound and the two issues flagged in the prior review are now fixed.
Approach judgment: The PR correctly targets Flue workflow interruption recovery inside the private infra/flue-review deployment. It inspects the persisted run status, fences superseded admissions with expectedRunId/runId, retries admission a bounded number of times, preserves the original GitHub Check Run, and updates the check with useful progress copy. The change stays within the private deployment infrastructure, so the core EmDash conventions (Lingui, RTL, SQL safety, API envelopes, locale filtering) do not apply, and the empty changeset is appropriate.
What was fixed since the prior review:
workflowStatusnow respectsisError.infra/flue-review/.flue/cloudflare.tstreatsrun.isError(andrun.status === "errored"/ missing run) as"recoverable", so a workflow that ends withstatus === "completed"butisError: trueno longer gets reported as a successful review.reviewWasPostedpagination is now bounded.infra/flue-review/.flue/lib/github.tscaps the marker-inspection loop atREVIEW_LOOKUP_MAX_PAGES = 10and returns"unavailable"when the ceiling is hit, failing closed instead of paging indefinitely.
What I checked:
- Read the full diff and the changed source files.
- Traced the watchdog alarm, recovery retry, workflow admission, heartbeat/finish fencing, and
postReviewidempotency paths. - Verified the new tests cover both prior concerns plus retry/fencing/active/completed reconciliation.
- Confirmed the change is scoped to
infra/flue-reviewplus an empty changeset; no core package or admin UI conventions are involved.
No new blocking issues or regressions were found.
github-actions
Bot
added
review/approved
What does this PR do?
Recovers automated reviews when a Flue workflow is interrupted, while preventing concurrent or duplicate reviews. Recovery checks the persisted Flue run status, fences superseded runs, retries admission twice, and preserves the original GitHub Check Run.
It also resets the incompatible pre-1.0 FlueRegistry schema using a two-deployment class migration, and makes the Check Run status useful with a human-readable progress checklist, next-step guidance, and relevant details links.
The production reset was applied. PR #2044's replacement review and PR #2053's post-reset review both completed successfully.
Follow-up to #2019.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
pnpm buildpnpm typecheckpnpm lintpnpm --dir infra/flue-review test(34 tests)pnpm --dir infra/flue-review buildpnpm lint:json | jq '.diagnostics | length'(0)git diff --checkTry this PR
Open a fresh playground →
A full working EmDash site, deployed from this branch. Each visit gets its own session-scoped sandbox: no login needed and no shared state. Try the admin, edit content, hit the public site.
Tracks
fix/flue-review-interruption-recovery. Updated automatically when the playground redeploys.