mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-23 19:06:33 +00:00
xarmian
1dbe04399a
* fix(store): optimistic concurrency for collection settings writes (BUG-2265) Collection-level settings (e.g. quick_actions) were written by reconstructing the whole settings JSON from a caller's local Collection snapshot, and UpdateCollection replaced the column with no concurrency check. Two ItemDetails in the same collection (full-page pane host master + pane) hold independent snapshots and clobbered each other. Mirror the item optimistic-concurrency pattern (IDEA-1480): add CollectionUpdate.ExpectedUpdatedAt; when set, UpdateCollection re-reads updated_at atomically under the workspace write lock (SQLite BEGIN IMMEDIATE / Postgres advisory xact lock) and returns CollectionUpdateConflictError on a mismatch. Empty token keeps the legacy last-write-wins path unchanged for CLI/MCP/API callers. No DB migration — reuses collections.updated_at. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * feat(server): collection.updated broadcast + 409 conflict mapping (BUG-2265) - handleUpdateCollection boundary-validates expected_updated_at (400 on a malformed token) and maps store.CollectionUpdateConflictError to the shared update_conflict envelope (HTTP 409) — byte-identical wire shape to the item path, via the extracted writeUpdateConflictEnvelope helper. - Add the collection_updated EventBus type and publish it after a successful update so sibling ItemDetails / collection pages refresh their independent Collection snapshot proactively, shrinking the 409 window. Routed by Collection (slug) through the existing SSE visibility filter. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(web): 409-aware collection settings writes + sibling refresh (BUG-2265) - CollectionUpdate carries expected_updated_at; add isUpdateConflictError. - QuickActionsMenu sends the token and, on a 409, refetches the collection, re-appends the new action onto the FRESH settings, and retries once — no silent loss, no user-visible error. - EditCollectionModal captures the token at open-time (edge-gated seed so a concurrent broadcast can't wipe in-progress edits) and shows a non-destructive "changed elsewhere, reload" message on 409 rather than auto-merging a full-form edit. - Subscribe to collection_updated over SSE: ItemDetail and the collection page refresh their own Collection snapshot (gen/slug-fenced against the persistent pane host's no-remount switch), so siblings converge before the next save. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(store): harden collection optimistic concurrency + web fetch ordering (BUG-2265, Codex round 1) Address Codex review findings: - [P1] same-second clobber: now() is one-second precision, so two guarded writes in the same second kept an identical token. The accepted write now advances updated_at strictly past the token (only when now() hasn't already moved on), making a stale-token replay deterministically conflict. Add a same-second regression test. - [P1] tokenless-writer race on Postgres: the advisory lock only serialized writers that also took it. Replace it with a `FOR UPDATE` row lock on the in-tx re-read (Postgres) — SQLite's BEGIN IMMEDIATE already serializes every writer — so a concurrent tokenless UpdateCollection can't slip between the re-read and the UPDATE. - [P2] rename broadcast: only publish collection_updated when the slug is unchanged. A rename's old-slug event would make siblings refetch a dead slug (404) and a new-slug event can't reach old-slug visibility snapshots; renames are handled by the existing navigation path. - [P2] out-of-order refreshes: ItemDetail and the collection page now use a dedicated monotonic refresh counter so two rapid collection_updated fetches can't resolve out of order and clobber newer state (loadSeq/loadGeneration only bump on route/item loads). Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(store): make collection updated_at strictly monotonic for ALL writes (BUG-2265, Codex round 2) The previous same-second advance ran only on guarded updates, so a tokenless UpdateCollection could write the current second over a forced expected+1s, regressing the concurrency token and letting a stale guarded client clobber newer data (Codex P1). Route every collection update through one small transaction that re-reads updated_at (FOR UPDATE on Postgres; SQLite BEGIN IMMEDIATE covers it) and derives the new timestamp atomically: strictly advance past the row's current value when now() hasn't already moved on. This makes updated_at a reliable concurrency token for guarded AND tokenless writers. Add a tokenless-monotonic regression test. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix: close remaining collection-concurrency gaps (BUG-2265, Codex round 3) - [P1] Board column reordering (handleGroupReorder) rebuilt the full schema from a stale local snapshot and wrote it with no token — a lost-update path identical to the bug being fixed. Now sends expected_updated_at and, on 409, refetches, re-applies the reorder onto the fresh schema, and retries once. - [P2] The workspace settings page seeded EditCollectionModal from a page-load-time collections list, so a change that predated editing produced a false 409. It now refreshes the list on collection_updated (seq-guarded). - [P2] collection.updated is now delivered to item-grant-only SSE subscribers for collections they can see — it's itemless but leak-free (only the slug), so guests' ItemDetail schema/settings snapshots converge too. Filter test extended. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(web): switch-safety + conflict-merge fixes for collection writes (BUG-2265, Codex round 4) - Board column reorder now ABORTS on a 409 (with a "reorder again" toast) instead of replaying a stale option order onto the fresh field, which would silently drop a concurrent option add/remove/rename. Reordering is cosmetic; never worth clobbering a real schema edit. Also captures ws/slug/base before the await and fences the write against a route switch. - QuickActionsMenu captures workspace + collection identity BEFORE the first await, so a mid-save navigation can't make the 409 refetch/retry target the wrong collection (no guaranteed remount). - Settings-page SSE refresh captures the workspace and drops the result if the workspace changed while fetching, so a slow refresh for workspace A can't overwrite workspace B's freshly loaded collection list. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(store): sub-second collection updated_at token, no future drift (BUG-2265, confirming pass #6) The same-second monotonic advance manufactured whole-second FUTURE updated_at values; sustained >1 write/sec on one collection drifted arbitrarily ahead of wall-clock. collections.updated_at is TEXT on both dialects and never compared lexically (only via time.Equal + display), so switch the update write to sub-second nowNano(): same-second collisions become near-impossible, so the token advances naturally. Keep a strict-monotonicity guard but step by a single NANOSECOND on the (now near-impossible) coarse-clock/step-back collision, so any drift is bounded to nanoseconds. Dual-dialect; covered by make test-pg. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(server): sanitize + always-broadcast collection event (BUG-2265, confirming pass #2,#3) - #2 (P1): collection_updated is delivered to item-grant guests, but the event carried ActorName/Source, leaking the owner's identity + edit source. Strip them — publishCollectionEvent now emits workspace + slug (+ new_slug) only. - #3 (P2): always broadcast (including on rename), routed by the OLD slug and carrying the NEW slug via a new Event.NewSlug field, so remote tabs on the old slug can re-target instead of silently 404ing on their next action. Tests: assert no actor/source leak on a settings update; assert a rename routes by old slug + carries new_slug. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(web): decisive switch-safety + rename handling for collection writes (BUG-2265, confirming pass #1,#3,#4,#5) - #1 (P1): EditCollectionModal captures the target collection id/slug/name/ws + updated_at when the form is SEEDED, and handleSave/handleArchive now operate on that captured identity (not the live props). The seed effect re-seeds when the collection IDENTITY changes (not on a same-id broadcast refresh), so a reused route can't leave A's form saving/deleting to B. - #3 (P2): on a rename event the collection route navigates to the new slug (preserving the pane query) and ItemDetail refetches by new_slug; the SSE event type carries new_slug. - #4 (P2): the reorder-conflict path refetches the collection (reseeds the token) before prompting, so a missed SSE event doesn't make every retry 409 forever. - #5 (P2): QuickActionsMenu only invokes oncollectionupdated when the live workspace/slug still match the captured identity, so a reused route can't assign an old response to the newly-navigated page. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(server): nano token round-trip, rename visibility, publish-before-migration (BUG-2265, confirming pass 2) Address the round-2 confirming-pass findings (server-only): 1. (P2) The shared update_conflict envelope formatted actual_updated_at with second precision (time.RFC3339), truncating the now sub-second collection token so the client's retry token never matched — a permanent 409 loop. Format with time.RFC3339Nano. Item tokens are zero-nanosecond, so RFC3339Nano emits no fractional part — the item 409 wire shape is byte-identical and the item path still compares via time.Equal. Added a test that the returned token round-trips as a usable retry token. 2. (P2) Rename events are routed by the OLD slug, but a subscriber that revalidated after the rename only has the NEW slug in visibleSlugSet, so the visibility check dropped the event before the new_slug branch. Accept a rename when EITHER the old slug or the (authorized) NewSlug is visible; downstream item-grant gating uses whichever slug is visible. Filter test extended. 3. (P2) The collection_updated event was published only after field migrations succeeded, but UpdateCollection already committed (updated_at advanced). On a migration failure clients got a 500 and no refresh, leaving siblings with stale tokens that 409 blindly. Publish on the commit (before the migration), so siblings always resync regardless of migration outcome. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix: atomic collection update+migration; modal same-id rename retarget (BUG-2265, confirming pass 3) Address the round-3 confirming-pass findings; defer cross-tab rename RE-NAVIGATION to BUG-2272 (placeholder) per coordinator. 1. (P1) Migration atomicity. UpdateCollection committed the schema + concurrency token BEFORE MigrateItemFieldValues ran, so a migration failure returned 500 with the row already changed → the retry was guaranteed to 409 and item values were left inconsistent with the committed schema. Made the two ATOMIC: extracted applyFieldMigrationsTx and run it inside UpdateCollection's own transaction (after taking the workspace seq lock), so a migration failure rolls back the schema AND the token — nothing changes, the retry works. The handler now passes migrations through instead of running them separately, and publishes the event only after the fully-atomic commit. store/tx work → make test-pg run green. 2. (P2) EditCollectionModal same-id rename. The round-1 identity capture ignores same-id prop refreshes (to preserve edits), but a concurrent RENAME changes the slug (not the id), so handleSave/handleArchive PATCHed a dead slug → 404 before the token could 409. On a same-id prop change whose slug changed, the seed effect now retargets the endpoint slug + re-captures the token WITHOUT reseeding the form (in-progress edits preserved). Deferred (BUG-2272, TODO comments added, already broken on main — no regression): - ItemDetail full-page item URL/collSlug not retargeted after a remote rename. - Collection route chained-rename events during SSE replay landing on a dead intermediate slug. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(web): keep seeded token on same-id rename retarget (BUG-2265) On the EditCollectionModal same-id rename branch, retarget the endpoint (slug/name/ws) only — drop the token re-capture. Re-capturing let a later handleSave succeed against the renamed collection and apply the modal's stale pre-rename full form, silently REVERTING the concurrent rename (the exact stale-snapshot clobber BUG-2265 prevents). Keeping the seeded token means a concurrent rename correctly yields a 409 → the non-destructive "collection changed, reload" message. Slug-retarget without token-recapture gives both: no 404 (right URL) and no clobber (409 fires). Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix: lock-order deadlock + unified collection-snapshot fences (BUG-2265, confirming pass 4) 1. (P1) DEADLOCK regression. UpdateCollection's atomic migration path took the collection-row FOR UPDATE lock and THEN the workspace seq lock, but item creation takes them in the reverse order (workspace advisory lock first, then the collection-row FK lock on INSERT) — a concurrent item-create + schema-migration ABBA-deadlocks on Postgres. Fix: acquire the workspace seq lock BEFORE the collection-row FOR UPDATE (matching item-create's order). Every store path that locks both now takes them workspace-seq → collection-row (tryCreateItem, UpdateItem, MigrateItemFieldValues, UpdateCollection). Added a concurrency regression test (item-create racing schema-migration); make test-pg green. 2+3. (P2) Cross-generation fence gap. The SSE collection refresh and route/item loads used SEPARATE counters, so a stale in-flight load could complete after a fresh SSE refresh and revert the collection + its concurrency token. Unified to a SINGLE monotonic collection-snapshot generation in BOTH the collection route and ItemDetail — every collection-snapshot write (loadCollection/loadData, the SSE refresh, reorder, and the quick-action/edit-modal callbacks) bumps it on start and gates its assignment on "still latest". ItemDetail's load keeps a switch-escape so a stale refresh for the OLD collection can't block loading a NEW one. Settings page unified the same way over its collections-list writes. 4. (P2) Settings page fed a stale editingCollection to the edit modal after a remote rename (its prop never changed → the same-id-rename retarget never fired → 404). The unified refresh now re-points editingCollection at the refreshed object for the same id, so the modal's retarget fires. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix: emit item-changes signal on field migration; QuickActions retry-by-id (BUG-2265, confirming pass 5) 1. (P1) A collection update that runs a field migration mutates item `fields` JSON and advances item `seq`, but only collection_updated was published — open item views refreshed collection METADATA and returned without reconciling the migrated items, so clients kept stale field JSON under the new schema and a later full-fields item update could UNDO the migration (a clobber). UpdateCollection now returns the migrated-item count; when > 0 the handler ALSO emits the existing bulk item-mutation signal (items_bulk_updated, Op=migrate) so open views reconcile via /items-changes. Fires only when the migration touched >= 1 item — a pure settings/quick-actions update emits nothing extra. No store SQL/locking change (Go signature + count plumbing only); make test / make test-pg both green. 2. (P2) QuickActionsMenu's 409 retry GET-by-slug 404s if the competing update renamed the collection. Resolve the fresh collection by STABLE id (list + find by id) before re-appending + retrying, mirroring EditCollectionModal's identity approach; the result-propagation guard is now id-based too so a rename doesn't spuriously drop it. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix: uniform sweep of item-grant delivery, rename routing, and 409/404 retries (BUG-2265, confirming pass 6) One pattern-sweep instead of per-site patches. Audited every event this PR publishes and every client retry path, applying three patterns uniformly: PATTERN A (item-grant SSE reconcile) + B (old-slug rename routing): instead of a SEPARATE items_bulk_updated migration event (which carries op/count for items an item-grant subscriber can't see and isn't rename-routed), FOLD a SANITIZED `items_changed` bool onto collection_updated — already item-grant-delivered (round 3) and already old-slug-routed with new_slug (round 2). On it the client triggers a /items-changes deltaSync (server-filtered to the caller's grants) and ItemDetail refetches its open item, so item-grant EDITORS reconcile migrated field JSON — closing the clobber where a stale full-fields update would UNDO the migration. Leak surface: "a collection you can see items in changed [+ renamed + had item changes]" — a bool, no per-item data. Removed the round-5 items_bulk_updated publish. The pre-existing items_bulk_updated (archive/move) is untouched and correctly stays suppressed for item-grant users. PATTERN C (409 AND 404 in retries): a competing RENAME can 404 a slug-targeted write before it can 409, bypassing recovery. Added isNotFoundError / isConflictOrNotFound helpers; every write/retry path now treats BOTH: QuickActions save resolves-by-id and retries on either; board reorder reseeds-by-id and aborts on either; EditCollectionModal save shows the reload prompt and archive resolves-by-id and retries on either. Tests: server asserts collection_updated sets items_changed on migration (not on settings-only) and stays sanitized; the SSE-filter test asserts the migration variant reaches item-grant subscribers for a visible collection; web unit tests assert 404/409 classification and a real component-driven not_found -> resolve- by-id -> retry in QuickActionsMenu. make test / make test-pg / npm run test all green. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix: stable collection-ID identity for collection events + request-based items_changed (BUG-2265, confirming pass 7) 1. (P1) Collection events were identified only by MUTABLE, reusable slugs, and events replay — so a stale rename event's old slug, once re-owned by a DIFFERENT collection, could pass a slug-based match and misroute a client (navigate away / load the wrong schema) or leak the new slug. Fix at the ROOT: carry the STABLE CollectionID on collection_updated (Event.CollectionID) and match by ID everywhere: - Server visibility: sseEventVisibleFor matches collection_updated on a new visibleCollIDSet (built from the same VisibleCollectionIDs), not the slug — so an event for a collection the subscriber can't see by ID is dropped even if its (reused) slug is in visibleSlugSet. Filter test proves the slug-reuse drop. - Clients: ItemDetail and the collection route match `event.collection_id === <their collection>.id`, not slug. Slug(s)/new_slug stay only for the rename-navigation URL. Settings refreshes its whole list (already id-safe). 2. (P1) items_changed was keyed off the affected-ROW count, delivered to item-grant subscribers → a subscriber whose own items were unaffected could infer that HIDDEN items matched the migrated value. Now keyed off whether a field MIGRATION WAS REQUESTED (len(input.Migrations) > 0), independent of row count — leaks nothing about hidden item values. Reverted round-5's UpdateCollection count-return (no longer needed). Test: a migration matching ZERO items still sets items_changed. Deferred with markers: - NOTE(BUG-2273) at ItemDetail's reconcile-skip AND updateField: the web editor's full-fields field write lacks item-level OCC (never adopted IDEA-1480/v0.14), so the migration reconcile is best-effort. - TODO(BUG-2272) at the reorder 404 reseed: it refreshes `collection` but not the route `collSlug` (renavigation, deferred). Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix: archive OCC (no destructive wrong-target) + settings load fence (BUG-2265, confirming pass 8) 1. (P1) EditCollectionModal handleArchive resolved the target by stable id but the server DELETE re-resolves by the MUTABLE slug — a rename that re-owned that slug before the delete landed would archive the WRONG collection. Close the TOCTOU with an expected_updated_at OCC on the delete, mirroring the update OCC: DeleteCollection re-reads updated_at under a lock (FOR UPDATE on Postgres) and 409s on mismatch; the handler validates the token + maps the 409; the client sends it as a query param; handleArchive passes the seeded token (and the fresh token on the resolve-by-id retry). A reused slug or a concurrently-changed target now yields a clean 409 → the reload message, never a wrong-collection archive. Server test: stale token 409s (and the collection survives); current token 204s; malformed 400s; no token 204s. 2. (P2) settings load(): the generation was bumped AFTER awaiting setCurrent, so a slow load for workspace A could resume after B's load and clobber B's name/context/collections/members. Capture a dedicated loadGen at load() ENTRY (before any await) and fence EVERY continuation on it; the collections write additionally respects collectionsGen so it can't revert a fresher SSE refresh. Using a dedicated loadGen (not the SSE-shared collectionsGen) means an SSE collections-refresh mid-load doesn't drop the name/members writes. Deferred: TODO(BUG-2272) at the collection route's rename-navigation site — the global collectionStore (sidebar/pickers) isn't refreshed and the workspace layout ignores collection_updated, so the sidebar keeps the dead slug. Layout- level renavigation, deferred. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra * fix(web): dedicated item-snapshot fence + id-based rename comparisons in ItemDetail (BUG-2265, confirming pass 9) One comprehensive ItemDetail async-snapshot fence sweep so this file's item/ collection fencing is uniform and ID-based. 1. (P1) The migration item-refetch and loadData shared loadGeneration, so the refetch could apply migrated fields and then a stale loadData response overwrite them (a later full-fields edit then undoes the migration). Added a DEDICATED itemGen (separate from loadGeneration and collectionGen), bumped at the start of BOTH loadData's item load AND the migration refetch, and gated BOTH `item = ` writes on "still latest itemGen" — neither can stale-overwrite the other. Swept the other PASSIVE item snapshot-refreshes onto itemGen too (SSE item_updated/archived/restored, onSync deleted/incremental/full, the collab refresh) so they're ordered against each other and the migration/load. 2. (P2) A settings update that follows a rename before the rename fetch completes requested the OLD slug and bumped collectionGen, cancelling the valid rename fetch. Fetch slug is now `event.new_slug || event.collection || slug`. 3. (P2) The loadData collection fence-escapes compared the stale load's SLUG vs the freshly-renamed snapshot's slug (they differ on a rename → escape let the stale result overwrite). They now compare stable collection IDs; the SSE refresh's post-fetch identity check is id-based too. Audit (site -> generation -> id?): every PASSIVE snapshot-refresh (loadData item+collection, migration refetch, SSE x3, onSync x3, collab) bumps the correct dedicated gen (item->itemGen, collection->collectionGen) and compares identity by id. The DELIBERATE user/action writes (title/field/tag/assignee/role/content/ link/version/restore saves) keep loadGeneration + item-id switch-safety; their item-snapshot concurrency vs the migration refetch is the deferred item-OCC gap (BUG-2273, best-effort) — reordering them last-started-wins is orthogonal to that. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra