Repository navigation
docs(update): name the lock holder before the upgrade, not after - #856
Merged
Merged
Conversation
#848's three bad states came from an MCP server this repo tells people to install. An editor with gflow registered starts one `gflow mcp run` per session; they are long-lived and usually plural, and on Windows they hold files inside the install open, so a package manager can abort part-way and leave the environment broken. The advice to stop those processes already existed — in the remediation `gflow update` prints once the damage is done. Saying it in the upgrade section is the difference between a recovery step and not needing one. Also states why `gflow update` cannot pre-flight this itself: it is one of those processes. `structlog` imports `greenlet` unconditionally, so every `gflow` maps the venv's native modules, and refusing while any gflow holds the install would refuse every run. Mechanism reported with a reproduction by Cstanish127 on #824. Closes #848
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe documentation now requires stopping other ChangesWindows upgrade guidance
Priority: ➖ Normal Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Other · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The upgrade documentation provides the required process-stop and recovery guidance. No actionable current-head risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…e holder is installed Council review of #856 converged on three findings. Three dimensions independently flagged that the new "if an upgrade does abort" paragraph restated the exit-11 / forced-reinstall paragraph fifteen lines above it, with less precision — the original also names the repair command and why a plain `upgrade` cannot do it. Replaced with a back-reference, so the mechanism is stated once and cannot drift. The block also contradicted the paragraph immediately before it: that one says a running `gflow.exe` holding its own launcher is benign and the upgrade lands anyway, this one said a running gflow leaves the environment broken. Same-sounding phenomenon, opposite guidance, adjacent. Named the distinguisher — the launcher you are running versus a different gflow holding the install's contents. `tasklist` matches image names only, so it finds the `gflow.exe` shim but not a server registered as `uv run gflow mcp run`; said so rather than implying parity with `pgrep -f`, which is now `-fa` so the command line that matched is visible before anything is stopped. Softened two frequency claims ("people actually hit", "usually more than one") that a single reproduction does not support, per the claims discipline in AGENTS.md, and pointed the citation at the comment carrying that reproduction instead of the issue body, which still carries the refuted chrome.exe attribution. docs/MCP.md is the page that installs the lock holder and never said registering it gates upgrades; one note at the end of § 4 now covers every client listed there. USAGE.md's own instruction landed 67 lines into the section, so the section head now carries a one-line pointer.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#850 shipped the detection half of #848 —
gflow updatenow imports gflow-cli in a fresh isolated interpreter before reading any version, so a half-replaced install is reported as unusable rather than as a version. It also said plainly that the cause was unexplained.It is partly explained now, by an independent reproduction from @Cstanish127 while testing something unrelated: an editor with gflow registered as an MCP server starts one
gflow mcp runper session, they are long-lived, and on Windows they hold files inside the install open.uv tool install --forcethen fails on…\uv\tools\gflow-cli\Scriptsand leaves the install answeringNo module named 'gflow_cli'.That closes the holder-class gap in every measurement taken for #850: it is a Python process, which is what a
Scripts/.pydlock requires —chrome.execan hold neither, so this issue's original attribution could not have been the mechanism — and it is long-lived, which is why it never reproduced from a singlegflow update.It does not close the mechanism, and the docs are hedged accordingly: the reproduction failed on the launcher dir, not on the mapped
_greenlet.pydof the original incident, where uv was measured to replace a held file successfully (exit 0). TheAccess is denied (os error 5)on a mapped.pydis still open.What this changes
The advice was already correct and already shipped — it is in the remediation
gflow updateprints. The problem is when you see it: after the damage. This puts it in the upgrade section, where it prevents the damage instead of recovering from it, which is what the reporter suggested.It also names the distinguisher against the paragraph directly above it, which describes the benign Windows lock: the
gflow.exeyou are running holds its own trampoline, uv exits 1 copying it, and the upgrade lands anyway. A different gflow, holding the install's contents open, is the one that can leave a broken environment. Those two read as the same phenomenon with opposite guidance until you say which is which.And it states why
gflow updatecannot pre-flight this itself, so the question does not get re-asked: it is one of those processes.structlogimportsgreenletunconditionally, so everygflowmaps the venv's native modules — a pre-flight that refused while any gflow held the install would refuse every single run. Checking for other processes namedgflowis a different and much narrower check; the docs point attasklist/pgrep -fafor it, and say thattasklistmatches image names only, so a server registered asuv run gflow …shows up asuv.exe.docs/MCP.mdis the page that installs the lock holder, and it never said that registering the server gates your upgrades. One note at the end of § 4 now covers every client listed there.Verification
Docs only — no
src/change, so no e2e applies and none was written; this touches no Flow surface and no behaviour. doc-links, website/docs mirror, website PII and repo hygiene all green, and every factual claim aboutgflow updatewas checked againstsrc/gflow_cli/update_check.py(the-Ifresh-interpreter probe, the exit-11ConfigurationError, and the pinned forced-reinstall hint).Closes #848