fix(dev-fleet): refuse the sync when pip cannot reinstall into the live venv - #2445
Conversation
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsNo findings. The change adds a Windows-only write-lock probe ( [OPUS-REVIEWED] 3ba147f Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSAdvisory design-level review of Design-Verdict: PASS A deterministic breakage is turned into a safe, self-explaining refusal; the bounded probe, whole-sync refusal, and tracked deeper fix (#2297) are the right proportionality. Watch
[DESIGN-REVIEWED] 3ba147f |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
c5c2cfa to
5d53ffc
Compare
Disposition — GPT 5.6 Review on
|
5d53ffc to
e837325
Compare
Disposition — GPT 5.6 Review round 2 on
|
e837325 to
26e7074
Compare
CI triage — the two Windows shard failures on
|
| run | result |
|---|---|
| that test alone | 1 passed |
whole test_mcp_gateway_transport.py |
32 passed, 14 skipped |
that test alone, with my files reverted to origin/main |
1 passed |
assert 4 == 1 is a counter that accumulated across earlier tests in the shard rather than a single-call assertion, which is the signature of leaked state under the parallel sharded run. My diff touches neither mcp_gateway.transport nor anything it imports.
Not filing these as flakes-to-retry: (1) is a real reproducible failure on main for Windows and deserves its own fix; (2) is a genuine determinism bug in the shard, and the repo's testing conventions say a confirmed flake is a bug with a root cause rather than something to paper over with a rerun. Both are out of scope here, and I have deliberately not touched either file.
Gates after the rebase: targeted tests 9 passed, isort clean, flake8 clean, mypy 0 errors in the changed file. Worktree clean, still a single commit.
…ve venv
On Windows, Pull + Build's `pip install -e .` step can never succeed when the
gateway is served by the venv it reinstalls into -- the ordinary
single-checkout setup. Windows holds a mandatory lock on a running
executable's image, so pip cannot rewrite `Scripts\kirocrew.exe`:
ERROR: Could not install packages due to an OSError: [WinError 32] The
process cannot access the file because it is being used by another
process: '...\.venv\scripts\kirocrew.exe'
The failed step is the smaller half of the problem. pip's uninstall is not
atomic: by the time it reaches the locked script it has already renamed the
dist-info aside and deleted the editable `.pth` that puts `src` on
`sys.path`, and it rolls back neither. The venv is left unable to import the
package at all, which also kills the console script the gateway is restarted
through. The running process survives on already-imported modules, so nothing
surfaces until the next restart fails -- one click turns a working install
into one that is fine right up until it is stopped.
Probe the console scripts pip would have to rewrite before doing anything, and
refuse the whole sync when one of them is write-locked. An `r+b` open is
non-destructive and discriminates correctly: a running executable refuses it
while every other script in the same directory opens fine. It cannot model
every way a delete can fail -- an opener that permits writes but denies
deletes would pass -- so a clean result means "no known blocker" rather than a
guarantee, and a miss simply leaves the previous behaviour.
Refusing the whole sync, rather than running fetch/merge and omitting only the
reinstall, is the part that matters. Merging without installing is not a safe
consolation prize: a revision that adds a dependency lands on disk with that
dependency absent, the run exits 0, and the UI then offers "restart gateway to
apply" -- so the next restart imports the new code, fails on the missing
import, and the gateway does not come back. Newly spawned subprocesses hit the
same gap without any restart. That is the same unstartable-gateway outcome the
guard exists to prevent, reached later and with a success report in front of
it. The checkout is left on a revision whose dependencies are satisfied, and
the error names both the blocking file and the exact commands to run with the
gateway stopped.
Those commands name an absolute, quoted repository path rather than `-e .`.
The refusal's whole purpose is to hand the user a recovery line, so that line
must not depend on where it is pasted: this project's normal working state is
several worktrees checked out at once, and `-e .` copied into a feature
worktree's terminal would install THAT tree into the primary venv and repoint
its editable install away from the primary checkout -- the same
wrong-tree-in-the-venv outcome the refusal exists to avoid. `git -C` already
pinned the pull, which made an unpinned `.` actively misleading, since the
line read as though it were cwd-independent. Quoting covers the spaces that
are routine in a Windows home directory.
Pin the whole pipe to UTF-8 while here. `_start_run` decodes that stream as
UTF-8, but a piped stdout on Windows encodes with the process locale
codepage -- a writer/reader mismatch that mangles non-ASCII output and can
raise UnicodeEncodeError before the first step. Reconfiguring the runner's own
stdout fixes only one of the two writers: each step is a separate process that
inherits the same pipe and re-derives its encoding from the locale, so the
Python steps -- pip, and the build-and-stage child -- would still encode a
non-ASCII checkout path with the codepage and die on it. The environment is
the only channel that reaches a child, so each step is spawned with
`PYTHONIOENCODING=utf-8:replace`; non-Python steps ignore it. errors="replace"
additionally guarantees no print can be fatal.
POSIX is untouched: an executing binary can be unlinked there, which is why
pip has always been able to replace it.
Tests cover the platform gate, a locked script, a writable script, an
unrelated locked executable that must not block the sync, a non-lock OSError
that must leave the judgement to pip, the refusal starting no run at all, the
full step list when nothing is locked, the recovery command carrying an
absolute quoted path in both its prose and its command, the encoding pinned
ahead of the first print, the per-step environment pin reaching the env
actually handed to `subprocess.run`, and both halves of the encoding mechanism
under a forced legacy codepage -- the runner's own print, and a child that
dies on a non-ASCII path without the variable and survives with it. The step
assertions parse the generated script's structured step list rather than
substring-matching its text, because the steps are embedded double-JSON-encoded
and a naive quoted match passes whether or not the step is present.
26e7074 to
3ba147f
Compare
Round 3 disposition — both blocking findings accepted and fixed in
|
| step | interpreter | affected |
|---|---|---|
git fetch / git merge |
not Python | no — emits bytes, parent decodes with errors="replace" |
pip install -e |
Python | yes — echoes the checkout path |
npm ci |
not Python | no |
sys.executable -c ... build_and_stage |
Python | yes — the staging child you named |
So on a CJK home directory the runner survives its own prints and then dies inside pip or the staging child, which is the same defect one process further down.
Fixed as suggested — set in the environment, the only channel that reaches a child:
env = dict(st['env'])
env['PYTHONIOENCODING'] = 'utf-8:replace'
r = subprocess.run(st['argv'], cwd=cwd, env=env)Set in the script's step loop rather than on each wrapped_steps entry, so it covers every step including ones added later, and _build_env()'s allowlist stays untouched. Assigned rather than setdefault: the reader's encoding is fixed at UTF-8, so a divergent inherited value would be the defect, not a preference worth preserving. errors="replace" matches the reconfigure call and the reader's line.decode(errors="replace").
New tests: the per-step pin reaches the env actually handed to subprocess.run (plus env=st['env'] asserted absent, so the old form cannot creep back), and the mechanism proven end to end — the same non-ASCII print dies with UnicodeEncodeError under PYTHONIOENCODING=cp1252 and survives with utf-8:replace.
Gates: test_dev_fleet_app.py 39 failed / 263 passed — the 39 are the pre-existing Windows-host baseline (identical on clean main), and none of my 11 tests is among them; passed rose 260 → 263 with the new ones. isort clean, flake8 clean, mypy 0 errors in the changed file. One commit, worktree clean, commit message updated to cover both changes.
The remaining red Backend Tests (Windows) (2) is the inherited test_dashboard_chat_pins.py::test_load_transient_io_error_preserves_existing_state failure, unchanged and untouched — attribution in #2445 (comment).
Disposition — Design Review (Fable 5) PASS,
|
…ve venv (kirodotdev#2445) On Windows, Pull + Build's `pip install -e .` step can never succeed when the gateway is served by the venv it reinstalls into -- the ordinary single-checkout setup. Windows holds a mandatory lock on a running executable's image, so pip cannot rewrite `Scripts\kirocrew.exe`: ERROR: Could not install packages due to an OSError: [WinError 32] The process cannot access the file because it is being used by another process: '...\.venv\scripts\kirocrew.exe' The failed step is the smaller half of the problem. pip's uninstall is not atomic: by the time it reaches the locked script it has already renamed the dist-info aside and deleted the editable `.pth` that puts `src` on `sys.path`, and it rolls back neither. The venv is left unable to import the package at all, which also kills the console script the gateway is restarted through. The running process survives on already-imported modules, so nothing surfaces until the next restart fails -- one click turns a working install into one that is fine right up until it is stopped. Probe the console scripts pip would have to rewrite before doing anything, and refuse the whole sync when one of them is write-locked. An `r+b` open is non-destructive and discriminates correctly: a running executable refuses it while every other script in the same directory opens fine. It cannot model every way a delete can fail -- an opener that permits writes but denies deletes would pass -- so a clean result means "no known blocker" rather than a guarantee, and a miss simply leaves the previous behaviour. Refusing the whole sync, rather than running fetch/merge and omitting only the reinstall, is the part that matters. Merging without installing is not a safe consolation prize: a revision that adds a dependency lands on disk with that dependency absent, the run exits 0, and the UI then offers "restart gateway to apply" -- so the next restart imports the new code, fails on the missing import, and the gateway does not come back. Newly spawned subprocesses hit the same gap without any restart. That is the same unstartable-gateway outcome the guard exists to prevent, reached later and with a success report in front of it. The checkout is left on a revision whose dependencies are satisfied, and the error names both the blocking file and the exact commands to run with the gateway stopped. Those commands name an absolute, quoted repository path rather than `-e .`. The refusal's whole purpose is to hand the user a recovery line, so that line must not depend on where it is pasted: this project's normal working state is several worktrees checked out at once, and `-e .` copied into a feature worktree's terminal would install THAT tree into the primary venv and repoint its editable install away from the primary checkout -- the same wrong-tree-in-the-venv outcome the refusal exists to avoid. `git -C` already pinned the pull, which made an unpinned `.` actively misleading, since the line read as though it were cwd-independent. Quoting covers the spaces that are routine in a Windows home directory. Pin the whole pipe to UTF-8 while here. `_start_run` decodes that stream as UTF-8, but a piped stdout on Windows encodes with the process locale codepage -- a writer/reader mismatch that mangles non-ASCII output and can raise UnicodeEncodeError before the first step. Reconfiguring the runner's own stdout fixes only one of the two writers: each step is a separate process that inherits the same pipe and re-derives its encoding from the locale, so the Python steps -- pip, and the build-and-stage child -- would still encode a non-ASCII checkout path with the codepage and die on it. The environment is the only channel that reaches a child, so each step is spawned with `PYTHONIOENCODING=utf-8:replace`; non-Python steps ignore it. errors="replace" additionally guarantees no print can be fatal. POSIX is untouched: an executing binary can be unlinked there, which is why pip has always been able to replace it. Tests cover the platform gate, a locked script, a writable script, an unrelated locked executable that must not block the sync, a non-lock OSError that must leave the judgement to pip, the refusal starting no run at all, the full step list when nothing is locked, the recovery command carrying an absolute quoted path in both its prose and its command, the encoding pinned ahead of the first print, the per-step environment pin reaching the env actually handed to `subprocess.run`, and both halves of the encoding mechanism under a forced legacy codepage -- the runner's own print, and a child that dies on a non-ASCII path without the variable and survives with it. The step assertions parse the generated script's structured step list rather than substring-matching its text, because the steps are embedded double-JSON-encoded and a naive quoted match passes whether or not the step is present.
Fixes #2297.
Problem
On Windows, Dev Fleet's Pull + Build dies at the
pip installstep, and the failure leaves the venv broken rather than merely un-upgraded._sync_start_locked()runs pip with the main checkout's own venv interpreter, deliberately — the existing comment explains it must not re-point a worktree venv's editable install atMAIN_REPO. That reasoning is right, but in the ordinary single-checkout setup that venv is the one running the gateway. Windows holds a mandatory lock on a running executable's image, so pip cannot rewriteScripts\kirocrew.exe, and the step can never succeed. It is deterministic, not a race.Why it matters
The failed step is the smaller half. pip's uninstall is not atomic — by the time it reaches the locked script it has already:
kirocrew-<ver>.dist-infoto its~irocrew-<ver>.dist-infobackup, and__editable__.kirocrew-<ver>.pth, the file that putssrconsys.pathand it rolls back neither. Observed aftermath on a real install:
pip show kirocrewPackage(s) not found: kirocrewpython -c "import kiro_crew"ModuleNotFoundErrorWARNING: Ignoring invalid distribution ~irocrewThe running gateway survives on already-imported modules, so nothing surfaces at the time. But the
kirocrewCLI is dead (its entry point can't import), which meanskirocrew stop/restartno longer work, new subprocesses that importkiro_crewfail, and the gateway does not come back from its next restart. One click takes a working install to one that is fine right up until it is stopped.Fix (symptom → root cause → change)
1. Probe before doing anything. Check the console scripts pip would have to rewrite, and refuse when one is locked. pip is never started, so it cannot begin an uninstall it can't finish.
The probe is an
r+bopen — non-destructive, and it discriminates correctly. Validated against a genuinely locked exe:kirocrew.exe(running)PermissionErrorpip.exe,idna.exe,f2py.exe, … (same dir)os.remove("kirocrew.exe")winerror=32— exactly pip's errorIt runs on
subprocess_executor()alongside the existinggit/npmresolution, so it costs no extra round-trip and never blocks the event loop.Deliberately bounded: the probe does not model every way a delete can fail — an opener that permits writes but denies deletes would pass it. A clean result therefore means "no known blocker", not a guarantee, and a miss just leaves the pre-fix behaviour. It is worth doing anyway because it removes the failure that actually occurs.
2. Refuse the whole sync, not just the reinstall. An earlier revision of this PR ran fetch/merge and omitted only the pip step. GPT 5.6 Review showed that is unsafe, and it was right — disposition. Merging without installing lands a revision on disk whose new dependencies are absent, the run exits 0, and the UI then offers "restart gateway to apply" (
build_pending) — so the next restart imports the new code, fails on the missing import, and the gateway does not come back. Newly spawned subprocesses hit the same gap with no restart at all. That is the same unstartable-gateway outcome this guard exists to prevent, reached later and with a green run in front of it.So the sync now refuses early, alongside the function's existing guards (wrong branch, no venv, no git, no npm), leaving the checkout on a revision whose dependencies are satisfied. The error names the blocking file and the exact remedy:
Keeping fetch/merge and merely reporting failure was considered and rejected: the hazard is the merged-but-uninstalled revision itself, so a non-zero exit would not remove it.
That remedy line names an absolute, quoted repository path rather than
-e ., which GPT 5.6 also flagged. Handing the user a recovery command is the refusal's entire purpose, so the line must not depend on where it is pasted: this project is normally checked out as several worktrees at once, and-e .copied into a feature worktree's terminal would install that tree into the primary venv and repoint its editable install away from the primary checkout -- the same wrong-tree-in-the-venv outcome the refusal exists to avoid.git -Calready pinned the pull, which made an unpinned.actively misleading, since the line read as though it were cwd-independent. The surrounding prose no longer shows the bare-e .form either, so the message cannot teach the wrong command anywhere.Consequence, stated plainly: on Windows, Pull + Build now refuses while the gateway runs from that venv, rather than half-working. That is the honest state of the configuration until a deeper fix exists (an out-of-process or restart-time installer, tracked in #2297) — a refusal that explains itself beats a success that breaks the next restart.
3. Pin the whole pipe to UTF-8.
_start_rundecodes that stream as UTF-8 (line.decode(errors="replace")), while a piped stdout on Windows encodes with the process locale codepage. That writer/reader mismatch is pre-existing: it mangles any non-ASCII output and can raiseUnicodeEncodeErrorbefore the first step.errors="replace"additionally guarantees no print can be fatal.Reconfiguring the runner's own stdout fixes only one of the two writers, which GPT 5.6 caught on the rebased revision and was right about again -- disposition. Each step is a separate process that inherits the same pipe and re-derives its encoding from the locale, and two steps are Python --
pip install -e, and thebuild_and_stagechild -- so a non-ASCII checkout path would still be encoded with the codepage and die there, one process further down. The environment is the only channel that reaches a child, so every step is now spawned withPYTHONIOENCODING=utf-8:replace; non-Python steps (git,npm) ignore it and are unaffected. (Raised by GPT 5.6 on the first revision — disposition.)POSIX is untouched — an executing binary can be unlinked there, which is why pip has always been able to replace it. The helper returns immediately on non-Windows.
Tests
test_write_locked_console_scripts_is_a_posix_noop— the platform gate.test_write_locked_console_scripts_flags_a_locked_script— the real failure is detected.test_write_locked_console_scripts_passes_a_writable_script— a venv the gateway is not running from still syncs normally.test_write_locked_console_scripts_ignores_unrelated_executables— an unrelated locked exe in the sameScripts/must not block the sync; that would turn any other process into a silent refusal.test_write_locked_console_scripts_lets_pip_judge_other_errors— a non-lockOSErroris not evidence of a lock; refusing on any error would block syncs that would have worked.test_sync_refuses_entirely_when_a_console_script_is_locked— no run is started at all, asserted via_start_runnever being called, so fetch/merge provably did not happen; and the message names both blocker and remedy.test_sync_runs_every_step_when_nothing_is_locked— control: the full step list includingpip install.test_sync_runner_pins_utf8_stdout_before_its_first_print— ordering asserted, not just presence.test_utf8_reconfigure_survives_a_legacy_codepage_pipe— runs the mechanism in a real subprocess withPYTHONIOENCODING=cp1252, asserting exit 0 and a clean UTF-8 round-trip, so the test proves the fix rather than only proving the line exists.The step assertions parse the generated script's structured step list rather than substring-matching its text. Worth calling out: an early draft asserted
'"pip install"' not in script, which is vacuous — the steps are embedded double-JSON-encoded, so a label reads as\"pip install\"and the quoted check passes whether or not the step is present. The parsed form fails if the gate is removed.Manual verification
Local gates:
isortclean,flake8clean,mypyreports 0 errors in the changed file.test_dev_fleet_app.pyon this branch: 39 failed, 260 passed, 6 skipped. Cleanorigin/mainin the same worktree: 39 failed, 251 passed, 6 skipped — the identical 39 pre-existing failures (Windows-hostmake_live/ service-pointer / module-entry tests), plus the 9 new tests passing. None of the new test names appear in the failure list. (mypyrun locally on Windows also reports 157 pre-existing errors across 34 untouched files for POSIX-only stubs —fcntl,resource,os.fork; CI'sbackend-lintruns onubuntu-latest, where those resolve.)Screenshots
N/A — no user-visible UI change; this is one backend module and its test.