Repository navigation
Conversation
A turn is a single runTopLevelAgentLoop call that can run for hours, so the end-of-turn snapshot — and the cleanup that runs right after it — never ran. The log grew without bound while every state load re-read it: the incident session reached 320k events / 909 MB of payload behind a 95 KB snapshot, and a single state load cost ~12 s of synchronous better-sqlite3 work several times a minute, pinning the main thread at 98% of a core for 9h30 and starving the HTTP/WS loop until the UI could no longer stop the session. Bounded writes: - snapshot on a byte (48 MB) / time (10 min) cadence during the turn, at the end of every workflow step and inside sub-agent loops, pruning what the snapshot absorbed. Snapshots only happen at a quiescent point (no message streaming, no tool call awaiting its result) — the context fold drops later events for a messageId already in the snapshot. - throttle streamed tool.preparing events to one per 250 ms per tool call; they re-send the accumulated argument JSON, which produced 146k events for 691 real calls. Bounded reads: - the context-window fold reads only the four event types that can change the window id (6.9 s -> 1.4 ms on the incident session) - the conversation build replays the latest snapshot instead of the raw log - the user-message count is a SQL COUNT instead of a full materialization - the snapshot builder folds the previous snapshot plus the tail Store: - add the covering index (session_id, event_type, seq) and drop the two redundant ones; the snapshot lookup walked 320k index entries per call (105 ms -> <1 ms) - stop evicting the parsed snapshot cache on non-snapshot appends Observability: - warn when a state load exceeds 1 s (throttled) or when the database exceeds 300 MB at startup; the incident produced no log line at all.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Nested cadence state can snapshot unresolved parent tool calls, while message counting and byte accounting are inaccurate after pruning.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Bounds event-log growth and improves session-state read performance during long-running turns.
Changes:
- Adds periodic snapshotting, pruning, and streamed-event throttling.
- Optimizes event queries, indexes, caching, and observability.
- Expands regression coverage and debugging documentation.
| File | Description |
|---|---|
src/server/workflows/executor.ts |
Adds workflow snapshot cadence. |
src/server/workflows/executor-subgroup.test.ts |
Updates event-store mocks. |
src/server/workflows/executor-retry.test.ts |
Updates retry test mocks. |
src/server/workflows/executor-resume-nudge.test.ts |
Updates resume test mocks. |
src/server/workflows/executor-mode.test.ts |
Updates mode test mocks. |
src/server/utils/session-utils.ts |
Moves message counting into SQL. |
src/server/sub-agents/manager.ts |
Adds sub-agent snapshot cadence. |
src/server/queue/processor.test.ts |
Mocks SQL message counting. |
src/server/events/store.ts |
Optimizes queries, indexes, caches, and diagnostics. |
src/server/events/store.test.ts |
Tests storage optimizations. |
src/server/events/session.ts |
Adds bounded context-window reads. |
src/server/events/session.test.ts |
Tests filtered context reads. |
src/server/db/migrations.test.ts |
Tests index migration. |
src/server/db/index.ts |
Uses shared event-index DDL. |
src/server/db/events-schema.ts |
Defines shared event indexes. |
src/server/chat/system-reminder.test.ts |
Updates event-store mocks. |
src/server/chat/stream-pure.ts |
Throttles preparing events. |
src/server/chat/stream-pure.test.ts |
Tests preparing-event throttling. |
src/server/chat/snapshot-cadence.ts |
Implements snapshot cadence and pruning. |
src/server/chat/snapshot-cadence.test.ts |
Tests cadence behavior. |
src/server/chat/orchestrator.ts |
Integrates cadence into chat turns. |
src/server/chat/orchestrator.test.ts |
Updates orchestrator mocks. |
src/server/chat/conversation-history.ts |
Replays snapshots plus event tails. |
src/server/chat/agent-loop.test.ts |
Updates context-read mocks. |
src/server/chat/agent-loop-pause.test.ts |
Updates pause-test mocks. |
docs/SESSION-DEBUGGING.md |
Documents the covering index. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Anything already in flight at this point predates this cadence (a parent | ||
| // turn's message, a tool the caller is waiting on) — only new opens block. | ||
| const atRest = eventStore.getEventLogTail(sessionId, state.lastSnapshotSeq) | ||
| state.openMessagesAtRest = atRest.openMessages | ||
| state.pendingToolCallsAtRest = atRest.pendingToolCalls |
| const row = this.db | ||
| .prepare( | ||
| `SELECT COUNT(*) AS events, | ||
| COALESCE(SUM(LENGTH(payload)), 0) AS bytes, |
| `SELECT COUNT(*) AS n FROM events | ||
| WHERE session_id = ? AND event_type = 'message.start' | ||
| AND json_extract(payload, '$.role') = 'user' | ||
| AND COALESCE(json_extract(payload, '$.isSystemGenerated'), 0) = 0`, |
|
Hi @co-l — first PR here. The For context: #400 documents the incident this fixes, and I've already built and run this branch against my production database — the mid-turn snapshot cadence is confirmed there (details in the PR body). Happy to change anything you'd rather see done differently. |
Three issues raised in review of co-l#401, all verified against the code. Byte accounting: LENGTH() on a TEXT value counts characters, not bytes, so a non-ASCII payload (accents, CJK, emoji) understated the un-snapshotted tail by up to 3x — measured 1.15x for accented French, 3.00x for CJK — and let the 48 MB cadence budget overshoot before a snapshot was taken. Measure the BLOB representation instead. User-message count: real user messages live in the latest snapshot as well as in the raw rows, because cleanupOldEvents() does not whitelist message.start (unlike mode.changed or context.state) and therefore deletes the rows once a snapshot absorbs them. Counting only the raw rows collapsed the count to ~0 after every prune, re-arming the "is this the first user message?" condition that drives session naming. Count the snapshot's messages plus the rows after its seq, and restore the tombstone exclusion the previous getEvents()-based implementation had. Mid-turn tool results: a mid-turn snapshot can capture a tool call whose result has not been emitted yet — a sub-agent's cadence fires while its parent turn is still awaiting that call (the parent's in-flight call became the nested cadence's baseline, so it does not defer the snapshot), and cleanupOldEvents() then deletes the parent's tool.call row. buildContextMessagesFromEventHistory() dropped the parent's later tool.result because its messageId was already in the snapshot, which left the call unfulfilled; stripOrphanedToolCalls() then removed the call from the message entirely, silently losing the sub-agent's return value from the parent's context. Keep an event that completes the snapshot rather than duplicating it. Regression tests added for all three.
|
Thanks for the review — all three findings were valid, and I verified each against the code before changing anything. Nested cadence pruning the parent's tool call (High) — confirmed. Fixed on the replay side: an event that completes the snapshot (a UTF-8 byte length (Medium) — confirmed empirically. Snapshot-embedded user messages (Medium) — confirmed. Regression tests added for all three. Full suite: 443 files / 5,986 tests passing, |
|
Hi @mossaab, thanks for this PR. I've been running it with a local model (llama.cpp, Qwen3-based) on long autonomous turns with sub-agents, and found four issues: one makes the database grow without bound, and in the other three a long sub-agent run never gets a mid-turn snapshot, or loses its context once one is taken. Sharing them here since they're in this PR's code. I have fixes with tests for all four, and can push them on top of your branch, open a follow-up PR once this one is merged, or just leave them to you, whichever you and @co-l prefer. 1. Every snapshot doubles the compaction history (most severe)
Fix: when the fold seeds from a snapshot, keep one entry per compaction / retry (same windows or attempt, same timestamp). This also shrinks a snapshot already inflated this way at the next snapshot (here 433 MB → 2.7 MB; the DB went from 625 MB to 5 MB after a VACUUM). 2. Every compaction leaves one message "open" in the tail count A successful compaction appends Fix: count distinct 3. A deferred check waits a full period again When a check finds the tail over budget but the turn busy, it still resets Fix: once deferred, the next quiet point ( 4. The sub-agent's context ignores the snapshot
Verified live with fixes 2–4 and |
|
Thanks — this is a much more useful review than the PR deserves. Running it with a local model on long autonomous turns with sub-agents is the configuration I couldn't test. Four bugs with tests is more work than the change itself. Yes to option 1: please push 1–3 on top of my branch. Fixes 2 and 3 aren't optional for me — they're what makes the cadence fire at all. Your own numbers make the point: 7 and 12 minute sub-agent runs, tail over budget, no mid-turn snapshot. Landing it without them means landing something that often doesn't fire. # 4 as the follow-up you suggested makes sense. One framing point on # 1, because it changes who owns the fix. You place the replay at #373, which matches the base I have installed: I see the synthetic events being pushed ahead of the snapshot, and the fold reseeding from turn.snapshot on top of them. So the doubling is pre-existing, and the cadence takes it from occasional to per-turn. Worth doing as its own fix against the base and landing first — it shrinks snapshots that are already inflated, independent of this PR. One implementation note for 1 and 4: the replayed events are stubs, and for # 4 the stub is missing more than the summary. A replayed context.compacted carries the window ids and timestamp but summary is empty, and no subAgentId at all — so it is indistinguishable from a main-turn compaction for anything filtering on that field. A replayed pattern.retry keeps only attempt and maxAttempts. Which I think is why your dedupe key is (windows or attempt, timestamp): those are the only fields that survive. For # 4 it means the boundary has to come from the snapshot's messages rather than from the replayed event. A gap on my side: I can't reproduce # 1. 92% of my database is turn.snapshot payload (630 MB of 684 MB), but the large snapshots are large because of message count, not duplicated history — I find zero Two questions:
Happy with whichever you and @co-l prefer — my preference is 1–3 here, then # 4 once your sub-agent compaction fix lands. |
|
Thanks, and good questions. Short answers first, details below. #1: the doubling needs this PR, it doesn't happen on the base alone. You're right that both halves exist on I ran the same test on both: one
That probably explains why you see zero duplicates: the snapshots in your DB were written by the base, or by sessions with no compaction since you installed this branch. To reproduce: run SELECT seq, json_array_length(json_extract(payload, '$.contextWindows')) AS entries
FROM events WHERE session_id = ? AND event_type = 'turn.snapshot' ORDER BY seq;So I'd keep it in this PR, as its own commit before the cadence ones. The dedupe also shrinks snapshots that are already inflated, which matters for anyone who has run this branch. Stubs: agreed, that's exactly why the key is (windows or attempt, timestamp), since nothing else survives. And for #4, the boundary does come from the snapshot's messages: the fix treats a replayed sub-agent summary message as the boundary, not the stub event. Q1, distinct messageIds: you're right, a set is safer. Counting distinct starts minus distinct dones lets a done whose start is before Q2, cost at 400 MB: measured on a synthetic tail (assistant messages with deltas and tool rounds, ~3.5 KB payloads):
The four counts use Next step: I can't push to your fork directly, so I'll open a PR against |
|
Thanks — and you're right, I had # 1 wrong. I argued the doubling was pre-existing because both halves are on develop, but I never checked whether anything there feeds the combined stream into a snapshot. So # 1 stays here, as its own commit before the cadence ones. Your 1 → 1 → 1 → 1 versus 1 → 2 → 4 → 8 settles it, and I'll run your query on my side to confirm before merging. Both answers land, and the set-based count being faster as well as correct is a nice result — I'll take it. On the O(1) offer, I'd rather keep the query. An in-memory counter is only correct if the cadence observes every mutation of the tail, and it doesn't: Yes please to the PR against |
A snapshot is built from the previous one plus the events since (combineEventsWithSnapshot). That stream also replays the previous snapshot's compactions and format retries as synthetic events, just before it, for the consumers that count them; the fold then seeded its history from the snapshot on top of them, so each snapshot doubled the history and every later one doubled it again. With the mid-turn cadence snapshotting often, it exploded: seen live, 7 compactions stored as 2.3 million entries in a 433 MB snapshot, rewritten at every turn end (8 s per write, a prompt took that long to show) until the disk was full. The fold keeps one entry per compaction or retry (same windows or attempt, same timestamp) when it seeds from a snapshot. This also shrinks a snapshot already inflated that way on the next snapshot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A successful compaction starts its summary attempt's message again with the summary as content, then closes it once. The tail counted starts minus dones, so every compaction left one message open for good: the snapshot cadence then found the turn busy at every check and never took a mid-turn snapshot for the rest of the turn (main turn or sub-agent). The tail now counts the ids started and not done in it (and tool calls without a result), instead of subtracting counts: a message started twice counts once, and a done or result whose start is before the tail can no longer hide another open message or pending call. The counts read only their own event types through the session/type/seq index; the check measured 30 ms on a 48 MB tail (the default budget), 64 ms on 100 MB, 269 ms on 400 MB, dominated by summing the tail's payload bytes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The cadence measures the tail at most every 30 s, on whatever event comes in, and only snapshots at a quiet point (no message streaming, no tool round in flight). A deferred check still restarted the 30 s period, and a sub-agent is nearly always streaming or running a tool: the check kept landing on a busy moment and the snapshot was put off to the end of the turn. Seen live on 7 and 12 minute sub-agent runs, the tail over its budget and no mid-turn snapshot (together with the open-message miscount fixed in the next commit). A check that finds the tail over budget while the turn is busy now marks the cadence deferred, and the next quiet point (a message.done or a tool.result) checks again without waiting for the period, at most every 5 s: each check scans the tail (~0.1 s at 46 MB), and parallel tools or sub-agents can keep the turn busy over many quiet points in a row. The budget is evaluated before the in-flight test, so only an over-budget tail defers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
One correction on my side: the set-based count isn't faster, I compared the wrong numbers. The 3 ms was the open-message subquery alone, the 66 ms the whole tail check. Measured end to end, the check costs about the same either way (30 ms at 48 MB, 269 ms at 400 MB on an idle machine, as in the PR description): it's more correct, not faster. Agreed on keeping the query rather than in-memory counters, for the reasons you give. |
`x NOT IN (subquery)` is never true when the subquery returns a NULL: a single message.done or tool.result without an id in the tail would read openMessages and pendingToolCalls as 0, and the cadence would snapshot mid-message. The subqueries now skip NULL ids. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Update: the follow-ups from @vv7r are in — 4 commits, fast-forwarded onto this branch (no merge commit), tip
Items 2 and 3 are what make the cadence actually fire during a long sub-agent run, so this branch is materially different from the one you last saw — worth a re-read rather than a look at the appended commits only. Heads-up: the |
|
I had Qwen3.8-27B independently review it, and the implementation is solid. I am currently conducting real-world testing. |
|
It works well. |
|
The task I am testing has currently completed 16 rounds of compression and is working perfectly. |
Summary
Fixes #400.
A turn is a single
runTopLevelAgentLoopcall that can run for hours, so the end-of-turn snapshot — and thecleanupOldEvents()that runs right after it — never ran while the turn was alive. The event log grewwithout bound while every state load re-read it: on the reporter's instance one session reached 320k
events / 909 MB of payload behind a 95 KB snapshot, costing ~12 s of synchronous
better-sqlite3workseveral times a minute. That pinned the main thread at 98% of a core for 9.5 h and starved the HTTP/WS loop
until the UI could no longer stop the session.
Bounded writes
plus end-of-turn and abort, pruning what the snapshot absorbed. Snapshots are only taken at a quiescent
point (no message streaming, no tool call awaiting its result) — the context fold drops later events for a
messageIdalready in the snapshot. Cadence state is per session and shared with workflow steps andsub-agents, which append to the same table.
tool.preparingevents are coalesced to one per 250 ms per tool call. Theyre-send the whole accumulated argument JSON, which produced 146k events for 691 real calls.
Bounded reads
COUNT(*)instead of materializing the session.Store
(session_id, event_type, seq)and drops the two superseded ones. The snapshotlookup walked the whole session backwards, fetching each row to test
event_type, on every state load.permanently cold.
Observability
at startup. The incident produced no log line at all.
Verified
Unit suite 443 files / 5,982 tests pass (18 new);
tsc --noEmit, ESLint and Prettier clean.Measured on a copy of the reporter's 1.4 GB production database (74,519-event session), warm page cache,
median of 5 runs:
getLatestSnapshot()getContextWindowEvents()EXPLAIN QUERY PLANconfirms the change, fromSEARCH events USING INDEX idx_events_session_seq (session_id=?)toSEARCH events USING INDEX idx_events_session_type_seq (session_id=? AND event_type=?).The build was then run against that live database. The mid-turn cadence was confirmed by event ordering —
the snapshot precedes the
running.changed: false, so it cannot be an end-of-turn snapshot:The prune that followed collapsed 35,396 visible events to 117. On the same run:
tool.preparingwas 1.0per real tool call (was 213), page-cache reads ~2 KB/s (was ~170 MB/s), CPU 3.1% (was 98%), and the
database payload went from 1,258 MB to 391 MB.
Known limitations
turn.snapshotsize is still unbounded. Snapshots reach 24.5 MB on the reporter's database, and everygetEventsSinceSnapshot()caller JSON-parses one on load. PR fix: plafonner le streaming sans résultat dans les snapshots + timeout 60 s skippable (bouton « Passer le timeout ») #384 caps the streaming output kept inside asnapshot, which is one source of that growth — but not a bound on the snapshot as a whole. Happy to take
that on separately if the direction looks right.
byte budget, so a
tool.callwhosetool.resulttakes hours holds the log open for its whole duration.fix: plafonner le streaming sans résultat dans les snapshots + timeout 60 s skippable (bouton « Passer le timeout ») #384 was written for exactly this shape (
172k events / 472 MB from one call). A hard ceiling thatoverrides quiescence would close it.
— an older build recreates the dropped ones via
CREATE INDEX IF NOT EXISTSon its next start — but itruns at startup against a live database, so it is worth a look.
AI-Enhanced Development
Cache Impact
The reconstructed message list is unchanged — only the read path that produces it is bounded. The
server-side snapshot/prompts caches are affected (
invalidateSessionCachenow keeps the parsed snapshotentry when the appended event is not a
turn.snapshot), but that is an in-process cache, not the LLM promptcache.