feat(observability): response-store metrics on /api/system/memory + read-only dashboard card - #427
feat(observability): response-store metrics on /api/system/memory + read-only dashboard card#427dev-shinyu wants to merge 5 commits into
Conversation
…ead-only dashboard card Thin observe-only layer on top of the built-in RSS watchdog (PR lidge-jun#388 increment 1). - state.ts: side-effect-free responseStateMetrics() (count/totalBytes/largestBytes/oldestAgeMs), reuses existing sizeBytes accounting; disk schema unchanged - system-routes.ts: add scalar-only responseState field to the existing GET /api/system/memory payload; same management auth gate, no new endpoints - gui: new read-only MemoryObservabilityCard (RSS ring samples, jscHeap, response-store metrics, RSS drift/hour); no sliders, no restart controls - i18n: dash.mem.* keys for en/de/ko/zh/ru/ja - docs: response-store attribution note in windows-memory.md - tests: responseStateMetrics coverage + endpoint shape (responseState, scalar-only)
📝 WalkthroughWalkthroughThe system memory endpoint now reports continuation-store metrics. A dashboard card polls and displays these diagnostics with localized labels, while tests cover metric accuracy and side-effect-free observation. Troubleshooting documentation explains the payload and dashboard view. ChangesMemory observability
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant MemoryObservabilityCard
participant SystemMemoryRoute
participant responseStateMetrics
Dashboard->>MemoryObservabilityCard: Render with apiBase
MemoryObservabilityCard->>SystemMemoryRoute: Poll GET /api/system/memory
SystemMemoryRoute->>responseStateMetrics: Read continuation-store snapshot
responseStateMetrics-->>SystemMemoryRoute: Return count, bytes, and age
SystemMemoryRoute-->>MemoryObservabilityCard: Return memory diagnostics
MemoryObservabilityCard-->>Dashboard: Render formatted metrics
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gui/src/components/MemoryObservabilityCard.tsx`:
- Around line 28-35: Make SystemMemory.responseState optional and update the
Details rendering that reads data.responseState.count to safely handle its
absence, displaying — for successful legacy payloads without responseState while
preserving the existing value for newer responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 679a5e4b-7cbc-4004-82ff-1211c99de580
📒 Files selected for processing (13)
docs-site/src/content/docs/troubleshooting/windows-memory.mdgui/src/components/MemoryObservabilityCard.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/Dashboard.tsxsrc/responses/state.tssrc/server/management/system-routes.tstests/memory-watchdog.test.tstests/responses-state.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d127822d59
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| <Stat label={t("dash.mem.jscHeap")} value={data?.jscHeap ? formatBytes(data.jscHeap.heapSize) : "—"} /> | ||
| <Stat | ||
| label={t("dash.mem.growth")} | ||
| value={growth === null ? "—" : `${growth >= 0 ? "+" : ""}${formatBytes(Math.abs(growth))}/h`} |
There was a problem hiding this comment.
When the watchdog samples span a drop in RSS, such as after GC or the OS returns pages, this expression removes the negative sign with Math.abs(growth) and only prepends + for positive values, so a -200 MB/h drift renders as 200.0 MB/h. That makes the new observability card report memory recovery as continuing growth; keep the sign or add an explicit - branch when formatting growth.
Useful? React with 👍 / 👎.
| <div className="muted text-label" style={{ margin: "14px 0 6px" }}>{t("dash.mem.store")}</div> | ||
| <div className="muted text-control" style={{ marginBottom: 10 }}>{t("dash.mem.storeHint")}</div> | ||
| <div className="stat-row"> | ||
| <Stat label={t("dash.mem.storeEntries")} value={data ? new Intl.NumberFormat(locale).format(data.responseState.count) : "—"} /> |
There was a problem hiding this comment.
When this dashboard talks to a proxy version that already has GET /api/system/memory but predates the new responseState field, the fetch succeeds and data is set, then render immediately dereferences data.responseState.count and trips the page error boundary instead of showing the intended unavailable/degraded card. Default the response-state block or validate it before rendering these stats so older-but-compatible memory payloads do not crash the dashboard.
Useful? React with 👍 / 👎.
…SS drift sign Review follow-up (lidge-jun#427): - responseState is now optional: a 200 from an older proxy that predates the field renders em-dashes instead of crashing the card (CodeRabbit major, Codex) - negative RSS drift keeps its sign so memory recovery no longer renders as growth (Codex) - docstrings for formatBytes/formatAge/Stat (docstring-coverage warning)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gui/src/components/MemoryObservabilityCard.tsx (2)
78-99: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPrevent overlapping memory polls.
setIntervalstarts a newfetchMemory()without waiting for the previous request. If/api/system/memorystalls or responses complete out of order, requests can accumulate and an older payload can overwrite newer metrics. Serialize polls with an in-flight guard, or abort/time out the previous request before starting another.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/src/components/MemoryObservabilityCard.tsx` around lines 78 - 99, Update the MemoryObservabilityCard useEffect polling logic around fetchMemory to prevent concurrent requests: track whether a memory fetch is in flight and skip interval-triggered polls until it completes, while preserving cancellation cleanup and existing unavailable handling.
47-62: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve zero age for non-empty stores.
A store entry created in the same clock tick can legitimately have
oldestAgeMs === 0, butformatAgerenders every non-positive value as"—". DistinguishresponseState.count === 0at Line 146 from invalid/negative ages so a non-empty store can display0sor its locale equivalent.Also applies to: 146-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/src/components/MemoryObservabilityCard.tsx` around lines 47 - 62, Update formatAge and its call site near responseState.count so zero is rendered as the locale-equivalent 0-second duration for non-empty stores, while empty stores continue displaying "—". Keep invalid and negative ages treated as unavailable, and use responseState.count === 0 to distinguish empty stores from valid zero ages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gui/src/components/MemoryObservabilityCard.tsx`:
- Line 131: Update the growth display in MemoryObservabilityCard to replace the
hardcoded “/h” suffix with the localized dash.mem.perHour translation via
t(...). Add the dash.mem.perHour key with the appropriate per-hour value to
every locale file used by gui, preserving the existing sign, formatting, and
null behavior.
---
Outside diff comments:
In `@gui/src/components/MemoryObservabilityCard.tsx`:
- Around line 78-99: Update the MemoryObservabilityCard useEffect polling logic
around fetchMemory to prevent concurrent requests: track whether a memory fetch
is in flight and skip interval-triggered polls until it completes, while
preserving cancellation cleanup and existing unavailable handling.
- Around line 47-62: Update formatAge and its call site near responseState.count
so zero is rendered as the locale-equivalent 0-second duration for non-empty
stores, while empty stores continue displaying "—". Keep invalid and negative
ages treated as unavailable, and use responseState.count === 0 to distinguish
empty stores from valid zero ages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 97a358d2-43fe-46d3-b312-4278063f66f4
📒 Files selected for processing (1)
gui/src/components/MemoryObservabilityCard.tsx
… store age Review follow-up round 2 (lidge-jun#427, CodeRabbit minors): - in-flight guard so a stalled /api/system/memory poll cannot stack up or let an older payload overwrite a newer one - oldest-age cell distinguishes an empty store (em-dash) from a legitimate same-tick zero age (0s) - growth suffix moved to a localized dash.mem.perHour key (en/de/ko/zh/ru/ja)
|
All bot review findings have been addressed in two follow-up commits: 174a79d (round 1 — CodeRabbit major + Codex):
759f586 (round 2 — CodeRabbit minors):
Each round re-verified locally: @coderabbitai resolve |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gui/src/components/MemoryObservabilityCard.tsx (2)
38-50: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRoute memory-card fallback values through i18n.
The changed helper block and empty-store branch emit hardcoded user-visible values (
0 B/—). Add locale keys such asdash.mem.zeroBytesanddash.mem.unavailableValue, pass them into the helpers, and use the translated value at Line 156.As per path instructions, user-visible strings in
gui/**must go through the i18n locale files rather than hardcoded text.Also applies to: 153-157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/src/components/MemoryObservabilityCard.tsx` around lines 38 - 50, Route the user-visible fallback values in formatBytes and formatAge through i18n: add locale keys such as dash.mem.zeroBytes and dash.mem.unavailableValue, pass the translated strings into these helpers, and use the translated unavailable value in the empty-store branch around the affected render logic. Remove the hardcoded "0 B" and "—" literals while preserving the existing formatting behavior for valid values.Source: Path instructions
80-98: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound serialized polls with a timeout.
If the current
fetchnever settles,inFlightremainstrueforever and every 5-second poll is skipped. The initial card can remain blank indefinitely, bypassing the unavailable fallback. Abort each request after a bounded timeout and abort the active request during effect cleanup.This conflicts with the supplied PR objective that the card handle unavailable endpoints.
Proposed fix
let cancelled = false; let inFlight = false; + let activeController: AbortController | null = null; const fetchMemory = async () => { if (inFlight) return; inFlight = true; + const controller = new AbortController(); + activeController = controller; + const timeout = setTimeout(() => controller.abort(), 10_000); try { - const res = await fetch(`${apiBase}/api/system/memory`); + const res = await fetch(`${apiBase}/api/system/memory`, { + signal: controller.signal, + }); ... } finally { + clearTimeout(timeout); + if (activeController === controller) activeController = null; inFlight = false; } }; return () => { cancelled = true; + activeController?.abort(); clearInterval(interval); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/src/components/MemoryObservabilityCard.tsx` around lines 80 - 98, Update the fetchMemory polling flow in MemoryObservabilityCard to use an AbortController with a bounded timeout for each request, ensuring stalled calls reach the existing unavailable fallback. Track the active controller and abort it during effect cleanup, while preserving the inFlight serialization and avoiding state updates after cancellation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@gui/src/components/MemoryObservabilityCard.tsx`:
- Around line 38-50: Route the user-visible fallback values in formatBytes and
formatAge through i18n: add locale keys such as dash.mem.zeroBytes and
dash.mem.unavailableValue, pass the translated strings into these helpers, and
use the translated unavailable value in the empty-store branch around the
affected render logic. Remove the hardcoded "0 B" and "—" literals while
preserving the existing formatting behavior for valid values.
- Around line 80-98: Update the fetchMemory polling flow in
MemoryObservabilityCard to use an AbortController with a bounded timeout for
each request, ensuring stalled calls reach the existing unavailable fallback.
Track the active controller and abort it during effect cleanup, while preserving
the inFlight serialization and avoiding state updates after cancellation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5eb196b7-873f-4cb6-b2fc-4976e3168602
📒 Files selected for processing (7)
gui/src/components/MemoryObservabilityCard.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.ts
|
🐇 ✅ ✅ Action performedComments resolved. Approval is disabled; enable |
Review follow-up round 3 (lidge-jun#427, CodeRabbit major): - a hung /api/system/memory request could pin the in-flight guard forever, skipping every later poll and leaving the card blank; each poll now aborts after 10s so the unavailable fallback is reachable - effect cleanup aborts the active request to avoid late state updates Skipped (with reason): localizing the em-dash/0 B fallbacks - the em-dash placeholder is the established convention across the dashboard (Dashboard.tsx, Storage.tsx, ProviderOverview.tsx) and byte units are locale-neutral there as well.
Resolve Dashboard.tsx conflict from the upstream dashboard workspace-view refactor (section rail + named section consts). - take upstream's Dashboard.tsx wholesale, then re-mount MemoryObservabilityCard at the end of overviewSection so it renders in both the workspace and classic dashboard views - i18n dash.mem.* keys (incl. perHour/details) auto-merged cleanly Re-verified: GUI typecheck/lint/build green; responses-state 34/0 + memory-watchdog 7/0.
|
Rebased conflict resolution: merged the latest |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gui/src/components/MemoryObservabilityCard.tsx (1)
39-44: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse correct and locale-aware byte formatting.
Lines [39-44] divide by 1024 but label the results
KB/MB/..., which misstates binary values; useKiB/MiB/...or divide by 1000. ThetoFixedoutput also hardcodes.decimal separators for every locale. Pass the locale into this helper and use localized unit/number formatting for all RSS, heap, store, and growth values.As per path instructions,
gui/**user-visible strings must go through the i18n locale files rather than hardcoded text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/src/components/MemoryObservabilityCard.tsx` around lines 39 - 44, Update formatBytes in MemoryObservabilityCard to use locale-aware number formatting and binary unit labels (B, KiB, MiB, GiB, TiB) consistent with division by 1024. Pass the active locale into the helper and update every RSS, heap, store, and growth call site to use it; route any user-visible unit strings through the gui i18n locale files instead of hardcoding them.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@gui/src/components/MemoryObservabilityCard.tsx`:
- Around line 39-44: Update formatBytes in MemoryObservabilityCard to use
locale-aware number formatting and binary unit labels (B, KiB, MiB, GiB, TiB)
consistent with division by 1024. Pass the active locale into the helper and
update every RSS, heap, store, and growth call site to use it; route any
user-visible unit strings through the gui i18n locale files instead of
hardcoding them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 034204ce-d289-4653-a376-81d1933868cd
📒 Files selected for processing (8)
gui/src/components/MemoryObservabilityCard.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/Dashboard.tsx
🔒 Maintainer integration in progress — please hold off on merging@lidge-jun (maintainer) is actively integrating this PR into What is happening This PR is being integrated on the maintainer branch What this means for you
This PR will be closed with a merge receipt (integration commit SHA, verification output, Integration tracker: |
PR #427 (head a4a212d, author dev-shinyu)과 PR #385 (head ac0260b, author latemonk)을 통합하고 단위 표기 결함과 테스트 공백을 함께 고친다. #427은 previous_response_id 연속성 저장소의 count/bytes/age를 /api/system/memory에 얹어, RAM 증가가 대화 보존 때문인지 런타임 할당자 때문인지 구분할 수 있게 한다. 노출값은 숫자뿐이고 스냅샷은 lazy-load, prune, evict를 하지 않는다. 단위 표기 수정: 카드가 1024로 나눈 값을 KB/MB로 표시해 단계마다 약 2.4%씩 어긋났다. 이제 KiB/MiB 표기를 쓰고 숫자도 활성 로케일을 따른다. RSS, JS heap, JSC heap, 증가량, 저장소 total/largest, watchdog 임계값 7곳을 모두 바꿨다. rendered 회귀 3건: 정상 payload가 이진 단위로 표시됨, unmount 후 폴링이 멈춤, non-OK 응답이 unavailable 노트로 degrade됨. formatter는 모듈 내부로 두었다 — export하면 react-refresh 규칙에 걸린다. #385 BizRouter는 registry 목록 존재만 검사되고 있었다. 엔드포인트, adapter, authKind, 기본 모델이 seed에 있음, vendor 네임스페이스 ID가 그대로 통과함(카탈로그 슬러그만 평탄화)을 고정하는 테스트를 추가했다. Co-authored-by: dev-shinyu <dev-shinyu@users.noreply.github.com> Co-authored-by: latemonk <latemonk@users.noreply.github.com>
✅ Integrated into
|
|
Closing: integrated into |
Summary
Per reviewer direction on #388 ("rebase onto the current built-in RSS watchdog", "separate observability from restart policy"), this PR is increment 1 of 2: a thin observe-only layer on top of the existing built-in RSS watchdog. It supersedes the observability half of #388. The opt-in restart policy will follow as a separate focused PR (increment 2).
Changes
src/responses/state.ts— side-effect-freeresponseStateMetrics()(count / totalBytes / largestBytes / oldestAgeMs), reusing the existingsizeBytesaccounting. Disk schema unchanged.src/server/management/system-routes.ts— adds a scalar-onlyresponseStatefield to the existingGET /api/system/memorypayload. Same management auth gate; no new endpoints, no PUT.MemoryObservabilityCard: headline stats (RSS, JS heap, JSC heap, RSS drift/hour) always visible; secondary diagnostics collapsed under a<details>section. No sliders, no restart controls.dash.mem.*keys for en/de/ko/zh/ru/ja.windows-memory.md.responseStateMetricscoverage +/api/system/memoryendpoint shape incl.responseState(scalar-only).Explicitly out of scope (deferred to increment 2)
Restart policy, restart history, supervisor detection, PowerShell commit-axis probe,
/api/memorysettings surface.memory-watchdog.tsis untouched.Verification
typecheck,lint:gui,build:gui,privacy:scan: all greenresponses-state,memory-watchdog): 41 pass / 0 failtests/memory-watchdog.test.tspasses unmodifiedSummary by CodeRabbit
New Features
Documentation
GET /api/system/memoryto explain the new continuation-storeresponseStatefields.Bug Fixes
Tests