fix(review): bound idle-review queue memory with weak parents and frozen snapshots

[kokhlo]

What

agent/review_idle_queue.ReviewIdleQueue kept every deferred review alive with two unbounded strong owners: the parent AIAgent instance and a structural clone of the whole transcript (messages_snapshot). On a busy managed-local host one such entry could sit per session for the full defer window (default 30 min), pinning agents that the cache had already evicted plus every large immutable tool-result string in the clone (#108234).

Change — all at the queue boundary, no call-site changes

• _PendingReview now holds the parent through weakref.ref (objects that cannot be weak-referenced degrade to a dead ref — the review drops at dispatch). A collected parent drops the best-effort review with a log line instead of extending the lifetime of clients, tool state and provider state.
• The already-cloned snapshot is frozen into UTF-8 JSON bytes at enqueue (json.dumps(..., ensure_ascii=False)), detaching the queue from the transcript's nested object graph. It is thawed back into a message list only after an item is popped, so dispatch semantics (_spawn_background_review_now(**kwargs)) are unchanged; the JSON round-trip preserves exact message data.
• Aggregate accounting (_snapshot_bytes) is maintained under the existing lock: coalescing replaces the previous entry's bytes, popping releases them immediately.
• Process-wide budgets (8 MiB of frozen snapshots / 16 pending sessions, constants _SNAPSHOT_BUDGET_BYTES / _SESSION_BUDGET): pressure evicts the oldest entries with a warning; a single snapshot larger than the whole budget is rejected rather than retained forever; non-serializable snapshots are rejected at enqueue. Background review stays best-effort and never outranks foreground memory ownership.
• The dispatcher now runs through _dispatch(), which keeps the existing enabled-gate recheck and adds fail-closed handling: dead parent or unreadable frozen payload → drop + warn, never poison the thread.

Tests

tests/agent/test_review_idle_queue.py grew from 19 to 28 tests, covering every regression item from the issue: frozen bytes (not the transcript list) in pending entries, weakref clearing after the last external reference dies, coalesce-replaces-accounting, byte- and session-budget eviction of the oldest entry, oversized/non-serializable rejection, dispatch round-trip reconstructing the message list with accounting back to zero, and corrupt-payload fail-closed. All pre-existing 19 tests still pass unchanged (one assertion updated to the new frozen-snapshot contract).

Fixes #108234

[kokhlo]

Sibling PR #108240 (Xipong) addresses the same issue — posting an honest comparison for the maintainer, timeline first.

Timeline

• 13:31:04Z — I claimed the issue with the diagnosis + plan (comment); the issue had no other activity at that point.
• 13:31:56Z — fix(agent): bound deferred background review memory #108240 opened (no issue comment before or since).
• 13:5xZ — this PR.

Both PRs implement the same core contract from the issue: weakref.ref parents, an 8 MiB / 16-session aggregate budget with oldest-first eviction, coalesce-replaces-accounting, and oversized-snapshot rejection before the existing entry is touched. The real difference is how retained snapshot memory is measured and held:

	this PR	#108240
Snapshot held as	frozen UTF-8 JSON bytes	the cloned object graph itself
Accounting	len(bytes) — exact, O(1)	iterative sys.getsizeof walk with identity dedup + cycle guard
Isolation from transcript graph	total (bytes share nothing)	shares immutable leaves (strings) with the old history, as _clone_background_review_messages already does
Non-JSON payload values	review rejected at enqueue (best-effort drop, logged)	still queued — any Python object survives
Dispatch	json.loads thaw (new strings allocated then)	passes the clone straight through, zero-copy
Transient memory at enqueue	one dumps peak ≈ snapshot size	walk bookkeeping only
New tests	9 (28 total in the existing spec file)	separate spec file, 114 lines

The maintainer-relevant trade-off, stated plainly: #108240 avoids the serialization peak and keeps unusual payload types alive at the cost of a graph-walking accountant; this PR pays a transient dumps peak and drops non-serializable reviews in exchange for exact O(1) accounting, total graph detachment, and a bytes payload whose size is known before admission (so the per-entry budget decision is exact rather than estimated).

Happy to defer to #108240 if the graph-accounting direction is preferred, or to rebase my regression tests onto it — the budget/eviction/weakref test surface is nearly identical and the two changes don't compose (same _PendingReview shape).

[alt-glitch]

This was generated by AI during triage.

Competing fix for #108234 alongside #108240 (earlier). Both bound the deferred background-review queue memory; only one should land.