Record working-set memory in resource history - #2184
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Repository: raphaeltm/simple-agent-manager/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
UI evidence — Session resource history drawerExact deployed Pages bundle rendering the captured fresh-VM staging summary. Desktop (1280×800) Mobile (375×667) Reviewed for hierarchy, wrapping, overflow, clipping, readability, and responsive behavior. No visual issues found. The displayed 427 MB peak, 280 MB mean, 1.2 GB cache-inclusive total, and 53 samples match the real-VM captured summary. |
|
@coderabbitai review |
524a30e to
aa21d7c
Compare
|
|
@coderabbitai review |
1 similar comment
|
@coderabbitai review |





Summary
memory.current - inactive_file, clamped at zero, while retaining the cache-inclusive current and kernel peak values for historical comparison.null/—) and cannot overwrite known aggregates with zero.A cache-heavy staging workload demonstrated the sizing impact: 427 MB working-set peak versus 1.2 GB cache-inclusive sampled peak.
Validation
pnpm lint(API and web; web has 3 existing warnings and 0 errors)pnpm typecheck(API and web)go test -race ./internal/resourcehistory,go vet, andgofmtpassed before the final main rebase; the rebase did not change Go sourcepnpm quality:migration-safety— 205 relationships, 0 violationsStaging Verification (REQUIRED for all code changes — merge-blocking)
36550714488passed for working-set commit8f07e3712Staging Verification Evidence
Fresh staging node
01M3P9M7KHACV1420HJEKEV82Pran a 128 MiB file-cache workload. The final flush retained 53 samples, including 24 known working-set values; every known value satisfied0 <= working set <= memory.current. Mean working set was 293,991,082 bytes (280 MB), peak working set was 447,438,848 bytes (427 MB), and cache-inclusive current peak was 1,293,877,248 bytes (1.2 GB). The test session, workspace, and node were deleted; the staging node list was empty at handoff.Evidence: captured summary, verification notes.
The final rebase integrated merged attribution PR #2181 and updated its SQLite fixtures; the working-set collector source verified on staging did not change.
UI Compliance Checklist (Required for UI changes)
UI Screenshot Evidence
Surface: Session resource history drawer
End-to-End Verification (Required for multi-component changes)
Data Flow Trace
packages/vm-agent/internal/resourcehistory/collector.go:readCgroupCountersreadsmemory.current,memory.peak, and strictinactive_file;summarizeemits known-only mean/peak/count.apps/api/src/services/workspace-resource-history.ts:storeWorkspaceResourceChunkvalidates upload metadata, stores the compressed chunk in R2, and aggregates nullable summary fields in D1 without absence overwrites.getWorkspaceResourceHistoryreturns nullable summary fields and downsampled detail samples through project HTTP routes.apps/api/src/routes/mcp/session-tools.tsandapps/api/src/durable-objects/sam-session/tools/get-resource-history.tsexpose the same result to both MCP implementations.apps/web/src/lib/api/sessions.tstypes the response;SessionResourceHistoryDrawer.tsxrenders working set as memory needed and current memory as cache-inclusive total.Untested Gaps
N/A: automated tests cover each storage/read boundary; fresh-VM staging covers collector upload and the deployed UI.
Post-Mortem (Required for bug fix PRs)
What broke
Session RAM peaks treated reclaimable page cache as required memory, so file-heavy tasks appeared to need much larger VMs.
Root cause
The original resource-history implementation recorded only cgroup
memory.currentandmemory.peak, both of which include reclaimable cache; it had no working-set signal.Class of bug
Resource-accounting semantic mismatch between an observable kernel total and the sizing value consumers need.
Why it wasn't caught
Collector fixtures and end-to-end tests did not distinguish inactive file cache from non-reclaimable memory.
Process fix included in this PR
Realistic
memory.statfixtures, mixed-version aggregation tests, an HTTP/D1/R2 vertical slice, legacy UI cases, and fresh-VM cache-heavy staging evidence now enforce the distinction. Existing rule 73 already defines absence semantics, so no new standing rule was needed.Post-mortem file
Task and findings
Specialist Review Evidence (Required for agent-authored PRs)
needs-human-reviewlabel added and merge deferred to human — N/A; every reviewer completedCodeRabbit Review Evidence (Required for agent-authored PRs)
coderabbit-reviewlabel applied after local review, staging, and CI gates passedCodeRabbit Notes
The visible green
CodeRabbitstatus is not usable review evidence. CodeRabbit edited its existing summary at 2026-09-29 12:40:54 UTC with the exact result: “Review skipped — Bot user detected” (run ID0d55d980-fca5-42ac-bf2e-722c245b7d5c). GitHub shows zero submitted CodeRabbit reviews and zero review threads for the PR.The repository-compliant trusted workflow run 36567341700 succeeded from
mainand posted an owner-authored@coderabbitai reviewcommand at 2026-09-29 12:19:48 UTC. CodeRabbit nevertheless classified that request as bot-authored and skipped it. Earlier owner-authored requests at 10:48 and 11:27 UTC also produced no review. This is the vendor-provided reason; no additional human-only or permissions explanation is inferred.Per the repository
/domerge gate, the trusted request did not produce usable review evidence. Theneeds-human-reviewlabel is applied and merge is deferred pending a PR-specific waiver or a real CodeRabbit review. The request is not being repeated again without a materially new ready state or a passed rate-limit window.Exceptions (If any)
Agent Preflight (Required)
Classification
External References
No external API was changed. The implementation follows the established cAdvisor/kubelet working-set definition named in the task and repository guidance.
Codebase Impact Analysis
packages/vm-agentcollection and upload schema;apps/apiD1/R2 storage, HTTP, and MCP reads;apps/webtypes and drawer;apps/wwwguide/API docs; migration 0176.Documentation & Specs
Updated
apps/www/src/content/docs/docs/guides/session-resources.md,apps/www/src/content/docs/docs/reference/api.md, their screenshots, and the active task/evidence record.Constitution & Risk Check
Checked nullable-field preservation, additive migration safety, bounded retention, no hardcoded configuration, privacy of retained payloads, and old/new agent rollout compatibility. The internal known-sample count prevents old-agent chunks from biasing the working-set mean.