Review Windows IPC follow-up fixes - #7
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds Windows process and daemon plumbing, including safe executable lookup, named-pipe metadata and trust checks, updated proxy wiring, and process cleanup. It also changes symbol search allocation behavior and hardens two git command invocations. ChangesWindows runtime plumbing
Symbol search allocation cap
Git command isolation
Sequence Diagram(s)sequenceDiagram
participant runCliDaemon
participant mainImpl
participant cliTryProxy
participant cliConnect
participant cliDaemonListen
participant "cli-daemon.pipe" as pipeFile
runCliDaemon->>cliDaemonListen: data_dir
cliDaemonListen->>pipeFile: write metadata
cliDaemonListen->>pipeFile: create named pipe instance
mainImpl->>cliTryProxy: probe_dir
cliTryProxy->>cliConnect: data_dir
cliConnect->>pipeFile: read metadata
cliConnect->>pipeFile: CreateFileW
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Benchmark Regression ReportThresholds: 10.00% and 50,000 ns absolute delta
|
![chatgpt-codex-connector[bot]](https://cdn.github.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: d2638af4aa
ℹ️ 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".
| defer allocator.free(image_path); | ||
| if (!windowsPathsEqual(allocator, image_path, expected_exe)) return false; | ||
|
|
||
| return TerminateProcess(process, 0) != .FALSE; |
There was a problem hiding this comment.
On Windows with a running codedb daemon, TerminateProcess only initiates termination; the process may still hold the lock, pipe metadata, index, or binary handles when nuke immediately proceeds to remove binaries and ~/.codedb. That can make the uninstall intermittently leave files behind or report failed to remove even though the daemon was counted as killed. Request a waitable handle and wait for the process to exit before returning success here.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main.zig (1)
361-380: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winKeep POSIX proxy hits from paying
getDataDircost.
data_diris only needed for Windows pipe metadata and for the spawn-lock fallback. On POSIX, this now doesgetDataDirbefore every successful proxy hit even thoughcliConnectignores it.Proposed refactor
- const probe_dir = if (abs_root.len > 0) getDataDir(io, allocator, abs_root) catch null else null; + var probe_dir: ?[]u8 = null; defer if (probe_dir) |d| allocator.free(d); + + if (builtin.os.tag == .windows and abs_root.len > 0) { + probe_dir = getDataDir(io, allocator, abs_root) catch null; + } if (cliTryProxy(io, allocator, abs_root, probe_dir, args, use_color)) |code| { out.flush(); std.process.exit(code); } @@ if (abs_root.len > 0) { - const lock_free = if (probe_dir) |d| daemonLockAvailable(d) else true; + if (probe_dir == null) { + probe_dir = getDataDir(io, allocator, abs_root) catch null; + } + const lock_free = if (probe_dir) |d| daemonLockAvailable(d) else false; if (lock_free) {🤖 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 `@src/main.zig` around lines 361 - 380, Move the `getDataDir` lookup out of the unconditional path in `main` so POSIX proxy hits in `cliTryProxy` don’t pay that cost unnecessarily. Keep `probe_dir` only for the Windows pipe metadata flow and the `daemonLockAvailable` fallback, and gate the `getDataDir` call behind the specific cases that actually need it while preserving the existing `cliTryProxy` and spawn-lock behavior.
🤖 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 `@src/cli_proxy.zig`:
- Around line 546-557: The retry counter in cli-proxy is being reset too early,
so repeated writeCliPipeMetadata failures always look like the first retry and
never increase the backoff. In the retry path around writeCliPipeMetadata, keep
retry_failures unchanged on failure, only reset it after metadata is
successfully written, and preserve the existing logging and sleep behavior tied
to retry_failures so pipe retry backoff can grow across consecutive failures.
In `@src/git.zig`:
- Line 10: The git invocation in src/git.zig uses an empty core.hooksPath value,
which is not a reliable way to disable hooks across Git versions and platforms.
Update the argument list in the git command setup to use a specific non-existent
absolute hooks path instead, and keep the change localized to the same command
construction so the rev-parse flow still works with deterministic hook
isolation.
In `@src/nuke.zig`:
- Around line 163-171: In terminateWindowsProcessIfMatches, the process is
terminated but the function returns before the Windows daemon has fully exited
and released file handles. After the successful TerminateProcess call, wait on
the process handle with WaitForSingleObject before returning so callers like the
deleteTree flow only proceed once the process is fully gone. Keep the existing
OpenProcess, windowsProcessImagePath, and windowsPathsEqual checks unchanged,
and only add the post-termination wait in this function.
---
Nitpick comments:
In `@src/main.zig`:
- Around line 361-380: Move the `getDataDir` lookup out of the unconditional
path in `main` so POSIX proxy hits in `cliTryProxy` don’t pay that cost
unnecessarily. Keep `probe_dir` only for the Windows pipe metadata flow and the
`daemonLockAvailable` fallback, and gate the `getDataDir` call behind the
specific cases that actually need it while preserving the existing `cliTryProxy`
and spawn-lock behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 248af07e-c238-4b27-bfa3-ef0ffbc16f34
📒 Files selected for processing (9)
src/cio.zigsrc/cli_proxy.zigsrc/commands.zigsrc/explore.zigsrc/git.zigsrc/main.zigsrc/nuke.zigsrc/test_explore.zigsrc/test_mcp.zig
Benchmark Regression ReportThresholds: 10.00% and 50,000 ns absolute delta
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Benchmark Regression ReportThresholds: 10.00% and 50,000 ns absolute delta
|
1 participant
Purpose
Review-only PR for the follow-up fixes to upstream PR justrach#641. This PR compares against a fork-local base branch that already contains the rebased Windows IPC PR, so the diff should be limited to the maintainer/Codex review responses.
What changed
CreateNamedPipefailures and keep a replacement pipe instance available before closing the connected one..bat/.cmd.nuketo terminate matching Windows daemon processes registered in pipe metadata before removing data.searchSymbolscollection to top-N candidates without hash-order truncation.Scope note
The updater is intentionally untouched in this review PR.
Validation
zig build --global-cache-dir .zig-global-cache test --summary all-> 23/23 steps, 862/866 tests passed, 4 skippedzig build --global-cache-dir .zig-global-cache --summary all-> 3/3 steps succeededPYTHONUTF8=1 PYTHONIOENCODING=utf-8 python scripts/e2e_mcp_test.py --binary zig-out/bin/codedb.exe --project .-> 20/20 passedSummary by CodeRabbit
New Features
Bug Fixes
Tests