Repository navigation
Wait for the process group in NodeChildProcessSpawner release and kill - #8018
Merged
tim-smart merged 6 commits intoSep 4, 2026
Merged
Conversation
🦋 Changeset detectedLatest commit: c2230a3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
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 |
ghost
added
the 
tim-smart
changed the title
Contributor
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
tim-smart
deleted the
eff-1226/childprocess-process-group-cleanup-tests
branch
September 4, 2026 09:11
TFSebben
pushed a commit
to TFSebben/supabase_cli
that referenced
this pull request
Oct 9, 2026
Bumps `effect`, `@effect/platform-bun`, `@effect/platform-node`, `@effect/platform-node-shared`, `@effect/sql-pg`, and `@effect/vitest` from `4.0.0-rc.112` to the stable `4.0.2` release. `@effect/tsgo` is unchanged. The goal is zero change to CLI behaviour. Every edit is either required by a removed or renamed API, or restores a behaviour whose upstream default moved. ## Commit layout The first three commits are the bump plus two scripted rewrites and can be skipped by reviewers: - `effect/unstable/*` import paths moved to `effect/*` (removed upstream in rc.118, [effect#8354](Effect-TS/effect#8354)). - CLI and Config constructors renamed to PascalCase ([effect#7453](Effect-TS/effect#7453), [effect#8121](Effect-TS/effect#8121)). The remaining commits are hand migrations, one topic each. ## Hand migrations - **node-postgres pool bridge** — `@effect/sql-pg` 4.0 is a native client and dropped `PgClient.fromPool` ([effect#7426](Effect-TS/effect#7426)). The remote database session keeps its own node-postgres pool for the zero idle timeout, the per-connection role step-down hook, and raw `COPY` connections, so `db-connection.pool-client.ts` is a small `SqlClient.make` adapter over that pool with the same statement execution, cancellation, and error classification the old bridge had. - **Stack RPC strictness** — parse-option annotations no longer affect decoding ([effect#8131](Effect-TS/effect#8131)) and the RPC transport passes no parse options, so the initialization command and `runCommand` payload schemas now reject unknown keys through a `Record` filter ahead of the struct decode, with the same "Expected no excess property" wording. Encoding drops `undefined` entries first, so optional fields passed as explicit `undefined` (for example `pgProve.workingDir`) still cross the RPC JSON codec as they did with the plain struct. - **One statement per query** — the native `@effect/sql-pg` client sends everything through the extended protocol, which rejects multi-command strings. The realtime schema bootstrap runs as two statements, and the generated `ALTER ROLE` / `ALTER DATABASE` batches come back from Postgres as an array and run one at a time inside the same transaction. - **Socket addresses, file sizes, encoding, sockets** — server addresses are `NetAddress.SocketAddress` values, `File.seek` and `File.Info.size` use `bigint` and `ByteSize`, `Encoding` moved to `effect/encoding/Base64Url`, and the whole-stack WebSocket test helper uses the reader and writer pair. - **`Effect.partition`** returns `[passes, fails]`; the `stack destroy` handler destructures in that order. - **Management API contracts** — `packages/api` contracts are regenerated; the 4.0.2 schema codegen emits code-point length checks and Unicode-flagged patterns. - **Config package** — `SchemaAST.Union.mode` moved under `options`, `ToJsonSchemaOptions.additionalProperties` became `onExcessProperty`, and regex patterns need the Unicode flag to be exported into JSON Schema ([effect#8482](Effect-TS/effect#8482)). Peer dependency ranges are untouched and already satisfy 4.0.2. ## Preserved defaults - The stack's `PgClient.layer` calls pin `idleTimeout: "10 seconds"`; the default since 4.0.1 is 60 seconds ([effect#8679](Effect-TS/effect#8679)). - The published JSON Schema files (`@supabase/config` `schema.json` and `project-schema.json`, mirrored at `apps/docs/public/cli/*.schema.json`) are byte-identical to the develop build. `toCliConfigJsonSchema` passes `onExcessProperty: "error"` so structs keep `additionalProperties: false` ([effect#8147](Effect-TS/effect#8147)); the shared document assembler leaves pattern-keyed records open, as the generator has no per-schema setting for that; and bucket tables are modelled so they render as the published object-or-array union. ## Behaviour changes reviewed and accepted These upstream changes have no configuration knob. Each was checked against the repo's call sites and judged not user-visible: - Child-process `kill` and scoped release now wait for the process group, bounded to one second without `forceKillAfter` ([effect#8018](Effect-TS/effect#8018)). The stack host, bundled Postgres clients, and containers already set `forceKillAfter`; other spawn sites can only see up to one extra second of teardown when a descendant lingers. - `Effect.cached` treats interruption as abandonment ([effect#8719](Effect-TS/effect#8719)). Every cached effect in the repo is awaited by a single fiber or forked into an owning scope, so the shared-interruption semantics do not apply. - Empty bucket tables under `storage.analytics.buckets` and `storage.vector.buckets` are modelled as an object-or-array union instead of `Schema.Struct({})`. Runtime decoding now rejects a primitive bucket value (`foo = "x"`), which the old schema accepted and ignored; objects and arrays decode as before. - HTTP client spans are named after the bare method (`GET` instead of `http.client GET`), following the OpenTelemetry convention. Nothing in the repo keys on the old name. - Generated Management API string length checks count code points instead of UTF-16 code units, which only differs for characters outside the Basic Multilingual Plane. ## Patches - Rebased: the `@effect/platform-node-shared` stdin EPIPE listener, now against 4.0.2. 4.0.2 still removes the stdin sink's error listener before the final `end()` flush, so a child that exits before reading its input raises an uncaught EPIPE on Linux; [effect#7712](Effect-TS/effect#7712) and [effect#7714](Effect-TS/effect#7714) do not cover that path. - Rewritten: the `@effect/vitest` `runTest` patch. Upstream's finalizer ordering fix ([effect#8154](Effect-TS/effect#8154)) covers the abort hook, so the patch now only layers the two behaviours the stack's `effect-timeout` tests assert on top of it: rejecting with the pretty errors and reporting a finalizer failure after a timeout. ## Test-only changes - Stack integration and CLI `db reset` e2e fixtures that seeded data through `PgClient.layer` with semicolon-joined statements now issue one statement per call; fixtures that go through `psql` are unchanged. - The `db reset` e2e fixture reads `count(*)::int`, since the native client decodes `int8` as `bigint`. ## Follow-ups - Migrate the remote database session to the native `@effect/sql-pg` client and delete the pool adapter. That changes result decoding (int8, timestamps, bytea) across the SQL consumers and needs its own review. --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Julien Goux <hi@jgoux.dev>
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
1 participant
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
NodeChildProcessSpawnersignalled the process group on scoped release andkillbut only waited for the leader'sexit. When the leader dies before its descendants (for example ash -cwrapper that does notexec, as in #7821), release returned early, descendants kept running andforceKillAfternever escalated.Release and
killnow wait for the leader'sexitand then for the rest of the process group to terminate, based on process existence (kill(-pgid, 0)) rather than on stdio. This is a replacement for the approach in #7875, which waited on Node'scloseevent and could hang release when a pipe was held or left unread.Closes #7821
Closes EFF-1226
Semantics
exitCodeandisRunningstay tied to the leader'sexit.forceKillAfter: after the leader exits, the remaining group members get up to 1 second to exit. NoSIGKILLis ever sent.forceKillAfter: the group wait is unbounded until the timeout fires, then the group receivesSIGKILLfollowed by a final 1 second bounded wait (a non-reaping PID 1 can leave zombies in the group).close, so a descendant holding an inherited pipe or unread backpressured stdout cannot hang release.0still leaves its group alone. A non-zero exit still signals the group once without waiting, as before.exitwait only;taskkill /T /Falready terminates the tree.forceKillAfterdeadline use native time, so cleanup and escalation are not governed by the Effect clock (TestClocktests). The leader'sexitevent wakes the check immediately, so an ordinary release does not pay the 10ms poll interval.kill(-pgid, 0), which counts zombies. Under a non-reaping PID 1 every release pays the full wait; documented in the changeset and module docs.Tests
The first commit adds
test/fixtures/process-group.tsand five tests. Three failed onmainand pass with the fix:scope release waits for descendants that outlive the leaderscope release force kills descendants that ignore the kill signalkill force kills descendants that ignore the kill signalOne more fails when escalation runs on the Effect clock and passes with the native deadline:
forceKillAfter escalation does not depend on the Effect clock(it.effect, so underTestClock)Two guard the #7875 failure modes and pass before and after:
scope release returns when a descendant holds the inherited pipe without forceKillAfterscope release returns when stdout is unread and backpressuredThe fixture is plain Node: a leader with the default
SIGTERMdisposition and a same-group descendant with inherited stdio, so it does not depend on/bin/shexec behaviour. Assertions use marker and heartbeat files, not the stdout stream.Test plan
pnpm lintpnpm checkinpackages/platform/node-sharedandpackages/effectpnpm jsdocs --check(no diagnostics for the touched file)pnpm test --run packages/platform/node-shared/test/NodeChildProcessSpawner.test.ts: 75 passedpnpm test --run packages/effect/test/unstable/process/ChildProcess.test.ts: 30 passedpnpm test --run packages/tools/api-diff/test/Worktrees.test.ts: 1 passed