✨ Add @effectionx/forceable and implement it on Worker and Process - #242
✨ Add @effectionx/forceable and implement it on Worker and Process#242taras wants to merge 12 commits into
Conversation
Graceful teardown assumes the other side is listening. A Worker spinning in a tight loop never reads its control channel, so waiting on it holds the enclosing scope open forever, and no amount of patience fixes it. withForce() puts a deadline on that wait without changing how a resource tears itself down. The resource still runs its own graceful teardown; the policy runs alongside it and may cut it short. Whichever finishes first wins, and a resource that closes in time cancels the policy where it stands. Resources opt in by implementing a single symbol, so one policy shape works across Workers, processes, and anything else holding a handle the runtime will not reclaim on its own. This replaces the shutdown option, its "graceful" | "forced" union, and the policy plumbing inside useWorker, which shrinks by 54 lines. Forcing is quiet: the policy decided to force, so the policy is the natural place to log or count it. Raising from inside a policy is not a reliable alternative — whether the error escapes races against how many turns the resource's teardown needs after being forced.
|
Warning Review limit reached
Next review available in: 27 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds the ChangesForceable resource teardown
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant HostTask
participant withForce
participant Resource
participant ForcePolicy
HostTask->>withForce: acquire Resource
HostTask->>withForce: start cleanup
withForce->>Resource: request graceful teardown
withForce->>ForcePolicy: run policy concurrently
ForcePolicy->>Resource: invoke force(reason)
Resource-->>withForce: settle forced teardown
withForce-->>HostTask: complete cleanup
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@forceable/forceable.test.ts`:
- Around line 43-45: Replace the non-zero sleep-based synchronization in the
affected tests around the ensure/spawn lifecycle with deterministic signals
using withResolvers() or a channel. Signal graceful-close start, force
invocation, and teardown completion explicitly, then await those signals at each
synchronization point so the quiet-halt test confirms forcing occurred. Remove
the corresponding test sleeps while preserving the existing lifecycle
assertions.
- Line 1: Migrate forceable/forceable.test.ts (anchor, lines 1-1) from
`@effectionx/vitest` to `@effectionx/bdd`, replace sleep(50) with deterministic
synchronization, and exclude the suite from Vitest. Update
forceable/package.json (sibling, lines 30-32), the TypeScript reference,
lockfile, and test commands to use the Node.js test runner and the new BDD
dependency.
In `@forceable/forceable.ts`:
- Around line 21-43: Define and apply one consistent public contract for force
behavior: decide whether an independently awaited resource rejects with
ForcedTerminationError after force or remains quiet, then update the Forceable
and ForcedTerminationError documentation in forceable/forceable.ts (lines
21-43), the rejectOutcome example in forceable/README.md (lines 62-70), and the
quiet-forcing statement in forceable/README.md (lines 84-86) to describe that
same behavior.
In `@forceable/package.json`:
- Around line 2-4: Update the package manifest description associated with
`@effectionx/forceable` to exactly “Put a deadline on a resource's graceful
teardown,” matching the README text before its `---` separator; leave the
package name and version unchanged.
- Around line 2-4: Add a files field to the package metadata for
`@effectionx/forceable`, explicitly including dist, mod.ts, and the package source
files so the published artifact is not determined by npm defaults.
In `@forceable/README.md`:
- Around line 88-93: Update the generator example so `reason` is initialized
before the `logger.warn` call and the subsequent `force(reason)` invocation,
ensuring both use the same defined value and the policy does not throw due to an
undefined variable.
In `@worker/worker-force.test.ts`:
- Around line 9-29: Replace the timing-based sleep in the withForce test with
deterministic shared-state synchronization: update spinning() to return the
allocated state alongside the worker task, then use when() from
`@effectionx/converge` to wait until Atomics.load(state, 0) indicates the
CPU-bound worker has entered its spin loop before continuing the test.
In `@worker/worker.ts`:
- Line 125: Update the UseWorkerOptions documentation near the worker
constructor to remove the reference to shutdown options and state that forcing
is opt-in through withForce().
- Around line 157-164: Update workerMain() graceful-shutdown handling to release
all remaining Node.js handles before returning after parentPort closes. Use
withForce for workers that cannot close their handles, and preserve terminate()
only for forced/error paths; do not unconditionally call worker.terminate()
during graceful cleanup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0f73d076-88b5-440d-ad24-cca592b9b718
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
forceable/README.mdforceable/forceable.test.tsforceable/forceable.tsforceable/mod.tsforceable/package.jsonforceable/tsconfig.jsonpnpm-workspace.yamltsconfig.jsonworker/README.mdworker/package.jsonworker/test-assets/cpu-bound-worker.tsworker/tsconfig.jsonworker/worker-force.test.tsworker/worker.test.tsworker/worker.ts
| "name": "@effectionx/forceable", | ||
| "description": "Put a deadline on a resource's graceful teardown and tear it down forcibly when it expires", | ||
| "version": "0.1.0", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Match the manifest description to the README description.
The manifest description differs from the package README text before the --- separator. Set it to Put a deadline on a resource's graceful teardown.
As per coding guidelines, “Package README text before the --- separator is used as the package description.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forceable/package.json` around lines 2 - 4, Update the package manifest
description associated with `@effectionx/forceable` to exactly “Put a deadline on
a resource's graceful teardown,” matching the README text before its `---`
separator; leave the package name and version unchanged.
Source: Coding guidelines
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Declare the published package files.
Add a files field that includes dist, mod.ts, and the package source files. Do not rely on npm defaults for the published artifact.
Proposed fix
"version": "0.1.0",
+ "files": ["dist", "mod.ts", "forceable.ts"],
"keywords": ["concurrency"],As per coding guidelines, “The files field must include dist, mod.ts, and source files.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "name": "@effectionx/forceable", | |
| "description": "Put a deadline on a resource's graceful teardown and tear it down forcibly when it expires", | |
| "version": "0.1.0", | |
| "name": "`@effectionx/forceable`", | |
| "description": "Put a deadline on a resource's graceful teardown and tear it down forcibly when it expires", | |
| "version": "0.1.0", | |
| "files": ["dist", "mod.ts", "forceable.ts"], |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forceable/package.json` around lines 2 - 4, Add a files field to the package
metadata for `@effectionx/forceable`, explicitly including dist, mod.ts, and the
package source files so the published artifact is not determined by npm
defaults.
Source: Coding guidelines
There was a problem hiding this comment.
Two findings here, handled differently.
files — applied in 8c1921a, thank you. The field was genuinely missing and the package would have published its tests and source. It is now files: ["dist"] rather than the suggested dist + mod.ts + source: #216 landed a few hours before this PR and rewrote that guideline. AGENTS.md now reads "Include only published artifacts (dist)", with bdd and inline as the sole exceptions.
Description — not applied. No package in the repo follows the "README text before ---" convention today:
| package | package.json | README |
|---|---|---|
| timebox | Constrain operations to complete within a time limit | Constrain any operation to complete within a certain time. |
| converge | Poll and wait for conditions to be met with automatic retry and timeout | Recognize a desired state and synchronize on when that state has been achieved. |
| task-buffer | Limit concurrent task execution with automatic queuing | Manages concurrent task execution by enforcing a maximum limit… |
The current description is 88 characters, verb-first, no Markdown, no trailing period — compliant with .policies/package-json-metadata.md, which is the strict policy .coderabbit.yaml points at for **/package.json.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- forceable/package.json ---'
cat forceable/package.json
printf '%s\n' '--- forceable/README.md (opening section) ---'
sed -n '1,35p' forceable/README.md
printf '%s\n' '--- relevant repository guidance ---'
fd -a -t f 'AGENTS.md|package-json-metadata\.md|index\.md|coderabbit\.ya?ml' . \
| while IFS= read -r file; do
printf '\n--- %s ---\n' "$file"
rg -n -i -C2 'published artifacts|files|readme.*description|description' "$file" || true
done
printf '%s\n' '--- commit 8c1921a manifest change ---'
git show --format=fuller --stat --oneline 8c1921a -- forceable/package.json 2>/dev/null || true
git show --format= -- forceable/package.json 8c1921a 2>/dev/null || trueLength of output: 5290
@taras, files: ["dist"] is correct. The current AGENTS.md rule requires only published artifacts in files, and the forceable package has no listed exception. The files finding is addressed.
The manifest description meets the strict metadata-format policy. However, the current AGENTS.md also states that the README text before --- is the package description. forceable/README.md and forceable/package.json still differ. Existing package differences do not define an exception to that rule.
🧠 Learnings used
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-23T17:32:37.408Z
Learning: Applies to **/package.json : package.json must include a `description` field: single sentence, under 120 chars, no Markdown, no trailing period (see Package.json Metadata policy)
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-07T19:16:27.135Z
Learning: Applies to **/package.json : Package descriptions are required, single-sentence, under 120 characters, Markdown-free, and must not end with a period.
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-25T17:56:28.931Z
Learning: Applies to **/package.json : For `package.json` files, enforce `.policies/package-json-metadata.md`: ensure non-empty `description` field with required format.
Learnt from: CR
Repo: thefrontside/effectionx PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-07T19:16:27.135Z
Learning: Applies to **/README.md : Package README text before the `---` separator is used as the package description.
Learnt from: taras
Repo: thefrontside/effectionx PR: 216
File: bdd/package.json:29-29
Timestamp: 2026-04-26T13:41:09.571Z
Learning: In this repo, the publish workflow runs `.internal/strip-dev-exports.ts` before `pnpm publish` to strip the `development` export condition from every workspace package’s `package.json`, ensuring published tarballs don’t reference source `.ts` entry points via `development` conditions. Therefore, when reviewing any workspace `package.json`, you generally should not add TypeScript source entry points (e.g., `mod.ts`, `mod.node.ts`, or other `.ts` files) to the `files` field solely to support `development` exports; the publish step should ensure those `development` exports are removed from the published artifacts.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
You are interacting with an AI system.
On removing the inline close-message loop
while (!outcomeSettled) {
const event = yield* once(worker, "message");
// ...resolveOutcome / rejectOutcome
}Worth spelling out why that is safe, since teardown now depends on the spawned It was redundant, not a second line of defense. The inline version was the weaker one. It settles the outcome but never The ordering it relies on is not new in effection 4.1. Since 4.1 fixed a
Unrelated, but found while checking: |
Graceful teardown used to read the close message inline, duplicating the spawned message loop that was already handling it. Removing the duplicate leaves teardown depending on that loop still being alive, which is true but invisible. Say so, and say which effection versions it was checked against, so the next person weighing a teardown change knows what is holding it up.
0d0edc6 to
bfeef62
Compare
A process that traps SIGTERM, or a descendant holding the inherited stdio open, never lets teardown finish: exec() signals the group and then waits on stdout and stderr closing, and that wait has no bound. Implement the force symbol so withForce() can put one there. POSIX sends SIGKILL to the process group, which cannot be trapped and reaches descendants holding the stdio open. Windows runs taskkill /T /F from a task owned by the resource, so cancelling whatever policy asked for the kill cannot cancel the kill itself. daemon() returns the same Process, so it gains this too.
Explain what cooperative shutdown waits for, the two ordinary cases that never satisfy that wait, and how withForce() bounds it. Record the force symbol alongside join() and expect() in the Process interface. Minor bump: exec() and daemon() gain a capability without changing any existing behaviour.
taras
changed the title
The withForce tests waited on the clock. Worse, the worker one allocated the shared spin flag and never read it, so on a loaded machine the halt could run against a worker that was still cooperative and the test would pass without forcing anything. Wait on the flag with when(), and assert that forcing actually happened rather than only that the halt was quiet. Give the stubborn fixture a trigger instead of a timer so every case is caused rather than timed. The suite drops from half a second to 8ms. The force contract also claimed ForcedTerminationError is reported to whoever awaits the resource. Nothing usually is: forcing happens during teardown, so on halt the awaiter is cancelled first and on normal exit the body has already returned. Say that, and point at the policy as the thing that knows. Also declare files for the new package, define the reason the README example logs, and drop the reference to a shutdown option this branch removes.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@forceable/forceable.test.ts`:
- Around line 41-43: Update the useStubborn test fixture and quiet-halt test to
settle with a ForcedTerminationError for the forced path instead of resolving
settled identically for graceful and forced paths. Assert the resource outcome
separately from the enclosing task.halt() result, covering both the forced
failure and successful halt behavior while preserving the existing graceful-path
assertions.
- Around line 79-90: Update the test around the scoped withForce policy so it
signals when execution reaches yield* suspend() before triggering graceful
teardown. Await or yield until that policy-ready signal is set, then perform the
cancellation and lifecycle assertions, ensuring policyResumed remains a
meaningful cancellation invariant rather than a timing-dependent result.
In `@process/src/exec/posix.ts`:
- Around line 143-153: Update the force handler around the visible [force]
method to track whether processResult has settled at every resolution site,
including normal completion and forced termination. Return immediately when
settled, before checking the PID or calling process.kill(); retain the
processResult.resolve(Err(new ForcedTerminationError(reason))) path only for
unsettled resources, without relying solely on the direct child exit state.
In `@process/test/force.test.ts`:
- Line 22: Replace every positive-duration sleep in the force tests with
deterministic synchronization: consume the fixture’s readiness signal before
sending signals, use explicit test-owned triggers to exercise the force policy,
and await an observable process-completion event before assertions. Update the
affected scenarios around the fixture setup and force/termination checks while
preserving their intended outcomes; do not use sleep-based timing, including the
sleep calls near lines 30–31, 40, 59, and 67.
- Line 3: Update process/test/force.test.ts to import test helpers from
`@effectionx/bdd`, add that dependency, and include the file in the Node.js test
command. Replace the wait-only sleep calls in the force tests around the
affected lifecycle assertions with deterministic process-state or lifecycle
synchronization, while preserving the delayed policy-action sleeps.
In `@worker/worker-force.test.ts`:
- Line 3: Update the worker test setup by adding `@effectionx/bdd` to
worker/package.json, replacing the describe/it import in worker-force.test.ts
with the package’s BDD import, and updating the Node.js test command to include
the worker tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: be1f774c-93c6-4495-ab05-c6b97505efba
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
forceable/README.mdforceable/forceable.test.tsforceable/forceable.tsforceable/package.jsonprocess/README.mdprocess/package.jsonprocess/src/exec/posix.tsprocess/src/exec/types.tsprocess/src/exec/win32.tsprocess/test/fixtures/shutdown-resistant.tsprocess/test/force.test.tsprocess/tsconfig.jsonworker/worker-force.test.tsworker/worker.ts
exec()'s force implementation signalled before checking whether the process had settled, so a late call could send SIGKILL to a process group id the operating system was free to have reused. Track settlement at every resolution site and return early, which is what the Forceable contract already promised. Also tighten the tests that were passing for the wrong reasons. The cancellation test closed the resource gracefully without waiting for the policy to suspend, so it could pass without cancelling anything. The process tests slept to decide when the child was ready, which let SIGTERM arrive before the fixture installed its handlers, and then the child died of graceful shutdown rather than of forcing. Wait on the fixture's own readiness line and on the process actually being reaped.
1 participant
Motivation
Closes #229 and #228. Supersedes #235 and #230.
Cooperative shutdown assumes the other side is listening, and sometimes it
isn't.
A Worker spinning in a tight loop never reads its control channel, so
useWorker()waits forever for a close it will never get (#229). A process thattraps
SIGTERM, or a descendant holding inherited stdout open, never satisfiesexec()'s wait for the process and its stdio to close (#228). In both cases theowning scope stays open indefinitely, and no amount of patience fixes it.
#235 and #230 each solved this in place, with a
shutdownoption carrying a"graceful" | "forced"union. Review feedback on #235 was that this puts thedecision in the wrong place: a resource should try to shut down gracefully, and
only conditions the application can observe make extraordinary measures
reasonable. The same objection applies to #230, and solving it twice would mean
two policy vocabularies for one idea.
Approach
Add
@effectionx/forceable, and implement it on both resources.withForce(op, policy)puts a deadline on a resource's graceful teardownwithout changing how the resource tears itself down.
alongside it and may cut it short. Whichever finishes first wins, and a
resource that closes in time cancels the policy where it stands.
clock. Neither package infers what makes a resource unhealthy.
Workers, processes, and anything else holding a handle the runtime will not
reclaim on its own.
Each resource implements the symbol with the kill it already had:
[force]Worker.terminate()SIGTERMto the group, await stdio closeSIGKILLto the grouptaskkill /T /FSIGKILLandtaskkill /T /Fboth address the tree, which is what makes themeffective against the inherited-stdio case in #228.
Windows needed one accommodation:
taskkillis an operation but[force]issynchronous, so the kill runs in a task owned by the resource. Cancelling
whatever policy asked for the kill therefore cannot cancel the kill itself —
independently reproducing a fix #230 had to arrange deliberately.
Impact
useWorker(),exec(), anddaemon()are unchanged for existing callers. Theshutdownoptions, the unions, and the policy plumbing from #235 and #230 areall gone.
worker.tsshrinks by 54 lines and its teardown returns to what itwas before #235; the process changes are +14 in
posix.tsand +12 inwin32.tsagainst #230's +66 and +87.
Forcing skips the resource's cleanup — that is the point, and it is not free.
Whatever the graceful teardown was responsible for (flushing a buffer, removing
a lock file, acknowledging in-flight work) has not happened. Durable cleanup for
a resource that might be forced has to be host-owned.
Forcing is quiet:
force(reason)returns, teardown finishes, and the enclosinghalt is undisturbed. The policy decided to force, so the policy is the natural
place to log or count it.
Open question: should forcing be loud?
Raising from inside a policy is not a reliable way to make it loud. Whether
the error escapes races against how many turns the resource's teardown needs
after being forced:
So this is not something an application can arrange for itself. If forcing
should raise, it belongs in
withForce()where it can be deterministic, and itcarries a real cost: effection 4.1 replaces an in-flight error rather than
aggregating, so a raising teardown buries whatever was actually bringing the
scope down. Left out pending a decision.
One thing #230 had that this does not: an eager
{ exit }operation givingprocess policies the direct command's
ExitStatus. A policy can close over theProcessand calljoin()itself, but I have not verified that covers thedescendant-stdio case, so it is worth a look.
Validation
pnpm test— 396 passed, 6 skippedpnpm test:matrix— all ten rows pass (effection 3.0.0 and 4.1.0, effect 3.0.0and 3.22.1, vitest 3.0.0 and 4.1.10)
pnpm build,pnpm check,pnpm lint,pnpm fmt:check,pnpm sync— cleankill(pid, 0); the same fixture withoutwithForcehangs teardown forever,confirmed by timeout
Summary by CodeRabbit
@effectionx/forceablepackage for reusable shutdown policies.