Repository navigation
fix(sessions): keep attachment references a rewritten message still points at - #2236
Conversation
🦋 Changeset detectedLatest commit: b3af551 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
🟡 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 |
agents
@cloudflare/ai-chat
@cloudflare/codemode
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
…oints at Writers derived attachment references only from media extracted on that write, so a message written back in its stored pointer form lost its reference on update (and the payload was collected under a live pointer) or took none when appended under a new id. extractAttachments now reports every address the stored message points at, and append/update/import record those.
…ny depth Review follow-ups on the attachment-reference fix. Recording a pointer no longer ends the extraction walk. A record can carry a pointer in `url` and still hold inline media in a sibling field — a thumbnail beside the image it previews — and that media is offloaded like any other rather than being left in the row the offload policy exists to protect. Reads are symmetric: a resolved pointer no longer stops the walk either, so nested media comes back too. Reference collection also continues past the rewrite depth cap, to its own far looser limit. Whether to rewrite a node is a judgement call and stopping early only leaves a payload inline; whether a node holds a pointer is not, because missing one collects bytes the stored row still names. Adds the patch changeset the fix needs, and moves the `messageRowBytes` doc comment back onto the method it describes.
Replace the rewritten extraction and resolve walks with one scan of the staged message: every attachment pointer it contains, extracted on this write or already there, at any depth, is a reference. Extraction and reads are unchanged.
b568dbc to
9c08140
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| into = new Set<string>(), | ||
| depth = 0 | ||
| ): Set<string> { | ||
| if (depth > 64 || value === null || typeof value !== "object") return into; |
There was a problem hiding this comment.
🔴 Deep stored pointers lose their payloads
When a stored pointer sits beyond eight levels, pointersOf records no reference for it. Deleting its other owner collects the payload, so moving the pointer into readable depth later cannot restore it.
Learn more
A message row can retain an attachment pointer even when the read walker does not hydrate it. extractAttachments stores the row unchanged past its depth limit, but this scan omits its pointer. When releaseMessages removes the last recorded reference, the payload is deleted while the row still contains its address. The early return after finding another pointer causes the same loss for nested pointers or a second pointer in the same object.
Example: Message m1 stores an image. Message m2 copies its pointer under ten nested child objects. Deleting m1 now removes the image because m2 has no recorded reference; flattening m2's child objects leaves an unresolvable pointer instead of the image.
Recommended fix: Keep references for every valid pointer retained in the stored row, including pointers past MAX_WALK_DEPTH and inside objects with another pointer. Preserve the read walker’s existing hydration cap independently of reference tracking, and test deletion of an original owner followed by exposing the surviving deep pointer.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
References now follow exactly the pointers a read restores: pointersOf walks the way resolveAttachments does, to the same depth and stopping at a pointer. Sessions never writes a pointer deeper than that, and a read never restores one, so a pointer down there is an opaque string rather than a reference.
| message.id, | ||
| attachments.map((attachment) => attachment.hash) | ||
| ); | ||
| this.#attachments.addRefs(sessionId, message.id, references); |
There was a problem hiding this comment.
🟡 Pointer copies bypass image token charges
When append preserves an existing image pointer, estimateRowTokens charges no image tokens because it only recognizes data: URLs. Pointer-form copies can delay token-threshold compaction despite retaining full images.
Learn more
The per-row token estimate is calculated by the write pipeline before attachment ingestion. estimateRowTokens charges inline file data URLs but not existing attachment pointers. This change now allows copies bearing pointers to keep their payloads, yet the copied rows get estimates that omit those images. The threshold in compactAfter depends on those estimates.
Example: Copy a stored image message under a new id and call compactAfter(1000). The image remains available, but its new row contributes roughly four tokens instead of the 1,600-token image charge.
Recommended fix: For pointer-form writes, derive the attachment token charge from referenced payload metadata before stamping token_estimate; preserve the existing charge for inline media without counting the same attachment twice. Apply this to append, update and import.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Out of scope here. estimateRowTokens runs before ingestion and never charged pointer-form writes. That is unchanged by this PR, which only stops their payloads from being collected.
| const parts = walk(message.parts, 0) as SessionMessagePart[]; | ||
| if (parts === message.parts) return { message, attachments }; | ||
| return { message: { ...message, parts }, attachments }; | ||
| const references = [...pointersOf(parts)]; |
There was a problem hiding this comment.
🔍 Pointer scan adds a second full traversal
Every write now scans parts again after extraction, including text-only messages. Nested tool outputs also allocate an Object.values array per object. Consider measuring this added write cost for large transcripts.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
The scan is bounded by the same depth as extraction and only visits the staged parts. JSON.stringify of the same message follows straight after and walks all of it, so the scan is small next to that.
Walk the staged message the way resolveAttachments does: to the same depth, and stopping at a pointer. A pointer Sessions never writes and a read never restores is not a reference, and counting one would charge the history byte budget for media no read returns.
1 participant
Sessions stores a message's media as
attachment:sha256:<hash>pointers, andcf_agents_session_attachment_refskeeps each payload alive while some message points at it.The writers recorded references only for media extracted on that write. A message written back in its stored form, with pointers already in place, extracts nothing. So:
updateMessageof that form replaced the message's references with none, and the payload was collected while the row still pointed at it.appendMessage/importMessageof a pointer-form copy under a new id recorded no reference, so the bytes went when the original was deleted.Either way the message could never be resolved again.
extractAttachmentsnow also returnsreferences: every pointer in the staged message, whether extracted on this pass or already there, at any depth.append,updateandimportrecord those: