Skip to content

fix(pilot): debounce process resource alarms#176

Open
Monsooooon wants to merge 2 commits into
mainfrom
code-issue-676895-cp1-oss
Open

fix(pilot): debounce process resource alarms#176
Monsooooon wants to merge 2 commits into
mainfrom
code-issue-676895-cp1-oss

Conversation

@Monsooooon

Copy link
Copy Markdown
Collaborator

Background

Process resource alarms currently fire on single CPU or memory threshold samples, which creates noisy alerts from transient CPU spikes and repeated memory readings near 512MB. CPU and memory also shared the same process alarm type, allowing aggregation windows to overwrite one resource signal with the other.

Core Changes

  • Split process CPU and memory alarms into independent alarm types to avoid aggregation overwrite.
  • Normalize CPU usage by CPU count and require consecutive samples before alerting.
  • Add memory soft / hard / critical levels, with soft debounce and alert cooldown.
  • Add focused unit coverage for debounce behavior, severity selection, independent aggregation, and cooldown.

Modified Files

File Notes
src/metrics/alarm-manager.ts Adds independent process CPU and memory alarm types.
src/metrics/metrics-writer.ts Adds CPU normalization/debounce and memory levels/debounce/cooldown.
tests/unit/metrics/metrics-writer.test.ts Covers resource alarm debounce, severity, aggregation, and cooldown behavior.

Verification

  • npx vitest run tests/unit/metrics/metrics-writer.test.ts — 21/21 PASS
  • npx vitest run tests/unit/metrics/ — 62/62 PASS
  • npm run typecheck — PASS
  • Real Codex CLI E2E trace validation — PASS, 0 ERROR
    • Target CLI: /opt/homebrew/bin/codex, codex-cli 0.142.5
    • Probe coverage: 4 STEP spans, 3 TOOL spans
    • Trace validation: Summary: 1 traces, 13 spans, Pass: 33 Warn: 138 Error: 0 Skipped: 0
    • Trace ID: 2981a5fc3b47cbe517ba4f9039ee90d6

Split CPU and memory process alarms so they no longer collide in aggregation. Debounce normalized CPU and soft memory thresholds, add hard and critical memory tiers, and cool down repeated process resource alerts.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@ralf0131 ralf0131 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Splits the generic PROCESS_RESOURCE_ALARM into dedicated PROCESS_CPU_ALARM and PROCESS_MEMORY_ALARM types, with CPU normalization by core count (os.cpus()), tiered memory thresholds (soft 512MB / hard 1024MB / critical 2048MB), consecutive-sample requirements (3 samples) to suppress transient spikes, and a 1-hour cooldown to prevent alarm flapping. Comprehensive test coverage for all threshold tiers, normalization, suppression, and cooldown behavior. All CI green (Node 18/20/22). Clean, well-structured change.

Findings

No issues found — the implementation is correct, properly typed, and thoroughly tested.


*Automated review by *

Comment thread src/metrics/metrics-writer.ts Outdated

@ralf0131 ralf0131 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Re-review after commit 011f1785. The new commit correctly addresses @Snssn's feedback — CPU alarms now compare raw process CPU percent (from calcCpuPercent()) against the 80% threshold, without normalizing by host core count. The 3-consecutive-sample debounce and tiered memory thresholds (soft/hard/critical) with independent per-type cooldowns are well-implemented and comprehensively tested.

Findings

  • [Info] src/metrics/metrics-writer.ts — The PR body still states "Normalize CPU usage by CPU count", but the implementation (after the fix) uses raw process CPU percent. Consider updating the body to match the actual behavior.

Verdict

LGTM. The fix is correct, code is clean, and test coverage (6 cases) is thorough. CLA is signed.


Automated review by github-manager-bot

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.

3 participants