Skip to content

Commit 9bcb71f

Browse files
committed
store: tasks are durable on Valkey, and the plugin path is what proves it
`busbar_api::Store` defaults `put_task` to `Ok(())`, `get_task` to `Ok(None)` and `list_tasks` to `Ok(vec![])`. This backend overrode none of them, so every task write a Valkey deployment made was DISCARDED and REPORTED AS SUCCESS. store-sqlite and store-mysql already persist tasks; Valkey and Postgres did not, so half the fleet silently fell back to the in-memory default and an operator would have found "task state survives a restart" false on their own deployment, which is the worst place to discover it. Nothing in the existing gate could see it. The conformance suite boots the in-process RAM store, where those defaults ARE the honest answer and nothing looks wrong; and in production this backend is only ever reached AS A PLUGIN, which was the one path with zero coverage of these methods. So the proof added here is over the real path, not the crate: a real dlopen of the built cdylib, the real C ABI, the real DynStore, an unload/reload between the write and the read, and a third leg that reads the same rows back through a plain ValkeyStore that never touches the cdylib at all. RED FIRST, on that test, before a line of the implementation existed: test task_store_survives_an_unload_and_reload_over_the_real_plugin_abi ... FAILED panicked at store-valkey-plugin/tests/e2e.rs:1053:58: an in-flight task must survive the unload/reload over the plugin ABI; got None back, which is exactly the accept-and-keep-nothing shape of the trait default an unimplemented backend (or an unrelayed seam) substitutes This is a key-value store, so the shape is NOT the SQL siblings' shape and that is the substrate answering, not a shortcut: - The row is ONE JSON STRING at `busbar:task:row:<id>`, not a column per field. Valkey indexes nothing by itself, so columns would buy nothing and cost a hash-field encoding for every field. It also means the FULL u64 range round-trips with no clamp and no refusal: the SQL siblings need either an unsigned column (mysql) or an outright refusal (sqlite, postgres) because a clamped artifact_cursor reads back as a different number and then replays delivered artifacts or skips undelivered ones with no error reported. Here there is no ceiling to hit. `the_task_store_round_trips_the_full_u64_range`. - The two questions JSON cannot answer get index structures, exactly as the MCP call log does: `busbar:tasks` (SET) is the enumeration list_tasks walks, and `busbar:tasks:byupdated` (ZSET by updated_at) is the retention sweep's age index. The ZSET is an INDEX, never the truth -- purge_tasks_before re-reads each candidate ROW and re-checks its state and updated_at under WATCH before deleting, so a put_task that resumes an interrupted task between the candidate read and the delete aborts the transaction instead of losing the resumed task. That is also what makes an IEEE-double score safe as a coarse filter, and it is why the count returned is one actually performed rather than the size of the candidate list. - Per-task provenance is one ZSET per task scored by seq, so the read comes back in chain order for free. The upsert on (task_id, seq) is ZREMRANGEBYSCORE-then-ZADD, atomically, and that pairing is load-bearing: a corrected event is a DIFFERENT member string at the SAME score, which a bare ZADD would add ALONGSIDE the old one -- two events at one seq, the exact duplication the contract rules out. The call log's fork check is deliberately NOT copied here; it would be wrong in a way that looks right, because the task-event write-through is specified to be idempotent on replay. - Key construction is collision-free STRUCTURALLY, not by convention. `row:` and `events:` are fixed distinct segments and there is exactly ONE variable component per key, so no task id can make its row key render as another task's events key. This file already carries a recorded hazard where credential keys join caller-supplied components on an unescaped ':' and two distinct tuples render to one key; a task id is protocol-supplied and opaque, colons very much included, so that question had to be answered rather than assumed. `a_task_id_containing_the_key_separator_cannot_alias_another_tasks_chain` is the adversarial spelling of it. SCHEMA_VERSION deliberately does NOT move, and the constant now says why. This keyspace is purely additive, so there is nothing for a migration to do -- and a bump here does not mean "migrate", it means WIPE: migrate() handles any version < SCHEMA_VERSION by SCANning `busbar:*` and deleting everything, whose own justification ("1.5.0 is unreleased") expired three releases ago. The durable MCP call log landed on the same reasoning. The marker moves again when a migrate-in-place path exists to move it for. On the collation class of bug store-mysql fixed (`vk_alice` reading `vk_Alice`'s chain): it has no way in here. A Valkey key is compared as bytes and the terminal-state check is a Rust `==` on &str, so there is no collation to get wrong. `task_ids_differing_only_in_case_are_distinct_tasks` is added to keep it that way, because the consequence would be the siblings' consequence -- two ids collide on one row key and one task is silently lost. Nine live-instance unit tests cover the same surface from the crate side. Gate is green: fmt clean, clippy -D warnings clean, 54 lib + 5 e2e passing.
1 parent ccf08a0 commit 9bcb71f

3 files changed

Lines changed: 1065 additions & 2 deletions

File tree

store-valkey-plugin/tests/e2e.rs

Lines changed: 239 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -936,3 +936,242 @@ fn mcp_call_log_survives_an_unload_and_reload_over_the_real_plugin_abi() {
936936

937937
Store::purge_mcp_calls_before(&direct, u64::MAX).expect("clean up this run's records");
938938
}
939+
940+
/// THE DURABILITY PROOF FOR THE SIX A2A TASK-STORE METHODS, OVER THE REAL PLUGIN PATH.
941+
///
942+
/// The sibling proof above does this for the MCP call log; this one exists because the task methods
943+
/// are a SEPARATE half of the same defaulted seam and half the fleet used to be missing them.
944+
/// `busbar_api::Store` defaults `put_task` to `Ok(())`, `get_task` to `Ok(None)` and `list_tasks` to
945+
/// `Ok(vec![])`: a backend that does not override them ACCEPTS EVERY WRITE AND REPORTS SUCCESS while
946+
/// keeping nothing. An operator would find "task state survives a restart" false on their own
947+
/// deployment, which is the worst place to discover it.
948+
///
949+
/// The conformance suite cannot see this: it boots the in-process RAM store, where those defaults
950+
/// ARE the honest answer and nothing looks wrong. The plugin seam is the ONLY path a real Valkey
951+
/// deployment takes, so it is the only path worth proving on. A unit test against `ValkeyStore`
952+
/// proves the function compiles and works in-process; it does not prove the plugin path reaches it.
953+
///
954+
/// So: a REAL `dlopen` of the built cdylib, the real C ABI, the real `DynStore`. Write at arity > 1
955+
/// (two tasks, one of them UPSERTED a second time, plus two independent provenance chains), DROP the
956+
/// handle — `busbar_close` runs and the library is unloaded, so nothing this process still holds can
957+
/// answer the reads — then `dlopen` again and read everything back. A third leg reads the same rows
958+
/// through the plain `ValkeyStore`, never touching the cdylib, so a plugin answering out of its own
959+
/// in-process cache still fails.
960+
#[test]
961+
fn task_store_survives_an_unload_and_reload_over_the_real_plugin_abi() {
962+
use busbar_api::{TaskEventRow, TaskRow};
963+
964+
let path = plugin_path();
965+
let Some(url) = valkey_url() else {
966+
return;
967+
};
968+
let cfg = serde_json::json!({ "url": url }).to_string();
969+
970+
// Start from an EMPTY task keyspace. `purge_tasks_before` is GLOBAL and terminal-only, and the
971+
// count it returns is asserted exactly below, so a leftover terminal row from an earlier run
972+
// would make that assertion meaningless. `purge_tasks_before(MAX)` is the store's own
973+
// contract-level wipe of exactly that population, so this needs no key-pattern guesswork.
974+
let direct = ValkeyStore::connect(&url).expect("connect directly to clean up and verify");
975+
Store::purge_tasks_before(&direct, u64::MAX).expect("wipe terminal tasks before this run");
976+
977+
let stamp = format!(
978+
"{}_{}",
979+
std::process::id(),
980+
std::time::SystemTime::now()
981+
.duration_since(std::time::UNIX_EPOCH)
982+
.unwrap()
983+
.as_nanos()
984+
);
985+
// Per-run ids, so every read below can only be answered by THIS run's writes.
986+
let t_live = format!("task_abi_live_{stamp}");
987+
let t_done = format!("task_abi_done_{stamp}");
988+
989+
// A timestamp BAND well above any plausible leftover, so the purge cutoff picked below names
990+
// this run's rows and nothing else.
991+
const BASE_TS: u64 = 4_000_000_000;
992+
993+
let task = |id: &str, state: &str, updated_at: u64, cursor: u64| TaskRow {
994+
task_id: id.to_string(),
995+
context_id: format!("ctx-{id}"),
996+
principal: "vk_task_abi".to_string(),
997+
direction: "inbound".to_string(),
998+
state: state.to_string(),
999+
agent_id: "agent-7".to_string(),
1000+
artifact_cursor: cursor,
1001+
push_callback: "https://callback.example/hook".to_string(),
1002+
created_at: BASE_TS,
1003+
updated_at,
1004+
};
1005+
let event = |id: &str, seq: u64, kind: &str, prev: &str, hash: &str| TaskEventRow {
1006+
task_id: id.to_string(),
1007+
seq,
1008+
ts: BASE_TS + seq,
1009+
kind: kind.to_string(),
1010+
context_id: format!("ctx-{id}"),
1011+
principal: "vk_task_abi".to_string(),
1012+
agent_id: "agent-7".to_string(),
1013+
state: "working".to_string(),
1014+
request_id: format!("req-{seq}"),
1015+
prev_hash: prev.to_string(),
1016+
hash: hash.to_string(),
1017+
};
1018+
1019+
{
1020+
// BOOT 1 — a real dlopen of the cdylib; every call below crosses the C ABI.
1021+
let store = load_store(&path, &cfg).expect("the valkey plugin must load over the real ABI");
1022+
store
1023+
.put_task(&task(&t_live, "working", BASE_TS + 100, 3))
1024+
.expect("put_task over the ABI");
1025+
// The SECOND write for the same id: the engine writes through on every state transition, so
1026+
// this must REPLACE the row, never append a second one. An interrupted task waiting on a
1027+
// human is exactly what a restart has to find.
1028+
store
1029+
.put_task(&task(&t_live, "input-required", BASE_TS + 200, 9))
1030+
.expect("put_task over the ABI");
1031+
store
1032+
.put_task(&task(&t_done, "completed", BASE_TS + 50, 1))
1033+
.expect("put_task over the ABI");
1034+
// Two INDEPENDENT chains: per-task provenance that leaked across tasks is a real defect
1035+
// class, and a single-chain test is blind to it.
1036+
for (seq, prev, hash) in [(1_u64, "", "h1"), (2, "h1", "h2"), (3, "h2", "h3")] {
1037+
store
1038+
.append_task_event(&event(&t_live, seq, "task.working", prev, hash))
1039+
.expect("append_task_event over the ABI");
1040+
}
1041+
store
1042+
.append_task_event(&event(&t_done, 1, "task.completed", "", "d1"))
1043+
.expect("append_task_event over the ABI");
1044+
// Dropping the boxed store drops the loader's `Library` handle: `busbar_close` runs and the
1045+
// dylib is UNLOADED. Nothing this process still holds can be answering the reads below.
1046+
drop(store);
1047+
}
1048+
1049+
// BOOT 2 — a second, independent dlopen over the same file, a fresh `busbar_open`, a fresh
1050+
// connection inside the plugin.
1051+
let store =
1052+
load_store(&path, &cfg).expect("the valkey plugin must load again over the real ABI");
1053+
1054+
let got = store.get_task(&t_live).expect("get_task").expect(
1055+
"an in-flight task must survive the unload/reload over the plugin ABI; got None back, \
1056+
which is exactly the accept-and-keep-nothing shape of the trait default an unimplemented \
1057+
backend (or an unrelayed seam) substitutes",
1058+
);
1059+
assert_eq!(
1060+
got,
1061+
task(&t_live, "input-required", BASE_TS + 200, 9),
1062+
"every field must round-trip, and the row read back must be the SECOND write: put_task \
1063+
upserts by task_id"
1064+
);
1065+
assert!(
1066+
store
1067+
.get_task(&format!("task_abi_nonexistent_{stamp}"))
1068+
.expect("get_task on an unknown id is not an error")
1069+
.is_none(),
1070+
"an unknown task id reads back None, not an error"
1071+
);
1072+
1073+
let listed = store.list_tasks().expect("list_tasks");
1074+
let mine = listed
1075+
.iter()
1076+
.filter(|t| t.task_id == t_live || t.task_id == t_done)
1077+
.map(|t| t.task_id.clone())
1078+
.collect::<Vec<_>>();
1079+
assert_eq!(
1080+
mine,
1081+
vec![t_done.clone(), t_live.clone()],
1082+
"list_tasks is UNFILTERED — the terminal row is returned too — and the upserted task \
1083+
appears exactly ONCE; got {} of this run's rows back",
1084+
mine.len()
1085+
);
1086+
1087+
let events = store.list_task_events(&t_live).expect("list_task_events");
1088+
assert_eq!(
1089+
events.iter().map(|e| e.seq).collect::<Vec<_>>(),
1090+
vec![1, 2, 3],
1091+
"the per-task provenance chain must survive the reload, oldest-first by seq; got {} \
1092+
event(s), the empty shape of the trait default",
1093+
events.len()
1094+
);
1095+
for w in events.windows(2) {
1096+
assert_eq!(
1097+
w[1].prev_hash, w[0].hash,
1098+
"the chain must still link after the reload: seq {} carries prev_hash {:?} but seq {} \
1099+
persisted hash {:?}",
1100+
w[1].seq, w[1].prev_hash, w[0].seq, w[0].hash
1101+
);
1102+
}
1103+
assert_eq!(events[2].kind, "task.working");
1104+
assert_eq!(events[2].request_id, "req-3");
1105+
assert_eq!(
1106+
store
1107+
.list_task_events(&t_done)
1108+
.expect("list_task_events")
1109+
.len(),
1110+
1,
1111+
"one task's chain must not carry another's events"
1112+
);
1113+
1114+
// The task-event contract UPSERTS on (task_id, seq) — the engine's write-through is idempotent
1115+
// on replay, and rejecting or duplicating a replayed seq breaks the chain it will verify.
1116+
let mut replayed = event(&t_live, 3, "task.working", "h2", "h3");
1117+
replayed.state = "input-required".to_string();
1118+
store
1119+
.append_task_event(&replayed)
1120+
.expect("a replayed (task_id, seq) upserts rather than erroring");
1121+
let events = store.list_task_events(&t_live).expect("list_task_events");
1122+
assert_eq!(
1123+
events.len(),
1124+
3,
1125+
"a replayed seq must not append a 4th event"
1126+
);
1127+
assert_eq!(events[2].state, "input-required");
1128+
1129+
// Retention crosses the ABI too, COUNT AND ALL — checked for the number it ACTUALLY removed,
1130+
// because a relay that dropped the return value would read as 0 and look like a no-op sweep.
1131+
assert_eq!(
1132+
store
1133+
.purge_tasks_before(BASE_TS + 100)
1134+
.expect("purge_tasks_before"),
1135+
1,
1136+
"only the TERMINAL row older than the cutoff goes; the interrupted task is never swept no \
1137+
matter how old, because an interrupt waiting on a human is exactly the row that \
1138+
legitimately sits still"
1139+
);
1140+
assert!(
1141+
store.get_task(&t_done).expect("get_task").is_none(),
1142+
"the purged task is gone"
1143+
);
1144+
assert!(
1145+
store.get_task(&t_live).expect("get_task").is_some(),
1146+
"a non-terminal task is never purged"
1147+
);
1148+
assert!(
1149+
store
1150+
.list_task_events(&t_done)
1151+
.expect("list_task_events")
1152+
.is_empty(),
1153+
"the purge is the ONLY retention method the contract gives task_events, so a swept task's \
1154+
chain must go with it or it is unbounded forever"
1155+
);
1156+
drop(store);
1157+
1158+
// LEG 3 — read the surviving row through the plain `ValkeyStore`, a code path that never
1159+
// touches the cdylib, the C ABI or the loader. A plugin answering the reads above out of its own
1160+
// in-process state (rather than Valkey) passes both boots and fails here.
1161+
let direct_task = Store::get_task(&direct, &t_live)
1162+
.expect("get_task via the direct connection")
1163+
.expect("the task must be physically present in Valkey, not just cached in-process");
1164+
assert_eq!(direct_task.artifact_cursor, 9);
1165+
assert_eq!(direct_task.state, "input-required");
1166+
assert_eq!(
1167+
Store::list_task_events(&direct, &t_live)
1168+
.expect("list_task_events via the direct connection")
1169+
.len(),
1170+
3
1171+
);
1172+
1173+
// Clean up this run's rows through the contract: mark the survivor terminal, then sweep.
1174+
Store::put_task(&direct, &task(&t_live, "canceled", BASE_TS + 200, 9))
1175+
.expect("clean up this run's task");
1176+
Store::purge_tasks_before(&direct, u64::MAX).expect("clean up this run's rows");
1177+
}

0 commit comments

Comments
 (0)