Skip to content

fix(windows): use ProcessManager::run_command for 7z extraction instead of system() Fixes #2313#2322

Open
Phqen1x wants to merge 4 commits into
lemonade-sdk:mainfrom
Phqen1x:windows-7z-fix
Open

fix(windows): use ProcessManager::run_command for 7z extraction instead of system() Fixes #2313#2322
Phqen1x wants to merge 4 commits into
lemonade-sdk:mainfrom
Phqen1x:windows-7z-fix

Conversation

@Phqen1x

@Phqen1x Phqen1x commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • On Windows, system() called from a SUBSYSTEM:WINDOWS process causes a brief console/progress window to flash during 7z extraction. Replaces it with ProcessManager::run_command(), which uses CreateProcess with CREATE_NO_WINDOW to suppress that window while still capturing output and exit codes.

Test plan

  • Trigger a backend installation that extracts a .7z archive on Windows
  • Confirm no console/progress window appears during extraction
  • Confirm extraction completes successfully and the backend loads

Fixes #2313

…ad of system() Fixes lemonade-sdk#2313

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions github-actions Bot added the bug Something isn't working label Jun 19, 2026
@kenvandine

Copy link
Copy Markdown
Member

@claude review this

@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kenvandine's task in 1m 12s —— View job


Code Review

  • Read the changed file and diff
  • Inspect ProcessManager::run_command implementation (Windows path)
  • Assess correctness, consistency, and edge cases

Summary

The change is correct and well-targeted. It swaps the final system(command.c_str()) call in BackendUtils::extract_seven_zip for ProcessManager::run_command(command, output, 300).

On Windows, run_command launches the child via CreateProcessA(..., CREATE_NO_WINDOW, ...) (process_windows.cpp:687-690), which is exactly what suppresses the console/progress flash that system() produces in a SUBSYSTEM:WINDOWS process. This matches the root cause in #2313, and it captures the exit code reliably via GetExitCodeProcess. 👍

What's good

  • Consistency — the --list probe a few lines above (backend_utils.cpp:170-175) already uses run_command. This change makes the actual extraction use the same mechanism, so the function no longer mixes system() and run_command. That inconsistency was almost certainly the real source of the flash.
  • Shell semantics preservedrun_command wraps the command in cmd /c, so the quoted tar.exe -xf "..." -C "..." string behaves the same way it did under system(). No quoting regression.
  • Minimal & scoped — 2 lines, no behavioral change on the success path.

Suggestions (non-blocking)

  1. Log captured output on failure. You now capture output but discard it. Since tar/bsdtar writes errors to stderr (merged into the pipe), surfacing it would make extraction failures far easier to debug:

    if (result != 0) {
        LOG(ERROR, backend_name) << "Extraction failed with code: " << result
                                 << (output.empty() ? "" : " - " + output) << std::endl;
        return false;
    }

    Fix this →

  2. Timeout behavior (pre-existing, worth knowing). If the 300s timeout is hit, run_command does not kill the child — WaitForSingleObject returns, then GetExitCodeProcess yields STILL_ACTIVE (259), which is reported as a non-zero failure while the tar process keeps running orphaned (process_windows.cpp:707-713). For large CUDA .7z archives on slow disks, 300s is probably fine, but this is a latent edge case in run_command itself, not introduced by this PR. No action needed here.

Verdict

Correct, minimal fix that resolves the flash described in #2313 and improves internal consistency. Only the failure-output logging is worth considering before merge — everything else is optional. Nice work. ✅
• branch windows-7z-fix

Surface captured tar/bsdtar stderr in the error log when run_command
returns nonzero, so extraction failures are easier to diagnose.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@kenvandine kenvandine left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks great, and thanks for adding the log output. I am particularly happy to see this actually brings it more in line with how we handle this elsewhere.

@kenvandine kenvandine enabled auto-merge June 19, 2026 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug Report: Severe Llama.cpp/CUDA Downloader Loop & Hardcoded Folder Deletion Issue on Windows

2 participants