diff --git a/.add/state.json b/.add/state.json index 6821d819..f046efb2 100644 --- a/.add/state.json +++ b/.add/state.json @@ -1,7 +1,7 @@ { "project": "moon", "stage": "production", - "active_task": "client-identity-introspection", + "active_task": "protocol-error-lifetime", "active_milestone": "v0-9-client-compat", "tasks": { "hotpath-lock-quickwins": { @@ -332,14 +332,14 @@ }, "protocol-error-lifetime": { "title": "Protocol errors reply and close cleanly, never stall or eat the valid prefix", - "phase": "ground", + "phase": "tests", "gate": "none", "milestone": "v0-9-client-compat", "depends_on": [ "client-compat-harness" ], "created": "2026-08-09T16:42:35+00:00", - "updated": "2026-08-09T16:42:35+00:00" + "updated": "2026-08-12T04:55:05+00:00" }, "multi-exec-queue-semantics": { "title": "MULTI queues every command, and EXEC answers correctly on every shard count", @@ -476,7 +476,7 @@ } }, "created": "2026-06-11T03:18:21+00:00", - "updated": "2026-08-11T18:20:42+00:00", + "updated": "2026-08-12T04:55:06+00:00", "setup": { "locked": true, "locked_at": "2026-06-11T03:28:00+00:00", diff --git a/.add/tasks/multi-exec-queue-semantics/TASK.md b/.add/tasks/multi-exec-queue-semantics/TASK.md index 81d0fa0f..20f3db08 100644 --- a/.add/tasks/multi-exec-queue-semantics/TASK.md +++ b/.add/tasks/multi-exec-queue-semantics/TASK.md @@ -15,33 +15,131 @@ phase: ground @@ -53,11 +151,62 @@ Assumptions — lowest-confidence first: ```gherkin -Scenario: - Given - When - Then - And # required for every rejection +Scenario: an unknown command poisons the transaction # Must 1,2,3 · Reject 1,5 + Given a connection that has sent MULTI + When it queues NOSUCHCMD, then SET k v, then EXEC + Then the NOSUCHCMD reply is "ERR unknown command 'NOSUCHCMD', with args beginning with: " + And the SET reply is "+QUEUED" + And the EXEC reply is "EXECABORT Transaction discarded because of previous errors." + And k does not exist # the measured data-loss case: Moon SETs it today + +Scenario: wrong arity poisons the transaction # Must 1,2 · Reject 2,5 + Given a connection that has sent MULTI + When it queues GET with no arguments, then EXEC + Then the GET reply is "ERR wrong number of arguments for 'get' command" + And the EXEC reply begins "EXECABORT" + +Scenario: DISCARD clears the poison # Must 4 + Given a connection whose MULTI has been poisoned by an unknown command + When it sends DISCARD, then MULTI, then SET k v, then EXEC + Then the EXEC reply is an array of one "+OK" + And k equals v # the dirty flag did not leak into the next transaction + +Scenario: SUBSCRIBE is refused at queue time, not executed # Must 5 · Reject 3 + Given a connection that has sent MULTI + When it queues SUBSCRIBE ch + Then the reply is "ERR SUBSCRIBE is not allowed in transactions" + And the connection is NOT in subscriber mode # Moon subscribes for real today + And a following EXEC replies "EXECABORT ..." + +Scenario: WATCH is refused inside MULTI # Reject 4 + Given a connection that has sent MULTI + When it queues WATCH k + Then the reply is "ERR WATCH inside MULTI is not allowed" + +Scenario: a malformed frame inside MULTI names itself before the close # Must 6 + Given a connection that has sent MULTI + When it sends a bulk header with a non-numeric length + Then the server replies "ERR Protocol error: invalid bulk length" + And only then closes # Moon closes bare today + +Scenario: a blocking command inside MULTI returns the right NULL TYPE # Must 7 + Given an empty keyspace + When a connection runs MULTI / BLPOP nokey 0 / EXEC + Then EXEC replies an array whose single element is a Null Array ("*-1" in RESP2) + And it is not a Null Bulk ("$-1") # Moon mistypes it today + +Scenario: a runtime error does not abort the block # Must 8 + Given k holds a list + When a connection runs MULTI / GET k / SET other v / EXEC + Then EXEC replies a 2-element array whose first element is a WRONGTYPE error + And other equals v # a runtime error is data, not a queue-time fault + +Scenario: already-matching behavior stays matching # Must 8 + Given a fresh connection + When it exercises nested MULTI, EXEC without MULTI, DISCARD without MULTI, + DISCARD inside MULTI, SELECT inside MULTI, empty MULTI/EXEC, and RESET inside MULTI + Then every reply is byte-identical to redis-server 8.6.1 + And no connection is closed ``` @@ -69,13 +218,47 @@ Scenario: ## 3 · CONTRACT — freeze the shape ▸ docs/05-step-3-contract.md ``` - body: { } - 200 -> { } - 4xx -> { error: "" | "" } -Schema: +The contract is the RESP wire, not an HTTP route. + +MULTI -> +OK (already correct) + ... while queueing -> +QUEUED when COMMAND_META has AND arity matches + -> -ERR unknown command '', with args beginning with: + -> -ERR wrong number of arguments for '' command + -> -ERR is not allowed in transactions (pubsub verbs) + -> -ERR WATCH inside MULTI is not allowed + ... and any of the four errors sets dirty = true; nothing is queued. +EXEC dirty -> -EXECABORT Transaction discarded because of previous errors. + queue cleared · dirty cleared · connection leaves MULTI state + clean -> * of per-command replies (already correct) + no MULTI -> -ERR EXEC without MULTI (already correct) +DISCARD -> +OK, queue cleared, dirty cleared (dirty-clear is NEW) +RESET -> +RESET, queue cleared, dirty cleared + +State: one added `bool` on the existing per-connection MULTI state. No new lock, no new +allocation, no shared/global state — the flag is connection-local by construction, so shard count +cannot affect it. + +Arity source of truth: `COMMAND_META::lookup(name)` + its `arity` field — the SAME table +`dispatch` consults, so a command cannot be queueable-but-undispatchable or the reverse. + +Null-type fix: the zero-timeout path of the blocking family returns `Frame::Array(vec![])`-null +(RESP2 `*-1`), not `Frame::Null` bulk (`$-1`). ``` -Status: DRAFT +Status: FROZEN @ v1 — approved by Tin Dang (2026-08-12) + +Least-sure flag surfaced at freeze: +- [contract] That every command Moon dispatches is present in COMMAND_META with a correct arity. + Why it might be wrong: Moon has THREE dispatch paths, and a command reachable through one but + absent from the table would be REJECTED inside MULTI while still working outside it — a + regression strictly worse than the bug. Cost if wrong: a working command becomes unusable in + transactions. + OUTCOME: **this flag was correct.** COMMAND_META (263 entries) omits TS.*, JSON.*, TXN, FT and + bare GRAPH. Nothing broke only because TXN is intercepted above the queue gate, FT.* is rejected + above it, and TS./JSON. do not exist — four accidents, not a safety argument. Resolved with a + dotted-name carve-out in `queue_time_rejection`, pinned by me10b. +- [scenario] me7 (BLPOP null TYPE) turned out to be unfixable in scope: `Frame` has no null-array + variant, so RESP2 `*-1` is inexpressible anywhere in Moon. Filed; test #[ignore]d with its reason. -phase: ground +phase: tests @@ -15,33 +15,120 @@ phase: ground @@ -53,11 +140,47 @@ Assumptions — lowest-confidence first: ```gherkin -Scenario: - Given - When - Then - And # required for every rejection +Scenario: the valid prefix is answered before the fault # Must 2 + Given a fresh connection + When it writes "PING\r\n*-9\r\n" in ONE write + Then it receives "+PONG" + And the PING is not swallowed # Moon answers nothing at all today + +Scenario: a malformed bulk length names itself # Must 1 · Reject 1 + Given a fresh connection + When it sends a GET whose bulk header is "$abc" + Then it receives "ERR Protocol error: invalid bulk length" + And only then is the connection closed # Moon closes with no error today + +Scenario: a negative multibulk count is ignored, not fatal # Must 5 + Given a fresh connection + When it sends "*-9\r\n" + Then no error is returned + And the connection is still alive: a following PING answers "+PONG" + +Scenario: an oversized inline request names itself # Must 4 · Reject 4 + Given a fresh connection + When it sends an inline command longer than the inline cap + Then it receives "ERR Protocol error: too big inline request" + And only then is the connection closed + # Moon already BUILDS this exact string in src/protocol/inline.rs — it never reaches the client + +Scenario: an unbalanced quote is rejected, not silently accepted # Must 6 · Reject 5 + Given a fresh connection + When it sends 'GET "unclosed\r\n' + Then it receives "ERR Protocol error: unbalanced quotes in request" + And no key lookup is performed # Moon answers "$-1" today, treating the quote as key bytes + +Scenario: an error does not stall the connection # Must 3 + Given a fresh connection + When it writes "@bogus\r\nPING\r\n" in one write + Then it receives an error followed by "+PONG" + And a further PING still answers # Moon answers both, then STALLS on the next command + +Scenario: every handler agrees # Must 7 + Given the same malformed bytes + When they are sent to a monoio server, a sharded-tokio server, and the inline fast path + Then all three produce the same wire bytes and the same connection outcome ``` @@ -69,13 +192,52 @@ Scenario: ## 3 · CONTRACT — freeze the shape ▸ docs/05-step-3-contract.md ``` - body: { } - 200 -> { } - 4xx -> { error: "" | "" } -Schema: +The contract is the RESP wire and the socket lifecycle. + +ParseError gains a typed kind (no new allocation on the error path beyond the existing message): + + enum ProtoFault { BulkLen, MultibulkLen, ExpectedDollar(u8), InlineTooBig, UnbalancedQuotes, + MbulkCountTooBig, UnknownType(u8) } + + ParseError::Invalid { kind: ProtoFault, message: String, offset: usize } + ^^^^ NEW ^^^^ kept: detailed, for logs + fuzz triage + +ProtoFault::wire_text() -> &'static str, Redis 8.6.1 verbatim: + BulkLen -> "Protocol error: invalid bulk length" + MultibulkLen -> "Protocol error: invalid multibulk length" + ExpectedDollar(c) -> "Protocol error: expected '$', got ''" (one formatted case) + InlineTooBig -> "Protocol error: too big inline request" + UnbalancedQuotes -> "Protocol error: unbalanced quotes in request" + MbulkCountTooBig -> "Protocol error: too big mbulk count string" + UnknownType(c) -> "Protocol error: expected '$', got ''" (Redis's own conflation) + +Read-loop contract, IDENTICAL in all three handlers — replacing today's +`Err(_) => { break_outer = true; break; }` which discards BOTH the reason and `batch`: + + Err(ParseError::Incomplete) => break (unchanged: wait for more bytes) + Err(ParseError::Io) => close, no reply (unchanged: the socket is already gone) + Err(ParseError::Invalid{kind, ..}) => + 1. execute and flush `batch` — every valid frame parsed before the fault + 2. write "-ERR \r\n" + 3. flush, then close + +Special case: a negative multibulk count is NOT a fault. `*-9\r\n` consumes its bytes and +yields no frame, matching Redis, so it never enters the Invalid arm at all. + +No new state. No allocation added to the SUCCESS path — `wire_text()` returns &'static str and is +reached only when the connection is already terminating. ``` -Status: DRAFT +Status: FROZEN @ v1 — approved by Tin Dang (2026-08-12) + +Least-sure flag surfaced at freeze: +- [contract] The `@bogus` stall's CAUSE was unknown at freeze — the contract prescribes "must not + stall" without naming the mechanism. Why it might be wrong: a stall whose cause is a shared + buffer-state bug would not be fixed by the read-loop change alone. Cost if wrong: pe6 stays red + and the fix needs a second, unscoped change. + OUTCOME: the read-loop change fixed it; pe6 green on both runtimes. +- [spec] Answering the valid prefix BEFORE closing acknowledges writes into a socket nobody may + read. Redis has the same property, so parity was the tie-breaker.