fix(subprocess): make the Windows worker log handle inheritable - #1207
fix(subprocess): make the Windows worker log handle inheritable#1207mlandolfi90 wants to merge 1 commit into
Conversation
On Windows the supervised index worker's log file is created with CreateFileA(..., lpSecurityAttributes = NULL, ...), so the handle is NOT inheritable. STARTF_USESTDHANDLES is set and CreateProcessW is called with bInheritHandles = TRUE, but a non-inheritable hStdOutput/hStdError leaves the child's stdio invalid: every line the worker writes is discarded and the post-mortem log is created but stays 0 bytes. That is the Windows half of the blind spot described at the reap site in index_supervisor.c -- a worker that exits non-zero leaves nothing to diagnose, and the caller only ever sees the generic "crashed on a file" hint, even for causes the worker reported explicitly. Measured on Windows 11, same command, same machine, cross-compiled with llvm-mingw via test-infrastructure/docker-compose.yml: before: .worker-*.log = 0 bytes after: .worker-*.log = 346 bytes, containing "repo_path is required" Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: mlandolfi90 <mlandolfi90@users.noreply.github.com>
|
Thanks, and sorry for the delayed first response. Queued for review. A nine-line change to make the Windows worker log handle inheritable is a good ratio of blast radius to value. CI is green; the branch is CONFLICTING against Windows handle-inheritance behaviour is genuinely hard to get right, and log capture on that path has been a recurring pain point, so this is welcome. |
|
Thank you for this, and I am sorry it sat long enough to be overtaken. Your diagnosis was correct for the code you were looking at, and you did the right thing by reproducing it against the actual v0.9.0 release binary rather than a local build. The blind spot was real: on Windows the worker log handle was not inheritable, so worker logs came back empty exactly when someone needed them. It has since been fixed on Closing as already fixed — with genuine thanks. Your reproduction is still useful to us: it is a ready-made check that current |
Problem
On Windows the supervised index worker's log file is always empty (0 bytes), so any worker failure is undiagnosable and the caller only ever sees the generic
"Indexing worker crashed on a file"hint.src/foundation/subprocess.csetsSTARTF_USESTDHANDLESand callsCreateProcessW(..., bInheritHandles = TRUE, ...), but the log handle itself is created withlpSecurityAttributes = NULL:A non-inheritable
hStdOutput/hStdErrorleaves the child's stdio invalid, so every line the worker writes is discarded. The file is created (CREATE_ALWAYS) but stays 0 bytes.This is the Windows half of the blind spot the reap-site comment in
index_supervisor.calready describes:The log is now kept on failure, but on Windows it is empty, so the blind spot is still there.
Repro
Windows 11, reproduced on the released v0.9.0 binary and on current
main(7d6cdb2).bad.json— valid JSON, unknown key:{"path":"C:\\some\\repo"}Result:
{"status":"error","outcome":"exit_nonzero","hint":"Indexing worker crashed on a file. The crash was contained (the server survived). Re-run to retry; a future release isolates the culprit file."}and
<CBM_CACHE_DIR>/logs/.worker-*.log→ 0 bytes.The worker did report the real cause. It was thrown away.
Fix
Open the log with an inheritable handle (
SECURITY_ATTRIBUTES.bInheritHandle = TRUE), which is whatSTARTF_USESTDHANDLES+bInheritHandles = TRUErequire.Verification
Cross-compiled with llvm-mingw via
test-infrastructure/docker-compose.yml(build-windows), run natively on Windows 11 — same command, same machine, same cache dir:repo_path is requiredFull log contents after the fix:
This restores the post-mortem for every supervised worker failure on Windows, not just this one — several open Windows reports of "worker crashed" with no further detail are likely the same missing diagnostic.
Not fixed here
The user-facing hint still says
"Indexing worker crashed on a file"for every non-zero worker exit, which is misleading when the worker exited for a reason it reported explicitly. Happy to follow up in a separate PR if you want that reworded or made conditional.🤖 Generated with Claude Code