Skip to content

fix(perf): gateway startup benchmark reports n/a CPU/RSS on Windows#110253

Draft
paulcam206 wants to merge 1 commit into
openclaw:mainfrom
paulcam206:perf/windows-cpu-rss-probes
Draft

fix(perf): gateway startup benchmark reports n/a CPU/RSS on Windows#110253
paulcam206 wants to merge 1 commit into
openclaw:mainfrom
paulcam206:perf/windows-cpu-rss-probes

Conversation

@paulcam206

@paulcam206 paulcam206 commented Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

AI-assisted (GitHub Copilot CLI, Codex/gpt-5.6-sol autoreview). I understand what the code does; a sanitized session transcript is included below.

What Problem This Solves

Fixes an issue where developers profiling OpenClaw on Windows would see cpu=n/a cpuCore=n/a rss=n/a in the gateway startup benchmark (pnpm test:gateway:cpu-scenarios / scripts/bench-gateway-startup.ts). The CPU-per-core and RSS observations that the hot-CPU regression gate relies on were never captured on Windows, so the benchmark could not report — or gate on — startup CPU and memory there.

Why This Change Was Made

scripts/lib/gateway-bench-probes.ts implemented readProcessRssMb and readProcessTreeCpuMs only for Unix (via ps) and returned null on win32.

  • RSS: readProcessRssMb gains a Windows implementation via a CIM Win32_Process WorkingSetSize query (with PID validation). The Unix ps path is unchanged.
  • CPU: measured at the gateway's own ready boundary instead of via an external process query. The gateway already emits a memory.ready startup-trace snapshot; it now also reports process.cpuUsage() as cpuMs, and the bench reads memory.ready.cpuMs. This is exact and ready-anchored on every platform and retires the external ps/CIM CPU read from the startup bench, so post-ready work can no longer be charged to startup CPU. readProcessTreeCpuMs stays for the Unix-only restart bench.

This directly resolves the ClawSweeper/Codex review's [P1]: the earlier Windows approach read CPU via a synchronous CIM query that ran after readyAt, so post-ready CPU could inflate the ratio and falsely trip the hot-CPU gate. Reading a child-reported value fixed at the ready instant removes that skew.

Behavior notes:

  • The gateway's memory.ready / memory.post-ready / restart.ready / close startup-trace lines gain a cpuMs= field on all platforms (one cheap process.cpuUsage() call; additive diagnostics only).
  • During startup the gateway is a single OS process (plugins load in-process; worker threads share the PID; supervisors spawn after ready), so process.cpuUsage() equals the previous process-tree sum — verified empirically (no child processes at ready). On Unix this also slightly improves accuracy (exact ready anchor; no ps fork per sample).

User Impact

Developers and maintainers running the Windows gateway startup benchmark now get real cpuMs, cpuCoreRatio, and maxRssMb values instead of n/a, and the CPU value is anchored to the ready boundary on every platform, so the hot-CPU regression gate is accurate. No product runtime behavior changes beyond the additive cpuMs= trace field.

Evidence

AI-assisted; reviewers should inspect the code, tests, and CI. Highlights:

Before (Windows): [gateway-startup-bench] default run 1/1: ... cpu=n/a cpuCore=n/a rss=n/a

After — gateway emits ready-anchored CPU (Windows 11 build 26652, Node v24.18.0):

[gateway] startup trace: memory.ready rssMb=1015.3 ... cpuMs=52468.0 ...
=== parsed cpuMs: 52468.0
=== gateway child processes (ParentProcessId=9408): (none)

(~52.5 CPU-s over a ~55 s startup → cpuCoreRatio ≈ 0.95; no child processes at ready, so process.cpuUsage() == the old process-tree sum.)

  • New/updated cross-platform unit tests — test/scripts/gateway-bench-probes.test.ts (ps + Windows CIM RSS, Unix CPU, null/failure paths) and a cpuMs assertion in src/gateway/restart-trace.test.ts: 
    ✓ test/scripts/gateway-bench-probes.test.ts (11 tests)
✓ src/gateway/restart-trace.test.ts (3 tests)
  • Static checks green: pnpm tsgo (core), pnpm tsgo:scripts, pnpm check:test-types, oxfmt --check, oxlint.
  • autoreview (Codex gpt-5.6-sol, high) — clean ("patch is correct"): consistently replaces the external Windows CPU probe with a ready-anchored in-process metric, updates all call sites, and preserves Unix behavior.

Note: As a fork PR, upstream CI may require maintainer approval to run.

Scope: scripts/bench-gateway-startup.ts, scripts/lib/gateway-bench-probes.ts, src/gateway/restart-trace.ts (+ its test), src/gateway/server.impl.ts, src/gateway/server-close.ts, and the new probe test — +195/−22 across 7 files.

Agent Transcript

Redacted agent transcript (GitHub Copilot CLI — trimmed to this fix) 
source: local GitHub Copilot CLI session (redacted)
omitted: raw tool outputs, system/tool context, reasoning, local paths, secrets
scope: trimmed to the Windows CPU/RSS probes fix and the ClawSweeper review response;
       unrelated earlier session tasks are excluded

[user] let's fix readProcessTreeCpuMs / readProcessRssMb

[assistant] Root cause: both probes early-return null on win32 and use Unix `ps`, so the gateway
startup bench logs cpu/cpuCore/rss as n/a on Windows.

[assistant] Added Windows CIM implementations (RSS from WorkingSetSize; CPU from process-tree
KernelModeTime+UserModeTime), kept out of the startup-timing window; added cross-platform tests.
Validated (14 tests, tsgo, oxfmt/oxlint) and opened a draft PR upstream.

[user] let's address this comment: <ClawSweeper/Codex review>

[assistant] The review's [P1]: on Windows the external CIM CPU read runs ~hundreds of ms after
readyAt, so post-ready CPU is charged to startup CPU and can falsely trip the hot-CPU gate. RSS
accepted.

[user] wouldn't a process-only (not tree-based) measurement fundamentally change what's measured?

[assistant] Investigated the gateway's startup process model: plugins load in-process, worker
threads share the PID (counted either way), and supervisors spawn after ready — so for the startup
bench there are no child processes and process.cpuUsage() equals the ps tree-sum. The tree->process
shift is definitional, not numerical, for this benchmark.

[user] proceed with process.cpuUsage() cross-platform, with an empirical check + a code comment.

[assistant] Emitted process.cpuUsage() as cpuMs in the gateway's existing memory.ready trace; the
bench now reads memory.ready.cpuMs (exact, ready-anchored, cross-platform) and the external CPU
query is retired. Reverted the Windows CIM CPU code; kept Windows CIM RSS. Empirical proof on
Windows: memory.ready reported cpuMs=52468.0 at ready with no child processes, confirming
process==tree here. tsgo/check:test-types/tests/oxfmt/oxlint green; autoreview clean ("patch is
correct").

@openclaw-barnacle openclaw-barnacle Bot added scripts Repository scripts size: M labels Jul 17, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Jul 18, 2026
@clawsweeper

clawsweeper Bot commented Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 17, 2026, 9:56 PM ET / July 18, 2026, 01:56 UTC.

Summary
The branch adds a Windows CIM RSS probe for the gateway startup benchmark and replaces startup CPU sampling with a ready-boundary process.cpuUsage() metric emitted in gateway traces.

PR surface: Source +7, Tests +109, Other +57. Total +173 across 7 files.

Reproducibility: yes. from source: current main explicitly returns null for the relevant Windows process probes, which explains the reported n/a CPU and RSS fields. A live Windows run was not independently performed in this read-only review.

Review metrics: none identified.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🦐 gold shrimp
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Preserve peak-RSS semantics on Windows or rename the value as ready-time RSS and update its consumers.
  • Post a redacted Windows terminal run of the completed benchmark showing populated cpu=, cpuCore=, and rss= output.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The PR includes a Windows gateway memory.ready trace for CPU, but not a redacted completed Windows startup-benchmark run that shows populated CPU, CPU-core, and RSS output or demonstrates the RSS collection contract. Add terminal/live output with private paths, endpoints, and identifiers redacted; updating the PR body should trigger re-review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] On Windows, a single ready-time working-set sample can be lower than the actual startup peak while the benchmark still presents it as maxRssMb; a memory-regression comparison could therefore be falsely reassuring.
  • [P1] The PR body does not yet show a redacted Windows run of the completed startup benchmark with populated cpu=, cpuCore=, and rss= output, so the changed user-visible benchmark result remains unverified.

Maintainer options:

  1. Restore an honest RSS contract (recommended)
    Before merge, preserve a defensible Windows peak-RSS measurement or rename and separately handle the single ready-time sample so benchmark output does not call it a maximum.
  2. Accept ready-time RSS explicitly
    Maintainers may accept a Windows ready-time working-set metric only if the field, output, and any regression consumers stop treating it as peak RSS.
  3. Pause the benchmark extension
    Pause this PR if a bounded Windows peak measurement cannot be implemented without unacceptable benchmark overhead.

Next step before merge

  • [P1] The remaining code correction is narrow, but the contributor must also supply real Windows behavior proof and the exact Windows RSS contract must be confirmed before merge.

Security
Cleared: The diff adds a local, PID-validated diagnostic query and trace metric; no concrete security or supply-chain regression is evident in the reviewed change.

Review findings

  • [P2] Do not report one Windows sample as peak RSS — scripts/bench-gateway-startup.ts:806-813
Review details

Best possible solution:

Keep ready-anchored CPU reporting, but either preserve a bounded Windows peak-RSS measurement or expose the single Windows value as a distinct ready-time RSS metric; then provide a redacted Windows benchmark run proving the selected contract.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: current main explicitly returns null for the relevant Windows process probes, which explains the reported n/a CPU and RSS fields. A live Windows run was not independently performed in this read-only review.

Is this the best way to solve the issue?

No. Ready-boundary CPU is a stronger solution to the earlier timing skew, but the Windows RSS change is not the best fix while it reports one post-startup snapshot as a maximum.

Full review comments:

  • [P2] Do not report one Windows sample as peak RSS — scripts/bench-gateway-startup.ts:806-813
    Windows disables the periodic sampler and only reads WorkingSetSize after readiness, but the result is still returned as maxRssMb. A short-lived pre-ready RSS peak is therefore missed and can make the benchmark under-report memory use. Preserve peak semantics or expose this as a separately named ready-time metric.
    Confidence: 0.94

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 3d1b77103511.

Label changes

Label justifications:

  • P2: The benchmark is developer-facing and the remaining defect can misstate a performance metric without affecting normal gateway runtime behavior.
  • merge-risk: 🚨 other: Merging would retain maxRssMb naming and consumers while changing Windows collection to a single non-peak sample.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR includes a Windows gateway memory.ready trace for CPU, but not a redacted completed Windows startup-benchmark run that shows populated CPU, CPU-core, and RSS output or demonstrates the RSS collection contract. Add terminal/live output with private paths, endpoints, and identifiers redacted; updating the PR body should trigger re-review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +7, Tests +109, Other +57. Total +173 across 7 files.

View PR surface stats
Area Files Added Removed Net
Source 3 16 9 +7
Tests 2 112 3 +109
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 2 67 10 +57
Total 7 195 22 +173

What I checked:

  • Current-main Windows gap: Current main’s startup benchmark helper returns null for both process RSS and process-tree CPU on win32, which source-reproduces the reported n/a values. (scripts/bench-gateway-startup.ts, 3d1b77103511)
  • Windows RSS no longer measures a maximum: The PR disables the periodic RSS timer on Windows and samples once after readiness; the returned field is still maxRssMb, so a transient pre-ready peak is silently omitted. (scripts/bench-gateway-startup.ts:806, b15c94e0f70c)
  • Prior review continuity: The prior completed ClawSweeper review already identified preserving peak-RSS semantics as a P2 concern and requested a complete Windows benchmark trace. The current PR body shows a gateway trace with cpuMs, not the benchmark output with populated CPU and RSS values. (scripts/bench-gateway-startup.ts:862, b15c94e0f70c)
  • Ready-boundary CPU design: The PR wires memory.ready.cpuMs from gateway process usage into the benchmark, avoiding the prior post-ready external CPU query skew. (src/gateway/restart-trace.ts:204, b15c94e0f70c)

Likely related people:

  • unresolved: The available review context identifies the proposed-branch author but provides no current-main blame, log, or merged-PR provenance for the benchmark and gateway trace surfaces; the PR author is therefore not used as an ownership proxy. (role: current-main history owner not established; confidence: low; files: scripts/bench-gateway-startup.ts, scripts/lib/gateway-bench-probes.ts, src/gateway/restart-trace.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (2 earlier review cycles)
  • reviewed 2026-07-18T00:03:11.708Z sha c7e971e :: found issues before merge. :: [P2] Anchor Windows CPU sampling to the ready event
  • reviewed 2026-07-18T01:14:06.620Z sha b15c94e :: needs real behavior proof before merge. :: [P2] Preserve peak RSS semantics on Windows

@paulcam206
perf(bench): report Windows RSS + ready-anchored CPU in gateway start…
b15c94e 
…up bench

The gateway startup benchmark logged cpu/cpuCore/rss as n/a on Windows because
readProcessRssMb and readProcessTreeCpuMs were Unix-only (ps).

RSS: add a Windows implementation of readProcessRssMb via a CIM Win32_Process
WorkingSetSize query (with pid validation); Unix ps path unchanged.

CPU: measure it at the gateway's own ready boundary instead of via an external
process query. The gateway already emits a memory.ready startup-trace snapshot;
it now also reports process.cpuUsage() as cpuMs, and the bench reads
memory.ready.cpuMs. This is exact and ready-anchored on every platform and
retires the external ps/CIM CPU read from the startup bench, so post-ready work
can no longer be charged to startup CPU. During startup the gateway is a single
OS process (verified: no child processes at ready), so this equals the previous
process-tree sum. readProcessTreeCpuMs stays for the Unix-only restart bench.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 8b7f11bb-0f17-4cd2-809d-a62dca26f62d
@paulcam206
paulcam206 force-pushed the perf/windows-cpu-rss-probes branch from c7e971e to b15c94e Compare July 18, 2026 01:10
@openclaw-barnacle openclaw-barnacle Bot added the gateway Gateway runtime label Jul 18, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 18, 2026
@BradGroux BradGroux mentioned this pull request Jul 18, 2026
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

gateway Gateway runtime merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. scripts Repository scripts size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@paulcam206