store: the A2A task store and the MCP call log are durable, and a reconnect is what proves it - #7
Open
MattJackson wants to merge 2 commits into
Open
store: the A2A task store and the MCP call log are durable, and a reconnect is what proves it#7MattJackson wants to merge 2 commits into
MattJackson wants to merge 2 commits into
Conversation
…es it
The MCP plane's call evidence rode the admin audit ring: an in-memory,
size-bounded working set shared with admin mutations. Data-plane tool calls
arrive at request rate, so a busy afternoon evicts every admin row from the
ring -- the question of who changed a registration becomes unanswerable
exactly when an incident makes somebody ask. And nothing survived a restart.
This backend has no tables, so the SQL backends' "opaque body plus index
columns" shape becomes three structures:
busbar:mcp:calls:{principal} ZSET scored by seq -- the chain itself, the
member being the whole record as JSON, which
the store never interprets. Scoring by seq is
what makes a read come back in the order the
engine verifies in, for free.
busbar:mcp:principals SET -- the boot enumeration. A restart has to
resume a chain for a principal this process
has not yet seen, and scanning the keyspace
for that answer would be O(keyspace) on the
boot path.
busbar:mcp:byts ZSET scored by ts -- the retention index.
Retention is global by timestamp while the
chains are scored by seq, so without this a
purge would have to read every principal's
entire chain to find what aged out.
One atomic pipeline lands all three. A record in the chain but missing from
the retention index would never age out; one in the index but missing from
the chain would make a purge report a deletion it did not perform.
The purge range is `-inf`..`(before` -- an EXCLUSIVE upper bound, which is
the contract's strictly-older-than, and expressed as a range rather than
`before - 1` because that underflows at zero. The count returned comes from
what ZREMRANGEBYSCORE actually removed, never from the size of the candidate
list, which would over-report if a concurrent purge got there first. A
principal whose chain empties leaves the enumeration, or a restart keeps
resuming a chain with nothing in it.
SCHEMA_VERSION is deliberately NOT bumped. In this backend a version bump
WIPES the whole busbar:* namespace, and its own doc already warns that the
next bump after 1.5.0 ships must not reuse that shortcut. Nothing here needs
it: there is no DDL, so new key patterns are additive by construction, and
bumping would have destroyed live data to add a feature.
The retention index joins seq to principal with U+0001 rather than a colon,
because a busbar key id is caller-visible and may itself contain a colon -- a
separator that can occur in the data is a parser that silently mis-splits,
and the failure would surface as a purge that quietly removed nothing. There
is a test with a colon-bearing principal for exactly that.
The test that carries this drops the store -- closing its connection --
then connects a genuinely new one and asserts the chain still links from what
the server hands back. Run with the four methods removed, so the trait's
accept-and-keep-nothing defaults apply, all five fail, the headline being
"got 0 records back"; that is the behaviour this backend replaces.
`.busbar-ref` moves off the 1.5.3 pin, which predates the contract, onto the
`dev` commit that carries it. CI already builds against the same-named core
branch, so CI and release now name the same line without a hand-held pin.
… what proves it A2A is asynchronous by design. A task spans turns, can sit interrupted waiting on a human, and can outlive the process that started it. The engine has had a seam for that since 1.5.6 -- `put_task`/`get_task`/`list_tasks`/ `purge_tasks_before`/`append_task_event`/`list_task_events` -- and this backend implemented none of them, so the trait's defaults applied: accept the write, return `Ok(())`, keep nothing. Every in-flight task was lost on every deploy, and nothing anywhere reported it, because the return value of a write is not evidence that anything was stored. There are no columns here, so the SQL backends' "every field its own column" becomes "the value IS the row": one string per task holding the whole row as JSON, plus a SET indexing every task id so `list_tasks` -- which the contract defines as returning EVERY row, unfiltered -- is one SMEMBERS rather than a SCAN of the keyspace on the hot path of a boot rehydrate. The row and its index entry land in one atomic pipeline: a row present but unindexed is a task the listing cannot see, so the rehydrate silently loses it. Events live in a HASH per task, field = seq, value = the event as JSON, and NOT in a `seq`-scored ZSET. A ZSET score is an IEEE-754 double, so a seq above 2^53 would silently collide with its neighbours -- exactly the class of silent corruption this store exists not to do -- and the contract's required UPSERT on `(task_id, seq)` is what HSET already is, where a scored ZSET reaches it only via a remove-then-add that is two round trips and a window. The cost is that chain order is restored in-process on read; it is sorted NUMERICALLY, because the field names are decimal strings and a lexicographic sort would place seq 10 before seq 2 and hand the verifier a chain that appears not to link. The upsert is also where the task contract genuinely DIFFERS from `append_mcp_call`'s, a few methods up this same file. That method treats a different record at an occupied sequence as a forked log and refuses it. A task event is specified to upsert so the engine's write-through is idempotent on replay -- "rejecting or duplicating a replayed seq breaks the chain the engine will verify on read". Copying this file's own fork check would have been wrong in a way that looks right. Retention does a full pass over the task rows rather than reading a scored index, and that is deliberate rather than the lazy option. A ZSET scored by `updated_at` hits the same double-precision hazard, no score can express "terminal" so the row would have to be read anyway, and `list_tasks` is already specified as an every-row read the rehydrate performs routinely -- so an every-row sweep is not a new cost class, and it is exact. Terminal is a closed named set: a state token this build has never heard of reads as not-terminal, so a store compiled before a state existed cannot delete a task it does not understand. A purged task takes its provenance chain with it, because `purge_tasks_before` is the only retention method the contract gives this data. A DIVERGENCE FROM THE SQL BACKENDS, asserted rather than assumed. Those store these counters in a signed 64-bit column and must REFUSE a `u64` above `i64::MAX`, because such a value cannot read back as itself. Here the row is JSON, where a `u64` is exact across its whole range, so there is nothing to refuse and a refusal would be inventing a limit this backend does not have. The boundary is proven the other way round instead: `u64::MAX` goes in and comes back out unchanged. The test that carries this drops the store -- closing its connection -- then connects a genuinely new one and reads the task back off the server. Run before the methods existed, against the accept-and-keep-nothing defaults, all seven fail, the headline being "got None back from a new connection"; that is the behaviour this backend replaces. One test-only fix rides along, because it would otherwise redden this branch's CI about once a run. `purge_mcp_calls_before` and `purge_tasks_before` are global by timestamp -- that is the contract, not a shortcut -- and this suite deliberately shares one Valkey without wiping, isolating by key namespace. A sweep does not name the rows it removes, so namespace isolation cannot help it: one test's purge deleted another test's records between that test's append and its read. It presented as "retention quietly removed nothing" and it MOVED from one test to another as the timestamp bands were shuffled, which is how it was diagnosed. The tests that sweep now serialise against each other; the ones that do not sweep place their rows above every cutoff, so no sweep can reach them.
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.
Closes the A2A goal item D.19 for this backend, plus the same-defect-class gap in the MCP tool-call log.
The defect
busbar_api::Storehas ten methods whose defaults are accept and keep nothing —put_task/append_task_event/append_mcp_callreturnOk(()),get_taskreturnsNone, thelist_*methods return empty. This backend overrode none of them. Every in-flight A2A task, and every MCP tool-call record, was lost on every restart, and nothing anywhere reported it: the return value of a write is not evidence that anything was stored.The data model, and where it honestly diverges
There are no columns here, so the SQL backends' "every field its own column" becomes "the value is the row": one string per task holding the row as JSON, plus a SET indexing every task id so
list_tasks— defined by the contract as returning every row, unfiltered — is oneSMEMBERSrather than a keyspaceSCANon the hot path of a boot rehydrate. Row and index entry land in one atomic pipeline.Three places this backend cannot or should not mirror the SQL ones, called out rather than papered over:
seq-scored ZSET. A ZSET score is an IEEE-754 double, so aseqabove 2^53 would silently collide with its neighbours.HSETis also natively the upsert-on-(task_id, seq)the contract requires, where a scored ZSET reaches it only via remove-then-add. Cost: chain order is restored in-process, sorted numerically — the field names are decimal strings, and a lexicographic sort would put seq 10 before seq 2 and hand the verifier a chain that appears not to link.updated_athits the same double hazard; no score can express "terminal" so the row must be read anyway; andlist_tasksis already an every-row read the rehydrate performs routinely, so this is not a new cost class — and it is exact.u64abovei64::MAX. Here the row is JSON, where au64is exact across its whole range, so a refusal would be inventing a limit this backend does not have. The boundary is proven the other way round instead:u64::MAXgoes in and comes back out unchanged.append_task_eventupserts, which is where the task contract genuinely differs fromappend_mcp_call's fork check a few methods up the same file. Copying that check here would have been wrong in a way that looks right..busbar-refBumped from
c8780349(1.5.3) to5a4f0195, thedevcommit that carries both contracts — the old pin predates them and would not compile these methods. The engine-side diff ofcrates/apibetween the two is purely additive:TaskRow,TaskEventRow, and six defaulted trait methods.ci.ymlis unchanged and still builds against the same-named core branch.How this was verified
Red before green. All 7 task tests were run first against the unimplemented state and seen to fail, headline
got None back from a new connection.The property is "it survives a restart." The durability tests drop the store — closing its connection entirely — then connect a genuinely new one and read the row back off the server. A round-trip on one live handle cannot distinguish a real write from a
HashMapbehind the same trait.A test-only fix rides along
purge_mcp_calls_beforeandpurge_tasks_beforeare global by timestamp — that is the contract, not a shortcut — and this suite deliberately shares one Valkey without wiping, isolating by key namespace. A sweep does not name the rows it removes, so namespace isolation cannot help it: one test's purge deleted another test's records between that test's append and its read. It presented as "retention quietly removed nothing" and it moved from one test to another as the timestamp bands were shuffled, which is how it was diagnosed. The sweeping tests now serialise against each other; the non-sweeping ones place their rows above every cutoff. Failed ~1 run in 2 before; 9 consecutive green runs after.Gates run locally
public-hygiene-lint --selftest/--rootcargo fmt --all -- --checkcargo build --all-targetscargo clippy --all-targets -- -D warningscargo test(lib, real Valkey 8)executable-config-lint --selftest/--root