diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index dfb55d77..ef20d8af 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -379,7 +379,6 @@ fn add_category(value: &str, effects: &mut Vec>) { return; } effects.push(Box::new(effect::AddCategory(name.to_string()))); - effects.push(effect::mark_dirty()); } fn commit_cell_value( diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 8a058530..3790102d 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -734,165 +734,128 @@ mod tests { ); } - /// improvise-955n, the half Copilot found on PR #26 — twice, because the - /// first answer fixed the example rather than the rule. + /// **The capability the contract is for**: a chain of data-dependent + /// commands, applied in order, each seeing what the previous one left. /// - /// Answering every command against the pre-frame `App` is right for the - /// claims a client can actually construct, and it is *only* right while - /// no command in the frame can invalidate another's. Two document - /// mutations break that, and **the failure is silent**: - /// `[RemoveItem("North"), SetCell(the North record)]` passes both checks, - /// then `RemoveItem` deletes the record, `model_effects` resolves the - /// already-answered `SetCell` against the mutated document, finds - /// nothing, and returns an empty effect batch. The frame is reported as - /// accepted having applied only its first command, and the client - /// believes its edit landed. Note this is a **regression the batch check - /// introduced**: under the old per-command check the `SetCell` would have - /// been refused with `RecordGone`. + /// `[AddItem("East"), SetCell(the East record)]` is the archetype, and it + /// is the case a per-command basis check **cannot** admit however it is + /// ordered: a client cannot claim a counter for a record its own chain is + /// about to create, because `BasisCache::claim` filters to records the + /// cache has been told about. So the basis is answered once, on the first + /// command that carries one, and the rest of the chain runs against what + /// its predecessors left. /// - /// So the rule is the count, not a list of dangerous variants: **a frame - /// may carry at most one document mutation**. The first fix refused only - /// the document-*replacing* case, which is the same bug seen through one - /// variant — `[LoadModel, TransposeAxes]` — instead of through the rule. - /// **Three** shapes are driven here, because a guard written for the one - /// that came to mind is what got this wrong three times running - /// (`doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)`). - /// The third is `SwitchView`, which a first version excluded on the - /// grounds that a record surrogate is view-independent — true of - /// `SetCell` and `ClearCell` and false of every other model command, - /// because they all act on the *active view*. - /// - /// The accepting arms are the point of the test as much as the refusals: - /// one mutation surrounded by view commands must land whole, or - /// "refused" would be true for the wrong reason and the batch shape - /// improvise-bpq2 actually produces would be broken. + /// The second assertion is the one that earns the test. Asserting only + /// that the item exists would pass against a host that applied the first + /// command and dropped the second — which is exactly what the previous + /// rule did, loudly, and what the pre-frame check did, silently. #[test] - fn a_frame_carrying_two_document_mutations_is_refused_whole() { + fn a_data_dependent_chain_lands_in_order() { use improvise_protocol::ModelCommand; - // Shape 1: a structural command that invalidates its sibling's - // surrogate. The one that was silently accepted. - let mut structural = host(); - let (asker, _) = structural.connect(10, 10); + let mut host = host(); + let (asker, _) = host.connect(10, 10); + let add = crate::ui::reduce::caught_up( + host.app(), + ModelCommand::AddItem { + category: "Region".into(), + item: "East".into(), + }, + ); + let write = crate::ui::reduce::caught_up( + host.app(), + ModelCommand::SetCell { + cell: addressed("East"), + value: CellValue::Number(9.0), + }, + ); + + let answer = host.handle(asker, ClientFrame::Commands(vec![add, write])); + + assert!( + !answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Rejected(_))), + "a data-dependent chain must not be refused: {answer:?}" + ); + assert!( + host.app() + .model_state + .workbook + .model + .category("Region") + .is_some_and(|c| c.ordered_item_names().contains(&"East")), + "the first command must land" + ); + assert_eq!( + host.app() + .model_state + .workbook + .model + .get_cell(&cell("East")), + Some(&CellValue::Number(9.0)), + "and the second must land on top of it — a write to a record the \ + frame itself created is the whole point" + ); + } + + /// **Characterization of the open hole (clause 4), not an endorsement.** + /// + /// The contract says a frame is all-or-nothing. It is not yet: + /// `App::apply_effects` aborts without undoing (gotcha 21 — an abort is + /// not a rollback), so a chain whose later mutation fails leaves the + /// earlier one standing. `[RemoveItem("North"), SetCell(the North + /// record)]` is the shape: the removal lands, the write then resolves to + /// nothing, `model_effects` returns an **empty** batch, and the frame is + /// reported accepted having applied half of itself. Both the partial + /// application and the silence are open. + /// + /// It is unreachable from any client here — the only producer of a + /// multi-command frame is `keys::commit`, which emits one mutation among + /// view commands — which is what makes it safe to document rather than + /// rush. + /// + /// **This test must be inverted when clause 4 lands.** Its job until then + /// is to make the hole fail loudly if anyone believes it is closed. + #[test] + fn a_frame_whose_later_mutation_fails_is_not_yet_undone() { + use improvise_protocol::ModelCommand; + + let mut host = host(); + let (asker, _) = host.connect(10, 10); let remove = crate::ui::reduce::caught_up( - structural.app(), + host.app(), ModelCommand::RemoveItem { category: "Region".into(), item: "North".into(), }, ); let write = crate::ui::reduce::caught_up( - structural.app(), + host.app(), ModelCommand::SetCell { cell: addressed("North"), value: CellValue::Number(5.0), }, ); - let answer = structural.handle(asker, ClientFrame::Commands(vec![remove, write])); + + let answer = host.handle(asker, ClientFrame::Commands(vec![remove, write])); + assert!( - matches!( - to(&answer, asker).as_slice(), - [ServerFrame::Rejected(Rejection::FrameNotAtomic)] - ), - "{answer:?}" - ); - assert!( - structural + !host .app() .model_state .workbook .model .category("Region") .is_some_and(|c| c.ordered_item_names().contains(&"North")), - "nothing in the frame may have been applied" - ); - - // Shape 2: a document replacement followed by a command that names - // no surrogate at all, so the resolve-to-nothing argument that - // covered shape 1's sibling does not cover it. - let (mut swapper, path) = host_over_a_loadable_document(); - let (asker, _) = swapper.connect(10, 10); - let load = crate::ui::reduce::caught_up(swapper.app(), ModelCommand::LoadModel(path)); - let reshape = crate::ui::reduce::caught_up(swapper.app(), ModelCommand::TransposeAxes); - let answer = swapper.handle(asker, ClientFrame::Commands(vec![load, reshape])); - assert!( - matches!( - to(&answer, asker).as_slice(), - [ServerFrame::Rejected(Rejection::FrameNotAtomic)] - ), - "{answer:?}" - ); - assert_eq!( - swapper.app().model_state.workbook.model.name, - "Test", - "the document must not have been replaced" - ); - - // Shape 3: `SwitchView` is a document mutation too, although it - // carries no basis. Every model command *except* the two that name a - // record by surrogate acts on the **active view**, so a switch - // beside one of them redirects it: this pair transposed `Second` - // rather than the view the transpose was composed against. - let mut switcher = host(); - switcher - .app_mut() - .model_state - .workbook - .views - .insert("Second".to_string(), crate::view::View::new("Second")); - let (asker, _) = switcher.connect(10, 10); - let reshape = crate::ui::reduce::caught_up(switcher.app(), ModelCommand::TransposeAxes); - let answer = switcher.handle( - asker, - ClientFrame::Commands(vec![Command::SwitchView("Second".into()), reshape]), + "TODAY: the first command lands. Under clause 4 it must be undone." ); assert!( - matches!( - to(&answer, asker).as_slice(), - [ServerFrame::Rejected(Rejection::FrameNotAtomic)] - ), - "{answer:?}" - ); - assert_eq!( - switcher.app().model_state.workbook.active_view, - "Default", - "nothing in the frame may have been applied" - ); - - // The accepting arm: one mutation, surrounded by the view commands a - // locally-resolved keypress really does batch with it. - let mut accepted = host(); - let (asker, _) = accepted.connect(10, 10); - let edit = crate::ui::reduce::caught_up( - accepted.app(), - ModelCommand::SetCell { - cell: addressed("North"), - value: CellValue::Number(7.0), - }, - ); - accepted.handle( - asker, - ClientFrame::Commands(vec![ - Command::SetSelected { row: 1, col: 0 }, - edit, - Command::SetRowOffset(1), - ]), - ); - assert_eq!( - accepted - .app() - .model_state - .workbook - .model - .get_cell(&cell("North")), - Some(&CellValue::Number(7.0)), - "one mutation among view commands must still land" - ); - let mirror = accepted.sessions.get_mut(asker).unwrap().view().cursor(); - assert_eq!( - (mirror.selected, mirror.row_offset), - ((1, 0), 1), - "and so must the view commands around it" + !answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Rejected(_))), + "TODAY: the frame is reported accepted. Under clause 4 it must be \ + refused, and this assertion inverts: {answer:?}" ); } diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index b771a64a..a1b2f5f8 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -112,126 +112,116 @@ pub struct Checked<'a>(&'a Command); /// *not even partially*, and that was true of a single command and false of /// a batch. /// -/// # One basis, and therefore at most one document mutation +/// # One basis, checked once, and then the chain runs /// -/// The question that had to be answered before any code moved: what does a -/// frame's basis mean when its later commands were composed against its -/// earlier ones' *effects*? A client resolving one keypress emits -/// `SetSelected` plus the offsets it computed from the selection it just set. +/// The contract, as the user stated it: *"a sequence of commands sent to the +/// server should assume that it's played in order from start to end and that +/// each command sees the state after the previous command runs"*, sent as +/// *"a single transaction from a given base"*. Four clauses: /// -/// There is **one** basis and it describes the document the client was -/// looking at when it composed the frame. That is not a choice: a client -/// cannot claim anything else, because -/// [`BasisCache::claim`](improvise_protocol::BasisCache::claim) hands back -/// what the server last *published*, and the server publishes after a batch -/// and never during one. So no client can hold a basis describing the -/// document as it would be halfway through its own frame. +/// 1. A frame states **one basis**, describing the base the client composed +/// against. +/// 2. That basis is checked **once**, against that base, before anything is +/// applied. +/// 3. Commands are applied **in order**, and command N sees the state command +/// N-1 left. Data dependence between them is expected, not an error. +/// 4. **All or nothing.** If any command fails, the frame leaves the document +/// exactly as the base described. /// -/// **That forces the rule: a frame may carry at most one document -/// mutation** — a [`Command::Model`] or a [`Command::SwitchView`]. -/// Checking every command against the pre-frame `App` -/// is right for the claims that can exist, and it is *only* right while no -/// command in the frame can invalidate another's. Two document mutations in -/// one frame break that, and the failure is silent rather than loud: -/// `[RemoveItem("North"), SetCell(the North record)]` passes both checks, -/// then `RemoveItem` deletes the record and `model_effects` resolves the -/// already-answered `SetCell` against the mutated document, finds nothing, -/// and returns an **empty** effect batch. The frame is reported as accepted -/// having applied only its first command, and the client believes its edit -/// landed. (Found by Copilot on PR #26, against a first version of this type -/// that refused only the document-*replacing* case — which was the same bug -/// spotted through one variant instead of through the rule.) +/// Clauses 1–3 are here. **Clause 4 is not yet implemented** — see the hole +/// below and the design note on PR #26. /// -/// The rule costs nothing real. Two model mutations in one frame could never -/// both be honestly based — the second's claim describes a document that -/// stopped existing when the first landed — so refusing them is not a -/// restriction, it is that incoherence said out loud. A client that wants -/// two sends two frames and composes the second against what the first -/// earns it. And the batch shape that actually exists — `SetSelected` plus -/// the offsets `viewport_effects` returns, optionally around one edit — is -/// untouched, because a view command carries no basis and cannot invalidate -/// one. +/// The basis is checked once and **later commands are not checked against +/// the pre-frame state**, which is not a relaxation but the only contract +/// under which a data-dependent chain can exist at all. The record tier +/// makes that concrete: a client cannot claim a counter for a record its own +/// chain is about to create, because +/// [`BasisCache::claim`](improvise_protocol::BasisCache::claim) filters to +/// records the cache has been told about. So `[AddItem(East), SetCell(East)]` +/// — the archetypal chain — is refused `RecordGone`/`RecordUnstated` by a +/// per-command check no matter what order it is checked in. Per-command +/// checking is not awkward here; it is incapable. /// -/// **`Command::SwitchView` counts too**, although it carries no basis of its -/// own (ADR 001 names that hole and leaves it open). A first version excluded -/// it, on the grounds that a record surrogate is view-independent so a switch -/// cannot invalidate an edit beside it — true of `SetCell` and `ClearCell`, -/// and **false of every other model command**, because `TransposeAxes`, -/// `SetAxis`, `HideItem`, `SetNumberFormat` and their siblings all act on the -/// *active view*. `[SwitchView("Second"), TransposeAxes]` passed both checks -/// and then transposed `Second` rather than the view the transpose was -/// composed against. Copilot found that one too, which makes three rounds of -/// the same mistake: an argument that holds for the command that came to mind. -/// The rule is now "a document mutation", not "a `Command::Model`", and what -/// is left uncounted is only what genuinely cannot reach the document. +/// What the base means is therefore: **what the client saw**. The transaction +/// promises the whole chain lands on top of it or none of it does. The +/// chain's internal coherence is the transaction's job, not the basis +/// check's. /// -/// With that, atomicity is not an argument: at most one command in a frame -/// can be refused, and it is refused before anything is applied. +/// # The hole clause 4 leaves, stated rather than hidden +/// +/// `App::apply_effects` aborts without undoing (gotcha 21 — an abort is not a +/// rollback), so a frame whose second mutation fails leaves the first +/// applied. `[RemoveItem("North"), SetCell(the North record)]` is the +/// example: the removal lands, the write then resolves to nothing and +/// `model_effects` returns an **empty** batch, so the frame is reported +/// accepted having applied half of itself. Both halves of that are open — +/// the partial application and the silence. +/// +/// It is **unreachable from any client in this workspace**: the only producer +/// of a multi-command frame is `improvise-wasm-client::keys::commit`, which +/// emits one mutation among view commands. That is what makes it safe to +/// document rather than rush, and it is why an earlier "at most one document +/// mutation per frame" rule — which closed the hole by forbidding the +/// capability — was rejected. +/// +/// # Two things the rule that was rejected got wrong, worth keeping +/// +/// **Arity is not a property of a variant.** A future `Command::RunScript` is +/// one variant performing an unbounded number of mutations, so no count over +/// the vocabulary can be right: a classifier can honestly answer *whether* a +/// command changes the document, never *how many times*. A +/// `mutates_the_document` predicate written for the rejected rule is deleted +/// with it rather than kept unused — but it was made an **exhaustive match** +/// on review, because a `matches!` answers `false` for a future variant in +/// silence, and clause 4 will need that classifier back. Write it exhaustive +/// when it returns. +/// +/// **The transaction boundary is not the frame.** Because one command may +/// already be a chain, clause 4 is a property the system needs *inside* a +/// command as much as across a frame of them, which is why the design note +/// puts the mechanism at `App::apply_effects` rather than here. pub struct CheckedBatch<'a>(Vec>); impl<'a> CheckedBatch<'a> { /// Answer for every command in a frame, against the state the frame was /// composed against — which is the state *before* any of them runs. /// - /// The first refusal is the frame's answer and nothing is applied. It - /// is a `Result` rather than a per-command verdict for the same reason + /// The refusal is the frame's answer and nothing is applied. It is a + /// `Result` rather than a per-command verdict for the same reason /// [`Checked`] is a type rather than a guard: the only enforceable - /// version of "answer for all of them first" is not being able to reach - /// the commands without having done so. + /// version of "answer before applying" is not being able to reach the + /// commands without having done so. /// - /// The count comes first, because a frame carrying two mutations is one - /// no basis can describe — there is nothing to check it *against* before - /// deciding that. + /// **The basis is answered once**, on the first command that carries one + /// — clause 2 — and every later command is admitted without one, which + /// is clause 3. A frame with no mutation at all answers trivially. + /// + /// The `Basis` on a *later* `Command::Model` is ignored rather than + /// compared, and that is an honest interim rather than the end state: + /// clause 1 says the **frame** states one basis, but the wire still + /// carries a `Basis` inside each `Command::Model` — right when one + /// command was one transaction, and now one field per command describing + /// something that belongs to the frame. Moving it to + /// `ClientFrame::Commands` would make "one basis per frame" + /// unrepresentable-otherwise instead of a convention, and that protocol + /// change belongs with clause 4. pub fn against(app: &App, cmds: &'a [Command]) -> Result { - let mutations = cmds.iter().filter(|cmd| mutates_the_document(cmd)).count(); - match mutations > 1 { - true => Err(Rejection::FrameNotAtomic), - false => cmds - .iter() - .map(|cmd| Checked::against(app, cmd)) - .collect::, _>>() - .map(Self), + let mut answered = false; + let mut checked = Vec::with_capacity(cmds.len()); + for cmd in cmds { + let one = match (answered, cmd) { + // Clause 3: the chain's later commands are not compared to a + // base that describes a document they are not running + // against. + (true, _) => Checked::within_an_answered_frame(cmd), + (false, _) => { + answered = matches!(cmd, Command::Model { .. }); + Checked::against(app, cmd)? + } + }; + checked.push(one); } - } -} - -/// Does this command change the shared document? -/// -/// The two `Command` variants `Sessions::apply` routes away from the mirror -/// (gotchas 36, 38), and it is deliberately the *document* question rather -/// than "does it carry a basis": `SwitchView` carries none and still moves -/// `Workbook::active_view`, which every view-config model command is -/// relative to. Anything else in the vocabulary is the asking client's own -/// view slice and cannot invalidate a sibling's claim. -/// -/// **Exhaustive on purpose**, like [`records_named`], and for the same -/// reason: a `matches!` would answer `false` for a future variant in -/// silence, and silence is exactly the failure mode here — an uncounted -/// document mutation walks past the atomicity guard and is applied against a -/// document a sibling has already changed. A new `Command` variant fails the -/// build until it says which it is (raised by Copilot on PR #26). -fn mutates_the_document(cmd: &Command) -> bool { - match cmd { - Command::Model { .. } | Command::SwitchView(_) => true, - - Command::SetSelected { .. } - | Command::SetRowOffset(_) - | Command::SetColOffset(_) - | Command::ResetCursor - | Command::ChangeMode(_) - | Command::SetStatus(_) - | Command::SetYanked(_) - | Command::SetSearchQuery(_) - | Command::SetSearchMode(_) - | Command::SetBuffer { .. } - | Command::ClearBuffer(_) - | Command::SetTileCatIdx(_) - | Command::SetPanelOpen { .. } - | Command::SetPanelCursor { .. } - | Command::ToggleCatExpand(_) - | Command::HelpPage(_) - | Command::SetTransientPrefix(_) - | Command::Project(_) => false, + Ok(Self(checked)) } } @@ -271,6 +261,17 @@ impl<'a> Checked<'a> { } } + /// A command in a frame whose basis has **already been answered** + /// (clause 2), admitted without a second answer (clause 3). + /// + /// Private, and that is what keeps it from being a bypass: from outside + /// this module the only doors are still [`Checked::against`] and + /// [`CheckedBatch::against`], and the only caller here is the loop that + /// has just answered the frame's basis. + fn within_an_answered_frame(cmd: &'a Command) -> Self { + Self(cmd) + } + /// The command this proof is about. /// /// It hands back a `&'a Command` rather than one borrowed from `&self`, diff --git a/crates/improvise-protocol/src/basis.rs b/crates/improvise-protocol/src/basis.rs index b69436e5..65bb1547 100644 --- a/crates/improvise-protocol/src/basis.rs +++ b/crates/improvise-protocol/src/basis.rs @@ -103,7 +103,7 @@ impl Basis { /// about the host, and a frame carrying two mutations is answered by /// splitting it, not by claiming a fresher basis. /// -/// **Three classes live here**, and the doc enumerates them because the +/// **Two classes live here**, and the doc enumerates them because the /// name says "basis" and only one of them is about a basis: /// /// 1. **Staleness** (ADR 001) — [`Rejection::DocumentReplaced`], @@ -114,10 +114,12 @@ impl Basis { /// [`Rejection::NoDocumentPath`] and [`Rejection::SaveFailed`], answering /// a [`ClientFrame::Save`](crate::ClientFrame::Save) (improvise-368h). /// Nothing went stale; there is nothing to re-compose. -/// 3. **A frame that cannot be answered as one unit** — -/// [`Rejection::FrameNotAtomic`]. Not a fact about the document at all but -/// about the message: it carries two mutations, and one basis cannot -/// describe both. +/// +/// A third class is coming and is not here yet: a **transaction that was +/// rolled back** because a later command in a chain could not be applied +/// (clause 4 of the frame contract — see `CheckedBatch` in +/// `improvise-command`). Until rollback exists there is nothing to report, +/// because nothing is undone. /// /// They share this type rather than getting refusal channels of their own, /// for the reason the type exists: a refusal is "nothing happened, here is @@ -188,20 +190,6 @@ pub enum Rejection { /// fact a client can act on: the document it is editing is not on disk. /// Reporting a failed write as success is the thing this exists to stop. SaveFailed, - /// A `ClientFrame::Commands` carrying **more than one** document - /// mutation, which no single basis can describe (improvise-955n). - /// - /// Not staleness: nothing has moved, and re-sending the same frame would - /// be refused again. It is a fact about the *message* — the second - /// mutation's claim describes a document that stops existing when the - /// first one lands. What a client does is send them as separate frames - /// and compose each against what the previous one earns it. - /// - /// View commands are not mutations and are unaffected: the batch a - /// locally-resolved keypress produces — a selection plus the scroll - /// offsets that follow it, optionally around one edit — is exactly what - /// this permits. - FrameNotAtomic, } /// The basis a client holds, folded from the stream like any other widget diff --git a/crates/improvise-protocol/src/frame.rs b/crates/improvise-protocol/src/frame.rs index 2a21072f..aef7042d 100644 --- a/crates/improvise-protocol/src/frame.rs +++ b/crates/improvise-protocol/src/frame.rs @@ -86,19 +86,17 @@ pub enum ClientFrame { /// Every command is answered against the document the *frame* was /// composed against, which is the only claim a client can construct — a /// basis is published between batches and never during one. And - /// therefore **a frame may carry at most one document mutation** — a - /// [`Command::Model`] or a [`Command::SwitchView`]. Two could not both be - /// honestly based, and the second would be applied against a document its - /// own sibling had already changed. A frame that carries two is refused - /// with [`Rejection::FrameNotAtomic`]. + /// therefore the basis is answered **once**, on the first command that + /// carries one, and every later command is applied against the state its + /// predecessor left. Data dependence between them is expected: a chain + /// may create a record and then write it. Checking a later command + /// against the pre-frame base would refuse exactly those chains, because + /// a client cannot claim a counter for a record it is about to create. /// - /// `SwitchView` counts although it carries no basis of its own, because - /// every model command except the two that name a record by surrogate - /// acts on the **active view**: `[SwitchView("Second"), TransposeAxes]` - /// would transpose `Second` rather than the view the transpose was - /// composed against. View commands are not mutations, so the batch a - /// locally-resolved keypress produces — a selection, its scroll offsets - /// and at most one edit — is unaffected. + /// **All or nothing is the contract and is not yet delivered.** A chain + /// whose later command fails leaves its predecessors applied — + /// `App::apply_effects` aborts without undoing. `CheckedBatch` in + /// `improvise-command` carries the four clauses and the open hole. Commands(Vec), /// Write the document back to wherever this host keeps it /// (improvise-368h). @@ -183,15 +181,13 @@ pub enum ServerFrame { Commands(Vec), /// Nothing was applied, and here is why. /// - /// Three causes, all refusals: the batch was composed against a picture - /// of the document that had moved (ADR 001, improvise-fkfg); it was a - /// [`ClientFrame::Save`] this host cannot honour (improvise-368h); or it - /// was a [`ClientFrame::Commands`] carrying more than one document - /// mutation, which no single basis can describe (improvise-955n). They + /// Two causes, both refusals: the batch was composed against a picture of + /// the document that had moved (ADR 001, improvise-fkfg), or it was a + /// [`ClientFrame::Save`] this host cannot honour (improvise-368h). They /// share a frame because they are the same message — nothing happened, /// addressed to the asker alone — and the [`Rejection`] says which, so /// its variants are the list to match exhaustively rather than this - /// sentence. + /// sentence. A third will arrive with transactional rollback. /// /// A third variant because `Sessions::apply` grew a third case, exactly /// as `Snapshot`/`Commands` is. It is addressed to the one client that diff --git a/crates/improvise-wasm-client/src/socket.rs b/crates/improvise-wasm-client/src/socket.rs index 6d761392..7854bbf1 100644 --- a/crates/improvise-wasm-client/src/socket.rs +++ b/crates/improvise-wasm-client/src/socket.rs @@ -59,11 +59,6 @@ fn rejection_message(rejection: &Rejection) -> String { "this host has nowhere to write the document — it was not saved".into() } Rejection::SaveFailed => "the document could not be written".into(), - // Not staleness and not a host limitation but a badly composed - // frame, so the wording names what to do rather than what moved. - Rejection::FrameNotAtomic => { - "that batch carried two document changes at once — send them separately".into() - } } } diff --git a/crates/improvise-worker-server/src/worker.rs b/crates/improvise-worker-server/src/worker.rs index 54458860..ee2e44a1 100644 --- a/crates/improvise-worker-server/src/worker.rs +++ b/crates/improvise-worker-server/src/worker.rs @@ -510,36 +510,36 @@ mod tests { /// (improvise-gv9r), so a batch that *did* change something would be /// written whatever else the answer contained. /// - /// The fixture is the shape that had the bug — an accepted geometry - /// command followed by a stale one — and the assertion is that the axes - /// on disk and the axes in memory agree. + /// The fixture is a refused frame with a command behind the refusal, and + /// the assertion is that the axes on disk and the axes in memory agree. + /// Under the settled contract the refusal comes from the **frame's** + /// basis — answered once, on the first command that carries one — so + /// that is where the staleness is injected; a later command carries no + /// answer of its own. #[test] fn a_partly_refused_batch_does_not_leave_disk_and_memory_disagreeing() { let (mut worker, path) = worker_over_storage(); connect(&mut worker); - let moves = reduce::caught_up(worker.app(), ModelCommand::TransposeAxes); - let Command::Model { mut basis, cmd } = reduce::caught_up( - worker.app(), - ModelCommand::SetCell { - cell: RecordCell { - record: improvise_core::model::record::RecordHash::of( - &improvise_core::model::cell::CellKey::new(vec![( - "Region".to_string(), - "North".to_string(), - )]), - ), - measure: Some("Revenue".to_string()), - }, - value: CellValue::Number(1.0), - }, - ) else { + // The frame's basis is the FIRST command's, answered once (clause 2), + // so that is where the staleness must be injected: a later command + // carries no answer of its own any more. + let Command::Model { mut basis, cmd } = + reduce::caught_up(worker.app(), ModelCommand::TransposeAxes) + else { unreachable!("caught_up builds a Command::Model") }; basis.document_generation += 1; let stale = Command::Model { basis, cmd }; + let follows = reduce::caught_up( + worker.app(), + ModelCommand::AddItem { + category: "Region".to_string(), + item: "East".to_string(), + }, + ); - let out = worker.on_frame(ClientFrame::Commands(vec![moves, stale])); + let out = worker.on_frame(ClientFrame::Commands(vec![stale, follows])); assert!( matches!(out.as_slice(), [ServerFrame::Rejected(_)]), "the fixture must actually be refused, got {out:?}"