feat(witan): idempotency keys, so retrying an indeterminate write converges - #237
Open
blarghmatey wants to merge 1 commit into
Open
feat(witan): idempotency keys, so retrying an indeterminate write converges#237blarghmatey wants to merge 1 commit into
blarghmatey wants to merge 1 commit into
Conversation
…verges Measured in QA on 2026-08-15: 16 concurrent writers produced 15 INDETERMINATE writes. The store committed every one of them; ToolHive cut the response at exactly 30.000s and returned 502; witan went on to commit at 33.6s, 6.06s after nobody was listening. The caller is told the write failed and the row is there. Retrying that today writes a second row, because the slug's suffix is a fresh uuid4 per attempt and the caller never learns the slug the server chose. With a key the suffix is sha256(key)[:6], so the retry writes the SAME slug and omnigraph's `insert` upserts onto the first row. That upsert behaviour is the load-bearing assumption and was VERIFIED against 0.9.0, not inherited from the note that recorded it: two inserts of one slug leave one row carrying the second write's content. ★ The key identifies the REQUEST, not the content. Deriving the suffix from (kind, title) would need no parameter and is wrong: 29 of 1710 titled records in the real corpus collide on (kind, title, repo), one of them three ways, and since `insert` upserts those merges would be SILENT DATA LOSS. It also must be generated before the first attempt — afterwards the server's slug is unknowable. Wired through memory_store and task_create, the two writes that strand. Omitted, behaviour is exactly as before. RemoteWriteIndeterminate's docstring — which said the operation was unsafe to repeat — now states the precise condition under which it is safe, and that this makes the outcome CONVERGENT rather than known: learning whether the first attempt landed still needs the write receipts omnigraph added after 0.9.0 (upstream #479). The tests assert retry collapse and row counts, not that the parameter exists. Verified by injecting a key-ignoring regression: both collapse tests fail, the other three correctly stay green because they guard different properties. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JYb9sMjetD9Nxjf24Aw3m5
Contributor
There was a problem hiding this comment.
Pull request overview
Adds retry convergence for indeterminate Witan memory and task writes.
Changes:
- Adds deterministic slugs via
idempotency_key. - Adds retry-focused tests and documentation.
- Releases
witan-council0.13.0.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
uv.lock |
Updates the locked package version. |
packages/witan-core/witan_core/remote/proxy.py |
Documents keyed retry safety. |
mcp/servers/witan/witan/server.py |
Implements keyed memory/task slugs. |
mcp/servers/witan/tests/test_memory.py |
Tests retry convergence and compatibility. |
mcp/servers/witan/pyproject.toml |
Bumps the package version. |
mcp/servers/witan/CHANGELOG.md |
Documents the 0.13.0 release. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| prefix = _KIND_PREFIX.get(kind, "mem") | ||
| sanitised = re.sub(r"[^a-z0-9]+", "-", title.lower()).strip("-")[:48] | ||
| short_id = uuid.uuid4().hex[:6] | ||
| if idempotency_key: |
| symbol_refs = [symbol_refs] | ||
| now = now_iso() | ||
| slug = _make_slug(kind, title) | ||
| slug = _make_slug(kind, title, idempotency_key) |
| """ | ||
| now = now_iso() | ||
| slug = _make_slug("task", title) | ||
| slug = _make_slug("task", title, idempotency_key) |
Comment on lines
+165
to
+170
| ★ A RETRY IS SAFE IF — AND ONLY IF — THE ORIGINAL CARRIED AN | ||
| ``idempotency_key``. ``memory_store`` and ``task_create`` derive the slug's | ||
| suffix from that key, so a retry reusing it writes the SAME slug, and | ||
| omnigraph's ``insert`` upserts on the ``@key`` (verified against 0.9.0: two | ||
| inserts of one slug leave one row holding the second write's content). The | ||
| duplicate this class warns about is then structurally impossible. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are the relevant tickets?
N/A — no GitHub issue. Route (c) of
tk-the-write-gate-is-sized-against-a-3-45s-solo-wri-73fc2b: stop the reporting lie.Description (What does it do?)
idempotency_keytomemory_storeandtask_create. With a key, the slug's 6-char suffix issha256(key)[:6]instead of a freshuuid4(), so a retry writes the same slug and omnigraph'sinsertupserts onto the first row instead of creating a second.RemoteWriteIndeterminateis safe — that docstring previously said the operation was unsafe to repeat, full stop.witan-council0.13.0.The problem, measured in QA 2026-08-15. 16 concurrent writers → 15 indeterminate writes. Per-hop trace
003cfcd836f5f9963dd3cd6c7722421b:The store committed every write (16/16, 24/24 at the next level). The caller is told it failed and the row is there. Retrying today duplicates it, because the suffix is random per attempt and the caller never learns the slug the server picked.
How can this be tested?
To confirm the new tests bind, replace the
sha256(idempotency_key…)line in_make_slugwithuuid.uuid4().hex[:6]and re-run —test_same_key_collapses_a_retry_onto_one_rowandtest_task_create_retry_collapses_tooboth fail. I ran it both ways. The other three new tests correctly stay green: they guard different properties (no key leak, distinct memories stay distinct, unkeyed behaviour unchanged).The upsert semantics this rests on were verified directly against 0.9.0 rather than taken from the note that recorded them — two
inserts of one slug leave one row holding the second write's content:Additional Context
The key identifies the request, not the content — and that distinction is the design. Deriving the suffix from
(kind, title)would need no new parameter and looks simpler. It is wrong: 29 of 1710 titled records in the real corpus collide on(kind, title, repo), one of them three ways. Becauseinsertupserts, those merges would be silent data loss. The key must also be generated before the first attempt — afterwards the server's chosen slug is unknowable.This makes an indeterminate outcome convergent, not known. A retry reaches the right end state; it does not reveal whether the first attempt landed. That needs the write receipts omnigraph added after 0.9.0 (upstream #479), which the deployment does not run — we reverted 0.10.0 for halving the write ceiling.
It is inert until callers pass a key. This is the enabling primitive; having witan-core generate one per logical write and reuse it across its own retries is the natural follow-up, and is what would make it work without every agent remembering.
Omitting the key leaves behaviour exactly as today.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JYb9sMjetD9Nxjf24Aw3m5