Skip to content

Commit b0f6d38

Browse files
committed
store: an in-flight A2A task survives a reconnect, and a reconnect is 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.
1 parent 9ca7323 commit b0f6d38

2 files changed

Lines changed: 629 additions & 2 deletions

File tree

store-valkey/src/lib.rs

Lines changed: 198 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,8 @@
7070
7171
use busbar_api::{
7272
AuditRecord, CredentialMeta, CredentialSecret, McpCallRecord, MeteringDelta, MeteringRow,
73-
ModelTokens, Store, StoreError, StoreResult, TierTokens, UsageDelta, UsageLedger, VirtualKey,
73+
ModelTokens, Store, StoreError, StoreResult, TaskEventRow, TaskRow, TierTokens, UsageDelta,
74+
UsageLedger, VirtualKey,
7475
};
7576
use redis::{Commands, Connection};
7677
use std::sync::Mutex;
@@ -124,6 +125,46 @@ const MCP_TS_MEMBER_SEP: char = '\u{1}';
124125
fn mcp_calls_key(principal: &str) -> String {
125126
format!("{MCP_CALLS_PREFIX}{principal}")
126127
}
128+
129+
// ── THE DURABLE A2A TASK STORE ────────────────────────────────────────────────────────────────
130+
//
131+
// An A2A task spans turns, can sit interrupted waiting on a human, and can outlive the process that
132+
// started it, so an in-memory task table loses every in-flight task on restart -- the difference
133+
// between a resume that is real and one that is nominal.
134+
//
135+
// ONE STRING PER TASK, holding the whole row as JSON. This backend has no columns, so the SQL
136+
// backends' "every field its own column" becomes "the value IS the row"; the two things a caller
137+
// needs that a JSON blob cannot answer -- enumerate every task, and find a task's events -- get the
138+
// structures below.
139+
const TASK_PREFIX: &str = "busbar:task:";
140+
/// Every task id currently held. A SET, so `list_tasks` (which the contract defines as returning
141+
/// EVERY row, unfiltered) is one SMEMBERS rather than a SCAN of the whole keyspace on the hot path
142+
/// of a boot rehydrate.
143+
const TASKS_INDEX: &str = "busbar:tasks";
144+
/// One HASH per task, field = `seq` as decimal, value = the event row as JSON.
145+
///
146+
/// A HASH, NOT A ZSET SCORED BY `seq`, and the choice is load-bearing twice over. A ZSET score is an
147+
/// IEEE-754 double, so a `seq` above 2^53 would silently collide with its neighbours -- the exact
148+
/// class of silent-corruption this store exists not to do. And the contract requires an UPSERT on
149+
/// `(task_id, seq)`, which HSET is natively and a scored ZSET is only via a remove-then-add that is
150+
/// two round trips and a window. The cost is that ordering is restored in-process on read rather
151+
/// than by the server, which is bounded by one task's event count and is the right trade.
152+
const TASK_EVENTS_PREFIX: &str = "busbar:taskevents:";
153+
154+
/// The task states that are TERMINAL, and therefore the only ones retention may drop. Named as a
155+
/// closed set rather than derived by negation on purpose: an unrecognised state token -- one a newer
156+
/// engine emits and this build has never heard of -- must read as NOT terminal, so a store compiled
157+
/// before a state existed cannot delete a task it does not understand. The wrong half to guess on is
158+
/// the deleting half.
159+
const TERMINAL_TASK_STATES: [&str; 4] = ["completed", "failed", "canceled", "rejected"];
160+
161+
fn task_key(task_id: &str) -> String {
162+
format!("{TASK_PREFIX}{task_id}")
163+
}
164+
165+
fn task_events_key(task_id: &str) -> String {
166+
format!("{TASK_EVENTS_PREFIX}{task_id}")
167+
}
127168
/// The signed-token REVOCATION denylist (1.5.0). `busbar:denylist:<sub>` holds the operator reason
128169
/// (a plain string), and `busbar:denylist` is a SET indexing every denied sub so `list_denylist` is
129170
/// a SMEMBERS.
@@ -1596,6 +1637,162 @@ impl Store for ValkeyStore {
15961637
Ok(removed)
15971638
}
15981639

1640+
fn put_task(&self, task: &TaskRow) -> StoreResult<()> {
1641+
// NOTE, and it is a real DIVERGENCE from sqlite/postgres/mysql rather than an oversight:
1642+
// those backends store `artifact_cursor`/`created_at`/`updated_at`/`seq`/`ts` in a SIGNED
1643+
// 64-bit column and must REFUSE a `u64` above `i64::MAX`, because such a value cannot read
1644+
// back as itself. Here the row is JSON, where a `u64` is exact across its whole range, so
1645+
// there is nothing to refuse and a refusal would be inventing a limit this backend does not
1646+
// have. `the_full_u64_range_round_trips_because_this_backend_has_no_signed_column` pins it.
1647+
let json = serde_json::to_string(task)
1648+
.map_err(|e| StoreError(format!("task encode failed: {e}")))?;
1649+
// UPSERT BY task_id: the engine writes through on EVERY state transition, so a second write
1650+
// for one task must REPLACE the row, never leave a second one. SET is that natively.
1651+
//
1652+
// One atomic pipeline: the row and its index entry must land together or not at all. A row
1653+
// present but unindexed is a task `list_tasks` cannot see, so a boot rehydrate silently
1654+
// loses it; an index entry with no row makes the same listing report a task that is gone.
1655+
self.with_conn(|c| {
1656+
redis::pipe()
1657+
.atomic()
1658+
.set(task_key(&task.task_id), &json)
1659+
.ignore()
1660+
.sadd(TASKS_INDEX, &task.task_id)
1661+
.ignore()
1662+
.query(c)
1663+
})
1664+
}
1665+
1666+
fn get_task(&self, task_id: &str) -> StoreResult<Option<TaskRow>> {
1667+
// No principal filter, deliberately: the contract puts the caller-scoping check engine-side,
1668+
// because an authorization check living in the backend is one an unauthorized reader
1669+
// bypasses by configuring a different backend.
1670+
let raw: Option<String> = self.with_conn(|c| c.get(task_key(task_id)))?;
1671+
match raw {
1672+
None => Ok(None),
1673+
Some(raw) => serde_json::from_str(&raw)
1674+
.map(Some)
1675+
.map_err(|e| StoreError(format!("task decode failed: {e}"))),
1676+
}
1677+
}
1678+
1679+
fn list_tasks(&self) -> StoreResult<Vec<TaskRow>> {
1680+
// UNFILTERED, terminal rows included. The boot rehydrate wants the active rows, the
1681+
// retention sweep wants the terminal ones and the scoped listing wants one principal's; a
1682+
// store that pre-filtered for any one of those would break the other two.
1683+
let mut ids: Vec<String> = self.with_conn(|c| c.smembers(TASKS_INDEX))?;
1684+
if ids.is_empty() {
1685+
return Ok(Vec::new());
1686+
}
1687+
// A SET has no order; sort so the listing is deterministic across calls and nodes.
1688+
ids.sort();
1689+
// A PIPELINE of GETs rather than one MGET: MGET requires every key to live in the same hash
1690+
// slot, which holds on the single-node deployment this crate targets and stops holding the
1691+
// day somebody points it at a cluster. A pipeline is the same one round trip and has no
1692+
// cross-slot rule at all.
1693+
let raws: Vec<Option<String>> = self.with_conn(|c| {
1694+
let mut pipe = redis::pipe();
1695+
for id in &ids {
1696+
pipe.get(task_key(id));
1697+
}
1698+
pipe.query(c)
1699+
})?;
1700+
let mut out = Vec::with_capacity(raws.len());
1701+
for (id, raw) in ids.iter().zip(raws) {
1702+
// A `None` here is an index entry whose row is gone — the reverse of the pipeline's
1703+
// guarantee, reachable only if somebody deleted the row by hand. Skipped rather than
1704+
// errored: one hand-deleted key must not make the whole boot rehydrate unreadable. The
1705+
// stale index entry is swept by the next purge that reaches it.
1706+
let Some(raw) = raw else { continue };
1707+
let row: TaskRow = serde_json::from_str(&raw)
1708+
.map_err(|e| StoreError(format!("task decode failed for {id}: {e}")))?;
1709+
out.push(row);
1710+
}
1711+
Ok(out)
1712+
}
1713+
1714+
fn purge_tasks_before(&self, before: u64) -> StoreResult<u64> {
1715+
// A FULL PASS OVER THE TASK ROWS, and that is deliberate rather than the lazy option. The
1716+
// SQL backends push this predicate into the server; here the alternative would be a ZSET
1717+
// scored by `updated_at`, and a ZSET score is an IEEE-754 double — a timestamp above 2^53
1718+
// would sort against a value it is not equal to, so the sweep would silently keep or drop
1719+
// the wrong rows. There is also no score that can express "terminal", so a scored index
1720+
// would need this same row read anyway to check `state`. Since `list_tasks` is already
1721+
// specified as an every-row read that the boot rehydrate performs routinely, an every-row
1722+
// retention sweep is not a new cost class — and it is exact.
1723+
let rows = self.list_tasks()?;
1724+
let doomed: Vec<TaskRow> = rows
1725+
.into_iter()
1726+
// STRICTLY less-than, per the contract: a row exactly at the cutoff is kept. And an
1727+
// active or interrupted task is never dropped no matter how old — an interrupt waiting
1728+
// on a human is exactly the row that legitimately sits still for a long time.
1729+
.filter(|t| t.updated_at < before && TERMINAL_TASK_STATES.contains(&t.state.as_str()))
1730+
.collect();
1731+
let mut removed = 0u64;
1732+
for t in &doomed {
1733+
// The count REPORTED is taken from the DEL reply, never from the size of the candidate
1734+
// list: a concurrent sweep that got there first would otherwise make this over-report a
1735+
// deletion it did not perform.
1736+
//
1737+
// The task's provenance chain goes with it. That cascade is load-bearing:
1738+
// `purge_tasks_before` is the ONLY retention method the contract gives this data, so a
1739+
// purge that left the events behind would leave the event keyspace with no bound
1740+
// anywhere in the trait.
1741+
let n: u64 = self.with_conn(|c| {
1742+
redis::pipe()
1743+
.atomic()
1744+
.del(task_key(&t.task_id))
1745+
.del(task_events_key(&t.task_id))
1746+
.ignore()
1747+
.srem(TASKS_INDEX, &t.task_id)
1748+
.ignore()
1749+
.query::<(u64,)>(c)
1750+
.map(|(n,)| n)
1751+
})?;
1752+
removed += n;
1753+
}
1754+
Ok(removed)
1755+
}
1756+
1757+
fn append_task_event(&self, event: &TaskEventRow) -> StoreResult<()> {
1758+
let json = serde_json::to_string(event)
1759+
.map_err(|e| StoreError(format!("task event encode failed: {e}")))?;
1760+
// UPSERT ON (task_id, seq), and this is where the task-event contract genuinely DIFFERS from
1761+
// `append_mcp_call`'s a few methods above: that one treats an occupied slot holding a
1762+
// different record as a FORK and refuses it, while this one is specified to upsert so the
1763+
// engine's write-through is idempotent on replay — "rejecting or duplicating a replayed seq
1764+
// breaks the chain the engine will verify on read". Copying this file's own call-log fork
1765+
// check here would be wrong in a way that looks right.
1766+
//
1767+
// The field is `seq` in DECIMAL, so it is exact for the whole `u64` range; the store never
1768+
// computes or recomputes `hash`/`prev_hash`, it persists them verbatim inside the value.
1769+
self.with_conn(|c| {
1770+
c.hset::<_, _, _, ()>(
1771+
task_events_key(&event.task_id),
1772+
event.seq.to_string(),
1773+
&json,
1774+
)
1775+
})
1776+
}
1777+
1778+
fn list_task_events(&self, task_id: &str) -> StoreResult<Vec<TaskEventRow>> {
1779+
let fields: std::collections::HashMap<String, String> =
1780+
self.with_conn(|c| c.hgetall(task_events_key(task_id)))?;
1781+
let mut out = Vec::with_capacity(fields.len());
1782+
for raw in fields.values() {
1783+
let row: TaskEventRow = serde_json::from_str(raw)
1784+
.map_err(|e| StoreError(format!("task event decode failed: {e}")))?;
1785+
out.push(row);
1786+
}
1787+
// OLDEST-FIRST BY seq — the order the engine's chain verifier reads. Sorted here rather
1788+
// than by the server because the events live in a HASH (see TASK_EVENTS_PREFIX for why
1789+
// that beats a `seq`-scored ZSET), and sorted NUMERICALLY: the field names are decimal
1790+
// strings, so a lexicographic sort would place seq 10 before seq 2 and hand the verifier a
1791+
// chain that appears not to link.
1792+
out.sort_by_key(|e| e.seq);
1793+
Ok(out)
1794+
}
1795+
15991796
fn add_denylist(&self, sub: &str, reason: &str) -> StoreResult<()> {
16001797
self.with_conn(|c| {
16011798
redis::pipe()

0 commit comments

Comments
 (0)