feat: memory observability + opt-in watchdog (v1, observe-first) - #388
feat: memory observability + opt-in watchdog (v1, observe-first)#388dev-shinyu wants to merge 9 commits into
Conversation
…/Recommend/Adjust) Mitigate the Bun/mimalloc committed-memory retention that can exhaust the system commit charge in long sessions. App code cannot free native-retained memory, so this ships mitigation (observe -> recommend -> safe restart), not a cure. Runtime: portable memory snapshot (memory-usage.ts) reads Windows private/committed via a guarded PowerShell probe (hard timeout, soft-fail), Linux /proc/self/status, RSS fallback elsewhere; all thresholds are fractions of total RAM so behavior is identical on every machine. memory-watchdog.ts adds detectSupervisor, pure evaluate, recommend, live applyWatchdogRuntimeConfig, and a requireSupervisor gate that suppresses auto-restart to warn-only when no supervisor is present. responseStateMetrics() is a side-effect-free seam to distinguish JS-heap vs native growth. API: GET /api/memory (read-only report) and PUT /api/memory/settings (validated, live-applied, persisted) behind the existing management auth + local-only gate. GUI: dashboard (beta) MemoryCard with three layered opt-in options; i18n keys added across en/de/ko/zh/ru/ja. Defaults: enabled, warn-only, autoRestart=false, requireSupervisor=true. Tests: memory-watchdog (34) + memory-api + responseStateMetrics.
…raceMs Replace the hard-coded 5s drain before a memory-driven restart with a configurable restartGraceMs (default 30s). drainAndShutdown already rejects new turns (503) and returns the moment the in-flight set empties, so the restart now lands on a natural idle gap and only aborts turns that outlive the budget. Exposed via config (memoryWatchdog.restartGraceMs), env (OCX_MEMORY_WATCHDOG_RESTART_GRACE_MS), and PUT /api/memory/settings (validated as a positive number). Live-tunable through applyWatchdogRuntimeConfig.
Add a calm, non-alarming muted note beneath the auto-restart control explaining what happens when it fires: in-flight requests drain first, a reply still streaming at that instant is regenerated on retry, the conversation context is preserved, and restarts are rate-limited. Adds memory.autoRestartNote to the en source of truth plus de/ko/zh/ru/ja translations (compile-checked key set).
…, honest cross-process loop guard - restartGraceMs is clamped into [1s, 10min] on every entry path (env, config file, management API — which now 400s out-of-range values), bounding how long a restart can hold the proxy draining (503) on a stuck turn. - minRestartIntervalMs is normalized to at least restartGraceMs so a second restart can never arm while the first drain is still inside its grace window; intervalMs gets a 1s floor and junk numeric config values fall back instead of flowing into setInterval; maxRestarts keeps 0 as "never auto-restart". - defaultRestart is single-flight and stops the sampling timer before draining; a throwing/rejecting restart hook is logged instead of becoming an unhandled rejection. - Honest loop guard: the in-memory cooldown/cap counters die with the process a fired restart ends, so they are now re-seeded from a small best-effort history file (timestamps only, atomic-rename write, every failure swallowed, rolling ~6h window). Comments/types no longer claim restart loops are "prevented" — cross-boundary protection degrades to the supervisor's own restart policy, and exit code 75 is documented as a respawn request, nothing more. - drainAndShutdown documents why concurrent invocations (signal path racing the watchdog) are safe without locking.
…r/Windows docs
- The auto-restart note now interpolates the actual restartGraceMs ("up to
{seconds} seconds"), states that new requests pause and unfinished streaming
replies may be interrupted and retried — no more unbounded "drained first"
reading and no overstatement of preservation. Re-translated in all six
locales.
- New "Restart grace period (seconds)" input in the Adjust section: range
[1, 600]s mirroring the server clamp, blocked with a clear message when the
value exceeds the minimum restart interval (the dashboard cannot save a
combination the runtime would have to correct).
- docs-site (en + ko): new memoryWatchdog reference section — field table, env
overrides, and a supervisors-and-Windows note: NSSM/Windows services are not
auto-detected (set OCX_SUPERVISED=1), exit code 75 is only a respawn request,
and in-process maxRestarts is best-effort across process boundaries, so the
supervisor's own restart limit/backoff remains the outer protection layer.
…mmit axis (v1) Rework of the watchdog measurement layer per the joint audit (terra-high + qwen-ultimate v4 spec): async collect, sync decide. - memory-usage: the Windows probe is now an async Bun.spawn (array args, -NoProfile -NonInteractive, hidden window) collecting, in ONE child, this PID's Private Bytes plus the system-wide CommittedBytes / CommitLimit / AvailableBytes (Win32_PerfRawData_PerfOS_Memory — all bytes, verified live). The event loop is never blocked; on timeout the child is killed and the cycle degrades to an RSS fallback with a sanitized probeError code (first-resolution-wins: a late result after the timeout is dropped). Budget 15s, sized from live cold-spawn measurements of 4.0-5.9s. - MemorySnapshot renamed for honesty: processPrivateBytes (real Windows Private Bytes) vs processPressureBytes (what decisions use — private on Windows, VmRSS+VmSwap on Linux, RSS otherwise) with processSource naming the origin; system commit fields + systemCommitAvailable; capturedAt. - watchdog driver: self-rescheduling probe loop — each completed probe triggers exactly one evaluation; first probe fires immediately at start; a generation guard + AbortController drop late results and kill the in-flight child on stop; live intervalMs changes re-arm the pending timer. - evaluate: process-pressure axis unchanged (warn/critical/auto-restart). NEW system-commit axis is OBSERVE-ONLY: crossing the high-water (default 0.90; env-only experimental OCX_MEMORY_WATCHDOG_COMMIT_HIGH_WATER, clamped [0.50,0.99]) logs one latched warning, re-arms only on a MEASURED recovery (measurement loss holds the latch), and NEVER arms a restart — the cause may be another process that restarting OpenCodex would not free. - report: per-probe cache (never recomputed on 5s polls) exposing the measurement block, responseStateMetrics, lastProbeAt (completion time) and lastSuccessfulSystemProbeAt for degraded/stale display. - stress harness: three async committed probes (start / before-clear / after-clear+GC) record Private>>RSS detectability — live smoke run on this box shows afterClear private/RSS = 4.4-4.8 (the field signature, detected). Framed honestly: detectability evidence, not a non-regression proof. Tests: 73 pass (fixtures migrated; new coverage for commit axis latch/re-arm/ unavailable-hold, never-restarts, immediate first probe, late-result-after- stop, no probe overlap, stop-aborts-capture, probe assembly per source, plus a real Windows end-to-end probe test).
…mmit axis
- MemoryCard: second Monitor row showing actual Private Bytes (distinct from
the pressure value in the gauge), system commit used/limit/% (or n/a when
unmeasured), response-cache entries/bytes, probe freshness ("Ns ago", from
the fetch clock — no Date.now() in render) and a red sanitized degraded
code when the probe fell back to RSS. Nine new i18n keys in all six
locales.
- docs-site (en + ko): memoryWatchdog section now documents the async
non-blocking probe, the observe-only system-commit warning (high-water
0.90, latched, never restarts), the experimental env-only
OCX_MEMORY_WATCHDOG_COMMIT_HIGH_WATER override, and the new /api/memory
report fields.
…row fallback Four fixes from the joint post-implementation audit (terra-high + qwen-ultimate), all verified against the code before applying: - lidge-jun#1 (blocker): when an auto-restart fired, tick()'s restart hook synchronously nulled the global `running` (defaultRestart calls stopMemoryWatchdog before draining), and probeOnce's trailing `scheduleNextProbe(generation, running.cfg.intervalMs)` then evaluated `running.cfg` on null — an unhandled TypeError at the exact moment of every real auto-restart (surfacing as crash-guard noise, and a dead loop for injected restart hooks). probeOnce now pins its instance in a local `current` and re-checks identity against the global after capture, after tick, and before rescheduling: a stopped/replaced watchdog is left untouched and never rescheduled — the restart owns the exit. - lidge-jun#2: an unexpected exception escaping capture() no longer silences the whole cycle; probeOnce evaluates exactly once on a new exported rssFallbackSnapshot(now, "capture-threw") instead of skipping evaluation. - lidge-jun#3: the PowerShell probe now emits labeled values (P=/C=/L=/A=) parsed by the new pure parseWindowsProbeOutput() — a $null first value previously rendered as a blank line that trim() removed, shifting positional indexes and misassigning commit values to privateBytes. Order-independent, loss-explicit, unit-tested; first-resolution-wins/timeout/kill handling untouched. - lidge-jun#4: applyWatchdogRuntimeConfig now finite-guards systemCommitHighWater and maxRestarts (same pattern as resolveWatchdogConfig): a programmatic partial carrying undefined/NaN would otherwise make `fraction >= NaN` permanently false and silently disable the commit warning. Regression tests: auto-restart-stops-watchdog mid-tick (validated to fail on the pre-fix code via bun's unhandled-rejection surfacing), capture-throw RSS fallback, NaN/undefined live re-clamp, and label-parser unit cases. 77 pass.
…pe, audit trail Permanent record for the PR: why the watchdog exists (79GB committed / 97% system commit field incident), the async-collect/sync-decide architecture and its defended invariants, the platform matrix (cross-platform core, Windows- first measurement fidelity; v1 system-commit axis is Windows-only), why the commit axis is observe-only in v1 (it cannot know the pressure is ours — and honestly does NOT auto-mitigate the original incident yet), the quiet-window restart + honestly-scoped loop guards, verification evidence incl. live measurements, the three audit rounds, and deferred follow-ups.
📝 WalkthroughWalkthroughAdds a cross-platform memory watchdog with warn-only monitoring, optional supervised graceful restarts, persisted restart history, management APIs, dashboard controls, localized UI text, stress testing, and expanded configuration documentation. ChangesMemory watchdog
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9590de762
ℹ️ 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".
| if (patch.enabled === false) { | ||
| stopMemoryWatchdog(); | ||
| } else if (applyWatchdogRuntimeConfig(patch) === null) { | ||
| // Not currently running (was disabled / first enable) — (re)start from the saved config. | ||
| startMemoryWatchdog(config); |
There was a problem hiding this comment.
On servers started with an OCX_MEMORY_WATCHDOG_* env override, this live path applies/stops based on the raw dashboard patch instead of the resolved config. For example, OCX_MEMORY_WATCHDOG_ENABLED=1 can be defeated by PUT {"enabled":false}, and _AUTO_RESTART=0 can be defeated by PUT {"autoRestart":true} until the next process restart, even though resolveWatchdogConfig gives env vars highest precedence. Re-resolve the saved config, or ignore env-owned fields, before stopping/applying live settings.
Useful? React with 👍 / 👎.
| const interval = setInterval(() => { | ||
| // Don't stomp the sliders the user is dragging. | ||
| if (!editingRef.current) void refresh(); |
There was a problem hiding this comment.
After any threshold or grace edit sets editingRef.current = true, this interval never refreshes again unless a save path eventually resets the flag. If a user drags a slider, or enters an invalid grace value and then leaves the card without applying, the Monitor panel freezes and stops reporting current memory pressure. Reset the flag on pointer-up/blur, or gate polling on an actual active drag rather than a sticky draft.
Useful? React with 👍 / 👎.
| })}</span> | ||
| <button type="button" className="btn btn-sm" style={{ marginLeft: "auto" }} disabled={saving} onClick={applyRecommended}>{t("memory.applyRecommended")}</button> | ||
| </div> | ||
| <div className="muted text-label" style={{ marginTop: 4 }}>{rec.rationale}</div> |
There was a problem hiding this comment.
Under any non-English locale, this renders the API's English recommendation prose directly (insufficient samples... / observed peak...) instead of formatting localized copy, so the new dashboard card mixes English into translated UI. Return structured reason/values and render them with t(...) entries in every locale file.
AGENTS.md reference: gui/AGENTS.md:L5-L9
Useful? React with 👍 / 👎.
| ); | ||
| if (criticalFraction <= warnFraction) criticalFraction = Math.min(0.99, warnFraction + 0.10); | ||
|
|
||
| const maxRestartsRaw = envNum("OCX_MEMORY_WATCHDOG_MAX_RESTARTS") ?? c.maxRestarts; |
There was a problem hiding this comment.
When an operator sets OCX_MEMORY_WATCHDOG_MAX_RESTARTS=0 to disable automatic restarts, this goes through envNum(), which drops zero as undefined, so the runtime falls back to the config/default cap of 3 and can still restart once autoRestart is armed. Parse this env var with a non-negative helper so the documented emergency-off value is honored.
Useful? React with 👍 / 👎.
| bytes = JSON.stringify(state.items).length; | ||
| if (state.providers) bytes += JSON.stringify(state.providers).length; |
There was a problem hiding this comment.
When the continuation cache contains many large or long previous_response_id chains, every completed watchdog probe synchronously JSON.stringifys each retained item array. Those arrays can include replayed prefixes and image payloads, so the watchdog itself can allocate/stringify far more than the live heap and stall streaming requests right when memory is already pressured; cap the byte walk or keep an incremental approximate size instead.
Useful? React with 👍 / 👎.
| autoRestart: envFlag("OCX_MEMORY_WATCHDOG_AUTO_RESTART") ?? c.autoRestart ?? DEFAULTS.autoRestart, | ||
| requireSupervisor: | ||
| envFlag("OCX_MEMORY_WATCHDOG_REQUIRE_SUPERVISOR") ?? c.requireSupervisor ?? DEFAULTS.requireSupervisor, |
There was a problem hiding this comment.
Because memoryWatchdog is currently accepted through the passthrough config schema, manually edited JSON can supply strings here, and these ?? c.autoRestart / ?? c.requireSupervisor assignments then pass truthy strings into the decision core. In practice "autoRestart":"false" becomes armed auto-restart when critical pressure is reached, which violates the opt-in safety model; validate the object in configSchema or coerce only real booleans/numbers in resolveWatchdogConfig.
Useful? React with 👍 / 👎.
| } else { | ||
| // critical | ||
| const cooledDown = nowMs - state.lastRestartAt >= cfg.minRestartIntervalMs || state.lastRestartAt === 0; | ||
| const underCap = state.restartCount < cfg.maxRestarts; |
There was a problem hiding this comment.
After the process has consumed maxRestarts, this scalar restartCount never decays; if the watchdog remains alive in critical warn-only mode past the documented rolling history window, underCap stays false forever and automatic restarts never resume. Keep restart timestamps in memory and prune them by RESTART_HISTORY_WINDOW_MS (or reload/prune history) before this check so the cap is actually rolling.
Useful? React with 👍 / 👎.
| const explicit = parseFlagValue(env.OCX_SUPERVISED); | ||
| if (explicit === true) return { supervised: true, hint: "OCX_SUPERVISED" }; | ||
| if (env.pm_id !== undefined || (env.PM2_HOME ?? "") !== "") return { supervised: true, hint: "pm2" }; | ||
| if ((env.INVOCATION_ID ?? "") !== "" || (env.NOTIFY_SOCKET ?? "") !== "") return { supervised: true, hint: "systemd" }; | ||
| if (explicit === false) return { supervised: false, hint: "OCX_SUPERVISED=off" }; | ||
| return { supervised: false, hint: "none" }; |
There was a problem hiding this comment.
The repository's own ocx service launchers set OCX_SERVICE=1 and are configured to restart the proxy, but this detector ignores that marker, so users running under the built-in launchd/Task Scheduler service still see supervisor: none and the default requireSupervisor gate suppresses memory auto-restart. Treat OCX_SERVICE=1 as supervised (while still allowing explicit OCX_SUPERVISED=off to override) so the new feature works with the supported service manager.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@docs-site/src/content/docs/reference/configuration.md`:
- Around line 113-173: Sync the Russian configuration documentation with the
English memoryWatchdog section by adding the equivalent content to
docs-site/src/content/docs/ru/reference/configuration.md, preserving all fields,
defaults, environment overrides, API details, and supervisor notes. Also update
structure/09_memory-watchdog.md in the specified range to include ru as a
tracked translation locale; no change is needed to the English configuration
source.
In `@gui/src/components/MemoryCard.tsx`:
- Line 189: Replace the hardcoded “beta” text in both badge render locations
within MemoryCard with the component’s existing t(...) translation helper, using
the appropriate memory beta locale key. Add or reuse that key in the i18n locale
files so both badges are translatable.
- Around line 230-233: Update applyThresholds so criticalFraction is capped
below 1 before calling save, while preserving the requirement that it remains
greater than warnFraction. Use the existing warnPct and critPct values, ensure
the 99% warn-slider maximum produces a valid fraction, and correct the nearby
comment to describe server validation rather than clamping.
- Around line 372-383: Update MemoryCard.tsx to import the CSSProperties type
directly from React, then replace the React.CSSProperties annotations on
betaBadge and rowStyle with CSSProperties.
In `@gui/src/i18n/en.ts`:
- Line 8: Update the memory.intro translation in gui/src/i18n/en.ts at lines
8-8, gui/src/i18n/de.ts at lines 7-7, gui/src/i18n/ja.ts at lines 8-8, and
gui/src/i18n/ko.ts at lines 8-8 so supervisor detection is described as required
by default or when “Require supervisor” is enabled, rather than universally
required.
In `@scripts/memory-stress-harness.ts`:
- Around line 19-21: Update the header safety documentation and related RSS-cap
documentation near the environment configuration to remove the nonexistent
--rss-cap-mb CLI flag and identify OCX_STRESS_RSS_CAP_MB as the supported
configuration mechanism, including its 1200 MB default.
- Around line 222-257: Correct the chain completion count in the loop around
`turn` so normal completion reports the actual number of processed turns rather
than the post-loop counter value. Preserve the abort-path count, and use the
corrected count consistently for `chain-final@...`, `summary.chain.turns`, and
`approxUniqueContentMb` so the derived ratios remain accurate.
In `@src/responses/state.ts`:
- Around line 206-217: Update responseStateMetrics() to avoid fully
JSON.stringify-ing every state.items and providers entry on each probe. Replace
the per-entry serialization with a cheaper approximate size calculation or
bounded estimation that caps work and allocations while preserving totalBytes,
largestBytes, and oldestCreatedAt metric updates.
In `@src/server/management-api.ts`:
- Around line 269-302: Validate the memory watchdog patch before saveConfig in
the management API: reject keys outside the supported watchdog settings and
reject the effective configuration when criticalFraction is not greater than
warnFraction, including partial patches merged with existing config. Reuse or
introduce a shared normalizer used by both this API path and
applyWatchdogRuntimeConfig so persisted, live, and response configurations
remain consistent, and add regression coverage for inverted thresholds and
unknown keys.
🪄 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: 55635394-7bea-4545-8ea2-e3e6e9287c90
📒 Files selected for processing (24)
docs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/configuration.mdgui/src/components/MemoryCard.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.tsxscripts/memory-stress-harness.tssrc/lib/memory-usage.tssrc/responses/state.tssrc/server/index.tssrc/server/lifecycle.tssrc/server/management-api.tssrc/server/memory-restart-history.tssrc/server/memory-watchdog.tssrc/types.tsstructure/09_memory-watchdog.mdtests/memory-api.test.tstests/memory-restart-history.test.tstests/memory-watchdog.test.tstests/responses-state.test.ts
| ### `memoryWatchdog` | ||
|
|
||
| Observes process memory pressure as a fraction of total system RAM and warns when it crosses | ||
| thresholds. By default it only warns. Optionally — and only when explicitly enabled — it can | ||
| gracefully restart the proxy at the critical threshold, as a mitigation for native | ||
| committed-memory retention that cannot be fixed in application code. | ||
|
|
||
| Measurement is fully asynchronous: on Windows a hidden child PowerShell process collects this | ||
| PID's Private Bytes plus the system-wide **Committed Bytes / Commit Limit** in one spawn, so the | ||
| proxy's event loop is never blocked by a slow probe (a timed-out probe is killed and that cycle | ||
| degrades to an RSS fallback). The first probe fires immediately at startup. System commit is a | ||
| separate **observe-only** axis: crossing an internal high-water (default 0.90 of the commit | ||
| limit) logs one latched warning and re-arms only on a measured recovery — it **never triggers a | ||
| restart**, because the commit pressure may come from another process that restarting OpenCodex | ||
| would not free. Tunable via the experimental env-only override | ||
| `OCX_MEMORY_WATCHDOG_COMMIT_HIGH_WATER` (clamped to `[0.50, 0.99]`; no config/UI knob until the | ||
| axis is validated by field measurement). | ||
|
|
||
| | Field | Type | Default | Meaning | | ||
| | --- | --- | --- | --- | | ||
| | `enabled?` | `boolean` | `true` | Master switch. The default observation mode is warn-only and cannot lose data. | | ||
| | `intervalMs?` | `number` | `60000` | Sampling interval. Floored at 1000 ms. | | ||
| | `warnFraction?` | `number` | `0.60` | Warn when pressure / total RAM crosses this fraction. Clamped to `[0.10, 0.99]`. | | ||
| | `criticalFraction?` | `number` | `0.75` | Critical threshold; arms the opt-in restart. Kept above `warnFraction`. | | ||
| | `autoRestart?` | `boolean` | `false` | Opt in to a graceful drain-then-restart at the critical threshold. | | ||
| | `requireSupervisor?` | `boolean` | `true` | Only restart when a process supervisor is detected, so the proxy is never left dead. | | ||
| | `restartGraceMs?` | `number` | `30000` | Quiet-window drain budget: in-flight turns get up to this long to finish before the restart proceeds; new requests receive `503` + `Retry-After` while draining. Clamped to `[1000, 600000]`. | | ||
| | `minRestartIntervalMs?` | `number` | `600000` | Cooldown between restart requests. Raised to at least `restartGraceMs`. | | ||
| | `maxRestarts?` | `number` | `3` | Memory-driven restarts allowed within a rolling ~6 h window before the watchdog degrades to warn-only. `0` disables auto-restart. | | ||
|
|
||
| Environment overrides (highest precedence): `OCX_MEMORY_WATCHDOG_ENABLED` / `_DISABLED`, | ||
| `_INTERVAL_MS`, `_WARN_FRACTION`, `_CRITICAL_FRACTION`, `_AUTO_RESTART`, `_REQUIRE_SUPERVISOR`, | ||
| `_RESTART_GRACE_MS`, `_MIN_RESTART_INTERVAL_MS`, `_MAX_RESTARTS`, `_COMMIT_HIGH_WATER` | ||
| (experimental, env-only — all prefixed `OCX_MEMORY_WATCHDOG`). All entry paths — environment, | ||
| `config.json`, and the dashboard / management API — go through the same final validation and | ||
| clamping. | ||
|
|
||
| The `/api/memory` report (and the dashboard's Memory card) additionally exposes the last probe's | ||
| measurements: actual Private Bytes vs. the pressure value the decisions use (`processSource` | ||
| names the origin — `windows-private`, `proc-status`, or the degraded `rss-fallback`), system | ||
| commit used/limit/fraction, available physical memory, response-cache metrics (entry count / | ||
| serialized bytes / largest entry), a sanitized `probeError` code when degraded, plus | ||
| `lastProbeAt` / `lastSuccessfulSystemProbeAt` timestamps for staleness. | ||
|
|
||
| :::note[Supervisors and Windows] | ||
| A memory-driven restart works by **exiting with code 75** — a request to an external supervisor | ||
| to respawn the process, nothing more. Without a supervisor the proxy would simply stay stopped, | ||
| which is why `requireSupervisor` defaults to `true`. | ||
|
|
||
| - Supervisor auto-detection covers **pm2**, **systemd**, and the explicit `OCX_SUPERVISED=1` | ||
| flag. **NSSM, Windows services (`sc.exe`), Task Scheduler and similar Windows service | ||
| managers are not auto-detected** — set `OCX_SUPERVISED=1` in the service environment when the | ||
| service manager is configured to restart the process on exit. | ||
| - The watchdog's cooldown and `maxRestarts` counters are seeded across restarts from a small | ||
| best-effort history file (timestamps only). When that file cannot be read or written, they | ||
| restart from zero in the new process — so they limit restart churn but are **not a permanent | ||
| cross-process guarantee**. Configure your supervisor's own restart limit / backoff policy | ||
| (e.g. pm2 `max_restarts` + `exp_backoff_restart_delay`, systemd `StartLimitIntervalSec` + | ||
| `StartLimitBurst`) as the outer layer of protection. | ||
| ::: | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether a ru configuration.md exists and whether it already mentions memoryWatchdog.
fd -i 'configuration.md' docs-site/src/content/docs
echo "---"
fd -i 'configuration.md' docs-site/src/content/docs/ru --exec grep -n "memoryWatchdog" {} \;Repository: lidge-jun/opencodex
Length of output: 446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## docs-site locale configuration files"
fd -i 'configuration.md' docs-site/src/content/docs
echo
echo "## ru memoryWatchdog occurrences"
rg -n "memoryWatchdog|OCX_MEMORY_WATCHDOG|Memory card|Private Bytes|Commit Limit" docs-site/src/content/docs/ru/reference/configuration.md
echo
echo "## ko memoryWatchdog occurrences"
rg -n "memoryWatchdog|OCX_MEMORY_WATCHDOG|Memory card|Private Bytes|Commit Limit" docs-site/src/content/docs/ko/reference/configuration.md
echo
echo "## ja memoryWatchdog occurrences"
rg -n "memoryWatchdog|OCX_MEMORY_WATCHDOG|Memory card|Private Bytes|Commit Limit" docs-site/src/content/docs/ja/reference/configuration.md
echo
echo "## zh-cn memoryWatchdog occurrences"
rg -n "memoryWatchdog|OCX_MEMORY_WATCHDOG|Memory card|Private Bytes|Commit Limit" docs-site/src/content/docs/zh-cn/reference/configuration.md
echo
echo "## follow-up note around the translation bullet"
cat -n structure/09_memory-watchdog.md | sed -n '120,130p'Repository: lidge-jun/opencodex
Length of output: 516
Sync the Russian config docs with the new memoryWatchdog section
docs-site/src/content/docs/reference/configuration.md#L113-L173 adds a new memoryWatchdog block, but docs-site/src/content/docs/ru/reference/configuration.md exists and does not include it, so the Russian docs will drift from the English source. Add the same section there and include ru in structure/09_memory-watchdog.md#L122-L130 so the translation gap is tracked.
📍 Affects 2 files
docs-site/src/content/docs/reference/configuration.md#L113-L173(this comment)structure/09_memory-watchdog.md#L122-L130
🤖 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 `@docs-site/src/content/docs/reference/configuration.md` around lines 113 -
173, Sync the Russian configuration documentation with the English
memoryWatchdog section by adding the equivalent content to
docs-site/src/content/docs/ru/reference/configuration.md, preserving all fields,
defaults, environment overrides, API details, and supervisor notes. Also update
structure/09_memory-watchdog.md in the specified range to include ru as a
tracked translation locale; no change is needed to the English configuration
source.
| <div className="panel" style={{ marginBottom: 24 }}> | ||
| <div style={{ display: "flex", alignItems: "center", gap: 8 }}> | ||
| <strong>{t("memory.title")}</strong> | ||
| <span className="badge" style={betaBadge}>beta</span> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hardcoded beta badge bypasses i18n.
The beta badge text is hardcoded here and again at Line 253, while every other user-visible string in this component goes through t(...). Route it through a locale key (e.g. t("memory.beta")) so the badge is translatable and consistent with the rest of the panel.
As per path instructions: "user-visible strings 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/MemoryCard.tsx` at line 189, Replace the hardcoded “beta”
text in both badge render locations within MemoryCard with the component’s
existing t(...) translation helper, using the appropriate memory beta locale
key. Add or reuse that key in the i18n locale files so both badges are
translatable.
Source: Path instructions
| const applyThresholds = () => { | ||
| // Server clamps + keeps critical > warn; send fractions. | ||
| void save({ warnFraction: warnPct / 100, criticalFraction: Math.max(critPct, warnPct + 1) / 100 }); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
applyThresholds can emit criticalFraction: 1.0 at the top of the warn slider, which the server rejects.
The warn Slider allows max={99} (Line 321). When warnPct === 99, Math.max(critPct, warnPct + 1) is 100, so criticalFraction becomes 100 / 100 = 1.0. The management endpoint validates with isFrac = v > 0 && v < 1 (strict upper bound), so this PUT is rejected with 400 "criticalFraction must be a number in (0,1)" and the user just sees a save error — the inline comment "Server clamps + keeps critical > warn" is inaccurate here, since the server validates and rejects before any clamping happens.
Clamp the fraction below 1 (99%) before sending:
As per path instructions: "Check that GUI state changes stay consistent with the management API responses".
🐛 Proposed fix
const applyThresholds = () => {
- // Server clamps + keeps critical > warn; send fractions.
- void save({ warnFraction: warnPct / 100, criticalFraction: Math.max(critPct, warnPct + 1) / 100 });
+ // Server keeps critical > warn; both fractions must stay strictly < 1 (server rejects 1.0).
+ void save({ warnFraction: warnPct / 100, criticalFraction: Math.min(99, Math.max(critPct, warnPct + 1)) / 100 });
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const applyThresholds = () => { | |
| // Server clamps + keeps critical > warn; send fractions. | |
| void save({ warnFraction: warnPct / 100, criticalFraction: Math.max(critPct, warnPct + 1) / 100 }); | |
| }; | |
| const applyThresholds = () => { | |
| // Server keeps critical > warn; both fractions must stay strictly < 1 (server rejects 1.0). | |
| void save({ warnFraction: warnPct / 100, criticalFraction: Math.min(99, Math.max(critPct, warnPct + 1)) / 100 }); | |
| }; |
🤖 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/MemoryCard.tsx` around lines 230 - 233, Update
applyThresholds so criticalFraction is capped below 1 before calling save, while
preserving the requirement that it remains greater than warnFraction. Use the
existing warnPct and critPct values, ensure the 99% warn-slider maximum produces
a valid fraction, and correct the nearby comment to describe server validation
rather than clamping.
Source: Path instructions
| const betaBadge: React.CSSProperties = { | ||
| background: "transparent", | ||
| color: "var(--muted)", | ||
| border: "1px solid var(--border, var(--muted))", | ||
| padding: "1px 7px", | ||
| borderRadius: "var(--radius-pill)", | ||
| fontSize: 11, | ||
| textTransform: "uppercase", | ||
| letterSpacing: 0.4, | ||
| }; | ||
|
|
||
| const rowStyle: React.CSSProperties = { display: "flex", alignItems: "center", gap: 8, cursor: "pointer" }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does this file bind the React namespace at all?
rg -n "import .* from ['\"]react['\"]" gui/src/components/MemoryCard.tsx
# Is the React.CSSProperties-without-namespace-import pattern used elsewhere (established convention)?
rg -n 'React\.CSSProperties' gui/src
# tsconfig knobs that would make the UMD global legal in a module
fd -t f 'tsconfig*.json' | xargs rg -n '"allowUmdGlobalAccess"|"jsx"'Repository: lidge-jun/opencodex
Length of output: 454
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tsconfig files ---'
fd -t f 'tsconfig*.json' .
echo
echo '--- tsconfig settings ---'
for f in $(fd -t f 'tsconfig*.json' .); do
echo "### $f"
sed -n '1,220p' "$f"
echo
done
echo '--- React namespace patterns in gui/src ---'
rg -n 'React\.CSSProperties|import\s+\*\s+as\s+React|import\s+React\b|import\s+type\s+React' gui/src
echo
echo '--- MemoryCard.tsx imports and local context ---'
sed -n '1,30p' gui/src/components/MemoryCard.tsx
sed -n '360,390p' gui/src/components/MemoryCard.tsxRepository: lidge-jun/opencodex
Length of output: 3445
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- gui file list around tsconfig ---'
fd -t f . gui | sed -n '1,200p'
echo
echo '--- gui package/config files mentioning TypeScript or Vite ---'
rg -n '"jsx"|"allowUmdGlobalAccess"|"typescript"|tsconfig|vite' gui package.json . -g '!**/node_modules/**'Repository: lidge-jun/opencodex
Length of output: 22768
Import CSSProperties directly in gui/src/components/MemoryCard.tsx
gui/tsconfig.app.json does not enable allowUmdGlobalAccess, so React.CSSProperties at lines 372 and 383 relies on an unbound React namespace in this module and can fail tsc -b with TS2686. Use import type { CSSProperties } from "react"; and type both style objects as CSSProperties.
🤖 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/MemoryCard.tsx` around lines 372 - 383, Update
MemoryCard.tsx to import the CSSProperties type directly from React, then
replace the React.CSSProperties annotations on betaBadge and rowStyle with
CSSProperties.
| "memory.title": "Memory watchdog", | ||
| "memory.disabledHint": "Watchdog disabled. Enable it to observe memory pressure and receive warnings.", | ||
| "memory.enable": "Enable", | ||
| "memory.intro": "Observe process memory vs total RAM. Warn-only by default; auto-restart is opt-in and only fires when a supervisor is detected.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the supervisor condition conditional in every locale. requireSupervisor is user-configurable and may be false, so auto-restart is not universally supervisor-gated.
gui/src/i18n/en.ts#L8-L8: say supervisor detection is required by default or when “Require supervisor” is enabled.gui/src/i18n/de.ts#L7-L7: apply the same conditional wording.gui/src/i18n/ja.ts#L8-L8: apply the same conditional wording.gui/src/i18n/ko.ts#L8-L8: apply the same conditional wording.
📍 Affects 4 files
gui/src/i18n/en.ts#L8-L8(this comment)gui/src/i18n/de.ts#L7-L7gui/src/i18n/ja.ts#L8-L8gui/src/i18n/ko.ts#L8-L8
🤖 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/i18n/en.ts` at line 8, Update the memory.intro translation in
gui/src/i18n/en.ts at lines 8-8, gui/src/i18n/de.ts at lines 7-7,
gui/src/i18n/ja.ts at lines 8-8, and gui/src/i18n/ko.ts at lines 8-8 so
supervisor detection is described as required by default or when “Require
supervisor” is enabled, rather than universally required.
Source: Path instructions
| * SAFETY: | ||
| * - Runs the workload in a CHILD process (parent orchestrates the A/B). | ||
| * - Child self-aborts if its own RSS exceeds --rss-cap-mb (default 1200 MB). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Docstring references a --rss-cap-mb CLI flag that doesn't exist.
The header comment says the child "self-aborts if its own RSS exceeds --rss-cap-mb", but the file never parses that flag — process.argv is only checked for --child (Line 432). The actual (and only) way to configure the cap is the OCX_STRESS_RSS_CAP_MB env var read at Line 62. Anyone trying bun run scripts/memory-stress-harness.ts --rss-cap-mb=2000 would silently get the 1200 MB default instead.
📝 Proposed doc fix
- * - Child self-aborts if its own RSS exceeds --rss-cap-mb (default 1200 MB).
+ * - Child self-aborts if its own RSS exceeds OCX_STRESS_RSS_CAP_MB (default 1200 MB).Also applies to: 56-64
🤖 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 `@scripts/memory-stress-harness.ts` around lines 19 - 21, Update the header
safety documentation and related RSS-cap documentation near the environment
configuration to remove the nonexistent --rss-cap-mb CLI flag and identify
OCX_STRESS_RSS_CAP_MB as the supported configuration mechanism, including its
1200 MB default.
| let prevId: string | undefined; | ||
| let turn = 0; | ||
| for (turn = 1; turn <= CONFIG.chainTurns; turn++) { | ||
| const payload = makePayload(CONFIG.chainPayloadBytes, `chain-t${turn}-`); | ||
| const body = prevId | ||
| ? { model: MODEL, previous_response_id: prevId, input: [{ role: "user", content: payload }], store: false } | ||
| : { model: MODEL, input: [{ role: "user", content: payload }], store: false }; | ||
| const expanded = expandPreviousResponseInput(body); | ||
| const resp = buildTurnResponse(payload); | ||
| rememberResponseState(expanded, resp, undefined, { force: true }); | ||
| prevId = resp.id as string; | ||
| if (turn % CONFIG.sampleEvery === 0) { | ||
| emitSample(sample(`chain@${turn}`)); | ||
| if (overRssCap()) { | ||
| summary.aborted = true; | ||
| summary.abortReason = `RSS cap ${CONFIG.rssCapMb}MB exceeded during chain at turn ${turn}`; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| forceGc(); | ||
| { | ||
| const s = sample(`chain-final@${turn}`); | ||
| emitSample(s); | ||
| // Unique content is created once per turn (input payload + output text of ~equal size), then | ||
| // shared by reference across every later entry: ~ 2 * turns * payload. | ||
| const approxUniqueContentMb = round((2 * (turn) * CONFIG.chainPayloadBytes) / MiB); | ||
| summary.chain = { | ||
| turns: turn, | ||
| heapUsedMb: s.heapUsedMb, | ||
| storeTotalMb: s.storeTotalMb, | ||
| approxUniqueContentMb, | ||
| heapToUniqueRatio: approxUniqueContentMb > 0 ? round(s.heapUsedMb / approxUniqueContentMb) : 0, | ||
| storeToHeapRatio: s.heapUsedMb > 0 ? round(s.storeTotalMb / s.heapUsedMb) : 0, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Off-by-one: turn overshoots the actual processed count on normal loop completion.
turn is reused as the for loop's own counter (Line 224: for (turn = 1; turn <= CONFIG.chainTurns; turn++)). When the loop finishes without hitting the RSS-cap abort, the final increment runs before the exit check fails, leaving turn === CONFIG.chainTurns + 1 — one more than the number of turns actually processed. This value then flows into chain-final@${turn} (Line 244), summary.chain.turns (Line 250), and approxUniqueContentMb (Line 248), skewing the derived heapToUniqueRatio/storeToHeapRatio reported in the final table. The abort path (early break) is unaffected and reports correctly.
🔧 Proposed fix
let prevId: string | undefined;
let turn = 0;
- for (turn = 1; turn <= CONFIG.chainTurns; turn++) {
+ let processedTurns = 0;
+ for (turn = 1; turn <= CONFIG.chainTurns; turn++) {
const payload = makePayload(CONFIG.chainPayloadBytes, `chain-t${turn}-`);
const body = prevId
? { model: MODEL, previous_response_id: prevId, input: [{ role: "user", content: payload }], store: false }
: { model: MODEL, input: [{ role: "user", content: payload }], store: false };
const expanded = expandPreviousResponseInput(body);
const resp = buildTurnResponse(payload);
rememberResponseState(expanded, resp, undefined, { force: true });
prevId = resp.id as string;
+ processedTurns = turn;
if (turn % CONFIG.sampleEvery === 0) {
emitSample(sample(`chain@${turn}`));
if (overRssCap()) {
summary.aborted = true;
summary.abortReason = `RSS cap ${CONFIG.rssCapMb}MB exceeded during chain at turn ${turn}`;
break;
}
}
}
forceGc();
{
- const s = sample(`chain-final@${turn}`);
+ const s = sample(`chain-final@${processedTurns}`);
emitSample(s);
// Unique content is created once per turn (input payload + output text of ~equal size), then
// shared by reference across every later entry: ~ 2 * turns * payload.
- const approxUniqueContentMb = round((2 * (turn) * CONFIG.chainPayloadBytes) / MiB);
+ const approxUniqueContentMb = round((2 * processedTurns * CONFIG.chainPayloadBytes) / MiB);
summary.chain = {
- turns: turn,
+ turns: processedTurns,
heapUsedMb: s.heapUsedMb,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let prevId: string | undefined; | |
| let turn = 0; | |
| for (turn = 1; turn <= CONFIG.chainTurns; turn++) { | |
| const payload = makePayload(CONFIG.chainPayloadBytes, `chain-t${turn}-`); | |
| const body = prevId | |
| ? { model: MODEL, previous_response_id: prevId, input: [{ role: "user", content: payload }], store: false } | |
| : { model: MODEL, input: [{ role: "user", content: payload }], store: false }; | |
| const expanded = expandPreviousResponseInput(body); | |
| const resp = buildTurnResponse(payload); | |
| rememberResponseState(expanded, resp, undefined, { force: true }); | |
| prevId = resp.id as string; | |
| if (turn % CONFIG.sampleEvery === 0) { | |
| emitSample(sample(`chain@${turn}`)); | |
| if (overRssCap()) { | |
| summary.aborted = true; | |
| summary.abortReason = `RSS cap ${CONFIG.rssCapMb}MB exceeded during chain at turn ${turn}`; | |
| break; | |
| } | |
| } | |
| } | |
| forceGc(); | |
| { | |
| const s = sample(`chain-final@${turn}`); | |
| emitSample(s); | |
| // Unique content is created once per turn (input payload + output text of ~equal size), then | |
| // shared by reference across every later entry: ~ 2 * turns * payload. | |
| const approxUniqueContentMb = round((2 * (turn) * CONFIG.chainPayloadBytes) / MiB); | |
| summary.chain = { | |
| turns: turn, | |
| heapUsedMb: s.heapUsedMb, | |
| storeTotalMb: s.storeTotalMb, | |
| approxUniqueContentMb, | |
| heapToUniqueRatio: approxUniqueContentMb > 0 ? round(s.heapUsedMb / approxUniqueContentMb) : 0, | |
| storeToHeapRatio: s.heapUsedMb > 0 ? round(s.storeTotalMb / s.heapUsedMb) : 0, | |
| }; | |
| } | |
| let prevId: string | undefined; | |
| let turn = 0; | |
| let processedTurns = 0; | |
| for (turn = 1; turn <= CONFIG.chainTurns; turn++) { | |
| const payload = makePayload(CONFIG.chainPayloadBytes, `chain-t${turn}-`); | |
| const body = prevId | |
| ? { model: MODEL, previous_response_id: prevId, input: [{ role: "user", content: payload }], store: false } | |
| : { model: MODEL, input: [{ role: "user", content: payload }], store: false }; | |
| const expanded = expandPreviousResponseInput(body); | |
| const resp = buildTurnResponse(payload); | |
| rememberResponseState(expanded, resp, undefined, { force: true }); | |
| prevId = resp.id as string; | |
| processedTurns = turn; | |
| if (turn % CONFIG.sampleEvery === 0) { | |
| emitSample(sample(`chain@${turn}`)); | |
| if (overRssCap()) { | |
| summary.aborted = true; | |
| summary.abortReason = `RSS cap ${CONFIG.rssCapMb}MB exceeded during chain at turn ${turn}`; | |
| break; | |
| } | |
| } | |
| } | |
| forceGc(); | |
| { | |
| const s = sample(`chain-final@${processedTurns}`); | |
| emitSample(s); | |
| // Unique content is created once per turn (input payload + output text of ~equal size), then | |
| // shared by reference across every later entry: ~ 2 * turns * payload. | |
| const approxUniqueContentMb = round((2 * processedTurns * CONFIG.chainPayloadBytes) / MiB); | |
| summary.chain = { | |
| turns: processedTurns, | |
| heapUsedMb: s.heapUsedMb, | |
| storeTotalMb: s.storeTotalMb, | |
| approxUniqueContentMb, | |
| heapToUniqueRatio: approxUniqueContentMb > 0 ? round(s.heapUsedMb / approxUniqueContentMb) : 0, | |
| storeToHeapRatio: s.heapUsedMb > 0 ? round(s.storeTotalMb / s.heapUsedMb) : 0, | |
| }; | |
| } |
🤖 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 `@scripts/memory-stress-harness.ts` around lines 222 - 257, Correct the chain
completion count in the loop around `turn` so normal completion reports the
actual number of processed turns rather than the post-loop counter value.
Preserve the abort-path count, and use the corrected count consistently for
`chain-final@...`, `summary.chain.turns`, and `approxUniqueContentMb` so the
derived ratios remain accurate.
| for (const state of states.values()) { | ||
| let bytes = 0; | ||
| try { | ||
| bytes = JSON.stringify(state.items).length; | ||
| if (state.providers) bytes += JSON.stringify(state.providers).length; | ||
| } catch { | ||
| /* unserializable entry (should not happen): count as 0 rather than throw in a metrics path */ | ||
| } | ||
| totalBytes += bytes; | ||
| if (bytes > largestBytes) largestBytes = bytes; | ||
| if (state.createdAt < oldestCreatedAt) oldestCreatedAt = state.createdAt; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the store re-stores shared chain prefixes (O(N^2) growth) and the metric's call cadence.
rg -nP -C3 'items:\s*\[|pruneResponses|TTL|createdAt|MAX' src/responses/state.ts
# Confirm responseStateMetrics is invoked once per probe (not per poll/request).
rg -nP -C2 'responseStateMetrics\s*\(' src/server/memory-watchdog.ts scripts/memory-stress-harness.tsRepository: lidge-jun/opencodex
Length of output: 5366
responseStateMetrics() should avoid full JSON.stringify on every probe src/responses/state.ts:206-217 serializes each stored entry just to produce a rough metric. Since rememberResponseState() stores the full expanded turn history, longer chains duplicate shared prefixes and this path can allocate large throwaway strings under memory pressure. Use a cheaper estimate or an early cap instead.
🤖 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 `@src/responses/state.ts` around lines 206 - 217, Update responseStateMetrics()
to avoid fully JSON.stringify-ing every state.items and providers entry on each
probe. Replace the per-entry serialization with a cheaper approximate size
calculation or bounded estimation that caps work and allocations while
preserving totalBytes, largestBytes, and oldestCreatedAt metric updates.
| const b = raw as Record<string, unknown>; | ||
| const isBool = (v: unknown) => typeof v === "boolean"; | ||
| const isPosNum = (v: unknown) => typeof v === "number" && Number.isFinite(v) && v > 0; | ||
| const isFrac = (v: unknown) => typeof v === "number" && Number.isFinite(v) && v > 0 && v < 1; | ||
| if (b.enabled !== undefined && !isBool(b.enabled)) return jsonResponse({ error: "enabled must be boolean" }, 400); | ||
| if (b.autoRestart !== undefined && !isBool(b.autoRestart)) return jsonResponse({ error: "autoRestart must be boolean" }, 400); | ||
| if (b.requireSupervisor !== undefined && !isBool(b.requireSupervisor)) return jsonResponse({ error: "requireSupervisor must be boolean" }, 400); | ||
| if (b.intervalMs !== undefined && !isPosNum(b.intervalMs)) return jsonResponse({ error: "intervalMs must be a positive number" }, 400); | ||
| if (b.restartGraceMs !== undefined && (!isPosNum(b.restartGraceMs) || (b.restartGraceMs as number) < RESTART_GRACE_MIN_MS || (b.restartGraceMs as number) > RESTART_GRACE_MAX_MS)) { | ||
| return jsonResponse({ error: `restartGraceMs must be a number between ${RESTART_GRACE_MIN_MS} and ${RESTART_GRACE_MAX_MS} ms` }, 400); | ||
| } | ||
| if (b.warnFraction !== undefined && !isFrac(b.warnFraction)) return jsonResponse({ error: "warnFraction must be a number in (0,1)" }, 400); | ||
| if (b.criticalFraction !== undefined && !isFrac(b.criticalFraction)) return jsonResponse({ error: "criticalFraction must be a number in (0,1)" }, 400); | ||
|
|
||
| const patch: NonNullable<OcxConfig["memoryWatchdog"]> = {}; | ||
| if (b.enabled !== undefined) patch.enabled = b.enabled as boolean; | ||
| if (b.intervalMs !== undefined) patch.intervalMs = b.intervalMs as number; | ||
| if (b.restartGraceMs !== undefined) patch.restartGraceMs = b.restartGraceMs as number; | ||
| if (b.warnFraction !== undefined) patch.warnFraction = b.warnFraction as number; | ||
| if (b.criticalFraction !== undefined) patch.criticalFraction = b.criticalFraction as number; | ||
| if (b.autoRestart !== undefined) patch.autoRestart = b.autoRestart as boolean; | ||
| if (b.requireSupervisor !== undefined) patch.requireSupervisor = b.requireSupervisor as boolean; | ||
|
|
||
| config.memoryWatchdog = { ...config.memoryWatchdog, ...patch }; | ||
| saveConfig(config); | ||
|
|
||
| // Apply live so the change takes effect without a proxy restart. | ||
| if (patch.enabled === false) { | ||
| stopMemoryWatchdog(); | ||
| } else if (applyWatchdogRuntimeConfig(patch) === null) { | ||
| // Not currently running (was disabled / first enable) — (re)start from the saved config. | ||
| startMemoryWatchdog(config); | ||
| } | ||
| return jsonResponse({ ok: true, memoryWatchdog: config.memoryWatchdog, report: memoryWatchdogReport() ?? { enabled: false } }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject unknown and contradictory watchdog settings before persisting.
{ warnFraction: 0.8, criticalFraction: 0.7 } returns success and is saved as-is, while applyWatchdogRuntimeConfig() silently changes the live critical threshold to 0.9. The response therefore exposes conflicting persisted and resolved configurations. Typos are also accepted as successful no-ops. Validate allowed keys and the effective criticalFraction > warnFraction relation before saveConfig, preferably through one shared normalizer used by both API and runtime. Add regression cases for inverted thresholds and unknown keys.
🤖 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 `@src/server/management-api.ts` around lines 269 - 302, Validate the memory
watchdog patch before saveConfig in the management API: reject keys outside the
supported watchdog settings and reject the effective configuration when
criticalFraction is not greater than warnFraction, including partial patches
merged with existing config. Reuse or introduce a shared normalizer used by both
this API path and applyWatchdogRuntimeConfig so persisted, live, and response
configurations remain consistent, and add regression coverage for inverted
thresholds and unknown keys.
Source: Path instructions
Ingwannu
left a comment
There was a problem hiding this comment.
Memory observability is worth pursuing, but this 3.8k-line conflicting branch is not safe to merge as-is. The unresolved runtime issues include live settings overriding env-owned safety controls, string booleans arming opt-in restart, maxRestarts=0 being ignored, a non-rolling restart cap, failure to detect the built-in service supervisor, and synchronous full response-store serialization during memory pressure. The GUI also has frozen polling and invalid threshold paths. Please rebase onto the current built-in RSS watchdog, separate observability from restart policy, validate the config boundary, resolve the open findings, and return with focused exact-head tests.
Summary
Adds a memory observability layer and an opt-in, observe-first memory watchdog for the Bun-native proxy, addressing a field incident where the process reached ~75 GB private / ~97% system commit and froze the machine.
Design is intentionally conservative:
OCX_SUPERVISED=1, or pm2/systemd auto-detection).Source of truth:
structure/09_memory-watchdog.md(design rationale, platform scope, audit trail).Verification
Please confirm each is green locally before merge (repo-root commands):
bun run typecheckbun run testbun test tests/memory-watchdog.test.tsbun run lint:guibun run build:guibun run privacy:scanChecklist
dev(notmain)privacy:scangreen — no logging of request bodies, tokens, or account identifiersstructure/09_memory-watchdog.md; GUI notes)Summary by CodeRabbit
New Features
Documentation
Bug Fixes