feat(spider-execution-manager): Add liveness actor with session ID tracker; Refactor integration tests to extract common helpers into test-utils. - #328
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a forward-only SessionTracker, LivenessClient types and a spawnable heartbeat actor, a subprocess executor test harness and MockLiveness, end-to-end tests for executor and process pool, and workspace/Cargo manifest updates to include new crates. ChangesExecution Manager Liveness and Test Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/spider-execution-manager/src/process_pool.rs (1)
163-173: ⚡ Quick winSerialize the request before taking the executor mutex.
build_request()only does local encoding, but it currently runs after Line 163 inside the same mutex scope that guards the child process. Moving it ahead of the lock keeps large input serializations and local encoding failures from extending head-of-line blocking on the single executor.🤖 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-execution-manager/src/process_pool.rs` around lines 163 - 173, Call build_request(request) before acquiring the executor mutex so local serialization/encoding work does not hold the child-process lock; specifically, move the build_request(request)? invocation out of the critical section that surrounds self.handle.lock().await and handle.run(...).await, so you compute frame_request (via build_request) first, then acquire the mutex (self.handle.lock().await), get handle (handle_guard.as_mut().ok_or(InternalError::NotRunning)?), log and call handle.run(frame_request, hard_timeout).await.
🤖 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-execution-manager/src/process_pool.rs`:
- Around line 278-325: The current run function (async fn run(&mut self,
request: Request, hard_timeout: Duration) -> Outcome) only starts the
tokio::select! timeout after awaiting self.requests.send(...).await, so a
blocked send can prevent the hard_timeout from ever firing; change run to cover
the full send+receive window by moving the timeout to wrap both send and
response handling (e.g., use tokio::time::timeout(hard_timeout, async {
self.requests.send(Bytes::from(bytes)).await?; self.responses.next().await }) or
include the send future in the same tokio::select! alongside responses and the
sleep), ensuring the send call (self.requests.send) is protected by hard_timeout
and still returns Outcome::Timeout on expiration.
In `@components/spider-task-executor/src/bin/spider_task_executor.rs`:
- Around line 73-77: The package identifier is used directly to build a
filesystem path in the else branch (the block that calls manager.get(package)
and manager.load(&path)), allowing path traversal; before joining
pkg_dir.join(package) validate/sanitize `package` (e.g., reject empty strings,
any path separators like '/' or '\\', any ".." components, and allow only a safe
whitelist such as [A-Za-z0-9_-]); if validation fails return an error instead of
constructing the path; apply this check where you construct `path` and before
calling `manager.load` so only safe package names are used.
---
Nitpick comments:
In `@components/spider-execution-manager/src/process_pool.rs`:
- Around line 163-173: Call build_request(request) before acquiring the executor
mutex so local serialization/encoding work does not hold the child-process lock;
specifically, move the build_request(request)? invocation out of the critical
section that surrounds self.handle.lock().await and handle.run(...).await, so
you compute frame_request (via build_request) first, then acquire the mutex
(self.handle.lock().await), get handle
(handle_guard.as_mut().ok_or(InternalError::NotRunning)?), log and call
handle.run(frame_request, hard_timeout).await.
🪄 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: fed459f0-c414-490d-9e84-a0fc3c793720
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
Cargo.tomlcomponents/spider-core/Cargo.tomlcomponents/spider-core/src/lib.rscomponents/spider-core/src/session.rscomponents/spider-execution-manager/Cargo.tomlcomponents/spider-execution-manager/src/client.rscomponents/spider-execution-manager/src/client/liveness.rscomponents/spider-execution-manager/src/client/scheduler.rscomponents/spider-execution-manager/src/client/storage.rscomponents/spider-execution-manager/src/lib.rscomponents/spider-execution-manager/src/liveness.rscomponents/spider-execution-manager/src/process_pool.rscomponents/spider-task-executor/Cargo.tomlcomponents/spider-task-executor/src/bin/spider_task_executor.rscomponents/spider-task-executor/src/error.rscomponents/spider-task-executor/src/lib.rscomponents/spider-task-executor/src/manager.rscomponents/spider-task-executor/src/protocol.rstaskfiles/test.yamltests/huntsman/integration-test-tasks/Cargo.tomltests/huntsman/integration-test-tasks/src/lib.rstests/huntsman/task-executor/Cargo.tomltests/huntsman/task-executor/src/lib.rstests/huntsman/task-executor/tests/overhead_instrument.rstests/huntsman/task-executor/tests/test_executor.rstests/huntsman/task-executor/tests/test_process_pool.rstests/huntsman/tdl-integration/tests/complex.rs
Introduces tests/huntsman/test-utils, a shared support crate that consolidates the executor subprocess harness, TDL wire-payload helpers, and in-process mock client implementations (scheduler/storage/liveness) so multiple integration suites can reuse them. task-executor-tests is converted to a thin test-only crate that depends on it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The scheduler and storage mocks are only needed by the execution-manager runtime tests, which land on a later branch. Drop them here so this branch's test-utils exposes just the liveness client mock that the liveness work uses. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
LinZhihao-723
changed the title
test-utils.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/huntsman/test-utils/src/mock.rs (1)
162-170: ⚡ Quick winValidate
em_idin the mock heartbeat path.The mock currently accepts any manager ID, which can hide ID-propagation regressions in tests. Fail fast on mismatched IDs to keep the mock aligned with the client contract.
Proposed fix
async fn heartbeat( &self, - _em_id: ExecutionManagerId, + em_id: ExecutionManagerId, ) -> Result<SessionId, LivenessResponseError> { + if em_id != self.inner.em_id { + return Err(LivenessResponseError::IllegalId); + } self.inner.heartbeat_count.fetch_add(1, Ordering::Relaxed); self.inner.heartbeat_notify.notify_waiters(); let queued = lock(&self.inner.heartbeat_responses).pop_front(); queued.unwrap_or_else(|| Ok(self.inner.default_session.load(Ordering::Relaxed))) }🤖 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 `@tests/huntsman/test-utils/src/mock.rs` around lines 162 - 170, In heartbeat, stop ignoring the ExecutionManagerId parameter and validate it against the mock's expected ID (e.g., compare the incoming em_id to self.inner.expected_em_id or self.inner.em_id), returning an appropriate LivenessResponseError on mismatch and only proceeding with heartbeat_count/notify/pop_front when they match; also remove the underscore from the parameter name so the value is used.
🤖 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 `@tests/huntsman/test-utils/src/mock.rs`:
- Around line 162-170: In heartbeat, stop ignoring the ExecutionManagerId
parameter and validate it against the mock's expected ID (e.g., compare the
incoming em_id to self.inner.expected_em_id or self.inner.em_id), returning an
appropriate LivenessResponseError on mismatch and only proceeding with
heartbeat_count/notify/pop_front when they match; also remove the underscore
from the parameter name so the value is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a30c9f45-0de5-4f77-b528-cc277dc61535
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomltests/huntsman/task-executor/Cargo.tomltests/huntsman/task-executor/src/lib.rstests/huntsman/task-executor/tests/overhead_instrument.rstests/huntsman/task-executor/tests/test_executor.rstests/huntsman/task-executor/tests/test_process_pool.rstests/huntsman/test-utils/Cargo.tomltests/huntsman/test-utils/src/executor.rstests/huntsman/test-utils/src/lib.rstests/huntsman/test-utils/src/mock.rs
✅ Files skipped from review due to trivial changes (3)
- tests/huntsman/test-utils/Cargo.toml
- tests/huntsman/task-executor/src/lib.rs
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/huntsman/task-executor/Cargo.toml
- tests/huntsman/task-executor/tests/overhead_instrument.rs
- tests/huntsman/task-executor/tests/test_executor.rs
sitaowang1998
requested changes
Jun 1, 2026
Comment on lines
+134
to
+139
| if self.session_tracker.try_advance(session_id) { | ||
| tracing::info!( | ||
| from = previous, | ||
| to = session_id, | ||
| "Session advanced by heartbeat." | ||
| ); |
Collaborator
There was a problem hiding this comment.
I think apart from bumping the id internally, we need a way to signal that heartbeat has been updated. Otherwise the user have to poll the session id.
Member
Author
There was a problem hiding this comment.
I'm not sure if I got what u mean.
Which component do we need to signal for?
Collaborator
There was a problem hiding this comment.
Does execution manager need to stop running tasks when the session id changes? Or should we just let them run and reject them when they finish?
Member
Author
There was a problem hiding this comment.
Similar to the process pool design: to keep things simple, we should just let it run and reject when they finish.
Adding a signal protocol is non-trivial, and the benefit is limited: a session bump should be rare in reality.
sitaowang1998
approved these changes
Jun 1, 2026
This was referenced Jun 6, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR depends on #327.
This PR adds the liveness actor and the shared session-tracker primitive that the rest of the execution-manager runtime will plug into.
spider_core::session::SessionTracker— a forward-only counter wrappingArc<AtomicU64>for the runtime's view of storage's current session id. Cloneable, withcurrent()/try_advance()semantics: writers always move the stored value forward via a CAS loop, and reads observe the latest committed value. Lives inspider-coreso the future scheduler service can reuse the same primitive.spider_execution_manager::liveness— a tokio-actor driving the periodic storage heartbeat:tokio::time::intervalticks everyheartbeat_interval; each tick callsLivenessClient::heartbeat(em_id)and forwards storage's reply to the sharedSessionTracker. The interval usesMissedTickBehavior::Skipas a defensive guard against starvation-induced burst-replay.LivenessCommand::Refreshlets the rest of the runtime ask for an off-schedule heartbeat. The command does not advance the tracker directly — storage's heartbeat reply is the only source of truth for the current session id, so the actor always re-checks rather than trusting the caller's observation.interval.reset()runs at the end of every heartbeat call, so aRefresh-triggered heartbeat naturally rate-limits the next scheduled tick. Two consecutive heartbeats are never closer together thanheartbeat_interval.MarkedDead/IllegalId) cancel the actor'sCancellationToken, which the rest of the runtime will observe to tear everything down.This PR also creates
test-utilsundertests/huntsmanto collect all reusable test helpers.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores