feat(canvas): add version history with atomic restore - #6780
Conversation
b939919 to
d508818
Compare
Base upstream/main@583af0229; 11 commits of PR block#6780 cherry-picked -x, zero conflicts. Canvas surface byte-identical to PR head; gates green (fmt, buzz-cli 371, buzz-sdk 266, buzz-db 111, clippy clean, desktop 5563). buzz-relay mesh_demo failure proven inherited at pristine upstream tip (504!=200 timing race), recorded not owned.
Base upstream/main@583af0229; 11 commits of PR block#6780 cherry-picked -x, zero conflicts. Canvas surface byte-identical to PR head; gates green (fmt, buzz-cli 371, buzz-sdk 266, buzz-db 111, clippy clean, desktop 5563). buzz-relay mesh_demo failure proven inherited at pristine upstream tip (504!=200 timing race), recorded not owned.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Two blockers at bc07f7c10ef84b2798b28702399e7cf553aace98:
-
Reset canvas UI state when
channelIdchanges.ChannelCanvaskeepsisEditing,draft,editBaseRevision, andshowHistoryin component-local state, but the same unkeyed component instance receives a newchannelIdwhen the still-mounted management sheet switches channels. The query and mutation correctly retarget to the new channel, while the editor state does not. A concrete disclosure path is: start creating canvas A, type A-specific text, switch to canvas-less channel B, then save. The retaineddraftis submitted through B's mutation with the retained"none"precondition, so A's text becomes B's canvas. Key the subtree by channel or synchronously reset/close edit, draft, base revision, history selection, and mutation state on channel changes; add a switch-while-creating regression. -
Do not collapse an accepted publish and a failed verification read into one generic failure. Desktop submits successfully and obtains
result.event_id, then propagates any error fromcurrent_canvas_head_ancestry; CLI restore similarly hasour_idafter accepted submit, then can fail its head re-read before printing it. Desktop invalidates canvas/history only on mutation success and leaves the editor open on error, so retrying after reconnect encounters a stale-precondition conflict against the already-accepted write with no surfaced recovery ID. Preserve the durable accepted state: return/print the known event ID, invalidate/refetch, and expose an explicit “published, verification unavailable” outcome. Cover a successful submit followed by a failed verification query.
Evidence: ChannelCanvas.tsx, ChannelManagementSheet.tsx, canvasHooks.ts, canvas.rs, and channels.rs.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
One blocker at 37601b9b5e624c75918e9fab69b6674825a6f506:
Invalidate canvas caches when an accepted write is reported as superseded. set_canvas publishes the event first, then returns CANVAS_SUPERSEDED if its verification read sees an unrelated head. That revision is durable and recoverable from history, but useSetCanvasMutation invalidates channel-canvas and channel-canvas-history only in onSuccess. The supersession marker rejects the shared save/restore mutation, so neither cache is invalidated and the UI can keep stale current/history data precisely when it tells the user to reload and restore the retained revision.
Move invalidation to onSettled, or explicitly invalidate this recognized durable-supersession outcome. Add regression coverage where setCanvas rejects with the supersession marker after publication and assert that both query keys invalidate/refetch for the save and restore paths.
The two blockers from the prior head are fixed: channel changes now remount the canvas subtree, and a failed post-write verification read now returns durable success with verified: false. The security/trust pass found no blocker.
Evidence: canvas.rs, canvasHooks.ts.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested at 5db94ff72dc8d6aa3f85714fea379068e46f8e84. The append-only history, keyset pagination, timestamp discipline, edit-start snapshot, create guard, channel-switch remount, and settled-outcome cache invalidation are sound. Three user-facing gaps remain:
-
[Medium] Preserve the accepted-but-unverified outcome on restore. Normal save inspects
SetCanvasResult.verifiedand shows the recovery notice, butCanvasHistoryPanel.handleRestorediscards the same result and collapses the selected row. Reproduction: the relay accepts the restore, then the post-write head query fails. The restore is durable, but the user gets no confirmation or History guidance; if cache refetch also fails, the old canvas can remain visible and invite a retry. Surfaceverified: falsenon-destructively for restore just as save does, and add a causal restore regression mirroring the unverified-save test. -
[Medium] Do not report a transitive descendant as supersession.
canvas_write_survivedrecognizes only our event or a head whose directexpected-revisionis ours; Desktop and CLI each pass only that one parent tag (Desktop, CLI). If accepted writes A (ours), B (expected-revision=A), then C (expected-revision=B) all land before A's verification read, C is a legitimate transitive descendant but A is reported asCANVAS_SUPERSEDED/ CLI exit 5. That tells the user to recover a save which remains in the accepted chain. Traverse ancestry with an explicit bound/cycle guard, or otherwise make classification and user guidance truthful; cover A→B→C at the Desktop and CLI seams. -
[Medium, accessibility] Announce mutation outcomes and restore focus. Loading, error, unverified-save, and restore states are inserted as plain
<p>elements without status/alert semantics (save, restore). Successful save/restore also removes the focused control. A keyboard or screen-reader user can receive neither an announced result nor a useful focus destination. Add appropriate live-region semantics and move focus to the resulting canvas/history notice, with interaction coverage. This is required by the repository's WCAG 2.1 AA product target.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested at 599cac2cf5b6f1ea854dea7e5171f6a7aa0699bb. The earlier restore-verification, transitive-ancestry, cache-invalidation, and accessibility/focus gaps are fixed. One UI recovery defect remains:
- [Medium] Reset a rejected save before opening the next edit session.
handleStartEditingresets the draft/base/notice but notsetCanvasMutation. TanStack Query retains the mutation'serror, and the editor renders that error whenever it opens (lines 172–177). Reproduction: let a save reject (stale/superseded/network), click Cancel, then Edit canvas. The prior alert immediately reappears before the user attempts the new session.CanvasHistoryPanelalready avoids the analogous cross-selection leak withrestoreMutation.reset(). Reset this mutation when starting a new canvas edit session, and cover reject → cancel → re-edit.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested at e18a8c292df72e2b689fc6955b51538d69d410dc. The append-only revision model, deterministic pagination, timestamp discipline, ancestry classification, durable-unverified handling, cache invalidation, and prior recovery fixes are sound. Two blockers remain:
-
[High] Pin conflict and post-write verification reads to the writer. Desktop now reads the head before publishing and reads ancestry immediately afterward through generic
query_relay(canvas.rslines 75–99); CLI restore does the same (channels.rslines 417–466). That endpoint routes catch-all/queryreads throughquery_events_routed(bridge.rslines 1421–1429), which may serve the read replica when bounded-staleness routing is enabled. The DB contract explicitly says reads influencing a write must use the writer (buzz-db/src/lib.rslines 1254–1289). Reproduction: enable replica routing with ordinary replication lag, save or restore a canvas, and let the immediate verification query hit the replica before the accepted event arrives. The successful query returns the old ancestry, soclassify_post_writereportsCANVAS_SUPERSEDEDeven though no competitor exists. A stale pre-write read can likewise validate against the wrong head. Add a writer-pinned/fenced query path for preconditions and read-after-write verification, and cover a lagging-replica sequence. Display-only history can remain routed. -
[Medium] Confirm before restore overwrites the shared head. Expanding an old revision exposes “Restore this revision,” and its first activation immediately calls the shared write mutation (
CanvasHistoryPanel.tsxlines 89–105, lines 188–211). An accidental mouse, touch, or keyboard activation while inspecting the adjacent diff changes the channel-wide canvas for everyone. History makes recovery possible, but only by finding the displaced head and performing another shared write. Require an explicit confirmation that identifies the selected revision and says it will become current, or provide an equivalent reversible undo flow. Update the E2E journey, which currently codifies immediate mutation (channels.spec.tslines 3185–3195).
CI is green for this exact head. Per the automation’s read-only policy, I did not execute PR code locally.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested at exact head a0fb8e8a2df43e7f0080f8f4f362db008a112ffc. The prior writer-pinning, restore-confirmation, channel-switch, cache-invalidation, and accessibility issues are fixed. Two blockers remain:
-
[High] Keep persisted empty canvases reachable for read-only members.
ChannelManagementSheetdefines existence from trimmed content and only renders the Canvas ingress whenhasCanvas || canEditNarrative(ChannelManagementSheet.tsxlines 307–312, lines 819–829). A persisted empty revision still exists and has history, but a member without edit permission loses the only ingress and cannot view that shared state or its history. This occurs after intentionally saving or restoring an empty revision. Determine existence fromeventId !== nullfor ingress while retaining content-based preview behavior, and cover the read-only empty-canvas case. -
[High] Do not advance an accepted boundary timestamp beyond the relay’s ingest ceiling. The SDK accepts a head exactly at
client_now + 900and returnshead + 1(builders.rslines 632–640); its boundary test explicitly expectsnow + 901(lines 3125–3137). The authoritative relay rejects timestamps more than 900 seconds from relay time (ingest.rslines 2224–2230). An authenticated writer can therefore publish an accepted head at the ceiling, after which first-party save/restore immediately constructs a rejected event; refreshing the boundary head can keep writes unavailable, and ordinary client/server skew worsens it. Make the relay authoritative for head advancement, or reserve advancement plus clock/network margin in the client limit, and add an ingest-level boundary regression.
Current exact-head CI is otherwise green. Review used GitHub metadata, diff, and exact-head source only; no PR code was executed.
… a11y Carl round 3 (three Medium findings) on the canvas version-history surface: - Restore now inspects the set_canvas result and surfaces the same non-destructive unverified notice as save when verification read fails, instead of discarding the outcome. - canvas_write_survived walks the head's expected-revision ancestry chain (bounded, cycle-guarded) so a transitive descendant landing before the verification read is no longer misclassified as a supersession. Signature changed to take the recent revision slice; desktop and CLI callers fetch the ancestry page and pass it through. - Mutation-outcome states in ChannelCanvas and CanvasHistoryPanel expose role=status/alert and aria-live live-region semantics, and focus is restored to a sensible destination after save/restore settles (WCAG 2.1 AA per VISION.md). Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
A rejected set-canvas save leaves the shared TanStack Query mutation error in place, and the editor renders it whenever it opens. Reopening the editor after a rejection (reject -> Cancel -> Edit) re-surfaced the stale alert before the user acted. Reset the mutation in handleStartEditing, the sole editor-open path, mirroring CanvasHistoryPanel's restoreMutation.reset(). Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Write-influencing canvas reads (a save's head precondition and its post-write ancestry verification) are shape-indistinguishable from display reads, so a replica-routed read could miss the caller's own just-accepted write. Add a client-carried "consistency": "strong" raw-filter extension that pins those specific reads to the writer pool; absent routes normally, any other value is a 400. Only the writer direction is representable — there is deliberately no inverse. Restore mutated the shared channel head with no confirmation, so an accidental click published a new head for everyone. Gate it behind a confirmation dialog naming the target revision. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Gating restore behind a confirmation dialog made the three pre-existing tests that exercise restore (accessibility focus, unverified-restore note, supersession invalidation) click a button that now only opens a Radix AlertDialog, so their set_canvas assertions no longer fired and the dialog's focus machinery threw under bare jsdom. Extract the Radix DOM-global shim into canvasDialogTestEnv and click through the confirm action in each restore path. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Three pieces of mutation-killable evidence for the consistency=strong routing guard:
1. CLI request-filter assertions (buzz-cli):
- set_head_read_carries_strong_consistency: captures /query bodies from
cmd_set_canvas and asserts the kind-40100/limit-1 precondition filter
carries "consistency":"strong"; removing the injection makes it red.
- get_head_read_does_not_carry_consistency: inverse guard — display path
must NOT carry consistency; adding the field would flip this red.
- restore_write_influencing_reads_carry_strong_consistency: asserts IDs
filters (revision fetch) have no consistency, while all non-IDs
kind-40100 filters (pre-write head + post-write ancestry) carry
"consistency":"strong"; mutation oracle covers three injection sites.
2. Desktop filter assertions (desktop/src-tauri):
- head_filter_carries_strong_consistency: directly calls the extracted
canvas_head_filter() helper and asserts consistency=strong; removing
the field from the helper breaks this immediately.
- ancestry_filter_carries_strong_consistency: same for canvas_ancestry_filter().
- get_canvas_filter_does_not_carry_consistency: inverse guard for the
display path.
Extraction of canvas_head_filter/canvas_ancestry_filter into named helpers
is the minimal production change required to make these tests causal.
3. Bridge dispatch test (buzz-relay, #[ignore = "requires Postgres"]):
strong_consistency_dispatches_to_writer_pool_not_replica — lagging-replica
sequence through the real /query router:
- Two scratch Postgres databases (writer + replica), migrations applied.
- Canvas event inserted on writer only; replica pool stays empty.
- Probe 1: routed read (no consistency) → replica → empty; proves the
replica path is genuinely live.
- Probe 2: consistency=strong → writer pool → sees the event; mutation
oracle: changing ReadRoute::Writer arm to query_events_routed returns
empty and assert_eq!(strong_events.len(), 1) fails.
- Probe 3: consistency=weak → 400 Bad Request.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
… follow-up) Fix two test-infrastructure gaps identified in Thufir's round-6 re-check: 1. Desktop display-inverse guards are now causal. Previously get_canvas_filter_does_not_carry_consistency asserted on a JSON literal copied into the test; Thufir showed that adding "consistency":"strong" to the real get_canvas production filter left the test green. Fixed by extracting two named display filter helpers (get_canvas_filter, get_canvas_history_base_filter) and wiring both handlers to consume them, then asserting the helpers directly in the tests. Mutation oracle verified: adding "consistency" to either helper makes the corresponding inverse-guard test red. Added get_canvas_history_base_filter_does_not_carry_consistency as a second causal inverse guard for the history pagination path. 2. Bridge dispatch test wired into Backend Integration CI. strong_consistency_dispatches_to_writer_pool_not_replica was #[ignore] with no CI selector; a Writer→query_events_routed regression could merge with every CI lane green. Added a dedicated "Canvas writer-pin dispatch test" step in the backend-integration job, selecting the test by exact name with --run-ignored ignored-only. The step inherits the job's live Postgres/Redis services and matches the pattern of all neighboring ignored-only selector steps. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
… s, add relay ingest guard
Two Carl findings from review 5058583405:
1. Canvas ingress (ChannelManagementSheet) now keys existence on eventId != null
rather than content length. Restoring to empty leaves a live kind:40100
revision; read-only members must reach it via the ingress row. Extracted
canvasIngressOpen() into canvasIngress.ts; ChannelManagementSheet imports
it; ChannelCanvasIngressGating.test.mjs covers all four cases with a
mutation oracle (3 reds confirmed on the reverted content-only gating).
2. canvas_write_created_at_at(head = now+900, now) returned now+901, which the
relay general ±900 s timestamp check rejects. Fixed on two levels:
- Client (buzz-sdk builders.rs): CANVAS_MAX_FUTURE_SKEW_SECS 900 → 60.
A ceiling head at now+60 produces now+61, well inside the relay bounds.
- Relay (buzz-relay ingest.rs): new CANVAS_MAX_INGEST_FUTURE_SECS = 300 s
kind-40100-specific guard. Extracted as validate_canvas_future_timestamp()
for testability; canvas_ingest_future_timestamp_boundary() covers the
at-ceiling (accepted), at-ceiling+1 (rejected), +901 (rejected), and
past (accepted) cases.
Invariant: client ceiling (60 s) < relay canvas bound (300 s) < relay
general bound (900 s). CLI test set_stamps_ahead_of_future_head_and_asserts_it
updated to use now+30 (within the new 60 s ceiling).
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…ingest wiring, boundary self-reference) Fix 1 (sheet wiring): ChannelCanvasIngressGating.test.mjs now includes two source-wiring assertions that read ChannelManagementSheet.tsx directly. Reverting the call site to 'hasCanvas || canEditNarrative' removes the canvasIngressOpen call → assert.match fails. 8/8 pass at head; 2/8 fail on sheet-call-site mutation. Fix 2 (ingest wiring): two new ignored Postgres tests prove the kind-40100 canvas guard is wired in the shipping ingest path: - handlers::ingest::tests::canvas_ingest_guard_wired_through_ingest_event_inner: calls ingest_event_inner directly with now+301, asserts the canvas- specific rejection message; guard deletion changes it to the h-tag message. - api::bridge::tests::canvas_ingest_future_timestamp_guard_is_wired: goes through the full HTTP router; now+301 must produce the canvas rejection body; guard deletion produces the membership-check body. Both confirm the local Postgres PASS at authorship. Two new CI steps in Backend Integration select them by exact name with --run-ignored ignored-only (same pattern as the existing writer-pin step). Fix 3 (boundary self-reference): added canvas_ingest_numeric_contract (ingest.rs) and canvas_write_created_at_numeric_contract (builders.rs) — standalone tests using ONLY fixed numeric literals, no constants. 300→900 and 60→900 mutations both fail on the new contracts and on the existing boundary tests (verified locally with both mutations). Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…inate clock-race flakiness The two real-ingest wiring tests previously signed events at relay_now+301 and relied on the guard firing before production re-sampled Utc::now(). Thufir reproduced a green-head FAIL (1.72s run): scheduler latency shrunk the nominal +301 to within the 300 s ceiling, letting the event through and changing the rejection reason to the h-tag check. Fix: move both wiring tests to relay_now+600. The +600 offset sits 300 s above the canvas ceiling (300 s) and 300 s below the general drift bound (900 s). Scheduler latency would need to exceed 300 s to erase the margin — not possible under any realistic load. The oracle mechanism is unchanged: guard deletion changes the rejection body from the canvas-specific message to the h-tag (direct) or membership (HTTP) message. Exact 300/301 boundary coverage remains in the pure fixed-literal tests (canvas_ingest_numeric_contract, canvas_ingest_future_timestamp_boundary), which pass fixed arguments to validate_canvas_future_timestamp and have no clock race. Update ci.yml step comments to reflect this separation. Verified locally: - 3/3 direct-ingest runs green at +600 (1.23s, 1.97s, 1.81s) - HTTP bridge run green at +600 (2.02s) - Deletion mutation (guard commented out): direct FAIL with h-tag message, HTTP FAIL with 'not a channel member' body — both oracles causal Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
… relay ingest Add atomic compare-and-insert for canvas (kind-40100) writes: a DB-level advisory-lock CAS that serializes concurrent edits on the same canvas, and relay-side parsing + dispatch for the `expected-revision` precondition tag. The CLI `canvas set` command remains an unconditional replace (no `expected-revision` tag) but still applies writer-pinned timestamp discipline: `created_at = max(now, head + 1)` so an accepted write always becomes the canonical visible head. buzz-db: - Add `ChannelHeadWriteStatus` enum: Inserted, Duplicate, RevisionMissing, RevisionMismatch, SupersedeFailed - Add `ChannelHeadPrecondition`: ExpectNoHead | ExpectedHead(Vec<u8>) - Add `insert_channel_head_checked`: acquires pg_advisory_xact_lock keyed on (community, kind, channel) — excludes author so concurrent writers serialize rather than silently stomp — reads the current head, evaluates the precondition, short-circuits idempotent replay, then calls insert_event_with_thread_metadata_tx + insert_mentions_in_transaction in the same transaction - Re-export ChannelHeadPrecondition and ChannelHeadWriteStatus from buzz-db lib buzz-relay: - Add `CanvasRevisionSpec` enum and `parse_canvas_expected_revision` parser: rejects duplicate/malformed tags; accepts absent (None), "none" (NoHead), or 64-hex id (Head) - Wire CAS dispatch in ingest_event_inner between parameterized-replaceable and generic append: RevisionMissing/RevisionMismatch/SupersedeFailed map to conflict: rejections; Inserted/Duplicate fall through normally - Add 7 parser unit tests covering every branch of parse_canvas_expected_revision buzz-sdk: - Add `build_set_canvas_unconditional_after_head`: stamps created_at = max(now, head+1) for ordering discipline but passes expected_revision=None — no CAS tag buzz-cli: - Rewrite cmd_set_canvas: reads head for timestamp discipline, calls build_set_canvas_unconditional_after_head when head exists, falls back to build_set_canvas(None) when no head. No expected-revision tag emitted. - Add/update set_canvas_tests: set_stamps_ahead_of_future_head_and_is_untagged asserts created_at=head+1 AND no expected-revision tag; set_with_no_head_creates_ without_expected_revision asserts no tag on first create Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Three gates required by review:
1. Real ingest-path CAS test through ingest_event_inner:
canvas_cas_dispatch_wired_through_ingest_event_inner proves the
expected-revision parser → dispatch → DB transaction round-trip end-to-end.
Steps: ExpectNoHead → Inserted, ExpectedHead match → Inserted, stale
same-head competitor → conflict: rejection, loser absent from DB, untagged
write → unconditional append via generic path.
Mutation oracle: removing the canvas_revision_spec dispatch block makes
the stale step return Ok, failing the assert.
Also adds build_canvas_ingest_state helper to keep the test body focused.
2. CI Backend Integration steps for #[ignore]d CAS tests:
- Canvas CAS DB tests: filter pattern channel_head_checked_/ selects all
12 DB tests; --no-tests=fail makes a typo exit with code 4, not silently
pass. --test-threads=1 prevents advisory-lock key contention between
concurrent test runs.
- Canvas CAS ingest-path wiring test: exact-match selector for the new
ingest wiring test.
3. Trim redundancy in event.rs:
- ChannelHeadWriteStatus outer doc: 5-line context → 1 line
- RevisionMismatch/SupersedeFailed variant docs: tightened
- candidate_supersedes_head doc: 7 lines → 2 lines
- insert_channel_head_checked doc: 28-line two-section doc → 12 lines
- Redundant inline comments removed where the doc already covers
Also fix unused CliError import in set_canvas_tests and two
stored.event.id.to_vec() → to_bytes().to_vec() compile errors in
concurrent-first-create test.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…ce, and full serialization boundary Reserve Duplicate exclusively for a byte-identical candidate already proven to be the canonical live head. On a post-insert conflict in insert_channel_head_checked, return RevisionMismatch under the held advisory lock — the idempotent-replay branch above already handled the live-head case, so any conflict here is definitively non-live. Eliminates the H → A → soft- delete A → replay A-on-H false success path Thufir reproduced. Replace the caller-supplied canvas_channel_id hint in soft_delete_event_and_update_thread with internal derivation. The function now reads the target event's kind and channel_id from the database inside its own transaction and conditionally acquires the advisory lock for kind-40100 events. Callers cannot bypass the serialization invariant. Serialize every kind-40100 live-head mutator on the same (community, kind, channel) advisory key. insert_event_with_thread_metadata acquires the key for kind-40100 writes with a channel_id so untagged unconditional appends cannot interleave with concurrent tagged read-check-insert sequences. soft_delete_event_and_update_thread derives target kind/channel and acquires the same key for kind-40100 deletes. Replace the two scheduler-dependent concurrent tests with deterministic equivalents using an external lock-holder that queues both writers before releasing. Add channel_head_checked_deleted_replay_returns_revision_mismatch as a direct regression for the tombstone path. Add channel_head_untagged_canvas_append_serializes_on_advisory_key and channel_head_canvas_deletion_serializes_on_advisory_key using the is_finished() blocker pattern to prove both mutators acquire the key. Mutation reds confirmed for all four oracles. Update CI filter to select the two new serialization tests; bump Canvas CAS DB test count from 12 to 13. Update stale contract comments in buzz-sdk builders.rs, buzz-cli channels.rs, and desktop events.rs to describe authoritative relay CAS semantics; remove residual phase-2/advisory/no-relay-enforcement language. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…ete_event bypass Replace the 50 ms sleep + is_finished() heuristic in both tagged-write race tests with a pg_locks waiter poll. wait_for_advisory_waiters() queries pg_locks for ungranted advisory-lock waiters matching the exact (classid, objid) pair of the canvas coordinate key. Both writer sessions must appear as waiters before the blocker is released, proving they have opened their transactions and reached pg_advisory_xact_lock — causal evidence independent of scheduler timing. Mutation oracle confirmed: removing pg_advisory_xact_lock from insert_channel_head_checked causes wait_for_advisory_waiters to time out (0 waiters observed) and panic with a clear message. Both race tests fail deterministically across 3/3 runs with the mutant applied. Clean source restored. first-create race test: add explicit loser-ID-absent assertion to match the same-head test's correlation proof. Remove Db::soft_delete_event and its free function crate::event::soft_delete_event. Both had zero production callers at this ref and provided a bypass path that could reach kind-40100 rows without acquiring the canvas coordinate advisory lock. The invariant-owning soft_delete_event_and_update_thread is the only public deletion seam. ci.yml: update mutation oracle comments to describe the pg_locks timeout failure mode and the first-create loser-absent assertion. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
… failure After a save or restore settles with verified:false, the mutation's onSettled fires invalidation which triggers a background refetch. When that refetch fails the query enters an error+data state: TanStack Query v5 retains the last successful data alongside the new error. The prior unconditional error guards in both components would return the full destructive error branch, replacing the unverified-save/restore notice and unmounting the cached canvas or history panel. Fix: gate the full error return on data === undefined (no cached data, i.e. a genuine first-load failure). When data is defined alongside an error, remain in the normal render path and show a separate non- destructive refresh warning (channel-canvas-refresh-error / channel-canvas-history-refresh-error) which clears when the next refetch succeeds. Add CanvasRefetchErrorRecovery.test.mjs covering: - save verified:false + refetch failure: notice, canvas, and warning all visible; full error state absent - save recovery: warning clears, notice persists after successful refetch - restore verified:false + refetch failure: history panel stays mounted, restore notice and rows visible, both canvas and history warnings shown - initial-error no-data: full error state still fires when no cache exists Reverting either component's data === undefined guard turns the corresponding scenario(s) red. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
getCanvasCallCount and getCanvasHistoryCallCount were scaffolding variables never read by any assertion — remove them. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…factor The origin/main Postgres-test isolation refactor (#6730) extracted the local test DATABASE_URL fallback into crate::test_support::database_url(). The merge conflict resolution in bridge.rs left two raw TEST_DB_URL references behind; replace them with the new helper to restore compilation. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…irm-time
Without this fix the mutation's CAS guard could silently advance past
the head the user saw when they clicked Restore. Sequence:
1. User opens the restore confirmation dialog (head = A).
2. A background refetch succeeds and installs head C.
3. User confirms — handleRestore read currentRevision from the live
render, submitting expectedRevision: C instead of A.
The relay CAS check then allowed a write the user never approved against
the current head. Fix: capture {revision, frozenExpectedRevision} together
at dialog-open (setConfirmRevision) and pass the frozen value through
handleRestore so the mutation always submits the head the user saw.
Adds a mounted regression: open confirm at head A, successful refetch
installs CONCURRENT head C, confirm — assert set_canvas receives
expectedRevision: A. Revert-causality verified: restoring the live
currentRevision read turns exactly this test red (4/5 pass, new test
fails with actual=CONCURRENT expected=HEAD).
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Four items from the rebase round: 1. Delete five stale canvas CI steps from _ci-relay.yml whose selectors used the old `::tests::` module path. After #6730 renamed all modules to `postgres_tests`, the first step matched zero tests and exited 4. All five tests already run and pass in the new discoverable PostgreSQL Domain lane (396/396 at this head). Deletion confirmed by check-postgres-test-discovery.py: all five tests remain discoverable. 2. Add `disabled={restoreMutation.isPending}` to CanvasHistoryPanel row buttons. Without this guard, clicking another row during a pending restore calls `restoreMutation.reset()`, unobserving the running mutation so a subsequent rejection never reaches `restoreMutation.error` and isPending clears, permitting a second concurrent restore. Regression test: CanvasRestorePendingGuard.test.mjs (mounted, real QueryClient, deferred IPC) — revert-red confirmed. 3. Reconcile ambiguous CLI restore submits in `cmd_restore_canvas`. If A(expected=H) commits but the response is lost, B(expected=A) commits, and the retry of identical A sees RevisionMismatch. The CLI now catches a conflict-shaped relay error and walks the writer-pinned ancestry: A reachable → Conflict (superseded, preserved); A absent → genuine Relay error; ancestry read fails → Other with explicit unknown-outcome naming our_id. Three new CLI unit tests cover all three cases. 4. Fix JSON-only stdout for the already-current short-circuit. The bare `println!("revision ... is already the current revision")` violated VISION.md:163. Now emits `{event_id, accepted:true, message:"already-current"}` on stdout and moves the human-readable note to stderr. Two unit tests cover the short-circuit exit path. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Rustc rejects doc comments (///) on function parameters. Move the event_reachable description into the function's doc block so the CLI test code compiles. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
… tests
- Desktop test: rewrite CanvasRestorePendingGuard.test.mjs to enact the
actual failure mode — reject the deferred IPC while pending, dispatch a
second-row click, assert set_canvas stays at exactly 1 call and the
rejection is visible under the originating row. Revert-red confirmed:
removing disabled={restoreMutation.isPending} from the row button makes
the secondRowIsDisabled assertion fail.
- CLI conflict fixture: rework conflict_relay to model the submit_stored_event
same-byte retry seam — attempt 1 returns 503 (retried by with_retry_body,
A is persisted), attempt 2 returns canonical 409. Reachable case now
returns B(exp=A) as head + A in the stream (not A-as-head), binding the
concurrent-write ancestry shape described by the fixture comment.
- CLI classifier: require status == 409 on the conflict match arm to prevent
any non-409 body lookalike from entering reconciliation. Add
non_409_lookalike_with_conflict_phrase_is_not_reconciled test: relay
captures event ID and returns it as reachable in ancestry walk, so removing
the status guard produces CliError::Conflict instead of Relay{500} —
oracle confirmed. Revert-red confirmed.
- CLI absent case: return original CliError::Relay unchanged (no fabricated
body); test now asserts status == 409 and body contains original phrase.
- CLI unknown outcome: use CliError::DeliveryUnknown (exit 2,
delivery_unknown) instead of CliError::Other (exit 4, error); update
affected test expectation.
- P3 JSON: add out: &mut dyn Write parameter to cmd_restore_canvas (matches
existing warn_sink pattern); already-current short-circuit writes to out;
normal success path also writes to out. Replace two non-asserting tests
with single already_current_restore_emits_json_with_required_fields test
that captures output via Vec<u8>, parses JSON, and asserts event_id,
accepted, and message fields. All callers updated.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
- Route verification-read-fails branch through injected out writer so cmd_restore_canvas captures all stdout; assert event_id/accepted/message in restore_succeeds_when_verification_read_fails. - Scope pending-guard alert query to the originating <li> and assert errorText contains the rejection message; narrow invariant-3 comments to match what the test actually exercises (row expand buttons, not the Restore action button). - Update conflict_relay doc block: attempt 1 returns retryable HTTP 503, not a dropped TCP connection. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…on in 409 reconciliation canvas_write_survived checks whether A is an ancestor of the live head, but it returns false for two distinct cases: (1) A genuinely absent, and (2) A present in the stream but not an ancestor (e.g. an unconditional legacy write B becomes head with no expected-revision link back to A). This caused two wrong outcomes after a lost-response 409: - Legacy write B (no expected-revision) becomes head, A retained in stream: predicate false → original 409 rejection propagated; caller cannot tell A committed. - Tagged write B(expected=A) becomes head: predicate true → CliError::Conflict exit 5 advising re-restore, inconsistent with the accepted-submit path treating the identical descendant chain as success. Fix: establish A's persistence via a separate writer-pinned IDs lookup (fetch_canvas_event_exists), then apply the four-way classification: - survived (ancestor of head): accepted JSON, exit 0 - persisted but head unrelated: supersession naming A - genuinely absent: original 409 relay error - either read fails: DeliveryUnknown naming A Tests: corrected the descendant-ancestor regression (now expects Ok) and added the legacy-supersession regression. Both new/corrected tests confirmed red against the unfixed logic before fixing. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Mechanical conflict resolution: origin/main added react-day-picker (status indicators, #7112) while this branch replaced date-fns with diff (canvas diff view). Merged result keeps both react-day-picker and diff, removes date-fns from the desktop importer. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…event.rs The rebase replay incorrectly placed an inline definition of insert_reaction_event_with_thread_metadata inside the impl Db block (after insert_event's closing brace). This function is defined in crate::reaction and re-exported; it should not exist as a duplicate in event.rs. Applied a clean 3-way merge of the branch against the merge base and origin/main to produce the correct combined state. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
2297a39 to
d3be6bd
Compare
…iation The two load-bearing guarantees on fetch_canvas_event_exists were not falsifiably covered: - An existence-query Err(_) must become DeliveryUnknown (not the original 409 relay error silently misclassified as absent). - The IDs filter must carry consistency=strong to prevent replica-lag from hiding a durable event and producing false absence. Added two tests with confirmed revert-reds: - conflict_shaped_submit_with_failed_existence_read_returns_unknown_outcome: ancestry succeeds with A unreachable (ExistenceReadFails scenario), IDs query returns 500 → asserts DeliveryUnknown naming A's ID. Red: changing Err(_) arm to return the original 409 drops to 13/14. - conflict_shaped_submit_existence_read_carries_strong_consistency: captures the post-conflict IDs query body via the extended conflict_relay fixture and asserts ids=[A], kinds=[40100], #h=[channel], limit=1, consistency=strong. Red: removing consistency=strong from fetch_canvas_event_exists drops to 13/14. The existing ConflictScenario::ReadFails scenario only fails the first ancestry query; neither guarantee was reachable through it. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear for the agreed corrective scope. The remaining CLI restore-recovery blocker from review 5097047079 is resolved at a2a138f357b94c374aa1f917216e44c0d6764140 (base 7a9a5233d9d755e715be0c585cf7850e935d28cf). No actionable blockers found in this source-only re-review.
cmd_restore_canvasnow returns accepted JSON when the submitted revision survives in the live head’s ancestry. Otherwise, a separate writer-pinned ID lookup distinguishes retained-but-superseded history from a genuinely rejected write. Failed reconciliation preserves an explicit unknown outcome and the submitted revision ID (crates/buzz-cli/src/commands/channels.rs:480–533, 614–632).- Traced the production transport, strong-read routing, SDK ancestry, and relay/DB CAS integration. The previously reviewed canvas CAS, legacy serialization, desktop mutation handling, and ancestry semantics remain intact across the rebase. The command-level regression fixtures cover descendant success, legacy supersession, absence, both reconciliation-read failures, and the exact strong-consistency ID filter (
channels.rs:4005–4184, 4297–4547). - Validation was immutable source and test-text inspection only, with independent regression and backend integration reviews. No checkout, build, tests, or PR code execution. The descendant-output test checks JSON field presence rather than exact values; production currently emits the correct values. This is not a runtime/CI pass or an approval.
🤖 Adds append-only channel canvas history, restore, and relay-authoritative optimistic concurrency so stale edits cannot silently replace a newer canvas.
What this adds
buzz canvas history --channel …,buzz canvas get --channel … --revision <id>, andbuzz canvas restore --channel … --revision <id>use keyset pagination over the retained revision stream.expected-revisiontags, bounded ancestry classification, and shared timestamp discipline keep first-party writes ahead of the head they read without propagating poisoned future timestamps.Authoritative concurrency contract
Tagged canvas writes use compare-and-swap semantics in the relay and database:
expected-revision=nonesucceeds only when no live canvas exists.created_at DESC, id ASC.The database serializes tagged writes, unconditional kind
40100writes, and canvas soft deletion on the same(community, kind, channel)advisory key. Untaggedbuzz canvas setremains an unconditional write, but participates in that serialization boundary so it cannot race a tagged save or deletion. Deletion classification is derived inside the datastore transaction; unrelated event kinds do not take the canvas lock.Read and timestamp consistency
Precondition and post-write ancestry reads request strong consistency from
/query, pinning them to the writer when replica routing is enabled. Display-only history remains replica-eligible.First-party canvas writers stamp
created_at = max(now, head + 1)with a conservative client skew limit. The relay applies a canvas-specific future-timestamp ceiling before persistence, preventing an accepted boundary head from making subsequent first-party writes invalid while preserving the broader ingest limit for other event kinds.Durable outcomes and recovery
After an accepted publish, a failed verification read is reported as durable success with
verified: falserather than a failed save. Desktop and CLI preserve the new event ID and direct users to History when verification is unavailable. A successful read walks the bounded, cycle-guardedexpected-revisionancestry chain so transitive descendants are not misclassified as supersession. Canvas caches invalidate for every settled mutation outcome, including a retained but superseded revision.The 409 reconciliation path in
cmd_restore_canvasnow separately establishes persistence evidence from ancestry classification.canvas_write_survivedchecks reachability only; a secondary writer-pinned IDs lookup distinguishes "A absent" from "A present but superseded by a legacy write." The four-way outcome (survived / persisted-but-superseded / genuinely-absent / read-fail) is now correct for all lost-response shapes including unconditional legacy writes.