Skip to content

[codex] make windows native embedding start identity reliable#1070

Merged
TheGreenCedar merged 2 commits into
dev/codestory-nextfrom
codex/1067-windows-native-start-identity
Jul 13, 2026
Merged

[codex] make windows native embedding start identity reliable#1070
TheGreenCedar merged 2 commits into
dev/codestory-nextfrom
codex/1067-windows-native-start-identity

Conversation

@TheGreenCedar

@TheGreenCedar TheGreenCedar commented Jul 13, 2026

Copy link
Copy Markdown
Owner

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

  • Replaced the Windows PowerShell/CIM start-identity probe with OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION) and GetProcessTimes.
  • Owns the process handle with std::os::windows::io::OwnedHandle so cleanup remains RAII-managed and no dependency is added.
  • Converts Windows FILETIME creation ticks to the existing .NET DateTime.Ticks epoch, truncates the sub-microsecond remainder exposed only by the native API, and preserves values previously serialized through CIM as windows:<ticks>.
  • Returns None only when Windows reports that the PID no longer exists; process access and query failures remain fail-closed errors.
  • Kept the existing before/snapshot/after PID-reuse checks and the CIM snapshot used for executable, arguments, and approximate start time.
  • Added one Windows regression test for the known epoch conversion and repeated identity reads immediately after child spawn.
  • Added an Unreleased changelog entry.

Exact implementation head: 6d3e3e2e5a319021fbd57a3579e4b7f6cae342bc

How To Review

  1. In crates/codestory-retrieval/src/sidecar.rs, verify the Win32 declarations match the documented signatures and the process handle is adopted exactly once by OwnedHandle.
  2. Verify WINDOWS_DATETIME_TICKS_AT_FILETIME_EPOCH preserves the old PowerShell .CreationDate.ToUniversalTime().Ticks value space and that the conversion removes only FILETIME's sub-microsecond remainder.
  3. Confirm that only ERROR_INVALID_PARAMETER maps to a missing process and every access/query failure is returned.
  4. Confirm the existing process snapshot and double start-identity check are unchanged.
  5. Run the Windows regression and focused identity/readiness lanes below.

Verification

Passed locally on Windows x64 at 6d3e3e2e5a319021fbd57a3579e4b7f6cae342bc:

  • cargo fmt --all -- --check
  • cargo test --locked -p codestory-retrieval --lib windows_native_embedding_start_identity_is_stable_and_compatible
  • cargo test --locked -p codestory-retrieval --lib native_embedding (21 passed)
  • cargo test --locked -p codestory-cli readiness_broker with an isolated test temp root outside %USERPROFILE% (43 relevant tests passed across unit and integration targets)
  • cargo check --locked -p codestory-retrieval -p codestory-cli
  • cargo clippy --locked -p codestory-retrieval --lib -- -D warnings
  • git diff --check

The first readiness-broker run placed its injected root under the normal %USERPROFILE%\AppData\Local\Temp path and triggered the existing test's platform-cache prefix assertion. Rerunning with TEMP and TMP set to the isolated C:\codestory-test-temp-1067 root passed; that root was removed afterward.

Risk

  • The change is Windows-only and preserves the public identity format, but the packaged Windows x64 Vulkan acceptance proof is still required before merge.
  • Exact-head source CI, independent re-review, and the Windows x64/ARM64 package cells are pending.
  • After merge, raise cold sidecar query budget #1052 must rebase and rerun its unchanged-generation packet/drill parity proof; this PR does not claim that downstream acceptance yet.

@TheGreenCedar

Copy link
Copy Markdown
Owner Author

Exact-head review context

Independent read-only review accepted exact head 6d3e3e2e5a319021fbd57a3579e4b7f6cae342bc against base 1216d19ce4a07e5a4cfee2e6d18b817abc0e916d for source-proof dispatch.

The first review of a204c49402b30bec26d3942b65933eaeb1cd575b correctly blocked promotion because raw GetProcessTimes values retain 100 ns precision while the replaced CIM CreationDate value is microsecond-granular. Same-PID probes demonstrated a 0-9 tick compatibility gap that could reject an identity persisted before upgrade. The current head truncates only that sub-microsecond remainder before applying the .NET epoch, and the regression now uses a known value with an 8-tick remainder so removing the compatibility conversion fails the test. The current head also fails closed for PID 0 instead of treating ERROR_INVALID_PARAMETER as a vanished child.

Review confirmed:

  • OpenProcess/GetProcessTimes declarations and access rights match the Win32 contract.
  • OwnedHandle adopts each successful process handle exactly once.
  • Missing-process handling is distinct from access and query failures.
  • The persisted windows:<ticks> value remains compatible with the old CIM writer.
  • Existing before/snapshot/after PID-reuse checks and the CIM executable/argument snapshot are unchanged.
  • Non-Windows implementations are unchanged.
  • P1/P2 findings on the current head: none.

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 git diff --check.

Disposition: apply review-accepted and dispatch exact-head source proof. Keep the PR draft pending source proof and full packaged platform proof, including Windows x64 and Windows ARM64.

@TheGreenCedar TheGreenCedar added the review-accepted Exact PR head accepted for full workspace tests and clippy label Jul 13, 2026
@TheGreenCedar TheGreenCedar requested a deployment to macos-release-signing July 13, 2026 20:18 — with GitHub Actions Waiting
@TheGreenCedar TheGreenCedar requested a deployment to macos-release-signing July 13, 2026 20:18 — with GitHub Actions Waiting
@TheGreenCedar TheGreenCedar marked this pull request as ready for review July 13, 2026 20:23
@TheGreenCedar TheGreenCedar merged commit 6f3f8a2 into dev/codestory-next Jul 13, 2026
32 of 35 checks passed
@TheGreenCedar TheGreenCedar deleted the codex/1067-windows-native-start-identity branch July 13, 2026 20:23
@TheGreenCedar

Copy link
Copy Markdown
Owner Author

Windows packaged live proof

Exact-head source proof and the Windows package cells completed for 6d3e3e2e5a319021fbd57a3579e4b7f6cae342bc in platform run 29281875582. Both Windows x64 and Windows ARM64 built and passed their package smoke and managed-plugin handoff lanes. The overall dispatch is not green: the repo-scale stats job failed and the macOS package cells are still waiting.

I downloaded the exact Windows x64 artifact into a new isolated cache and verified the archive checksum:

  • codestory-cli-v0.14.3-windows-x64.zip
  • SHA-256 662ed6f80f3025e475320c3a25affdd1157158d8549c29f2a757f4c34f030dd2

The live ready --goal agent --repair retry still failed before indexing or packet execution:

native embedding identity did not converge after spawn: live native embedding process start identity is unavailable

This failure is narrower than the original blocker. The new native OpenProcess/GetProcessTimes identity capture succeeded immediately; otherwise the command would have failed with the distinct unavailable immediately after spawn error. The later convergence check still obtains NativeEmbeddingProcessSnapshot.started_at_epoch_ms through PowerShell/CIM. On this Windows host, Win32_Process.CreationDate is already a System.DateTime, so passing it to ManagementDateTimeConverter::ToDateTime fails and leaves the approximate start time unavailable.

The native log again proves AMD Radeon RX 7900 XT Vulkan execution with 13/13 layers offloaded and a live loopback listener. gpu_proof=verified, retrieval full, packet, and drill remain unproven because readiness stopped at identity convergence.

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 FILETIME. That requires a new exact head, independent re-review, source proof, Windows x64/ARM64 package cells, and a fresh isolated packaged live proof.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-accepted Exact PR head accepted for full workspace tests and clippy

Projects

Development

Successfully merging this pull request may close these issues.

1 participant