Repository navigation
fix(update): release the restart lease before the service refresh (#5760) - #6404
Conversation
) The dashboard update worker held the ownership-mutation lease across the entire restart, including 'ocx service repair'. The service manager's 'ocx start' child is not a descendant of the worker — Task Scheduler, launchd and systemd spawn it with the stored registration environment — so it carries no delegated token, dies at the lease acquire deadline, and the repair's serving probe fails. Every service-installed dashboard update then fell through to an unmanaged detached start beside a dead supervisor — the same class bin/ocx.mjs fixed for failed npm updates. The restart callback now receives an UpdateRestartLeaseControl: the worker releases the lease immediately before the service refresh and re-runs the recorded-owner veto at the fallthrough boundary, so a claim landing during the unleased refresh stops the direct start instead of killing or replacing it. The job records the veto as succeeded/restarted:false. Regression coverage spawns a real supervised 'ocx start' against the held lease (dies with the lease error), then drives restartAfterUpdate through the service path post-fix (child binds and serves) and through the fallthrough boundary under a mid-refresh claim (no kill, no spawn). Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
✅ Deterministic PR hygiene checks passed. |
…lthrough (#5760) The previous seam released the lease for the service refresh but left the fallthrough's veto re-check, killAllOcxOnPort reclaim and pinned start unleased — a desktop claim landing in that window could be killed. reacquireForDirectStart takes the lease back (bounded; a still-claimed lease fails closed), re-arms the delegation token, then re-runs the veto under the held lease. Regression coverage: claim-in-window veto, lease re-held through reclaim/direct start with a live contender probe, and a real lease-holding child proving the bounded acquire fails closed. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…does The suite pinned the lock dir to home/.opencodex, which is only the Windows authority: USERPROFILE tracks the sandbox home there. On POSIX homedir() ignores $HOME and the armed test-home guard drops that legacy entry from serviceStatePaths(), so the lease binds to the OPENCODEX_HOME record instead — existsSync(lockDir) was false at every step and the spawned child's contention was never the path the test probed. Resolve the authority with serviceStatePaths().at(-1) and mirror leasePath()'s realpath fallbacks, so both the lock-dir assertions and the foreign-claimant probes address the path the module actually locks. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… watchdog The POSIX CI shard keeps this child alive past the watchdog; without its stdout the stuck point is invisible. Log the captured output before the watchdog throws. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
⏳ DRAFT
What to do
Review readiness checklist
0/0 boxes ticked. Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked. |
The Linux child bound and served under the held lease, so its serviceStatePaths() resolved a different authority — likely the legacy home entry that the armed guard did not drop in the spawned env. Log the candidates (os-homedir lock, env REAL_HOME, resolved paths) so the next CI run shows which path it used. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…90861312-restart-lease-release
…90861312-restart-lease-release
…a non-serving refresh
# Conflicts: # structure/ops/service-and-sidecars.md
|
Maintainer integration record (MAINTAINERS.md, dev-only)
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: e7a590533c
ℹ️ 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".
| // `service repair` re-activates a manager whose `ocx start` child is not a | ||
| // process descendant and cannot join this lease; held through its serving | ||
| // wait, the lease makes that child die at the acquire deadline (#5760). | ||
| io.releaseForServiceManagerFn?.(); |
There was a problem hiding this comment.
On macOS and Linux, releasing the ownership-mutation lease before spawning the entire service repair command creates a takeover race: repairService() reads ownership once at src/service/repair.ts:169, but a desktop claim can commit immediately afterward while the repair continues to rewrite, stop, and restart the service. If that refresh serves successfully, the early return at line 1234 skips the new reacquisition/veto entirely, so the updater can mutate a service that became desktop-owned during the released window. Keep the lease through the repair's ownership check and service mutations, releasing it only at the OS-manager activation boundary, then revalidate ownership before accepting success.
Useful? React with 👍 / 👎.
| manager children. A replacement refusal passes through owner-aware recovery: only the same CLI | ||
| owner revives the stopped runtime; foreign ownership stays transferred and unknown ownership | ||
| remains a reported recovery requirement. Dashboard restart delegates the lease token to its repair child. Direct start holds the same lease through bind plus PID and runtime-address publication. If listener rollback cannot prove the socket closed, the process retains its lease until exit. | ||
| remains a reported recovery requirement. Dashboard restart delegates the lease token to its `service repair` child, and releases the lease before that refresh: the service manager spawns `ocx start` outside the worker's process tree with no token, so the managed child must take the lease itself (#5760). Before the direct-start fallthrough kills or spawns on the port, the worker takes the lease back — a refresh that stayed claimed stops the restart — and re-runs the recorded-owner veto under it because a claim could have landed during the unleased refresh window. Direct start holds the same lease through bind plus PID and runtime-address publication. If listener rollback cannot prove the socket closed, the process retains its lease until exit. |
There was a problem hiding this comment.
This contract now says the dashboard delegates its lease token to service repair while also saying it releases the lease before spawning that refresh. The new releaseForServiceManager() path restores or removes OCX_OWNERSHIP_MUTATION_LEASE_TOKEN before runService, so the repair child is intentionally not delegated the current token. Describe the refresh as unleased instead; otherwise this architecture document gives maintainers the opposite concurrency model from the implementation.
AGENTS.md reference: structure/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
1 participant
Summary
ocx service repair. Task Scheduler, launchd, and systemd start the managed child outside the worker's process tree, so that child cannot inherit the worker's delegated token and otherwise times out acquiring the lease.succeededwithrestarted: false.Verification
Refreshed from
devat06cc3815c06a10120d89ef986c46be95be37f28awithout rewriting branch history, including the accepted upstream role-route and owner-registry ACL fixture corrections. Targeted verification: role routes 7 passed; owner-registry ACL 6 passed. Required CI passed ate572fde011a0116a2e8a205c10a167a59632372a: zero failed and zero pending checks (platform/path-inapplicable checks remain skipped).Initial regression reproduced the failure with a real token-less
bun src/cli/index.ts startchild on Windows, then verified successful health publication after release.Follow-up regressions exercise an installed-service refresh with a claim arriving during the released window, competing acquisition during fallback, and bounded failure when a claimant keeps the lease. All seven restart-lease tests passed on Windows after the correction.
Linux CI exposed mismatched parent/child fixture authority. The child now forwards the parent's captured protected home and uses that guarded home on POSIX, while Windows retains its sandbox home. The authority equality assertion and real CLI lease refusal remain enforced. A cancellable watchdog bounds exit and output draining. All seven focused tests and typecheck passed locally; the latest Linux shard and aggregate CI passed at
e572fde011a0116a2e8a205c10a167a59632372a.Initial focused update regressions: 126 passed. Follow-up typecheck, privacy scan, structure check, and focused update/service/CLI regressions passed.
Required CI is green. Native desktop takeover evidence for the separate Windows lifecycle PRs does not exercise this dashboard-update path.
Checklist
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.
Link to Devin session: https://app.devin.ai/sessions/a8795b27d4114aa6be37a88565e404b2
Open in Devin Desktop: https://app.devin.ai/desktop/session/a8795b27d4114aa6be37a88565e404b2?variant=devin
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.