Skip to content

perf(desktop): share cache state and eliminate duplicate cold hydration - #972

Open
ozymandiashh wants to merge 2 commits into
getagentseal:mainfrom
ozymandiashh:perf/desktop-shared-cache
Open

perf(desktop): share cache state and eliminate duplicate cold hydration#972
ozymandiashh wants to merge 2 commits into
getagentseal:mainfrom
ozymandiashh:perf/desktop-shared-cache

Conversation

@ozymandiashh

@ozymandiashh ozymandiashh commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Fixes #971.

Summary

  • use one call-time CodeBurn cache-directory contract across TypeScript and the native menubar (CODEBURN_CACHE_DIR, otherwise ~/.cache/codeburn), with one-time adoption of the historical XDG sync ledger
  • route the first real Desktop/Menubar request through the resident serve --stdio child, eliminating the artificial warm-up and duplicate cold parse
  • stream cold-scan progress and invalidate resident output when config content, Claude discovery roots, or watched session inputs change
  • keep Electron and Swift resident lifecycles bounded across mutation, cancellation, timeout, overflow, crash, restart, and shutdown

This 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:

  • Electron rechecks terminal shutdown after scheduler admission, sends resident fallbacks through the global two-child limiter, fences in-flight reads across mutations, and retains only concurrent same-flight coalescing (no stale five-second result cache)
  • both desktop clients enforce a 16 MiB resident response budget; resource-policy failures do not repeat the cold request through a one-shot fallback
  • the stdio server uses a positive command/option allowlist, refuses mutation-capable optimize flags, forwards progress outside capture, and closes root watchers plus the parser validator on EOF/error
  • parser/output memo validation is anchored before input reads and uses clean / dirty / unknown watcher state: dirty always invalidates, unknown keeps only ordinary TTL/burst reuse, and clean enables bounded extended reuse
  • Codex and Antigravity state is isolated by resolved cache directory; Codex pins the call-time directory through the complete async parse transaction; effective Claude discovery roots participate in parser reuse identity
  • the sync ledger writes canonically, adopts and merges legacy XDG state safely, and reset deletes canonical and eligible legacy sources independently even on partial failure
  • Swift serializes protocol lifecycles independently of caller cancellation, discards orphan/stale-generation replies, starts timeouts only when admitted, and retries only eligible read-only status requests
  • a timed-out Swift generation is detached immediately so its successor can start, consumes exactly one death, receives SIGTERM then SIGKILL after a bounded grace, and is still reaped if app shutdown lands during that grace window

Two 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 status marks 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:

Electron startup metric Before median After median Change
Visible first result 14.493 s 8.595 s -40.7%
Aggregate heavy work 19.389 s 8.595 s -55.7%
Concurrent heavy parses 2 1 duplicate eliminated
Peak RSS 2,633 MiB 2,465 MiB -6.4%

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.

  • root Vitest: 199 files passed, 2 skipped; 2,652 tests passed, 5 skipped
  • lock suites: 26/26 passed
  • Electron: 38 files / 487 tests passed
  • root and app TypeScript checks passed
  • CLI and Electron production builds passed
  • Swift debug and release builds passed with strict concurrency and warnings-as-errors
  • manual Swift Testing runner: ServeConnection 20/20 passed
  • Hermes Agent builder: DeepSeek V4 Flash, reasoning max — BUILD VERDICT: PASS
  • Claude Code final adversarial gate: claude-opus-5, effort max, read-only, exact diff hash above — VERDICT: PASS
  • GitHub checks on published head a95a2c5: 6/6 passed (appx, assess, check, semgrep, snap, test)

The first root full-suite run had one 10-second cli-plan timeout under load; the exact test passed in 1.85 seconds and the complete rerun passed with the counts above.

Compatibility

@avs-io avs-io left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Electron shutdown can still admit a new child after shutdownAll() has finished reaping work.

    spawnCli and spawnCliAction check shuttingDown only before awaiting a scheduler slot (app/electron/cli.ts:617,659 and :676,683). pumpSlots() removes a waiter and increments running synchronously, 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 calls runCli/runAction after terminal shutdown.

    I reproduced both paths: an admitted read ran after shutdownAll() and returned bad-json instead of cancellation, while an admitted action returned ok: true. Please re-check shutdown after slot acquisition and add regressions for work admitted-before-spawn on both paths.

  2. Resident failure fallback bypasses the global two-child limiter.

    The serve error path calls runCli directly at app/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.

  3. 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-141 and src/providers/antigravity.ts:163-180,323-371 retain one module-global cache while resolving the destination path afresh. Switching CODEBURN_CACHE_DIR from 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.

  4. Electron’s five-second read cache defeats immediate same-argv configuration freshness.

    app/electron/cli.ts:623-625 returns 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 direct config.json change 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.

  5. 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-3251 keys discovery only on the date range, provider filter, and CLAUDE_CONFIG_DIRS/CLAUDE_CONFIG_DIR environment values. claudeConfigDirs from config.json is 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.

  6. Canceling a stuck macOS request can permanently occupy the serialized resident queue.

    ServeConnection.send races the response against a timeout in a task group (mac/Sources/CodeBurnMenubar/Data/ServeConnection.swift:131-155). Parent cancellation cancels the timeout task, while cancelPendingRequest deliberately leaves the child running for caller cancellation (:180-206). If the command never returns, no timeout owner remains to terminate it, and src/serve.ts:212-219 keeps 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.

@avs-io

avs-io commented Aug 12, 2026

Copy link
Copy Markdown
Member

One concise follow-up from the completed independent adversarial pass. I re-ran each case against the same exact head (44f2dca) and confirmed four additional blockers; I am omitting claims I could not substantiate.

  1. The resident read-only boundary admits a mutating command. src/serve.ts:42,53-58 allowlists optimize and validates only flag-shaped argv, so optimize --apply --yes reaches runOptimizeApply (src/main.ts:1805-1835). In an isolated home, a protocol request for optimize --apply --yes --only bash-output-cap --period today returned ok: true, appended the CodeBurn block to .zshrc, and created an applied action in actions/journal.jsonl. Please use a command-specific option allowlist and add a no-write/refused protocol regression for every served command with mutation-capable flags.

  2. A pre-mutation in-flight read can publish stale state after a successful mutation. Action completion clears readCache, but does not invalidate readInflight; the older flight can later repopulate the cache at app/electron/cli.ts:650/666. My repro started a slow alias-list read over state old, completed the mutation to new, then let the old read settle. The next identical read returned old with only one read spawn. Please generation-fence in-flight keys and cache publication, with a slow-read → mutation → immediate-refetch regression.

  3. Provider-root watchers keep the resident alive after stdin EOF. startRootWatchers() returns close() (src/serve.ts:143-170), but the handle is discarded and the parser reuse validator is never reset. With an existing watched Claude root, closing stdin left the resident alive after 1.5 seconds with no child processes. Also, startedAt is recorded before asynchronous probe/stat/watch setup, so the validator can backdate coverage to a period before watches were armed. Please retain/close the watcher set and reset validator state in finally, treating coverage as valid only after successful arming; add a natural-EOF process-exit test.

  4. Resident responses bypass the existing Electron output bound. One-shot runCli enforces 16 MiB, but ServeClient.onData grows its line buffer and decoded output without a cap (app/electron/cli.ts:463-491). The same fake command produced 16 MiB + 1 KiB: resident mode accepted all 16,778,240 bytes, while one-shot correctly rejected it as too-large. Please bound protocol frames/progress and decoded command output, terminate the exact generation on overflow, and test an oversized valid JSON frame plus a never-terminated line.

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.

@ozymandiashh
ozymandiashh force-pushed the perf/desktop-shared-cache branch from 44f2dca to a95a2c5 Compare August 12, 2026 17:31
@ozymandiashh
ozymandiashh requested a review from avs-io August 12, 2026 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf: make GUI cold startup single-flight and shared-cache invalidation coherent

2 participants