Skip to content

fix(ecc2): resolve duplicate kill_process definition that breaks the Windows build#2195

Merged
affaan-m merged 1 commit into
affaan-m:mainfrom
mohameddsh3ban:windows-build-fixes
Jun 15, 2026
Merged

fix(ecc2): resolve duplicate kill_process definition that breaks the Windows build#2195
affaan-m merged 1 commit into
affaan-m:mainfrom
mohameddsh3ban:windows-build-fixes

Conversation

@mohameddsh3ban

@mohameddsh3ban mohameddsh3ban commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Problem

cargo build fails on Windows with a duplicate-definition error:

error[E0428]: the name `kill_process` is defined multiple times
    --> src\session\manager.rs:3638:1
     |
3609 | fn kill_process(pid: u32) -> Result<()> {
     | --------------------------------------- previous definition of the value `kill_process` here
...
3638 | async fn kill_process(pid: u32) -> Result<()> {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `kill_process` redefined here
     |
     = note: `kill_process` must be defined only once in the value namespace of this module
For more information about this error, try `rustc --explain E0428`.
error: could not compile `ecc-tui` (bin "ecc-tui") due to 1 previous error

ecc2/src/session/manager.rs defines three cfg-gated kill_process variants:

  • #[cfg(unix)] — sync, libc signals
  • #[cfg(windows)] — sync, taskkill
  • #[cfg(not(unix))]async, taskkill

On Windows, both cfg(windows) and cfg(not(unix)) evaluate true, so the sync and async versions both compile in and collide. This affects every Windows target — the overlap is independent of toolchain, so x86_64-pc-windows-msvc hits the identical collision as x86_64-pc-windows-gnu. Unix builds are unaffected because only cfg(unix) matches there — which is why Linux/macOS CI never caught it.

Fix

Delete the stray #[cfg(not(unix))] async definition. It was unusable regardless of the collision — both call sites are synchronous and never .await it:

  • enforce_session_heartbeats_with<F> is bound where F: Fn(u32) -> Result<()> (a sync fn) and receives kill_process as a plain fn-pointer, invoked as terminate_pid(pid) with no await;
  • stop_session_recorded calls kill_process(pid)? synchronously.

The surviving #[cfg(windows)] sync version is also strictly better than the deleted async one: it passes /T to taskkill (terminates the whole child process tree, so spawned agent subprocesses aren't orphaned) and surfaces the actual exit status on failure. The async version had neither.

One file, 18-line deletion. Verified on Windows (x86_64-pc-windows-gnu): builds clean, cargo check passes, and no other cfg(windows) / cfg(not(unix)) overlaps exist in the crate.


Summary by cubic

Fix Windows build by removing a duplicate kill_process definition. The async #[cfg(not(unix))] version is deleted so only the Windows sync taskkill implementation remains.

  • Bug Fixes
    • Removed the #[cfg(not(unix))] async kill_process that collided with #[cfg(windows)] (E0428).
    • Windows builds now succeed; callers remain sync and use the taskkill /T /F implementation.

Written for commit 545fcf4. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Refactor
    • Streamlined internal process management logic by removing deprecated code paths and consolidating platform-specific implementations.

On Windows both cfg(windows) and cfg(not(unix)) evaluate true, so the sync taskkill kill_process and the async taskkill kill_process both compiled in and collided (E0428). Call sites are synchronous and never await it (passed as a fn pointer to enforce_session_heartbeats_with, and called as kill_process(pid)? in stop_session_recorded), so remove the stray async cfg(not(unix)) definition. The sync cfg(windows) version already handles termination via taskkill /T /F.
@mohameddsh3ban mohameddsh3ban requested a review from affaan-m as a code owner June 8, 2026 13:35
@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: de44f520-08ee-42de-bf2b-d24f6e2522b7

📥 Commits

Reviewing files that changed from the base of the PR and between 90dfd95 and 545fcf4.

📒 Files selected for processing (1)
  • ecc2/src/session/manager.rs
💤 Files with no reviewable changes (1)
  • ecc2/src/session/manager.rs

📝 Walkthrough

Walkthrough

This PR removes an unused async process-termination function. The #[cfg(not(unix))] variant of kill_process that invoked taskkill is deleted from the session manager. Platform-gated implementations for Unix and Windows process handling remain in place.

Changes

Process Termination Cleanup

Layer / File(s) Summary
Remove non-Unix kill_process implementation
ecc2/src/session/manager.rs
Deleted the async taskkill-based process termination function gated for non-Unix systems. Remaining platform-specific process termination logic remains elsewhere in the file.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

A taskkill that never took flight,
Now gone from the code, out of sight,
Process cleanup done right,
One function removed with delight, 🐰

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and specifically addresses the main change: removing a duplicate kill_process definition that was breaking Windows builds due to cfg attribute conflicts.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cubic-dev-ai cubic-dev-ai Bot 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.

No issues found across 1 file

Re-trigger cubic

@alexraphael07-prog

Copy link
Copy Markdown

🤖 Bezalel Engine — Automated Code Review

1. Summary

PR #2195 intends to unblock the Windows build by removing the redundant async fn kill_process that shadows the sync fn kill_process shared by all platforms. The linter fails because only one definition may exist across cfg-blocks.

2. Issues

No bugs or security issues are introduced. However, the PR is incomplete:

  • Lines 3613–3635: the sync kill_process remains only guarded with #[cfg(not(windows))].
    On Windows this leaves zero implementations →

@affaan-m affaan-m merged commit 0ce14a4 into affaan-m:main Jun 15, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants