feat(spider-storage): Add ServerRuntime to gracefully shutdown storage service.#324
feat(spider-storage): Add ServerRuntime to gracefully shutdown storage service.#324sitaowang1998 wants to merge 8 commits into
ServerRuntime to gracefully shutdown storage service.#324Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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.
Actionable comments posted: 2
🤖 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 `@components/spider-storage/src/state/server.rs`:
- Around line 102-109: The timeout currently moves
self.task_instance_pool_join_handle into tokio::time::timeout so if the timeout
fires the handle is dropped and the background task keeps running; change the
logic to detect timeout without consuming the handle (e.g., use tokio::select!
between the join handle future and tokio::time::sleep) so you can call abort()
on the JoinHandle (self.task_instance_pool_join_handle.abort()) when the timeout
branch wins, then return the StorageServerError::Stopping error; reference the
existing stop_timeout_sec and task_instance_pool_join_handle symbols and ensure
you await or ignore the aborted handle appropriately after aborting.
In `@components/spider-storage/src/task_instance_pool.rs`:
- Around line 169-170: Clamp the public config fields channel_size and
gc_interval to at least 1 before creating Tokio primitives to avoid panics: when
calling mpsc::channel(...) use config.channel_size.max(1) and when creating the
interval with tokio::time::interval(...) use config.gc_interval.max(1) (or
assign clamped values to locals first). Update the TaskInstancePool
construction/initialization sites (references: TaskInstancePoolConfig,
mpsc::channel, tokio::time::interval, variables sender/receiver and the GC
interval setup) to pass the clamped values so zero cannot be forwarded into
Tokio APIs.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 71fa0b19-7bcb-4d56-96c9-4681b1e1b273
📒 Files selected for processing (5)
components/spider-storage/Cargo.tomlcomponents/spider-storage/src/cache/error.rscomponents/spider-storage/src/state.rscomponents/spider-storage/src/state/server.rscomponents/spider-storage/src/task_instance_pool.rs
Description
This PR:
ServerRuntimetype that keeps aCancellationTokenandJoinHandleto gracefully shutdown the service.ExecutionManagerLivenessStorein task instance pool intoExecutionManagerLivenessManagementin db.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Release Notes
Chores
Refactor