opt-in ping_interval_secs keepalive on the listener#1942
Conversation
|
Someone is attempting to deploy a commit to the motia Team on Vercel. A member of the Team first needs to authorize it. |
License agreement recorded@Atlas1225, your agreement has been recorded and you have been added to contributors.md. All future PRs from your account will pass this check automatically. |
📝 WalkthroughWalkthroughAdds an opt-in ChangesPing Keepalive Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Engine
participant WorkerRegistry
Engine->>Engine: start ping interval timer
loop on each tick
alt inbound inactive beyond 2x interval
Engine->>WorkerRegistry: close/reap connection
else
Engine->>Client: send WebSocket Ping frame
end
end
Client-->>Engine: inbound frame received
Engine->>Engine: update last_inbound timestamp
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/src/engine/mod.rs (1)
1542-1607: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument
ping_interval_secsin worker-manager docs and SDK examples. The engine accepts the new option, but the worker-manager docs and SDK-facing config surface don’t expose it yet.🤖 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 `@engine/src/engine/mod.rs` around lines 1542 - 1607, The worker ping interval option is implemented in the engine loop, but it is not exposed in the worker-manager documentation or SDK-facing configuration examples. Update the relevant docs and examples to include ping_interval_secs, and make sure the worker-manager config surface and any SDK config structs or builders that feed into this code path reference the new option consistently so users can discover and set it.Source: Path instructions
🧹 Nitpick comments (2)
engine/src/workers/worker/mod.rs (1)
275-282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend default-value test to cover the new field.
worker_config_default_valuesasserts port/host/middleware/rbac defaults but doesn't assertping_interval_secsdefaults toNone, leaving the new field's default behavior unverified by this test.✅ Proposed test addition
assert!(config.middleware_function_id.is_none()); assert!(config.rbac.is_none()); + assert!(config.ping_interval_secs.is_none());🤖 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 `@engine/src/workers/worker/mod.rs` around lines 275 - 282, The worker_config_default_values test in WorkerManagerConfig should also verify the new ping_interval_secs field defaults to None. Update the existing assertion set in worker_config_default_values to include an assertion for ping_interval_secs alongside the current port, host, middleware_function_id, and rbac checks so the default behavior of WorkerManagerConfig remains covered.engine/src/engine/mod.rs (1)
1542-1607: 🩺 Stability & Availability | 🔵 TrivialConsider whether
/otelconnections need the same liveness check.
handle_otel(unchanged in this PR) shares the same WebSocket transport and can equally hang on a peer that disappears without FIN/RST (e.g., device suspension, NAT path loss), but it has no ping/reap mechanism. This is out of scope for this PR's stated goal (worker-manager listener), but may be worth a follow-up for consistency.🤖 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 `@engine/src/engine/mod.rs` around lines 1542 - 1607, The `/otel` WebSocket path currently lacks the same ping-based liveness handling used in the worker connection loop, so `handle_otel` can hang indefinitely on dead peers. Update `handle_otel` to share the same ping/reap behavior as the existing `ping_timer`/`ws_rx.next()` loop in `engine::mod`, or refactor the shared connection-handling logic so both `handle_otel` and the worker listener use the same keepalive mechanism.
🤖 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.
Outside diff comments:
In `@engine/src/engine/mod.rs`:
- Around line 1542-1607: The worker ping interval option is implemented in the
engine loop, but it is not exposed in the worker-manager documentation or
SDK-facing configuration examples. Update the relevant docs and examples to
include ping_interval_secs, and make sure the worker-manager config surface and
any SDK config structs or builders that feed into this code path reference the
new option consistently so users can discover and set it.
---
Nitpick comments:
In `@engine/src/engine/mod.rs`:
- Around line 1542-1607: The `/otel` WebSocket path currently lacks the same
ping-based liveness handling used in the worker connection loop, so
`handle_otel` can hang indefinitely on dead peers. Update `handle_otel` to share
the same ping/reap behavior as the existing `ping_timer`/`ws_rx.next()` loop in
`engine::mod`, or refactor the shared connection-handling logic so both
`handle_otel` and the worker listener use the same keepalive mechanism.
In `@engine/src/workers/worker/mod.rs`:
- Around line 275-282: The worker_config_default_values test in
WorkerManagerConfig should also verify the new ping_interval_secs field defaults
to None. Update the existing assertion set in worker_config_default_values to
include an assertion for ping_interval_secs alongside the current port, host,
middleware_function_id, and rbac checks so the default behavior of
WorkerManagerConfig remains covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3c8ef60b-e861-4b9c-8fa7-b9735c0c324c
📒 Files selected for processing (4)
engine/src/engine/mod.rsengine/src/workers/worker/mod.rsengine/tests/rbac_infrastructure_e2e.rsengine/tests/worker_manager_ping_keepalive_e2e.rs
|
Do I need to do any change? |
|
Hey @Atlas1225 thanks for your contribution! This really makes sense. Before I merge it, do you mind fixing the linter issue? |
2 participants
The worker-manager listener has no active liveness mechanism: the engine
only answers Ping frames, never initiates them, and a connection whose
peer died without FIN/RST (suspended device, NAT-dropped path) lingers in
the worker and trigger registries until a write to it exhausts TCP
retransmissions.
Add an opt-in
ping_interval_secsto WorkerManagerConfig. When set, thelistener sends a WebSocket Ping frame to each connection every interval
and closes connections that produce no inbound frame (Pong included —
browsers and WS libraries auto-reply per RFC 6455) for two consecutive
intervals. Unset keeps the previous behavior unchanged.
Summary by CodeRabbit
New Features
Bug Fixes
Tests