Repository navigation
fix: refuse links at SQLite store paths on Windows and rebalance CI shards for the new affinity tests - #8102
Conversation
…ating them Windows creates a new file through a symbolic link at its name, so the affinity store's create-then-lstat order created the link's target before refusing the path (windows 11/11, run 41437409412). The spend-ledger owner had the same order. SQLite's Windows VFS also opens -journal/-wal/-shm by name and follows a link there. src/lib/private-sqlite-path.ts checks the database and its sidecars before anything is created (and, on Windows, the reparse-point attribute), and the affinity store re-checks sidecars before every write transaction.
…fore every write A hard link added to the open database is refused before the next write transaction, as a planted sidecar link already was. Docs now separate open-time from per-transaction checks and state that Windows relies on the hardened ACL rather than an ownership comparison.
Keeps scripts/test-layout/layout.json under the file-size ratchet, and narrows the clean-flag wording to the poisoned and failed-final-flush cases.
…t tests once Linux test 1/4 hit the 15-minute job timeout in run 41437409412: seven new affinity and bridge files had no recorded duration and weighed the median. Both duration tables are refreshed from that run with scripts/ci/test-durations.ts (the only run that measured them; the source lines record the cancelled shard and the retained Windows rows). The two affinity restart files spawned a full seeding worker before nearly every case. They now seed one template home per file and copy it, rebinding the ownership root; clean-restart, crash, unowned-home, two-home and one unknown-issuer handler case keep real seeding. Locally 19.2 s -> 13.2 s.
Tests replace process.platform with win32 to drive Windows branches on POSIX; the native GetFileAttributesW query must follow the operating system the process runs on, or those tests see every owner database refused (tests/server/server-stop-config-hardening.test.ts).
lidge-jun
added
the
ci:full
|
✅ Deterministic PR hygiene checks passed. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Findings
 |
| Layer / File(s) | Summary |
|---|---|
Shared path validation src/lib/private-sqlite-path.ts, tests/lib/private-sqlite-path.test.ts, scripts/test-layout/layout-file-identity.json, tests/fixtures/test-layout-expected.json |
The shared helpers reject unsafe database entries and journal, WAL, or SHM sidecars. They create a missing database with mode 0600 and validate it again. Tests cover unsafe entries, probe failures, and native Windows reparse-point detection. |
Spend-ledger owner checks src/lib/spend-ledger-owner.ts, tests/lib/spend-ledger-owner.test.ts, tests/helpers/spend-ledger-owner-link-worker.ts, tests/helpers/windows-link-create-emulation.ts, tests/helpers/windows-link-create-preload.ts, structure/transports/responses-spend.md |
The owner prepares its database with the shared helper and checks sidecars before opening SQLite. Tests verify that database and sidecar links cause acquisition to fail with SPEND_LEDGER_OWNER_UNAVAILABLE without creating the target. The transport documentation describes these checks. |
Affinity store checks src/codex/routing/thread-affinity-store.ts, structure/providers/codex-pool-affinity.md |
The affinity store records the opened database’s FileIdentity and checks the database entry and sidecars before write transactions. The provider documentation describes the checks and failure behavior. |
Affinity integration coverage tests/codex-integration/codex-affinity-restart-matrix.test.ts, tests/codex-integration/codex-affinity-restart.test.ts, tests/helpers/codex-affinity-matrix-worker.ts, tests/helpers/codex-affinity-seed.ts |
Integration tests cover unsafe database and sidecar paths, links or hard links added after the store opens, interrupted-transaction recovery, and seeded-home behavior. Test helpers prepare seeded homes and create late links. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant AffinityStore
participant PrivateSqlitePath
participant Filesystem
participant SQLite
AffinityStore->>PrivateSqlitePath: Prepare database and validate sidecars
PrivateSqlitePath->>Filesystem: Check entries and create missing database
AffinityStore->>SQLite: Open database
AffinityStore->>Filesystem: Record opened file identity
AffinityStore->>PrivateSqlitePath: Check paths before write transaction
PrivateSqlitePath->>Filesystem: Validate sidecars and database identity
AffinityStore->>SQLite: Start transaction after checks
Possibly related PRs
- lidge-jun/opencodex#8019: Adds the
FileIdentityandsameFileIdentitycontract used by the affinity store. - lidge-jun/opencodex#8023: Uses bigint stats and
sameFileIdentityfor other filesystem identity checks. - lidge-jun/opencodex#8098: Adds the SQLite-backed affinity store that this change updates with path and file-identity checks.
Merge Risk: 
be51c
-
The implementation is mergeable; only targeted regression coverage for large filesystem identities remains recommended.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 57.89% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title names the main change: refusing links at SQLite store paths on Windows. It also names the CI shard rebalancing, which the PR objectives confirm. The changeset covers both the link-refusal ch… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the main change: refusing links at SQLite store paths on Windows. It also names the CI shard rebalancing, which the PR objectives confirm. The changeset covers both the link-refusal checks (private-sqlite-path.ts, thread-affinity-store.ts, spend-ledger-owner.ts) and the CI duration tables. The title is concise and specific. One caveat: the link checks apply on POSIX and Windows, not only Windows, so 'on Windows' narrows the scope somewhat. The title is still related to the main change.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 57.89% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 19 functions across 12 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b49d88052
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Bun on Windows ARM64 cannot execute bun:ffi, so the reparse attribute query failed closed there and the spend-ledger owner could never be acquired. On ARM64 the check relies on lstat, which reports symbolic links, junctions and app execution aliases as links; the docs state the narrower coverage. The affinity store compares database identity with BigInt stats through sameFileIdentity: NTFS file IDs exceed Number.MAX_SAFE_INTEGER.
|
Maintainer integration into Exact-head verification for
After this merge I will rerun |
Summary
Fixes the two defects that turned the dev batch run red after #8092, #8096, #8094 and #8098 (run 41437409412).
windows 11/11: a link at the affinity database path created its target.
thread-affinity-store.tscreatedcodex-affinity.sqlitewithopenSync(path, "wx")and only thenlstat-refused a link. POSIXO_CREAT|O_EXCLrefuses a link at the final name, but WindowsCREATE_NEWfollows a dangling file symlink and creates its target, so the store was disabled only after the target existed.spend-ledger-owner.tsused the same order forspend-ledger-owner.sqlite(its symlink tests skip Windows and use an existing target). SQLite's Windows VFS also opens-journal/-wal/-shmby name and follows a link there.src/lib/private-sqlite-path.tsrequires the database and its three sidecars to be absent or private regular files before anything is created or opened: no symbolic link, no Windows reparse point of any kind (GetFileAttributesW/FILE_ATTRIBUTE_REPARSE_POINT, the same FFI call the repo's other Windows file checks use; fails closed if it cannot answer), no directory, no second hard link, and on POSIX the current uid.flush,pruneOnly): sidecars again, and that the database path is still the single-link file it opened. A refusal is a failed flush; a poisoned store or a refused final flush leaves the clean flag unset, so the next owner discards rows.SPEND_LEDGER_OWNER_UNAVAILABLE.bun:ffi, the native attribute query is skipped and the check relies onlstat, which reports symbolic links, junctions and app execution aliases as links; other reparse tags are not visible there (documented). Failing closed would have stopped the proxy starting on ARM64. Store identity comparisons use BigInt stats (sameFileIdentity): NTFS file IDs exceed 2^53.test 1/4 (Ubuntu) cancelled at the 15-minute job timeout. All four Linux shards were predicted at 632 s from
scripts/ci/test-durations.tsvbut took 10–15 min; seven new affinity/bridge files had no row and weighed the 151 ms median (codex-affinity-restart22 s,codex-affinity-restart-matrix16.5 s on Linux/macOS, 69 s and 47 s on Windows), and shard 1/4 was still passing tests when it was cancelled.scripts/ci/test-durations.tsfrom that run's job logs. It is the only run that measured the new files; the source lines record that Linux 1/4 contributed only its completed files (macOS 1/4–4/4, all green, measured every file) and that 174 Windows rows were retained. Simulated assignment: four Linux shards at ~728 s each.timeout-minuteschange.Verification
src/: 9 failed, including the original Windows case reproduced on macOS throughtests/helpers/windows-link-create-preload.ts, which makes an exclusive create through a link open the link's target as Windows does. All pass after the fix. The planted-journal-link and late-hard-link cases pass before the fix on POSIX because SQLite's unix VFS already refuses both; native Windows is their real check.bun run typecheck,bun run structure:check,bun run privacy:scan: pass.tests/lib/private-sqlite-path.test.ts(new),tests/lib/spend-ledger-owner.test.ts,tests/server/spend-ledger-owner-startup.test.ts, CI shard/lane/crash tests, file-size ratchet, test layout: 0 fail.bun scripts/test.ts --changed=dev --parallel=1(macOS, shared host at load average 5-8): it found one real regression,tests/server/server-stop-config-hardening.test.ts, which spoofsprocess.platformas win32 so the reparse probe failed closed; fixed in 3b49d88 by gating the native probe on the platform read at load (5/5 pass). The remaining failures were 5 s timeouts inlocal-credential-generation,combo-replay-refusal-attribution,server-live-cancelandserver-search; the first three fail the same way with this branch'ssrc/changes reverted to dev (12/24, 3/9 and 1/2), so they are host contention, not this change.bun run testwas not run locally: the host is shared with other agent lanes and the unchanged base already times out under that load (AGENTS.md resource exception). Full coverage is left to this PR'sci:fullLinux shards and the Windows/macOS dispatch below.gh workflow run ci.yml --ref goat/fix-dev-batch-affinity-windowson the final head (maintainer-authorized for this red batch) on be51cb8: run 41455278872 success: windows 1/11-11/11, macos 1/4-4/4 + control, test 1/4-4/4 (11m-12m53s), all other jobs green. The Windows logs show every new case ran and passed natively, includingsymlink at the database path disables the store, the junction / hard-link / sidecar / late-journal-link / late-hard-link / hot-journal cases, the native reparse probe test and the owner cases.ci:full): all green; Linux shards 10m50s-12m58s.Checklist
structure/providers/codex-pool-affinity.md,structure/transports/responses-spend.md)