[windows] Safe worker shutdown signal handling#22
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (8)
📝 WalkthroughWalkthroughPull request guards SIGTERM/SIGQUIT registration on Windows across core worker code, examples, and tests to avoid subscribing to unsupported signals. Also includes many formatting/refactor-only edits across dashboard UI, library code, and tests, a version bump to 0.2.1-wip, and a Changes
*Multiple example and test file paths are grouped for brevity — see diff for exact file list. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
kingwill101
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 the current code and only fix it if needed.
Inline comments:
In `@packages/stem/pubspec.yaml`:
- Line 3: The root package's version is set to 0.2.1-wip but several internal
packages still pin stem: ^0.2.0 which excludes pre-releases; update the stem
dependency entries in the eight listed pubspec.yaml files (packages/stem_sqlite,
stem_redis, stem_postgres, stem_cli, stem_builder, stem_memory,
stem_adapter_tests, dashboard) replacing stem: ^0.2.0 with a constraint that
includes pre-releases (for example stem: ">=0.2.0-dev, <0.3.0" or an explicit
allow for 0.2.1-wip) so workspace dependency resolution picks the wip version.
Ensure you update the stem dependency field in each package's pubspec.yaml and
run pub get to verify resolution.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30f800af-ea68-41ed-b0ba-6c450fa6fb65
📒 Files selected for processing (74)
.gitignorepackages/dashboard/lib/src/server.dartpackages/dashboard/lib/src/services/stem_service.dartpackages/dashboard/lib/src/ui/failures.dartpackages/dashboard/lib/src/ui/jobs.dartpackages/dashboard/lib/src/ui/overview.dartpackages/dashboard/lib/src/ui/tasks.dartpackages/stem/CHANGELOG.mdpackages/stem/example/autoscaling_demo/bin/worker.dartpackages/stem/example/daemonized_worker/bin/worker.dartpackages/stem/example/dlq_sandbox/bin/worker.dartpackages/stem/example/docs_snippets/lib/daemonization.dartpackages/stem/example/ecommerce/bin/server.dartpackages/stem/example/ecommerce/lib/src/database/orm_registry.g.dartpackages/stem/example/email_service/bin/enqueuer.dartpackages/stem/example/email_service/bin/worker.dartpackages/stem/example/encrypted_payload/worker/bin/worker.dartpackages/stem/example/image_processor/bin/api.dartpackages/stem/example/image_processor/bin/worker.dartpackages/stem/example/microservice/beat/bin/beat.dartpackages/stem/example/microservice/enqueuer/bin/main.dartpackages/stem/example/mixed_cluster/postgres_worker/bin/worker.dartpackages/stem/example/mixed_cluster/redis_worker/bin/worker.dartpackages/stem/example/monolith_service/bin/service.dartpackages/stem/example/ops_health_suite/bin/worker.dartpackages/stem/example/postgres_tls/bin/worker.dartpackages/stem/example/postgres_worker/worker/bin/worker.dartpackages/stem/example/progress_heartbeat/bin/worker.dartpackages/stem/example/rate_limit_delay/bin/worker.dartpackages/stem/example/redis_postgres_worker/worker/bin/worker.dartpackages/stem/example/routing_parity/bin/worker.dartpackages/stem/example/scheduler_observability/bin/beat.dartpackages/stem/example/scheduler_observability/bin/worker.dartpackages/stem/example/signals_demo/bin/producer.dartpackages/stem/example/signals_demo/bin/worker.dartpackages/stem/example/signing_key_rotation/bin/worker.dartpackages/stem/example/task_context_mixed/bin/worker.dartpackages/stem/example/worker_control_lab/bin/worker.dartpackages/stem/lib/src/bootstrap/workflow_app.dartpackages/stem/lib/src/core/contracts.dartpackages/stem/lib/src/core/payload_codec.dartpackages/stem/lib/src/core/payload_map.dartpackages/stem/lib/src/core/stem.dartpackages/stem/lib/src/worker/worker.dartpackages/stem/lib/src/worker/worker_config.dartpackages/stem/lib/src/workflow/core/run_state.dartpackages/stem/lib/src/workflow/core/workflow_event_ref.dartpackages/stem/lib/src/workflow/core/workflow_ref.dartpackages/stem/lib/src/workflow/runtime/workflow_runtime.dartpackages/stem/pubspec.yamlpackages/stem/test/bootstrap/shortcut_allow_auto_start_test.dartpackages/stem/test/bootstrap/stem_app_test.dartpackages/stem/test/bootstrap/workflow_module_bootstrap_test.dartpackages/stem/test/support/fixtures/daemon_stub.dartpackages/stem/test/unit/core/contracts_test.dartpackages/stem/test/unit/core/queue_events_test.dartpackages/stem/test/unit/core/stem_core_test.dartpackages/stem/test/unit/core/task_context_enqueue_test.dartpackages/stem/test/unit/core/task_enqueue_builder_test.dartpackages/stem/test/unit/core/task_invocation_test.dartpackages/stem/test/unit/core/task_registry_test.dartpackages/stem/test/unit/core/task_result_test.dartpackages/stem/test/unit/workflow/flow_context_test.dartpackages/stem/test/unit/workflow/workflow_metadata_views_test.dartpackages/stem/test/unit/workflow/workflow_result_test.dartpackages/stem/test/unit/workflow/workflow_resume_test.dartpackages/stem/test/workflow/workflow_runtime_test.dartpackages/stem_builder/example/bin/main.dartpackages/stem_builder/example/bin/runtime_metadata_views.dartpackages/stem_cli/test/unit/cli/cli_worker_stats_test.dartpackages/stem_postgres/lib/src/workflow/postgres_workflow_store.dartpackages/stem_postgres/lib/src/workflow/postgres_workflow_store_new.dartpackages/stem_redis/lib/src/workflow/redis_workflow_store.dartpackages/stem_sqlite/lib/src/workflow/sqlite_workflow_store.dart
💤 Files with no reviewable changes (4)
- packages/stem/lib/src/core/payload_codec.dart
- packages/stem/lib/src/workflow/core/workflow_event_ref.dart
- packages/stem/test/unit/core/task_registry_test.dart
- packages/stem_cli/test/unit/cli/cli_worker_stats_test.dart
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (86)
.gitignore (1)
38-39: Looks good for local workspace hygiene.Line 39 correctly keeps the local
packages/stem_serinus/module out of version control.packages/stem/lib/src/workflow/runtime/workflow_runtime.dart (1)
327-327: Assertion refactor is safe.Line 327 keeps the same exclusivity check for
decode,decodeJson, anddecodeVersionedJson.packages/dashboard/lib/src/ui/failures.dart (1)
111-111: LGTM for the lambda compacting change.Line 111 is a formatting-only simplification with unchanged behavior.
packages/stem_postgres/lib/src/workflow/postgres_workflow_store_new.dart (1)
84-86: Run ID normalization remains correct.Lines 84-86 preserve existing behavior while improving compactness.
packages/stem_postgres/lib/src/workflow/postgres_workflow_store.dart (1)
109-111: Create-run ID selection is unchanged and valid.Lines 109-111 keep the same
runId.trim()fallback-to-UUID behavior.packages/stem/lib/src/core/stem.dart (1)
544-544: Safe assert-only refactor.Line 544 keeps the same one-decoder-only invariant in
waitForTask.packages/stem/lib/src/workflow/core/run_state.dart (1)
115-115: Getter compaction is correct.Line 115 preserves the same nullable
parentRunIdextraction behavior.packages/stem/test/unit/core/task_enqueue_builder_test.dart (2)
166-178: LGTM - Formatting adjustment only.The indentation changes improve consistency without altering test logic.
197-212: LGTM - Formatting adjustment only.The indentation changes improve consistency without altering test logic.
packages/stem/test/workflow/workflow_runtime_test.dart (3)
821-865: LGTM - Formatting adjustment only.The indentation changes improve readability without altering test behavior.
871-915: LGTM - Formatting adjustment only.The indentation changes improve readability without altering test behavior.
921-967: LGTM - Formatting adjustment only.The indentation changes improve readability without altering test behavior.
packages/stem_redis/lib/src/workflow/redis_workflow_store.dart (1)
360-362: LGTM - Formatting consolidation.The ternary expression has been condensed to a single line while preserving the original logic (null/empty check, trimming, and fallback ID generation).
packages/dashboard/lib/src/ui/tasks.dart (1)
174-174: LGTM - Lambda formatting consolidated.The
actionsBuildercallback has been condensed to a single line without changing its behavior.packages/dashboard/lib/src/server.dart (1)
1034-1063: LGTM - Restructured filtering to fluent chain.The filter logic has been reformatted into a fluent chain (
tasks.where(...).toList()) while preserving all filter conditions (queue, namespace, task name, run ID, and state checks).packages/stem_builder/example/bin/main.dart (2)
14-16: LGTM - JSON encoder formatting adjustment.The
JsonEncoder.withIndentconstruction has been reformatted without changing its behavior.
46-46: LGTM - Method call condensed.The
enqueueAndWaitinvocation has been condensed to a single line while preserving all parameters.packages/stem/test/unit/core/task_result_test.dart (1)
108-114: LGTM - Test matcher reformatted.The assertion matcher has been reformatted for better readability without changing the expected error validation logic.
packages/stem/lib/src/core/payload_map.dart (2)
89-93: LGTM - Method call formatting adjustment.The
valueJsoncall has been reformatted with adjusted indentation while preserving the logic.
129-135: LGTM - Method call formatting adjustment.The
valueVersionedJsoncall has been reformatted with adjusted indentation while preserving the logic.packages/stem/test/unit/workflow/flow_context_test.dart (1)
124-126: Formatting-only test reflow looks good.This is a non-functional readability change and preserves test behavior.
packages/stem/test/unit/core/task_context_enqueue_test.dart (1)
142-168: Indentation/scoping cleanup is correct.The updated block remains behaviorally identical and improves readability.
packages/stem/test/unit/workflow/workflow_result_test.dart (1)
79-79: Matcher reformat is safe and clear.These edits are presentation-only and keep assertion semantics intact.
Also applies to: 86-86, 109-115
packages/stem/test/unit/workflow/workflow_metadata_views_test.dart (1)
203-207: Assertion formatting updates look good.No correctness impact observed; this is a clean readability improvement.
Also applies to: 214-218, 224-228, 235-239, 521-525, 532-536
packages/stem/test/unit/core/queue_events_test.dart (1)
175-215: Test-body reformat is correct.Behavior is unchanged and readability is improved.
packages/stem/test/unit/workflow/workflow_resume_test.dart (1)
491-493: Method-chain line wrapping is safe.No logic changes introduced; tests still exercise the same path.
Also applies to: 517-519
packages/stem/test/unit/core/stem_core_test.dart (1)
412-416: Formatting reflow in tests is good.I don’t see behavior changes in these segments.
Also applies to: 841-864
packages/stem_builder/example/bin/runtime_metadata_views.dart (1)
13-15: Example formatting cleanup looks correct.These changes are non-functional and preserve behavior.
Also applies to: 34-37
packages/stem_sqlite/lib/src/workflow/sqlite_workflow_store.dart (1)
93-95: Formatting-only reflow is safe here.Line 93-Line 95 keeps the same run-id resolution behavior and fallback generation; no functional regression risk in this change.
packages/dashboard/lib/src/services/stem_service.dart (1)
41-43: Interface signature change is non-functional and clean.Line 41-Line 43 is a formatting-only update with unchanged contract semantics.
packages/dashboard/lib/src/ui/jobs.dart (1)
16-28: Refactor preserves filtering semantics and improves readability.Line 16-Line 28 keeps the same task/queue matching logic while making the predicate flow easier to read.
packages/stem/lib/src/bootstrap/workflow_app.dart (2)
561-563: Assert reformat is behavior-preserving.Line 561-Line 563 keeps the same guard and message; no change to validation behavior.
1072-1074: Signature formatting update is safe.Line 1072-Line 1074 is a presentation-only change with unchanged inputs and control flow.
packages/stem/test/bootstrap/workflow_module_bootstrap_test.dart (1)
164-180: Test-body reindent is non-functional and correct.Line 164-Line 180 retains the same assertions and lifecycle handling.
packages/stem/test/bootstrap/shortcut_allow_auto_start_test.dart (1)
43-87: Test reformat is safe and keeps the same coverage intent.Line 43-Line 87 preserves runtime/worker state assertions and completion checks without logic changes.
packages/stem/test/bootstrap/stem_app_test.dart (1)
552-589: Formatting-only test block update looks good.Line 552-Line 589 keeps the same codec path and assertions, so this is a safe non-functional change.
packages/dashboard/lib/src/ui/overview.dart (2)
71-73: Signature formatting is non-functional and clear.Line 71-Line 73 preserves the same
defaultNamespacedefault and API behavior.
332-339: Call-site reindent is safe.Line 332-Line 339 does not alter arguments or table options; output behavior remains unchanged.
packages/stem/example/ecommerce/lib/src/database/orm_registry.g.dart (1)
1-85: LGTM - Generated file with formatting-only changes.This is an auto-generated file with cosmetic reformatting (semicolon placement, empty body condensing, multi-line parameter formatting). No functional changes.
packages/stem/example/rate_limit_delay/bin/worker.dart (1)
49-52: LGTM - Correct platform-aware signal handling.The SIGTERM guard is appropriate since Windows doesn't support this signal and
ProcessSignal.sigterm.watch()would throw aSignalException. SIGINT remains active on all platforms as expected.packages/stem/example/signals_demo/bin/worker.dart (1)
46-49: LGTM - Consistent with the platform-aware signal handling pattern.packages/stem/example/postgres_tls/bin/worker.dart (1)
64-67: LGTM - Consistent platform-aware signal handling.packages/stem/example/monolith_service/bin/service.dart (1)
150-153: LGTM - Consistent platform-aware signal handling.packages/stem/example/image_processor/bin/worker.dart (1)
58-61: LGTM - Consistent platform-aware signal handling.packages/stem/example/encrypted_payload/worker/bin/worker.dart (1)
60-63: LGTM - Consistent platform-aware signal handling.packages/stem/example/ecommerce/bin/server.dart (1)
41-44: LGTM - Consistent platform-aware signal handling.packages/stem/CHANGELOG.md (1)
3-7: Changelog update is clear and release-relevant.This entry is concise and appropriately documents the behavior change for the upcoming version.
packages/stem/example/signing_key_rotation/bin/worker.dart (1)
46-48: Platform guard is applied cleanly.The change is minimal, targeted, and keeps shutdown wiring consistent with the rest of the worker flow.
packages/stem/example/docs_snippets/lib/daemonization.dart (1)
24-26: Doc snippet change is consistent and easy to follow.This keeps the daemonization example aligned with the new cross-platform shutdown approach.
packages/stem/example/ops_health_suite/bin/worker.dart (1)
41-43: Targeted update with no collateral changes.Good scoped adjustment to the signal registration block.
packages/stem/example/mixed_cluster/postgres_worker/bin/worker.dart (1)
60-62: Nicely scoped platform-specific adjustment.This follows the same pattern used elsewhere and keeps the worker setup straightforward.
packages/stem/example/redis_postgres_worker/worker/bin/worker.dart (1)
62-64: Change is consistent with the rest of the PR.The update is localized and preserves the existing shutdown path structure.
packages/stem/example/postgres_worker/worker/bin/worker.dart (1)
66-68: Good focused update in the shutdown registration block.This remains aligned with the intended platform-aware behavior change.
packages/stem/example/dlq_sandbox/bin/worker.dart (1)
39-41: Clean, minimal platform-aware change.The adjusted signal registration is applied consistently and without unnecessary churn.
packages/stem/example/mixed_cluster/redis_worker/bin/worker.dart (1)
56-59: LGTM!Correct platform-conditional SIGTERM registration. On Windows,
ProcessSignal.sigterm.watch()throws aSignalExceptionsince SIGTERM is not supported, while SIGINT works cross-platform.packages/stem/example/image_processor/bin/api.dart (1)
75-78: LGTM!Consistent platform-guarded SIGTERM handling.
packages/stem/example/task_context_mixed/bin/worker.dart (1)
23-32: LGTM!Platform-guarded SIGTERM handling is consistent with the other examples in this PR.
packages/stem/example/microservice/enqueuer/bin/main.dart (1)
262-265: LGTM!Consistent platform-guarded SIGTERM handling.
packages/stem/example/worker_control_lab/bin/worker.dart (1)
49-52: LGTM!Consistent platform-guarded SIGTERM handling.
packages/stem/example/progress_heartbeat/bin/worker.dart (1)
44-47: LGTM!Consistent platform-guarded SIGTERM handling.
packages/stem/example/email_service/bin/enqueuer.dart (1)
84-87: LGTM!Consistent platform-guarded SIGTERM handling.
packages/stem/example/microservice/beat/bin/beat.dart (1)
61-64: LGTM!Consistent platform-guarded SIGTERM handling.
packages/stem/example/daemonized_worker/bin/worker.dart (1)
21-24: LGTM!The conditional SIGTERM registration correctly avoids installing an unsupported signal handler on Windows while maintaining SIGINT for cross-platform graceful shutdown. This aligns with the core worker implementation pattern.
packages/stem/example/email_service/bin/worker.dart (1)
73-76: LGTM!Consistent platform-guarded SIGTERM handling. The email worker retains graceful shutdown capability via SIGINT on all platforms.
packages/stem/example/signals_demo/bin/producer.dart (1)
38-41: LGTM!Platform-guarded SIGTERM registration follows the established pattern. The producer will shut down cleanly via SIGINT on Windows.
packages/stem/test/support/fixtures/daemon_stub.dart (1)
27-33: Verify test behavior on Windows after SIGTERM handler removal.This test fixture is used by
cli_worker_health_test.dartandcli_worker_multi_test.dart, both of which send SIGTERM to shut down the stub process (seecli_worker_health_test.dart:76and theworker multi stopcommand). On Windows, the stub will now ignore SIGTERM, potentially causing:
- Orphaned processes — mitigated by the 5-second fallback timer on lines 17-21
- Test timing differences — tests may wait longer for the stub to exit via timeout rather than immediate signal response
Consider whether the tests need adjustment for Windows (e.g., using SIGINT instead, or relying on the fallback timer).
[raise_minor_issue, request_verification]
#!/bin/bash # Check how tests interact with this daemon stub and handle process termination echo "=== Files that reference daemon_stub.dart ===" rg -l "daemon_stub" --type dart echo -e "\n=== SIGTERM usage in test files ===" rg -n "sigterm|SIGTERM" --type dart -g '*test*' -C 2 echo -e "\n=== Process.kill usage in test files ===" rg -n "\.kill\(" --type dart -g '*test*' -C 2packages/stem/example/scheduler_observability/bin/beat.dart (1)
58-61: LGTM!Consistent platform-guarded SIGTERM registration for the beat scheduler. Graceful shutdown via SIGINT remains available on all platforms.
packages/stem/example/autoscaling_demo/bin/worker.dart (1)
69-72: LGTM!Platform-guarded SIGTERM handling mirrors the core worker implementation in
packages/stem/lib/src/worker/worker.dart:3220-3233. The autoscaling demo maintains cross-platform shutdown capability via SIGINT.packages/stem/example/routing_parity/bin/worker.dart (1)
48-51: LGTM!Consistent platform-guarded SIGTERM registration for the routing parity worker.
packages/stem/example/scheduler_observability/bin/worker.dart (1)
39-42: LGTM!Platform-guarded SIGTERM handling aligns with the core worker pattern and other examples in this PR.
packages/stem/test/unit/core/contracts_test.dart (4)
345-349: Matcher reflow is clean and preserves assertion intent.No behavioral change here; this formatting keeps the expectation readable and equivalent.
358-362: Versioned matcher formatting looks correct.The
having(...)chain remains semantically identical after reformatting.
591-595: Typed metadata assertion formatting is good.Readable and functionally unchanged.
602-606: Versioned metadata matcher reflow is correct.No concerns; behavior remains unchanged.
packages/stem/lib/src/workflow/core/workflow_ref.dart (3)
48-60:resultCodecselection remains correct after reflow.The nullability and precedence between versioned/json decoders are preserved.
86-98:versionedJsonfactory codec branching is still sound.Formatting-only change with unchanged decode path behavior.
166-178:versionedMapresult codec conditional is preserved correctly.No regression in fallback logic.
packages/stem/lib/src/core/contracts.dart (3)
2807-2819:TaskDefinition.jsoncodec branch is behaviorally unchanged.This refactor keeps decode selection logic intact.
2849-2861:TaskDefinition.versionedJsoncodec branch remains correct.No semantic change in decoder precedence or null fallback.
2947-2959:TaskDefinition.versionedMapresult codec selection is still correct.Formatting-only adjustment; logic is preserved.
packages/stem/lib/src/worker/worker.dart (2)
3212-3234: LGTM! Platform-aware signal handling is correctly implemented.The guard for
!Platform.isWindowsbefore registering SIGTERM/SIGQUIT handlers is appropriate since Windows doesn't support these signals. This aligns with the pattern used in the test fixture (daemon_stub.dart:27-32). The existing_safeWatchtry-catch provides a secondary safeguard for other unsupported runtimes.
3236-3246: Doc comment update is accurate.The change from "platforms" to "runtimes" better captures the various contexts where signal watching may be unsupported (e.g., browser environments, not just Windows).
packages/stem/test/unit/core/task_invocation_test.dart (2)
280-286: LGTM! Formatting improvements enhance readability.The multiline formatting for the
expectassertion makes the expected value structure clearer.
536-579: LGTM! Chained method assertions are now more readable.Breaking the chained method calls (e.g.,
.argsVersionedJson<...>(...).stage) across multiple lines improves code clarity without changing test behavior.packages/stem/lib/src/worker/worker_config.dart (2)
163-179: LGTM! Documentation accurately reflects platform-specific signal handling.The updated table clearly communicates that SIGTERM and SIGQUIT handlers are only installed on platforms that support them, with an explicit note about Windows limitations. This aligns with the implementation in
worker.dart.
214-215: LGTM! Doc comment update is appropriately generic.Changing to "host platform process signal handlers" correctly abstracts away the platform-specific details, making the documentation accurate for all supported platforms.
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores