fix(spider-storage): Add DeleteResourceGroup and ResendReadyTasks. - #366
fix(spider-storage): Add DeleteResourceGroup and ResendReadyTasks.#366sitaowang1998 wants to merge 37 commits into
DeleteResourceGroup and ResendReadyTasks.#366Conversation
# Conflicts: # components/spider-proto-rust/src/generated/storage.rs # components/spider-proto/storage/storage.proto # components/spider-scheduler/src/storage_client/grpc.rs
…status_to_error convention
…tus_to_error convention
…der into storage-grpc-migration
…add-missing-services
…o storage-grpc-services
… add-missing-services
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds DeleteResourceGroup end to end across proto, storage, gRPC, and client layers. It also implements previously stubbed storage RPCs for polling, resource-group, execution-manager, scheduler-registration, and session flows, and wires scheduler resend_ready_tasks support through the scheduler client. ChangesStorage RPC Implementations and Resource Group Deletion
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
components/spider-storage/src/grpc.rs (1)
1303-1326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the returned
task_idin the ready-task test.
build_ready_tasksnow owns the lane-specific task-id conversion contract, but this test only checks resource group and job IDs. Add an assertion forTaskId::Index(TASK_INDEX)so regressions in the protobuf task-id mapping are caught.🤖 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/grpc.rs` around lines 1303 - 1326, The ready-task test in poll_ready_tasks_returns_entries is missing coverage for the task-id mapping returned by build_ready_tasks. Update the assertions to verify the first task’s task_id equals TaskId::Index(TASK_INDEX) in addition to the existing resource_group_id and job_id checks, so regressions in the protobuf task-id conversion are caught.
🤖 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/grpc.rs`:
- Around line 254-263: The
`StorageServerError::Cache(CacheError::Internal(InternalError::ReadyQueueChannelClosed))`
match arm in `grpc.rs` only returns an internal status and skips the
service-cancellation path. Update this branch so `ReadyQueueChannelClosed` is
handled the same way as the broader broken-queue failure path, triggering
cancellation/restart logic instead of just logging and returning
`Status::internal`. Keep the existing `StorageServerError`/`CacheError` match
structure but ensure the closed inbound queue causes the service to stop
accepting work.
In `@components/spider-storage/src/state/service.rs`:
- Around line 551-562: Serialize resource-group deletion with in-flight job
registration so the DB and cache updates stay linearizable for the same resource
group. Update `delete_resource_group` and `register_job` to use the same
per-resource-group exclusion or equivalent guard around the
`self.inner.db.delete`, `self.inner.job_cache.remove_by_resource_group`, and JCB
insertion path. This prevents `register_job` from recreating a cached job after
a group has already been deleted.
- Around line 557-562: Deletion in the service path currently removes the DB
rows and job cache via inner.db.delete and
inner.job_cache.remove_by_resource_group, but it leaves already-queued ready
work eligible for polling. Update the deletion flow in the resource-group delete
logic to also purge or invalidate any queued entries associated with that
resource group, or make the poll path filter queued items against live cache
state before returning them. Use the deletion code around inner.db.delete and
remove_by_resource_group as the place to ensure no stale ready work from deleted
jobs can be dispatched.
---
Nitpick comments:
In `@components/spider-storage/src/grpc.rs`:
- Around line 1303-1326: The ready-task test in poll_ready_tasks_returns_entries
is missing coverage for the task-id mapping returned by build_ready_tasks.
Update the assertions to verify the first task’s task_id equals
TaskId::Index(TASK_INDEX) in addition to the existing resource_group_id and
job_id checks, so regressions in the protobuf task-id conversion are caught.
🪄 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: 91369f7f-6583-467a-8682-9260935ee5c4
⛔ Files ignored due to path filters (1)
components/spider-proto-rust/src/generated/storage.rsis excluded by!**/generated/**
📒 Files selected for processing (15)
components/spider-proto-rust/src/lib.rscomponents/spider-proto-rust/src/scheduler_registration.rscomponents/spider-proto-rust/src/unpack/storage.rscomponents/spider-proto/storage/storage.protocomponents/spider-scheduler/src/core_impl/round_robin/tests.rscomponents/spider-scheduler/src/error.rscomponents/spider-scheduler/src/storage_client/grpc.rscomponents/spider-scheduler/src/storage_client/mod.rscomponents/spider-storage/src/cache/job.rscomponents/spider-storage/src/db/mariadb.rscomponents/spider-storage/src/grpc.rscomponents/spider-storage/src/state.rscomponents/spider-storage/src/state/job_cache.rscomponents/spider-storage/src/state/service.rscomponents/spider-storage/tests/mariadb_test.rs
💤 Files with no reviewable changes (1)
- components/spider-scheduler/src/error.rs
| StorageServerError::Cache(CacheError::Internal( | ||
| InternalError::ReadyQueueChannelClosed, | ||
| )) => { | ||
| tracing::warn!( | ||
| service = SERVICE_NAME, | ||
| tag, | ||
| "Inbound queue channel is closed." | ||
| ); | ||
| Status::internal("inbound queue is closed") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cancel the service when the inbound queue channel is closed.
This branch shadows the broader CacheError::Internal(e) arm, so ReadyQueueChannelClosed returns INTERNAL but never triggers the cancellation/restart path. Since the queue can no longer yield entries, keeping the service alive leaves schedulers repeatedly hitting a permanently broken inbound queue.
Proposed fix
StorageServerError::Cache(CacheError::Internal(
InternalError::ReadyQueueChannelClosed,
)) => {
tracing::warn!(
service = SERVICE_NAME,
tag,
"Inbound queue channel is closed."
);
+ self.cancellation_token.cancel();
Status::internal("inbound queue is closed")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| StorageServerError::Cache(CacheError::Internal( | |
| InternalError::ReadyQueueChannelClosed, | |
| )) => { | |
| tracing::warn!( | |
| service = SERVICE_NAME, | |
| tag, | |
| "Inbound queue channel is closed." | |
| ); | |
| Status::internal("inbound queue is closed") | |
| } | |
| StorageServerError::Cache(CacheError::Internal( | |
| InternalError::ReadyQueueChannelClosed, | |
| )) => { | |
| tracing::warn!( | |
| service = SERVICE_NAME, | |
| tag, | |
| "Inbound queue channel is closed." | |
| ); | |
| self.cancellation_token.cancel(); | |
| Status::internal("inbound queue is closed") | |
| } |
🤖 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/grpc.rs` around lines 254 - 263, The
`StorageServerError::Cache(CacheError::Internal(InternalError::ReadyQueueChannelClosed))`
match arm in `grpc.rs` only returns an internal status and skips the
service-cancellation path. Update this branch so `ReadyQueueChannelClosed` is
handled the same way as the broader broken-queue failure path, triggering
cancellation/restart logic instead of just logging and returning
`Status::internal`. Keep the existing `StorageServerError`/`CacheError` match
structure but ensure the closed inbound queue causes the service to stop
accepting work.
| pub async fn delete_resource_group( | ||
| &self, | ||
| resource_group_id: ResourceGroupId, | ||
| password: &[u8], | ||
| ) -> Result<(), StorageServerError> { | ||
| self.inner.db.verify(resource_group_id, password).await?; | ||
| self.inner.db.delete(resource_group_id).await?; | ||
| let evicted_jobs = self | ||
| .inner | ||
| .job_cache | ||
| .remove_by_resource_group(resource_group_id) | ||
| .await; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize resource-group deletion against in-flight job registration.
register_job persists the job before it builds and inserts the JCB, while this path deletes the DB rows before it evicts cached JCBs. If both run concurrently for the same resource group, delete can remove the DB state and then the register path can still insert a fresh JCB afterwards, leaving a ghost cached job for a deleted group. Please put register/delete behind the same per-resource-group exclusion or otherwise make the DB+cache update linearizable.
🤖 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 551 - 562,
Serialize resource-group deletion with in-flight job registration so the DB and
cache updates stay linearizable for the same resource group. Update
`delete_resource_group` and `register_job` to use the same per-resource-group
exclusion or equivalent guard around the `self.inner.db.delete`,
`self.inner.job_cache.remove_by_resource_group`, and JCB insertion path. This
prevents `register_job` from recreating a cached job after a group has already
been deleted.
| self.inner.db.delete(resource_group_id).await?; | ||
| let evicted_jobs = self | ||
| .inner | ||
| .job_cache | ||
| .remove_by_resource_group(resource_group_id) | ||
| .await; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Delete does not invalidate already-queued ready work.
This only removes DB rows and cached JCBs. Any entries already sitting in the ready queues will still be returned by the poll RPCs, so the scheduler can keep receiving work for jobs that no longer exist. Please purge or invalidate queued entries as part of deletion, or filter them against live cache state before returning them.
🤖 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 557 - 562,
Deletion in the service path currently removes the DB rows and job cache via
inner.db.delete and inner.job_cache.remove_by_resource_group, but it leaves
already-queued ready work eligible for polling. Update the deletion flow in the
resource-group delete logic to also purge or invalidate any queued entries
associated with that resource group, or make the poll path filter queued items
against live cache state before returning them. Use the deletion code around
inner.db.delete and remove_by_resource_group as the place to ensure no stale
ready work from deleted jobs can be dispatched.
…missing-services # Conflicts: # components/spider-scheduler/src/storage_client/grpc.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-scheduler/src/runtime.rs`:
- Around line 66-87: The shutdown timeout path in SchedulerRuntime::stop
currently drops the core JoinHandle when tokio::time::timeout expires, leaving
the scheduler core detached and still running. Update the stop logic to keep a
borrow of self.core_join_handle, call abort() on it before returning the
SchedulerRuntimeError::Stopping timeout error, and then continue handling the
join result in stop so the core task is forcibly stopped on timeout.
🪄 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: b1382774-b52b-408f-9067-818b260f23ea
📒 Files selected for processing (7)
components/spider-client/src/client.rscomponents/spider-client/src/grpc/resource_group.rscomponents/spider-scheduler/src/core_impl/round_robin/tests.rscomponents/spider-scheduler/src/error.rscomponents/spider-scheduler/src/runtime.rscomponents/spider-scheduler/src/storage_client/grpc.rscomponents/spider-scheduler/src/storage_client/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- components/spider-scheduler/src/core_impl/round_robin/tests.rs
- components/spider-scheduler/src/error.rs
- components/spider-scheduler/src/storage_client/mod.rs
- components/spider-scheduler/src/storage_client/grpc.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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-scheduler/src/runtime.rs`:
- Around line 66-87: The shutdown timeout path in SchedulerRuntime::stop
currently drops the core JoinHandle when tokio::time::timeout expires, leaving
the scheduler core detached and still running. Update the stop logic to keep a
borrow of self.core_join_handle, call abort() on it before returning the
SchedulerRuntimeError::Stopping timeout error, and then continue handling the
join result in stop so the core task is forcibly stopped on timeout.
🪄 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: b1382774-b52b-408f-9067-818b260f23ea
📒 Files selected for processing (7)
components/spider-client/src/client.rscomponents/spider-client/src/grpc/resource_group.rscomponents/spider-scheduler/src/core_impl/round_robin/tests.rscomponents/spider-scheduler/src/error.rscomponents/spider-scheduler/src/runtime.rscomponents/spider-scheduler/src/storage_client/grpc.rscomponents/spider-scheduler/src/storage_client/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- components/spider-scheduler/src/core_impl/round_robin/tests.rs
- components/spider-scheduler/src/error.rs
- components/spider-scheduler/src/storage_client/mod.rs
- components/spider-scheduler/src/storage_client/grpc.rs
🛑 Comments failed to post (1)
components/spider-scheduler/src/runtime.rs (1)
66-87: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '## runtime.rs outline\n' ast-grep outline components/spider-scheduler/src/runtime.rs --view expanded || true printf '\n## runtime.rs relevant lines\n' nl -ba components/spider-scheduler/src/runtime.rs | sed -n '1,180p' printf '\n## search for stop usage/docs/tests\n' rg -n "SchedulerRuntime|stop_timeout|cancelled and joined|core_join_handle|abort\(" components/spider-scheduler -S || trueRepository: y-scope/spider
Length of output: 1453
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' import pathlib p = pathlib.Path('components/spider-scheduler/src/runtime.rs') print(p.exists(), p.stat().st_size if p.exists() else 'missing') PYRepository: y-scope/spider
Length of output: 163
🌐 Web query:
Tokio JoinHandle dropped detaches task timeout &mut JoinHandle cancel-safe docs💡 Result:
In Tokio, dropping a JoinHandle detaches the associated task [1][2]. This means the task continues to run in the background, but the owner of the JoinHandle loses the ability to join on it or retrieve its output [2][3]. Because the task is no longer tracked by that handle, any return value or panic result is effectively lost [2][4]. Regarding cancel safety, the &mut JoinHandle type is explicitly documented as cancel-safe [2][3]. If an &mut JoinHandle is used as an event in a tokio::select! branch and another branch completes first, the operation is guaranteed to be safe and the task's output will not be lost [4][5]. It is important to distinguish between dropping a JoinHandle and cancelling a task: 1. Dropping a JoinHandle detaches the task, allowing it to continue running to completion independently [2][4]. 2. To actively stop a task, you must use the JoinHandle::abort method or an AbortHandle [5][6]. Awaiting an aborted task will typically result in a cancelled JoinError [5][6]. For scenarios requiring task cancellation that respects the task's own lifecycle (e.g., stopping a loop), users typically combine aborts with cooperative cancellation patterns or utilize tools like JoinSet for managing multiple tasks [6].
Citations:
- 1: https://docs.rs/tokio/latest/tokio/task/struct.JoinHandle.html
- 2: https://docs.rs/tokio/latest/src/tokio/runtime/task/join.rs.html
- 3: https://github.com/tokio-rs/tokio/blob/master/tokio/src/runtime/task/join.rs
- 4: https://docs.rs/tokio/1.36.0/tokio/task/struct.JoinHandle.html
- 5: https://docs.rs/tokio/1.48.0/tokio/task/struct.JoinHandle.html
- 6: https://docs.rs/tokio/latest/tokio/task/
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- runtime.rs (around stop) ---' nl -ba components/spider-scheduler/src/runtime.rs | sed -n '1,140p' echo echo '--- surrounding docs/comments in runtime.rs ---' rg -n "cancelled and joined|stop\(|shutdown|timeout" components/spider-scheduler/src/runtime.rs -n -S || true echo echo '--- scheduler runtime references ---' rg -n "SchedulerRuntime|core_join_handle|stop_timeout" components/spider-scheduler -S || trueRepository: y-scope/spider
Length of output: 225
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path path = Path('components/spider-scheduler/src/runtime.rs') lines = path.read_text().splitlines() def show(start, end, title): print(f"--- {title} ({start}-{end}) ---") for i in range(start-1, min(end, len(lines))): print(f"{i+1:4d}: {lines[i]}") print() show(60, 95, "stop()") show(280, 350, "tests around stop_runtime_on_*") PYRepository: y-scope/spider
Length of output: 4573
Abort the core task on shutdown timeout. When
timeout(self.stop_timeout, self.core_join_handle)elapses, theJoinHandleis dropped and detached, so the scheduler core keeps running in the background with no way to stop or observe it. Borrow the handle and callabort()before returning the timeout error.🤖 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-scheduler/src/runtime.rs` around lines 66 - 87, The shutdown timeout path in SchedulerRuntime::stop currently drops the core JoinHandle when tokio::time::timeout expires, leaving the scheduler core detached and still running. Update the stop logic to keep a borrow of self.core_join_handle, call abort() on it before returning the SchedulerRuntimeError::Stopping timeout error, and then continue handling the join result in stop so the core task is forcibly stopped on timeout.
|
Closed as include change of delete resource group that should not be implemented now. Will open smaller change in another PR. |
2 participants
Description
Note
This PR depends on #364
This PR:
ResendReadyTasksRPC to a dedicated scheduler-facing service #352 by addingResendReadyTaskstoInboundQueueService.DeleteResourceGroupimplement in db, cache and service layer.Checklist
breaking change.
Validation performed
Summary by CodeRabbit