[Bug]: synchronous Windows ACL hardening stalls the proxy on response-state writes

[luvs01]

Client or integration

Codex App

Area

Proxy and routing

Summary

On Windows, response-state persistence can block the OpenCodex proxy event loop while NTFS ACL hardening runs. The current path is schedulePersist() -> persistNow() -> atomicWriteFile() -> hardenSecretPath() -> synchronous Bun.spawnSync(["icacls.exe", ...]). While that child process is running or stalled, local /healthz requests and other proxy work cannot be serviced.

The shared ACL deadline from #160 limits an individual hardening call, but it does not make the wait non-blocking. In addition, the timeout cache is keyed by the exact temporary pathname. atomicWriteFile() generates a new .ocx.<pid>.<sequence>.tmp name on every write, so a timeout cached for one response-state temp file does not suppress the same stall on the next response-state write.

I expected the ACL attempt and its existing result policy to remain ordered before publishing the atomic file, while the proxy event loop stayed responsive during the child-process wait. If a later ACL child stalls, a new temporary pathname should not make the proxy event loop block again.

Reproduction

The timing-dependent production symptom can be reduced to a deterministic test using the existing ACL test seams. This does not run or modify real ACLs:

1. Use Windows, OpenCodex 2.7.42 or current dev commit 7710185c0548f50ea5a1c0034c2a6ea398fd865e, and the package's bundled Bun 1.3.14.
2. Set OPENCODEX_ACL_TIMEOUT_MS=1000, force the test platform to win32, and replace the icacls runner with a runner that synchronously waits for the supplied timeout and returns { success: false, exitCode: null, timedOut: true, stdout: "" }.
3. Before each hardenSecretPath() call, schedule a 10 ms timer, record when it actually fires, and await that timer before starting the next measurement.
4. Call hardenSecretPath(pathA, { required: false }), repeat the same call for pathA, then call it for a second existing path pathB.
5. Observe that the first unique path blocks the event loop for the full deadline, the identical path is skipped by timedOutPaths, and the second unique path blocks for the full deadline again.

Minimal runner shape:

process.env.OPENCODEX_ACL_TIMEOUT_MS = "1000";
setPlatformForTests("win32");
resetHardenedStateForTests();
setIcaclsRunnerForTests((_args, timeoutMs) => {
  Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, timeoutMs);
  return { success: false, exitCode: null, timedOut: true, stdout: "" };
});

Measured result from the bundled Bun 1.3.14 run:

Call	hardenSecretPath()	10 ms timer fired after
first unique path	1015 ms	1017 ms
same path again	0 ms	12 ms
second unique path	1010 ms	1025 ms

Every response-state write uses a new temp path and therefore misses both exact-path caches. If icacls.exe stalls or times out, a later write can incur the synchronous wait again.

As supporting live evidence, one correlation sample observed a live icacls.exe child while loopback /healthz took 742 ms; idle samples in the same run were about 61 ms. That service used a Bun 1.4 canary override, so the deterministic unit-level reproduction on the bundled Bun 1.3.14 runtime above is the primary evidence.

Version

• Released package: 2.7.42
• Also present on current dev: 7710185c0548f50ea5a1c0034c2a6ea398fd865e
• Deterministic reproduction runtime: bundled Bun 1.3.14 (1.3.14+0d9b296af)

Operating system

Microsoft Windows 11 Pro 25H2, x64 (build 26200.8894)

Provider and model

Not provider-specific.

Logs or error output

[opencodex] ACL hardening timed out (ETIMEDOUT) — transient icacls stall; the volume may still support per-user NTFS ACLs; ACL state unverified (budget exhausted) — continuing without NTFS ACL harden

Deterministic result:

first unique temp path: harden=1015ms, 10ms timer=1017ms
same path again:        harden=0ms,    10ms timer=12ms
second unique temp path:harden=1010ms, 10ms timer=1025ms

Screenshots and supporting files

Relevant current source paths:

• src/lib/windows-secret-acl.ts: the default runner uses synchronous Bun.spawnSync; timedOutPaths is keyed by exact path.
• src/config.ts: atomicWriteFile() generates a unique temp pathname and hardens it synchronously before rename.
• src/responses/state.ts: the debounced response-state snapshot calls atomicWriteFile() directly from the main event loop.

This is related to, but not a duplicate of, #160, #499 / PR #502, and #596 / PR #601. #160 bounds and diagnoses ACL timeouts. #499 / PR #502 handles stale response-state temp cleanup. #596 / PR #601 preserves the current-user ACE by making the sequence owner-first. None changes the synchronous event-loop wait or the unique-temp timeout-cache behavior.

Redacted configuration

{
  "provider_specific": false,
  "OPENCODEX_ACL_TIMEOUT_MS": "1000 for deterministic reproduction; default remains 5000"
}

Checks

• I searched existing issues and documentation.
• I removed secrets, tokens, account details, request credentials, and personal data.

Root cause

Bun.spawnSync() runs each icacls.exe step on the proxy's main thread. The total deadline prevents an unbounded wait, but the JavaScript event loop cannot service health checks or requests until the synchronous call returns.

The timeout memoization only helps when the exact same path is retried. Response-state atomic writes use a fresh sequence-numbered temp path for every snapshot, so each subsequent write is treated as unrelated even though it serves the same logical destination.

Suggested safe direction

A narrow asynchronous path for high-frequency response-state persistence appears safer than increasing GUI health-probe timeouts or weakening ACLs:

1. Add an asynchronous ACL runner for this path using non-blocking process creation with windowsHide, the same shared deadline, and the same honest timeout classification.
2. Add an asynchronous atomic writer that awaits the ACL attempt and applies the existing success, timeout-only soft-fail, or real-error policy before rename.
3. Serialize response-state persistence with a single-flight queue so overlapping snapshots cannot publish out of order, and await the final flush during graceful shutdown.
4. Preserve the owner-first invariant from PR fix(windows): grant owner ACE before ACL inheritance removal #601 and the current fail-closed behavior for real ACL errors. Keep the existing timeout-only policy explicit.

Skipping per-file hardening based only on the parent ACL is not a safe general fix. A parent directory may legitimately grant read access to additional principals, so a new response-state temp file can inherit a broader ACE until file-level hardening removes it.

The mutation steps that do not need output can use ignored stdout, while /findsid still needs captured output. An isolated Windows benchmark showed that changing stdout handling alone did not remove the stall; the synchronous process wait is the material issue.

[github-actions]

_{Automated translation bookkeeping — detected language: English.}

[lidge-jun]

Triage: confirmed on dev, routed to security review

Verified both halves of your root cause against 7710185c0.

• src/lib/windows-secret-acl.ts:79 runs Bun.spawnSync(["icacls.exe", ...]), and hardenSecretPath() (:311) sits on the write path, so the event loop is held for the duration of each child process. The #160 deadline bounds the wait; it does not yield.
• The timeout memo is keyed by exact path, and atomicWriteFile() mints a fresh .ocx.<pid>.<seq>.tmp name per write, so a cached timeout for one response-state snapshot cannot suppress the next one. That matches the "stalls again on every write" symptom rather than a one-time stall.

Your separation from #160, #499/#502, and #596/#601 is correct: none of those changes the synchronous wait or the per-temp-path keying.

On the way forward, one correction to an idea we considered internally and are dropping: re-keying the memo by target directory would be a smaller change, but your issue already rules it out for a good reason — a parent directory can legitimately carry broader ACEs, so treating a directory-level result as authoritative for a new temp file would let it stay readable until file-level hardening runs. We are not pursuing that shortcut.

What that leaves is essentially the four-step direction you proposed (async ACL runner sharing the same deadline and timeout classification, async atomic writer preserving the existing success/timeout/error policy, single-flight serialization for response-state persistence, awaited flush on shutdown, all keeping the owner-first invariant from #601). That is a change to how credential-bearing files are protected, which MAINTAINERS.md places under explicit security review, so it needs a maintainer decision before implementation rather than being picked up as a routine fix.

Keeping this open and labeled accordingly. The analysis is not in question; the sequencing is.

[luvs01]

Security-review follow-up: async timeout classification

Thank you for the clear triage and for routing the implementation through security review. I rechecked the cited dev commit (7710185c0548f50ea5a1c0034c2a6ea398fd865e) and ran one narrow Windows experiment to reduce uncertainty around the async runner.

On both the bundled Bun 1.3.14+0d9b296af and Bun 1.4.0-canary.1+b22e0e6d0, an asynchronous built-in timeout and an AbortSignal termination produced the same observable result after waiting for process exit:

• exitCode: null
• signalCode: SIGTERM
• proc.exited: 143

Control processes remained distinguishable (exit 5 produced 5; normal exit produced 0). The asynchronous Subprocess interface also does not expose the synchronous result's exitedDueToTimeout marker.

This supports one narrow implementation constraint: the async runner needs to record which deadline or cancellation path fired, and still await the child process to settle before applying the existing timeout/error policy. These observations do not validate the full persistence design and do not suggest changing that policy.

My recommendation for the security review is to preserve explicit timeout provenance and confirmed child-process settlement within the already-described async/serialized direction.