fix(sdk): keep Windows OS probes and slow syncs off the session-index lock (#4544) - #4555
Conversation
![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: a857b8a6dc
ℹ️ 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".
| const current = probed.get(`${row.sessionId}\u0000${row.endpointGeneration}\u0000${row.pid}`); | ||
| if (current === undefined || current !== recordedIncarnation) continue; |
There was a problem hiding this comment.
When lock acquisition is delayed, the PID observation in probed can be up to the full 60-second retry budget old. If the recorded process exits and its PID is reused after the probe but before this locked check, alive(row.pid) sees the replacement while this stale value still matches the former incarnation, causing a heartbeat to be appended for the wrong process and advertising the dead session as live for up to another heartbeat window. Revalidate the incarnation after contention or otherwise reject observations made before a delayed acquisition.
Useful? React with 👍 / 👎.
| const note = setTimeout( | ||
| () => logger.warn(`sdk broker: session index "${operation}" still holds the index lock after 10s`), | ||
| SESSION_INDEX_OP_SLOW_MS, | ||
| ); | ||
| return withFileLock(logFor(agentDir), callback, SESSION_INDEX_LOCK_OPTIONS).finally(() => clearTimeout(note)); |
There was a problem hiding this comment.
The timer starts before withFileLock has acquired anything, so whenever another process legitimately holds the index lock for more than 10 seconds, every waiting contender logs that its own named operation “still holds” the lock even though its callback has not started. This makes the new diagnostic attribute the stall to unrelated operations; arm and clear the timer inside the lock callback so it measures only actual critical-section duration.
Useful? React with 👍 / 👎.
| const incarnationModule = await import("../src/sdk/broker/process-incarnation"); | ||
| const realProcessIncarnation = incarnationModule.processIncarnation; | ||
| const lockModule = await import("../src/config/file-lock"); |
There was a problem hiding this comment.
These executable await import(...) calls, repeated again later in this test, violate the repository's top-level-import-only contract. Replace them with module-scope namespace imports; vi.spyOn can operate on those namespaces without dynamically loading the modules inside each test.
AGENTS.md reference: AGENTS.md:L113-L116
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3d93d08ac
ℹ️ 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".
| - Managed fallback local snapshot failures now auto-recover with a bounded same-model retry (capped at `retry.maxRetries`) instead of terminating the turn: the discarded attempt is replay-safe and content-free, so the session re-issues the request without charging the provider fallback chain, advancing models, or mutating credentials. Exhausted retries still surface the explicit local diagnostic. | ||
|
|
||
| - Managed fallback local buffer overflows (`local_buffer_overflow`) now surface immediately with the original local diagnostic instead of entering the bounded `unknown` retry class: re-streaming the same request reproduces the same oversized response, and a local staging failure must never charge or advance the provider fallback chain, emit `model_fallback_switched`, or rotate credentials. | ||
| - Fixed Windows startup lock starvation in the SDK session index (#4544): a live detached broker holding `index.jsonl.lock` across a wedged Windows sync-family await exhausted every later launch's full 600-attempt lock budget. OS process-incarnation probes (which can spawn `powershell.exe` on Windows) now run before the machine-global index lock is taken in the heartbeat checkpoint pass and the own-pid registration derivation, the PowerShell fallback is time-bounded (`timeout` + `SIGKILL` enforcement), locked index transactions log an actionable slow-operation warning after 10s naming the exact operation, and lock exhaustion errors now identify the live owner (pid, liveness, lock path) instead of a bare attempt count. Stale-lock safety is unchanged: a proven-live owner's lock is never stolen. |
There was a problem hiding this comment.
Adding this SDK entry also deletes the parent commit’s two existing Unreleased entries for managed snapshot retry recovery and local_buffer_overflow handling. If released in this state, both user-visible fixes disappear from the package release notes; retain those entries and add this one alongside them.
AGENTS.md reference: AGENTS.md:L188-L188
Useful? React with 👍 / 👎.
|
@probepark — requesting your independent review of exact head This fixes #4544: a live detached Windows SDK broker held Scope: 3 commits — the causal fix (OS incarnation probes moved before the machine-global index lock; bounded PowerShell fallback with SIGKILL enforcement; named slow-operation diagnostics; actionable lock-exhaustion errors naming the live owner), Windows tmpdir test portability, and the three Codex review findings (timer attribution inside the callback, stale-probe refresh, top-level imports). Stale-lock safety is unchanged: a proven-live owner is never displaced. Deterministic suite The PR body carries the required — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c485b6ba55
ℹ️ 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".
| probed.set( | ||
| `${row.sessionId}\u0000${row.endpointGeneration}\u0000${row.pid}`, | ||
| processIncarnation(row.pid), | ||
| ); |
There was a problem hiding this comment.
When the unlocked probe batch itself takes over one second—for example, one Windows PowerShell probe reaches the new 5-second timeout, or several ordinary probes accumulate—probedAt was recorded before that batch, so this stale branch runs immediately after acquisition. It then calls synchronous processIncarnation for every candidate while holding the machine-global lock, recreating the starvation this change is intended to remove; enough slow probes can still make other launches exhaust their 60-second lock budget. Record completion after the batch and fail closed or release the lock before refreshing.
Useful? React with 👍 / 👎.
| // stale incarnation still matched the dead process, checkpointing the | ||
| // wrong host. Refresh the observation set whenever contention delayed | ||
| // the pass; a fresh-enough set is kept as-is. | ||
| if (Date.now() - probedAt > SESSION_INDEX_LOCK_STALE_PROBE_MS) { |
There was a problem hiding this comment.
The new one-second cutoff still trusts an observation after shorter contention. If the recorded process exits and its PID is reused during a 100–999 ms lock wait, alive(row.pid) observes the replacement while the cached incarnation still matches the former process, allowing a heartbeat for the dead session. The fresh evidence is this explicit threshold: revalidate or reject candidates whenever acquisition was delayed rather than treating observations under one second old as identity-safe.
Useful? React with 👍 / 👎.
|
@probepark — review request refreshed to exact head
Verification on this head: sdk-session-index ×5 repeated (no flake), lock-contention suite, sdk-broker/router/gc suites (182 pass), lifecycle e2e (82 pass), PR body carries the single — |
7ac9174 to
a2257a6
Compare
|
@probepark — review request updated after rebasing onto current dev: exact head Content unchanged from the reviewed state; all round-1 and round-2 Codex findings remain addressed. Re-verified post-rebase: lock-contention + session-index suites (52 pass), file-lock/broker-restart/router suites (60 pass), Single verdict line carries — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2257a69de
ℹ️ 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".
| if (Date.now() - probedAt > SESSION_INDEX_PROBE_FRESHNESS_MS) return 0; | ||
| await this.#replayUnderLock(); |
There was a problem hiding this comment.
When a large index or slow filesystem makes #replayUnderLock() exceed the 50 ms freshness window, this check has already passed, so a recorded process can exit and its PID can be reused during the awaited replay; the later alive(row.pid) then sees the replacement while probed still contains the former incarnation matching the dead row, allowing a heartbeat to be appended for the wrong host. The fresh evidence in this revision is that the freshness check now precedes the asynchronous replay; recheck after replay and immediately before consuming the cached observations.
Useful? React with 👍 / 👎.
|
Exact-head evidence refreshed after fixing Codex round-4 finding (comment 3783532261):
Round-4 P2 accepted and fixed: the probe-freshness bound was checked only at lock acquisition; the cached observations are consumed after the awaited locked replay, which re-reads the whole log (≤4 MiB rotation bound) and fsyncs audit rows. A pid reused across a slow replay could be checkpointed as the wrong host. The heartbeat pass now rechecks freshness after the replay ( Local verification on the new head: lock-contention suite 6 pass (new regression case proven baseline-red at @probepark — independent review requested on exact head — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25093f59d7
ℹ️ 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".
| : probedIncarnations !== undefined | ||
| ? probedIncarnations.get(probeKey) | ||
| : processIncarnation(latest.pid); |
There was a problem hiding this comment.
On Windows when native incarnation lookup falls back to PowerShell, dead-registration cleanup still calls unregisterIfCurrent(), which holds the machine-global withFileLock and then calls listSessionIdentities() without a probe map; this fallback therefore runs the synchronous probe once per composite identity while the lock is held. With the new five-second per-probe timeout, enough indexed identities can still hold the lock beyond other launches' 60-second acquisition budget, reproducing the starvation this change is meant to fix. Pre-probe this path or provide a projection that does not perform OS liveness probes under the lock.
Useful? React with 👍 / 👎.
| if (info) { | ||
| const liveness = ownerLiveness(info.pid); |
There was a problem hiding this comment.
When withFileLock is used with ownerHostId for the shared-volume topic registry and the lock record belongs to a different host, its PID is meaningful only on that remote host. This diagnostic nevertheless probes the same numeric PID locally and labels the foreign owner “live” or “dead but not reaped,” while omitting owner_host_id; a coincident local PID can therefore be misidentified as the holder and send the operator toward an unrelated process. Pass the requesting host identity into this helper and report foreign owners with unknown liveness instead.
Useful? React with 👍 / 👎.
|
Exact-head CI classification receipt — head Superseded run 31810838937 (cancelled) — attributed, non-product cascade. Both runs are Fresh exact-head run 31811214744 is in progress on Standing requirement for MERGE_READY: native-build, Windows required lanes, all matrix shards, evidence producer + Affected path validation aggregate, and Virtual integration validation all terminal green on — |
|
Exact-head evidence refreshed after Codex round-5 findings (head Round-5 triage — 2 real (fixed), 2 stale re-posts (already addressed):
New digest: Local verification on the new head: lock-contention 7 pass (both new regression cases baseline-red at their unfixed sources); session-index + broker-restart + file-lock-gc-toctou 68 pass; router-authority 39 pass; gc suites 81 pass; broker lifecycle e2e 82 pass; @probepark — independent review requested on exact head — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7171a4f7f7
ℹ️ 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".
| if (recordedIncarnation === undefined) continue; | ||
| probed.set(`${row.sessionId}\u0000${row.endpointGeneration}\u0000${row.pid}`, processIncarnation(row.pid)); | ||
| } | ||
| const probedAt = Date.now(); |
There was a problem hiding this comment.
When multiple sessions are eligible and a later Windows incarnation probe is slow or reaches its five-second timeout, the first cached observation may already be seconds old even though both freshness checks see an age near zero because probedAt is recorded only after the sequential batch. If that first process exits and its PID is reused while the remaining probes run, the locked pass sees the replacement as alive while the cached incarnation still matches the dead registration, so it emits a heartbeat that remains fresh for up to two heartbeat intervals. The fresh evidence in this revision is this post-batch timestamp; retain the batch start or a per-observation timestamp and reject observations that exceeded the freshness bound.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 309b23fb70
ℹ️ 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".
| for (const row of reduceEvents(this.#events, this.#policy.clock(), this.#agentDir).identities) { | ||
| const recordedIncarnation = row.hostIncarnation ?? row.processIncarnation; | ||
| if (recordedIncarnation === undefined) continue; | ||
| probed.set(`${row.sessionId}\u0000${row.endpointGeneration}\u0000${row.pid}`, processIncarnation(row.pid)); |
There was a problem hiding this comment.
When Windows native incarnation lookup falls back to the synchronous PowerShell path, this enumeration already probes each identity inside reduceEvents (and probes each selected authority again while building sessions), after which line 1173 probes it yet again. The dead-registration sweep may invoke unregisterIfCurrent for up to 64 rows, so an index with many fallback identities performs this repeated full-index batch for every unregister, potentially blocking the broker event loop for minutes or hours despite moving the work outside the lock. Derive the probe targets without a liveness-projecting reduction, or capture and reuse each observation once.
Useful? React with 👍 / 👎.
|
Cohort generation 2 — exact head The boundary review cohort (cleaner + architect + QA red-team) joined on the previous head and found one real defect plus review gaps, all fixed at this head:
Local verification on this head: lock-contention 10 pass; session-index + broker-restart + file-lock-gc-toctou + router-authority 107 pass; gc suites 81 pass; broker lifecycle e2e 82 pass; @probepark — independent review requested on exact head — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 084f6e08b3
ℹ️ 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".
| // on that host, so probing the same numeric pid here could mislabel a | ||
| // coincident local process as the holder. Report the owner host with | ||
| // unknown liveness instead. | ||
| if (info.owner_host_id !== undefined) { |
There was a problem hiding this comment.
When withFileLock is called with ownerHostId, the presence of owner_host_id does not by itself mean the holder is foreign: a competing process on the same host writes the same ID, while an unqualified legacy/malformed record is deliberately treated as non-local by staleLockSnapshot. This branch therefore labels local live owners as remotely unverifiable and can still probe a coincidental local PID for an unqualified record on a shared volume. Pass the requester's ownerHostId into this helper and probe only when the two host IDs match.
Useful? React with 👍 / 👎.
084f6e0 to
a543a20
Compare
|
Rebase receipt — dev advanced (#4558 merged), branch rebased onto current dev:
Post-rebase verification on a543a20: lock-contention + session-index + broker-restart + file-lock-gc-toctou + config-cli → 94 pass; #4558 notification suites (inbound-acceptance/production-path/reaction-ordering/topic-registry/turn-ordering) → 101 pass; Windows session-path fsync + broker lifecycle e2e + router authority → 122 pass 1 skip (Windows-only skip); @probepark — fresh independent review request on exact head — |
|
Dependency-held — cross-PR integration dependency recorded (exact head Classification of the guard red (real product red, NOT attributable to this PR): the Telegram daemon generation guard fails on this head with four stale discord/slack Exact repair lives in dedicated PR #4563 (head #4555 standing state: every other exact-head product job is green or still running (plan/gjc-state-gates/native-build/Windows lanes/shards); Terminal path: #4555 is dependency-held on #4563's terminal merge. The moment #4563 merges: rebase onto the new dev, rerun full exact-head CI, recompute the digest, refresh the needs-human verdict + signed evidence, and merge on independent exact-head approval + green + clean — then close #4544 and run current-dev verification. Any non-guard product failure on this PR is fixed forward immediately; none exists at this head. — |
|
Exact-head CI terminal decomposition — run 31821397603 on
Green on this head: native-build, Windows dev:doctor + session-path regression, Windows Telegram daemon safety, check:@gajae-code/coding-agent, cli-smoke, all cargo builds, gjc-state-gates, plan. Skips are structural (darwin smoke plan-gated, Windows toolchain path-gated). Dependency hold stands on #4563 terminal merge → then rebase, full CI rerun, digest refresh, merge on approval + green + clean. — |
|
Reviewer coverage closed + dependency/terminal product evidence — exact head Reviewer requests now explicit on GitHub (previously only named in the body): @probepark (primary, named in the verdict line) and @HaD0Yun (backup) — both carry Terminal product evidence at this head (CI run 31821397603):
Local exact-head product verification (this worktree, same head): lock-contention + session-index + broker-restart + file-lock + config-cli 94 pass; #4558 notification suites 101 pass (rebase preserved); fsync.windows + broker-lifecycle-e2e + router-authority 122 pass (1 Windows-only skip); Standing dependency path (no change): #4555 is dependency-held on #4563's terminal merge → rebase onto new dev → full exact-head CI → digest refresh → fresh explicit reviewer request → merge only on approval + green + clean → close #4544 → current-dev verification. — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve at a543a2046
fix(sdk): keep Windows OS probes and slow syncs off the session-index path, head commit
"make probe freshness monotonic and close cohort review gaps". +736/-36 across 9 files.
Differential
# base 9d2a2d2f2, with this head test files applied
90 pass 10 fail
(fail) dev-ci canonical-plan workflow contract > routes Windows session-path regression suite onto windows-latest
(fail) planTargetedTasks PR-mode targeting > routes Windows session-path regression for session I/O sources
... 8 more
# head a543a2046
100 pass 0 fail
$ bun --cwd=packages/coding-agent run check -> exit 0
Ten failures on base against ten behaviours. The two visible ones are CI-routing contracts - the
Windows session-path regression suite has to be scheduled onto a Windows runner and targeted in
PR mode - which is the right pairing for a change about keeping Windows probes off a hot path:
the fix is only meaningful if the Windows suite actually runs.
"Monotonic probe freshness" is the part I would have asked about had it not been in the title. A
freshness value that can go backwards is how a slow probe overwrites a newer result and reintroduces
the stall this removes; making it monotonic is the property that makes the off-path move safe rather
than merely faster.
merge-approved.
Reviewed by @probepark - method: fresh-worktree run and package typecheck at the exact head, separate clean base worktree with the head test files to prove the differential.
a543a20 to
0e34526
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Exact-head evidence — fresh current-dev reconstruction on PR #4555: Supersession: head advanced
@probepark — fresh independent review requested on exact head — |
|
Terminal exact-head CI decomposition — run 31858127992 on
On a fresh APPROVED at — |
… lock (#4544) A live detached SDK broker holding <agentDir>/sdk/sessions/index.jsonl.lock across a wedged Windows sync-family await starved every new gjc launch past the full 600-attempt lock budget (~60s) and crashed startup. The stale-lock recovery discipline (#652) is correct — a proven-live owner must never be stolen from — so the fix bounds what the lock holder can do to the machine-global critical section instead of weakening it. Causal changes, each independently testable: - OS process-incarnation probes now run BEFORE the machine-global index lock is taken: the heartbeat checkpoint pass probes candidates from the last replayed history, then the locked section re-validates each row against the fresh replay plus the probed observation (a stale probe set can only skip a heartbeat for one cycle, never checkpoint a wrong host — fail-closed). The own-pid registration derivation in append() moved before the lock the same way, and reduceEvents/projectIdentity accept a probed-incarnation map so the locked projection never probes the OS either. - The Windows PowerShell incarnation fallback (Bun.spawnSync of powershell.exe) is bounded: timeout 5s with SIGKILL enforcement, so a wedged powershell can no longer block its caller — and the machine-global lock — indefinitely. - Every locked session-index transaction now runs through one named choke point (withSessionIndexLock) that logs an actionable slow-operation warning after 10s identifying the exact operation still holding the lock, instead of the failure surfacing only as a bare lock-exhaustion crash on later launches. - Lock exhaustion errors now carry bounded, actionable diagnostics: the live owner's pid, its liveness verdict, and the lock path, plus the explicit never-displaced guarantee — the user can act on the broker pid instead of only an attempt count. Deterministic regression coverage (sdk-session-index-lock-contention.test.ts, also routed into the Windows dev:doctor CI job via the affected-plan path list): live-owner lock retention without stealing plus the new diagnostics, heartbeat/append probes outside the lock-held section, throw-inside-critical- section release, aborted-acquisition fail-fast, and concurrent launches converging behind a legitimate bounded holder. The telegram baseline manifest regeneration it carries is the mechanical sync required by check:sdk-closure. Lore-id: issue-4544-windows-index-lock Constraint: never steal a lock from a proven-live owner (#652 discipline) Constraint: Windows durability (fsync/FlushFileBuffers) semantics unchanged Rejected: stealing/force-releasing the live broker's lock | breaks #652 and corrupts concurrent index transactions Rejected: dropping fsync on Windows | weakens durability; #4254 already fixed the read-only-handle EPERM Rejected: timeout-interrupting locked index transactions | risks torn log/snapshot state mid-write Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test packages/coding-agent/test/sdk-session-index-lock-contention.test.ts Tested: bun test packages/coding-agent/test/sdk-session-index.test.ts packages/coding-agent/test/file-lock-gc-toctou.test.ts (x3 repeated) Tested: bun test packages/coding-agent/test/sdk-broker*.test.ts (6 suites, 155 tests) Tested: bun test packages/coding-agent/test/sdk-broker-lifecycle-e2e.test.ts (82 tests) Tested: bun test packages/coding-agent/test/gc-runtime.test.ts gc-disk-retention.test.ts gc-e2e.test.ts Tested: bun --cwd=packages/coding-agent run check (biome + tsc clean) Not-tested: real windows-latest run (routed to windows-dev-doctor in dev CI)
On windows-latest TMPDIR is unset, so path.join(TMPDIR ?? "/tmp", …) produced \tmp and every mkdtemp failed ENOENT before exercising any product behavior — the Windows dev:doctor matrix (job 94749721733) reported all five cases failing while the focused Linux run passed. Use os.tmpdir() like the existing sdk-session-index-fsync.windows.test.ts convention; no production code or assertion changes. Lore-id: issue-4544-windows-test-portability Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test packages/coding-agent/test/sdk-session-index-lock-contention.test.ts (5 pass) Tested: bun test sdk-session-index.test.ts sdk-session-index-fsync.windows.test.ts file-lock-gc-toctou.test.ts Not-tested: real windows-latest rerun (dev-ci will re-run it on this head)
Three review findings on the first fix, all accepted: - Slow-op timer attribution (P2): the 10s warning timer armed before lock acquisition, so queueing time behind another legitimate holder was attributed to this operation's name. The timer now arms inside the lock callback and clears in its finally — it measures only actual critical-section duration. - Stale incarnation observation (P2): a pid could exit and be reused between the pre-lock probe and the locked write (acquisition may queue behind another holder for up to the full retry budget), letting a stale observation that still matched the dead process checkpoint a heartbeat for the reused pid. Observations older than 1s at acquire time are now re-probed before any heartbeat is written. - Test contract (P1): dynamic `await import(...)` calls in the new test violated the top-level-import-only contract; replaced with module-scope namespace imports (vi.spyOn works on the namespaces directly). Lore-id: issue-4544-review-findings Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test packages/coding-agent/test/sdk-session-index-lock-contention.test.ts (5 pass) Tested: bun test sdk-session-index.test.ts file-lock-gc-toctou.test.ts sdk-broker-restart.test.ts sdk-session-router-authority.test.ts (107 pass) Tested: bun test packages/coding-agent/test/sdk-broker.test.ts (62 pass) Tested: bun --cwd=packages/coding-agent run check (biome + tsc clean) Tested: live-broker contention harness (launch converges <1s behind bounded holder)
…tries Second review round on the #4544 fix, all accepted: - P1: the stale-probe refresh ran processIncarnation while HOLDING the machine-global lock — recreating the exact starvation this change removes whenever the unlocked batch itself was slow (e.g. one Windows probe hitting the 5s timeout). Removed entirely: identity evidence gathered before a delayed acquisition is now discarded and the cycle writes no heartbeat (fail-closed). The OS is never probed under the lock, in any path. - P2: the 1s staleness threshold still trusted observations after short contention (100-999ms waits), where pid reuse would checkpoint a dead session as live. Replaced with an explicit freshness bound measured from probe-batch completion: uncontended acquisition + scheduler jitter passes, anything delayed fails closed. - P2: the first round's CHANGELOG conflict resolution displaced dev's two existing Unreleased entries (managed snapshot retry recovery, local_buffer_overflow handling); both restored ahead of this fix's entry. Lore-id: issue-4544-review-round2 Constraint: OS probes never run while the machine-global index lock is held Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test sdk-session-index.test.ts x5 repeated (47 pass, no flake) Tested: bun test sdk-session-index-lock-contention.test.ts file-lock-gc-toctou.test.ts (69 pass) Tested: bun test sdk-broker*.test.ts sdk-session-router-authority.test.ts gc-*.test.ts (182 pass) Tested: bun test sdk-broker-lifecycle-e2e.test.ts (82 pass) Tested: bun --cwd=packages/coding-agent run check (biome + tsc clean) Tested: live-broker contention harness green
Codex review round 4 on the #4544 exact head (comment 3783532261): the probe-freshness bound was checked only at lock acquisition, but the observation set is consumed after the awaited locked replay. A replay re-reads the whole log (up to the 4 MiB rotation bound) and fsyncs pending audit rows, so a large index or a wedged Windows sync-family await can stretch the probe-to-write interval well past the acquisition bound while the machine-global lock is held. A pid can exit and be reused across that window; alive(pid) would see the replacement while the cached incarnation still matched the dead row, checkpointing the wrong host as live. The heartbeat pass now rechecks freshness after the replay against a dedicated replay bound (2s, sized for the full locked replay cost envelope of a healthy machine instead of scheduler jitter alone) and fails closed - no heartbeat this cycle - before consuming the cached observations. The OS is still never probed under the lock; the next pass re-probes from scratch. Regression test stretches the replay deterministically (clock jump at the locked log read) and proves the baseline-red: without the recheck the heartbeat is written; with it the cycle writes nothing and the following pass recovers normally. Lore-id: issue-4544-review-round4 Constraint: OS probes never run while the machine-global index lock is held Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test sdk-session-index-lock-contention.test.ts (6 pass; new case baseline-red at e6f8c92) Tested: bun test sdk-session-index.test.ts sdk-broker-restart.test.ts file-lock-gc-toctou.test.ts (69 pass) Tested: bun test sdk-broker-lifecycle-e2e.test.ts (82 pass) Tested: bun --cwd=packages/coding-agent run check (biome + tsc clean)
Codex review round 5 on the #4544 fix head: - P1 (session-index.ts:380): the conditional-unregister pass held the machine-global lock and then projected every composite identity through listSessionIdentities(), whose unlocked projection probes the OS once per identity - on Windows a powershell.exe spawn under the lock, the exact starvation this change removes whenever enough identities are indexed. The probes now run before the lock is taken and seed the locked projection; the equality checks still compare against replayed events, so a stale observation can only refuse an unregister (fail-closed), never retire a live registration. - P2 (file-lock.ts:342): a lock record carrying a foreign owner_host_id (shared-volume topic registry) belongs to another machine, but the exhaustion diagnostic probed the same numeric pid locally and labeled the coincident process live/dead, sending the operator toward an unrelated process. Foreign owners now report their host with unknown liveness instead. - The remaining two round-5 comments restate round-3/round-4 findings already addressed (acquisition-bound freshness recheck and the post-replay recheck) and need no change. Lore-id: issue-4544-review-round5 Constraint: OS probes never run while the machine-global index lock is held Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test sdk-session-index-lock-contention.test.ts (7 pass; unregister case baseline-red without fix) Tested: bun test sdk-session-index.test.ts sdk-broker-restart.test.ts file-lock-gc-toctou.test.ts (68 pass) Tested: bun test sdk-session-router-authority.test.ts (39 pass) gc-*.test.ts (81 pass) Tested: bun test sdk-broker-lifecycle-e2e.test.ts (82 pass) Tested: bun --cwd=packages/coding-agent run check (biome + tsc clean)
Round-5 file-lock fix (#4544) shipped without a regression test for the foreign owner_host_id branch of lockHolderDescription: a shared-volume lock record owned by another host must report its owner host with liveness unknown instead of probing the coincidental local pid and labeling it live/dead. The test writes a foreign-host lock record whose pid is the live local test process and asserts the exhaustion error names the host, claims unknown liveness, and never labels the local process the holder; the foreign owner's lock is never stolen either. Verified baseline-red at the pre-fix parent 25093f5 (message labels the local pid live) and green at this head. Lore-id: issue-4544-review-round5 Constraint: foreign lock owners are never locally liveness-probed Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test sdk-session-index-lock-contention.test.ts (8 pass) Tested: baseline-red at 25093f5 (7 pass 1 fail on the new case)
Boundary cohort generation 2 on the #4544 change set: - Red-team defect (real): both heartbeat freshness bounds measured the probe-to-write interval with Date.now(), a wall clock. A backward step (manual clock fix, NTP slew, VM snapshot restore) makes the interval negative, both bounds pass, and an arbitrarily stale observation batch is consumed - reproduced with a 70s-stale batch writing a heartbeat. The bounds now measure performance.now() (monotonic, never steps backward), so the fail-closed guarantee cannot be defeated by the system clock moving. Regression test steps the wall clock back 70s while advancing the monotonic clock past the replay bound and proves the monotonic advance dominates; baseline-red at the pre-fix head. - Architect P2: unregisterIfCurrent used a raw withFileLock, bypassing the withSessionIndexLock choke point - no 10s named slow-operation warning and the 5s default retry budget instead of the 60s index budget. Routed through withSessionIndexLock('conditional unregister'). - Red-team comment inaccuracy: the unregister pre-lock probe batch does not 'refuse an unregister' on staleness - the decision never consults the projected live flag at all. The comment now states the truth: the observations are inert to the decision and exist only so the locked projection performs no OS probes. - Architect P3s: the slow-op warning timer is unref()ed so it can never retain the process on an exit path; the same-host lock diagnostic uses the full ownerIsAlive proof (pid + start_time identity) so a reused pid is not mislabeled '(live)'; changelog wording aligned. - Test gap: the 50ms acquisition-freshness branch was untested; a new regression test advances the monotonic clock inside lock acquisition and proves the cycle fails closed (baseline-red at the pre-fix head). Lore-id: issue-4544-cohort-generation2 Constraint: freshness bounds are monotonic; OS probes never run under the lock Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test sdk-session-index-lock-contention.test.ts (10 pass; wall-clock + acquisition cases baseline-red at 309b23f) Tested: bun test sdk-session-index.test.ts sdk-broker-restart.test.ts file-lock-gc-toctou.test.ts sdk-session-router-authority.test.ts (107 pass) Tested: bun test gc-runtime.test.ts gc-disk-retention.test.ts gc-e2e.test.ts (81 pass) Tested: bun test sdk-broker-lifecycle-e2e.test.ts (82 pass) Tested: bun --cwd=packages/coding-agent run check (biome + tsc clean)
0e34526 to
c20a672
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Dev advanced via #4540 (
@probepark (primary) / @HaD0Yun (backup): fresh exact-head review requested at — |
|
Terminal exact-head CI decomposition — run 31860058461 on Green at this head: native-build, plan, all cargo builds, cli-smoke, ci-dry-run/selftest, yaml-parse, workflow-permissions, gjc-state-gates (static/read/runtime/integrity), all affected test shards, Telegram daemon generation guard, Windows Telegram daemon safety, Windows dev:doctor + session-path regression (ran @probepark @HaD0Yun — exact-head — |
|
OWNER_CONFIRMATION_REQUIRED
— |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve at c20a6725e
Head moved since my approval of a543a2046; same change, rebased onto 96e718a2b. Re-verified
rather than assumed, since approvals are bound to the exact head:
$ bun test <the PR's touched suites>
100 pass
0 fail
$ bun --cwd=packages/coding-agent run check -> exit 0
The substantive review stands: keeping Windows OS probes and slow syncs off the session-index path,
with monotonic probe freshness as the property that makes the move safe - a freshness value that can
go backwards would let a slow probe overwrite a newer result and reintroduce the stall this removes.
merge-approved at c20a6725e.
Reviewed by @probepark - method: fresh-worktree run and package typecheck at the exact head.
Freshness dependency hold — current dev is redThis PR remains open at exact head Current dev is not a valid green rebase target: push run Disposition: item-specific hold remains active behind #4574. After #4574 merges and dev CI is green, this PR must be reconciled to that exact new dev head, with refreshed digest, CI, and review before any merge-ready claim. — |
Terminal retirement record — PR #4555 / issue #4544Reproducible ownership and merge evidence for the SDK-session-reset re-establishment run. Binding (all live-verified 2026-08-15, before and after mutation)
Stale-run disposition (not rerun)
Live re-read before mutation (sole-ownership re-establishment)PR state/head/base, all 30 check jobs of run 31881501892, review list filtered to exact head, issue #4574 state (closed, dev green again at Post-merge verification at
|
2 participants
Fixes #4544.
Problem (independently reproduced)
On Windows,
gjclaunches failed startup with:The lock owner was a live detached SDK broker holding
index.jsonl.lockacross a wedged Windows sync-family await inside the locked critical section. Stale-lock recovery correctly refused to remove the live owner (#652 discipline), so every later launch burned the full 600×100ms budget and crashed.Deterministic baseline-red harness (Linux seam): a live process acquires the lock through the production
withFileLockand awaits a never-settling promise inside the critical section; a second process runs productionSessionIndex.open()→ it exhausts all 600 attempts in 60.6s and crashes with exactly the reported error. Post-fix, the same permanent wedge still refuses to steal (correct) but fails with actionable owner diagnostics, and the realistic live-broker scenario (repeated slow-but-bounded locked passes) no longer starves a concurrent launch (LAUNCH-OK in <1s).Root cause and fix (minimal, causal)
The machine-global lock must not be held across unbounded work. Three independent contributors, all fixed:
checkpointLiveHeartbeatscalledprocessIncarnation(pid)per live row while holding the machine-global lock; on win32 the fallback isBun.spawnSync(powershell.exe …)— a synchronous OS spawn with no bound.projectIdentity(inside the lockedreduceEvents) probed too, as did the own-pid derivation inappend. All now run before the lock:reduceEvents/projectIdentityaccept a probed-incarnation map so the locked projection never probes the OS;appendderives the own-pid incarnation before taking the lock (the own pid is fixed for the process lifetime, so it cannot race anything).timeout: 5000withkillSignal: "SIGKILL"enforcement (verified: Bun honorstimeout/killSignaleven against a TERM-ignoring child).withSessionIndexLock(operation, …)) that logssdk broker: session index "<operation>" still holds the index lock after 10s— the exact operation is attributable from logs instead of surfacing as a bare 600-attempt crash. Lock-exhaustion errors now identify the holder: pid, liveness verdict, lock path, and the explicit never-displaced guarantee.Stale-lock safety is unchanged: a proven-live owner's lock is never stolen; a dead owner is still reclaimed immediately.
Regression coverage
New
packages/coding-agent/test/sdk-session-index-lock-contention.test.ts(deterministic, also routed into the windows-dev-doctor CI job via the affected-plan path list + workflow step):Also carries the mechanical regeneration of
telegram-baseline-v1.jsonrequired bycheck:sdk-closure(pre-existing drift on exact dev HEAD:cf07293f74addedtelegram-*tests without regenerating).Verification
bun test packages/coding-agent/test/sdk-session-index-lock-contention.test.ts→ 5 passbun test packages/coding-agent/test/sdk-session-index.test.ts packages/coding-agent/test/file-lock-gc-toctou.test.ts→ 64 pass (×3 repeated runs)bun test packages/coding-agent/test/sdk-broker*.test.ts(6 suites) → 155 pass;sdk-broker-lifecycle-e2e.test.ts→ 82 passbun test packages/coding-agent/test/gc-runtime.test.ts gc-disk-retention.test.ts gc-e2e.test.ts→ 81 passbun test scripts/ci-dev-affected.test.ts scripts/dev-ci-guard-topology.test.ts→ 98 passbun --cwd=packages/coding-agent run check(biome + tsc) → clean—
[repo owner's gaebal-gajae (clawdbot) 🦞]
GJC verdict
devbun checkpassesRebase onto current dev — supersession receipt (head
0e34526865)Dev advanced (#4563 merged, repairing the inherited Telegram daemon generation guard). The branch was reconstructed onto current dev:
9d2a2d2f2d/a543a20469815c1e657678d19c81e2c3a25fff91(digestsha256:d8073caec4f8…3542d) — probepark's APPROVED review ata543a20469is superseded by the new head; a fresh exact-head approval is required.64c15281691280be7854dac04baeb05188328ef4/0e34526865e035118df988eb39bc19059caad26a; new exact diff digestsha256:c8c2667959dee14f32ab9e294f386f43b85dbdf97fabb0a9938a1f2b37e5a3ef.clawdbot <clawdbot@users.noreply.github.com>, subject lines unchanged); the new base...head diff is byte-identical to the previously approved delta exceptpackages/coding-agent/CHANGELOG.mdcontext lines that only reflect dev's own SDK: terminal reconciliation expires before fire-and-wake consumption #4547 entry (verified by full-diff comparison).sdk-session-index-lock-contention10/10;sdk-session-index+file-lock-gc-toctou64/64;sdk-brokerfamily (restart/tail/notify/gc + core) 66 pass;sdk-broker-lifecycle-e2e82/82;sdk-session-router-authority+gc-runtime64/64; Windows session-path family 6 pass / 10 win32-gated skips;scripts/ci-dev-affected.test.ts90/90 with all four PR source paths confirmed to route to the Windows session-path regression job; Telegram daemon generation guard--validate-current-treeexit 0 (the dev-inherited red is repaired by chore(guard): refresh discord/slack session-router digests after #4542 #4563) and guard tests 75/75;telegram-baseline-v1.jsonmanifest 54/54 receipts green;bun --cwd=packages/coding-agent run checkclean;verify-gjc-state-writers --failclean.@probepark — requesting your fresh independent review of exact head
0e34526865e035118df988eb39bc19059caad26a(base64c1528169). @HaD0Yun — backup reviewer. One APPROVED review on0e34526865flips the verdict tomerge-approvedwith the new digest.—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Rebase receipt #2 — dev advanced via #4540 (head
c20a6725e7)Dev advanced again (
64c1528169→96e718a2b0f2a46cad4876bf922886966c6fd0e3, #4540 selection-order fixes; zero overlap with this PR's source files — only CHANGELOG context). All trial-merge/CI evidence bound to0e34526865/base64c1528169is stale for terminal merge:96e718a2b0f2a46cad4876bf922886966c6fd0e3/c20a6725e78b8d12f179a96bae0c26952f849e80; new exact diff digestsha256:71b9faedf9593ee2244e42f555f43bcf612c3d0f2a7e6be5f75cb7a9c4ee7011.clawdbot), base…head delta identical modulo CHANGELOG context (verified by full-diff comparison against the0e34526865delta).--validate-current-treeexit 0; state-writers fast gate clean; packagecheckclean.0e34526865) remains valid as product-behavior proof of the identical delta but is not terminal for merge; fresh CI is running onc20a6725e7.@probepark / @HaD0Yun — the exact-head review request now targets
c20a6725e78b8d12f179a96bae0c26952f849e80(base96e718a2b0). One APPROVED on that head flips the verdict tomerge-approvedwith digest71b9faed…e7011.—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Round-4 Codex finding (comment 3783532261) — fixed at 25093f5
P2 "Recheck probe freshness after replay" — accepted, fixed forward. The freshness bound was checked only at lock acquisition, but the cached observations are consumed after the awaited locked replay (
#replayUnderLock()re-reads the whole log up to the 4 MiB rotation bound and fsyncs pending audit rows). A large index or a wedged Windows sync-family await can stretch the probe→write interval past the acquisition bound while the lock is held; a pid reused across that window would letalive(pid)see the replacement while the cached incarnation still matched the dead row — heartbeat for the wrong host.Fix: the heartbeat pass rechecks freshness after the replay against a dedicated
SESSION_INDEX_REPLAY_FRESHNESS_MS = 2sbound (sized for the full locked replay cost envelope of a healthy machine, not scheduler jitter) and fails closed — no heartbeat this cycle — before consuming the cached observations. The OS is still never probed under the lock; the next pass re-probes from scratch.Regression test (
sdk-session-index-lock-contention.test.ts): stretches the replay deterministically (clock jump at the locked log read); verified baseline-red ate6f8c926dd(heartbeat written without the recheck) and green with the fix (cycle writes nothing; following pass recovers normally).—
[repo owner's gaebal-gajae (clawdbot) 🦞]