You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(config): revalidate ownership before honoring a spent publication wait - #8422
Follow-up fix for a review finding on #8270 (merged as 25a1bfc): a spent publication wait could be cached under the identity of a completed owner/manifest pair, after which every registration matched the record and returned unowned without reloading. The bounded creation timeout became a persistent registration failure, leaving later paths outside uninstall tracking.
On an expired-wait identity match, run one plain loadOwnership() before refusing: the record suppresses the bounded wait, never the strict reload.
Only record a spent wait when the observed identity stayed unchanged across the wait, so the record always names the state the last failed read saw; a pair that changed during the wait spends its own wait on the next registration.
Tests
a spent wait recorded on a completed pair still reloads and records: child process spends the wait on an invalid pair, completes it under an identical stat identity, then must register instead of sticking unowned (fails as unowned without the fix).
a pair changed during the publication wait spends its own wait: a mid-wait pair change is not marked spent; the next registration pays one wait, then suppression resumes.
bun run typecheck clean; the two new tests pass on the fix and fail on the unfixed code.
Note: in this elevated Windows shell, 21 unrelated tests in the same file fail identically on the base commit with ELEVATED_UNINSTALL_REFUSAL (uninstall refuses elevated runs); all ownership-registration paths this change touches pass.
Summary by CodeRabbit
Bug Fixes
Improved configuration ownership registration when ownership metadata is being created or updated concurrently. Registration now reloads ownership after a wait expires, and changes to the metadata during that wait receive a fresh wait before registration proceeds.
Prevented homes from being incorrectly treated as unowned when valid ownership metadata becomes available at the end of a wait.
… wait
A spent publication wait was remembered under the identity sampled after
loadOrCreateOwnership() returned. Another process completing the owner
marker between the wait's last failed read and that sampling stored the
completed pair's identity, so every later registration matched it and
returned unowned without reloading - the bounded timeout became a
persistent registration failure.
- On an identity match, run one plain loadOwnership() before refusing:
the record suppresses the wait, never the reload.
- Only record a spent wait when the identity stayed unchanged across it,
so it always names the state the last failed read actually saw; a pair
that changed during the wait spends its own wait next registration.
Reported by chatgpt-codex-connector review on #8270.
initializeConfigOwnership now reloads ownership after an expired wait and records that wait only when the metadata identity remains unchanged. New subprocess tests cover unchanged and changed metadata identities.
Changes: Ownership wait revalidation
Layer / File(s)
Summary
Revalidate ownership after wait src/lib/config-ownership.ts
After an unchanged metadata identity completes its wait, initializeConfigOwnership performs a plain ownership load. It clears the expired-wait marker if the load succeeds and records the marker only if the identity remains unchanged.
Test publication-wait identities tests/config/config-ownership-concurrent-record.test.ts
Subprocess helpers exercise ownership-marker publication races. Tests check registration when valid metadata returns at the spent identity and verify that changed metadata incurs another wait before a later initialization reuses the spent-wait record.
lidge-jun/opencodex#8121: Changes registration and metadata revalidation during the same ownership publication wait flow.
lidge-jun/opencodex#8270: Changes the first-registration publication wait and tests incomplete owner metadata in the same files.
Merge Risk: Minimal · up to cabf9
No demonstrated issue blocks merging this ownership-wait fix after normal checks.
Pre-merge checks: 5
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly describes the main change: revalidate ownership before honoring an expired publication wait.
Docstring Coverage
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
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: Docstring Coverage
Explanation
Docstring coverage is 100.00% which is sufficient. 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 1 functions across 2 files.
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.
The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins.
119-145: Trivial · Maintainability & Code Quality · Low value
Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.
Test depends on the exact count of Date.now reads, which makes it fragile.
Line 135 writes the owner marker when reads === 3. This count matches the current loadOrCreateOwnership loop (the deadline read, then one read per iteration). The test fails if a later change adds or removes a Date.now call on the initializeConfigOwnership path. Examples are a change to createOwnership or to the lock helper. Such a failure would look like a regression in the revalidation logic.
Also, the firstMs >= 90 and secondMs >= 90 checks at lines 983 and 986 use wall-clock time. The wait is 100 ms and Bun.sleepSync has a lower bound, so these checks should be stable. The thirdMs < secondMs check is safe only because the third call skips the wait.
Consider a more robust trigger. For example, write the marker on the first Date.now read made after the first Bun.sleepSync iteration. Alternatively, add a short comment that ties the number 3 to the source loop, so a maintainer knows to update it.
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
Integrate at cabf95195b43471e031df604e479112b9c7eaf27, subject to an unchanged head and mergeability. This fixes a real ownership-registration race: a completed owner/manifest pair could remain treated as unowned after an earlier publication wait expired, leaving later files outside uninstall tracking. The change reloads the pair without repeating a spent wait and records a spent identity only when it stayed stable during the wait.
There is no GUI or CLI presentation change and no config migration. Existing homes can now register a valid completed pair; invalid, foreign and tombstoned pairs must continue to fail closed. The change uses existing filesystem APIs and path checks. Windows and macOS CI shards did not execute on this head; stat identity and junction behavior on Windows remain unverified.
Fresh independent code and separate security reviews passed this exact head. The security review reports negative probes for tombstoned, foreign-root, symlinked and unsafe-path cases. The code review reports head/base regression tests, focused ownership suites and typecheck; its full local run reported 17 failures, reproduced by that reviewer on clean dev. I checked the diff and reviewed the owned structure contract: it already states later re-reads and a wait of up to 100 ms; explicitly documenting the spent-wait optimization would be clearer but is not required to resolve a contradictory contract.
CI run 42560377866 is successful on the exact head, including gates, test changed and the aggregate. No change-caused CI failure is known.
NOT RUN by maintainer: local tests, full suite, Windows/macOS shards and independent reproduction of the reviewer's probes and base failures. The security review did not run privacy:scan; the code reviewer reports it passing.
Maintainer integration decision: use the dev-only integration policy if GitHub blocks only on the approval requirement; that decision does not replace the independent and security reviews.
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
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
Follow-up fix for a review finding on #8270 (merged as 25a1bfc): a spent publication wait could be cached under the identity of a completed owner/manifest pair, after which every registration matched the record and returned
unownedwithout reloading. The bounded creation timeout became a persistent registration failure, leaving later paths outside uninstall tracking.loadOwnership()before refusing: the record suppresses the bounded wait, never the strict reload.Tests
a spent wait recorded on a completed pair still reloads and records: child process spends the wait on an invalid pair, completes it under an identical stat identity, then must register instead of stickingunowned(fails asunownedwithout the fix).a pair changed during the publication wait spends its own wait: a mid-wait pair change is not marked spent; the next registration pays one wait, then suppression resumes.bun run typecheckclean; the two new tests pass on the fix and fail on the unfixed code.Note: in this elevated Windows shell, 21 unrelated tests in the same file fail identically on the base commit with
ELEVATED_UNINSTALL_REFUSAL(uninstall refuses elevated runs); all ownership-registration paths this change touches pass.Summary by CodeRabbit