store: the A2A task store and the MCP call log are durable, and a restart is what proves it - #8
Open
MattJackson wants to merge 2 commits into
Open
store: the A2A task store and the MCP call log are durable, and a restart is what proves it#8MattJackson 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. So: `mcp_calls`, its own table, a different population from `audit_log` on purpose. The chain is scoped to the PRINCIPAL -- (principal, seq) is the primary key -- because a global chain would serialise every caller behind one append and would make one caller's evidence unverifiable without possessing every other caller's rows. Opaque `body` plus only the columns a query needs. `principal` and `ts` are the index columns; the chain columns -- seq, prev_hash, hash -- are REAL columns rather than buried in the body, because durability here is established by READING THE CHAIN BACK, and a digest reachable only by decoding a payload can be neither constrained nor indexed. `version` and `expires_at` are carried now and written by nothing: adding a column to a populated table later is a rewrite, carrying it from the first migration is free. The insert is ON CONFLICT DO NOTHING, and the incumbent is read afterwards WITHOUT a transaction -- safe precisely because this table is never rewritten, so a row that exists cannot change underneath the read. Identical is the at-least-once retry and succeeds; DIFFERENT is a forked or tampered log and is an error, because overwriting destroys the one case worth reporting. v6 -> v7 is purely additive: the table is new, so the unconditional CREATE TABLE IF NOT EXISTS is the whole migration and no backfill arm exists. The test that carries this drops the store -- closing its connection entirely -- 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, it fails with "got 0 records back"; that is the behaviour this backend replaces. One finding worth keeping: retention is GLOBAL by `ts` and cannot be scoped to a principal. Against the shared live database the purge test was deleting every other test's rows, which is why the suite now bands its timestamps -- the purge test owns the low band and every other test sits above the highest cutoff it uses. The cross-deletion is what surfaced it. `.busbar-ref` moves off the 1.5.3 pin that predates this contract onto the `dev` commit that carries it, so the ref names a core that actually has the methods this backend overrides. CI needs no hand-held pin alongside it: it already builds against the same-named core branch, so dev builds against dev and a release builds against what ships.
…es it A2A is async by design: a task spans turns, can sit interrupted waiting on a human, and can outlive the process that started it. The six task methods on the `Store` trait are DEFAULTED to accept-and-keep-nothing -- `put_task` returns Ok(()) and stores nothing, `get_task` answers None for everything, `list_tasks` empty, `purge_tasks_before` 0 -- and this backend overrode none of them. So every in-flight task on a Postgres-backed deployment was lost on the next deploy, and the write path never said a word about it. That is the difference between a resume that is real and one that is nominal. So: `tasks` keyed on `task_id`, and `task_events` keyed on `(task_id, seq)`. Every field of a task row is a REAL COLUMN, unlike `mcp_calls`'s opaque `body`, and that is not inconsistency. A call record carries open-ended narration no query filters on, so it pays for a blob and saves the columns. A task row is a small fixed state record where every field is part of what a resume reads back, `state` and `updated_at` are the retention sweep's own predicates -- indexed together for exactly that -- and there is no field left over to make opaque. `put_task` UPSERTS on `task_id`, because the engine writes through on EVERY state transition and a second write for one task must replace the row rather than accumulate a second one. `get_task` does NOT filter on principal: the contract puts caller-scoping engine-side deliberately, since an authorization check living in the backend is one an unauthorized reader bypasses by configuring a different backend. `list_tasks` is UNFILTERED, terminal rows included -- the boot rehydrate wants the active rows, the retention sweep the terminal ones and the scoped listing one principal's, so a store that pre-filtered for any one of those would break the other two. `append_task_event` UPSERTS on `(task_id, seq)`, and this is the one place the task contract genuinely DIFFERS from `append_mcp_call`'s in the commit before this one. That one treats an occupied slot holding a different record as a forked log and refuses it. This one 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 the call log's fork check across would have been wrong in a way that looks right, so the difference is stated at the method rather than left to be inferred. Retention drops TERMINAL rows only, strictly older than the cutoff. The terminal set is named as a closed list rather than derived by negation: an unrecognised state token -- one a newer engine emits and this build has never heard of -- must read as NOT terminal, because the wrong half to guess on is the deleting half. An interrupted task waiting on a human is precisely the row that legitimately sits still for a long time, and compacting it is losing the work, not reclaiming space. The purge CASCADES to the task's events, via a trigger rather than a foreign key. The cascade itself is load-bearing: `purge_tasks_before` is the ONLY retention method the contract gives this data, so a purge that left the provenance behind would leave `task_events` with no bound anywhere in the trait. A real FK would give the cascade for free but would also impose an ORDER on the writes -- no event could be appended before its task row existed -- and the engine is under no such obligation, since a `task.submitted` event and the first `put_task` are two independent write-throughs. The trigger buys the bound without inventing an ordering constraint the contract never stated, and putting it in the database rather than in the DELETE's own SQL means a row removed by any other route (an operator's psql session, an external job) takes its chain with it too. Postgres BIGINT is signed, so a `u64` past `i64::MAX` cannot round-trip. `clamp` would pin it and the row read back would not be the row written, with nothing ever reporting an error -- and the value that silently changes is the ARTIFACT CURSOR, i.e. how much of a stream has been durably relayed, so a pinned cursor either replays delivered artifacts or skips undelivered ones. Refused instead, exactly as `append_audit` already refuses its own out-of-range seq/ts, across all five affected fields (artifact_cursor, created_at, updated_at, event seq, event ts). Nothing escalates durability per write the way store-sqlite has to: sqlite raises `synchronous` to FULL around these writes because its default is weaker, whereas Postgres's default `synchronous_commit = on` already fsyncs the commit that carries the transition. Same property, already the default. v7 -> v8 is purely additive: two new tables and their cascade, so `SCHEMA`'s own unconditional CREATE TABLE IF NOT EXISTS is the whole migration and no `version < 8` backfill arm exists. Postgres has no CREATE TRIGGER IF NOT EXISTS, so that one object is created under an explicit pg_trigger existence check -- DROP-then-CREATE would also be idempotent but would take an ACCESS EXCLUSIVE lock on `tasks` at every connect, for nothing. THE TEST THAT CARRIES THIS drops the store -- closing its connection entirely -- then connects a genuinely NEW one to the same database and reads the task back off the server. A round-trip through one live handle proves nothing: it cannot tell a backend that wrote to Postgres from one holding a HashMap behind the same trait, nor either of those from the trait's own defaults exercised through the very handle that "wrote". Run against the unimplemented state, all eight new tests fail, and they fail on the property rather than on a detail: "an in-flight task must survive a restart; got None back on a new connection", "got 0 events back on a new connection", the unfiltered listing comes back `[]` against four written rows, the purge reports 0 where 4 rows were owed, and the range guard returns Ok for a cursor of u64::MAX. 42 passed, 8 failed before; 50 passed after. Each of these tests owns a disposable database rather than banding its timestamps in the shared one, and that is a consequence of the method: `purge_tasks_before` is GLOBAL by `updated_at` and cannot be scoped to a task or a principal, so against a shared database a retention test's cutoff deletes every other test's terminal rows. Isolation also lets the assertions be EXACT -- "the purge removed four rows", "these seven survive" -- rather than the `>=` a shared table forces, and an exact count is what proves a purge performed the number it reported. Gates run locally against a real postgres:16 and a sibling busbarAI at the .busbar-ref commit: public-hygiene-lint --selftest and --root, cargo fmt --all --check, cargo build --all-targets, cargo clippy --all-targets -D warnings, and cargo test with BUSBAR_TEST_POSTGRES_URL set.
Contributor
Author
CI is red for a reason outside this PR, and here is the proof
CI checks out busbar at branch Reproduced locally, both directions, from the same plugin workspace:
So this is an engine-side breakage that will redden every plugin repo's CI, on every PR, regardless of its contents, until Deliberately not worked around here. The obvious dodge — pinning |
Contributor
Author
|
Update: store-mysql#6 has now failed at the identical step on the identical six busbar files, and store-sqlite#7 / store-valkey#7 are green only because their runs checked busbar out before |
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
1 participant
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.Two commits, one per plane. On the task side:
tasks— everyTaskRowfield a real column,task_idprimary key,(state, updated_at)index for the retention sweep.put_taskUPSERTsON CONFLICT (task_id) DO UPDATE: the engine writes through on every state transition, so a second write must replace the row.task_events—(task_id, seq)primary key, andappend_task_eventUPSERTs. This is where the task contract genuinely differs fromappend_mcp_call's fork check — the contract says a store "must upsert on that pair … rejecting or duplicating a replayedseqbreaks the chain the engine will verify on read". Copying the call log's behaviour here would have been wrong in a way that looks right.AFTER DELETEtrigger. The cascade is load-bearing: there is nopurge_task_events_beforein the trait, so without ittask_eventshas no bound. A trigger rather than an FK, because an FK would also impose write ordering — no event before its task row — and the engine is under no such obligation.Schema v7 → v8, additive: new tables only, no backfill, nothing dropped.
.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; nothing else in theStoretrait moved. The full pre-existing suite was green against the new ref immediately after the cherry-pick, so the bump dragged in nothing that broke.ci.ymlis unchanged and still builds against the same-named core branch.How this was verified
Red before green. All 8 new tests were run first against the unimplemented state and seen to fail —
got None back on a new connection,left: 0 / right: 4on the purge count, and so on.The property is "it survives a restart." Each durability test uses a disposable database, drops the write handle to close its connection, and reads the rows back through a new
PostgresStore::connect. A round-trip on one live handle cannot distinguish a real write from aHashMapbehind the same trait. Isolated databases rather than the shared one, becausepurge_tasks_beforeis global byupdated_atand cannot be scoped — that isolation is also what lets the purge counts be asserted exactly instead of as>=.Gates run locally
public-hygiene-lint --selftest/--rootcargo fmt --all -- --checkcargo build --all-targetscargo clippy --all-targets -- -D warningscargo test(store suite, live PG 16)executable-config-lint --selftest/--rootadmin_api_e2eadmin_api_e2efails locally withboot #2's admin listener never came up within 15s. Bisected: it fails identically with this change reverted to the cherry-pick commit, and under the same conditions even boot #1 — memory store, no plugin at all — fails the same way, with the child hanging in_dyld_start. The machine was saturated by three sibling worktrees compiling. So this is environmental and not caused by this change, but it is unverified on this branch and Linux CI is the arbiter.Where Postgres cannot mirror sqlite exactly — stated, not papered over
synchronousto FULL around these writes; Postgres's defaultsynchronous_commit = onalready fsyncs the commit. Same property, different mechanism.plpgsql. Present by default on PG (including the tested 16), but a cluster with it removed would failmigrateat schema creation.BIGINT. Au64pasti64::MAXis refused rather than stored — same observable behaviour as sqlite, reached because this repo'sclampidiom would pin rather than wrap.The other half — this is not shippable durability yet
busbar-plugin-abi'sStoreRequestat5a4f0195has no variants for any of these ten methods, andbusbar-plugin-loader'sDynStore— thedyn Storethe engine actually holds when a store is loaded as a plugin — overrides none of them, so it falls through to the same accept-and-keep-nothing defaults. Neighbouring methods (put_usage,put_key_with_credential,lookup_credential_secret) are implemented there, so this is a real gap rather than a mis-read.Concretely: file-drop and runtime-install are the only two ways an operator gets this plugin, both go through
DynStore, and across that boundaryput_taskis still a no-op no matter how correct this repo is. The backend side is now right and waiting; the ABI/SDK/loader half is core work and belongs inbusbarAI.