fix(flows): expose flows_run_detached over RPC and switch both UI Run controls to it - #5296
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
Comment |
0a4a65b to
c24793c
Compare
|
Review follow-up: added the completion backstop Because Every other run-outcome surface already pairs The hook can't be reused verbatim (it is typed for Two tests pin it: the backstop fires when no 1639 frontend tests pass (145 files), typecheck and prettier clean. Also confirmed by review and worth recording: the subscribe-before-events window is not strictly zero — |
880a234 to
a999b0c
Compare
… controls to it The UI Run buttons (Workflow Canvas + Workflows list) awaited `flows_run`, which blocks server-side until the run terminates (up to 600s). This meant `activeRunId` was only set AFTER every `flow:run_progress` event had already fired and been dropped (`useFlowRunProgress` only subscribes once it's set), so the canvas overlay could never show live progress (F-M1), and the list page's page-global busy lock froze every OTHER row's Run/Toggle for the run's whole duration while toasting "Run started" only once it had actually finished (F-M2). `ops::flows_run_detached` already existed (registers the run and returns its id before returning) but was only reachable from the agent `run_flow` tool, never over RPC. This registers it as `flows.run_detached`: - src/openhuman/flows/schemas.rs: new controller at all five required sites (schema list, registered-controllers list, match arm + handler, and both in-file function-list pin tests). - app/src/services/api/flowsApi.ts: `runFlowDetached()`, using the client's default RPC timeout (not FLOW_RESUME_TIMEOUT_MS) since it returns immediately. - FlowCanvasPage.tsx: Run sets `activeRunId` from the immediate response, so the progress subscription is live before the engine executes a node. - FlowsPage.tsx: `busyKey` is now keyed per flow id (was page-global), Run uses the detached call, and `useFlowRunFinished` silently refetches the list on completion so `last_run_at`/`last_status` still update without the blocking-call timing trick this replaces. Also in scope (touch the same files): - F-m2: latch `chatFirst` at mount instead of re-deriving it from the live `initialBuildSeed` prop, which flipped false (revealing the blank canvas mid-conversation) once the build seed was consumed after a clarifying-question first turn. - F-m3: normalize both sides of the canvas's dirty check through the same workflowGraphToXyflow/xyflowToWorkflowGraph round-trip (`normalizeWorkflowGraphForDirtyCheck`) before comparing, so a graph saved without node positions no longer reads as dirty the instant a remounted canvas reports its auto-laid-out positions back. - F-m5: corrected FlowCanvas.tsx's doc comment, which claimed the `/flows/:id` viewer uses `ReadonlyFlowCanvas` — it doesn't (the sole consumer always passes `editable`). Kept the component rather than deleting it: it's small, self-contained, and has its own passing test coverage. Tests: Rust controller test + updated pin tests (554 passed in `openhuman::flows`); a new JSON-RPC E2E proving `flows_run_detached` returns before the run settles; Vitest coverage for the non-blocking client call, the FlowsPage per-row busy regression, and FlowCanvasPage setting `activeRunId` before any progress event is dropped (1637 passed across the touched frontend directories).
a999b0c to
4e95a21
Compare
Manual testing found the overlay never actually worked — two bugs, both fixed hereVerified live in the running app. The canvas overlay this PR is about did not render at all, and tracing it turned up two independent causes. 1.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
graycyrus has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e95a2115f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| event.run_id, | ||
| event.status | ||
| ); | ||
| outstandingRunsRef.current.delete(event.run_id); |
There was a problem hiding this comment.
When a detached run completes normally, the backend publishes FlowRunFinished from finish_flow_run_row before it calls store::record_run to update flow_definitions.last_run_at/last_status (src/openhuman/flows/ops.rs around 4955-4965 and 5306-5324). Because that publish is fire-and-forget, this socket handler can run in between; deleting the outstanding run before the listFlows() refetch means a raced refetch can read the old summary and disables the 30s backstop, leaving the row stale until some unrelated refresh. Keep the run outstanding until a delayed/confirmed summary refresh, or emit/refetch only after record_run has completed.
Useful? React with 👍 / 👎.
1 participant
Summary
flows_run_detachedover RPC and switches both UI Run controls to it.Problem
openhuman.flows_runblocks server-side until the run terminates (up toFLOW_RUN_TIMEOUT_SECS; the client mirrors it with a 610s timeout). Both Run entry points awaited it as though it were fire-and-forget:FlowCanvasPage.tsxsetactiveRunIdfrom the RPC's return value, butuseFlowRunProgressonly subscribes toflow:run_progressonceactiveRunIdis set — i.e. after every progress event has already fired and been dropped. So nodes never ringed running/success/error; the overlay only ever worked for the rarepending_approvalpause.FlowsPage.tsxawaited it under a page-globalbusyKey, so a 5-minute run disabled Run/Toggle on every row for 5 minutes and fired the "Run started" toast only when the run ended. Its inline comment claimed "Fire-and-forget: the caller doesn't wait for the run to finish" — which did not match the RPC's semantics.ops::flows_run_detachedalready existed and returns{run_id, detached:true}immediately, registering the run before returning its id — but it was never exposed over RPC. Its only caller was the agentrun_flowtool, and its doc comment claimed it "mirrors how the UI 'Run' control … fire runs fire-and-forget", which was simply false.Solution
Backend: registers
flows.run_detached(wireopenhuman.flows_run_detached) as a thin controller over the existingops::flows_run_detached.flows.runstays registered — the agent tool and existing callers still use the blocking form. The false doc comment is corrected rather than deleted, since it becomes true with this change.Frontend:
runFlowDetached()uses the default timeout (it returns immediately, so the 610s budget is wrong for it).FlowCanvasPagesetsactiveRunIdfrom the immediate response, so the progress subscription is established as early as it can be.To be precise rather than overclaim: this does not make the window strictly zero.
flows_run_detachedspawns the run and returns without awaiting it, so the firstFlowRunProgresscould in principle be published before the HTTP response is even sent. In practice the round-trip plus a React commit is far slower than atokio::spawnscheduling gap, and the consequence is cosmetic — at worst the first node's animation frame is missed, anduseFlowRunPoller's 2s durable-row fallback recovers it. Run history is never affected.FlowsPagetracks busy state per row (busyByFlow: Record<flowId, 'toggle'|'run'>) instead of one global value.Completion signal preserved: since the RPC no longer blocks until completion,
FlowsPagenow usesuseFlowRunFinished— the same hook the runs sidebar/drawer already use — to refetch when a run settles, solast_run_at/last_statusstill update. The runs rail itself is untouched; it already reconciled detached runs from the agent-initiated path.Also fixed (same files, so they belong here)
chatFirstwas re-derived from the liveinitialBuildSeedprop every render, soclearBuildSeedflipped it false and revealed the blank trigger-only canvas mid-conversation whenever the builder's first turn ended with a clarifying question. Now latched at mount.onGraphChangewrites concrete auto-layout positions for nodes the server stored withoutposition, so opening an agent-built flow and rejecting a proposal showed a dirty badge with zero user edits. Both sides of the dirty comparison are now normalized at comparison time. (A first attempt that pre-normalized only the persisted seed made the first mount read dirty and broke two tests — normalizing both operands inside the memo is correct for first-mount and remount alike.)ReadonlyFlowCanvas's doc claimed the/flows/:idviewer uses it, but no production caller reaches it. Doc corrected rather than deleted: it has 7 passing tests, and discarding that coverage for a cleanliness win was out of proportion to this PR's scope. Left as a documented, inert fallback with a pointer to remove it if no consumer ever appears.Submission Checklist
tests/json_rpc_e2e.rs; Vitest covers the detached call not using the long timeout, FlowsPage keeping other rows interactive (the F-M2 regression), and FlowCanvasPage settingactiveRunIdbefore any progress eventN/A: bug fix to existing Run controls, no feature rows added/removed/renamed## Related—N/A: no matrix feature rows affectedCloses #NNN—N/A: found by code review, no tracking issue filed yetImpact
openhuman.flows_run_detached. Purely additive;openhuman.flows_runis unchanged and still registered.FlowsPagehad only the event half of that pair, unlike the three other run-outcome surfaces which also carry a poll backstop; a bounded backstop has since been added and pinned by tests. Still the behaviour worth exercising manually, since it is where a regression would hide.Related
N/AReadonlyFlowCanvasif no consumer materializes.flows_resumethe run-lifecycle safetyflows_runalready had #5286 — merge that first. Touchesops.rs/schemas.rs, so it overlaps fix(flows): close two authorization boundaries in flow-run tools #5287 and the stacked fix(flows): pin a parked run to the graph it was approved against #5293/fix(flows): store resilience — skip corrupt rows, transactional step upserts, once-per-process schema init #5294; rebase as those land.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
feat/flows-run-detached-rpc0a4a65b03(plus fix(flows): giveflows_resumethe run-lifecycle safetyflows_runalready had #5286's0b7105fa7as its base)Validation Run
pnpm --filter openhuman-app format:check— prettier clean (after--writeon 4 files); eslint 0 errors on touched filespnpm typecheck— exit 0src/pages src/services/api src/components/flows src/lib/flows src/hooks→ 1637 passed, 145 files;cargo test --lib openhuman::flows→ 554 passed, 0 failedcargo checkclean on BOTH the default build and the disabled build (--no-default-features --features tokenjuice-treesitter)app/src-tauriuntouchedValidation Blocked
command:cargo fmt --checkerror:one pre-existing formatting drift atops.rs:5241impact:none — confirmed viagit show HEAD:...that it exists in the base commit and is outside this diff, so it was left alone rather than swept into this PRBehavior Changes
Parity Contract
openhuman.flows_runis untouched and still registered for the agent tool and any external caller; the runs rail/drawer/WorkflowRunsPageare untouched and already handled detached runs.flows_run_detachedregisters the run before returning its id, so aflows_cancel_runlanding immediately after the UI receives it still takes the signalled branch.Duplicate / Superseded PR Handling