fix(scan): only invalidate and bump when the scan roots actually moved - #233
Conversation
![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: 7219dc0a2d
ℹ️ 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".
The release grew from one merged PR plus the lens to four, so the notes now cover #233, #234 and #235 alongside what was already written. Three additions carry weight beyond describing a change. **#234's downgrade consequence leads the "before you update" section**, ahead of the cold-start note. Upgrading loses nothing — the migration exists so a version bump does not reset everyone's history, and it preserves every sample. Downgrading afterwards does: an older build cannot read schema 4, quarantines the file, and starts from nothing. The distinction that matters to a reader is that usage totals survive because they are recomputed from logs, while pace readings are observations taken over time and cannot be. Weeks of them are what lets a window say "Historically" instead of falling back to an average. The laziness is stated for the same reason a silent behaviour always is: the conversion happens on the next write the app had its own reason to make, so a file still reading `"schemaVersion": 3` after updating is normal. Without that sentence it reads as a migration that did not run, and someone reports it. **#235's token-expiry limit is written as a deliberate trade-off**, not omitted and not softened. An additional account whose token expires is not refreshed in place; the card says so and asks the user to run `claude` under that config directory. The refresh path reloads, validates and saves against the main configuration directory throughout, so routing an additional account through it would overwrite the main account's stored credential and log the user out of it. A limit that is not named in the notes is reported as a bug, and the reader cannot tell a chosen failure from an unnoticed one. **#233 is described by what the user paid**, not by the mechanism: an ordinary launch rebuilt caches it already had. The measurement that says this is not the cold-start regression belongs in the PR, not in notes written for users. Both files stay overrides, so `release_notes.sh` ships them verbatim and DeepSeek is skipped on both surfaces — the two prompts are independent and non-deterministic, and a release this size is the wrong one to let them describe differently.
|
Sync: #234 (quota-history schema 3→4) and #235 (multi-account quota cards) both merged into #235 restructured setConfigDirs(configDirsJSON)
Task { @MainActor in
await AgentUsageThrottle.shared.invalidate()
RegistryChange.signal()
}
let result = setScanPaths(json)
Task { @MainActor in
let moved = recordAppliedAndReportChange(json, result: result)
guard moved else { completion?(result); return } // only gates the scan side now
DashboardModel.invalidateScanDerivedCaches()
...
}After an auto-merge, Not fixing this here — it's your PR and your call whether the quota side should share the |
`ClaudeExtraRoots.apply()` dropped every Swift-side scan-derived cache and
advanced the generation views key their reloads on — both unconditionally, on
every call.
`apply()` runs at launch (`AppDelegate:90`) and on every Settings save, not only
when the list changes, so "an apply ran" has never implied "something changed".
Every launch therefore invalidated the reopen snapshot, the union scan, the
hourly cache and the attributed series' rows, and moved the generation. A
popover already open when that landed also took `reloadForRootChange()` — a
forced, cache-bypassing full graph refresh — for a registry byte-identical to
the one already installed.
The AppDelegate site at `:387` was already value-gated on the persisted list;
this puts the same test where it belongs, on the value the setter actually
installed, so it also covers the launch call and any future caller.
`recordAppliedAndReportChange` is the seam: it records and reports whether the
installed registry moved, which is the fact both consequences were missing.
`recordApplied` keeps its signature for the callers that do not care.
Measured, and this is NOT the cold-start regression
---------------------------------------------------
Timed `usage_graph::run` — the computation the dashboard's "Loading usage…"
actually waits on — at `v1.13.3` and at `7323eda5`, same corpus (119 days),
same machine, three runs each, through a throwaway `#[ignore]` probe rather
than the app's own probes, which enter the provider credential path.
v1.13.3 3516 / 3087 / 3038 ms
main 4290 / 2810 / 2995 ms
The warm runs are the comparable ones and the scan did not get slower. The
engine is also cleared by inspection: the vendored revision moved this release
(`731a2dcc` → `fc2941eb`) but the diff is 92 insertions, 0 deletions, one new
function — the parser and scanner are untouched, so the graph computation is
the same code on both sides.
So this commit removes real waste on a path the user waits on, and it is not
established as the cause of what they reported. Said plainly rather than
implied, because a fix that lands next to a complaint invites being credited
with it.
Verification
------------
`make selftest`: 1186 assertions, 0 failures (1182 before).
CE-MOVED asserts all four states, because the second alone is satisfied by a
function that never reports a change: a differing registry reports one, the
SAME registry reported again does not — that is the launch case and the one
that was paying — a genuinely different registry still does, so the gate is a
comparison rather than a one-shot latch, and a failed setter does not, because
the registry still holds what it held.
#235 moved the quota throttle drop and the `RegistryChange.signal()` to run between the two setters in `install()`, which is the correct POSITION — a stalled scan probe must not delay the quota cards. But the wake was unconditional, and `install()` runs at launch and on every Settings save, so every launch dropped the quota throttle and woke every sleeping poller for an account list identical to the one already installed. `lastApplied` cannot catch the launch case: it is in-memory state that starts nil every process, and a comparison against nil always reports a change. `appliedConfigDirsKey` persists the last-installed account list across launches, which is what the comparison needs to be meaningful here. Verified: two mutation-checked assertions. * M3-o2: re-applying the SAME account list wakes no poller — removing the guard FAILs it. * CE-CONFIG-MOVED: installing a different list reports a change, the same list again does not, and a different one after that still does — always-return-true FAILs the second. M3-o still passes: a genuinely different list fires the wake before the scan probe returns, which is the property that test was built to assert. Co-authored-by: multi-account-quota-symmetry (peer session that identified the interaction after its #235 merge)
7219dc0 to
bc3106a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc3106ad1b
ℹ️ 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".
| // the registry that is now installed rather than the one it | ||
| // replaced — same reason the scan side records before its own | ||
| // invalidation, below. | ||
| guard recordAppliedConfigDirsAndReportChange(configDirsJSON) else { return } |
There was a problem hiding this comment.
When configured Claude accounts persist across a relaunch, this guard compares against the previous process's marker even though the Rust account registry starts empty. AppDelegate launches the asynchronous apply before starting TrayAnimator, so its first quota fetch can race and use only the primary account; if the persisted JSON matches, the epoch never advances, that stale payload passes the guard in TrayAnimator.startQuotaPolling, and the missing account can remain absent until the five-minute poll (or the popover's one-minute poll). Track whether the account registry has been installed in this process and always invalidate/signal after that first nonempty install.
AGENTS.md reference: AGENTS.md:L25-L25
Useful? React with 👍 / 👎.
The persisted `appliedConfigDirsJSON` comparison answers "does this list differ from what a PREVIOUS install recorded". On a process's FIRST install that is the wrong question: the Rust account registry is process memory and starts empty regardless of what an earlier process persisted, so a relaunch with an unchanged account list installs into an empty registry while the marker already says "unchanged". The consequence is not cosmetic. `AppDelegate` starts `TrayAnimator` right after launching the asynchronous `apply()`, and the tray's quota poll fetches immediately — it can read only the primary account while the install is still in flight. Nothing signalled `RegistryChange`, so the epoch guard in `startQuotaPolling` accepts that partial payload, and the loop's own recovery is `RegistryChange.sleep(upTo: 300, ...)` — which waits for exactly the signal that never came. A configured account could be absent from the gauge for five minutes despite the registry having held it since launch. `didInstallConfigDirsThisProcess` tracks whether this process has installed the account registry at all. The first non-empty install wakes pollers regardless of the persisted marker; every install after that is gated on a real change as before. Restricted to non-empty. Waking for an empty registry that stays empty corrects nothing and would reintroduce the per-launch cost this branch exists to remove, one exemption down. Verified: two mutation-checked assertions. * M3-o3 (relaunch with a matching persisted marker, first install of the process, must wake) — removing the `firstNonEmptyInstall` term FAILs it. * M3-o4 (first install with nothing configured must NOT wake) — dropping the non-empty qualifier FAILs it. M3-o and M3-o2 still pass: a genuinely different list wakes before the scan probe returns, and a repeat of an already-installed list within a process wakes nothing. Reported by Codex review on bc3106a.
…xception #233 grew during review. The note described only its scan-cache half. Registering the Claude directories also dropped the quota throttle and woke every sleeping poller, unconditionally and on every launch — so the cost was not only rebuilding a cache but spending provider requests against an endpoint that rate-limits. Both halves are now gated on the registered set actually differing, and the note says so. The exception is stated rather than omitted. The first registration after a launch still refreshes the quota cards even when the user's list is unchanged, because the engine starts each run with nothing registered — a real change from its point of view, and skipping it could leave the menu bar showing a reading taken before the extra account was known. A note that claimed the work happens "only when something changed" would be wrong about launch, which is the case users will actually observe. Also drops "scan-root list" for "your Claude directories": the internal name described the payload, not the thing the reader configured.
1 participant
ClaudeExtraRoots.apply()dropped every Swift-side scan-derived cache and advanced the generation views key their reloads on — both unconditionally, on every call.apply()runs at launch (AppDelegate:90) and on every Settings save, not only when the list changes. So "an apply ran" has never implied "something changed", and every launch invalidated the reopen snapshot, the union scan, the hourly cache and the attributed series' rows, then moved the generation. A popover already open when that landed additionally tookreloadForRootChange()— a forced, cache-bypassing full graph refresh — for a registry byte-identical to the one already installed.The AppDelegate site at
:387was already value-gated on the persisted list. This puts the same test where it belongs: on the value the setter actually installed, so it covers the launch call and any future caller too.Measured — and this is NOT the cold-start regression
Timed
usage_graph::run, the computation the dashboard's "Loading usage…" actually waits on, atv1.13.3and at7323eda5. Same corpus (119 days), same machine, three runs each, through a throwaway#[ignore]probe rather than the app's own probes (those enter the provider credential path).v1.13.3mainThe warm runs are the comparable ones: the scan did not get slower.
run 0differs by first-touch I/O and is n=1.The engine is cleared by inspection as well. The vendored revision moved this release (
731a2dcc→fc2941eb) but that diff is 92 insertions, 0 deletions, one new function — parser and scanner untouched, so the graph computation is the same code on both sides.So this PR removes real waste on a path the user waits on, and it is not established as the cause of the slow cold start that prompted the investigation. Stated rather than implied, because a fix landing next to a complaint invites being credited with it.
Verification
make selftest: 1186 assertions, 0 failures (1182 before).CE-MOVEDasserts all four states, because the second alone is satisfied by a function that never reports a change: