Retry a lock sibling whose deletion has begun - #368
Merged
Conversation
CI went red on main with a Windows-only failure in flock_unsupported_serializes_through_exclusive_create: Access is denied. (os error 5) Windows reports a file that is unlinked but still has a handle closing as ERROR_ACCESS_DENIED, not ERROR_FILE_EXISTS. A waiter that calls create_new while the holder is releasing therefore sees a name that is neither takeable nor gone, and try_exclusive_create turned that into a hard failure rather than one more retry. In production that means a release can hand the next writer an error instead of the lock. The create is now retried for up to a second, which is orders of magnitude longer than the state takes to clear and short enough that a genuine permission problem still reports its own error rather than waiting out the ten-second acquire timeout. The retry is compiled on every platform and enabled only where the state exists, so it is type- and lint-checked off Windows too. The regression test runs the release-to-acquire handoff 25 times to widen the window the flake needed.
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
ceiling | c3323d3 | Commit Preview URL Branch Preview URL |
Aug 22 2026, 06:09 PM |
btsouth
enabled auto-merge (squash)
August 22, 2026 18:09
|
Warning Review limit reached
Next review available in: 4 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c3323d3. Configure here.
btsouth
added a commit
that referenced
this pull request
Aug 22, 2026
Two unrelated flakes have each failed PRs today that did not touch them. ## 1. `TrayPanel > keeps a counted star ask visible when the tray is empty` Failed on #367 and #355 with: ``` TypeError: Cannot read properties of undefined (reading 'then') The above error occurred in the <TrayPanel> component ``` `afterEach` calls `vi.restoreAllMocks()`, which resets every mock to the implementation it was **created** with — and for a bare `vi.fn()` that is "return undefined". `LocaleProvider` does `listen(...).then(...)`, so any call landing outside the `beforeEach`→test window throws. It is always this test because it is the only one that sets `starPromptMocks.reason`, so it is the only one that renders the prompt and takes the slower path. `eventMocks.listen` is now declared as `vi.fn(() => Promise.resolve(() => {}))`, so a restore leaves it harmless. `beforeEach` still installs the listener-capturing version the tests drive. **Proof** — same file, forcing `eventMocks.listen.mockReset()` before the render: | `listen` declared as | Result | |---|---| | `vi.fn()` (before) | `1 failed \| 24 passed` — the exact CI error | | `vi.fn(() => Promise.resolve(…))` (after) | `25 passed` | ## 2. `an_unenforceable_lock_fails_closed_instead_of_writing_unserialized` Failed on #355. **This one is my regression from #368.** That PR added a retry for Windows' delete-pending state, which reports access-denied. A *directory* planted at the sibling path — which is exactly what this test does — is also access-denied on Windows, but it never clears. The retry sat on it for the full second and broke the test's sub-second bound. The retry now skips a directory, the grace drops from 1s to 250ms (still orders of magnitude longer than delete-pending needs), and the test asserts against `DELETE_PENDING_GRACE` rather than a bare literal so the two cannot drift. ## Verified Frontend: **88 files, 707 tests passing.** Rust: 26 `secure_file` tests, clippy `-D warnings` clean. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Test-only mock change plus a narrower Windows lock retry that fails faster on an unfixable directory sibling. Does not change the successful lock or write path. > > **Overview** > Stops two unrelated CI flakes: a Vitest mock restore that made `listen()` return `undefined`, and a Windows exclusive-create retry that sat on a directory for the full grace period. > > `eventMocks.listen` is now created as a function that resolves to an unsubscribe, so `vi.restoreAllMocks()` in `afterEach` no longer breaks `LocaleProvider`'s `listen(...).then(...)` outside the `beforeEach` window. > > On the lock path, `try_exclusive_create` no longer retries `PermissionDenied` when the sibling is a directory (that never clears on Windows). `DELETE_PENDING_GRACE` drops from 1s to 250ms, and the fail-closed test asserts against that constant so it cannot drift. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 383d3b1. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- Macroscope's pull request summary starts here --> <!-- Macroscope will only edit the content between these invisible markers, and the markers themselves will not be visible in the GitHub rendered markdown. --> <!-- If you delete either of the start / end markers from your PR's description, Macroscope will append its summary at the bottom of the description. --> > [!NOTE] > ### Fix CI flakes in `TrayPanel` test mock and `secure_file` lock retry logic > - Changes the `eventMocks.listen` mock in [TrayPanel.test.tsx](https://github.com/tsouth89/ceiling/pull/369/files#diff-d28e96cb56241c4ab16b385da508fb6832756fb3f5b99c0ef7c2098ee0386269) to return a resolved Promise to prevent errors when mocks are restored. > - Reduces `DELETE_PENDING_GRACE` from 1 second to 250 milliseconds in [secure_file.rs](https://github.com/tsouth89/ceiling/pull/369/files#diff-ad931c0d6c98745631dc7a8420f5f246909a8c5b9f80d9987e7f76df43b3e157). > - Updates `StateWriteLock.try_exclusive_create` to fail fast when the lock path is a directory instead of retrying until the deadline. > - Behavioral Change: `DELETE_PENDING_GRACE` reduction lowers the maximum wait time for exclusive-create retries on Windows. > > <!-- Macroscope's review summary starts here --> > > <sup><a href="https://app.macroscope.com">Macroscope</a> summarized 383d3b1.</sup> > <!-- Macroscope's review summary ends here --> > <!-- Macroscope's pull request summary ends here -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
mainis red.Rust / sharedon8f4868e5failed:It is a flake, not a regression from that commit — #359 touched
usage_index.rsonly, and main was green on380db89cimmediately before. But the underlying bug is real and not test-only.Windows reports a file that has been unlinked while a handle is still closing as
ERROR_ACCESS_DENIED, notERROR_FILE_EXISTS. A waiter callingcreate_newwhile the holder is releasing therefore sees a name that is neither takeable nor already-taken, andtry_exclusive_createmapped that to a hardFailed. In production that means a release can hand the next writer an error instead of the lock.What
try_exclusive_createretries aPermissionDeniedcreate for up to one second — orders of magnitude longer than delete-pending takes to clear, and short enough that a genuine permission problem still reports its own error instead of waiting out the 10s acquire timeout.const RETRIES_DELETE_PENDING: bool = cfg!(windows)rather than#[cfg(windows)], so the branch is compiled and lint-checked on Linux too. I can't build the MSVC target here, so this keeps the Windows-only path from going unchecked.Verified
Linux: 26
secure_filetests pass, clippy-D warningsclean. The newreleasing_the_sibling_hands_the_lock_to_the_next_writerruns the handoff 25 times per invocation; 15/15 invocations clean.The Windows path itself can only be confirmed by CI on this PR.
Note
Medium Risk
Touches the cross-process lock that serializes settings and credential writes. The retry is short and Windows-only, but a misclassified permission error could delay a real failure by up to a second.
Overview
On Windows, releasing the flock-fallback sibling lock can fail the next writer instead of handing the lock over.
create_newracing an unlink that still has a handle closing returnsERROR_ACCESS_DENIED, which was treated as a hard failure.try_exclusive_createnow retriesPermissionDeniedfor up to one second, gated bycfg!(windows)so the path still compiles on Unix. A 25-round handoff test covers the race, and the changelog notes that a waiter in that window gets the lock rather than os error 5.Reviewed by Cursor Bugbot for commit c3323d3. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Retry
try_exclusive_createon Windows when sibling is delete-pendingOn Windows, deleting the exclusive-create sibling can leave a brief window where the next writer gets
PermissionDenied. The lock helper now retries for up to 1 second (DELETE_PENDING_GRACE) before returning an error, using a small sleep between attempts.try_exclusive_create_onceand wraps it in a retry loop gated byRETRIES_DELETE_PENDING(cfg!(windows)).releasing_the_sibling_hands_the_lock_to_the_next_writer) that runs 25 rounds of lock handoff to cover the delete-pending race.Macroscope summarized c3323d3.