Repository navigation
Change daemon capacity live and retire the old unit on rename - #873
alexeyzimarev merged 17 commits into
Conversation
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The counting double and the sequenced double expose disjoint evidence: register calls on one, rejections on the other. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…791) The ack returns once the live value is set; the server learns through the same single-flighted re-register the vendor-CLI watcher uses, so a burst of puts publishes once with the newest value. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Comparing the live value with the default 5 made a profile of exactly 5 read as unset and let the profile override an explicit --max-agents 5. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The unit bakes the daemon name in as --name, so a rename is a reinstall under a new label; retiring the old one in the same transaction keeps the app to one verb. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Retire now runs before the install's own forward budget starts, and spends its own budget, so a slow retire can never starve the install's readiness poll. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Refuse a --retire value that sanitizes to the fallback daemon name, guard the engine against retiring the install's own target, and make the two command-level refusal tests assert on the real message instead of the launchd-only gate they were accidentally passing through. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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. |
PR Summary by QodoChange daemon capacity live and retire the old unit on rename
AI Description
Diagram
High-Level Assessment
Files changed (36)
|
![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: 032fcdc396
ℹ️ 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 (validatedDaemonPid(serviceId) is not null) { | ||
| Say(VerifyExit.ContendedToken); | ||
| return VerifyExit.Contended; |
There was a problem hiding this comment.
If a daemon under the new serviceId starts after this one-shot probe while RetireAsync waits for or removes the old unit, the code never enforces this collision check again. The subsequent replace matrix can classify that newly started daemon as the owner and clear or kill it because replace is true, after the old unit has already been deleted. Recheck for a validated target owner immediately after retirement and before any target mutation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in c8c62f8: the collision check is repeated right after the retire returns, and the replace matrix now takes a refuseLiveOwner flag (set whenever a retire was requested) that returns verify_contended at both points of destruction, the owning-label clear and the validated-owner kill, instead of taking the daemon over. Three tests pin the window during the retire, the owner seen by the pre-query, and a label-less live owner.
| if (retireServiceId is not null) { | ||
| if (retireServiceId == spec.ServiceId) | ||
| throw new ArgumentException("retireServiceId must differ from the service being installed"); | ||
|
|
||
| // A rename's target must be free: a live daemon under the new name is another daemon, | ||
| // not a stale unit for --replace to take over. No pre-query needed for this check. | ||
| if (validatedDaemonPid(serviceId) is not null) { | ||
| Say(VerifyExit.ContendedToken); | ||
| return VerifyExit.Contended; | ||
| } | ||
| // Retire spends its OWN forward budget — never the install's — so a late-but-successful | ||
| // retire can never starve the install's own readiness poll and strand the operator with | ||
| // no daemon at all (a retired unit is never restored on the install's own timeout). | ||
| var retireBy = time.GetUtcNow() + _forwardBudget; | ||
| if (await RetireAsync(retireServiceId, spec, retireBy) is { } retireExit) return retireExit; |
There was a problem hiding this comment.
Because this retirement block runs after RecoverLeftoverMarker, a request naming an unreadable or foreign-profile retired unit can return RetireRefused only after recovery has already deleted the new target's marker or uninstalled its matching residue. That violates the exit's stated guarantee that nothing is touched on retirement refusal and can leave the target changed despite the refusal. Validate and lock the retired unit before performing target recovery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Recovery here is scoped to residue this engine's own earlier transaction for the new id provably left behind (a marker whose fingerprint matches the plist on disk), and it runs identically for every install, retire or not. Running the retire validation ahead of it would mean holding the retired label's lock across recovery for no gain: the target ends in the same verified-safe absent state the refused transaction would have left it in. What was wrong is the exit's doc claim that nothing is touched; it now says nothing is written for the new id and that the id's own residue recovery has already run.
| var retireBy = time.GetUtcNow() + _forwardBudget; | ||
| if (await RetireAsync(retireServiceId, spec, retireBy) is { } retireExit) return retireExit; |
There was a problem hiding this comment.
The retirement deadline is calculated before RetireAsync spends up to LockWait acquiring the retired label's lock, so lock contention is deducted from the promised independent forward budget. With the defaults, a near-10-second lock wait leaves only about half of the 20-second retirement budget; the code can then uninstall the old unit but fail before confirming the stop. Establish the forward deadline after the retired lock is acquired.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in c8c62f8: the retire's deadline is now taken inside RetireAsync after its lock is acquired, so a lock wait no longer eats the budget; the AdvertisedBound note says a caller allows one lock wait plus one forward budget for it.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…-extend-desktop-app-settings-to-configure-the-daemon # Conflicts: # docs/CHANGES.md
Its own budget now starts only once its lock is held, and a collision under the new name is rechecked after the retire settles and inside the replace matrix, not just before the retire starts. Also rejects a missing/flag-like --retire value instead of throwing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Step 5's snippets and placement prose lagged the actual insertion point and the refuseLiveOwner matrix parameter. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 participant
Part of #791 — AI-2535
What & why
The desktop app is getting a Settings window to change the daemon's agent capacity and its name. Capacity is a live field the daemon reads per launch and the server overwrites on a repeat connect, so a new local-socket frame pair (
DaemonSettingsPut23 /DaemonSettingsAck81, capabilitysettings/1) applies it without a restart and republishes the registration. The name is the daemon's identity locally and on the server and is baked into the service unit, so a rename is a reinstall:install --replace --verify --retire <old-id>removes the old unit inside the same transaction, keeping the app to one verb. The profile'smax_agentsnow applies whenever--max-agentsis absent; comparing against the default 5 made an explicit 5 read as unset. The window itself is the next PR.Where to look
ServiceVerify.RetireAsync: the retired label's lock is taken before its plist is read, and the retire runs on its own budget ahead of the install's forward cutoff, so a rename can take one forward budget plus a lock wait longer than a plain replace. A retired unit is never restored, and an absent unit is a no-op, so a detached daemon under the old name is left running.Verification
Core, daemon and CLI unit suites green locally; the daemon suite's one failure is the codex vendored-pin skew on this machine (installed 0.154.0, pin 0.147.0).
dotnet publish -c Release 2>&1 | grep -E 'IL[23][01][0-9]{2}': empty. Real-socket tests cover a put changing the live cap, a second status frame reaching an existing subscriber, the next launch over the cap being refused, and exactly one re-register.