perf(desktop): share cache state and eliminate duplicate cold hydration - #972
perf(desktop): share cache state and eliminate duplicate cold hydration#972ozymandiashh wants to merge 2 commits into
Conversation
avs-io
left a comment
There was a problem hiding this comment.
Thanks for the careful work here. The performance objective is valuable, and several parts of this are strong: removing the artificial warm-up, routing the first real request through the resident child, forwarding progress safely, restarting after mutations, and the breadth of the lifecycle tests.
I do need to request changes on exact head 44f2dca4dcc577aa6f4d4b6279ce3f26b16c4cec. The happy paths are well covered, but the following production-shaped lifecycle and cache-identity invariants remain open.
Blocking findings
-
Electron shutdown can still admit a new child after
shutdownAll()has finished reaping work.spawnCliandspawnCliActioncheckshuttingDownonly before awaiting a scheduler slot (app/electron/cli.ts:617,659and:676,683).pumpSlots()removes a waiter and incrementsrunningsynchronously, but its continuation resumes on a later microtask. If shutdown lands in that gap,reapAll()sees neither a queued waiter nor an active child; the continuation then callsrunCli/runActionafter terminal shutdown.I reproduced both paths: an admitted read ran after
shutdownAll()and returnedbad-jsoninstead of cancellation, while an admitted action returnedok: true. Please re-check shutdown after slot acquisition and add regressions for work admitted-before-spawn on both paths. -
Resident failure fallback bypasses the global two-child limiter.
The serve error path calls
runClidirectly atapp/electron/cli.ts:643-649; it never acquires a scheduler slot. When a resident dies with several requests pending, their rejection handlers can all start cold one-shot parses together. A six-request repro started all six fallback children within 2 ms, reopening the CPU/RSS stampede this PR is intended to eliminate.Please route fallbacks through the same scheduler and test simultaneous resident-request failure against the global concurrency cap.
-
The new cache-directory resolver is call-time dynamic, but the Codex and Antigravity in-memory caches are not keyed by directory.
src/codex-cache.ts:38-51,115-141andsrc/providers/antigravity.ts:163-180,323-371retain one module-global cache while resolving the destination path afresh. SwitchingCODEBURN_CACHE_DIRfrom A to B in one process returned A’s cached data under B ({"leaked":["secret-from-a"],"bExists":false}); a later dirty flush can then persist that object into B.Either pin the cache directory for the process lifetime or key all memo/dirty state by the resolved directory. Please cover A → B reads and writes for both providers.
-
Electron’s five-second read cache defeats immediate same-argv configuration freshness.
app/electron/cli.ts:623-625returns a cached argv result before consulting the resident process or any configuration fingerprint. Electron-originated mutations clear this cache, but an external CLI edit or directconfig.jsonchange can leave the same panel stale for five seconds. That does not meet #971’s “same-argv serve output reflects config changes immediately” criterion.Please make this cache configuration-aware, or remove it now that resident output memoization performs the coalescing, and add an external-config-change regression through
spawnCli. -
The resident output memo invalidates for
config.json, but the parser’s exact-key memo does not include config-derived Claude roots.src/parser.ts:3240-3251keys discovery only on the date range, provider filter, andCLAUDE_CONFIG_DIRS/CLAUDE_CONFIG_DIRenvironment values.claudeConfigDirsfromconfig.jsonis absent. After changing the configured roots from A to A+B with identical argv, the serve layer correctly regenerated output but the parser reused the old A-only parse; the second root remained missing.Please include the effective config-derived discovery roots, or an equivalent discovery fingerprint, in parser reuse and pin A → A+B on a long-lived serve process.
-
Canceling a stuck macOS request can permanently occupy the serialized resident queue.
ServeConnection.sendraces the response against a timeout in a task group (mac/Sources/CodeBurnMenubar/Data/ServeConnection.swift:131-155). Parent cancellation cancels the timeout task, whilecancelPendingRequestdeliberately leaves the child running for caller cancellation (:180-206). If the command never returns, no timeout owner remains to terminate it, andsrc/serve.ts:212-219keeps every later request queued behind that abandoned command.This is reachable because the UI watchdogs cancel refreshes after 60–90 seconds while a cold resident request is allowed ten minutes. The current cancellation tests release their finite fake and do not exercise a never-returning command. Please preserve an independent child timeout after caller cancellation, or terminate/replace the resident safely, and prove a canceled never-returning request cannot block the next request.
Cache-path contract
There is also a literal mismatch with #971’s acceptance criterion that all CodeBurn-owned cache files use one default/override contract. src/sync/ledger.ts:19-29 still selects XDG_CACHE_HOME/codeburn when CODEBURN_CACHE_DIR is unset, while the other caches use the centralized resolver. I understand the migration concern, but the result is still two active write roots. Please either migrate/preserve the ledger without retaining a second active contract, or explicitly revise the issue/PR contract before claiming closure.
Validation
The positive validation is substantial: Electron 38 files / 482 tests, Electron CLI 54/54, focused root 67/67, lock suites 26/26, TypeScript checks, and both production builds passed on this head. The full root run was effectively green after replacing a shared node_modules symlink with a clean install. The changed Swift sources and tests parse; full SwiftPM execution is unavailable on this machine because the installed CommandLineTools fail to link PackageDescription, which I am not attributing to this PR. The merge tree against current main is clean.
These findings are narrowly about terminal shutdown, bounded fallback concurrency, cache identity, immediate freshness, and cancellation progress. Once each has an end-to-end regression and the exact head is updated, I’ll be happy to re-review promptly.
|
One concise follow-up from the completed independent adversarial pass. I re-ran each case against the same exact head (
These are additive to the submitted changes-requested review. The first is the most urgent because it crosses the documented read-only safety boundary; the others preserve mutation freshness, process lifecycle, and the one-shot resource ceiling. Happy to re-run the repros on the next head. |
44f2dca to
a95a2c5
Compare
Fixes #971.
Summary
CODEBURN_CACHE_DIR, otherwise~/.cache/codeburn), with one-time adoption of the historical XDG sync ledgerserve --stdiochild, eliminating the artificial warm-up and duplicate cold parseThis remains one PR because cache identity, resident invalidation, and GUI process lifecycle jointly determine whether startup is both single-flight and fresh.
Review fixes
The follow-up pass addresses the requested lifecycle and cache-identity findings:
clean/dirty/unknownwatcher state: dirty always invalidates, unknown keeps only ordinary TTL/burst reuse, and clean enables bounded extended reuseTwo conservative choices are intentional: a time-only settled Electron cache is not restored because it violates next-call external-config freshness, and only a successful full
statusmarks a resident generation warm because narrower reads do not prove that a later all-provider status is hydrated.Measured behavior
Measured locally on one ~142 MB power-user cache:
These are local corpus measurements, not a universal performance claim. The native menubar QoS correction is behavior-tested; no numeric menubar A/B speedup is claimed.
Validation
Exact published source diff SHA-256:
36d610e395d8bda942ca46edc6c18f1454184725a45b98c4a44c7fd73c784567.BUILD VERDICT: PASSclaude-opus-5, effort max, read-only, exact diff hash above —VERDICT: PASSa95a2c5: 6/6 passed (appx,assess,check,semgrep,snap,test)The first root full-suite run had one 10-second
cli-plantimeout under load; the exact test passed in 1.85 seconds and the complete rerun passed with the counts above.Compatibility
CODEBURN_CACHE_DIRremains authoritativeCODEBURN_CACHE_DIRmay resend the bounded historical sync window once because the prior default ledger is not scanned outside the selected cache root; span IDs remain deterministic