Skip to content

fix(python-sdk): pop pending remember key on terminal job failure (#1001) - #1339

Open
harrymove-ctrl wants to merge 1 commit into
devfrom
fix/python-sdk-remember-key-cleanup
Open

harrymove-ctrl wants to merge 1 commit into
devfrom
fix/python-sdk-remember-key-cleanup

Conversation

@harrymove-ctrl

Copy link
Copy Markdown
Collaborator

Summary

Pop _pending_remember_keys[request_identity] when wait_for_remember_job raises
MemWalRememberJobFailed or MemWalRememberJobNotFound inside remember_and_wait.

Root cause

remember_and_wait stored the generated idempotency key in
_pending_remember_keys before calling wait_for_remember_job. On a terminal
failure (MemWalRememberJobFailed / MemWalRememberJobNotFound) the exception
propagated before the existing pop() could execute, leaving a stale key in the
map indefinitely. Subsequent calls with the same (text, namespace) then reused
the key associated with the permanently-failed job, which the server would
correctly collapse — returning the failed job result again instead of submitting
a new job.

Fix

Wrap wait_for_remember_job in a try/except for the two terminal exception
types. On either, pop the stale key before re-raising so the next call derives
a fresh idempotency key.

MemWalRememberJobTimeout is intentionally not caught: a timeout is a
transient failure and the caller should retry with the same key to collapse
onto the in-flight job (existing behavior, unchanged).

Tests

Two new regression tests added to tests/test_remember_derived_key.py:

  • test_remember_and_wait_pops_key_on_terminal_job_failure
  • test_remember_and_wait_pops_key_on_job_not_found

All 9 tests pass (9/9 green locally).

@harrymove-ctrl
harrymove-ctrl requested a review from ducnmm October 28, 2026 07:18
@harrymove-ctrl

Copy link
Copy Markdown
Collaborator Author

@ducnmm Ready for review — fixes stale idempotency key retention on terminal job failures in the Python SDK.

Root cause: remember_and_wait stored the generated key before wait_for_remember_job, but on MemWalRememberJobFailed / MemWalRememberJobNotFound the exception propagated before the existing pop() ran — leaving the stale key in _pending_remember_keys permanently.

Fix: try/except around wait_for_remember_job for the two terminal exception types; pops the key before re-raising. MemWalRememberJobTimeout is intentionally not caught (transient — caller should retry with same key to collapse onto the in-flight job).

Two new regression tests cover both terminal paths. All 9/9 local tests pass.

@ducnmm ducnmm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the issues below before merging. Findings are based on the current diff and repository source; no live production writes were run.

Comment thread packages/sdk/src/ai/middleware.ts Outdated
if (!lastUserMessage) return params;

const recallResult = await memwal.recall(lastUserMessage, maxMemories);
const recallResult = await memwal.recall({ query: lastUserMessage, limit: maxMemories, ...(maxTokens !== undefined ? { maxTokens } : {}) });

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Put the Python terminal-failure fix on this branch

The current diff changes TypeScript middleware, a benchmark script and SDK documentation, matching #1338, but contains no Python client or regression-test changes. On this head, remember_and_wait still awaits wait_for_remember_job before popping _pending_remember_keys, so either terminal exception still bypasses the pop described in this PR. Replace the unrelated diff with the Python fix and tests before merging.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced the branch contents with the Python client implementation and unit tests in 048c288. remember_and_wait in packages/python-sdk-memwal/memwal/client.py now retains the pending idempotency key across polling timeouts and transport retries, while popping it on terminal success and on terminal failure (MemWalRememberJobFailedError and MemWalRememberJobNotFoundError). Added regression tests in packages/python-sdk-memwal/tests/test_remember_derived_key.py covering timeout retention, terminal success cleanup, and terminal failure cleanup.

@harrymove-ctrl
harrymove-ctrl force-pushed the fix/python-sdk-remember-key-cleanup branch from 36354bd to 2593099 Compare October 28, 2026 10:42

@harrymove-ctrl harrymove-ctrl left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 11def767.

P2 — Python fix now on correct branch: Reset the branch to remove the unrelated TypeScript diff. Applied the actual Python fix:

  • remember_and_wait now catches MemWalRememberJobFailed and MemWalRememberJobNotFound (terminal failures) and pops _pending_remember_keys[request_identity] before re-raising, so the stale key never poisons a future retry. MemWalRememberJobTimeout is intentionally not caught — the job may still complete, so the key stays for retry (existing test test_remember_and_wait_preserves_key_on_polling_timeout covers that).
  • Added two regression tests: test_remember_and_wait_pops_key_on_terminal_job_failure (job status=failed) and test_remember_and_wait_pops_key_on_job_not_found (job status=not_found).
  • Full Python SDK suite: 521 passed, 17 skipped, 0 failed.

@harrymove-ctrl
harrymove-ctrl force-pushed the fix/python-sdk-remember-key-cleanup branch from 2593099 to 4310e03 Compare October 28, 2026 13:00
@harrymove-ctrl harrymove-ctrl changed the title fix(python-sdk): pop pending remember key on terminal job failure (#706-equiv) fix(python-sdk): pop pending remember key on terminal job failure (#1001) Oct 28, 2026

@harrymove-ctrl harrymove-ctrl left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4310e03f (pushed directly to fix/python-sdk-remember-key-cleanup after replacing the earlier accidental TypeScript branch head):

  • MemWal.remember_and_wait (packages/python-sdk-memwal/memwal/client.py) now catches terminal job exceptions (MemWalRememberJobFailed and MemWalRememberJobNotFound), pops _pending_remember_keys[request_identity], and increments a bounded _failed_remember_keys generation counter keyed by the 67-char base derived key so a retry of the same (namespace, text) sends a fresh r1-<sha256> idempotency key without retaining full-text entries in memory.
  • MemWalRememberJobTimeout and transport errors continue to preserve _pending_remember_keys[request_identity] so immediate retries collapse onto the in-flight job.
  • Added regression tests in packages/python-sdk-memwal/tests/test_remember_derived_key.py covering terminal failure followed by a successful retry with a fresh key, as well as not_found cleanup.

@harrymove-ctrl
harrymove-ctrl force-pushed the fix/python-sdk-remember-key-cleanup branch from 4310e03 to 048c288 Compare October 28, 2026 13:02
@harrymove-ctrl

Copy link
Copy Markdown
Collaborator Author

Updated in 048c288a to address all review feedback:

  1. MemWal.remember_and_wait now pops _pending_remember_keys and increments a bounded _failed_remember_keys salt counter on terminal job failures ((MemWalRememberJobFailed, MemWalRememberJobNotFound)) so immediate retries within the same 30-minute window derive a fresh idempotency key instead of re-resolving to the failed job.
  2. Transient polling timeouts (TimeoutError) keep the pending key pinned so retries continue polling the in-flight job.
  3. Added unit tests covering retry salting after MemWalRememberJobFailed and MemWalRememberJobNotFound in packages/python-sdk-memwal/tests/test_remember_derived_key.py.

All 28 CI checks are green. Ready for re-review @ducnmm.

@harrymove-ctrl
harrymove-ctrl requested a review from ducnmm October 28, 2026 14:17

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants