diff --git a/context/code/clients.md b/context/code/clients.md index b3e6448b..73c7cbde 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1472,14 +1472,24 @@ so another client's write to *that same record* bumped the counter in the client's `BasisCache`. The commit then claimed the fresh counter and was accepted, and the other write was silently overwritten. If the other client cleared the cell instead, the commit re-created it. An `EditTarget` is -`{ cell, version }`: the record's counter as the client had it when the editor -opened. `BasisCache::claim_as_read` states that version beside the *current* -document tiers. +`{ document_generation, cell, version, opened }`: the document and the record's +counter as the client had them when the editor opened, and the seed it opened +on (improvise-ixxx, above). `BasisCache::claim_as_read` +states both beside the *current* `layout_version`. -The document tiers stay current on purpose. The rows renumbering under the -editor, which was 2wu3's case, moves `layout_version`. Claiming that at open -would refuse every such commit as `LayoutMoved`, although the identity already -makes the commit safe. So the result is: +**The two document tiers are not alike, and only the layout stays current.** +The rows renumbering under the editor, which was 2wu3's case, moves +`layout_version`. Claiming that at open would refuse every such commit as +`LayoutMoved`, although the identity already makes the commit safe. The +generation is the opposite case (improvise-vndi). A `RecordHash` is derived +from coordinates, so after another client's `LoadModel` the same record can +resolve in the incoming document, and a fresh `DataStore`'s counter can equal +the one the editor read. Claiming the current generation then got the commit +**accepted**: the loaded document's value, which this user never saw, was +overwritten. Nothing composed against the outgoing document names anything in +the incoming one, which is `Rejection::DocumentReplaced`'s own contract, so the +generation is captured at open and the swap refuses the commit. So the result is: +- the document was replaced: `DocumentReplaced`; - the record moved: `RecordMoved`; - the record is gone: `RecordGone`; - only the rows moved: the commit is accepted. @@ -1491,10 +1501,11 @@ row. So the record resolves, and its counter has moved. `RecordGone` needs a record that neither the store nor the layout resolves. **`EditTarget` is the one object an editor captures at open, and it is a -struct so it can grow.** improvise-ixxx needs the seed captured at open too (a -commit whose buffer equals the seed writes nothing). That is a `seed: String` -field beside `cell` and `version`, and the page does not change, because the -target is opaque to it. +struct so it can grow.** Two fields have joined `cell` and `version` since: +the seed as `opened` (improvise-ixxx: a commit whose buffer equals the seed +writes nothing) and the generation (improvise-vndi). The page did not change +for either, because the target is opaque to it. `typed_target_frame` asks the +two in that order: an untouched editor writes nothing, so it is never refused. `typed_cell_command` is the one composition shared by the mouse path (`typed_target_frame`) and the keyboard path (`keys::commit`). Writing it twice is diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 3f91c403..1b893acb 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -5098,3 +5098,22 @@ on row 1. The test presses `i`, types `9`, and asserts the first editor really is touched. Enter must then land on (2, 0), and the folded `opened_editor` must be exactly `OpenedEditor::on("800")`. **Injection:** `resolve::advanced` carrying the previous editor's `touched` flag. This test fails alone. + +### an_edit_opened_before_a_document_swap_is_refused_in_the_new_document + +improvise-vndi, through a real `Host` over `MemoryFs`. A opens an inline editor +on North/Revenue, which shows 1200. B sends `LoadModel` for a document that +differs only in that cell, which holds 5555. Every name, the view included, is +kept, per gotcha 39's fixture warning: a rename would relay a `SwitchView` and +hide the bug. A folds the rebase `Snapshot` with the editor still open, as +`web/app.js` does, and then commits "42". The `EditTarget` held `{cell, +version}` and `claim_as_read` paired that version with the *current* +generation. `RecordHash` is coordinate-derived, so the same record resolved +in the new document at a matching counter, and the host **accepted** the +commit. It failed at the `DocumentReplaced` assertion: the answer was +`Accepted`, and 5555 had become 42. + +Rival injected: `claim_as_read` ignoring the captured generation and claiming +the current one. That is the pre-fix behaviour, restated after the fix. Only +this test failed in the whole workspace. The layout tier staying current is +still pinned by `an_edit_writes_the_cell_the_user_clicked_after_the_rows_renumber`. diff --git a/crates/improvise-protocol/src/basis.rs b/crates/improvise-protocol/src/basis.rs index 43108c66..4884e5be 100644 --- a/crates/improvise-protocol/src/basis.rs +++ b/crates/improvise-protocol/src/basis.rs @@ -94,15 +94,16 @@ impl BasisCache { } impl BasisCache { - /// A basis for records whose counters were read EARLIER, such as when an - /// editor opened (improvise-0j0f). The document tiers are current. + /// A basis for what was read EARLIER, such as when an editor opened: the + /// document and its records then, the layout now (improvise-0j0f, improvise-vndi). pub fn claim_as_read( &self, + document_generation: u64, read: impl IntoIterator, ) -> Option { let current = self.current.as_ref()?; Some(Basis { - document_generation: current.document_generation, + document_generation, layout_version: current.layout_version, records: read.into_iter().collect(), }) diff --git a/crates/improvise-wasm-client/src/socket.rs b/crates/improvise-wasm-client/src/socket.rs index 2da28cd7..1f1c0d21 100644 --- a/crates/improvise-wasm-client/src/socket.rs +++ b/crates/improvise-wasm-client/src/socket.rs @@ -12,6 +12,8 @@ use serde::{Deserialize, Serialize}; /// A struct so a field such as the seed can join it — `context/code/clients.md`. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct EditTarget { + /// The document the editor opened on; a swap under it refuses the commit (improvise-vndi). + pub document_generation: u64, pub cell: RecordCell, /// The record's write counter as this client had it at open. pub version: u64, @@ -117,9 +119,12 @@ impl Client { /// version of it the user is looking at. `None` before any basis arrives. pub fn edit_target_at(&self, row: usize, col: usize) -> Option { let cell = self.record_cell_at(row, col)?; - let version = self.state().basis.current()?.record(cell.record)?; + let basis = self.state().basis.current()?; + let version = basis.record(cell.record)?; + let document_generation = basis.document_generation; let seed = self.state().grid.cell(row, col)?.edit_text.clone(); Some(EditTarget { + document_generation, cell, version, opened: OpenedEditor::on(seed.unwrap_or_default()), @@ -132,10 +137,10 @@ impl Client { if untouched(Some(&target.opened), text) { return None; } - let basis = self - .state() - .basis - .claim_as_read([(target.cell.record, target.version)])?; + let basis = self.state().basis.claim_as_read( + target.document_generation, + [(target.cell.record, target.version)], + )?; let cmds = vec![Command::Model { basis, cmd: typed_cell_command(&improvise_protocol::CellId::Record(target.cell), text)?, diff --git a/crates/improvise-wasm-client/tests/browser_round_trip.rs b/crates/improvise-wasm-client/tests/browser_round_trip.rs index f92e9deb..1ab86c87 100644 --- a/crates/improvise-wasm-client/tests/browser_round_trip.rs +++ b/crates/improvise-wasm-client/tests/browser_round_trip.rs @@ -817,6 +817,70 @@ fn an_edit_is_refused_when_the_cell_was_cleared_while_the_editor_was_open() { ); } +/// improvise-vndi: an editor opened before another client's `LoadModel` must not +/// write into the loaded document. Evidence: test-evidence/clients.md. +#[test] +fn an_edit_opened_before_a_document_swap_is_refused_in_the_new_document() { + use improvise_command::ui::reduce::caught_up; + use improvise_protocol::{ModelCommand, Rejection, ServerFrame}; + + let north = CellKey::new(vec![ + ("Region".into(), "North".into()), + ("_Measure".into(), "Revenue".into()), + ]); + // The incoming document keeps every name, view included (gotcha 39's + // fixture warning), and differs only in the one value the edit targets. + let path = std::path::PathBuf::from("other.improv"); + let mut host = host(); + host.app_mut().storage = Box::new(improvise_command::storage::MemoryFs::new()); + let mut other = host.app().model_state.workbook.clone(); + other + .model + .set_cell(north.clone(), CellValue::Number(5555.0)); + improvise_command::persistence::save_to(&*host.app().storage, &other, &path).unwrap(); + + let mut editor = Client::new(); + let (a, snapshot) = host.connect(10, 10); + deliver(&mut editor, &[snapshot]); + let (b, _) = host.connect(10, 10); + + // A opens an editor on North/Revenue, which shows 1200. + let target = editor + .edit_target(0, 0) + .expect("serialization must not fail") + .expect("a stored cell has an edit target"); + + // B loads the other document; A folds its rebase Snapshot, editor still open. + let generation = host.app().document_generation(); + let load = caught_up(host.app(), ModelCommand::LoadModel(path)); + let relayed = host.handle(b, ClientFrame::command(load)); + assert_eq!( + host.app().document_generation(), + generation + 1, + "setup: the load must really replace the document" + ); + let for_a: Vec = relayed.into_iter().filter(|f| f.to == a).collect(); + deliver(&mut editor, &for_a); + + let commit = editor + .typed_cell(&target, "42") + .expect("serialization must not fail") + .expect("a stored cell composes a write"); + let answer = send(&mut host, a, &commit); + + assert!( + answer + .iter() + .any(|f| matches!(f.msg, ServerFrame::Rejected(Rejection::DocumentReplaced))), + "the edit was composed against the outgoing document: {answer:?}" + ); + assert_eq!( + host.app().model_state.workbook.model.get_cell(&north), + Some(&CellValue::Number(5555.0)), + "the loaded document's value was overwritten by an edit its user never saw" + ); +} + // ── numbers across the serialized wire (improvise-6oy4) ───────────────── /// A value serde_json's default f64 parser reads back one ULP low. Evidence file.