fix(memory): bound live-log retention and persist responses/conversations#488
Conversation
…ions Coding-agent workloads (few requests, very large bodies) could grow gateway memory to multiple GB with no recovery. Two mechanisms: 1. The live-logs broker retained up to 10,000 events indefinitely, and with LOGGING_LOG_BODIES enabled every audit.updated/completed event embedded a fresh copy of the full request/response bodies (up to 1MB each) in the replay ring and active snapshots. 2. /v1/responses snapshots and /v1/conversations history were kept only in process memory regardless of STORAGE_TYPE, capped by entry count (10,000) rather than bytes, for 24h/30 days. Broker: connected dashboard subscribers still receive full previews with bodies, but the copies retained for replay strip bodies and flag them request_body_captured/response_body_captured (the dashboard already hydrates detail from the persisted entry on expand). The ring is capped at the replay limit + 1 (events older than the replay window could never be served) and retained payloads are size-capped at 64KB, bounding worst-case broker memory by construction. Stores: responsestore and conversationstore gain SQLite/PostgreSQL/ MongoDB backends (30-day TTL, hourly expired-row sweep, atomic AppendItems) wired through app shared storage, mirroring the filestore/ batch factories. The in-memory fallbacks used by embedded setups are now additionally byte-capped at 64MB with oldest-first eviction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
RunCleanupLoop existed as identical copies in auditlog and usage (and would have gained two more in responsestore/conversationstore). Move the loop to internal/storage with the interval as a parameter and point all store backends at it; each package keeps its own CleanupInterval. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds response and conversation snapshot storage backends with factories and app wiring, extracts a shared cleanup loop helper, and updates live broker retention so retained audit events stay bounded while subscriber payloads can remain fuller. ChangesResponse & Conversation Persistent Stores
Shared Cleanup Loop Utility
Bounded Live Log Retention
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| var added int64 | ||
| for _, item := range items { | ||
| conversation.Items = append(conversation.Items, core.CloneRawJSON(item)) | ||
| added += int64(len(item)) | ||
| } | ||
| s.sizes[id] += added | ||
| s.totalBytes += added | ||
| s.enforceBoundsLocked() |
There was a problem hiding this comment.
Reject oversize appends
AppendItems adds bytes and enforces eviction without checking whether the grown conversation can still fit the byte budget. When a single append makes conv_grow larger than maxBytes, enforceBoundsLocked evicts entries oldest-first and can remove conv_grow itself, but the method still returns nil; the caller believes the turn was persisted even though the conversation is now gone. Recompute the serialized size after appending and return an explicit budget error before mutating, matching Create and Update.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/conversationstore/store_postgresql.go (1)
1-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMissing test coverage for the PostgreSQL backend.
Unlike the SQLite store,
PostgreSQLStore(and the MongoDB store) ship without unit tests validating the JSONB concatenation inAppendItems, the conditional upsert (ON CONFLICT ... WHERE) semantics, or the concurrent-append atomicity claimed in the comments. Given this is a new persistence path for/v1/conversations, a table-driven or integration test (e.g., viatestcontainers-goor a similar ephemeral Postgres) would give confidence equivalent to the SQLite suite.🤖 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 `@internal/conversationstore/store_postgresql.go` around lines 1 - 184, Add PostgreSQLStore test coverage similar to the SQLite backend by creating a table-driven integration/unit test suite around NewPostgreSQLStore, Create, Update, AppendItems, and DeleteExpired. Focus on verifying the ON CONFLICT ... WHERE behavior in Create, JSONB array concatenation in AppendItems, and that concurrent AppendItems calls remain atomic and do not overwrite each other. Use the PostgreSQLStore and helper methods like prepareStoredConversationForStorage, AppendItems, and Create to locate the behavior under test, and prefer an ephemeral Postgres setup if needed.internal/conversationstore/store_persistent.go (1)
1-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing direct unit tests for new persistence helpers.
prepareStoredConversationForStorage,scanStoredConversationRow, anddecodeStoredConversationare new, non-trivial normalization/error-handling logic without a corresponding test file in this cohort (onlystore_memory_test.go/store_sqlite_test.goare mentioned in the stack outline). As per coding guidelines, "Add or update tests for behavior changes, especially for request translation, response normalization, error handling, default configuration, and provider-specific parameter mapping."🤖 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 `@internal/conversationstore/store_persistent.go` around lines 1 - 118, Add direct unit tests for the new persistence helpers in store_persistent.go: cover prepareStoredConversationForStorage, scanStoredConversationRow, and decodeStoredConversation. Verify normalization and stamping behavior for StoredAt/ExpiresAt, item handling via itemsOrEmpty, no-rows/expired-row mapping to ErrNotFound in scanStoredConversationRow, and decodeStoredConversation applying authoritative timestamps over serialized values. Use the helper names to place the tests near the existing conversationstore test coverage.Source: Coding guidelines
internal/responsestore/store_persistent.go (1)
1-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing direct unit tests for new persistence helpers.
Same gap as
conversationstore/store_persistent.go:prepareStoredResponseForStorage,scanStoredResponseRow, anddecodeStoredResponsehave no dedicated test coverage in this cohort. As per coding guidelines, "Add or update tests for behavior changes, especially for ... response normalization, error handling, default configuration...".🤖 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 `@internal/responsestore/store_persistent.go` around lines 1 - 95, Add direct unit tests for the new persistence helpers in responsestore/store_persistent.go. Cover prepareStoredResponseForStorage, scanStoredResponseRow, and decodeStoredResponse with cases for response ID validation, stamp behavior for StoredAt/ExpiresAt defaults, marshal/unmarshal error handling, no-rows mapping to ErrNotFound, and expired-row filtering. Use the helper names themselves to place the tests alongside the existing responsestore persistence coverage.Source: Coding guidelines
🤖 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 `@internal/conversationstore/store_mongodb.go`:
- Around line 181-189: The unexpiredFilter helper is duplicated across
conversation and response Mongo stores, so factor the expiry predicate into a
shared helper in internal/storage or a small internal mongo-helpers package and
have both store implementations call that shared function. Update the existing
unexpiredFilter usage in store_mongodb.go and the matching helper in
internal/responsestore/store_mongodb.go so the expiry semantics live in one
place and cannot drift.
In `@internal/conversationstore/store_persistent.go`:
- Around line 56-59: The helper types/functions in conversationstore are
duplicated with responsestore, so consolidate them into shared storage utilities
instead of keeping separate copies. Move rowScanner, unixTime, and unixOrZero
into internal/storage as shared exported helpers, then update both
conversationstore/store_persistent.go and responsestore/store_persistent.go to
reference the shared versions. Use the existing internal/storage pattern from
RunCleanupLoop to keep backend-specific code aligned across both packages.
In `@internal/storage/cleanup.go`:
- Around line 8-22: Add a table-driven test for RunCleanupLoop to cover the
shared cleanup behavior now used by auditlog and usage stores. Verify the
initial immediate cleanupFn invocation, repeated calls on each ticker tick for a
short interval, and that no further calls happen after stop is closed. Place the
test near internal/storage/cleanup.go and reference RunCleanupLoop directly so
regressions in this utility are caught for all callers.
---
Outside diff comments:
In `@internal/conversationstore/store_persistent.go`:
- Around line 1-118: Add direct unit tests for the new persistence helpers in
store_persistent.go: cover prepareStoredConversationForStorage,
scanStoredConversationRow, and decodeStoredConversation. Verify normalization
and stamping behavior for StoredAt/ExpiresAt, item handling via itemsOrEmpty,
no-rows/expired-row mapping to ErrNotFound in scanStoredConversationRow, and
decodeStoredConversation applying authoritative timestamps over serialized
values. Use the helper names to place the tests near the existing
conversationstore test coverage.
In `@internal/conversationstore/store_postgresql.go`:
- Around line 1-184: Add PostgreSQLStore test coverage similar to the SQLite
backend by creating a table-driven integration/unit test suite around
NewPostgreSQLStore, Create, Update, AppendItems, and DeleteExpired. Focus on
verifying the ON CONFLICT ... WHERE behavior in Create, JSONB array
concatenation in AppendItems, and that concurrent AppendItems calls remain
atomic and do not overwrite each other. Use the PostgreSQLStore and helper
methods like prepareStoredConversationForStorage, AppendItems, and Create to
locate the behavior under test, and prefer an ephemeral Postgres setup if
needed.
In `@internal/responsestore/store_persistent.go`:
- Around line 1-95: Add direct unit tests for the new persistence helpers in
responsestore/store_persistent.go. Cover prepareStoredResponseForStorage,
scanStoredResponseRow, and decodeStoredResponse with cases for response ID
validation, stamp behavior for StoredAt/ExpiresAt defaults, marshal/unmarshal
error handling, no-rows mapping to ErrNotFound, and expired-row filtering. Use
the helper names themselves to place the tests alongside the existing
responsestore persistence coverage.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ec01707-bb6e-4b60-b5e6-55f70fa84e07
📒 Files selected for processing (31)
.env.templateCLAUDE.mdinternal/app/app.gointernal/auditlog/cleanup.gointernal/auditlog/store_postgresql.gointernal/auditlog/store_sqlite.gointernal/conversationstore/factory.gointernal/conversationstore/store.gointernal/conversationstore/store_memory.gointernal/conversationstore/store_memory_test.gointernal/conversationstore/store_mongodb.gointernal/conversationstore/store_persistent.gointernal/conversationstore/store_postgresql.gointernal/conversationstore/store_sqlite.gointernal/conversationstore/store_sqlite_test.gointernal/live/broker.gointernal/live/broker_test.gointernal/responsestore/factory.gointernal/responsestore/store.gointernal/responsestore/store_memory.gointernal/responsestore/store_memory_test.gointernal/responsestore/store_mongodb.gointernal/responsestore/store_persistent.gointernal/responsestore/store_postgresql.gointernal/responsestore/store_sqlite.gointernal/responsestore/store_sqlite_test.gointernal/server/handlers.gointernal/storage/cleanup.gointernal/usage/cleanup.gointernal/usage/store_postgresql.gointernal/usage/store_sqlite.go
💤 Files with no reviewable changes (2)
- internal/usage/cleanup.go
- internal/auditlog/cleanup.go
| func RunCleanupLoop(stop <-chan struct{}, interval time.Duration, cleanupFn func()) { | ||
| ticker := time.NewTicker(interval) | ||
| defer ticker.Stop() | ||
|
|
||
| cleanupFn() | ||
|
|
||
| for { | ||
| select { | ||
| case <-ticker.C: | ||
| cleanupFn() | ||
| case <-stop: | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider adding a table-driven test for the new shared utility.
This helper is now the single implementation backing cleanup for auditlog and usage (Postgres/SQLite) stores, so a regression here silently breaks retention for all callers. A short-interval test (e.g., verifying immediate invocation, N ticks before stop, and no further calls after stop is closed) would be cheap and catch regressions early.
As per coding guidelines, **/*.go: "Add or update tests for behavior changes... Prefer table-driven tests for Go behavior coverage."
🤖 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 `@internal/storage/cleanup.go` around lines 8 - 22, Add a table-driven test for
RunCleanupLoop to cover the shared cleanup behavior now used by auditlog and
usage stores. Verify the initial immediate cleanupFn invocation, repeated calls
on each ticker tick for a short interval, and that no further calls happen after
stop is closed. Place the test near internal/storage/cleanup.go and reference
RunCleanupLoop directly so regressions in this utility are caught for all
callers.
Source: Coding guidelines
… loop Review follow-ups on #488: move the retention-column helpers duplicated across responsestore and conversationstore (RowScanner, UnixTime, UnixOrZero, the Mongo unexpired filter) into internal/storage so the expires_at semantics live in one place, and add a test for RunCleanupLoop covering the immediate first run, per-tick reruns, and exit on stop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 `@internal/storage/cleanup_test.go`:
- Around line 34-48: The final “no cleanup after stop” check in cleanup_test.go
is unreachable because the drain loop on calls already consumes every queued
cleanup invocation after RunCleanupLoop exits. Update the test around the
RunCleanupLoop/done/calls logic to verify stop behavior by counting the total
number of cleanupFn invocations once after closing done, then assert that no
additional calls arrive beyond that count rather than performing a second
empty-channel check.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: be963c0d-bbd2-4390-91e6-78a4721c5244
📒 Files selected for processing (10)
internal/conversationstore/store_mongodb.gointernal/conversationstore/store_persistent.gointernal/conversationstore/store_postgresql.gointernal/conversationstore/store_sqlite.gointernal/responsestore/store_mongodb.gointernal/responsestore/store_persistent.gointernal/responsestore/store_postgresql.gointernal/responsestore/store_sqlite.gointernal/storage/cleanup_test.gointernal/storage/storehelpers.go
| // Drain anything sent before the loop observed stop; with the goroutine | ||
| // gone, no further cleanups can run. | ||
| for { | ||
| select { | ||
| case <-calls: | ||
| continue | ||
| default: | ||
| } | ||
| break | ||
| } | ||
| select { | ||
| case <-calls: | ||
| t.Fatal("cleanup ran after the loop exited") | ||
| default: | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Final "no cleanup after stop" assertion is dead code and can never fail.
By the time done is closed (line 29), RunCleanupLoop has already returned, so every cleanupFn() send it will ever make has already landed in calls. The drain loop at lines 36-43 exhausts the channel completely (it loops until it hits default), so the subsequent check at lines 44-48 will always hit default — it's unreachable and provides no real signal that cleanup didn't fire after stop.
This silently defeats the test's stated purpose of verifying stop behavior with no lingering calls.
🧪 Suggested fix: count total invocations once, after the loop has exited
- // Drain anything sent before the loop observed stop; with the goroutine
- // gone, no further cleanups can run.
- for {
- select {
- case <-calls:
- continue
- default:
- }
- break
- }
- select {
- case <-calls:
- t.Fatal("cleanup ran after the loop exited")
- default:
- }
+ // By the time done is closed, every cleanupFn call the loop will ever
+ // make has already been sent. Count them all in one pass: 3 expected
+ // (initial + 2 ticks already drained above), plus at most one extra
+ // from a benign race between the final tick and stop being observed.
+ extra := 0
+ for {
+ select {
+ case <-calls:
+ extra++
+ default:
+ if extra > 1 {
+ t.Fatalf("expected at most 1 extra cleanup call after stop, got %d", extra)
+ }
+ return
+ }
+ }📝 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.
| // Drain anything sent before the loop observed stop; with the goroutine | |
| // gone, no further cleanups can run. | |
| for { | |
| select { | |
| case <-calls: | |
| continue | |
| default: | |
| } | |
| break | |
| } | |
| select { | |
| case <-calls: | |
| t.Fatal("cleanup ran after the loop exited") | |
| default: | |
| } | |
| // By the time done is closed, every cleanupFn call the loop will ever | |
| // make has already been sent. Count them all in one pass: 3 expected | |
| // (initial + 2 ticks already drained above), plus at most one extra | |
| // from a benign race between the final tick and stop being observed. | |
| extra := 0 | |
| for { | |
| select { | |
| case <-calls: | |
| extra++ | |
| default: | |
| if extra > 1 { | |
| t.Fatalf("expected at most 1 extra cleanup call after stop, got %d", extra) | |
| } | |
| return | |
| } | |
| } |
🤖 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 `@internal/storage/cleanup_test.go` around lines 34 - 48, The final “no cleanup
after stop” check in cleanup_test.go is unreachable because the drain loop on
calls already consumes every queued cleanup invocation after RunCleanupLoop
exits. Update the test around the RunCleanupLoop/done/calls logic to verify stop
behavior by counting the total number of cleanupFn invocations once after
closing done, then assert that no additional calls arrive beyond that count
rather than performing a second empty-channel check.
…shot Review follow-ups on #488: an AppendItems call that grew a conversation past the memory store's byte budget made oldest-first eviction remove the very conversation just appended to — while still returning success. Oversize growth is now rejected before mutating (mirroring Create and Update), and enforceBoundsLocked skips the entry the caller just wrote in both memory stores, which the byte-budget checks guarantee fits on its own. Update had the same self-eviction exposure when the updated entry was the oldest. Also drop the unreachable post-exit assertion from the RunCleanupLoop test — once the loop goroutine has returned, silence is guaranteed by construction, and bounding ticks that race the stop signal would be timing-dependent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r reads (#497) * fix(lifecycle): close leaked goroutines, sessions, and unbounded error reads Four lifecycle leaks from the memory investigation (follow-up to #488/#489): - llmclient now closes RawBodyReader on every error return that precedes the HTTP transport (circuit breaker open, request build failure), mirroring net/http's Do contract. Pipe-backed uploads (files, audio transcription) previously leaked their producer goroutine and the full upload buffer forever when the breaker rejected the request — exactly the agent retry-storm-during-outage pattern. - The audit middleware publishes the terminal live event when a handler panic unwinds through it (then re-panics for the outer recover). Previously the panic skipped all terminal events, permanently orphaning the entry in the live-dashboard active-snapshot state. - The realtime websocket relay gains a heartbeat (30s ping, 10s timeout, both peers). A silently dead peer — NAT timeout, power loss, no RST — left both copy loops blocked in Read forever, leaking two goroutines and both connections per session. A ping that fails because the connection is already locally closed defers to the copy loop's close cause instead of racing to report a secondary error. - Upstream error bodies are read through a 64KB LimitReader (matching core's audit capture cap) in DoStream's non-200 path and doRequest's error-status path, so a misbehaving provider answering errors with an endless body is no longer buffered whole. Successful response reads are unchanged. Also fixes TestProxyRelaysLargeFrame leaking its hijacked session past the test boundary (Accept hijacks the connection, so httptest's Close does not wait for the relay goroutines). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(realtime): give each heartbeat ping its own timeout budget Review follow-ups on #497: the shared pingBoth deadline let a slow-but-alive client ping leave the upstream ping a nearly expired budget, spuriously blaming a healthy upstream. Each peer now gets its own full heartbeatTimeout. Also widen the responsive-session test's pong timeout (150ms -> 2s) and loop window so scheduler stalls on loaded CI runners cannot fail a healthy session. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Problem
A user running coding-agent workloads (few requests/day, very large streamed bodies) reported gateway memory growing to ~3GB with no recovery, while a high-request/small-body instance stayed under 150MB. A full audit of the codebase found two retention mechanisms that scale with body size rather than request count:
Live-logs broker (
internal/live/broker.go): the replay ring held up to 10,000 events with no TTL or byte bound. WithLOGGING_LOG_BODIES=true, everyaudit.updated/audit.completedevent embedded a fresh serialized copy of the full request/response bodies (up to 1MB each), and each request publishes several such events. At low request rates the ring never wraps — a few hundred large requests retain multiple GB indefinitely. The ring was also 10× larger than the replay limit, so 9,000 slots could never be served to any client./v1/responsessnapshots and/v1/conversationshistory lived only in process memory regardless ofSTORAGE_TYPE—SetResponseStore/SetConversationStorewere never wired, and no persistent backends existed. Retention was capped by entry count (10,000), not bytes, for 24h/30 days. Agentic clients resend cumulative history each turn, so per-chain memory grows quadratically.Changes
Live-logs broker
request_body_captured/response_body_capturedflags; the dashboard already hydrates full detail from the persisted audit entry on expand (same mechanism attempt bodies use).DASHBOARD_LIVE_LOGS_REPLAY_LIMIT + 1and retained payloads are size-capped at 64KB, so worst-case broker memory is bounded by construction (~2–5MB typical).Responses/Conversations persistence
responsestoreandconversationstore, mirroring the filestore/batch factory pattern, wired through app shared storage. 30-day TTL with an hourly expired-row sweep.AppendItemsis atomic on all backends (chainedjson_insert('$[#]')for SQLite,jsonb ||for Postgres,$push $eachfor Mongo), so concurrent turns cannot lose each other's items.Refactor
RunCleanupLoopexisted as identical copies in auditlog and usage; consolidated intointernal/storageand reused by the new store backends.User-visible impact
/v1/responsesand conversations survive restarts and followSTORAGE_TYPE. Stored-response retention becomes 30 days (OpenAI parity) instead of 24h in-memory.response_snapshots,conversation_snapshots.Verification
/v1/responsescall persists its snapshot to SQLite (expiry = stored + 30 days) and the lifecycle GET serves it back; with body logging on, ring replay contains zero bodies (flags only) while a connected SSE subscriber still receives them live.Docs updated:
.env.templateandCLAUDE.md(live-logs buffer semantics, storage note).🤖 Generated with Claude Code
Summary by CodeRabbit