fix(sdk): heal leaked conversation store locks instead of timing out - #4181
fix(sdk): heal leaked conversation store locks instead of timing out#4181Yeachan-Heo wants to merge 6 commits into
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: b35689a57e
ℹ️ 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 (fileLock) await fileLock.close().catch(() => undefined); | ||
| if (fileLock && lockToken) await this.#unlinkOwnedLock(lockFile, lockToken); | ||
| } finally { | ||
| heldLockFiles.delete(lockFile); |
There was a problem hiding this comment.
When another same-process ConversationStore acquires this path after #unlinkOwnedLock removes the old file but before this finally runs, that acquire adds the path to heldLockFiles and this unconditional deletion immediately removes the successor's marker. A third waiter can then classify the successor's live lock as a leak, unlink it, and enter the critical section concurrently, allowing conversation updates to overwrite one another. Track the acquire token in the registry and delete only the entry owned by this release.
Useful? React with 👍 / 👎.
| await this.#fs.unlink(lockFile).catch(() => undefined); | ||
| return true; |
There was a problem hiding this comment.
If deletion of a same-process leaked lock fails—for example, an unsuccessful close() leaves the handle open on Windows and unlink returns EPERM—this method suppresses the error and returns true. The acquire loop consequently retries immediately via continue, bypassing both its timeout check and sleep on every iteration, so the daemon spins indefinitely instead of recovering or raising ConversationLockTimeoutError. Return true only after deletion succeeds or the path is confirmed absent.
Useful? React with 👍 / 👎.
| // A leaked reclaim lock from this process must not block reclaim of a | ||
| // provably stale main lock. The double-reclaim window this opens is | ||
| // benign: both reclaimers unlink the same stale main lock idempotently. | ||
| if (await this.#reclaimSameProcessLeak(reclaimFile)) { |
There was a problem hiding this comment.
When two same-process store instances concurrently encounter the same stale main lock, the first instance creates and actively holds the .reclaim file through #createLockFile, but reclaim locks are never added to heldLockFiles. The second instance therefore treats that live mutex as leaked here, removes it, and creates another reclaimer. Both can then pass the stale recheck; one may delete the main path after the other has already replaced it with a live lock, breaking mutual exclusion and risking lost conversation updates. Reclaim-lock ownership must be registered just like main-lock ownership rather than deliberately permitting two reclaimers.
Useful? React with 👍 / 👎.
b35689a to
7d0c951
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d0c951b10
ℹ️ 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 (heldLockFiles.has(lockFile)) return false; | ||
| await this.#fs.unlink(lockFile).catch(() => undefined); |
There was a problem hiding this comment.
When two same-process waiters encounter the same leaked lock, both can read its metadata and pass this held-set check before either deletion completes. After the first waiter unlinks the leak and acquires a replacement, the second waiter's path-based unlink can delete that replacement; it then returns true, retries, and may acquire the path while the first waiter is still in its critical section, allowing conversation updates to overwrite each other. Reclaim the leak under an ownership-checked mutex or otherwise ensure the inode/token validated here is still the one being removed.
Useful? React with 👍 / 👎.
Signed exact-head CI verdict (head
|
7d0c951 to
20f3a48
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6b454c732
ℹ️ 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 staleLock = await this.#readLock(lockFile); | ||
| if (!staleLock || !(await this.#isStaleLock(lockFile))) return false; |
There was a problem hiding this comment.
When a process exits after open("wx") but before publishing valid metadata, the resulting empty or malformed lock is deliberately considered stale after 30 seconds by #isExpiredUnpublishedLock, but this new !staleLock return prevents it from ever being removed. Every later conversation mutation therefore times out until the artifact is manually deleted, whereas the previous implementation reclaimed it; preserve an ownership-safe recovery path for expired unpublished locks without requiring parseable metadata.
Useful? React with 👍 / 👎.
| this.#releaseHeldLock(lockFile, lock); | ||
| await handle.close().catch(() => undefined); | ||
| await this.#fs.unlink(lockFile).catch(() => undefined); |
There was a problem hiding this comment.
If metadata writing succeeds but handle.sync() rejects, this path removes the held marker before awaiting close() and then unconditionally unlinks the path. During that await, another same-process store can classify the published lock as leaked, remove it, and acquire a replacement; the failed creator then deletes that successor at line 382, allowing another writer into the critical section concurrently. Keep the token registered through cleanup and ownership-check the cleanup unlink.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c815f86a8
ℹ️ 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".
| */ | ||
| async #reclaimSameProcessLeak(lockFile: string): Promise<boolean> { | ||
| const expected = await this.#readLock(lockFile); | ||
| if (!expected || expected.pid !== this.#pid || heldLockFiles.has(lockFile)) return false; |
There was a problem hiding this comment.
When two store instances address the same underlying directory through different path aliases—for example, a symlink and its real path or differently cased paths on Windows—the live holder is registered under one raw string while this lookup uses the other. The waiter consequently classifies the live same-process lock as leaked, unlinks it after matching its metadata, and enters the critical section concurrently, allowing conversation updates to overwrite each other. Key the held-lock registry by a canonical filesystem identity rather than the caller-supplied path.
Useful? React with 👍 / 👎.
…me-process leaks The Discord fake-provider retry/ambiguous-send row (PR #4153, CI run 31387018881) and the Slack ACK-boundary replay row (PR #4098, earlier runs) both flaked with a 1000ms ConversationLockTimeoutError on an isolated effects.json.lock inside their own test process. Root cause: a well-formed lock file whose recorded pid is still alive is unreclaimable by any waiter (staleness only covers dead pids and empty files), so a release that failed between close() and unlink() -- or any interrupted release -- turns every later acquire into a full 1000ms timeout. The failure reproduces deterministically by planting a live-pid lock file. Fix, shared by every ConversationStore user (conversation mappings and the effects.json ChatEffectJournal alike): - release is idempotent: a failed close() no longer skips the unlink, and the unlink is ownership-checked (pid+incarnation+timestamp+nonce) so it can only remove the lock this holder wrote - waiters self-heal same-process leaks: a well-formed lock recorded under this process's own pid with no live holder (module-level held set) is removed instead of waiting out the timeout - the 1000ms lock timeout is unchanged and live cross-process locks are never deleted Verified with 3 new regression tests in sdk-daemon-concurrency.test.ts, the full Discord (52) and Slack (49) daemon suites, and the 12-file lock-consumer shard batch (390 tests). Constraint: no timeout inflation -- the 1000ms lock window is untouched Constraint: no blind lock deletion -- only provably leaked same-process locks are removed Rejected: raising the lock timeout | masks the leak instead of fixing it Rejected: unconditional waiter-side unlink | could delete a live holder's lock Confidence: high Scope-risk: medium Reversibility: revert-safe Tested: leaked-lock heal, live same-process holder protection, cross-process lock preservation Not-tested: cross-process leak heal (holder process must exit first; unchanged behavior)
A close failure must not leave the exclusive conversation-store lock behind.\n\nThe regression exercises the real release path and confirms a later write can acquire the lock.\n\nConfidence: high\nScope-risk: low\nReversibility: revert-safe\nTested: bun test packages/coding-agent/test/sdk-daemon-concurrency.test.ts; bun --cwd=packages/coding-agent run check
Concurrent same-process waiters could each classify a leaked lock as removable and delete a successor.\n\nAn ownership-tracked reclaim lock now serializes healing, preserves successor markers, and returns to the configured timeout path when unlink cannot complete.\n\nConstraint: retain the 1000ms lock timeout\nConstraint: never delete a successor lock\nRejected: blind unlink retry | permits concurrent critical sections\nConfidence: high\nScope-risk: medium\nReversibility: revert-safe\nTested: bun test packages/coding-agent/test/sdk-daemon-concurrency.test.ts; bun test packages/coding-agent/test/session-resident-lifecycle.test.ts; bun --cwd=packages/coding-agent run check
Reclaim markers must recover safely after interrupted publication without letting cleanup poison later same-process recovery.\n\nNested cleanup now always clears its ownership marker, and stale auxiliary locks are reclaimed under their own fence.\n\nConstraint: preserve live reclaim locks\nConstraint: recover expired unpublished lock files\nConfidence: high\nScope-risk: medium\nReversibility: revert-safe\nTested: bun test packages/coding-agent/test/sdk-daemon-concurrency.test.ts; bun --cwd=packages/coding-agent run check
A failed stale-lock unlink must honor the configured acquisition deadline rather than escaping as a raw filesystem error.\n\nConstraint: preserve the existing 1000ms timeout contract\nConfidence: high\nScope-risk: low\nReversibility: revert-safe\nTested: bun test packages/coding-agent/test/sdk-daemon-concurrency.test.ts; bun test packages/coding-agent/test/session-resident-lifecycle.test.ts; bun --cwd=packages/coding-agent run check
6c815f8 to
d445319
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d445319d33
ℹ️ 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".
| parsed.pid !== expected.pid || | ||
| parsed.incarnation !== expected.incarnation || | ||
| parsed.timestamp !== expected.timestamp || | ||
| parsed.nonce !== expected.nonce |
There was a problem hiding this comment.
If a stale lock contains a valid JSON object or array in nonce—for example, from corruption or an incompatible writer—isConversationStoreLock accepts it, but each read creates a distinct object reference, so this comparison always fails. #unlinkOwnedLock then refuses to remove the stale lock and every subsequent conversation mutation times out; validate nonce as either absent or a string before treating the metadata as a ConversationStoreLock.
Useful? React with 👍 / 👎.
| fs.files.set(`${store.filePath}.lock`, leaked); | ||
| const started = performance.now(); | ||
| await expect(store.write("mapping", undefined, record(1))).resolves.toBe(true); | ||
| expect(performance.now() - started).toBeLessThan(900); |
There was a problem hiding this comment.
On a loaded or temporarily descheduled CI worker, the correct immediate-healing implementation can take more than 900 ms between these measurements and fail this test nondeterministically. The preceding resolved-write assertion already distinguishes healing from the timeout path, which rejects, so this redundant timing assertion should be removed or replaced with a deterministic injected-sleep observation.
AGENTS.md reference: AGENTS.md:L156-L160
Useful? React with 👍 / 👎.
A failed lock sync must retain ownership through cleanup so a concurrent same-process waiter cannot reclaim and lose a successor.\n\nConstraint: failed publication must not weaken live-lock safety\nConfidence: high\nScope-risk: low\nReversibility: revert-safe\nTested: bun test packages/coding-agent/test/sdk-daemon-concurrency.test.ts; bun --cwd=packages/coding-agent run check
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3094c45341
ℹ️ 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 lock: ConversationStoreLock = { | ||
| pid: this.#pid, | ||
| incarnation: this.#pidIncarnation(this.#pid) ?? "unavailable", | ||
| timestamp: this.#clock(), | ||
| nonce: randomUUID(), | ||
| }; |
There was a problem hiding this comment.
If the injected pidIncarnation callback throws—or randomUUID() fails—after open("wx") succeeds, the exception occurs before the try block, so the handle is never closed and the newly created empty lock file is never removed. Later mutations then time out until unpublished-lock recovery becomes eligible after 30 seconds; on platforms that prevent unlinking open files, the leaked handle can keep the store blocked indefinitely. Include token construction in the guarded cleanup path.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes the shared notification
ConversationLockTimeoutregressions observed on current dev:DiscordNotificationDaemon fake-provider acceptance > retries definite pre-send SDK and binding failures but preserves ambiguous sendsflaked in run 31387018881 (PR fix(notify): report a generation-skewed daemon as unhealthy #4153, shard-1) withConversationLockTimeoutError: Timed out waiting 1000ms for conversation store lock: …/effects.json.lock(1479ms test).SlackNotificationDaemon fake-provider acceptance > replays an ACK-boundary command receipt with its persisted idempotency keyflaked in run 31378224791 (PR refactor(sdk): make core own session lifecycle and attachments #4098, shard-2) with the identical 1000ms timeout on its own isolatedeffects.json.lock(1046ms test).Both rows timed out on an
effects.json.lockinside their own test process — no other process touched that tempdir.Root cause
The file-lock release in
ConversationStore(#withLock) is not idempotent and no waiter can recover a leaked lock whose recorded pid is still alive:finallyrunsclose()thenunlink(). Ifclose()throws, theunlinkis skipped entirely, leaving a well-formed lock file on disk whose recorded pid is the still-running test process.#isStaleLockonly reclaims dead pids, mismatched incarnations, or empty files older than 30s. A well-formed lock from the current (alive) process is never stale, so every later acquire waits the full 1000ms and throws — exactly the CI signature. Deterministically reproduced by planting a live-pid lock file: the write times out at 1001ms and the lock stays.The failure is shared infrastructure:
ConversationStorebacks both the conversation mappings and theChatEffectJournaleffects.jsonfor Discord and Slack alike.Fix (shared, in
conversation-store.ts)close()no longer skips theunlink; the unlink is ownership-checked (pid + incarnation + timestamp + a new per-acquirenonce) so it can only remove the exact lock this holder wrote — it can never delete a successor's lock.Verification
test/sdk-daemon-concurrency.test.ts:Notes
sdk-host.test.tsfailures in the same shard runs ("SDK host logs a bounded reason…" 15s/60s timeouts) are a separate, unrelated flake and are out of scope here.