✨ Add configurable Worker shutdown policies - #235
Conversation
📝 WalkthroughWalkthroughThe Worker API now supports graceful, forced, and policy-driven shutdown. Teardown terminates unresponsive Workers and reports ChangesWorker shutdown
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HostScope
participant useWorker
participant ShutdownPolicy
participant Worker
HostScope->>useWorker: begin worker teardown
useWorker->>ShutdownPolicy: evaluate shutdown configuration
alt graceful completion
ShutdownPolicy-->>useWorker: return graceful
useWorker->>Worker: post close message
Worker-->>useWorker: complete outcome
else forced termination
ShutdownPolicy-->>useWorker: return forced
useWorker->>Worker: terminate worker
Worker-->>useWorker: report "halted"
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1📝 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: |
taras
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@worker/worker.test.ts`:
- Line 5: Replace the `@effectionx/vitest` import in worker.test.ts with the
corresponding `@effolutionx/bdd` utilities, using `@effolutionx/bdd/node` when an
explicit node:test entrypoint is required. Update the package test script and
development dependency to use the Node.js test runner consistently.
🪄 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: 50b25567-50ed-4ac9-a2be-6c8c92b97c78
📒 Files selected for processing (5)
worker/README.mdworker/package.jsonworker/test-assets/cpu-bound-worker.tsworker/worker.test.tsworker/worker.ts
| import { fileURLToPath } from "node:url"; | ||
| import { beforeEach, describe, it } from "@effectionx/vitest"; | ||
| import { when } from "@effectionx/converge"; | ||
| import { beforeEach, describe, it } from "@effectionx/vitest"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline worker/worker.test.ts --items all
printf '\nTest-runner references:\n'
rg -n -C 2 '`@effectionx/`(bdd|vitest)|node --test|vitest' \
worker/worker.test.ts \
--glob 'package.json' \
--glob 'pnpm-workspace.yaml' \
--glob '*.test.ts'Repository: thefrontside/effectionx
Length of output: 829
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Package files:\n'
git ls-files | rg '(^package.json$|/package.json$|pnpm-workspace.yaml|package-lock.json|pnpm-lock.yaml|yarn.lock)' || true
printf '\nWorker package.json:\n'
if [ -f worker/package.json ]; then
cat -n worker/package.json
fi
printf '\nTest file imports and Node test compatibility:\n'
sed -n '1,80p' worker/worker.test.ts | cat -n
printf '\nSearch `@effectionx/bdd` definitions/usages:\n'
rg -n "from ['\"]`@effectionx/bdd`['\"]|`@effectionx/bdd`|function useBdd|interface Bdd|describe\\(" . \
--glob '*.ts' --glob '*.tsx' --glob '*.json' --glob 'README.md' || trueRepository: thefrontside/effectionx
Length of output: 18248
Use the Node.js test runner and @effolutionx/bdd utilities.
worker/worker.test.ts imports beforeEach, describe, and it from @effectionx/vitest, so this package does not use the required Node.js test runner. Import these utilities from @effectionx/bdd and update the package test script/dev dependency accordingly. Use @effectionx/bdd/node if the entrypoint needs to explicitly target node:test.
🤖 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 `@worker/worker.test.ts` at line 5, Replace the `@effectionx/vitest` import in
worker.test.ts with the corresponding `@effolutionx/bdd` utilities, using
`@effolutionx/bdd/node` when an explicit node:test entrypoint is required. Update
the package test script and development dependency to use the Node.js test
runner consistently.
Source: Coding guidelines
cowboyd
left a comment
cowboyd
left a comment
There was a problem hiding this comment.
I'm a bit worried that this is too prescriptive of how shutdown should proceed. In other words, everything being equal, a worker should try to attempt a graceful shutdown, and it is only under certain conditions such as heavy CPU contention that it might not be an option.
In that case, you want to react to facts on the ground to decide when to take extraordinary measures that otherwise would be unwarranted.
I think it would be really cool if we made shutdown an api that was invoked if there were a middleware for it (but not otherwise):
Here's an example to set a policy for shutdown that adjusts how long it waits before termination based on CPU usage.
let worker = yield* useWorker("./worker.ts", {
type: "module",
*shutdown(args, next) {
let usage = yield* measureCPUUsage();
if ( usage < .5) { // less than 50%
yield* sleep(10_000) // wait 10 seconds
} else if (usage < .9) {
yield* sleep(2_000) // wait only 2 seconds
} else { // over 90% utilization
yield* sleep(100) //wait only the shortest of times.
}
return yield* next(...args); // terminate
}
});That way the shutdown middleware can use whatever context it needs to make its decisions.
I was also worried it was too prescriptive. This is the less prescriptive version. Let me look at the middleware path. |
taras
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@worker/worker.ts`:
- Around line 195-204: Update the around function to register shutdown
middleware in the caller’s scope rather than shutdownScope, or explicitly remove
the registration when that caller scope exits; preserve middleware behavior for
the active caller while preventing nested-scope middleware from affecting later
worker teardown. Add a regression test covering a termination policy installed
in a nested scope, scope exit, and subsequent graceful teardown.
🪄 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: 70f71ed9-c08d-425e-bd76-34060a2b528c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
worker/README.mdworker/package.jsonworker/tsconfig.jsonworker/worker.test.tsworker/worker.ts
taras
changed the title
2 participants
Motivation
Closes #229.
useWorker()shuts Workers down cooperatively by posting a close message andwaiting for Worker-side teardown. A Worker that cannot service that control
message makes graceful cancellation non-preemptible, but terminating every
Worker would discard useful Worker-owned cleanup. Callers need to choose based
on the workload and application-owned health state.
Approach
shutdown: "graceful" | "forced" | WorkerShutdownPolicy; omissionpreserves graceful shutdown.
"graceful"post the close message and await Worker-side teardown andresult delivery.
"forced"callWorker.terminate()immediately.context, and return either
"graceful"or"forced".a pending policy;
"graceful"keeps waiting;"forced"terminates an activeWorker. A policy error also terminates so teardown cannot remain stranded.
around()ormiddleware
next()machinery.visibility, pending-policy cancellation, and state-driven termination of a
CPU-bound Worker.
@effectionx/context-apidependency, document thecontract, and bump
@effectionx/workerfrom0.5.4to0.6.0.The dynamic example uses a host-owned control-channel health signal instead of
an arbitrary delay:
The package does not infer that CPU use or message latency means a particular
Worker is unhealthy. Applications define that semantic state. If the Worker
finishes while the operation is pending, Effection cancels the policy and
preserves graceful Worker-side cleanup.
Impact
Existing callers retain graceful cleanup without changes. Callers can opt into
immediate preemption or contextual escalation. Forced termination cannot run
Worker-side finalizers, so durable cleanup for a forcibly terminated Worker
must remain host-owned.
Validation
pnpm test— 385 passed, 6 skippedpnpm test worker/worker.test.ts— 25 passed, 4 skippedpnpm checkpnpm buildpnpm lintpnpm fmt:checkpnpm syncgit diff --check