Repository navigation
fix(server): continue recall batch decrypt across chunks when an earlier chunk fails (#1120) - #1343
Conversation
…ier chunk fails (#1120)
nikola0x0
left a comment
nikola0x0
left a comment
There was a problem hiding this comment.
Issue counts by severity
bugs: 0
suggestions: 3
nits: 1
Verdict: blocking on the PR description, not the code. The fold change works as intended. I tested it locally against testnet, results below.
Blocker: Fixes #1120 overclaims. The #1120 repro is "bulk-write ~25 memories, recall immediately". 25 blobs is exactly one decrypt chunk (SEAL_DECRYPT_BATCH_SIZE = 25), and a single-chunk failure still returns Err, by design and pinned by fold_chunks_single_chunk_failure_returns_err. It also doesn't touch the 22.6s post-write latency or give the "distinct retryable status" the reporter asked for. Merging as-is auto-closes #1120 with the bug still live.
Please change Fixes #1120 to Refs #1120 and reword the summary as a partial-results improvement for recalls over 25 blobs.
What I ran. I ran a local relayer for dev and for this branch against testnet (Railway dev config, same account, same 30 memories). A proxy in front of the sidecar failed the first /seal/decrypt-batch call of a recall:
| Recall size | dev |
this PR |
|---|---|---|
| 25 memories (the #1120 case, one chunk) | 503 | 503, unchanged |
| 30 memories (two chunks: 25 + 5) | 503, the second chunk is never sent | 5 results, dropped_count: 25 |
Without the injected failure, both builds returned everything. I also ran the plain #1120 repro on dev (bulk-write 25, recall every 2s from the moment the writes were accepted): 53 recalls, 0 errors, with partial results growing as uploads landed. So the reported failure doesn't reproduce on a single local relayer and is probably caused by production conditions. cargo test walrus_seal passes 16/16.
Suggestions / nits (4) — details inline
walrus_seal.rs:461: errors that always repeat (expired session, bad delegate key) now hit the sidecar once per chunk before failingwalrus_seal.rs:482:analyze.rs:313wrapsfetch_batchin a timeout; continuing past a slow chunk 0 eats that budgetrecall-token-budget-e2e.test.mjs:72/test_client.py:846: the SDK tests don't exercise this changewalrus_seal.rs:522: "every non-empty chunk" can't happen
| let mut chunk_results = Vec::new(); | ||
| for chunk in batch_input.chunks(SEAL_DECRYPT_BATCH_SIZE) { | ||
| match seal::seal_decrypt_batch( | ||
| let res = seal::seal_decrypt_batch( |
There was a problem hiding this comment.
[suggestion] Before, a chunk-0 failure stopped immediately. Now an error that will always repeat (expired session or bad delegate key → sidecar 4xx, sidecar down → transport error) is sent once per chunk, one after another, before the recall fails. With limit: 200 that's 8 sidecar calls that can't succeed.
Consider stopping early when the error isn't transient. Fill the rest with Missing (or return Err if nothing has succeeded yet) and only keep going on timeout/429/503-style failures. DecryptOutcome::permanent_from_error already has that classification, though it would need to accept the whole-batch AppError message.
| } | ||
| chunk_results.push((chunk.len(), res)); | ||
| } | ||
| let decrypted = fold_decrypt_chunk_results(chunk_results)?; |
There was a problem hiding this comment.
[suggestion] routes/analyze.rs:313 runs fetch_batch under tokio::time::timeout and falls back to plain extract on expiry. If chunk 0 is slow and then fails, this loop now keeps going instead of returning, so the analyze pre-extraction is more likely to hit its timeout and lose everything. Probably fine, but worth a sentence in the PR so it's a known trade-off.
| assert.equal(result.results.length, 0); | ||
| }); | ||
|
|
||
| test("recall preserves dropped_count when maxDistance and maxTokens are applied", async () => { |
There was a problem hiding this comment.
[suggestion] This test, and the Python one at test_client.py:846, checks client-side maxDistance/maxTokens filtering against a mocked relayer response. No SDK source changes in this PR, so they pass the same on dev. They're fine to keep as coverage, but they aren't evidence for the server fix, so the PR summary shouldn't list them as verifying it.
| /// When one chunk fails with `Err(e)`, we fill `DecryptOutcome::Missing` for | ||
| /// every blob in that chunk and continue attempting remaining chunks so that | ||
| /// chunk 0 failing does not abort chunk 1+ when later chunks can still | ||
| /// succeed. If every non-empty chunk failed, returns `Err` with the last |
There was a problem hiding this comment.
[nit] .chunks() never yields an empty chunk, so "every non-empty chunk" can't happen as written. "If every chunk failed" is enough. Non-blocking.
|
Thanks @nikola0x0 for the thorough review and testing. Updated the PR description:
Ready for re-review. |
nikola0x0
left a comment
nikola0x0
left a comment
There was a problem hiding this comment.
Verdict: approve. The blocker is fixed. I checked the PR itself, not just the description: GitHub now lists no closing issues for this PR (Refs #1120), so merging leaves #1120 open. The head is still 51860f2, the commit I tested on testnet, so the earlier results stand: 25 memories with a failed chunk still returns 503 (unchanged), and 30 memories returns 5 results with dropped_count: 25. Thanks for reframing the summary around the multi-chunk case. It now describes what the code does.
One thing outside the diff: your #1120 comment still tells the reporter this PR "completes the post-write partial-result handling". Please update it to say this covers recalls over 25 blobs and that the single-chunk case and post-write latency are still open, so the reporter doesn't expect a fix from this merge.
Open threads from round one (4), optional follow-ups, not blocking
walrus_seal.rs:461: errors that always repeat are now sent once per chunk; consider stopping early on non-transient errorswalrus_seal.rs:482: analyze'sfetch_batchtimeout trade-off is worth a sentencerecall-token-budget-e2e.test.mjs:72/test_client.py:846: the SDK tests don't cover the server changewalrus_seal.rs:522: "every non-empty chunk" wording
A one-line reply on each thread (done / won't do / follow-up) would let them be resolved.
Summary
Refs #1120
WalrusSealEngine::fetch_batch,seal_decrypt_batchis invoked in chunks ofSEAL_DECRYPT_BATCH_SIZE(25). Previously, if chunk 0 failed withErr(e),decrypted.is_empty()caused an immediatereturn Err(e)without attempting subsequent chunks, whereas a failure in chunk 1+ degraded gracefully by fillingDecryptOutcome::Missingfor the failed chunk and returning partial results withdropped_count.fold_decrypt_chunk_resultsto process all chunk results uniformly: when any chunk fails in a multi-chunk batch, it fillsDecryptOutcome::Missingfor that chunk's items and continues through remaining chunks. Only if every chunk fails doesfetch_batchreturnErr(last_err). When at least one chunk succeeds, surviving chunks are hydrated and the failed chunk's items are counted indropped_count.services/server/src/engine/walrus_seal.rscovering single-chunk failure, chunk 0 failure + chunk 1 success, chunk 0 success + chunk 1 failure, and all-chunks failure.packages/sdk/test/recall-token-budget-e2e.test.mjs) and Python (packages/python-sdk-memwal/tests/test_client.py) verifyingdropped_countis preserved onRecallResultwhenmaxDistanceandmaxTokensfilters are applied.Test Plan
cargo test --manifest-path services/server/Cargo.toml walrus_seal(16 passed)pnpm --filter @mysten-incubation/memwal test(370 passed)uv run --extra dev pytest testsinpackages/python-sdk-memwal(520 passed)