Self-heal insert-or-skip phantoms by re-staging created ids after upload#345
Self-heal insert-or-skip phantoms by re-staging created ids after upload#345Stvad wants to merge 10 commits into
Conversation
Client CREATEs upload as insert-or-skip (`ON CONFLICT (id) DO NOTHING`), so a deterministic-id create that races initial sync — a mutating plugin effect minting a row while its already-synced `blocks_synced` row is momentarily un-materialized (offline-reconnect) — writes a real-time-stamped phantom into `blocks`. The server keeps its authoritative row and the skipped create produces no echo, so nothing re-triggers reconciliation: `blocks` sits permanently ahead of an unchanged `blocks_synced` (the integrity check's "unsynced local"). The reconcile gate would apply the server row if re-run — it just never is. After a create upload drains, re-enqueue each created id into `blocks_synced_changes` so the observer (which subscribes to that table) re-reconciles it against the current staging row. Ordering is load-bearing: the re-stage runs only AFTER `complete()` clears `ps_crud`, else the still-pending create makes the gate `skip-stale` the heal — the same trap that created the phantom. Self-selecting for creates: a skipped create has a staging row to heal to; a genuinely-new create has no staging row yet and no-ops (never deletes the fresh local row). Creates that also carry a separate patch op are excluded — the patch is a real server write whose echo already reconciles them. Best-effort: a re-stage failure never fails the already-drained upload. Covers the whole insert-or-skip class (matrix ingest, daily notes, tag/config bootstrap), not just the matrix case that surfaced it. Complements #336 (gate mutating effects until sync-ready), which narrows the race window but doesn't make an already-stranded phantom self-heal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Stvad
added
bug
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e6b174c04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Adversarial review (areview) surfaced a bulk-import perf regression and a narrow race, both fixed by the same gate: only re-stage a created id that ALREADY has a `blocks_synced` row. - Scale (major): re-staging every created id made the local bulk-create path (Roam import ⇒ ~10k creates/batch) enqueue a no-op `'upsert'` per id and re-drain them all — ~2x `blocks_synced_changes` churn on the path the queue is tuned for. A genuinely-new insert has no staging row yet (its echo hasn't round-tripped) and reconciles via its own later sync-down trigger, so the gate loses no heal while enqueuing zero rows for a fresh import. - Correctness (minor race): an `'upsert'` for an id with no staging row could dedup-mask a concurrent real `'delete'` in the same drain window (server revokes a deterministic id the client is re-creating), stranding a revoked row. The gate never emits such an `'upsert'`. Implemented as one `INSERT … SELECT … WHERE EXISTS`, which also collapses the per-id round-trips into a single write. Also documents two unenforced invariants the review flagged: the fused-create data-loss edge (no current creator triggers it) and the batch-vs-per-tx scope of the patch-exclusion (self-healing off-UI transient in the fallback). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b2972bd91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Per-tx fallback: defer re-stage until the whole loop has drained ps_crud, so a create is never re-staged while a later same-id tx is still pending. Previously the observer would skip-stale and CONSUME the synthetic upsert; if that later tx was then quarantined (no server echo), the phantom stayed stuck. Now the fallback collects created ops and re-stages once at the end — creates-only, so the cross-tx patch-exclusion doesn't drop a create whose sibling patch was rejected. Regression test added (create + later same-id patch, patch rejected → still heals; re-stage deferred past the completed sibling). - Tests: replace the hand-rolled setTimeout poller with vi.waitFor per AGENTS.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bcc858642
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
observer.test.ts, materialize.test.ts, and powersync.selfHeal.test.ts each re-implemented the same @powersync/node harness: DB lifecycle (one DB per file, reset per test), row/queue helpers (seedLocalBlock / stageRow / blocks / allBlocks / queueLen / crudCount / queuePendingUpload / deleteStagingRow), the observer starter, constMat/noKey, and stagingCiphertextParams. Centralize them in syncObserver/test/harness.ts (`setupObserverTestDb()` + shared constants); the three files drop ~195 lines of duplicated setup. NOT centralized (deliberate): each file's blockData/data builder stays local — their default timestamps/authors diverge and are asserted on (materialize expects updated_at 1700000000000), so a shared default would change assertions. Also local: file-specific helpers (racingDb, e2eeStaging, observer.test's waitFor). Behavior-preserving: `yarn run check` green (4762 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7c97702de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex follow-up: in the per-tx fallback, a create that already drained ps_crud before a later tx re-throws (transient upload error, or encryptOps throwing before its inner try) was never re-staged — the end-of-loop re-stage was skipped by the throw, and PowerSync's retry re-collects only the UNdrained txs, so that create's insert-or-skip phantom was stranded. Worse, the reconcile-rescan backstop is marker-gated to run at most once per (workspace, client) (repo.runReconcileRescan) and may already be spent, so there may be no later recovery. Move the re-stage into a `finally` around the fallback loop: it runs before the re-throw propagates, so drained creates heal in the same pass. A drained create whose id still has a pending same-id op is safely skip-stale'd by the observer and reconciled by that op's echo. Regression test: tx1 create drains, tx2 transient → re-throw, and tx1's create is still re-staged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e84746ca77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex follow-up: collectUploadBatch caps each upload pass, so re-staging after a batch drains only proves THIS slice is clear — a same-id patch/delete for a deterministic-id create can still sit in a later, not-yet-collected ps_crud slice. Re-staging then is skip-stale'd by the observer (the pending upload wins) and consumes the synthetic upsert for nothing. Add a NOT EXISTS ps_crud clause to the re-stage SELECT: only enqueue an id with no pending blocks upload anywhere in ps_crud. A same-id op that later SUCCEEDS heals the id via its own echo; only a genuinely-orphaned phantom is re-staged. This subsumes the per-invocation patch-exclusion for the cross-batch case. Documented residual: if the later same-id op is permanently REJECTED, its (already-drained) create is not retried, so it is not auto-healed here — that is the repair-sweep's job, tracked separately. Test added (pending ps_crud op defers the re-stage). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cedcbf8d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex (on 0cedcbf): collecting creates-only for the fallback finally stripped the patch context, so a create + a SEPARATE same-id patch that BOTH succeed lost the patch-exclusion — restageCreatedIds re-staged the create and transiently reverted the just-acked patch on disk until its echo arrived (the revert the batch path avoids for create+patch). Collect ALL ops from the SUCCEEDED txs (not creates-only). restageCreatedIds then sees the sibling patch and excludes the create — matching the batch path — so a succeeded sibling patch heals via its own echo with no revert. A REJECTED sibling patch is NOT collected (only successful txs push), so its create is still re-staged and the phantom heals. Test added (create+succeeded-patch → both ops passed to the re-stage so the exclusion fires). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 915a62f5cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The insert-or-skip phantom self-heal lived in the upload connector, which only ever sees an in-memory, capped slice of ps_crud per uploadData call and loses its memory across PowerSync retries. Each incremental fix (finally- re-stage, creates-vs-all-ops, EXISTS gate, pending gate) closed one interleaving and left another — the residual being a create whose same-id sibling op is permanently REJECTED: the create drained long ago, the reject leaves no echo, and nothing re-stages it, so the phantom stays stuck forever. Move the decision to durable state. A local pending_restage table records a phantom-candidate create id on drain (crash-safe, in a finally); a flush re- stages any id whose ps_crud is now clear — including after a rejected sibling drains the queue — enqueuing the synthetic blocks_synced_changes upsert the observer needs. Two clear-on-synced triggers drop an id the moment a REAL staging insert (echo) or delete (revoke) reconciles it, so a create whose sibling SUCCEEDS heals via its own echo, never a synthetic re-stage. The flush runs after the upload loop drains and once on repo startup (closing the crash window between record and flush). This avoids any full blocks scan, so it stays cheap at 400k–1M rows. Residual (documented): a phantom plus a separate cross-batch sibling patch that SUCCEEDS may see a brief, self-healing on-disk revert if the flush wins the race against the patch's echo — near-unreachable for deterministic-id creators, and strictly better than the prior stuck-forever. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db112ab26b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Adversarial review of db112ab (4 agents) + a Codex bot comment surfaced a handful of narrow issues in the self-heal outbox; none were blockers. Fixes: - Exclude a succeeded same-id DELETE (not just a PATCH) from re-stage candidates (Codex). A cross-tx create+delete in the per-tx fallback both reach drainedOps; recording the create let the flush enqueue a synthetic upsert from the stale staging row and RESURRECT the deleted block before the delete echo. Latent today (v1 has no hard-delete upload path) but a real gap — exclude any server-changing sibling (patch OR delete), symmetric with the patch rationale. - Flush the outbox in a `finally` at the end of runUploadLoop. A persistently- transient UNRELATED tx re-throws out of the loop and, before, skipped the flush — starving an already-queue-clear phantom until an app restart. The flush's per-id ps_crud gate re-stages only the clear ids and leaves the stuck one; flushPendingRestageQuietly swallows its own errors so it can't mask the re-throw. - Gate the clear-on-synced-insert trigger on "no same-id op pending". A bulk blocks_synced re-stream landing while a sibling edit is queued was dropping the outbox entry even though that echo is skip-staled (didn't reconcile); if the sibling then rejected, nothing re-staged the id → stuck-forever. An echo only reconciles when no upload is pending, so only clear then. Case-2 (succeeding sibling) still clears: its echo arrives after its ps_crud drains. Cleanup (folded in because the trigger gate adds a 3rd copy): extract the `ps_crud` no-pending predicate into `noPendingBlockCrudSql(idExpr)`, shared by both flush statements and the clear-insert trigger so the json-path can't drift. Docs/tests: correct the record-error backstop claim, the "in a finally" precision, and document the complete()->record crash window + the rejected- user-edit revert as data-loss-invariant tails; add pending_restage to the test reset; add mutation-tests for the delete-exclusion, the finally-flush, and the gated clear trigger. yarn run check green (4776 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14d0a88fcd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex caught that the round-2 F2 fix was incomplete: I gated the clear-on- synced-INSERT trigger, but a re-delivery of an already-staged blocks_synced row is an INSERT OR REPLACE (BLOCKS_SYNCED_RAW_TABLE.put) = DELETE-then-INSERT, and the clear-on-synced-DELETE trigger was still UNCONDITIONAL. Its DELETE fires BEFORE the gated INSERT, so a re-delivery while a sibling upload is pending (e.g. another client edits the block — the #244 multi-client case) dropped the outbox entry before the gate could protect it; a later rejection of that sibling then stranded the phantom → stuck-forever, exactly what the gate was meant to prevent. Gate the delete-clear the same way (noPendingBlockCrudSql('OLD.id')). A true revoke stays safe: the row is gone, so the flush's EXISTS-staging gate emits no synthetic upsert and its ps_crud gate removes the entry once no sibling is pending — a revoked row is never resurrected, just cleaned a beat later. Tests: the prior F2 test used a FRESH insert, so it never exercised the REPLACE path that fires the DELETE trigger — it gave false coverage. Replaced the clear-trigger tests with ones that stage-then-restage (a real REPLACE) and cover both triggers x pending/no-pending. Mutation-verified: reverting the delete gate to unconditional fails both pending tests. yarn run check green (4777 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd640d263a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| DELETE FROM pending_restage | ||
| WHERE ${noPendingBlockCrudSql('pending_restage.id')} |
There was a problem hiding this comment.
This is a different race from the earlier pending-gate fixes: the same-id upload can be queued after this NOT EXISTS passes but before the observer drains the synthetic blocks_synced_changes row. In that window (startup flush or post-upload flush, especially with the observer throttle), the materializer sees hasPendingUpload, skip-stales and consumes the synthetic upsert, while this delete has already removed the durable outbox row; if that new edit is later quarantined/rejected, there is no server echo and no pending_restage entry left to retry, so the original insert-or-skip phantom is stranded again. Please only clear the outbox after the observer actually applies the synthetic echo, or re-record/retain it when the synthetic upsert is skipped due to a newly pending same-id upload.
Useful? React with 👍 / 👎.
1 participant
Problem
Client CREATEs upload as insert-or-skip (
applyBlockCreates→upsert(..., {onConflict:'id', ignoreDuplicates:true})=ON CONFLICT (id) DO NOTHING), so a deterministic-id create that races sync leaves a phantom:blocks_syncedrow is momentarily un-materialized (offline → reconnect: the effect and sync run concurrently and the effect wins).blocksviatx.get, misses, andtx.creates a fresh row with a real-timeupdated_at.skip-staled (the create was still pending) and consumed.Result:
blocks.updated_atsits permanently ahead of an unchangedblocks_synced.updated_at,ps_crudempty — the integrity check's "unsynced local". The server keeps the correct row; the local client shows a stale phantom, and nothing ever re-triggers the reconcile gate (decideStagingRow, which wouldapplythe server row if it re-ran).This is the cross-client divergence reported in #244: edit a matrix message on client A, client B independently re-ingests the pre-edit message from Matrix before A's edit propagates, and B is stranded on the stale copy while the server (and A) hold the new one.
Config/bootstrap blocks dodge this because they
systemMint(updated_at = 0, the gate's stamp-0 exemption always yields); the matrix ingest stamps a winning real timestamp, so its phantom looks like genuine newer-local truth.Fix — durable
pending_restageoutboxSynthesize the missing echo, but drive the decision from durable state rather than the connector's transient, capped view of
ps_crud:blocks_syncedrow) is recorded into a localpending_restagetable — crash-safe, in afinally, unconditional on queue state.ps_crudis clear, a flush enqueues a syntheticblocks_synced_changesupsert (waking the observer toapplythe server row) and drops the outbox row. The flush runs after the upload loop drains and once on repo startup (closing the crash window between record and flush).Why durable and not in-connector:
collectUploadBatchcaps each pass and PowerSync retries spin up fresh, memorylessuploadDatainvocations, so no in-memory list can carry a create across the batch/retry boundaries. The outbox does — including the case a same-id sibling op is permanently rejected: its drain leaves the queue clear and lets the flush finally fire (the residual an in-connector re-stage structurally couldn't close). Noblocksscan, so it stays cheap at 400k–1M rows.Covers the whole insert-or-skip class (matrix ingest, daily notes, tag/config bootstrap), not just the matrix case that surfaced it.
Data-loss invariant (unenforced): the heal applies the server row over the local one, so a deterministic-id creator that fuses genuinely-unrecoverable content on a colliding id would be reverted with no echo to reconverge. Every creator today fuses reconstructible content (daily-note scaffolds, targets, prefs/ui-state) or content the server already shares (matrix messages), so this can't fire — noted for any future creator.
Residual (documented; near-unreachable, self-healing): a phantom plus a separate cross-batch sibling patch that succeeds can see a brief on-disk revert if the flush wins the race against that patch's echo — strictly better than the prior stuck-forever.
Tests
powersync.test.ts— orchestrator wiring/ordering: record fires aftercomplete()on batch and per-tx paths; a quarantined tx is not recorded; a create whose rejected sibling drains is recorded; same-id patch context preserved; a record failure doesn't fail the drained upload;runUploadLoopflushes exactly once, after the drain.powersync.selfHeal.test.ts(real@powersync/nodeDB + the real observer): outbox record shape (EXISTS-staging gate, patch-exclusion, records even with a pending sibling); flush queue-gate (defers a pending id, drops a revoked id); clear-on-synced triggers; end-to-end heal; rejected-sibling residual closes; genuine-insert no-op leaves the fresh local row intact.yarn run checkgreen (compile + lint + 4773 tests + sync/rpc/role checks).Relationships
systemMintthe matrix ingest so it never mints a winning timestamp in the first place.🤖 Generated with Claude Code