Repository navigation
Fix/1984 synthetic compaction - #2036
threepointone merged 7 commits into
Conversation
🦋 Changeset detectedLatest commit: 3703678 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…loudflare#1984) getHistory() synthesizes a compaction_<id> overlay on read; when a client echoed it back it was filed as a real row, duplicating the summary in every later read. Session now refuses to persist reserved compaction_ ids, and applyCompactions filters such rows so existing sessions self-heal on read.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| --- | ||
| "agents": patch | ||
| --- | ||
|
|
||
| Fix `useAgentChat` dropping an in-flight optimistic send on reconnect. When a message was sent while the socket was down, PartySocket buffers the frame for delivery, but the idle-connect transcript the server replays on reconnect (`cf_agent_chat_messages`) doesn't include it yet — so the whole-array `setMessages` replace erased the just-sent message from the UI until the turn completed and the server rebroadcast. The reconnect replay now preserves a trailing local-only `user` message that was buffered while disconnected, while still letting the server snapshot win for a delivered send it deliberately rolls back (e.g. `messageConcurrency: "drop"`) and for a regenerate's assistant replacement. Also fixes `@cloudflare/ai-chat` and `@cloudflare/think`, which re-export the hook unchanged. |
There was a problem hiding this comment.
🟡 Compaction duplication fix ships without a release note, so users never get it
The bug fix that stops compaction summaries being stored and duplicated is added to a published package without an accompanying changeset entry (only .changeset/reconnect-optimistic-send.md exists, which describes an unrelated reconnect fix), so the fix is not released or documented.
Impact: The fix may not be published in a version bump and won't appear in the changelog users read.
Repository rule: changesets required for package bug fixes
AGENTS.md ("Contributing → Changesets") states: "Changes to packages/ that affect the public API or fix bugs need a changeset". Commit ffaafdc4 changes packages/agents/src/experimental/memory/session/session.ts:624-635, packages/agents/src/experimental/memory/session/providers/agent.ts:535-541 and packages/agents/src/experimental/memory/session/providers/postgres.ts:269-272 — a user-visible bug fix (#1984) — but adds no .changeset/*.md. The only changeset in the PR (.changeset/reconnect-optimistic-send.md:1-5) documents the useAgentChat reconnect fix from the other commit.
Prompt for agents
The PR contains two independent fixes but only one changeset. Add a second changeset markdown file under .changeset/ (patch bump for the "agents" package) describing the #1984 fix: Session no longer persists messages using the reserved compaction_ id prefix, and both the SQLite (AgentSessionProvider) and Postgres session providers filter compaction-prefixed rows out of the read projection so previously corrupted sessions self-heal on read.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Did you remove the changeset? It’s important to include one otherwise the change won’t be released when we merge.
There was a problem hiding this comment.
oh wait I now see that was an unrelated changeset. Can you add a new one for this change @justanotherbyte ?
There was a problem hiding this comment.
Added .changeset/compaction-overlay-echo.md (patch for agents and @cloudflare/think) describing the #1984 fix.
ffaafdc to
33b7b89
Compare
agents
@cloudflare/ai-chat
@cloudflare/codemode
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
cjol
left a comment
cjol
left a comment
There was a problem hiding this comment.
Overall shape is good, thank you! Definitely need a changeset, a few nits otherwise in comments.
| if (isCompactionMessage(message)) { | ||
| console.warn( | ||
| `[Session] Refusing to persist a message with the reserved compaction id "${message.id}" — synthetic compaction overlays are computed on read and are never stored (#1984).` | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Is this necessary? Receiving a compaction message from the browser is expected, so we’re logging a warning under normal, happy-path conditions (potentially even on every single turn after a compaction?)
There was a problem hiding this comment.
Removed: the guard now lives in sessions/handle.ts (this file was deleted on main in #2196) and drops echoed compaction_ messages silently, with no warning.
| expect(compactions).toHaveLength(1); | ||
| }); | ||
|
|
||
| it("does not persist synthetic compaction overlays echoed back by a client (#1984)", async () => { |
There was a problem hiding this comment.
For completeness, it might be good to add a test for the postgres provider as well
There was a problem hiding this comment.
The Postgres provider no longer exists on main (removed in #2196), so there is nothing to test there; the fix now lives in the single sessions/ store.
| // Simulate an already-corrupted session: a synthetic overlay was filed as | ||
| // a real row on a prior turn (before the intake guard existed), parented | ||
| // into the live chain via the raw insert seam. | ||
| await agent.rawInsertChildForTest("m5", overlay!.id); |
There was a problem hiding this comment.
For completeness, I might test that a child of the overlay message is restored and re-parented correctly
There was a problem hiding this comment.
Added: capability.test.ts and migration.test.ts now check that a stray overlay row's child shows up in its place in history (both directions), in getBranches of the parent, and as the latest leaf.
|
I checked this against current
This needs a full rewrite rather than a rebase. All three files it edits (
|
Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # packages/agents/src/experimental/memory/session/providers/agent.ts # packages/agents/src/experimental/memory/session/providers/postgres.ts # packages/agents/src/experimental/memory/session/session.ts # packages/agents/src/tests/experimental/memory/session/provider.test.ts # packages/agents/src/tests/experimental/memory/session/session.test.ts
Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🟡 agents import sizes: 1 entry point grew
Changed exports (1)
How this worksEach runtime export is bundled on its own, minified, and gzipped. Changes smaller than 100 B, or smaller than 1% and 1 KiB, are ignored. Growth over 10% or 5 KiB is marked 🔴. This report is informational and does not fail CI. The workflow artifact contains every measurement. Compared |
…'s overlays Address review: imported history() summaries stay visible (no matching compaction record), getBranches keeps seq order when flattening a hidden row's children, and getRecentHistory judges the depth cap from the unfiltered walk. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ed rows Address review: #pathIds bound an unqualified id to the compactions table, so newest-first replay kept stored overlay echoes; tokenEstimate recorded the visible row count as the memo depth, so appends past the walk cap kept counting the dropped oldest row. Co-authored-by: Cursor <cursoragent@cursor.com>
Addressed in the rework: the Postgres provider was removed from main (so the dialect concern no longer applies), and the fix now filters on read + guards writes, with the test cases requested. Dismissing so this can land; happy to follow up on anything further.
3 participants
Fixes #1984
Original fix by @justanotherbyte. Maintainer update: the files this PR originally edited (
experimental/memory/session/{session,providers/agent,providers/postgres}.ts) were removed frommainin #2196, so the same fix has been rebuilt on the newpackages/agents/src/sessions/code. Main has been merged in; no rebase and no force-push.What was wrong
After a session compacts, history reads substitute a synthetic
compaction_<recordId>message for the compacted range. That message is computed on read and has no row of its own. The browser transport posts the whole transcript back on the next turn, and the overlay'screatedAtis regenerated on every read, so after any cache rebuild the echoed copy no longer matches the server's copy byte for byte. It then went throughupsertMessageintoappendMessageand was stored as a real row. From then on every read returned the summary twice: the model saw it twice, clients got duplicate React keys, and each further compaction added another copy. The stray row also became the parent of the next message.How it's fixed
sessions/handle.ts):appendMessage,upsertMessage,updateMessageand the internal syncupsertdrop any message whose id starts withcompaction_. Nothing is written and no change event is sent. It does notconsole.warn, because the echo happens normally after every compaction.importMessagestays verbatim on purpose: a copied transcript can have later rows parented to a stray row, and dropping it would cut them off.sessions/core.ts): repairs sessions already affected, including stray rows copied forward by the legacyassistant_*migration, on read and with no migration. A storedcompaction_<id>row is hidden only when<id>names one of this session's own compaction records, meaning it duplicates an overlay the session already renders. Path reads (getHistory,history()in both directions,getRecentHistory,getHistoryRowStats, the auto-compaction token estimate) walk through hidden rows without returning them, so a hidden row's child takes its place.getBrancheslists a hidden row's children under its parent inseqorder,getLatestLeafnever returns a hidden row, andsearchskips hidden rows. The record lookup runs only for ids with the prefix, so healthy sessions read no extra rows.compaction_row with no matching record, such as a summary brought in by the documentedhistory()→importMessage()cross-object move, is the only copy of that context and is shown as ordinary content.getRecentHistory().truncatedis judged from the unfiltered walk, so a hidden row inside a capped path can't hide the truncation.compaction_id, and the guard sits below reconciliation in Sessions, so it is dropped whichever path it takes. Hosts other than Think get the same protection.Review feedback (@cjol)
.changeset/compaction-overlay-echo.md, patch foragentsand@cloudflare/think).mainany more (removed in feat(agents): move sessions into a Lifecycle capability #2196).getBranchesof the stray row's parent, and as the latest leaf. A branch from the child also walks through the hidden row.Tests
packages/agents/src/tests/sessions/capability.test.ts: write-guard test (every write aperture, no events, the next message attaches to the real leaf) and a test for a stray row plus its re-parented child.packages/agents/src/tests/sessions/migration.test.ts: a legacy stray row plus child is hidden after migration, re-parented, and excluded from search.packages/think/src/tests/think-session.test.ts: an overlay echoed at Think's intake is not stored (the contributor's original test).capability.test.ts(review follow-ups): ahistory()→importMessage()round trip keeps the summary; a hidden row's children are listed among their siblings inseqorder; a 10,000+ row path holding a hidden row reportstruncated: true.Known limits
compaction_id onappendMessage/upsertMessage/updateMessage, so an imported summary can't be edited through those calls afterwards.getMessage(id)still returns a stray row if asked for it directly by id.createdAtis still regenerated on each read. That's harmless now, but a stable timestamp would let Think skip the echo before reaching Sessions. Possible follow-up.