relayflowd run.get times out under CPU load and kills the run: a flow's own test suite can destroy 30 steps of journaled work - #613
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Relayflow: the adversarial review did not pass. This branch is not approved: the flow stopped here and did not mark it ready to merge. Review of PR #613Changes requested. Reviewed head FindingsP1 — A disconnected read session still terminalizes the authored rootLocation: The new dedicated reader can disconnect independently while the primary and root worker sessions remain healthy. The reproduction uses a real loopback socket for reads and the existing authored-root fixture for journal writes: the first Carry a typed read-transport interruption across the SDK and Node bridge; reconnect retryable reads within their remaining budget, and route exhausted transport interruptions through the resumable report. Keep protocol refusals and write failures distinct. Add coverage for a reader disconnect after a timeout while the root worker connection remains available. P2 — Cancellation cannot interrupt the new long-running snapshot waitLocation: Once the two-second poll starts The reproduction starts the snapshot read, aborts the signal, advances another 100 ms, and observes that the wait is still unfinished. Make read cancellation settle and drain the underlying request promptly, preserving authored promise-scope accounting, and propagate the abort before interpreting the snapshot. Add cancellation coverage after the first snapshot has begun. PR comments and scopeRead the complete PR diff, source call paths, added/changed tests, and the submitted verification artifacts. The PR context and inline comments are captured in gh pr view 613 --json url,headRefOid,baseRefOid,comments,reviews
gh api --paginate repos/AgentWorkforce/flows/pulls/613/commentsAt review time the only conversation comment was CodeRabbit's “Review skipped / Bot user detected”; Verification performed for this reviewThe following are independent executions at the reviewed head. The failing probes use the existing authored-root fixture, a real loopback read socket for the disconnect case, and simulated time for cancellation. They do not claim a real-daemon disconnect reproduction or mutation verification. The probe script creates and removes a temporary test file without modifying existing tests. Reproduce both findings: python3 evidence/pr-613-review/probe.pyCaptured output (also Focused existing regression suite: cd packages/sdk && RELAYFLOWD_BIN=/home/daytona/.relayflows-toolchain/target/2962130851/debug/relayflowd npx vitest run tests/journal-client-read-timeout.test.ts tests/running-step-watch.test.ts tests/heartbeat-timeout.test.ts tests/run-daemon-unresponsive.test.ts tests/authored-root.test.ts tests/run-read-load-live.test.ts tests/read-timeout-resume-live.test.ts --maxWorkers=1 --minWorkers=1Type checking: cd packages/sdk && npm run typecheckThe PR also includes a full-suite run with failures in |
A flow's CPU-heavy step could turn a delayed
run.getinto a terminal root failure. This change reduces read pressure, retries read timeouts, and leaves the root resumable when the daemon still cannot answer.status: running, and the resume command. A fresh probe distinguishesdaemon_unresponsivefromdaemon_unreachable, without claiming that a failed probe proves the daemon is dead.Retry opt-ins:
runDirectFlow, declarativeexecuteCheckedFlow,resumeFlow, andauthored-node-entry. Worker peers and utility clients keep single-shot bounded requests.The retry policy, completion wait, and diagnostic classifier live in separate small modules. The existing large transport file retains the typed verb wrappers;
cli/run.tsshrinks. No Rust execution logic, journal format, workflow files, or gate definitions change.Design notes: watches alone cannot unblock a snapshot or child-journal read queued behind a different child's deterministic command, so the unconditional reader remains necessary. The protocol has no
run.unwatch; closing each scoped watch connection releases its server watcher.readChildJournalnormally reads once per completed child; adopted unfinished children can reread after settling, and retry-backoff inspection can reread. This change does not add a journal cursor cache or daemon snapshot cache.Validation details and remaining failures are recorded below. “Parked” here means execution is left resumable without a fabricated terminal journal fact; it is not a journaled human park and does not assert journal integrity without reading it.
Validation:
daemon_unresponsivewithoutworker_errorterminalization, then resumes the root and checkssaved\nafter\nexactly once. The CPU-load case saturates the daemon's CPU allocation (one affinity-pinned core on Linux) while reads remain in flight.npx tsc --noEmitandnpx tsc -p tsconfig.tests.json, exit 0 with no output.The full suite is not green. The final production build was tested with the explicit daemon binary after building the local Surface package; the additional resume test above was added afterward and run separately. Complete full-suite output:
Failing files, copied from that output:
These include unavailable bubblewrap, the standalone Bun prerequisite, hosted-extension timeouts, Surface flow-identity refusals, and live wrapper/analyzer failures. They remain unresolved here; standalone Bun integration is consequently unverified. One live stub's handshake failure also reproduces using unmodified SDK commit
3c58ee16d10a9e2400db980f5bbafac84e437f20against the same fixture path: current probe/output, current output, baseline probe, baseline output. This baseline comparison covers that specific failure, not all six suites.Checks
The checks fail on the base commit too, so these failures were not introduced by this change: they come from the repository itself or from the environment the checks ran in. This pull request is a draft until someone looks.
What ran (.relayflow/check.sh)
Output on this branch (last 80 lines)
Output on the base commit (last 80 lines)
Fixes #522
Summary by cubic
Prevents a CPU-heavy step from destroying journaled work when a delayed
run.gettimes out under load. Flow-execution runs now retry read-only requests within a bounded budget and, if the daemon still cannot answer, stay resumable instead of being terminalized.Bug Fixes
run.watchpushes with lease-deadline snapshots every two seconds; scoped watch sessions close on completion, cancellation, or failure.runDirectFlow, declarativeexecuteCheckedFlow,resumeFlow, andauthored-node-entry) serialize read-only requests and retry with fresh IDs, escalating bounds, jitter, and a 300-second total budget; writes and interactive clients keep their existing single-shot behavior.status: running, and the resume command, and a fresh probe distinguishesdaemon_unresponsivefromdaemon_unreachable.Validation
daemon_unresponsivewithoutworker_errorterminalization, resumes the root, and checks the journaled effect runs exactly once.Written for commit a50a8de. Summary will update on new commits.