Skip to content

chore: upgrade effect to 4.1.0 and fix a SQLite lock-up on interrupt - #60651

Merged
kitlangton merged 4 commits into
devfrom
chore/effect-4-1
Oct 31, 2026
Merged

kitlangton merged 4 commits into
devfrom
chore/effect-4-1

Conversation

@kitlangton

@kitlangton kitlangton commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Effect is upgraded to 4.1.0, and stopping a database write at the wrong moment can no longer lock every later database call forever.
SQLite has one connection, guarded by a single permit. If a queued transaction was interrupted between taking the permit and registering its release, the permit leaked and every later query waited forever. Effect fixed this upstream (Effect-TS/effect@2cb498e); our two SQLite clients (Bun and workerd) copied the old code, so they get the same fix here.

❌ Before

  1. A transaction waits for the connection's only permit.
  2. It gets the permit, then registers "give it back when done" as a second step.
  3. ❌ An interrupt landing between those two steps skips the release. The permit is gone and the database hangs.

✅ After

  1. A transaction waits for the permit.
  2. Taking the permit and registering its release happen as one step that can't be split (Semaphore.takeScoped, new in Effect 4.1).
  3. An interrupt either lands before the take or after the release is registered. The permit always comes back.
- restore(take(1)) -> then addFinalizer(release(1))   // interruptible gap in between
+ restore(takeScoped(1))                              // take + finalizer, atomic

📊 Proof: interrupt a queued transaction at each of 31 timings, then check that the next query runs

Case Before After
Bun client, withTransaction ❌ 1/31 timings lock the DB ✅ 0/31
Bun client, reserve ❌ 1/31 ✅ 0/31
workerd client, reserve ❌ 1/31 ✅ 0/31

The new sqlite-interrupt.test.ts fails all 3 cases without the fix and passes with it. The holder fiber keeps the connection across two yields, so the interrupted fiber is really parked in the permit's waiter queue. A 4th case covers workerd's withTransaction, which passes before and after (it was already safe through withPermits).

What else the upgrade needed

  • FileSystem.readDirectoryEntries is now built into Effect and clashed with our own helper, so the helper is removed. Callers use Effect's entry types ("File", "Directory", "SymbolicLink", …), with the same no-follow behavior.
  • Command args and env can now be redacted values. Our cross-platform spawner now unwraps them, drops undefined env keys, hides redacted args in spawn errors, and supports the new duplex extra fds, all matching Effect's own Node spawner. It also ports two of that spawner's guards: an arg or env value containing a NUL byte fails with a typed BadArgument before spawning (and the redacted value never appears in the error), and a write error on stdin or a duplex fd surfaces as a stream failure instead of an uncaught error. Each has a test that failed first (a defect, and a 30s hang).
  • Effect 4.1 rejects Schema.Record values whose encoded side is optional. The app's saved-state records wrap entries in a fallback that has one, so the web app failed at load. Record entries now decode from unknown through the new Persistence.entry and keep their per-entry recovery (bad entries still fall back or get dropped, as before). The persistence README no longer suggests Persistence.optional for record values.
  • The recorded AI provider tests used the removed NodeSocket.layerWebSocketConstructorWS. They now use NodeWS.layerWebSocketConstructor.
  • Small type fixes: the Node WebSocket import moved, an Exit needs narrowing before reading .cause, and request methods are now plain strings in the HTTP recorder.
  • Free win, not measured on OpenCode: Effect.fn no longer leaks memory per call (Effect-TS/effect#9878).

SDK consumers: the generated client SDK has no diff. The paths and operation IDs in openapi.json don't change, and every request and response body refers to the same schemas as before. What does change:

  • Query parameters, 86 of 169 operations: optional ones are now plain types, not anyOf [T, null]. Three of them now inline a type they used to reference: Artifact.ProviderIDEncoded, Event.SeqEncoded and PersistentPty.ReadLinesEncoded.
  • New schemas, 362 → 488: JSON-encoded string fields such as SSE event payloads now declare their decoded shape with contentSchema. That adds component schemas like V2Event and Event.ID.
  • Event stream copies: Effect 4.1 reaches some schemas twice, once through the REST JSON codec and once through the event stream, and names the second copy <Name>_1. openapi-stabilize.ts now folds a copy back into its original when the two are deeply equal, so 14 copies are gone, Shell.Info_1 among them.
  • 12 copies remain: Form.*_1 (11) and Session.Inbox.MovePayload_1 are real differences. In the Form copies, NaN comes at a different spot in an enum. MovePayload_1 refers to Location.Ref where the original refers to Location.PublicRef. Only event schemas refer to them; no REST operation does.
  • Version literals: each event's version literal now gets a name from its content (Event.VersionEncoded_<hash>), not from encounter order (_37, _47).

👀 Read: packages/core/src/database/sqlite.ts and sqlite.workerd.ts, the transactionAcquirer lines. This is the actual bug fix, the same shape as upstream's. Workerd's own withTransaction uses withPermits, which was already safe. Also read Persistence.entry in packages/app/src/runtime/persistence/schema.ts, the one upgrade fix that touches saved user state. The rest is routine upgrade fallout.

🔍 Found while reviewing the Effect 4.1 changelog · no user reports · timings from the sweep test above

Checks run
  • bun turbo typecheck: all packages pass
  • packages/core: bun run script/test.ts on sqlite-interrupt, sqlite-workerd, database*, process, fs-util, filesystem, shell, session prompt/step/runner, skill, plugin, provider-azure, schedule, workspace, cross-spawn-spawner, environment, boot-work, tool-shell, session-shell: 691 pass, 2 fail. The 2 failures are tool-shell "conditional substitutions" cases, which need bash ≥ 4.4 (macOS ships 3.2, so wait on a process-substitution PID fails). They don't come from this change.
  • packages/app test:unit: 945 pass · packages/ai bun test: 1990 pass · packages/tui: 1595 pass · packages/server: 115 pass · packages/simulation: 48 pass · plugin, plugin-browser, client: all pass
  • packages/util bun test: 140 pass · packages/http-recorder bun test: 53 pass · packages/cli test/updater-install.test.ts: 30 pass
  • packages/protocol and packages/client check:generated: pass
  • After review fixes: packages/core cross-spawn-spawner, sqlite-interrupt, sqlite-workerd, shell, process: 47 pass · packages/schema: 90 pass · packages/protocol: 12 pass · packages/util: 140 pass · typecheck util, schema, protocol, core: pass

Added by the opencode bot (AI-generated).

Public API: regenerates openapi.json: adds contentSchema and new component schemas, folds identical _1 copies (12 Form.*_1/Session.Inbox.MovePayload_1 remain), inlines Event.SeqEncoded, PersistentPty.ReadLinesEncoded and Artifact.ProviderIDEncoded; HTTP paths, operations and generated SDK unchanged

@opencodejr opencodejr Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks sound. I ran the new sqlite-interrupt.test.ts against both branches. On the base, all 3 cases leak the connection permit at one interrupt timing each. With this PR, all 3 pass.

The nearby tests also pass with the PR:

  • core: fs-util, filesystem, shell, cross-spawn-spawner, process, skill, sqlite-workerd, plugin, workspace, schedule
  • util and http-recorder: full suites

tsc type checks pass for core, util, http-recorder and cli. I also checked that Effect 4.1's Node readDirectoryEntries doesn't follow symlinks, the same as the helper it replaces.

Public API: HTTP paths, operations and the generated client SDK are unchanged. Only the openapi.json schema document changes:

  • Changed: optional query params become plain types instead of anyOf [T, null].
  • Added: contentSchema on JSON-encoded string fields, plus about 140 component schemas.
  • Added: duplicate schemas with a _1 suffix (Form.*_1, Shell.Info_1, Session.ID_1).
  • Removed: Event.SeqEncoded and PersistentPty.ReadLinesEncoded, now inlined.

Third-party clients generated from the spec that refer to those two schemas by name would need updating. Everything else is backwards compatible.

No blocking concerns.

@opencodejr opencodejr Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new commits look sound. No earlier points were open.

How I checked them:

  • I decoded sample saved state through Persistence.entry. Valid entries are kept, invalid ones fall back or get dropped, and encoding gives the same data back.
  • Persistence.record behaves the same as on the base for malformed input.
  • packages/app test:unit passes (945 tests), and typecheck:tsc passes for packages/app and packages/ai.
  • The other Schema.Record uses in the app don't wrap entries in fallback, so they aren't affected by the Effect 4.1 restriction.

Public API: these commits add no changes beyond the earlier openapi.json schema regeneration.

No new concerns.

@opencodejr opencodejr Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new commits look right.

How I checked them:

  • The core spawner, SQLite, shell and process tests pass (47).
  • The sqlite-interrupt file, now 4 cases, passed 5 extra runs in a row.
  • Protocol tests and check:generated pass.
  • typecheck:tsc passes for util, protocol, schema and core.
  • The NUL-byte check covers piped commands too, because they spawn through the same path.

Public API: nothing new beyond the earlier openapi.json regeneration. There are fewer _1 schema copies now, and event version literals are named by content hash. Paths, operations and the generated SDK are unchanged.

One concern: the new duplex test depends on test order (inline comment).

}),
)

fx.live(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test only passes when earlier tests in this file have already spawned something. Run alone (bun test test/effect/cross-spawn-spawner.test.ts -t "duplex write failure"), it times out at 5s every time.

The cause isn't your change. In a fresh process, if the first spawn has an extra fd (fd3), it never finishes spawning, and the base branch does the same with an output fd3. A plain spawn earlier in the process makes both output and duplex work.

It also relies on a fixed Effect.sleep("100 millis"). Could you fix the first-spawn hang, or at least have the test do its own warm-up spawn, so it doesn't depend on test order or timing?

@opencodejr opencodejr Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The duplex test point is fixed. The test now passes when run on its own (5 runs out of 5) and no longer relies on a fixed sleep. The full cross-spawn-spawner file still passes (32 tests).

Public API: no change from these commits beyond the openapi.json regeneration covered earlier.

No new concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant