Skip to content

fix(update): resolve Windows installer update race; drop dead pip/uv detach - #135

Merged
elkaix merged 3 commits into
mainfrom
fix/windows-update-installer-race
Jun 14, 2026
Merged

fix(update): resolve Windows installer update race; drop dead pip/uv detach#135
elkaix merged 3 commits into
mainfrom
fix/windows-update-installer-race

Conversation

@elkaix

@elkaix elkaix commented Jun 14, 2026

Copy link
Copy Markdown
Member

Summary

Fixes the Windows in-app update showing a spurious "could not close the program, close it and retry" dialog even though the update succeeded.

Root cause: the native installer's Restart Manager scan (CloseApplications) runs at the start of the actual install — after InitializeSetup (confirmed via Inno Setup's Installation Order). It raced the launching pythinker.exe teardown, found the onedir _internal/ DLLs still locked by the not-yet-exited console app (which RM can't gracefully close), and showed the Abort/Retry/Ignore dialog. By the time the user reacted, the launcher had exited and the install completed — hence "scary error, but it updated."

Fix: pass the launcher PID via /PID and wait for that process to fully exit in InitializeSetup (bounded 15s; OpenProcess(SYNCHRONIZE) + WaitForSingleObject) before the scan runs. On timeout or a manual run with no PID, it falls through to today's behavior — strictly no regression. CloseApplications=yes stays, so other open instances are still handled correctly.

Also in this PR

  • Remove the now-unreachable Windows pip/uv/pipx detached-upgrade helper. Since every shipped Windows install updates through the native installer (routing in _detect_upgrade_command), the detach only remained reachable for a Windows source checkout — where the running executable is the interpreter, not a file the upgrade replaces, so there's no lock to dodge. That case now runs the upgrade inline like POSIX, surfacing real command output/errors instead of a fire-and-forget process. Deletes _spawn_detached_windows_upgrade and its two tests; the Windows do_update test is rewritten to assert the inline contract.
  • Docs housekeeping: drop the stale tracked docs/superpowers/specs/*.md (moved to the now-gitignored docs/archive/).

Verification

  • make check-pythinker-code (ruff + format + pyright) — passes.
  • tests/ui_and_conv/test_shell_update.py + test_native_update_parity.py — 94 passed.
  • The .iss [Code] change cannot be verified on macOS — Inno Setup is Windows-only and the Restart Manager behavior only reproduces on Windows. Full validation needs a Windows build via windows-installer.yml or a manual Windows /update run. Python-side, a unit test asserts the spawn args include /PID=<pid>.

Scope note

This targets the in-app /update path (pythinker hands the installer its PID). A user who manually runs the downloaded installer while pythinker is open has no PID hand-off, and "please close it" is legitimate there.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed spurious "could not close the program" error dialog in Windows updates by ensuring proper launcher process termination before installation begins.
    • Improved Windows update flow by running updates inline with visible command output and error messages instead of background operation.

elkaix added 2 commits June 13, 2026 21:46
The old design specs were moved to docs/archive/ (now gitignored). Remove the
stale tracked copies under docs/superpowers/specs/ so the working tree is clean.
… detach

The native Windows installer's Restart Manager scan raced the launching
pythinker.exe teardown, finding _internal still locked and showing a spurious
"could not close the program" dialog even though the update succeeded. Pass the
launcher PID via /PID and wait for it to exit in InitializeSetup before the scan.

Also remove the now-unreachable Windows pip/uv/pipx detached-upgrade helper:
every shipped Windows install updates via the native installer, so that path
only remained for source checkouts, which now run the upgrade inline like POSIX.
@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@elkaix, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 48 minutes and 37 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f1a2d35c-4d71-42da-b1c2-b4e66644656a

📥 Commits

Reviewing files that changed from the base of the PR and between a6ccd24 and 5634fcf.

📒 Files selected for processing (1)
  • tests/ui_and_conv/test_shell_update.py
📝 Walkthrough

Walkthrough

Removes the _spawn_detached_windows_upgrade helper and its sys.exit call so Windows pip/uv/pipx upgrades run inline. Adds /PID=<current pid> to the Inno Setup invocation, and introduces a WaitForLauncherExit procedure in the installer that uses OpenProcess/WaitForSingleObject/CloseHandle to block up to 15 seconds for pythinker.exe to exit before setup proceeds.

Changes

Windows Update Race Fix and Inline Upgrade

Layer / File(s) Summary
Remove detached-spawn helper and add /PID installer arg
src/pythinker_code/ui/shell/update.py
Deletes _spawn_detached_windows_upgrade and its early-exit call site in do_update; extends _windows_native_installer_args to append /PID=<current pid> with comments describing the Restart Manager synchronization intent.
Inno Setup WinAPI wait for launcher exit
packages/windows-installer/installer.iss
Declares OpenProcess, WaitForSingleObject, CloseHandle from kernel32.dll; implements WaitForLauncherExit to parse /PID, open the process, and wait up to 15 seconds; calls it at the top of InitializeSetup before scope-conflict checks.
Test suite updates and changelog
tests/ui_and_conv/test_shell_update.py, tests/ui_and_conv/test_native_update_parity.py, CHANGELOG.md, .gitignore
Replaces the detached-upgrade + SystemExit test with an inline-upgrade assertion; removes the _spawn_detached_windows_upgrade command test; updates Inno Setup command expectations to require /PID as the final argument; adds docs/archive/ to .gitignore.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

bug

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.14% which is insufficient. The required threshold is 70.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title follows conventional commits format with 'fix' type and clear scope; accurately describes the two main changes: resolving Windows installer race and removing dead Windows detach code.
Description check ✅ Passed Description covers root cause, fix strategy, related changes, and verification steps; includes all required template sections (Related Issue, Description, Checklist) and is substantive rather than generic.
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
  • Commit unit tests in branch fix/windows-update-installer-race

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.

@coderabbitai coderabbitai 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.

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/ui_and_conv/test_shell_update.py`:
- Around line 696-727: The test is overfitting to private implementation details
instead of testing observable public behavior, making it brittle to benign
internal refactors. In tests/ui_and_conv/test_shell_update.py lines 696-727,
reduce mocking of private functions like _get_latest_version,
_update_candidate_unavailable_reason, _is_windows, _detect_upgrade_command, and
_run_upgrade_command. Instead, set up the test context via public inputs
(platform and source-checkout conditions) to naturally drive the code path, and
assert only on observable outputs from the public do_update function. Similarly,
in tests/ui_and_conv/test_native_update_parity.py lines 132-168, keep any
helper-level checks narrow and move the main update flow assertions to test
through the public do_update entrypoint rather than patching internal routing
helpers.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 21ec94c2-2969-44c5-8870-94b54eb75c88

📥 Commits

Reviewing files that changed from the base of the PR and between f21a84a and a6ccd24.

⛔ Files ignored due to path filters (23)
  • docs/superpowers/specs/2026-05-05-openai-codex-auth-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-05-opencode-go-auth-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-06-anthropic-direct-auth-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-06-deepseek-auth-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-06-minimax-auth-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-06-openrouter-auth-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-07-codex-terminal-ui-followup-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-07-compact-terminal-input-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-07-readable-terminal-reports-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-07-selector-family-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-07-terminal-work-log-refresh-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-20-pythinker-review-foundation-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-21-tui-spacing-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-22-blackbox-src-tui-port-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-22-windows-native-installer-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-24-install-downloads-counter-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-24-tui-brand-rebrand-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-26-agent-live-tool-stream-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-29-tui-renderer-contract-hardening-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-31-release-orchestration-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-05-31-welcome-banner-redesign-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-06-01-shimmer-traveling-waves-design.md is excluded by !docs/**
  • docs/superpowers/specs/2026-06-03-pythinker-scope-config-design.md is excluded by !docs/**
📒 Files selected for processing (6)
  • .gitignore
  • CHANGELOG.md
  • packages/windows-installer/installer.iss
  • src/pythinker_code/ui/shell/update.py
  • tests/ui_and_conv/test_native_update_parity.py
  • tests/ui_and_conv/test_shell_update.py

Comment thread tests/ui_and_conv/test_shell_update.py
@codecov

codecov Bot commented Jun 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Make the no-detach contract observable: fail the test if subprocess.Popen is
invoked, so a regression that reintroduces the detached Windows upgrade is caught
via behavior rather than internal mocks alone.
@elkaix
elkaix merged commit e3d4057 into main Jun 14, 2026
33 checks passed
@elkaix
elkaix deleted the fix/windows-update-installer-race branch June 14, 2026 02:11
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.

1 participant