fix(windows): idempotency store, sliced download, and doctor permission check - #566
Conversation
…ows box All three assume POSIX file/OS semantics that Windows does not provide, and all three shipped. Found by running the full suite plus a broad CLI smoke test on a fresh Windows 11 install; each is verified there, not just in unit tests. 1. #528/#545 -- the deferred self-update never ran. The helper was spawned with DETACHED_PROCESS, which gives the child no console at all; powershell.exe is a console application whose host cannot start without one, so it exited 0, in under a second, with an empty stderr, having executed nothing. The spawn looked successful, the marker was written, and the single-flight guard then suppressed every retry for the full 24h staleness window while each launch still announced an update. CREATE_NO_WINDOW creates a console and never shows it. Verified end to end: 0.79.0 -> 0.80.0, exit 0, full install log, where the shipped build wrote no log at all. 2. #427 -- `job run --idempotency-key` raised PermissionError [WinError 5] on every Windows run. JobIdempotencyStore took its advisory lock on the state file itself and then renamed a temp file over it; Windows refuses to replace a file that still has an open handle. ConfigStore already used a separate .lock sidecar; the store now does the same. forget() had the same flaw. 3. `doctor` warned every Windows user about config-file permissions they could not change. The check compared stat mode bits against 0600 while its comment claimed "Unix only"; Windows reports 0o666 for any writable file regardless of the ACL, so the warning was permanent and unfixable. Test coverage was the real gap. The waiter script's text was asserted as a string and the spawn was tested with Popen mocked, so the one combination that ships was never executed -- and the Windows CI test ran the script through subprocess.run, which supplies the very console the real flags withheld. A new Windows-only test performs the real detached spawn; a cross-platform test pins the flag choice. test_job_idempotency_store.py joins the Windows CI job. The full suite has never run on Windows in CI: a real run showed 55 failures out of 5404, with a genuine product defect hiding among them. These fixes take it to 27, all of which are POSIX-only permission assertions and unguarded fcntl imports in the tests themselves. README gains a Windows install section: the curl|sh one-liner cannot work there (no sh, and Git for Windows only puts cmd\ on PATH), the standalone windows_amd64.zip was undocumented, WinGet has no package at all, and the Chocolatey package lags far behind.
Two more from the same family, both surfaced by running the E2E suite against a real project from Windows -- which is the only place either could show up. `storage download-table` (and every sliced download) failed with `PermissionError: [Errno 13] Permission denied`. The slice loop created a `tempfile.NamedTemporaryFile` and handed its *name* to the streaming downloader, which opens the path a second time; a NamedTemporaryFile cannot be reopened by name on Windows while its own handle is open. `mkstemp` plus an immediate close gives the same collision-free name without holding it. Unit tests could not catch this -- they mock the download, so they never reach the real temp file. The idempotency store now takes a real cross-process lock (`filelock`) rather than the `fcntl` helper, which is a silent no-op on Windows. Until the `os.replace` fix in the previous commit, `record()` crashed there so the point was moot; now that it runs, an unserialised read-modify-write can drop a concurrent writer's entry, and a dropped entry makes the next replay create a duplicate job -- the exact side effect the store exists to prevent. `auth/state_store.py` already chose filelock for this reason. Raised in review. The existing "concurrency" test was a sequential loop and exercised nothing; a real one now spawns parallel writer processes, and was confirmed to fail with the lock stubbed out (as it happens by crashing on the shared temp path rather than by losing an entry -- which failure mode you get is a matter of timing). Also fixes the `ty` error the new detached-spawn test introduced.
E2E run from Windows against a real projectRan the E2E suite from the Windows box against a live Keboola project — the part unit tests structurally cannot cover — and it found a fifth bug of the same family, fixed in c58b48a.
This one is worth calling out because unit tests could not have caught it: they mock the download, so they never touch the real temp file. Only a run against live data reaches that line.
The three that failed on the second run all pass on re-run, so they are flakes rather than regressions, and I checked each rather than assuming:
Still outstanding, deliberately not in this PR
|
|
Thanks for the update — and for the thorough write-up, the Especially pleased to see the Thanks also for flagging what isn't fixed ( — Radan's Claude 🤖 (posted on behalf of @papousek-radan) |
…rong I claimed the deferred Windows self-update from #543 "has never executed once" and that DETACHED_PROCESS was the cause. Retesting on a machine that was not falling asleep, the shipped v0.79.0 self-updates correctly and reproducibly: v0.79.0 -> v0.80.0 twice in a row, with the marker, exit file and full install log all written, using the released wheel and its DETACHED_PROCESS flag untouched. The original observations came from a laptop suspending every few minutes -- it was dropping SSH sessions throughout that window. A machine that sleeps mid-way never lets the helper finish, and I read "no exit file, no log" as "the helper never started". An isolated probe spawning powershell.exe with DETACHED_PROCESS does still exit 0 within a second having run nothing, and I have no clean explanation for why the real helper behaves differently. But the end-to-end behaviour is what governs users, so a micro-benchmark that disagrees with it is evidence my benchmark does not model the real spawn -- not evidence the product is broken. Shipping a change whose stated rationale is retracted would be worse than shipping nothing, so this reverts the flag, both regression tests, and the two changelog entries that rest on it. The rest of this PR is unaffected and stays: the idempotency-store lock fix, the sliced-download temp-file fix, and the doctor permission check are each reproduced on demand and independent of any timing.
Same rule that was raised on #567: CONTRIBUTING requires a (since vX.Y.Z) entry in the agent-facing behaviour log for anything an agent would not infer from --help. Three of these qualify and none had one. Two commands failed on EVERY Windows invocation before 0.80.1 and worked everywhere else, so an agent seeing the report needs to know it is the platform, not the project -- including what to do on an older kbagent, which for --idempotency-key means not trusting it at all, since it never recorded an entry there. The doctor change is the one an agent can actually mis-handle: config_file went from a permanent, unfixable warn to pass on Windows, so anything treating warn as actionable was reporting a problem with no remedy.
|
Thank you — genuinely, it made my day to read that you're pleased with where this is going. Windows support is something I want to be good, not merely present, and I finally have a real Windows machine on my desk as of yesterday. That changes everything: instead of reasoning about what Windows probably does, I can just run it. Most of this PR came out of that machine's first day. One correction, and it is on the exact part you enjoyed. I have since retracted the But your sentence is the interesting one now. "Good to know why we never saw the deferred update actually arrive" — if you have in fact never seen it arrive, that is the one piece of evidence pointing the other way, and I would rather chase it than assume my two clean runs settle it. If you can spare a moment:
Nothing urgent, and no need to dig if it is not worth your time — but a "we still never see it" from you outweighs my test box. On And the remainder you mentioned is no longer a remainder: |
`kbagent semantic-layer --help` and `kbagent context` exit 1 with
`UnicodeEncodeError` on Windows the moment their output is piped or redirected,
and any Rich table truncated by width emits a lone `0x85` for its ellipsis.
The split that matters is terminal vs not, not which codepage is active. Since
PEP 528 CPython writes to a real Windows console through the console API, so an
interactive kbagent already reports `encoding=utf-8` and renders anything --
measured on Windows 11:
stdout encoding "arrow"
console utf-8 encodes
pipe / file cp1252 UnicodeEncodeError
Redirect it and that path is gone; Python falls back to the locale encoding,
which cannot represent an arrow, an em dash, or a box-drawing glyph. Scripts,
CI and AI agents capturing output are the ones affected -- this CLI's primary
audience -- while the interactive users who would notice never see it.
So: reconfigure stdout/stderr to UTF-8 only when the stream is NOT a terminal.
Terminals are deliberately left alone, because forcing UTF-8 bytes at a cp852
console would replace a working display with mojibake. Redirected Windows
output now matches POSIX byte for byte, including the box-drawing characters
Rich previously downgraded to ASCII.
human/Rich path cannot use that escape hatch because Rich owns the writes.
Verified on a real Windows 11 box, piped:
before: semantic-layer --help -> CRASH context -> CRASH (invalid UTF-8)
after: semantic-layer --help -> clean context -> clean (valid UTF-8)
and under a real console, all three surfaces stay rc=0 and clean, confirming
the interactive path is untouched.
Ships in 0.80.1 alongside #566, rather than as its own version.
…ge (#567) `kbagent semantic-layer --help` and `kbagent context` exited 1 with `UnicodeEncodeError` on Windows the moment their output was piped or redirected, and any Rich table truncated by width emitted a lone `0x85` for its ellipsis. The split that matters is terminal vs not, not which codepage is active. Since PEP 528, CPython writes to a real Windows console through the console API, so an interactive kbagent already reported `utf-8` and rendered anything; only a pipe or a file fell back to the locale encoding (cp1252), which cannot represent an arrow, an em dash, or a box-drawing glyph. That makes this a bug for scripts, CI and AI agents capturing output -- this CLI's primary audience -- and invisible to the people at a terminal who would have reported it. stdout/stderr is therefore reconfigured to UTF-8 only when the stream is NOT a terminal. Terminals are deliberately left alone: forcing UTF-8 bytes at a cp852 console would replace a working display with mojibake. Redirected Windows output now matches POSIX byte for byte, including the box-drawing characters Rich had been downgrading to ASCII. #546 fixed this class for `--json` by writing to `sys.stdout.buffer`; the human/Rich path cannot use that escape hatch because Rich owns the writes. Verified on a real Windows 11 machine: piped, both commands go from CRASH to clean, valid UTF-8; under a real console all three surfaces stay rc=0 and clean, confirming the interactive path is untouched. Seven cross-platform tests pin the contract, including that a terminal is never reconfigured. Ships inside 0.80.1 with #566.
|
Following up, because I owe you a clean answer after sending you a wrong one. You were right, and my retraction was the mistake. The deferred update really never arrives — it has not worked on Windows since 0.78.0. Fixed in v0.80.2, out now. What settled it was finally running the experiment properly: same machine, back to back, fresh install of the same wheel each trial, changing only the creation flag.
So the mechanism I first described to you was correct after all: Your machine is almost certainly stuck. Anything from 0.78.0 to 0.80.1 never auto-updates, and That also lands everything from 0.80.1 you have been missing: You do not need to answer the three diagnostic questions I asked earlier — I reproduced it on my own machine in the end. But that only happened because your one sentence, "we never saw the deferred update actually arrive", outweighed my two clean test runs and made me look again. Between this and the |
|
Correcting my own arithmetic in the message above, because it took credit from someone who earned it. I told @papousek-radan he had "caused five Windows fixes". Checking the actual authorship: #545, #546 and #570 were filed by @MichalProchazka / @MichalProchazkaP3, not Radan. Radan reported #528 and the "we never saw the deferred update actually arrive" observation that reopened #571 — decisive, but two contributions, not five. The 0.80.3 changelog also thanked Radan for #570, which is Michal's. Fixed in the release notes and in #575. So, accurately:
Thank you both; sorry for blurring it. |
Three reproducible Windows bugs, found by running the full test suite and the E2E suite from a fresh Windows 11 machine. All three assume POSIX file semantics that Windows does not provide, and all three shipped.
1.
job run --idempotency-keyis entirely broken on Windows (#427)Every call raises
PermissionError: [WinError 5] Access is denied. Same code, same call:JobIdempotencyStoretook its advisory lock on the state file itself, thenos.replace()d a temp file over it. POSIX replaces an open file happily; Windows refuses to rename over a file with an open handle.ConfigStorealready had this right with a separate.locksidecar.forget()had the identical flaw.This accounted for 25 of the Windows suite failures.
Review also caught that the lock was still
fcntl-based, which is a silent no-op on Windows — so fixing the crash turned a dead path into an unserialised one. It now usesfilelock, the same choiceauth/state_store.pymade for the same reason. The existingtest_concurrent_distinct_keys_dont_clobberturned out to be a sequential loop exercising no concurrency at all; the new test spawns 6 writer processes × 8 keys and is confirmed to fail with the lock stubbed out.2.
storage download-tablefails on WindowsEvery sliced download dies with
PermissionError: [Errno 13] Permission denied. The slice loop built atempfile.NamedTemporaryFileand passed its name to the streaming downloader, which opens the path a second time — and aNamedTemporaryFilecannot be reopened by name on Windows while its own handle is open.mkstempplus an immediate close gives the same collision-free name without holding it.Worth calling out: unit tests could not have caught this. They mock the download, so they never touch the real temp file. Only an E2E run against live data reaches that line.
3.
doctorwarns every Windows user about permissions they cannot changeThe
config_filecheck comparedstatmode bits against 0600 while its comment claimed "Unix only" — but the code ran everywhere. Windows reports0o666for any writable file regardless of the ACL, so the warning was permanent and unfixable. Now short-circuits on Windows, matchingauth/state_store.py.The systemic problem
The full suite has never run on Windows in CI. The windows-latest job runs only the semantic-layer export tests, the self-update runner, and a wheel smoke test. A real full run:
A genuine product defect was hiding among them.
tests/test_job_idempotency_store.pyjoins the Windows job. The remaining 27 are test-portability only — POSIX mode-bit assertions and unguardedfcntlimports.E2E from Windows
download-table,PermissionErrorPermissionErroranywhere; 3 failures, all passing on re-runThe three were each diagnosed rather than waved off: a read-after-write consistency race on
config list, a job-timing dependency, and asemantic-layer diffat the end of a round trip.README
curl … | shcannot work on stock Windows — nosh, and Git for Windows only putscmd\onPATH. Added a Windows section with three verified paths, and recorded that WinGet has no package at all and the Chocolatey package lags far behind (0.66.1 against 0.80.0), even though the frozen binary points users at both.Not fixed here
kbagent semantic-layer --helpandkbagent contextstill crash withUnicodeEncodeErroron a non-UTF-8 console. #546 fixed only the machine-readable JSON writers. That fix is a startup-level stdout reconfigure with a real trade-off, so it gets its own PR.Testing