Repository navigation
chore: upgrade effect to 4.1.0 and fix a SQLite lock-up on interrupt - #60651
Conversation
There was a problem hiding this comment.
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:
contentSchemaon JSON-encoded string fields, plus about 140 component schemas. - Added: duplicate schemas with a
_1suffix (Form.*_1,Shell.Info_1,Session.ID_1). - Removed:
Event.SeqEncodedandPersistentPty.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.
There was a problem hiding this comment.
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.recordbehaves the same as on the base for malformed input.packages/apptest:unitpasses (945 tests), andtypecheck:tscpasses forpackages/appandpackages/ai.- The other
Schema.Recorduses in the app don't wrap entries infallback, 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.
There was a problem hiding this comment.
The new commits look right.
How I checked them:
- The core spawner, SQLite, shell and process tests pass (47).
- The
sqlite-interruptfile, now 4 cases, passed 5 extra runs in a row. - Protocol tests and
check:generatedpass. typecheck:tscpasses 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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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
✅ After
Semaphore.takeScoped, new in Effect 4.1).📊 Proof: interrupt a queued transaction at each of 31 timings, then check that the next query runs
withTransactionreservereserveThe new
sqlite-interrupt.test.tsfails 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'swithTransaction, which passes before and after (it was already safe throughwithPermits).What else the upgrade needed
FileSystem.readDirectoryEntriesis 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.undefinedenv keys, hides redacted args in spawn errors, and supports the newduplexextra 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 typedBadArgumentbefore 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).Schema.Recordvalues 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 fromunknownthrough the newPersistence.entryand keep their per-entry recovery (bad entries still fall back or get dropped, as before). The persistence README no longer suggestsPersistence.optionalfor record values.NodeSocket.layerWebSocketConstructorWS. They now useNodeWS.layerWebSocketConstructor.Exitneeds narrowing before reading.cause, and request methods are now plain strings in the HTTP recorder.Effect.fnno 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.jsondon't change, and every request and response body refers to the same schemas as before. What does change:anyOf [T, null]. Three of them now inline a type they used to reference:Artifact.ProviderIDEncoded,Event.SeqEncodedandPersistentPty.ReadLinesEncoded.contentSchema. That adds component schemas likeV2EventandEvent.ID.<Name>_1.openapi-stabilize.tsnow folds a copy back into its original when the two are deeply equal, so 14 copies are gone,Shell.Info_1among them.Form.*_1(11) andSession.Inbox.MovePayload_1are real differences. In the Form copies,NaNcomes at a different spot in an enum.MovePayload_1refers toLocation.Refwhere the original refers toLocation.PublicRef. Only event schemas refer to them; no REST operation does.Event.VersionEncoded_<hash>), not from encounter order (_37,_47).👀 Read:
packages/core/src/database/sqlite.tsandsqlite.workerd.ts, thetransactionAcquirerlines. This is the actual bug fix, the same shape as upstream's. Workerd's ownwithTransactionuseswithPermits, which was already safe. Also readPersistence.entryinpackages/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 passpackages/core:bun run script/test.tson 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 aretool-shell"conditional substitutions" cases, which need bash ≥ 4.4 (macOS ships 3.2, sowaiton a process-substitution PID fails). They don't come from this change.packages/apptest:unit: 945 pass ·packages/aibun test: 1990 pass ·packages/tui: 1595 pass ·packages/server: 115 pass ·packages/simulation: 48 pass ·plugin,plugin-browser,client: all passpackages/utilbun test: 140 pass ·packages/http-recorderbun test: 53 pass ·packages/clitest/updater-install.test.ts: 30 passpackages/protocolandpackages/clientcheck:generated: passpackages/corecross-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: passAdded by the opencode bot (AI-generated).
Public API: regenerates openapi.json: adds contentSchema and new component schemas, folds identical
_1copies (12Form.*_1/Session.Inbox.MovePayload_1remain), inlinesEvent.SeqEncoded,PersistentPty.ReadLinesEncodedandArtifact.ProviderIDEncoded; HTTP paths, operations and generated SDK unchanged