Skip to content

chore(runtime): add profiling contracts and audit gates - #568

Open
beruro wants to merge 1 commit into
developfrom
junyu/runtime-profiling-tooling
Open

chore(runtime): add profiling contracts and audit gates#568
beruro wants to merge 1 commit into
developfrom
junyu/runtime-profiling-tooling

Conversation

@beruro

@beruro beruro commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Problem

Runtime performance work lacks reproducible macOS/Rust memory profiling entry points, explicit heap-profiler lifecycle ownership, and validation that RPC/transport wire contracts remain measurable and type-safe.

Solution

  • Add opt-in macOS memory and Rust DHAT profiling scripts and documentation.
  • Feature-gate allocator/lifecycle wiring so production behavior is unchanged unless profiling is enabled.
  • Add explicit RPC output-validation modes with a bounded drift buffer.
  • Generate and test transport/session-persistence TypeScript bindings from Rust wire types.
  • Validation: RPC Vitest, TypeScript typecheck/ESLint, profiling script syntax, transport/session-persistence tests, DHAT-enabled Rust check, rustfmt, and diff check.

Potential risks

  • Feature-gated profiling paths still require real Tauri measurement; code-shape and compile checks alone do not prove memory improvement.
  • Generated bindings must stay synchronized with serde wire output.
  • Enabling DHAT changes allocator/runtime behavior and is intended only for explicit profiling builds.

@Harry19081

Copy link
Copy Markdown
Member

Blocker 1 — package.json is broken and carries another branch's work
"check:component-boundaries": "node scripts/quality/check-component-boundaries.mjs"
I verified that file 404s on both develop and the head branch junyu/runtime-profiling-tooling. scripts/quality/ contains only check-e2e-oauth-guards.mjs and check-missing-i18n-keys.mjs. So pnpm check:component-boundaries fails immediately.

Worse, the same hunk drops clsx and ali-react-table from dependencies — that's what most of the 472-line pnpm-lock.yaml churn is. No source file in this PR stops importing them. If anything still does, the build breaks; if nothing does, the removal belongs to whatever PR deleted those usages. Either way this is a chunk of a different branch that leaked in, and it has nothing to do with profiling.

Blocker 2 — the PR is mislabeled, and it hides a wire-protocol rewrite
The title is chore(runtime) and the body says "Runtime behavior is opt-in behind profiling features/scripts." That is not true of transport/src/adapters/. Unconditionally, in every build:

Eight event channels collapse into one. agent://session-created, agent://session-deleted, agent://dialog-turn-{started,completed,cancelled,failed}, agent://token-usage-updated, agent://session-state-changed all become a single agent://event carrying {type, payload}.
AgentEvent's serde tag gains rename_all = "camelCase", so discriminators change from SessionCreated to sessionCreated.
ToolEvent changes shape twice — the bespoke toolEvent nesting disappears and fields go tool_name → toolName, event_type → eventType.
The engineering rationale is sound (serde as the single source of truth, ts-rs-generated bindings that can't drift from hand-maintained json! remapping — that's a genuine improvement). The problem is that a reviewer reading "chore, profiling, opt-in" will not review this as a breaking IPC contract change, and it's buried at file 21 of 29.

Mitigating, and worth telling the author: I searched the repo for agent:// and it appears in exactly three files — adapters/mock.rs, adapters/tauri.rs, and transport_tests.rs. Nothing under src/ subscribes to any of these channels. So this transport layer looks like unwired scaffolding and the rename is probably harmless today. Confirm locally before trusting that, since code search won't catch a dynamically-built channel name:

rg -n "agent://" --hidden -g '!target' -g '!node_modules'
If that confirms zero consumers, the change is safe — but it should say so in its own PR description, not ride along in a chore commit.

What's genuinely good here
dhat_profiling.rs is careful work. Holding the profiler guard in a static because Tauri's run() exits the process instead of returning to main is a real insight most people get wrong; the delayed-start allocator that bypasses DHAT until the profiling window is a nice touch, as is the separate [profile.dhat] with debug = 1, strip = false so release artifacts keep their size.
invoke.ts output validation is the most valuable change in the PR. The off/warn/throw policy with defaults preserving existing behavior, plus the bounded __orgiiRpcOutputDrift ring buffer so E2E can assert drift without spying on console, is well-judged — and it comes with 107 lines of tests.
ts-rs bindings + TS_RS_LARGE_INT = "number" is the right call for this codebase, and the comment documents the 2^53 tradeoff and the per-field escape hatch.
Smaller things, not blockers
adapters/tauri.rs used bare json!, so it must have a use serde_json::json; — and no hunk in this PR removes it. That's now an unused import. Verify with cargo clippy -p transport; it'll fail CI if warnings are denied.
--all-features would enable dhat-heap and replace the global allocator in a release build. Worth a compile_error! guard on not(debug_assertions), or at least a note.
The three RpcPerformanceClosure.md files (190 lines across dated *-2026-07-29/ directories) are process artifacts, not code. Fine if that's your convention; noise if it isn't.
profile-macos-memory.sh is still ~110 lines longer than it needs to be, per the earlier review.
What I'd ask for
Split into three:

Revert the package.json + pnpm-lock.yaml hunk entirely (or land the missing check-component-boundaries.mjs in the PR that removed the clsx/ali-react-table usages).
Profiling + RPC validation + ts-rs bindings — this is coherent, well-tested, and genuinely opt-in. Ships as-is once the unused import is cleaned up.
Transport wire-protocol change on its own, titled as a breaking change, with the "zero current consumers" finding stated explicitly in the description.
If you're under time pressure and #1 is reverted, merging the rest is defensible — the risk is concentrated in a layer nothing consumes. But I wouldn't merge it under the current title, because the next person to git log this will not know the event protocol changed.

@beruro
beruro force-pushed the junyu/runtime-profiling-tooling branch 3 times, most recently from 9ec56d6 to 9e77f78 Compare July 30, 2026 08:05
@beruro
beruro force-pushed the junyu/runtime-profiling-tooling branch from 9e77f78 to f715ca8 Compare July 30, 2026 15:10
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