feat(sdk): auto-enroll default pollers into autoscaling - #1425
Conversation
5954b11 to
d600242
Compare
| /// `poller_autoscaling_auto_enroll` capability. Layers above core (the Rust SDK, the c-bridge) | ||
| /// set this; direct core users leave it `false`. | ||
| #[builder(default = false)] | ||
| pub workflow_task_poller_behavior_auto_enroll: bool, |
There was a problem hiding this comment.
I don't think we need these. They can just explicitly set the option for whatever they do want, rather than disabling the enrollment explicitly.
There was a problem hiding this comment.
Good point, I refactored the three poller-behavior fields on WorkerConfig to be Option<PollerBehavior> now
| impl TryFrom<&PollerBehavior> for Option<temporalio_sdk_core::PollerBehavior> { | ||
| type Error = anyhow::Error; | ||
| fn try_from(value: &PollerBehavior) -> Result<Self, Self::Error> { | ||
| fn try_from(value: &PollerBehavior) -> anyhow::Result<Self> { |
There was a problem hiding this comment.
This won't compile (and doesn't, in CI).
| at_task_mgr: Option<WorkerActivityTasks>, | ||
| /// The poller-dependent subsystems (workflows, activity task manager, nexus manager). Built | ||
| /// lazily once namespace capabilities are known so effective poller behavior can be resolved. | ||
| pollers: std::sync::OnceLock<PollerSubsystems>, |
There was a problem hiding this comment.
These really aren't (just) pollers. TaskSubsystems would be better I think.
There was a problem hiding this comment.
Good point, renamed to TaskSubsystems
| // If shutdown was already initiated before the pollers were built (e.g. a worker that | ||
| // never ran validate() and only starts polling as part of draining during shutdown), | ||
| // propagate the shutdown signal to the freshly-built subsystems so they don't hang waiting | ||
| // for work that will never arrive. `initiate_shutdown` deliberately does not force a build, | ||
| // so it can't have done this for us. | ||
| if shutdown_token.is_cancelled() { |
There was a problem hiding this comment.
If the task subsystems were never built, we shouldn't need to do anything at all I think?
There was a problem hiding this comment.
Good point, removed this unnecessary special case handling
| pollers: std::sync::OnceLock<PollerSubsystems>, | ||
| /// Builds [PollerSubsystems] on first access. Taken exactly once. | ||
| #[allow(clippy::type_complexity)] | ||
| poller_builder: parking_lot::Mutex<Option<Box<dyn FnOnce() -> PollerSubsystems + Send>>>, |
There was a problem hiding this comment.
I think we could avoid needing this, and I think we can kill two birds with one stone and get rid of PollerInitInputs at the same time.
The pollers (now task_subsystems) field, should instead be a https://doc.rust-lang.org/beta/std/sync/struct.LazyLock.html
The closure given to the LazyLock constructor can be the same one you give now, and build_poller_subsystems can be inlined to eliminate the need for PollerInitInputs
There was a problem hiding this comment.
Done, turned task_subsystems into a LazyLock. Removed PollerInitInputs and inlined build_poller_subsystems
| /// Whether this worker polls for (non-local) activities at all. | ||
| activities_enabled: bool, |
There was a problem hiding this comment.
We don't need a separate bool here. Simply not initializing the OnceLock is sufficient.
There was a problem hiding this comment.
Done, removed activities_enabled
d600242 to
e32bf5f
Compare
| /// True once `task_subsystems` has been built. Lets shutdown/introspection peek without forcing | ||
| /// a build (MSRV-safe stand-in for `LazyLock::get`, which is only stable since 1.94). | ||
| task_subsystems_built: Arc<AtomicBool>, |
There was a problem hiding this comment.
It's fine for us to upgrade to requiring 1.94 luckily, since Rust SDK is still not GA.
I'm also not sure we actually need to use get though. Since we force loading, it should be fine to just deref the lock where it's used.
There was a problem hiding this comment.
Done, dropped LazyLock::get and now just using deref at the use sites
I checked and deref/force/new are all ≤1.80 so should be good to keep MSRV stays at 1.88 for now.
| /// namespace capabilities are known so effective poller behavior can be resolved. | ||
| #[allow(clippy::type_complexity)] | ||
| task_subsystems: | ||
| std::sync::LazyLock<TaskSubsystems, Box<dyn FnOnce() -> TaskSubsystems + Send>>, |
There was a problem hiding this comment.
Nit: remove the std::sync:: prefix and just import directly.
There was a problem hiding this comment.
Done, imported LazyLock/OnceLock directly and dropped the std::sync:: prefixes
| // can be resolved with knowledge of those capabilities. Everything it needs is captured by | ||
| // clone (or moved) *before* the synchronous heartbeat manager below consumes the originals. | ||
| let task_subsystems_builder: Box<dyn FnOnce() -> TaskSubsystems + Send> = { | ||
| let task_subsystems_built = task_subsystems_built.clone(); |
There was a problem hiding this comment.
Fairly sure not all of these need to actually be clone()d and some of them could just be moved into the closure.
There was a problem hiding this comment.
Looked into this, tried removing the clone calls but it causes compiler errors. Unfortunately won't be possible in this case but thanks for flagging!
8489fd7 to
8611a23
Compare
When a namespace advertises the `poller_autoscaling_auto_enroll` capability, workers automatically switch workflow, activity, and nexus pollers to autoscaling — but only for poller types the user left at their default (neither a fixed poller count nor an explicit behavior). Explicitly configured pollers are left unchanged. The effective `PollerBehavior` is resolved once, after the namespace capability is known. Poller construction is deferred out of the synchronous `Worker::new_with_pollers` into a lazily-built step that runs after `Worker::validate()` populates capabilities (and on first poll for paths that skip validate, e.g. replay). This keeps `poll_buffer.rs` / `PollScaler` free of any runtime mode-switching — the behavior is simply passed in already resolved. Eligibility (user left the poller at its default) is signalled from the layers above core: the Rust SDK exposes the poller options as `Option`, and the c-bridge treats unset (both FFI pointers null) as eligible. Ports temporalio/sdk-go#2442.
Worker::run() now calls validate(), which describes the namespace to discover capabilities. Mock-based tests never wired up that expectation, so mockall panicked; during unwind this became a non-unwinding abort that killed the whole test process and cascaded into unrelated tests. Add a fallback describe_namespace returning an empty response (no capabilities, so poller behavior stays at its default) for both the automock (build_mock_pollers) and manual (mock_manual_worker_client, used by replay) paths. Registered with an open call count and after any test-provided expectation, so mockall's FIFO matching still prefers a test's own expectation and workers that never validate don't trip an unsatisfied expectation.
cecd2e3 to
1a84794
Compare
|
I manually tested this PR locally in all 5 Rust-based SDKs (Rust, Python, .NET, Ruby, TypeScript) successfully. Steps:
Result — ✅ all five pass Pending: We need to upgrade the Rust core version in all the Rust based SDKs. SDK team will handle this (discussed with Spencer) |
Ports temporalio/sdk-go#2442 to
sdk-core, so every SDK built on it (Rust directly; TS/Python/.NET/Ruby via the c-bridge) gets it.Supersedes #1406 — same feature, reworked per review feedback (see "How" below).
What it does
A worker's pollers can run in one of two modes: a fixed number, or autoscaling (the server tells the worker to add/remove pollers based on load).
This makes a worker automatically use autoscaling when the server says the namespace supports it (
poller_autoscaling_auto_enroll) — but only for pollers the user didn't configure. If you set a poller count or behavior yourself, it's left exactly as you asked.How it works
The worker figures out the right mode once, right after it checks in with the server (
validate()), and builds its pollers then. Previously the pollers were built before the check-in, so #1406 had to flip the mode later at runtime; this version avoids that entirely (poll_buffer.rsis unchanged here).worker/mod.rs: poller setup is deferred until after the server check-in, so the mode is known up front.validate()on startup (it never did before, so it was missing all namespace capabilities).Testing
Unit tests for the mode decision and the capability parsing; c-bridge conversion tests.
cargo check/fmt/clippyclean; sdk-core lib 368 pass, c-bridge 14 pass.