[shu-chen-dot-ca]

Summary

a registration racing uninstall is either removed or refused, never left unreported assumed uninstall always gets the ownership lock. After the gate opens, the gated children may still hold the lock when removeOwnedConfigState runs. Uninstall then fails fast with refused ("could not lock the ownership manifest ... busy") before it writes its tombstone, so every child records its path afterwards. That outcome is correct, because the home stays intact. The test still expected each recorded file to be removed, which is the expected false, received true failure at the existsSync assertion seen in #8090's CI.

• The race assertions move into a helper, expectRaceOutcome, that checks both outcomes:
  • Uninstall got the lock: the existing checks are unchanged. A recorded child's file is removed, a refused or unrecorded child's file is reported as residual, and the lock file is gone.
  • Uninstall was refused: residualPaths is the home only, the manifest carries no uninstallStartedAt, every child exits 0, and every racing path is listed in the manifest and still exists. A rerun then returns { status: "removed", residualPaths: [] } and the home is gone.
• New test an uninstall refused for a busy lock leaves racing registrations recorded for a rerun forces the refused path. It calls uninstall while the test still holds the lock, right after opening the gate, asserts refused, and runs the same helper. Both tests share a spawnGatedChildren setup that is the existing barrier code, unchanged.
• Test-only change. src/ is untouched.

Closes #8379

Verification

All runs used Bun 1.4.4 on Linux (Ubuntu 24.04).

• Reproduction on unchanged dev 3372695: a copy of the original race test that calls uninstall before releasing the held lock fails the same way CI did:

  uninstall status: refused could not lock the ownership manifest: config ownership metadata is busy in another opencodex process
  Expected: false
  Received: true
  (fail) repro #8379 > a registration racing uninstall is either removed or refused, never left unreported
  

• With this change, bun test tests/config/config-ownership-concurrent-record.test.ts -t "racing|refused for a busy lock" passes both tests:

  (pass) serialized ownership manifest (#8093) > a registration racing uninstall is either removed or refused, never left unreported
  (pass) serialized ownership manifest (#8093) > an uninstall refused for a busy lock leaves racing registrations recorded for a rerun
   2 pass
   0 fail
  

  Run 25 times in a row: 25/25 passed.

• Current head 3d09cccf, rebased onto dev de0914a1:

  • bun test tests/config/config-ownership-concurrent-record.test.ts: 60 pass, 0 fail. This includes the two tests dev added to the file.
  • bun run typecheck && bun run structure:check && bun run privacy:scan && OCX_TEST_MAIN_TIMEOUT_MS=3600000 bun run test --parallel=4 --timeout 30000: exit 0.
  • One earlier full run on the same head failed only on tests/update/update-orchestrated-entry.test.ts (npm child fences identity, deadline and sync before mutation...), which timed out at its 90 s budget on a heavily shared host. That file passes on its own on this head (10 pass, 0 fail), and the full rerun above passed.

Checklist

• Scope stays focused and avoids unrelated cleanup.
• Docs or release notes were updated when needed. (none needed: test-only)
• Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. (none touched)

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

• Required local validation passed; commands, results, and any full-suite exception are documented.

• I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).

• I resolved all correct Codex and CodeRabbit findings.

• My PR is ready for review.

Summary by CodeRabbit

• Tests
  • Expanded coverage for concurrent configuration recording and removal, including attempts to uninstall while ownership is locked.
  • Verified that a blocked uninstall leaves files intact and that removal succeeds when retried after recording completes.

[github-actions]

✅ Deterministic PR hygiene checks passed.

[github-actions]

⏳ DRAFT

• review readiness checklist open (0/4 boxes ticked).

What to do

• Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

• ⬜ Required local validation passed; commands, results, and any full-suite exception are documented.
• ⬜ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
• ⬜ I resolved all correct Codex and CodeRabbit findings.
• ⬜ My PR is ready for review.

0/4 boxes ticked.

This PR stays in draft until every box above is ticked.

Hygiene

✅ Deterministic PR hygiene checks passed.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The concurrent config ownership test now shares its gated-child setup and race assertions. It covers uninstall refusal while the ownership lock is held and verifies that a later uninstall removes the home after child registrations finish.

Changes: Config ownership race coverage

Layer / File(s)	Summary
Gate child registrations and verify uninstall outcomes tests/config/config-ownership-concurrent-record.test.ts	A shared setup starts six gated child processes, and a helper checks successful and refused uninstall outcomes. A new case holds the lock during uninstall, verifies refusal without tombstoning, then checks that a rerun removes the home.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other · Severity of issue fixed: Low

Possibly related issues

• Clarify existing tombstones after a busy uninstall refusal #8386: This issue describes a related busy-uninstall registration case, but its stated scope is documentation.

Possibly related PRs

• lidge-jun/opencodex#8238: Adds the gated-child test mechanism reused by this pull request.
• lidge-jun/opencodex#8121: Defines the ownership and refused-uninstall behavior that this pull request tests.

Suggested labels: chore

Merge Risk: Minimal · up to c7324

This change only updates a test to accept a refused uninstall in the concurrent ownership race and adds a forced-refusal case. It has no production behavior impact, and no merge-blocking risk is evident.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title accurately describes the main test change: it updates the concurrent ownership race test to accept a refused uninstall result. It is specific, concise, and related to the changeset.
Linked Issues check		Issue [#8379] requires the concurrent ownership test to accept a refused uninstall when the lock remains busy. In tests/config/config-ownership-concurrent-record.test.ts, expectRaceOutcome handles…
Out of Scope Changes check		The whole-PR diff changes only tests/config/config-ownership-concurrent-record.test.ts. The new ConfigRemovalResult type import, expectRaceOutcome helper, shared spawnGatedChildren setup, and …
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.

Full details: Linked Issues check

Explanation

Issue [#8379] requires the concurrent ownership test to accept a refused uninstall when the lock remains busy. In tests/config/config-ownership-concurrent-record.test.ts, expectRaceOutcome handles result.status === "refused" by checking residual home state, no uninstallStartedAt, successful child registrations, retained files, and successful cleanup on rerun. The existing race test uses this helper, and the new busy-lock test forces and verifies the refused path. The changed file is test-only, which matches the issue scope.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff changes only tests/config/config-ownership-concurrent-record.test.ts. The new ConfigRemovalResult type import, expectRaceOutcome helper, shared spawnGatedChildren setup, and forced-refusal test directly support [#8379]. No unrelated source, documentation, or production behavior changes are present.

Finishing Touches

Generate unit tests (beta)

• 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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

Recent review info

Run configuration

• Source mode: Auto
• Repository: lidge-jun/opencodex
• Branch: dev
• File: .coderabbit.yaml
• Commit: 39fd819
• Layers: Repository YAML
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 4fdf6924-972f-4afd-b0b7-52941d214428

Commits

Reviewing files that changed from the base of the PR and between 39fd819 and c73242b.

Files selected for processing: 1

• tests/config/config-ownership-concurrent-record.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

[shu-chen-dot-ca]

Rebased onto the current dev (de0914a1); there were no conflicts and the diff is unchanged. On the rebased head, bun run typecheck, bun run structure:check, bun run privacy:scan and OCX_TEST_MAIN_TIMEOUT_MS=3600000 bun run test --parallel=4 --timeout 30000 all pass (exit 0). tests/config/config-ownership-concurrent-record.test.ts alone: 60 pass, 0 fail.