fix(sdk): cap remember-job poll backoff (WALM-623) - #902
Conversation
|
@nikola0x0 gentle ping — still waiting on review of fea6621. |
nikola0x0
left a comment
nikola0x0
left a comment
There was a problem hiding this comment.
Blocking on the rate-limit comment. bugs: 2 · suggestions: 1
Blocking
The immediate first poll and dropping the 10s climb are right: a 21s write goes from ~9.7s idle to ~0.6s. But a flat 1.5s poll is ~40 req/min, and each poll costs 1 of the relayer's default 30/min per-delegate-key budget. A write that runs past ~45s, or a second write in the same minute, now gets 429s from its own polls, and a recall from that key in the window gets the 429. Growing 1.5× to a 3s cap keeps most of the gain (2.1s idle) with no 429s. Fix inline on polling-delay.ts:10, Python mirror on client.py:181.
Majors (details inline)
polling-delay.ts:8:pollIntervalMs: 0used to floor at 100ms. It is now a zero-sleep loop that spins on network errors and 429s, and WALM-623 says 0 stays unchanged.
Minor / info (1) — details inline
docs/python-sdk/usage.md:103: an explicit interval above 1.5s is silently clamped, and the Python examples pass 2000.
Verified
GET /api/remember/{job_id}andPOST /api/remember/bulk/statusgo throughrate_limit_middleware(services/server/src/main.rs:1656) at weight 1 (rate_limit.rs:166).RATE_LIMIT_DELEGATE_KEY_PER_MINUTEdefaults to 30, andPOST /api/remembercosts 5.- With the suggested
pollingDelayMs,tscis clean andremember-idempotency.test.mjspasses 5/5.polling-delay.test.mjsneeds new numbers. - Changelog placement is right: npm is at 0.1.6 and PyPI at 0.1.9, so 0.1.7 and 0.1.10 are unreleased.
pollingDelayMsis not inindex.tsorpackage.jsonexports.
Follow-up, not this PR
- Job-status polls probably belong outside the write budget, the way
/v1/owners/*reads were split out (main.rs:1708). That is a relayer ticket.
| export function pollingDelayMs(baseMs: number, attempt: number): number { | ||
| if (baseMs <= 0) return 0; | ||
| if (attempt === 0) return 0; | ||
| const capped = Math.min(1500, Math.max(100, baseMs)); |
There was a problem hiding this comment.
[bug] A flat 1.5s poll (~40 req/min) is more than the relayer's per-delegate-key budget. Status polls go through rate_limit_middleware at weight 1 (services/server/src/main.rs:1656, rate_limit.rs:166), the default limit is 30 weighted req/min, and POST /api/remember costs 5 of that.
Simulated against the 60s sliding window (mean jitter, 150ms RTT):
| scenario | before | this PR | 1.5× growth, 3s cap |
|---|---|---|---|
| one 21s write | 6 polls, 9.7s idle | 14 polls, 0.6s idle | 9 polls, 2.1s idle |
| two 21s writes back to back | 0 × 429 | 18 × 429; 2nd write 18.8s idle | 0 × 429 |
| one 60s write | 0 × 429 | 12 × 429 | 0 × 429 |
The waiter retries its own 429s, but recall/remember from the same key in that window throws. A 3s cap still meets both WALM-623 criteria. ceiling also stops clamping an explicit interval above the cap, and dropping line 8 brings back the 100ms floor for 0.
+const MIN_POLL_MS = 100;
+const MAX_BACKOFF_MS = 3000;
+
/**
* Delay before a remember-job poll.
*
- * `baseMs <= 0` means no wait (`pollIntervalMs: 0`).
- * Attempt 0 is immediate so the first GET is not delayed.
+ * Attempt 0 is immediate. Later polls grow 1.5x from `baseMs` (floor 100ms)
+ * to 3s, or stay at `baseMs` if the caller set it higher. Each poll costs 1
+ * of the relayer's 30/min per-delegate-key budget, so a flat short interval
+ * rate-limits the caller's own writes.
*/
export function pollingDelayMs(baseMs: number, attempt: number): number {
- if (baseMs <= 0) return 0;
if (attempt === 0) return 0;
- const capped = Math.min(1500, Math.max(100, baseMs));
+ const base = Math.max(MIN_POLL_MS, baseMs);
+ const ceiling = Math.max(MAX_BACKOFF_MS, base);
+ const capped = Math.min(ceiling, base * 1.5 ** Math.min(attempt - 1, 6));
const jitter = 0.75 + Math.random() * 0.5;
return Math.floor(capped * jitter);
}polling-delay.test.mjs, test_polling_delay.py and the changelog/doc numbers need updating to match.
| * Attempt 0 is immediate so the first GET is not delayed. | ||
| */ | ||
| export function pollingDelayMs(baseMs: number, attempt: number): number { | ||
| if (baseMs <= 0) return 0; |
There was a problem hiding this comment.
[bug] pollIntervalMs: 0 does not behave as before. Before this PR, Math.max(100, baseMs) set a 100ms floor that grew to ~1.1s. WALM-623 says callers passing 0 stay unchanged.
With no sleep, the loop spins. If the relayer is unreachable, fetch throws with no status, so isTransientPollingStatus(0) hits continue (memwal.ts:332). Each pass signs, builds a SEAL session and fetches again, until timeoutMs (60s, or 120s for bulk). A 429 spins the same way. The diff on line 10 drops this line. The "pollIntervalMs: 0 still means no wait" wording in both CHANGELOGs, both docs changelogs and docs/python-sdk/usage.md:103 should go too. The remember-idempotency.test.mjs edit still passes with the floor back.
| return 0 | ||
| if attempt == 0: | ||
| return 0 | ||
| capped = min(1500, max(100, base_ms)) |
There was a problem hiding this comment.
[bug] Python mirror of polling-delay.ts:8 and :10: 429s from the flat 1.5s cadence, and a zero-sleep loop for poll_interval_ms=0.
- ``base_ms <= 0`` means no wait (``poll_interval_ms: 0``). Attempt 0 is
- immediate so the first GET is not delayed.
+ Attempt 0 is immediate. Later polls grow 1.5x from ``base_ms`` (floor
+ 100ms) to 3s, or stay at ``base_ms`` if the caller set it higher. Each
+ poll costs 1 of the relayer's 30/min per-delegate-key budget.
"""
- if base_ms <= 0:
- return 0
if attempt == 0:
return 0
- capped = min(1500, max(100, base_ms))
+ base = max(100, base_ms)
+ ceiling = max(3000, base)
+ capped = min(ceiling, base * (1.5 ** min(attempt - 1, 6)))
jitter = 0.75 + random.random() * 0.5
return int(capped * jitter)| ``` | ||
|
|
||
| Bulk (up to 20 items per call) follows the same pattern with `remember_bulk_async`, `wait_for_remember_jobs`, and `remember_bulk_and_wait`. Polling uses jittered exponential backoff (1.5× capped at 10s, ±25%) to stay relayer-friendly at scale. | ||
| Bulk (up to 20 items per call) follows the same pattern with `remember_bulk_async`, `wait_for_remember_jobs`, and `remember_bulk_and_wait`. Polling starts immediately, then repeats at most ~1.5s with ±25% jitter; `poll_interval_ms: 0` means no wait. |
There was a problem hiding this comment.
[suggestion] An explicit poll_interval_ms above 1.5s is silently clamped. types.py:396 calls it the "base poll cadence", types.ts:235 says "How often to poll", and examples/async_remember_demo.py:137 and interactive_demo.py:189 pass 2000. The ceiling in the suggested delay function keeps a larger value. With that change:
-Polling starts immediately, then repeats at most ~1.5s with ±25% jitter; `poll_interval_ms: 0` means no wait.
+Polling starts immediately, then backs off 1.5× from `poll_interval_ms` to 3s (or to `poll_interval_ms`, if higher), with ±25% jitter.
|
Followed the inline patch on
Ready for another look. |
…s by hand Review items on this head. KEEP THE WAIT. `DEFAULT_REMEMBER_WAIT_MS` was 0, so `memwal_remember` returned at accept with no blob_id. D1 already decided the opposite: a result means the fact landed. Returning at accept is a product change on its own ticket, and the poll-cadence work it was bundled with belongs to #902. The default is the full 90s ceiling again; accept-and-continue stays reachable at `MEMWAL_MCP_REMEMBER_WAIT_MS=0` for an operator who chooses it, and the two suites that cover that path now opt in explicitly instead of inheriting it. A budget between zero and the real completion time stays the setting to avoid — against a 30-75s spread it pays the wait and still returns pending — so the default is the ceiling rather than something in between. MAKE THE IDEMPOTENCY CLAIM TRUE. The accept-timeout message told callers a retry was safe "because the write carries a content-derived idempotency key". That key exists in packages/sdk, which is NOT what runs here: the sidecar installs published 0.1.7, whose rememberAsync mints a crypto.randomUUID() and caches it per client instance — and a fresh client is built per transport session, so a retry after a reconnect stored the fact a second time at full cost. Rather than soften the wording, the tool now computes the key itself and passes it, which 0.1.7 accepts. The promise is true in the version actually deployed. Bulk still says the opposite, because /api/remember/bulk takes no key at all. DO NOT ASK FOR TEXT WE DO NOT HAVE. The recall failure report told the agent to send the fact again, but the relayer stores only SEAL ciphertext, so a failed write's wording is gone. It now says so and asks the user to restate it, rather than inviting the agent to invent it. (The repeat-every-recall half was fixed in 8f923d0 — the report is one-shot.) RELEASE BY HAND. The MCP contract, tools and transport changed under published 0.0.13, and the SDK's poll, timeout and idempotency behaviour under 0.1.7. Both bumped manually: 0.0.14 and 0.1.8, dual changelogs, every manifest the release verifier checks, and the verifier's own pins. The two changesets are deleted — this repo releases these by hand, so leaving them would have double-bumped. `node scripts/verify-manual-sdk-release.mjs` passes for all four packages, including the plugin npx pins it caught me missing in .mcp.json, .cursor-mcp.json and .codex-mcp.json.
I had changed the backoff here as part of the latency work. #902 (WALM-623) already owns it, does it better, and corrects a fact I had wrong. Its version returns 0 for attempt 0 — the immediate first check I had built by restructuring both wait loops — inside `pollingDelayMs` itself, which is the right place. Its ceiling is 3s against a documented 30 weighted-requests/min quota for status GETs. I had read 60/min off a 429 body and picked 2s, not accounting for status reads being weighted 2, so my cap sat closer to the quota than the one written by someone who knew the weighting. It also moves the function to its own module and applies the same fix to the Python SDK. Keeping my copy would have meant two implementations of one policy, and a conflict on merge: #902 deletes the function my change edited. Reverted here: the 2s ceiling, the 600ms base defaults, and the check-first restructuring of both loops. The latency tests that pinned that behaviour go with it — #902 pins the same thing in test/polling-delay.test.mjs, and asserting it twice would leave one copy silently wrong the next time the cap moves. What stays in that file is what this PR actually owns: the derived idempotency key. The 0.1.8 changelog drops its poll bullet to match. What this PR keeps in the SDK is unrelated to cadence: per-request deadlines (#918's own finding — `fetch` had none, so a stalled socket outran `timeoutMs` entirely) and content-derived idempotency keys. Verified against #902: the two branches now auto-merge, with no conflicting hunks in memwal.ts or either changelog.
|
@harrymove-ctrl gentle ping — still waiting on review of 4109e15. |
harrymove-ctrl
left a comment
harrymove-ctrl
left a comment
There was a problem hiding this comment.
Review — WALM-623
Direction is right and the implementation is clean, but the claim the cap rests on doesn't hold: the new schedule overruns the per-delegate-key quota for bulk in the average case, and for single remember under unlucky jitter. Worth a cap adjustment before merge.
What's good
- Extracting
pollingDelayMsintopackages/sdk/src/polling-delay.tsmakes it testable — it wasn't before. - TS/Python parity is exact. I swept both implementations across base ∈ {0, 50, 100, 400, 1500, 2999, 3000, 3001, 5000, 10000} × attempt 0–11 × five jitter values; no divergence (
Math.floorandint()agree on positives). All 9 assertions in both test files verify. - No stale "capped at 10s" text left anywhere on the branch; changelog headings (
0.1.7TS,0.1.10Python) matchpackage.json/pyproject.toml. - The
remember-idempotency.test.mjsrework is necessary, not incidental: with an immediate first poll, the oldjobPolls < 2gate could let the firstrememberAndWaitreachdoneand resolve, breaking theassert.rejects. Keying onrememberPostsfixes it deterministically. - Bulk polls through a single batched
/api/remember/bulk/status, so there's no N× fan-out per iteration.
1. The 3s cap overruns the delegate-key quota
polling-delay.ts justifies the cap with "so status GETs stay under the 30/min quota". That budget is tighter than it looks, because the quota is weighted (endpoint_weight, services/server/src/rate_limit.rs:142):
| endpoint | weight |
|---|---|
POST /api/remember |
5 |
POST /api/remember/bulk |
10 |
GET /api/remember/{job_id} |
1 |
POST /api/remember/bulk/status |
1 |
…against max_requests_per_delegate_key: 30 per sliding 60s window (rate_limit.rs:68; documented default in docs/relayer/self-hosting.md:126, and nothing in the repo overrides it). The POST is paid out of the same bucket, so polling only ever gets 25 units (single) or 20 (bulk).
Simulating the new schedule at the default pollIntervalMs: 1500 over the first 60s:
| scenario | polls | + POST | vs 30 |
|---|---|---|---|
| single, mean jitter | 21 | 26 | ok |
| single, worst jitter | 28 | 33 | over |
| bulk, mean jitter | 21 | 31 | over |
| bulk, worst jitter | 28 | 38 | over |
| single, old 10s cap | 9 | 14 | ok |
Growth does help versus flat polling (1500ms flat would be 40/min), so the mechanism is sound — but 3000ms leaves essentially no headroom, and the weight-10 bulk POST puts bulk over on its own. Any concurrent recall on the same delegate key tips single over too.
Suggestion: cap at 5s rather than 3s. Worst case ~12 polls/min → 17/30 single, 22/30 bulk, both with real headroom, and you still halve the old worst-case latency-to-report (10s → 5s), which is the UX win the ticket is after. Either way the comment should be corrected — as written it asserts something the numbers don't support.
2. The resulting 429 surfaces as a timeout
Pre-existing, but this PR makes it likely. On rejection isTransientPollingStatus(429) is true, so both SDKs continue (memwal.ts:332, client.py:440), keep polling into the closed window, burn the remaining timeout, and throw remember job timed out after 60000ms / MemWalRememberJobTimeout. A quota rejection reaches the caller as a timeout.
Minimum: track whether any poll saw a 429 and say so in the timeout error. Better: honour Retry-After — rate_limit_response (rate_limit.rs:463) already sends the window.
3. Callers above 3s lose backoff entirely (minor)
ceiling = Math.max(MAX_BACKOFF_MS, base) combined with capped = Math.min(ceiling, base * 1.5 ** …) means that for base > 3000 the result is flat base forever — pollingDelayMs(5000, 1) and (5000, 15) both return 5000. The changelog's "keep that interval" undersells it: those callers previously decayed toward 10s and now never back off at all. Someone who deliberately picked 5s for a long job sustains 12/min instead of decaying to 6/min. Worth wording as "backoff is disabled above the cap".
4. Python test patches stdlib random globally (nit)
monkeypatch.setattr("memwal.client.random.random", …) resolves memwal.client.random to the stdlib module object (client.py does a plain import random), so it patches random.random process-wide. monkeypatch unwinds it and pytest is single-threaded, so it works — it just reads as module-local when it isn't.
|
@harrymove-ctrl gentle ping — still waiting on review of 451ecdc. |
harrymove-ctrl
left a comment
harrymove-ctrl
left a comment
There was a problem hiding this comment.
Review — WALM-623
Depth: targeted (adaptive). Cross-language parity would normally be a full seven-lens pass; I checked the TS/Python formula and the wait-loop conflict directly instead.
Verdict: request changes. The cadence is right. This head is not OK to merge.
What is OK
- Immediate first poll, and dropping the climb to 10s. That is the idle WALM-623 measured (next poll 5–8s after a ~20s write was already
done). - Cap at 5s, not the ticket's suggested 1–1.5s. A flat 1.5s is ~40 req/min, and status GETs share the default 30 weighted req/min delegate-key budget (
rate_limit.rs, weight 1;POST /api/rememberis 5). 5s keeps headroom and still cuts the old worst case in half. Backoff disabled above the cap is the right wording. pollIntervalMs: 0keeps the old 100ms floor after the first poll. No newMemWalmethods or request fields.pollingDelayMsis not in package exports.- Naming a 429 on the timeout is worth keeping.
Acceptance: the measured ~20s write no longer waits 5–10s. Worst jitter at the cap is 6.25s, not 10–12.5s. Formula tests pin attempt 20 at 5000 before jitter. That is enough for the unit-test criterion.
What blocks merge
451ecdcconflicts withdev(f248bd37).packages/sdk/src/memwal.tsis a real logic conflict, not a changelog overlap. Taking this head's wait loop drops two fixes that landed after the branch point:pollDeadlineMson each status read. Without it, one stalled GET outlivestimeoutMsagain.retryAfterDelayMs(723baece). Dev clampsRetry-Afterto the remaining budget and still does one status read. This PR returns null and skips that read whenRetry-Afterdoes not fit.
- Changelog notes are under shipped versions. TS notes sit in 0.1.7; Python in 0.1.10. Published heads are 0.1.8 and 0.1.11. Put them in unreleased 0.1.9 / 0.1.12. Do not rewrite the "latest release" blurbs, and do not bump
package.json/pyproject.tomlin this PR. - Python
client.pyshould mirror the same combined policy and must keep the WALM-660 rule: a job missing from a bulk status stays pending.
How to land it
Keep this PR's pollingDelayMs (attempt 0 is 0; later polls floor 100ms, grow 1.5× toward 5s, ±25% jitter). Keep dev's sleep shape: a clamped Retry-After wins and does not advance the exponent; otherwise use pollingDelayMs. Keep { timeoutMs: this.pollDeadlineMs(deadline) } on both status calls. Append ; wait hit a rate limit (429) only when the wait actually times out.
I am not approving 451ecdc. Rebase onto dev with that resolution and this becomes an approve.
First status read is immediate. Later polls grow 1.5x from the caller interval toward 5s, with the 100ms floor kept. Status reads still use pollDeadlineMs, and Retry-After stays clamped to the remaining budget so that remainder still buys one read. A timeout that saw a 429 says so.
|
Rebased onto current
|
451ecdc to
5cddf3c
Compare
|
Correction: ignore the note that pointed at #1005. That PR is closed. The fix is on this branch now. Head is This is the PR to review. I dismissed the changes-requested review, which was against |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| seconds = float(raw) # type: ignore[arg-type] | ||
| except (TypeError, ValueError): | ||
| return None | ||
| if seconds <= 0 or seconds != seconds or seconds == float("inf"): |
harrymove-ctrl
left a comment
harrymove-ctrl
left a comment
There was a problem hiding this comment.
Approved. Head 5cddf3c5 is the rebase this review asked for.
Immediate first poll, 1.5× growth to a 5s cap, 100ms floor kept. Status reads still pass pollDeadlineMs. Retry-After is still clamped to the remaining budget and that remainder still buys one read. A 429 is named only on the timeout. Python matches, and a missing bulk status stays pending. No new public method, no version bump. Notes sit in unreleased 0.1.9 / 0.1.12.
4 participants
Ticket
WALM-623 — https://linear.app/mysten-labs/issue/WALM-623/sdk-cap-waitforrememberjob-poll-backoff-no-public-api-change
What changed?
waitForRememberJob/waitForRememberJobs(and Pythonwait_for_remember_job/wait_for_remember_jobs) no longer sleep 5–10s after a ~20s write is alreadydone.pollIntervalMs: 0/poll_interval_ms: 0still means no wait.Why is this needed?
Quiet-prod traces (WALM-618 / WALM-621): Walrus+index finished in ~21s; the client polled again 5–8s after
donebecause backoff had climbed toward 10s. That idle is entirely client-side. MCP SSE push / webhooks are out of scope — recall is already ~1.6–2.6s once the request arrives.Scope
Cap remember-job poll delay in the TypeScript and Python SDKs.
Out of scope
How was this tested?
Commands:
node scripts/verify-manual-sdk-release.mjs(0.1.7 / 0.1.10 / 0.0.13 / 0.0.6)packages/sdk:pnpm test— 122 passedpackages/python-sdk-memwal:pytest tests/ -m 'not integration'— 155 passed, 23 deselectedHow can the reviewer verify it?
packages/sdk/src/polling-delay.tsandpackages/python-sdk-memwal/memwal/client.py_polling_delay_ms.packages/sdk/src/index.tsdoes not exportpollingDelayMs.devalready ahead ofmain).Risks and dependencies
None. Callers that passed a large
pollIntervalMsnow wait at most ~1.5s between polls (the intended cap).Author checklist