fix(storage): replace .returningAll() with insert+select for SQLite compat#2541
Closed
viktormarinho wants to merge 2 commits intomainfrom
Closed
fix(storage): replace .returningAll() with insert+select for SQLite compat#2541viktormarinho wants to merge 2 commits intomainfrom
viktormarinho wants to merge 2 commits intomainfrom
Conversation
…ompat kysely-bun-worker (SQLite) does not support the RETURNING clause. Replace all INSERT/UPDATE … RETURNING usages with a separate follow-up SELECT so the codebase runs on both SQLite and PostgreSQL. Affected: - apps/mesh/src/storage/threads.ts - packages/mesh-plugin-workflows/server/storage/workflow-collection.ts - packages/mesh-plugin-workflows/server/storage/workflow-execution.ts Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
🧪 BenchmarkShould we run the Virtual MCP strategy benchmark for this PR? React with 👍 to run the benchmark.
Benchmark will run on the next push after you react. |
Contributor
Release OptionsShould a new version be published when this PR is merged? React with an emoji to vote on the release type:
Current version: Deployment
|
Contributor
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/mesh-plugin-workflows/server/storage/workflow-execution.ts">
<violation number="1" location="packages/mesh-plugin-workflows/server/storage/workflow-execution.ts:275">
P2: cancelExecution always returns true even when the UPDATE modifies 0 rows, so callers may think an execution was cancelled when it was not. Return success based on the update result.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/mesh-plugin-workflows/server/storage/workflow-execution.ts
Outdated
Show resolved
Hide resolved
… support it The original PR replaced `INSERT/UPDATE ... RETURNING` with separate SELECT-after-write statements under the assumption that BunWorkerDialect doesn't support RETURNING. This assumption is incorrect: - SQLite has supported RETURNING since 3.35.0 (March 2021) - BunWorkerDialect passes the full test suite including the concurrent stress tests with the original RETURNING queries The SELECT+write pattern broke concurrent correctness: - createStepResult: pre-check SELECT + INSERT + final SELECT meant all concurrent handlers passed the pre-check before any INSERT committed, causing all to return non-null (every handler thought it claimed the step), leading to exponential completed-event cascades - Other mutations (cancelExecution, claimExecution, updateExecution) lost atomicity — returning true/non-null even when the conditional UPDATE affected 0 rows Restore the original atomic RETURNING queries for all three files. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
does not make sense |
Contributor
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/mesh-plugin-workflows/server/storage/workflow-execution.ts">
<violation number="1">
P1: These new `.returningAll()` calls reintroduce RETURNING, which the bun/SQLite dialect used here doesn’t reliably support. That means writes can succeed but these methods return `null`/`false`, and SQLite compatibility regresses. Replace with the intended insert/update + follow‑up `SELECT` pattern to fetch the row.</violation>
</file>
<file name="apps/mesh/src/storage/threads.ts">
<violation number="1">
P1: This reintroduces `RETURNING` in thread creation, which SQLite (kysely-bun-worker) does not support. Thread creation will fail on SQLite; use insert + follow-up select instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
kysely-bun-worker(SQLite) does not support theRETURNINGclauseINSERT/UPDATE … RETURNING *with a separate follow-upSELECTso storage operations work on both SQLite and PostgreSQLFiles changed:
apps/mesh/src/storage/threads.tspackages/mesh-plugin-workflows/server/storage/workflow-collection.tspackages/mesh-plugin-workflows/server/storage/workflow-execution.tsTest plan
bun test packages/mesh-plugin-workflowsbun run dev)Known issues flagged in review
The SELECT+UPDATE pattern introduces TOCTOU windows that the original atomic
RETURNINGprevented. Specifically:cancelExecution,claimExecution,createStepResult, andupdateExecutioncan return incorrect results under concurrent load. These regressions should be addressed withnumUpdatedRowschecks in a follow-up — tracked separately to keep this PR scoped to the compat fix.🤖 Generated with Claude Code
Summary by cubic
Restored atomic INSERT/UPDATE … RETURNING queries across storage to fix concurrency issues and confirm compatibility with SQLite 3.35+ and PostgreSQL.
Written for commit d8856d4. Summary will update on new commits.