fix(export): refuse dumps past the transport limit instead of dropping the worker - #1143
fix(export): refuse dumps past the transport limit instead of dropping the worker#1143dmazhukov wants to merge 7 commits into
Conversation
Signed-off-by: Dmitrii Zhukov <dmitry0983@gmail.com>
… collection Signed-off-by: Dmitrii Zhukov <dmitry0983@gmail.com>
Signed-off-by: Dmitrii Zhukov <dmitry0983@gmail.com>
|
@dmazhukov is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe export flow now supports collection selection and pagination, reports collection totals, and enforces a configurable serialized-size limit. REST and MCP responses return HTTP 413 for oversized exports. Proxy errors include truncated response details. ChangesExport protection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant api_export as api::export
participant mem_export as mem::export
participant collections as Collection reads
Client->>api_export: Request export with collection selection and pagination
api_export->>mem_export: Forward validated export parameters
mem_export->>collections: Read selected collections and totals
collections-->>mem_export: Paginated data and totals
mem_export-->>api_export: ExportData or ExportTooLarge
api_export-->>Client: HTTP 200 or HTTP 413
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/server.ts (1)
365-381: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward
memory_exportpagination args tomem::export.
memory_exportalways callssdk.trigger({ function_id: "mem::export", payload: {} }), somem::exportignores pagination and never returnscollectionPagination. The REST export handler already reads and forwardsmaxSessions,offset,collectionLimit, andcollectionOffset, while the MCP tool schema still declares an emptyproperties. Add argument validation and pass the same fields into the payload.🤖 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 `@src/mcp/server.ts` around lines 365 - 381, Update the memory_export case and its MCP tool schema to validate and accept maxSessions, offset, collectionLimit, and collectionOffset, then forward those values in the payload of sdk.trigger for mem::export. Preserve the existing isExportTooLarge handling and response formatting while ensuring pagination arguments reach the export function.
🧹 Nitpick comments (1)
src/functions/export-import.ts (1)
176-191: 🚀 Performance & Scalability | 🔵 TrivialCollection pagination still requires a full
kv.list()read per collection.
sliceCollectionpagesmemories,graphNodes, and the other collections only afterkv.list()has already fetched every row for that scope. For very large collections (the issue mentions 8K+ memories, 34K observations per session),collectionLimit/collectionOffsetshrink the response size but do not reduce the KV read cost behind each collection. This matches the PR's stated goal (avoid oversized WebSocket responses), so it is not a blocking concern, but if a collection ever grows large enough that the full-list read itself becomes slow, pagination here will not help.Consider tracking this as a follow-up if
StateKV.listgains offset/limit support later.🤖 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 `@src/functions/export-import.ts` around lines 176 - 191, Track this as a follow-up rather than changing the current export flow: collection pagination in sliceCollection occurs after each kv.list call and cannot reduce the full KV read. If StateKV.list later supports offset/limit parameters, update the collection reads in the export function to pass collectionOffset and collectionLimit directly while preserving the existing collection mappings and response slicing behavior.
🤖 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.
Outside diff comments:
In `@src/mcp/server.ts`:
- Around line 365-381: Update the memory_export case and its MCP tool schema to
validate and accept maxSessions, offset, collectionLimit, and collectionOffset,
then forward those values in the payload of sdk.trigger for mem::export.
Preserve the existing isExportTooLarge handling and response formatting while
ensuring pagination arguments reach the export function.
---
Nitpick comments:
In `@src/functions/export-import.ts`:
- Around line 176-191: Track this as a follow-up rather than changing the
current export flow: collection pagination in sliceCollection occurs after each
kv.list call and cannot reduce the full KV read. If StateKV.list later supports
offset/limit parameters, update the collection reads in the export function to
pass collectionOffset and collectionLimit directly while preserving the existing
collection mappings and response slicing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 562fdced-a35f-4cde-bb2d-b2080bd80994
📒 Files selected for processing (7)
src/functions/export-import.tssrc/mcp/rest-proxy.tssrc/mcp/server.tssrc/triggers/api.tssrc/types.tstest/export-import.test.tstest/mcp-standalone-proxy.test.ts
|
Heads-up on the red CI here: both failures reproduce on a clean checkout of
An endpoint landed in #1132 / #1136 without the generated docs moving with it. I checked that this branch adds no endpoints: #1144 refreshes the four files that carry the count. Once it lands I'll rebase this branch so the matrix reflects the actual change. |
…commends Signed-off-by: Dmitrii Zhukov <dmitry0983@gmail.com>
|
Good catch on The empty
🤖 Addressed by Claude Code |
Signed-off-by: Dmitrii Zhukov <dmitry0983@gmail.com>
|
Correction to my note above: I've folded the same 4-line refresh into this branch (3966cb8, identical to #1144's commit) rather than waiting.
#1144 still stands on its own if you'd rather take the housekeeping separately — it is the same commit, so whichever merges first makes the other a no-op. 🤖 Addressed by Claude Code |
The (collectionOffset, collectionLimit) window applies to all 18 collections at once, so a client that reads six of them still pays for the other twelve. Measured against prod on 2026-08-03: an eight-page walk at collectionLimit=1000 moved 24,407,798 bytes carrying 11,061 rows the caller wanted and 19,518 it dropped on the floor — graphNodes 6998, graphEdges 8000, accessLogs 4520. ?collections=memories,summaries,semanticMemories,... keeps the rest out of the payload. Deselected collections are still listed and still counted, so collectionTotals reports the whole corpus exactly as before — clients read it for corpus size, not only to size their own walk. The saving is on the wire, not on the daemon. collectionPagination.hasMore now counts only the selected collections. Without that the flag stays true until the longest collection in the corpus runs out, which is why the bridge had to hand-roll an early stop against totals instead of just paging until the daemon said stop. An absent parameter behaves exactly as before. A present one is taken as an explicit choice: unknown names are dropped rather than refused, and a list that ends up empty selects nothing rather than everything — the fallback-to-everything reading would turn a client-side typo into the full multi-megabyte dump this parameter exists to avoid. Signed-off-by: Dmitrii Zhukov <dmitry0983@gmail.com>
The REST endpoint grew ?collections= in the previous commit, but the MCP tool is where an agent actually calls export, and it could only ask for all eighteen collections. An agent looking for a lesson had to pull graphEdges with it. Forwarded as the raw string the caller gave, empty value included: mem::export reads an empty selection as "no collections", and that only stays distinguishable from an absent argument if this layer does not helpfully drop it. Signed-off-by: Dmitrii Zhukov <dmitry0983@gmail.com>
c473c5b to
6bd1868
Compare
Summary
Fixes #1142.
Past ~16 MiB of serialized response,
GET /agentmemory/exportkills the worker↔engineWebSocket.
Export completeis logged, then[iii] Reconnecting attempt 1, and everyendpoint 404s until the worker re-registers about a second later. One GET takes down the whole
REST surface.
Bisected on a local instance: 16 771 046 B returns 200 in 365 ms, one 27 KB step further
returns 500 in 103 ms. That is 6 170 B short of 16 MiB (16 777 216). The failing request is
faster than the succeeding one, so this is a size limit and not a timeout. The limit is not
in this package —
iii-sdkopens its socket with nomaxPayload(so thewsdefault of100 MiB applies), while the engine binary carries tungstenite's
WebSocketConfigfield namesand tungstenite defaults
max_frame_sizeto exactly 16 MiB. Details and the reproduction arein the issue.
What this changes
1.
mem::exportmeasures the payload and refuses to send one that cannot arrive.Buffer.byteLength(JSON.stringify(exportData))againstEXPORT_MAX_BYTES(default 15 MiB,leaving room for framing). Over the ceiling it returns a structured
ExportTooLarge, whichapi::exportmaps to 413 with the byte counts and the parameters that get around it.The guard has to sit inside
mem::export, not in the trigger:api::exportreaches itthrough
sdk.trigger, so the result crosses the boundary twice and dies on the first hop —the HTTP layer never receives the oversized object at all.
2.
?collectionLimit=/?collectionOffset=page the other collections.Today
?maxSessions=slices onlysessionsand theirobservations;memories,summaries, and the 16 top-level collections come in full regardless, so on my repro store?maxSessions=1still returned 5.50 MB of which ~4.9 MB could not be reduced. That floorgrows with the store, and once it alone passes 16 MiB the export is impossible at any
parameter — the dead end #890 describes. The response reports
collectionPaginationwithper-collection totals and a combined
hasMoreso a caller can walk to the end.3.
?collections=narrows the payload to what the caller actually reads.An allowlist over the same 18 collections, so an agent after a lesson is not also pulling
graphEdges.sessions,observationsandprofilesare deliberately outside thevocabulary — they are windowed by
maxSessions/offsetand profiles are derived from thesession page.
totalsstill report every collection even when deselected, since that is whatclients read for corpus size, while
hasMoreconsiders only the selected ones so a clientthat asked for six of eighteen can stop on the flag instead of hand-rolling a comparison
against
totals.4. The refusal survives the trip to every consumer.
src/mcp/server.tswas returning whatevermem::exportproduced inside a200, so a refusalwould have shipped as a successful export whose body happens to be an error object. It now
branches on the same shared guard.
rest-proxy.tsdiscarded 4xx bodies and threw with statustext alone, which stranded the caller with
413and no idea which parameters help; it nowcarries a truncated body into the error.
Behaviour
Anything that returns 200 today returns exactly the same bytes. A
test/export-import.test.tscase pins the full-corpus response shape field by field so the change cannot quietly alter it.
The only altered path is the one that currently fails, and it now fails as one request instead
of an outage.
What this doesn't fix
or flag for it. This PR keeps a daemon alive that would otherwise drop its worker; it does
not make the transport carry more. If the limit is configurable, that is a better fix and
this guard should follow it.
collectionLimitdoes not slice them; they followthe session window. A single session whose observations alone exceed the ceiling cannot be
exported at any parameter combination.
mesh/export(mesh/export has no pagination — backlogs past the worker WS message limit fail forever with "Invocation stopped", making mesh sync unrecoverable after an outage #890) has the same shape and is untouched here.state::sethanging to that ceiling; I saw it on every reproduction. Fewer drops means fewer hangs, but
the timeout itself is fix: give StateKV calls their own short timeout, separate from the 180s LLM-sized worker default #1128's subject.
Testing
test/export-import.test.ts: full-corpus shape pinned; refusal returned over the ceilingand not under it;
collectionLimitbounds every collection while the unparameterised callstill returns everything; a three-page walk ends with
hasMore: false; guard rejectslook-alike values.
test/mcp-standalone-proxy.test.ts: a 413 body reaches the reported failure.npx vitest run --exclude test/integration.test.ts→ 1572 passed, 0 failed.npx tsc --noEmit→ 25 errors, unchanged frommainand none in the touched files.npm run buildclean.CI was red on
mainitself when this was opened —npm run skills:checkreported drift inthe generated REST reference and
test/consistency.test.tsdisagreed with the README'sendpoint count. That refresh is folded in here as its own commit, and stands alone as #1144 if
you would rather take it separately; the two are the same commit, so whichever merges first
makes the other a no-op.
Summary by CodeRabbit
New Features
memory_exporttool now accepts pagination and collection-selection options.Bug Fixes
Documentation