Skip to content

perf(runtime): consolidate lifecycle and media hot paths#622

Open
JustYannicc wants to merge 21 commits into
SuperCmdLabs:mainfrom
JustYannicc:codex/consolidate-runtime-lifecycle-final
Open

perf(runtime): consolidate lifecycle and media hot paths#622
JustYannicc wants to merge 21 commits into
SuperCmdLabs:mainfrom
JustYannicc:codex/consolidate-runtime-lifecycle-final

Conversation

@JustYannicc

@JustYannicc JustYannicc commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

What changed

  • Consolidates the selected runtime/lifecycle/media/native-helper performance PRs into one domain-scoped branch from origin/main.
  • Adds bounded AI stream parser buffers, async HTTP response decode, cloud media streaming, extension command refresh/build reuse, scoped extension lifecycle timers/listeners, guarded background refresh ticks, abortable extension HTTP/binary downloads, and native-helper readiness/buffer lifecycle hardening.
  • Adds standard regression tests under scripts/test-*.mjs for the consolidated behavior.

Why

  • The wave5 micro PRs depend on prior same-domain runtime consolidation that is not present on upstream main.
  • Consolidating the runtime/lifecycle/media/native-helper lane avoids a fragile stack of dependent PRs while keeping unrelated renderer/commands/search work out of scope.

Compatibility impact

  • Preserves Raycast runtime compatibility and existing extension API behavior.
  • Keeps URL-only binary download callers working while adding request-id cancellation support.
  • Keeps extension command build outputs compatible while reusing valid build stamps and cached refresh state.

How tested

  • PASS: direct standard regression command node --test 'scripts/test-*.mjs': 84 passed, 2 skipped.
  • Covered by standard npm test because package.json defines it as node --test 'scripts/test-*.mjs': scripts/test-ai-provider-stream-parser-bounds.mjs, scripts/test-background-refresh-inflight.mjs, scripts/test-cloud-media-buffering.mjs, scripts/test-extension-command-refresh-cache.mjs, scripts/test-extension-fetch-proxy-abort.mjs, scripts/test-extension-lifecycle-sandbox.mjs, scripts/test-extension-runner-multi-command-build.mjs, scripts/test-http-download-binary-cancel.mjs, scripts/test-http-response-decode-async.mjs, scripts/test-native-helper-process-lifecycle.mjs, scripts/test-registry-microtask-lifecycle.mjs.
  • PASS: node node_modules/typescript/bin/tsc -p tsconfig.main.json.
  • PASS: node node_modules/vite/bin/vite.js build.
  • PASS: git diff --check origin/main...HEAD.
  • PASS: codex-lsp diagnostics on touched TS/TSX files.
  • Local npm wrapper note: the wrapper did not reach the test script because npm is disabled on this machine and the pnpm install shim stopped on ignored build scripts; the direct command used by npm test passed.
  • Non-gating native-build note: node scripts/build-native.mjs could not resolve node-addon-api from this workspace install. No native source was touched by this consolidation.

Performance evidence

  • Standard regression/performance guard tests include parser buffer bounds, async decompression timer delay, media streaming, command refresh/build reuse, lifecycle cleanup, refresh inflight suppression, and cancellation cleanup.
  • Manual measurement harness, intentionally outside npm test: scripts/measure-extension-bundle-cache.mjs; local run reported repeated bundle-cache scenarios with 20 iterations in roughly 3.063ms and 2.73ms.

Stack validation

  • Branch: codex/consolidate-runtime-lifecycle-final, from current origin/main, pushed to JustYannicc.
  • Target: SuperCmdLabs/SuperCmd:main.
  • Diff scope inspected with git diff --stat origin/main...HEAD and git diff --name-status origin/main...HEAD; touched files stay in runtime/lifecycle/media/native-helper scope plus associated Raycast runtime hooks and proof tests.
  • Did not include broad unrelated renderer, commands/search, or integration-stack changes beyond the selected same-domain prerequisite consolidation and wave5 PRs.

Replaces

@shobhit99

Copy link
Copy Markdown
Contributor

Review: perf(runtime): consolidate lifecycle and media hot paths

Large consolidation (6.4k+ / 785-) folding several wave PRs (per the body, replaces #601/#603/#605/#606/#607/#609/#612/#614). Reviewed on its own merits. Most of it is solid leak-hardening and event-driven refactors, but I found one change that looks like an unintended and serious behavior inversion for extension require() of Node builtins.

Strengths

  • Native-helper readiness (native-helper-lifecycle.ts) replaces the 50ms setInterval polling with an event-driven markReady/timeout wait, and every kill*Server/exit handler now guards process === child to avoid a superseded restart clobbering a newer process. Genuinely better.
  • HTTP request/download cancellation + abort-listener cleanup (ai-provider.ts httpRequest/transcribeAudio, main.ts http-request/http-download-binary, ollama-pull finishPullRequest) consistently de-dupe settlement and remove abort listeners on end/close/error — real leak fixes.
  • spawn-process per-sender tracking with cleanup on render-process-gone/destroyed is a good backstop against orphaned child processes on renderer crash.
  • Registry microtask guards (action-/grid-/list-/menubar-runtime) add a mountedRef so queued queueMicrotask updates and post-unmount register/unregister calls no-op — prevents stale setState.
  • useBackgroundRefresh key-based reconcile + createInFlightBackgroundRefreshTick (skip overlapping ticks) is a clean improvement; cleanup-on-unmount effect is correct.
  • Streaming ElevenLabs TTS to disk via pipeline (instead of buffering the whole audio) with unlink-on-failure is a good media hot-path win.

Concerns / potential bugs

  1. 🔴 Builtin require priority inverted — extensions now get weak stubs instead of real Node. ExtensionView.tsx: the new shouldUseSuperCmdBuiltinFacade() gate runs before tryRealNodeRequire() and returns nodeBuiltinStubs[name] for any name in that map (fs, crypto, zlib, http, https, os, path, stream, util, net, tls, dns, …). Base behavior explicitly "Prefer real Node … better than returning a stub" (USE_REAL_NODE_BUILTINS = true), so this flips resolution from real → stub for all of them. That matters because several stubs are non-functional: cryptoStub.createHash(...).digest('hex') returns Math.random().toString(16) (a random string, not a hashExtensionView.tsx:1329-1333), createCipheriv/Decipheriv return empty buffers, and zlib.gzipSync/gunzipSync are identity no-ops (ExtensionView.tsx:2680-2688). Concrete failure: an extension doing crypto.createHash('sha256').update(data).digest('hex') (integrity/cache keys, HMAC API signing) silently gets garbage; zlib decompression returns raw bytes. The lifecycle goal only requires facading child_process (for spawn tracking) and timers (for scoping) — the gate should be narrowed to those, letting everything else fall through to tryRealNodeRequire as before. The added tests don't cover builtin fidelity, so this passes CI. Please confirm whether routing all builtins through stubs was intended; if not, this is a broad extension-compat regression.

  2. 🟡 Native-helper line buffer silently drops valid long lines. native-helper-lifecycle.ts appendNativeHelperLineBuffer: a complete line with line.length >= maxChars (256KB) is discarded (continue) rather than emitted. The genuine flood protection is the incomplete-buffer slice(-maxChars); dropping a completed newline-terminated line means a large-but-valid JSON response from a helper (e.g. a long qwen3/transcription result serialized on one line) is thrown away and the pending request hangs until timeout instead of resolving. Consider only bounding the incomplete tail and letting oversized complete lines through (or failing the pending request explicitly rather than dropping).

  3. 🟡 Scoped window/document Proxy is a broad semantic change riding in a "perf" PR. createExtensionLifecycleScope now hands extensions Proxy-wrapped window/document (previously raw globalThis). The isolation/cleanup intent is good and getOwnPropertyDescriptor forcing configurable:true is the standard invariant workaround, but this changes identity/descriptor semantics for every extension and is hard to bisect within a 6k-line diff. Worth calling out as the main regression-surface risk; heavier real-extension smoke testing (not just the sandbox unit tests) would de-risk it before merge.

Suggestions

  • Given the size, consider splitting the window/document scoping change out from the pure leak/cancellation fixes so a bad interaction can be reverted independently.

Verdict: 🔴 Needs changes before merge — concern #1 (crypto/zlib/etc. stub inversion) should be resolved or explicitly justified; #2/#3 are worth addressing.

🤖 Automated review by Claude Code (Opus 4.8).

@JustYannicc

JustYannicc commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the review feedback in f85bee3.

What changed:

  • Builtin require() priority: narrowed the SuperCmd builtin facade gate to child_process only. Timer modules still use the scoped lifecycle facade path, while crypto, zlib, fs, etc. now fall through to real Node before compatibility stubs. Added scripts/test-extension-builtin-require-priority.mjs to lock this in.
  • Native-helper long lines: appendNativeHelperLineBuffer no longer drops newline-terminated complete lines based on length; only the incomplete trailing buffer remains capped. Updated native-helper lifecycle coverage with an oversized complete JSON line case.
  • Window/document proxy scoping: kept the lifecycle scoping change, added commentary in createScopedHostProxy, and expanded sandbox coverage for scoped identity, window/self/globalThis, document.defaultView, descriptor invariants, enumeration, write-through, host method binding, and cleanup.

Verification:

  • node --test scripts/test-extension-builtin-require-priority.mjs scripts/test-native-helper-process-lifecycle.mjs scripts/test-extension-lifecycle-sandbox.mjs ✅ 21 passed
  • npm run build:main
  • git diff --check

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.

2 participants