Skip to content

fix(daemon): flush stdout/stderr before exit in od media wait/generate - #6800

Open
lorenzozanee wants to merge 1 commit into
nexu-io:mainfrom
lorenzozanee:fix/media-wait-flush-stdout-before-exit
Open

fix(daemon): flush stdout/stderr before exit in od media wait/generate#6800
lorenzozanee wants to merge 1 commit into
nexu-io:mainfrom
lorenzozanee:fix/media-wait-flush-stdout-before-exit

Conversation

@lorenzozanee

Copy link
Copy Markdown
Contributor

Fixes #6540

Why

od media wait and od media generate write their terminal result as a single JSON line on stdout and then call process.exit() in the same synchronous block. process.exit() terminates the process without waiting for pending async writes, so when stdout is a pipe the final line can be dropped: the task completes and the daemon records a valid file, but the caller sees exit 0 with empty stdout and reports the generation as failed — often re-submitting a duplicate job. This is reported on Windows (named-pipe writes are always async there) and is also reachable on any platform once a single write exceeds the pipe buffer, which large progress logs or result payloads do.

What users will see

No UI or CLI surface changes. Scripts and agents that capture od media wait / od media generate output through a pipe now reliably receive the final {"file": ...} / handoff JSON line on stdout before the process exits, so successful tasks are no longer misreported as failures.

Surface area

  • None — internal bug fix in the daemon CLI; the documented stdout JSON contract and exit codes are unchanged, they are now guaranteed to be delivered.

Screenshots

N/A — CLI behavior change, no UI.

Bug fix verification

  • Test path that reproduces the bug: apps/daemon/tests/media-wait-stdout-flush.test.ts spawns the real CLI against a fake daemon and returns a terminal snapshot whose payload (6 MB stdout / 2 MB stderr) exceeds the pipe buffer, forcing the write/exit race deterministically. It asserts exit code plus complete JSON/stderr delivery via deep-equality.
  • Did the test go red on main and green on this branch? Yes — 4/4 failed on the base commit (stdout/stderr truncated at the pipe-buffer boundary), 4/4 pass here.
  • The spec's throttled reader prevents a fixed-sleep "fix" from passing: a sleep-only change drains at most ~1s worth of data before the process dies.

Validation

  • pnpm --filter @open-design/daemon typecheck — PASS
  • pnpm exec vitest run -c vitest.config.ts tests/media-wait-stdout-flush.test.ts — 4/4 PASS
  • pnpm exec vitest run -c vitest.config.ts tests/media-generate-prompt-file.test.ts tests/media-generate-multi-image.test.ts — 4/4 PASS
  • pnpm guard — PASS

`od media wait`/`od media generate` write their terminal JSON line and then
call process.exit() in the same synchronous block. process.exit() terminates
the process without waiting for pending async writes, so when stdout is a pipe
(Windows named pipes, or output larger than the pipe buffer on any platform)
the final line is dropped: the task succeeds and the daemon records a valid
file, but the caller sees exit 0 with empty stdout and reports a failure -
often re-submitting a duplicate generation job.

Drain both streams before exiting so the terminal JSON always reaches the
caller. The exit-code contract (0 done, 2 still running, 5 failed) is
unchanged.

Regression spec spawns the real CLI against a fake daemon with a
multi-megabyte terminal snapshot to force the write/exit race, and asserts
exit code plus complete JSON/stderr delivery. Red on main, green here.
@lefarcen

Copy link
Copy Markdown
Contributor

Hey @lorenzozanee — the reproduction notes here are very strong, especially the pipe-buffer race setup and the red/green verification against the real CLI. I'm queuing maintainer review next and handling the validation bookkeeping for this daemon path.

💡 To drive this PR to merge hands-free, paste this to your AI coding agent (Claude Code / Codex / opencode / Cursor …):
Take over nexu-io/open-design#6800 until it merges — read https://raw.githubusercontent.com/nexu-io/looper/main/skills/pr-takeover/SKILL.md and follow it.

@lefarcen
lefarcen requested a review from mrcfps August 12, 2026 22:48
@lefarcen lefarcen added size/M PR changes 100-300 lines risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps type/bugfix Bug fix labels Aug 12, 2026
@lefarcen lefarcen added the needs-validation Runtime change detected; needs human or /explore agent validation. label Aug 12, 2026
@lefarcen

Copy link
Copy Markdown
Contributor

🧪 Queued for QA validation — this PR has changes that need a manual QA pass before it is merged. Nothing needed from you; we’ll update here once it is validated. Thanks for the contribution! 🙏

@mrcfps mrcfps left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lorenzozanee thank you for pinning this down so carefully. This is a nasty Windows/pipe race, and the red spec makes the fix trustworthy.

I reviewed the pollUntilDoneOrBudget exit paths in apps/daemon/src/cli.ts and the new apps/daemon/tests/media-wait-stdout-flush.test.ts. Replacing the same-tick process.exit() calls with flushStreamsAndExit() is the right invariant: writing an empty chunk with a callback only resolves after previously queued stdout/stderr writes have been handed to the kernel, which is exactly the userspace-buffer loss described in #6540. The helper covers every terminal branch in the poll loop (done, failed, interrupted, non-2xx, 404, fetch error, and the still-running handoff), and it still exits after the flush so the top-level dispatcher cannot race a second unflushed process.exit().

The 6 MB / 2 MB throttled-reader spec is a real red test, not a tautology: it forces the pipe-buffer race, asserts full JSON/stderr delivery plus the matching exit codes, and would reject a short sleep-before-exit fake fix. The covered branches match the user-visible stdout contract.

No correctness, safety, or test issues in the changed ranges. Nice work.

🔁 Powered by Looper · runner=reviewer · agent=grok-build · An autonomous AI dev team for your GitHub repos.

@lefarcen
lefarcen requested a review from ivy-ting August 12, 2026 23:01
@open-design-crew

Copy link
Copy Markdown
Contributor

@lorenzozanee

Thanks for the contribution. I completed QA validation for this PR.

QA Acceptance Record

Scope:

  • od media wait / od media generate terminal stdout and stderr flushing at head 541389a7bd04e68d4c1350513a4c107e9256b0d1.

Verified:

  • The focused real-CLI regression suite passes 4/4, covering complete large stdout delivery for done, failed, and interrupted states, plus complete large stderr delivery for a non-2xx response.
  • Native Windows 10 / PowerShell 7.6.4 capture returned exit 0, exactly one stdout record, valid JSON, and the complete 6,291,456-character payload with empty stderr.
  • Daemon typecheck, daemon build, repository guard, and all current GitHub checks pass.

Not verified:

  • No Electron, UI, analytics, or static-asset surface is changed, so those runtime paths were not exercised.

Risks / notes:

Conclusion:

  • Accepted.

@open-design-crew open-design-crew Bot added validated Runtime change validated (via /explore Pass or manual QA). and removed needs-validation Runtime change detected; needs human or /explore agent validation. labels Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps size/M PR changes 100-300 lines type/bugfix Bug fix validated Runtime change validated (via /explore Pass or manual QA).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Windows][0.18.0] successful media tasks can return empty stdout, causing false failures and duplicate jobs

4 participants