Skip to content

feat(spider-storage): Add resource group, execution manager and scheduler methods in ServiceState.#323

Merged
sitaowang1998 merged 46 commits into
y-scope:storage-service-devfrom
sitaowang1998:server-state
May 13, 2026
Merged

feat(spider-storage): Add resource group, execution manager and scheduler methods in ServiceState.#323
sitaowang1998 merged 46 commits into
y-scope:storage-service-devfrom
sitaowang1998:server-state

Conversation

@sitaowang1998
Copy link
Copy Markdown
Collaborator

@sitaowang1998 sitaowang1998 commented May 12, 2026

Description

As title.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  • New unit tests added.
  • GitHub workflows pass.

Summary by CodeRabbit

Release Notes

  • New Features

    • Resource group lifecycle management with secure password verification
    • Ready-queue task polling with configurable timeouts and batch retrieval
    • Automated deletion of expired terminated jobs
    • Execution manager registration and heartbeat tracking
  • Tests

    • Improved mock database with stateful resource group and execution manager tracking

Review Change Stack

Resolve conflict in job_cache.rs test imports by taking main's version
which includes ValidatedJobSubmission and additional cache types.
… ValidatedJobSubmission, session tracking, resource group/execution manager methods, and stale session tests
…llback reads, commit/cleanup task instance methods
@sitaowang1998 sitaowang1998 requested a review from a team as a code owner May 12, 2026 23:25
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 12, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: d5f09d03-331d-4254-9b4a-982ec0d95168

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
components/spider-storage/src/state/service.rs (1)

606-619: ⚡ Quick win

Heartbeat logs at info may be too noisy for a hot path.

update_execution_manager_heartbeat is invoked at the heartbeat cadence per execution manager, so logging at info for every successful call will produce a steady stream of log entries proportional to fleet size. Consider lowering to debug (or trace) so production info-level output stays focused on lifecycle/state-change events. The other new methods in this file already match that intent (e.g. verify_resource_group does not log on success, poll_ready_tasks does not log per poll).

♻️ Proposed change
         self.inner
             .db
             .update_execution_manager_heartbeat(execution_manager_id)
             .await?;
-        tracing::info!(
+        tracing::debug!(
             em_id = ? execution_manager_id,
             "Execution manager heartbeat updated.",
         );
         Ok(())
🤖 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 606 - 619, The
heartbeat log in update_execution_manager_heartbeat is too noisy at info level;
change the tracing::info! call in that function to a lower level (e.g.,
tracing::debug! or tracing::trace!) so successful per-heartbeat calls don't
flood production logs, preserving the em_id field and the message text; do not
alter any other behavior in update_execution_manager_heartbeat and keep
consistency with other methods like verify_resource_group and poll_ready_tasks.
🤖 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/service.rs`:
- Around line 1327-1338: The test poll_ready_tasks_returns_empty_when_no_tasks
currently drops the sender returned by create_test_service_with_real_ready_queue
by binding it to `_`, causing the async_channel to close and recv_batch to
return InternalError::ReadyQueueChannelClosed; keep the sender alive for the
test by binding it to a named variable (e.g., `_sender`) instead of `_` so the
sender lives for the test duration and the assertion can run, mirroring the
pattern used in recv_returns_empty_when_wait_elapses.

---

Nitpick comments:
In `@components/spider-storage/src/state/service.rs`:
- Around line 606-619: The heartbeat log in update_execution_manager_heartbeat
is too noisy at info level; change the tracing::info! call in that function to a
lower level (e.g., tracing::debug! or tracing::trace!) so successful
per-heartbeat calls don't flood production logs, preserving the em_id field and
the message text; do not alter any other behavior in
update_execution_manager_heartbeat and keep consistency with other methods like
verify_resource_group and poll_ready_tasks.
🪄 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: e48f7b68-efae-493d-8664-de0aa06fbe6d

📥 Commits

Reviewing files that changed from the base of the PR and between 3cff97e and 794ab66.

📒 Files selected for processing (2)
  • components/spider-storage/src/state/service.rs
  • components/spider-storage/src/state/test_utils.rs

Comment thread components/spider-storage/src/state/service.rs
Copy link
Copy Markdown
Member

@LinZhihao-723 LinZhihao-723 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed service APIs.

Comment thread components/spider-storage/src/state/service.rs Outdated
Comment thread components/spider-storage/src/state/service.rs Outdated
Comment thread components/spider-storage/src/state/service.rs Outdated
Copy link
Copy Markdown
Member

@LinZhihao-723 LinZhihao-723 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm done with my editing. Please check.

@sitaowang1998 sitaowang1998 merged commit dfb2170 into y-scope:storage-service-dev May 13, 2026
11 checks passed
@sitaowang1998 sitaowang1998 deleted the server-state branch May 13, 2026 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants