Host the daemon on every platform - #128
Conversation
The client is about to have one way to do things rather than two, and that way is the daemon -- so a machine that cannot serve becomes a machine xd cannot run on. Windows and macOS could not serve. Almost nothing stood in the way. server.c and turn.c use no Unix-only call between them; main.c's serve path wants getcwd and chdir. The single exception is a shell: forkpty is BSD's util.h on macOS rather than glibc's pty.h, and Windows has no forkpty at all. So macOS gains the daemon whole, and Windows gains every op except a terminal, which fails with a reason the way the Windows client's own terminal already does. Keeping the type rather than compiling the calls out is what lets server.c stay platform-neutral. Not yet verified on Windows or macOS: only CI builds those. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 38 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR separates Docker source, test, and build stages, adds platform-specific daemon and terminal handling, makes remote operations cancellable during shutdown, and updates tests for Windows and portable process or input handling. ChangesCross-platform daemon and build pipeline
Estimated code review effort: 3 (Moderate) | ~30 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
What CI found, which grepping for forkpty and kill did not. macOS has no execvpe -- it is glibc's alone. Replacing the environment and then execing is the same two steps it folds into one call, and this is the child of a fork about to be replaced, so nothing outlives the assignment. Apple reaches environ through _NSGetEnviron. Windows has neither SIGALRM nor alarm, and the remote suite is compiled there for the first time now that it is not gated away. It runs without its watchdog and relies on meson's own timeout; naming the stuck test was a convenience for reading a hung log, not something the suite asserts on. The daemon itself compiled on Windows untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
connection_close closed the stream while a read_line and possibly a write were still in flight, leaving GIO with a source polling a file descriptor that had gone. Linux reports that as POLLNVAL on the one entry and carries on, which is why it never showed; BSD fails the whole poll with EBADF, and GLib treats that as fatal. So the daemon died of it only on macOS, and only once the remote suite ran there at all -- has_server had excluded it, and the fourth test brought a server down while a client was still reading from it. The connection now owns a cancellable, every async operation takes it, and closing cancels before it closes. The server's own shutdown does the same by hand, since it cannot call connection_close while walking the array that would remove from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two more that only the platforms this branch exists for could find. macOS was still failing the same way after the last fix, because the descriptor left in the poll set was not a connection's: stopping a socket service stops it accepting and leaves its listening sockets open, with their sources in the main context until the service is finalized. Closing the listener takes both down together. Linux reports such a descriptor as POLLNVAL on its own entry and carries on; BSD fails the whole poll with EBADF, which GLib treats as fatal. Windows could not mint a test certificate because the paths never reached openssl. g_spawn_command_line_sync puts the string through g_shell_parse_argv first, where a backslash escapes what follows it, and a Windows temporary directory is made of them. An argv has nothing to parse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two things the remote suite got away with on Linux and not on BSD, now that it is compiled somewhere other than Linux for the first time. Letting go of a server cancels its reads without finishing them. Cancellation completes on a later iteration, and until it does the sources for those descriptors are still in the main context -- so the next test polled a descriptor the last one had closed. Linux reports that as POLLNVAL on the entry and carries on; BSD fails the whole poll with EBADF, which GLib makes fatal, and it took four daemons to accumulate. And the certificate was minted through g_spawn_command_line_sync, which parses its string with shell rules first. A Windows temporary directory is full of backslashes, every one of which escaped the character after it, so openssl was handed paths that were not the paths meant and wrote a key where nobody looked for it. An argv has no such step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # meson.build
Cancelling a read does not finish it. The completion lands on a later iteration, and the source polling that descriptor lives until it does -- so closing the stream in the same breath took the descriptor out from under a source still watching it. Linux reports that as POLLNVAL on the one entry and carries on; BSD fails the whole poll with EBADF, which GLib makes fatal, and macOS died of it before the first remote test finished. Nothing needs closing by hand. The stream goes when its last reference does, which is after the cancelled read has completed and let go of the connection. getline goes too: it is POSIX, MinGW has none, and the app-server test helper that used it is compiled everywhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cancelling is the only way to stop a file monitor; dropping the reference leaves the backend to come apart on its own schedule. Linux watches the whole tree through one inotify descriptor and never minded. macOS gives every watch a kqueue descriptor of its own and closes it as the monitor goes, with the source still watching it -- an EBADF out of the next poll, which GLib makes fatal. It was the first thing every daemon built and the first thing every daemon dropped, which is why the remote suite never reached its second test there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The same mistake as the connections, one level up, and mine: stopping a socket service ends the accepting but leaves its listening sockets' sources in the main context until the service is finalized. Closing the descriptors first left those sources watching descriptors that had gone. Linux reports that as POLLNVAL on the one entry and carries on. BSD fails the whole poll with EBADF, which GLib makes fatal, so macOS died on the first remote test that tore a daemon down. Letting go of the service does both, in the order that works. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![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: fca41263e4
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # is the single exception, which Windows takes as a stub the way its client | ||
| # already does. | ||
| # | ||
| has_server = true |
There was a problem hiding this comment.
On an installed Windows MSI without a separately installed OpenSSL executable, xd serve now reaches ensure_certificate(), which spawns openssl to create the initial certificate and exits before listening when that command is unavailable. The Windows release path in scripts/bundle-windows.sh packages xd.exe, runtime data, modules, and linked DLLs but no openssl.exe, so the newly enabled daemon is unusable on a clean Windows installation; package the generator or replace this runtime dependency before enabling the server there.
Useful? React with 👍 / 👎.
| # already does. | ||
| # | ||
| has_server = true | ||
| has_daemon_terminal = not is_windows |
There was a problem hiding this comment.
On Windows, has_server = true makes tests/meson.build build and run the entire remote suite, including /remote/terminal-is-shared-and-replayable. That test requires terminal-open to succeed, while the Windows terminal-stub.c deliberately returns G_IO_ERROR_NOT_SUPPORTED, so every Windows meson test run fails; register this test only when has_daemon_terminal is true, or add a Windows-specific refusal assertion.
Useful? React with 👍 / 👎.
| # is the single exception, which Windows takes as a stub the way its client | ||
| # already does. | ||
| # | ||
| has_server = true |
There was a problem hiding this comment.
Even after the terminal test is gated, enabling has_server on Windows runs several remote tests that create extensionless #!/bin/sh files named claude or git and expect GSubprocess to execute them directly. Native Windows process creation does not interpret shebang scripts, so the first such test (/remote/images-are-uploaded-to-the-daemon) cannot start its fake backend and the Windows test job still fails; use a portable test child executable or exclude these POSIX-only cases on Windows.
Useful? React with 👍 / 👎.
| # is the single exception, which Windows takes as a stub the way its client | ||
| # already does. | ||
| # | ||
| has_server = true |
There was a problem hiding this comment.
When the newly enabled Windows server is launched as xd.exe serve --pair, the executable is still linked with win_subsystem: 'windows' in src/meson.build, and there is no AttachConsole or equivalent setup. A Windows-subsystem process launched from a terminal therefore has no reliable console-backed stdout, so the pairing code printed by run_serve() is not available to the user; split out a console daemon executable or attach and initialize the parent console before enabling this CLI mode.
Useful? React with 👍 / 👎.
| # is the single exception, which Windows takes as a stub the way its client | ||
| # already does. | ||
| # | ||
| has_server = true |
There was a problem hiding this comment.
On macOS and Windows, xd serve --auto-update always fails the installed-bundle check because daemon_install_dir() only reads /proc/self/exe, requires a sibling Linux xd.sh, and compares the result with ~/.local/opt/<name>. Neither the macOS .app nor the Windows MSI uses that layout, so enabling the daemon on those platforms exposes a documented option that unconditionally exits with “requires an installed bundle”; add platform-specific executable and installer-layout detection or suppress the option there.
Useful? React with 👍 / 👎.
The tests were compiled from the finished app, and the finished app links whisper.cpp, so every run of the gate built a speech library first -- minutes of CPU before the first assertion, on a runner with a cold cache every time. Nothing under test needs it. util/voice-data.c is core and parses; chat/voice-input.c is the window's, and it is the one that listens and transcribes. So the sources become a stage of their own, the tests build from that, and the app keeps its own stage with whisper in it. The voice dependencies are asked for rather than demanded, which is what lets meson configure at all where they are absent. Where they are there, nothing changes. Ten seconds now, from a warm base. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f0c88c3f2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # is the single exception, which Windows takes as a stub the way its client | ||
| # already does. | ||
| # | ||
| has_server = true |
There was a problem hiding this comment.
With has_server enabled on Windows, folder requests reach valid_folder_name() in src/remote/server.c, which rejects only G_DIR_SEPARATOR (\ on Windows), although Windows also accepts / as a separator. A paired client can therefore submit a name such as existing/../../outside; if the intermediate directory exists, g_file_make_directory() resolves the mixed-separator path outside the selected workspace parent and xd_folder_settings_ensure() writes metadata there. Validate every character with G_IS_DIR_SEPARATOR() (or explicitly reject both separators) before enabling these operations on Windows.
Useful? React with 👍 / 👎.
| # is the single exception, which Windows takes as a stub the way its client | ||
| # already does. | ||
| # | ||
| has_server = true |
There was a problem hiding this comment.
On a fresh macOS installation where the window has never run, enabling has_server lets xd serve start with a nonexistent default Workspaces directory: xd_app_workspaces_root() only computes the path, while directory creation currently happens in xd_fs_tree_new(), which the daemon does not call. watch_for_local_changes() consequently fails to install its monitor, and the first new-folder request fails because g_file_make_directory() cannot create a child under the missing parent. Ensure the server creates its root (with parents) before starting the listener and monitor.
Useful? React with 👍 / 👎.
| # already does. | ||
| # | ||
| has_server = true | ||
| has_daemon_terminal = not is_windows |
There was a problem hiding this comment.
When a macOS client closes a daemon-hosted terminal whose shell or child ignores the PTY's implicit hangup, the explicit cleanup does nothing: xd_remote_terminal_close() and force_terminal_down() call visit_session_members(), but that function enumerates /proc, which macOS does not provide. The two-second callback then emits closed without signaling any process, leaving the shell or its jobs running after the terminal is removed from the server. Use a macOS process/session enumeration mechanism or at least signal the recorded session/process group before enabling daemon terminals there.
Useful? React with 👍 / 👎.
A chat is a row in the database, and the only thing that announced one was the watch on the database directory noticing the write. That works where a file monitor reports a write promptly, which is to say on inotify: the kqueue and Windows backends do not, so the device that asked for the chat sat waiting for a tree that never came. Every other change to the tree already says so itself -- rename, move, trash, and the folder operations all broadcast on their way out. Creating a chat now does too, which is both the consistent thing and one less reason to care what the filesystem noticed. It is also the last thing keeping the remote suite from passing off Linux, where it has never run before this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Building the bundle compiles whisper.cpp from source, with every CPU variant, on a runner whose cache starts empty. Nothing about it changes between pushes, and it was paid for on every one of them. buildx keeps its layers in the Actions cache now. Master fills it and pull requests read from it, so a branch does not build a speech library before its first check, and a second push to the same branch does not build one at all. The gate itself already stopped waiting on whisper; this is the other half, which is the part that was actually costing the twenty minutes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test-session.c`:
- Around line 561-564: Update the read loop around read_stdin_line so it detects
a NULL result before attempting to parse or process the line, then exits the
child normally on EOF while preserving existing handling for non-NULL input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 22a0cbc9-c168-4a70-8a51-8ac31a5b0b2d
📒 Files selected for processing (9)
Dockerfilemeson.buildsrc/main.csrc/meson.buildsrc/remote/server.csrc/remote/terminal-stub.csrc/remote/terminal.ctests/test-remote.ctests/test-session.c
| for (;;) | ||
| { | ||
| g_autofree char *line = read_stdin_line (); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle EOF before parsing the line.
Line 563 can return NULL; unlike the old getline loop, EOF now makes the child return 21 instead of exiting normally.
Proposed fix
for (;;)
{
g_autofree char *line = read_stdin_line ();
+ if (line == NULL)
+ break;
+
JsonNode *root_node;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (;;) | |
| { | |
| g_autofree char *line = read_stdin_line (); | |
| for (;;) | |
| { | |
| g_autofree char *line = read_stdin_line (); | |
| if (line == NULL) | |
| break; | |
| JsonNode *root_node; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test-session.c` around lines 561 - 564, Update the read loop around
read_stdin_line so it detects a NULL result before attempting to parse or
process the line, then exits the child normally on EOF while preserving existing
handling for non-NULL input.
Eight tests write a shell script and put it on PATH as the agent CLI. Windows cannot run one: there is no shebang for CreateProcess to honour, so the spawn fails with EINVAL and the request that needed an answer never gets one. They skip there now, with the reason said out loud. What goes untested is the stand-in, not the daemon -- every other test in the suite still runs against real spawning, and the suite has only ever run on Linux before this branch anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ccab1c400
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # is the single exception, which Windows takes as a stub the way its client | ||
| # already does. | ||
| # | ||
| has_server = true |
There was a problem hiding this comment.
On an installed Windows MSI, opening the diff pane for a chat hosted by this daemon immediately sends base and working-all diff-read requests, but handle_diff_read() implements both by spawning sh -c (src/remote/server.c:2631-2639). The Windows payload assembled by scripts/bundle-windows.sh contains only xd.exe, linked DLLs, and runtime data—not sh.exe—so these requests fail even when Git itself is installed (the normal Git-for-Windows PATH exposes git.exe, not its internal shell). Replace these scripts with portable subprocess calls or explicitly disable the diff endpoint on Windows.
Useful? React with 👍 / 👎.
The Windows daemon serves every op except opening a shell, deliberately: there is no forkpty, and remote/terminal-stub.c refuses with a reason instead of pretending otherwise. A test that needs a terminal is asking for the one thing that build does not have. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
First step toward the client using a daemon locally as well as remotely, so there is one way to do things rather than two.
That plan makes the daemon mandatory: a machine that cannot serve becomes a machine xd cannot run on. Windows and macOS could not serve —
has_serverwasnot is_windows and not is_macos, andxd serveprinted "not available in this build yet".What it took
Less than expected.
server.candturn.cuse no Unix-only call between them, andmain.c's serve path only wantsgetcwd/chdir. The entire platform surface was one file.forkptyis the same call, kept in BSD's<util.h>rather than glibc's<pty.h>.forkpty, and ConPTY is a different enough shape that pretending otherwise in this file would only move the problem — soremote/terminal-stub.crefuses with a reason, exactly as the Windows client's ownchat/terminal-panel-stub.calready does. Keeping the type rather than compiling the calls out is what letsserver.cstay platform-neutral.meson.build—has_serveris unconditional; a newhas_daemon_terminalselects the real terminal or the stub.main.c—repair_daemon_cwdis a no-op on Windows, where a directory cannot be deleted while it is anyone's working directory.Verification
Linux: 22/22 suites, no warnings. That proves little on its own — Linux takes the path it always did. This PR exists to get Windows and macOS built, which is the part that is actually new.
Known follow-up, not addressed here
The daemon mints its TLS certificate by spawning
openssl, and only the Linux image installs one; the MSI and the macOS bundle do not ship it. macOS likely survives on/usr/bin/openssl, but a locally-spawned daemon on Windows would fail at startup with no certificate.A loopback daemon arguably should not be doing TLS or pairing at all — that machinery is for remote clients — which would remove the dependency rather than package around it. That is a design call for the next step.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Platform Support