fix(cli): wait for dashboard daemon READY signal before returning from cli show#40725
Merged
Skn0tt merged 4 commits intomicrosoft:mainfrom May 8, 2026
Merged
fix(cli): wait for dashboard daemon READY signal before returning from cli show#40725Skn0tt merged 4 commits intomicrosoft:mainfrom
cli show#40725Skn0tt merged 4 commits intomicrosoft:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
microsoft#40642)" This reverts commit dd468b4.
…microsoft#40626)" This reverts commit f536e37.
…m `cli show` The daemon now writes one byte to fd 3 once the dashboard is ready, and `cli show` waits for that signal (or daemon exit, or 60s timeout) before returning. This makes the Windows named-pipe race band-aids reverted in the prior commits unnecessary. Also bumps the MCP test timeout to 60s on Windows where persistent-context launch is genuinely slow, and wraps `cli()` invocations in `test.step` for nicer reports.
This comment has been minimized.
This comment has been minimized.
dgozman
reviewed
May 8, 2026
| return; | ||
| } | ||
| const readyStream = (child.stdio as unknown as Readable[])[3]; | ||
| await new Promise<void>((resolve, reject) => { |
Collaborator
There was a problem hiding this comment.
If this one throws, we do not unref (does it mean this process never exits?), and we also don't output anything. Let's figure this out.
Member
Author
There was a problem hiding this comment.
When I set the timeout to 10ms, I see the error bubble up and exit the process with the error message. Doesn't seem like unref is needed in the error case. I added some code to SIGKILL the misbehaving daemon though.
This comment has been minimized.
This comment has been minimized.
Address review feedback: on a failed READY handshake, SIGKILL the orphan daemon and wait for the OS to reap it before propagating the error. Also replace ternary with if/else in settle().
dgozman
approved these changes
May 8, 2026
Contributor
Test results for "MCP"2 failed 7029 passed, 1068 skipped Merge workflow run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cli shownow waits for the dashboard daemon to signal readiness before returning, instead of returning as soon as the daemon is spawned.cli()intest.stepfor nicer test output.Diagnostic work in #40703.