Repository navigation
Conversation
Add an optional idempotency_key to remember_memory. A retry with the same key and content replays the original creation; different content returns 409 idempotency_conflict. Keys live 24 hours on the database clock in a per-Scope, per-principal namespace and are stored as digests only. Adds migration v1_5_2_001 for SQLite and MySQL-dialect backends, exposes the field over HTTP, the SDK, and MCP, keeps it hidden in Dify, and documents the behavior in both locales. Closes oceanbase#2521
Bind the bundled Desktop contract to Server source df79da6 and its built wheel after the native probe passed all four connection modes.
hidb4ai
left a comment
hidb4ai
left a comment
There was a problem hiding this comment.
The keyed in-process SDK path can replay a creation after scope contribution has been revoked. Please apply the required current authorization before receipt lookup; the inline comment includes a reproduced case.
| stored = await find_receipt(self._database, key) | ||
| if stored is not None and stored.live: | ||
| return _replay_receipt(stored, key, scope_id) |
There was a problem hiding this comment.
[P2] Authorize scope contribution before replaying SDK creates
This early return skips _prepare_atomic_creations, where in-process calls normally reach authorize_creates. With real SQLite and built-in access control, I created a keyed memory as Alice, revoked her scope.contributor binding, then called runtime.atomic_memory.for_scope(scope).create(..., idempotency_key=key, context=alice_context) again: it succeeded and returned the original result. An unkeyed call and a new-key call both raised AccessDeniedError, and the equivalent HTTP retry returned 403. Alice retains ownership of the artifact, so the later exact-revision read checks only artifact.read and cannot enforce the required scope.contribute check. A different payload with the old key also returns BaseValueConflictError before authorization. Enforce current scope contribution before receipt lookup/conflict/replay for the in-process entry point too, as required by the RFC's authorization contract.
There was a problem hiding this comment.
Rechecked 122dd8679efdd9ca9a20396e51dd9267e3112134: the revoked-contributor reproduction is fixed, and a trusted local context now shares the anonymous key namespace correctly.
One P2 authorization case remains in the new preflight guard. It skips authorization whenever execution_context.access is None, whereas the domain policy rejects an explicit context unless trusted_local is true. Reproduced with real SQLite: create a memory locally with key known, then call memory.create(..., context=ArtifactSearchExecutionContext()). No key, an unused key, and known with the original content all raise AccessDeniedError; known with different content instead raises BaseValueConflictError. Thus an explicitly untrusted invocation can distinguish an occupied key before authorization. No memory body was returned and no additional memory was written.
Please preserve the domain's distinction between absent context, trusted local context, and untrusted context before any receipt lookup/conflict response. Add this case alongside the revoked-contributor regression.
An in-process keyed create returned a replay or a conflict before the caller's creation authority was checked. Require it before the receipt lookup so a revoked contributor learns nothing from an old key.
| raise InvalidBaseAccessRequestError( | ||
| "idempotency_key", "must be 1 to 256 printable ASCII characters, no spaces" | ||
| ) | ||
| principal = None if context is None or context.principal is None else context.principal |
There was a problem hiding this comment.
[P2] Share the SDK key namespace when access control is disabled
This branch includes the principal from a valid ArtifactSearchExecutionContext(principal=alice, trusted_local=True) despite its absent access service, so submitting the same key/content without context and retrying with that context selects different receipts and creates duplicate memories, contrary to RFC 1809. Use an empty namespace principal whenever access control is disabled.
A context that named a principal without an access service put that principal into the key namespace, so the same key and content created a second memory. Use the empty principal unless access is enforced.
hireview001
left a comment
hireview001
left a comment
There was a problem hiding this comment.
- CI failed: CI result (failure).
- CI failed: Upgrade release gate (failure).
- CI failed: CI preflight (failure).
- CI failed: Main (failure). Failed steps: CI preflight / Check OpenAPI contract; Upgrade release gate / Require the selected parallel acceptance workflow; CI result / Require successful selected checks.
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "contractSha256": "e1ff2e3ad1d6a09a9288c4c66e111a54c4979026e29806ff70543988b8e29220", | |||
| "contractSha256": "36eb4ebc989a34be1872cd220c5289df81d690ea57d9e5565d50d06046ac9a81", | |||
There was a problem hiding this comment.
[P1] Register qualification for the new Desktop contract
The new bundled digest matches neither a profile nor an authenticated-client mapping in compatibility.json, so check_expected disables business operations on saved connections and connect_local rejects local setup with CompatibilityUnverified. Qualify this exact contract and register its evidence so Desktop connection workflows remain usable.
Which issue or RFC does this PR close?
Closes #2521.
Rationale for this change
A client that times out on
remember_memorycannot tell whether the write was committed, and a blind retry creates a duplicate memory. An optional idempotency key lets the caller retry the same creation safely.What changes are included in this PR?
remember_memoryaccepts an optionalidempotency_key(^[!-~]{1,256}$, compared byte for byte). The same rule applies over HTTP and the in-process SDK.changed: true. Replays are re-authorized and do not extend the key.409 idempotency_conflictwithdetails.kind = "memory_idempotency_key"and writes nothing.v1_5_2_001addspc_atomic_memory_idempotencyfor SQLite and the MySQL dialect; manifests and the release upgrade gate are updated.atomic_memory.idempotency.cleanup_failedand never changes the write result.Out of scope: deduplication across different keys, changes to
expected_revision, automatic client retries, and a general idempotency framework.Are there any user-facing changes?
Yes, additive.
remember_memorygains an optionalidempotency_key, and a new409 idempotency_conflictresponse applies only to keyed requests. Requests without a key behave as before. Upgrading runs migrationv1_5_2_001; no existing data is rewritten.How was this change tested?
tests/e2e/test_remember_memory_idempotency.py(new, 15 cases): replay, creation-state replay after later changes, conflict, key validation over HTTP and the SDK,expected_revisionrejection, expiry and bounded cleanup, concurrent requests in one process and across processes, and per-principal namespaces with re-authorization.tests/builtin/persistence/test_mysql_migration_runner.py -k oceanbase38 passed, 1 failed;tests/e2e/test_database_bootstrap.py -k oceanbase5 passed;tests/e2e/test_atomic_memory_read_snapshot.py -k oceanbase5 passed. A keyed-write scenario through the Server HTTP path (replay, conflict, 3 and 8 concurrent requests on one key, concurrent requests on an expired key) created exactly one memory in every round.test_full_server_upgrade_with_renamed_check[oceanbase-True](stale_ackon the no-op apply after a successful upgrade), fails identically onmasterat63f021d6and is not part of this change.cargo fmt --check,cargo clippy --all-targets -- -D warnings, andpnpm ipc:checkpass;cargo testpassed 130 cases before the profile existed, and thesessionsuite (9 cases, including the two that needed the profile) passed after it was added. Desktop UI typecheck and tests pass.sqlite-memory-idempotency-df79da6b-v1:desktop/tests/real_server.py --product-qualificationpassed anonymous loopback, static Bearer, HTTPS with a base path, and Provider modes against a wheel built fromdf79da6b; seedesktop/VALIDATION.md.generate_api.py --check,generate_js_operations.py --check,release_version.py --check,upgrade_release_gate.py, integration guidance and lifecycle checks,sync_skill.py --check, andruffpass.CI:
dsh-packagefails insetup validates native include trees observed by a real DSH startup. The same case fails onmaster(push run 42951538729 and scheduled run 42919036989) and on unrelated pull requests, so it is not addressed here.Not done: the Skill pins are updated to the new guidance, but no new model evaluation run has been made for it.
AI usage statement
Built with Claude Code (Claude Fable 5.2): design analysis, implementation, tests, documentation, and verification runs. The author reviewed the decisions and results.