feat(spider-storage): Implement scheduler registration gRPC service and re-enqueue ready tasks when a scheduler reconnects. - #374
Conversation
WalkthroughThis PR implements the previously stubbed ChangesScheduler registration and service state refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SchedulerRegistrationService
participant ServiceState
participant JobCache
Client->>SchedulerRegistrationService: register_scheduler(ip_addr, port)
SchedulerRegistrationService->>ServiceState: register_scheduler(ip_addr, port)
ServiceState->>ServiceState: lock has_previous_scheduler_connection
alt previous scheduler existed
ServiceState->>JobCache: spawn resend_ready_tasks()
JobCache-->>ServiceState: success or error
ServiceState->>ServiceState: cancel() via CancellationToken on error
end
ServiceState-->>SchedulerRegistrationService: scheduler_id or error
SchedulerRegistrationService-->>Client: RegisterSchedulerResponse or error status
Possibly related PRs
Suggested reviewers: 🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/spider-storage/src/state/service.rs (1)
715-745: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDetached resend task: consider overlapping resends and shutdown behaviour.
The mutex correctly serializes registrations and gates the resend. Two edge cases worth confirming:
- The resend is fire-and-forget and only guarded by the flag, not the lock, so rapid re-registrations can spawn multiple
resend_ready_tasks()executions that run concurrently against the ready queue.- The spawned handle isn't tracked, so it can't be awaited/aborted during
Runtime::stop.If both are acceptable for the reconnect flow (resend is idempotent-enough and shutdown-race tolerable), this is fine as-is.
🤖 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 `@components/spider-storage/src/state/service.rs` around lines 715 - 745, The resend path in register_scheduler can spawn multiple concurrent resend_ready_tasks() jobs on rapid re-registration, and the detached tokio::spawn handle is never tracked for shutdown. Serialize or deduplicate the resend by moving the guard/state to cover the spawned work, and store the JoinHandle somewhere in the service so Runtime::stop can abort or await it using the existing cancellation_token and job_cache flow.
🤖 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.
Nitpick comments:
In `@components/spider-storage/src/state/service.rs`:
- Around line 715-745: The resend path in register_scheduler can spawn multiple
concurrent resend_ready_tasks() jobs on rapid re-registration, and the detached
tokio::spawn handle is never tracked for shutdown. Serialize or deduplicate the
resend by moving the guard/state to cover the spawned work, and store the
JoinHandle somewhere in the service so Runtime::stop can abort or await it using
the existing cancellation_token and job_cache flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2cafb32c-269f-427f-a3c2-d1c5db4ee5c5
📒 Files selected for processing (5)
components/spider-storage/src/grpc.rscomponents/spider-storage/src/state.rscomponents/spider-storage/src/state/runtime.rscomponents/spider-storage/src/state/service.rscomponents/spider-storage/src/state/test_utils.rs
Description
This PR implements scheduler registration in the storage service end-to-end.
ServiceState::register_scheduleris fleshed out and wired into the previously-stubbedSchedulerRegistrationService::register_schedulergRPC handler, so a scheduler can register with storage and receive its assignedSchedulerIdand the current storage session.The core behavior added here is recovery on scheduler reconnect: registration is mutually exclusive, and when a new registration replaces a previously-connected scheduler, storage re-enqueues every cached job's ready tasks in a background task so the new scheduler's inbound queue is repopulated. To support cancelling the service if that background resend fails, the runtime's
CancellationTokenis now threaded intoServiceState. Along the way,ServiceState::new's growing argument list is bundled into aServiceStateParamsstruct to silenceclippy::too_many_arguments.The scope is limited to
SchedulerRegistrationService.get_schedulersremains unimplemented (it now returns an explicitUNIMPLEMENTEDstatus instead of panicking).Service layer (
state/service.rs)register_schedulernow guards registration with a per-service asyncMutex<bool>(has_previous_scheduler_connection), making registrations mutually exclusive: only one can be in flight at a time. It callsdb.register_scheduler, and on success, if a scheduler was previously registered, it spawns a background task that callsJobCache::resend_ready_tasks(). If that resend fails, the task logs the error and cancels the service via theCancellationToken. The flag is then set totrue. The resend runs off the critical path (in a spawned task), so the RPC returns promptly.ServiceStateInnergains two fields:has_previous_scheduler_connection: tokio::sync::Mutex<bool>andcancellation_token: CancellationToken.register_schedulermoves clones ofjob_cacheand the cancellation token into atokio::spawned future, the three service type parameters now carry a'staticbound (ServiceState,ServiceStateInner, and — transitively —RuntimeandGrpcServiceState).Constructor bundle (
ServiceStateParams,state/service.rs+state.rs)ServiceStateParams, apubstruct holding the eight fieldsServiceState::newneeds (db,session_id,job_cache,ready_queue_sender,ready_queue_receiver,task_instance_pool_connector,job_cache_gc_handle,cancellation_token).newnow takes a singleServiceStateParamsand destructures it, resolving theclippy::too_many_argumentswarning that the newcancellation_tokenargument would otherwise trip.ServiceStateParamsfrom thestatemodule soruntime.rs(and other constructors) can build it with struct-literal syntax.gRPC layer (
grpc.rs)SchedulerRegistrationService::register_scheduler(wastodo!("Not implemented")): it unpacks the request into(ip_addr, port), callsServiceState::register_scheduler, and returns aRegisterSchedulerResponsecarryingSchedulerRegistration { scheduler_id, session_id }.get_schedulersnow returnsStatus::unimplemented("not implemented")instead oftodo!— a still-unimplemented RPC no longer panics (and takes down) the service if called.scheduler_registration_service_error_handler, the single chokepoint mapping aStorageServerErrorfrom this service to aStatus. Every error maps to an opaqueINTERNALviadefault_error_handler(non-strict), mirroringinbound_queue_service_error_handler.GrpcServiceState's type parameters gain the'staticbound to matchServiceState.Runtime wiring (
state/runtime.rs)create_runtimenow passescancellation_token.clone()intoServiceStateParamswhen constructing the service state.Runtime's type parameters gain the'staticbound (propagated fromServiceState).ServiceStateParams { .. }struct-literal form.Test mock (
state/test_utils.rs)MockDbConnector::register_schedulerpreviously didunreachable!("not implemented for mock connector"), which would panic the moment the service called into it. It now returns an incrementingSchedulerIdfrom a newnext_scheduler_id: Arc<AtomicUsize>counter, mirroring the existingregister_execution_managermock.get_schedulers/is_scheduler_registeredremainunreachable!.Tests
register_scheduler_resends_ready_tasks_only_when_replacing_previous_scheduler. It builds a service backed by a real ready queue, submits and starts a single-task job, and drains the ready task thatstart_jobenqueues so the queue is empty before probing. It then asserts that the firstregister_schedulerdoes not resend (queue stays empty), and that the secondregister_schedulerdoes resend the job's ready task (one entry, matchingjob_id). Because the resend runs in a spawned background task, the test usestokio::task::yield_now()to let it run before polling.cancel_job_/succeed_task_instance_/fail_task_instance_enqueues_terminal_job_for_cache_gc, and the two service factories) were updated to theServiceStateParamsform.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Bug Fixes