[codex] make windows native embedding start identity reliable#1070
Conversation
Exact-head review contextIndependent read-only review accepted exact head The first review of Review confirmed:
Local exact-head verification passed: formatting, the Windows regression, 21 focused native-embedding tests, the readiness-broker lane, retrieval/CLI checks, focused clippy with warnings denied, and Disposition: apply |
Windows packaged live proofExact-head source proof and the Windows package cells completed for I downloaded the exact Windows x64 artifact into a new isolated cache and verified the archive checksum:
The live
This failure is narrower than the original blocker. The new native The native log again proves AMD Radeon RX 7900 XT Vulkan execution with 13/13 layers offloaded and a live loopback listener. Cleanup is complete and proof-scoped: the managed PID is dead; proof-owned process, container, and listener counts are zero; the sidecar state file is absent; and the stale broker snapshot/coordination lock were preserved as output evidence and removed from the isolated cache. Unrelated existing llama-server and Qdrant resources were not touched. The PR stays draft. The revised minimal fix is to keep CIM for executable path and command line but source the Windows snapshot's approximate start time from the same native creation |
Context
Windows native embedding startup records an exact process-start identity before the readiness broker accepts or reuses a managed llama-server process. The existing implementation obtained that identity through a fresh PowerShell/CIM query immediately after spawn. On the packaged Windows x64 Vulkan proof for #1052, that query failed to converge twice even though the process was live and offloaded all 13 layers to Vulkan.
This PR fixes the shared Windows identity source without changing #1052's scheduler work or #1059's identity-V3 migration.
Closes #1067
Refs #1039
Refs #1052
What Changed
OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION)andGetProcessTimes.std::os::windows::io::OwnedHandleso cleanup remains RAII-managed and no dependency is added.FILETIMEcreation ticks to the existing .NETDateTime.Ticksepoch, truncates the sub-microsecond remainder exposed only by the native API, and preserves values previously serialized through CIM aswindows:<ticks>.Noneonly when Windows reports that the PID no longer exists; process access and query failures remain fail-closed errors.Exact implementation head:
6d3e3e2e5a319021fbd57a3579e4b7f6cae342bcHow To Review
crates/codestory-retrieval/src/sidecar.rs, verify the Win32 declarations match the documented signatures and the process handle is adopted exactly once byOwnedHandle.WINDOWS_DATETIME_TICKS_AT_FILETIME_EPOCHpreserves the old PowerShell.CreationDate.ToUniversalTime().Ticksvalue space and that the conversion removes onlyFILETIME's sub-microsecond remainder.ERROR_INVALID_PARAMETERmaps to a missing process and every access/query failure is returned.Verification
Passed locally on Windows x64 at
6d3e3e2e5a319021fbd57a3579e4b7f6cae342bc:cargo fmt --all -- --checkcargo test --locked -p codestory-retrieval --lib windows_native_embedding_start_identity_is_stable_and_compatiblecargo test --locked -p codestory-retrieval --lib native_embedding(21 passed)cargo test --locked -p codestory-cli readiness_brokerwith an isolated test temp root outside%USERPROFILE%(43 relevant tests passed across unit and integration targets)cargo check --locked -p codestory-retrieval -p codestory-clicargo clippy --locked -p codestory-retrieval --lib -- -D warningsgit diff --checkThe first readiness-broker run placed its injected root under the normal
%USERPROFILE%\AppData\Local\Temppath and triggered the existing test's platform-cache prefix assertion. Rerunning withTEMPandTMPset to the isolatedC:\codestory-test-temp-1067root passed; that root was removed afterward.Risk