fix(pb): dont immediately reallocate actors on runner stopping #3413

MasterPtato · 2025-11-07T22:49:57Z

No description provided.

vercel · 2025-11-07T22:50:00Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Error			Nov 13, 2025 10:42pm

3 Skipped Deployments

Project	Deployment	Preview	Updated (UTC)
rivet-cloud	Ignored		Nov 13, 2025 10:42pm
rivet-inspector	Ignored	Preview	Nov 13, 2025 10:42pm
rivet-site	Ignored	Preview	Nov 13, 2025 10:42pm

MasterPtato · 2025-11-07T22:50:18Z

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

This stack of pull requests is managed by Graphite. Learn more about stacking.

claude · 2025-11-07T22:51:31Z

PR Review: Fix actor reallocation on runner stopping

Summary

This PR introduces a new GoingAway signal to gracefully handle actors when a runner is stopping, preventing immediate reallocation and allowing for controlled shutdown. This is a significant improvement to the actor lifecycle management.

Positive Changes

Architecture Improvements

✅ New GoingAway signal: Introduces a clear separation between actors being lost (unexpected) and runners going away (planned shutdown). This is a good architectural decision that makes the state transitions more explicit.

✅ Better state tracking: Adding stopping and going_away flags to LifecycleState provides better visibility into the actor's current state transitions.

✅ Cleaner separation of concerns: The refactoring of handle_stopped to use StoppedVariant enum and StoppedResult improves code clarity and makes the different stop scenarios more explicit.

Code Quality

✅ Removed unnecessary kill parameter: The removal of the kill parameter from the destroy workflow and LifecycleResult simplifies the interface and removes a confusing boolean flag.

✅ Better logging: The debug log in handle_stopped now logs the full variant which provides better observability.

✅ Idempotency improvements: Added guards to prevent duplicate signal sends (e.g., checking !state.stopping before sending stop signals).

Issues and Concerns

1. Potential Race Condition in `GoingAway` Handler (High Priority)

Location: actor/mod.rs:471-477

Main::GoingAway(sig) => {
    // Ignore signals for previous generations
    if sig.generation != state.generation {
        return Ok(Loop::Continue);
    }

    if sig.reset_rescheduling {
        state.reschedule_state = Default::default();
    }

    if !state.going_away {
        let Some(runner_workflow_id) = state.runner_workflow_id else {
            return Ok(Loop::Continue);
        };

Issue: The check if !state.going_away happens AFTER checking runner_workflow_id. However, if the actor is not allocated (no runner_workflow_id), we return early but don't set going_away = true. This means the signal could be processed multiple times if the actor gets reallocated.

Recommendation: Set state.going_away = true before the runner_workflow_id check, or handle this case more explicitly.

2. Missing State Transitions (Medium Priority)

Location: actor/mod.rs:333-361

The code sets state.stopping = true when handling ActorIntentStop, but I don't see where state.going_away transitions back to false in all code paths. While it's cleared in handle_stopped:602-604, there might be edge cases where the state could get stuck.

Recommendation: Add explicit state machine documentation or assertions to ensure all transitions are covered.

3. Generation Mismatch Edge Case (Low Priority)

Location: actor/mod.rs:640-654

// We don't know the state of the previous generation of this actor actor if it becomes lost, send stop
// command in case it ended up allocating
if let (StoppedVariant::Lost { .. }, Some(old_runner_workflow_id)) =
    (&variant, old_runner_workflow_id)
{
    ctx.signal(crate::workflows::runner::Command {
        inner: protocol::Command::CommandStopActor(protocol::CommandStopActor {
            actor_id: input.actor_id.to_string(),
            generation: state.generation,
        }),
    })

Issue: The comment says "previous generation" but the code uses state.generation which is the current generation. If we're stopping the old generation, should we use state.generation - 1?

Recommendation: Verify if this is correct or update the comment to match the code's intent.

4. Typo in Comment (Trivial)

Location: actor/mod.rs:640

// We don't know the state of the previous generation of this actor actor if it becomes lost

Issue: Double "actor" in the comment.

Recommendation: Change to "of this actor if it becomes lost".

Performance Considerations

✅ No significant performance concerns: The changes don't introduce expensive operations or blocking calls.

⚠️ State size increase: Adding stopping and going_away booleans increases the serialized state size slightly, but this is negligible.

Security Concerns

✅ No security issues identified: The changes don't introduce new attack vectors or expose sensitive data.

Test Coverage

⚠️ Missing tests: This PR doesn't include tests for:

The new GoingAway signal handling
State transitions between stopping, going_away, and normal states
Edge cases around generation mismatches
Race conditions when receiving multiple signals

Recommendation: Add integration tests that:

Verify actors gracefully stop when runner goes away
Test that actors aren't immediately reallocated during runner shutdown
Verify state transitions are handled correctly
Test generation mismatch scenarios

Code Style

✅ Follows CLAUDE.md conventions:

Uses structured logging with tracing
Lowercase log messages
Uses ? for error formatting in logs
Follows workspace dependency patterns
Uses hard tabs (as specified in rustfmt.toml)

Summary

This is a solid improvement to the actor lifecycle management system. The new GoingAway signal provides better control over actor shutdown during runner maintenance. However, there are a few edge cases and potential race conditions that should be addressed before merging.

Recommendations:

Fix the GoingAway state handling to prevent race conditions
Add comprehensive tests for the new signal and state transitions
Clarify generation handling in the handle_stopped comment/code
Consider documenting the state machine more explicitly (as noted in the TODO comment)

Overall: Approve with suggested changes 👍

🤖 Generated with Claude Code

pkg-pr-new · 2025-11-07T22:52:24Z

More templates

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/actor@3413

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3413

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/core@3413

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3413

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3413

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3413

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3413

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3413

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3413

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3413

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3413

commit: 87f1c1a

claude · 2025-11-10T22:51:40Z

PR Review: fix(pb): dont immediately reallocate actors on runner stopping

Summary

This PR refactors the actor lifecycle management in Pegboard to introduce a new GoingAway signal that prevents immediate reallocation of actors when a runner is stopping. This is a significant improvement to the shutdown behavior.

Code Quality & Best Practices

Strengths:

✅ Good separation of concerns with the new StoppedVariant enum that distinguishes between normal stops and lost actors
✅ The StoppedResult enum improves code clarity by making return intent explicit
✅ Proper use of structured logging patterns (tracing::debug!(?variant, "actor stopped"))
✅ Comments added to explain gc_timeout_ts purpose in runtime.rs:43-45
✅ Simplification of LifecycleResult by removing the kill field reduces complexity

Concerns:

⚠️ The TODO comment at runtime.rs:20-21 suggests the current state management approach needs redesign. Consider tracking this as a technical debt item
⚠️ The LifecycleState struct now has 3 boolean flags (sleeping, stopping, going_away) that represent state. This could be error-prone - consider using a proper state machine enum in the future
⚠️ The #[serde(default)] attributes on new fields (stopping, going_away) are good for backward compatibility, but ensure migration is tested

Potential Bugs & Issues

Critical:

🔴 Race condition potential: In mod.rs:445-474, the GoingAway signal handler checks if sig.generation != state.generation and returns early, but there's a TOCTOU (time-of-check-time-of-use) gap between checking and using state.runner_workflow_id. Consider holding state more carefully.

Medium Priority:

🟡 In handle_stopped (mod.rs:566-731), the function clears state.going_away and state.stopping at mod.rs:603-604, but if an error occurs during rescheduling, these flags remain cleared even though the operation failed. Consider using a scope guard or only clearing on success.
🟡 The destroy.rs file removes the kill parameter entirely, but the PR doesn't show if there are callers outside this module that might break. Verify all call sites are updated.

Minor:

🟠 In mod.rs:478-482, the Destroy handler sends a stop command but doesn't wait for acknowledgment before returning. Is this intentional? Add a comment explaining the fire-and-forget semantics.

Logic & Correctness

GoingAway Signal Logic:
The new signal is well-designed for the use case:

It sets gc_timeout_ts to allow graceful shutdown
It sends CommandStopActor to the runner
It prevents duplicate stop commands with the going_away flag

However, at mod.rs:415-422, when a runner is stopping, ALL actors get the GoingAway signal. Consider:

What if some actors are already stopping or sleeping? The code doesn't check current state before sending signals.
Should there be batching or rate limiting if there are thousands of actors?

State Machine Clarity:
The refactoring improves state handling, but the interaction between sleeping, stopping, and going_away is not immediately obvious:

Can an actor be both sleeping and going_away?
Can an actor be both stopping and going_away?
Document these invariants or use a proper enum-based state machine

Performance Considerations

✅ Removing the runner_workflow_id from UpdateStateAndDbOutput reduces data transfer
✅ The handle_stopped refactoring consolidates logic paths, reducing code duplication
⚠️ In the runner stopping path (runner.rs:414-422), signals are sent to all actors in a loop. For large runner fleets, this could cause signal queue buildup. Consider using a message broadcast or batching.

Security Concerns

✅ No obvious security issues
✅ The generation checks prevent stale signals from affecting newer actor incarnations
ℹ️ Ensure the CommandStopActor signals are properly authenticated/authorized in the protocol layer (outside scope of this PR)

Test Coverage

Major Concern:

🔴 No test files found in the pegboard package. This is a significant refactoring of critical lifecycle management code without apparent test coverage.
The PR changes complex state machine logic that handles actor lifecycle transitions
Recommend adding integration tests that cover:
- Runner stopping with active actors
- Race conditions between GoingAway and actor state changes
- Generation mismatch scenarios
- The interaction between stopping, going_away, and sleeping states

Specific Code Issues

`mod.rs:637-640` - Unclear Logic

// We don't know the state of the previous generation of this actor actor if it becomes lost, send stop
// command in case it ended up allocating
if let (StoppedVariant::Lost { .. }, Some(old_runner_workflow_id)) =
    (&variant, old_runner_workflow_id)

Typo: "actor actor" → "actor"
This sends a stop command for the OLD generation. Is this safe if the old runner has already deallocated? Add a comment explaining why this is necessary.

`mod.rs:680-686` - Inconsistent Error Handling

match (input.crash_policy, failed) {
    (CrashPolicy::Restart, true) => {
        match runtime::reschedule_actor(ctx, &input, state, false).await? {
            // ...
            runtime::SpawnActorOutput::Sleep | runtime::SpawnActorOutput::Destroy => {
                return Ok(StoppedResult::Destroy);
            }
        }
    }

The comment says "Its not possible for SpawnActorOutput::Sleep to be returned here" but the code handles it anyway. Either remove the arm or update the comment.

`runtime.rs:32-34` - State Flag Proliferation

#[serde(default)]
pub stopping: bool,
#[serde(default)]
pub going_away: bool,

These flags indicate the state management is becoming complex. The TODO at line 20 acknowledges this. Consider prioritizing the refactor to use proper state enums.

Recommendations

Add Tests: Critical - add integration tests for the new GoingAway signal and state transitions
State Machine Refactor: Plan the mentioned refactor to use nested enums for state clarity
Documentation: Add module-level documentation explaining the actor lifecycle states and transitions
Fix Typo: Line 637 "actor actor"
Clarify Comment: Line 681-682 about unreachable code
Consider Batching: For runner shutdown with many actors, consider batch signal sending

Verdict

This is a good refactoring that improves the shutdown behavior by preventing immediate reallocation. The code quality is generally high and follows project conventions. However, the lack of test coverage for such critical logic is concerning, and the state management approach is acknowledged (via TODO) to need improvement.

Recommendation: Request tests before merging, but the core logic looks sound.

🤖 Generated with Claude Code

claude · 2025-11-10T22:51:46Z

Code Review: Fix Pegboard Actor Immediate Reallocation on Runner Stopping

Overview

This PR addresses an issue where actors were being immediately reallocated when their runner was stopping, which is undesirable behavior. The solution introduces a new GoingAway signal and state management to handle graceful shutdown scenarios separately from lost/failed actor scenarios.

Positive Changes ✅

1. Improved State Management
The addition of stopping and going_away boolean flags to LifecycleState (runtime.rs:32-34) provides clearer state tracking for different shutdown scenarios. This is a good architectural improvement.

2. Separation of Concerns
The new GoingAway signal distinguishes between:

Unexpected actor loss (requiring immediate reallocation)
Expected shutdown due to runner draining (no immediate reallocation needed)

This is conceptually sound and addresses the core issue.

3. Code Cleanup
Removing the kill parameter from the destroy workflow simplifies the API. The decision to send stop commands is now handled at the callsite, which is clearer.

4. Better Error Handling
The refactoring of handle_stopped() with StoppedVariant enum (mod.rs:569-573) improves type safety and makes the different stopping scenarios more explicit.

Concerns & Issues 🔍

1. State Management Complexity ⚠️
The addition of stopping and going_away flags increases complexity. Both flags are cleared together in handle_stopped() (mod.rs:602-603), which suggests they might be modeling the same concept differently. Consider:

Are there scenarios where stopping is true but going_away is false?
Should these be combined into a single enum state?
The TODO comment at runtime.rs:20-21 acknowledges this complexity issue

2. Idempotency Check Missing ⚠️
In mod.rs:480-486, the GoingAway signal handler checks !state.going_away before processing, which is good. However, the Lost signal handler (mod.rs:447-470) doesn't have a similar idempotency check. This could lead to duplicate processing if multiple Lost signals are received.

3. Race Condition Potential ⚠️
In the GoingAway handler (mod.rs:480-507), there's a TOCTOU (time-of-check-time-of-use) pattern:

if !state.going_away {
    let Some(runner_workflow_id) = state.runner_workflow_id else {
        return Ok(Loop::Continue);
    };
    // ... state modifications ...
}

If runner_workflow_id becomes None between the check and the signal send, this could cause issues. Consider restructuring to capture both checks atomically.

4. reset_rescheduling Parameter Removed ⚠️
The reset_rescheduling parameter was removed from reschedule_actor() (runtime.rs:629) but it's still accepted in the GoingAway and Lost signals. However, in Lost signal handling (mod.rs:456), reset_rescheduling is checked before handle_stopped() but never passed through. This creates inconsistent behavior:

GoingAway can reset rescheduling (mod.rs:456)
Lost checks reset_rescheduling but the reset happens before determining if rescheduling is needed

5. Documentation Gaps 📝

The GoingAway signal (mod.rs:780-785) lacks documentation explaining when it should be used vs Lost
The distinction between stopping, going_away, and sleeping states needs clearer documentation
No doc comments explaining the lifecycle state machine transitions

6. Force Stop on Destroy ⚠️
In the Destroy signal handler (mod.rs:508-521), a stop command is sent even if the actor might already be stopped. While this might be intentional for cleanup, consider:

Could this create unnecessary work or log noise?
Should there be a check for current actor state?

7. Formatting Inconsistencies 🎨
Some formatting changes appear inconsistent with surrounding code:

mod.rs:293-296: Breaks let-else statement across lines
mod.rs:306-311: Nested formatting for config().pegboard()

While these follow the hard tab convention, they differ from the original style. Consider consistency with the existing codebase style.

8. Test Coverage ❗
No test files were found for the pegboard workflows. Given the complexity of state management and the critical nature of actor lifecycle management, this code would greatly benefit from:

Unit tests for handle_stopped() with different StoppedVariant inputs
Integration tests for the GoingAway vs Lost signal handling paths
State transition tests to verify the state machine correctness

Potential Bugs 🐛

1. Duplicate Stop Commands
In handle_stopped() (mod.rs:640-653), when an actor is lost, a stop command is sent to the old runner. But if GoingAway already sent a stop command, this could result in duplicate stop commands for the same actor/generation. While likely harmless, it's inefficient.

2. Unconditional reset_rescheduling in Lost Signal
At mod.rs:456, reset_rescheduling resets the state before calling handle_stopped(). This means even if force_reschedule is false and the actor is sleeping, the reschedule count gets reset. Is this intended?

Performance Considerations ⚡

The changes don't introduce obvious performance issues
Signal sending is already async, so the additional GoingAway signal shouldn't cause problems
State field additions are minimal (3 booleans)

Security Concerns 🔒

No security issues identified. The changes are internal to the actor lifecycle management system.

Recommendations 📋

Add comprehensive tests - This is critical given the complexity of state management
Document the state machine - Add clear documentation explaining all possible states and transitions
Refactor state flags - Consider consolidating stopping, going_away, and sleeping into a proper state enum as mentioned in the TODO
Add idempotency checks - Ensure all signal handlers protect against duplicate signal delivery
Fix reset_rescheduling semantics - Make the behavior consistent between Lost and GoingAway signals
Add structured logging - Include state transitions in logs to help with debugging: tracing::info!(?old_state, ?new_state, "actor state transition")

Architecture Suggestion 💡

Consider modeling the actor state more explicitly:

enum ActorRunState {
    Allocated { runner_id: Id, runner_workflow_id: Id },
    Sleeping { will_wake: bool },
    Stopping { runner_id: Id, runner_workflow_id: Id },
    GoingAway { runner_id: Id, runner_workflow_id: Id },
    Rescheduling,
}

This would make invalid states unrepresentable and clarify the state machine.

Verdict

This PR addresses a real issue and the approach is sound. However, the increasing complexity of boolean state flags is concerning and aligns with the existing TODO comment. The changes work but would benefit from the refactoring mentioned in runtime.rs:20-21 to make the state machine more transparent.

Recommendation: Approve with suggestions for follow-up work on state machine refactoring and test coverage.

claude · 2025-11-13T17:22:39Z

Code Review

Overview

This PR addresses an important issue where actors were being immediately reallocated when a runner was stopping. The fix introduces a new GoingAway signal to handle this scenario more gracefully, allowing actors to be stopped without triggering immediate reallocation.

Key Changes Summary

New GoingAway signal: Distinguishes between runner stopping (going away) and actor lost scenarios
Removed kill parameter: Simplified the destroy workflow by removing the conditional kill logic
Enhanced state tracking: Added stopping and going_away flags to LifecycleState
Improved handle_stopped function: Refactored with cleaner enum-based variants

Positive Aspects

Architecture & Design

✅ Good separation of concerns: The new GoingAway signal appropriately separates the "runner stopping" case from the "actor lost" case, making the state machine clearer.

✅ Simplified destroy workflow: Removing the kill parameter and associated logic from destroy.rs reduces complexity and removes the need to track whether stop signals should be sent.

✅ Better code organization: The StoppedVariant enum and StoppedResult enum make the handle_stopped function much more readable and maintainable.

Code Quality

✅ Consistent error handling: Proper use of Result types throughout.

✅ Good use of structured logging: Following the tracing patterns correctly (e.g., ?variant for debug logging).

✅ Idiomatic Rust: Proper use of pattern matching, enums, and Option types.

Concerns & Suggestions

1. State Management Complexity ⚠️

The LifecycleState now has three related boolean flags: sleeping, stopping, and going_away. This creates potential for invalid state combinations.

Issue: What happens if both stopping and going_away are true simultaneously? Are there invariants that should be maintained?

Suggestion: Consider the TODO comment at runtime.rs:20-21 about rewriting this as nested structs/enums:

enum ActorLifecycleState {
    Running {
        runner_id: Id,
        runner_workflow_id: Id,
    },
    Stopping {
        runner_id: Id,
        runner_workflow_id: Id,
        reason: StopReason,
    },
    Sleeping {
        will_wake: bool,
        alarm_ts: Option<i64>,
    },
}

enum StopReason {
    IntentToStop,
    RunnerGoingAway,
}

This would make invalid states unrepresentable.

2. Missing State Reset Logic ⚠️

In handle_stopped at lines 601-602 of mod.rs, you reset:

state.going_away = false;
state.stopping = false;

However, these flags are also set in other locations:

going_away is set at line 490 in the Main::GoingAway branch
stopping is set at line 343 in the ActorIntentStop handler

Question: Are there edge cases where these flags might not be properly reset, causing actors to remain in a stuck state?

3. Race Condition Potential 🔴

At lines 333-361 in mod.rs, the ActorIntentStop handler checks !state.stopping before sending the stop command. Similarly, the GoingAway handler checks !state.going_away.

Issue: If a GoingAway signal arrives while stopping == true, the stop command won't be sent. Is this intentional? What if the actor needs to be stopped due to runner shutdown while it's already stopping for another reason?

Suggestion: Consider whether these flags should be:

Mutually exclusive (one takes precedence)
Independent (both can be true, different handling)
Documented with clear invariants

4. Destroy Signal Behavior 📝

At lines 508-520 in mod.rs, the Main::Destroy handler sends a stop command if runner_workflow_id exists, then returns.

Question: What happens if the stop command fails or the runner doesn't respond? Should there be a timeout or fallback mechanism to ensure destruction completes?

5. Lost Actor Stop Command Logic 🤔

At lines 640-653 in mod.rs, when an actor is lost, you send a stop command to the old runner "just in case it ended up allocating."

Comment: This is good defensive programming, but the comment could be more detailed. Consider adding:

// When an actor becomes lost, we don't know its actual state on the runner.
// It might have allocated successfully but we lost connection, or it might
// have failed to allocate. Send a stop command to the old generation to ensure
// cleanup, as the runner will ignore it if the actor doesn't exist.

6. Force Reschedule Logic Simplification ✅

Good improvement! The removal of the separate reset_rescheduling parameter in favor of handling it at the signal level (Lost and GoingAway signals) is cleaner.

7. Test Coverage ❓

Missing: The PR doesn't include test changes. Given the complexity of the state machine changes:

Recommendation: Add tests for:

Runner stopping scenario (new GoingAway signal)
Actor receiving GoingAway while already stopping
Actor receiving GoingAway while sleeping
Race conditions between different stop signals
Proper flag reset after handle_stopped

8. Documentation 📖

The new GoingAway signal struct has minimal documentation:

#[derive(Debug)]
#[signal("pegboard_actor_going_away")]
pub struct GoingAway {
    pub generation: u32,
    #[serde(default)]
    pub reset_rescheduling: bool,
}

Suggestion: Add doc comments:

/// Signal sent to an actor when its runner is stopping gracefully.
/// Unlike the `Lost` signal, this indicates a planned shutdown rather than
/// an unexpected failure, so we don't force immediate reallocation.
#[derive(Debug)]
#[signal("pegboard_actor_going_away")]
pub struct GoingAway {
    pub generation: u32,
    /// Resets the rescheduling retry count to 0, treating this as a fresh start.
    #[serde(default)]
    pub reset_rescheduling: bool,
}

9. Potential Cleanup Opportunities 🧹

The removal of the kill field from multiple structs is great! Consider if there are any other unused fields or parameters that could be cleaned up in related code.

Security Considerations

✅ No obvious security concerns. The changes maintain proper generation checking to prevent cross-generation signal handling.

Performance Considerations

✅ No performance regressions expected. The changes actually reduce work by avoiding unnecessary reallocations.

⚠️ Minor concern: The handle_stopped function now has more branches. Ensure this doesn't significantly impact the hot path, though the impact should be minimal.

Summary

This is a solid fix for an important issue. The main concerns are around:

State management complexity with multiple boolean flags
Potential race conditions between different stop signals
Missing test coverage

Recommendation: ✅ Approve with suggestions

The core logic is sound, but would benefit from:

Additional tests for the new state transitions
Better documentation of the GoingAway signal
Consideration of the state management TODO for future refactoring

Great work on simplifying the destroy workflow and improving the runner shutdown behavior! 🎉

claude · 2025-11-13T21:01:05Z

Test comment

claude · 2025-11-13T21:01:18Z

Pull Request Review: fix(pb): dont immediately reallocate actors on runner stopping

Summary

This PR introduces a new GoingAway signal to handle the case when a runner is stopping gracefully. The key improvement is that actors are no longer immediately marked as "lost" and force-rescheduled when their runner begins draining. Instead, they receive a GoingAway signal that allows them to stop gracefully and then be rescheduled based on their crash policy.

Code Quality & Best Practices

Strengths

Improved State Management: The addition of stopping and going_away flags (runtime.rs:32-34) provides clearer state tracking and prevents duplicate signal sends.
Better Separation of Concerns: The new GoingAway signal distinguishes between:
- Actors that are truly lost (timeout, network issues)
- Actors whose runner is gracefully shutting down
Simplified Destroy Workflow: Removed the unnecessary kill parameter and signal sending from destroy.rs, making the workflow cleaner since the stop command is now sent before entering the destroy workflow.
Consistent Pattern: The handling of GoingAway follows the same pattern as ActorIntentStop, which is good for maintainability.

claude · 2025-11-13T21:01:32Z

Areas for Improvement

Missing Serialization Derives: The new state fields stopping and going_away are marked with #[serde(default)] which is correct for backward compatibility, but consider adding these to any migration documentation if you have one.
Code Comment Accuracy: In mod.rs:640, there is a duplicated "actor" word in the comment: "We dont know the state of the previous generation of this actor actor if it becomes lost" - should be fixed.
Enum Documentation: The new StoppedVariant enum (mod.rs:569-573) lacks documentation. Consider adding doc comments explaining when each variant is used.

Potential Issues

Critical

None identified - The logic appears sound.

Minor

State Cleanup in handle_stopped: Lines mod.rs:602-604 clear multiple state flags. Consider verifying this is always the desired behavior regardless of the StoppedVariant. For example, if an actor is lost while going_away=true, does clearing this flag make sense? (It likely does, but worth double-checking the edge cases)
Signal Ordering: In the Destroy signal handler (mod.rs:509-526), you send a stop command to the runner if the actor is allocated. However, there is no check for whether the actor is already stopping or going_away. While this is probably harmless (duplicate stop commands should be idempotent), it could be optimized.

claude · 2025-11-13T21:01:45Z

Performance Considerations

Positive Impact: By not immediately rescheduling actors when a runner is draining, you avoid unnecessary churn. Actors can shut down gracefully and then be rescheduled based on their crash policy, which is more efficient.
GC Timeout Reset: The gc_timeout_ts is properly managed throughout, preventing actors from being incorrectly marked as lost during normal shutdown flows.

Security Concerns

None identified - The changes do not introduce any new security concerns. The signal-based architecture already handles authorization/routing.

Test Coverage

Missing Test Cases

While I cannot see the test files in this PR, consider adding tests for:

GoingAway Signal Handling: Test that when a runner sends GoingAway, actors transition to the correct state without being force-rescheduled
State Deduplication: Test that sending multiple GoingAway or ActorIntentStop signals does not cause duplicate stop commands
Race Condition: Test the case where an actor receives GoingAway while already in a stopping state
Backward Compatibility: Test that actors with old state (without stopping/going_away fields) deserialize correctly with defaults

Suggested Test Locations

Based on the CLAUDE.md patterns, you should add tests at:

packages/pegboard/tests/ (if integration tests)
Unit tests within the modules using #[cfg(test)]

claude · 2025-11-13T21:02:03Z

Architecture Alignment

The changes align well with the codebase patterns:

Error Handling: No custom errors needed here - using Result<()> appropriately
Logging: Good use of structured logging with ?variant in mod.rs:586
State Management: Follows the existing pattern of using LifecycleState for tracking actor state

Additional Notes

Comment in runtime.rs:20-21: The TODO comment suggests this state management could benefit from a rewrite with nested structs/enums. The new stopping and going_away flags add to the complexity. Consider tracking this technical debt in a follow-up issue.
Reset Rescheduling Logic: The reset_rescheduling parameter is used consistently in both Lost and GoingAway signals, which is good. However, in handle_stopped (mod.rs:588-599), the reset logic for StoppedVariant::Normal with Ok code happens before the force_reschedule extraction. Make sure this is intentional and the order does not matter.

Recommendation

✅ Approve with minor suggestions

The core logic is solid and addresses a real issue with unnecessary actor rescheduling during runner shutdown. The suggested improvements are mostly cosmetic (documentation, minor optimizations) and do not block merging.

Before Merging

Fix typo: "actor actor" → "actor" in mod.rs:640
Consider adding documentation to StoppedVariant and StoppedResult enums
Verify test coverage for the new GoingAway signal path
Run the full test suite to ensure no regressions

Great work on improving the actor lifecycle management! The distinction between "lost" and "going away" actors is an important one for system reliability.

claude · 2025-11-13T21:43:35Z

Code Review: PR #3413 - fix(pb): dont immediately reallocate actors on runner stopping

Summary

This PR refactors the actor lifecycle management to introduce a new GoingAway signal that prevents immediate reallocation when a runner is stopping. The key change is separating the "runner stopping" scenario from the "actor lost" scenario, which should improve resource efficiency during graceful shutdowns.

Positive Changes

1. Better Separation of Concerns

The new GoingAway signal clearly distinguishes between "runner is draining" vs "actor actually lost"
This prevents unnecessary actor reallocations during planned runner shutdowns
Aligns with the PR title's goal

2. Simplified destroy Workflow

Removed the kill parameter and associated logic in destroy.rs
The destroy workflow is now cleaner and focuses solely on state cleanup
The CommandStopActor signal is now sent from the main actor workflow when needed

3. Enhanced State Tracking

Added stopping and going_away boolean flags to LifecycleState
Provides better visibility into the actor's transitional states
Uses #[serde(default)] for backward compatibility

4. Improved Code Organization

The handle_stopped function now uses an enum StoppedVariant instead of Option<StopCode> + Option<Lost> parameters
Returns StoppedResult enum instead of Option<LifecycleRes>, making the intent clearer
Better use of pattern matching throughout

Issues & Concerns

1. Potential Race Condition in `GoingAway` Handler ⚠️

Location: engine/packages/pegboard/src/workflows/actor/mod.rs:447-486

The GoingAway signal handler checks !state.going_away before processing, but there's a potential race:

if !state.going_away {
    // ... send stop command
}

Issue: If multiple GoingAway signals arrive before the state is persisted, they might all pass the check. While unlikely, this could result in duplicate CommandStopActor signals.

Recommendation: This is probably acceptable given workflow semantics, but adding a comment explaining why duplicate signals are safe (or implementing additional deduplication) would help future maintainers.

2. Missing State Transition Documentation 📝

Location: engine/packages/pegboard/src/workflows/actor/runtime.rs:20-21

There's a TODO comment:

// TODO: Rewrite this as a series of nested structs/enums for better transparency of current state (likely
// requires actor wf v2)

Issue: The state machine now has even more states (sleeping, stopping, going_away), making the TODO more pressing. The boolean flags can result in invalid state combinations (e.g., sleeping=true && going_away=true).

Recommendation:

Document valid state transitions in comments
Add runtime assertions to catch invalid states during development
Consider the v2 rewrite sooner rather than later

3. Idempotency Concerns in Signal Handlers ⚠️

Location: engine/packages/pegboard/src/workflows/actor/mod.rs:306-364

The handlers for ActorIntentSleep and ActorIntentStop now check flags before sending signals:

if !state.sleeping { /* send stop */ }
if !state.stopping { /* send stop */ }

Issue: If the workflow is replayed or the state isn't properly persisted between the check and the signal send, we might miss sending required signals.

Recommendation: Verify that the gasoline workflow engine properly handles these scenarios. Consider whether these checks should be warnings instead of hard guards.

4. Inconsistent `reset_rescheduling` Handling

Location: engine/packages/pegboard/src/workflows/actor/mod.rs:302, 446

In the Lost signal handler:

if sig.reset_rescheduling {
    state.reschedule_state = Default::default();
}

But in handle_stopped, the reset_rescheduling logic was removed from the function signature and is now handled by the caller.

Issue: This is fine, but it's a subtle change in responsibility that could be confusing.

Recommendation: Add a comment explaining where reset_rescheduling is handled for each signal type.

5. Missing Test Coverage ❌

Location: Entire PR

Issue: No test files were modified or added. Given the complexity of state transitions being modified, this is concerning.

Critical Scenarios to Test:

Runner goes into draining mode → actors receive GoingAway → actors reschedule on different runners
Actor stops gracefully while going_away=true
Actor becomes lost while going_away=true
Multiple rapid state transitions (sleep → wake → going_away)
GoingAway arrives for old generation (should be ignored)

Recommendation: Add integration tests covering these scenarios before merging.

Performance Considerations

Positive:

Reduces unnecessary actor reallocations during runner shutdowns
Should improve cluster efficiency during rolling deployments

Neutral:

Additional state flags increase memory footprint slightly (negligible)
StoppedVariant enum adds minimal matching overhead

Security Considerations

No security concerns identified. Changes are internal to the orchestration layer.

Code Style & Conventions

Good:
✅ Follows structured logging patterns (tracing::debug!(?variant, "actor stopped"))
✅ Uses #[serde(default)] for backward compatibility
✅ Proper error handling with Result<T>

Minor Issues:

Some formatting changes mixed with logic changes (makes review harder)
Consider splitting pure refactoring from behavioral changes in future PRs

Recommendations

Before Merging:

⚠️ HIGH PRIORITY: Add integration tests for the new GoingAway signal flow
⚠️ MEDIUM: Document valid state transitions for LifecycleState
⚠️ MEDIUM: Verify idempotency guarantees from the workflow engine

Future Work:
4. Consider the LifecycleState v2 rewrite to use nested enums instead of boolean flags
5. Add metrics/logging to track how often GoingAway prevents unnecessary reallocations

Verdict

The PR achieves its goal of preventing immediate reallocation on runner stopping. The code quality is good, and the refactoring to use enums improves clarity. However, the lack of test coverage for this critical state machine logic is concerning and should be addressed before merging.

Recommendation: Request tests before approval, then LGTM pending test coverage.

claude · 2025-11-13T21:51:08Z

Code Review: PR #3413 - Don't immediately reallocate actors on runner stopping

Summary

This PR introduces a significant improvement to the Pegboard actor lifecycle management by distinguishing between a runner stopping gracefully (GoingAway) and an actor being unexpectedly lost (Lost). This prevents unnecessary immediate reallocation of actors when a runner is shutting down gracefully.

Positive Changes ✅

1. Better State Modeling

New GoingAway signal: Properly distinguishes graceful runner shutdown from unexpected actor loss
New state flags: stopping and going_away flags provide clearer state tracking
Cleaner separation of concerns: StoppedVariant enum clearly models different stop scenarios

2. Improved Code Organization

StoppedResult enum: Makes the return intent from handle_stopped much clearer (Continue vs Destroy)
Consolidated stop handling: The refactored handle_stopped function is more maintainable with the variant-based approach
Removed the kill parameter: Simplifies the destroy workflow by removing the conditional killing logic

3. Bug Fix

The core issue is properly addressed - when a runner goes away gracefully, actors are marked appropriately without forcing immediate reallocation, which should reduce unnecessary churn.

Issues & Concerns ⚠️

1. Critical: State Management Complexity

The lifecycle state management has become quite complex with multiple boolean flags:

pub sleeping: bool,
pub stopping: bool,
pub going_away: bool,

Concern: These flags could lead to invalid state combinations. For example:

Can an actor be both stopping and going_away?
What if sleeping is true but going_away is also true?

Recommendation: As noted in the TODO comment at runtime.rs:20-21, consider rewriting this as nested enums for better type safety:

pub enum ActorLifecycleState {
    Running { runner_id: Id, runner_workflow_id: Id },
    Stopping { runner_id: Id, runner_workflow_id: Id },
    GoingAway { runner_id: Id, runner_workflow_id: Id },
    Sleeping { will_wake: bool },
    // etc.
}

2. Potential Race Condition

In mod.rs:445-475, when handling the GoingAway signal:

if !state.going_away {
    // ... set gc_timeout_ts, going_away flag
    // ... send stop command
}

Concern: There's a window between checking !state.going_away and setting it where another signal could arrive. While workflow state is typically single-threaded, consider if there are any edge cases.

3. Missing Reset Logic

In handle_stopped at mod.rs:599-601:

state.gc_timeout_ts = None;
state.going_away = false;
state.stopping = false;

Concern: These flags are reset regardless of the variant. Is this always correct? For example, if an actor is going_away and receives a normal stop event, should going_away be cleared?

4. Inconsistent Parameter Removal

The reset_rescheduling parameter was removed from reschedule_actor function signature but the logic still exists internally as reset:

state.reschedule_state.retry_count = if reset {
    0
} else {
    state.reschedule_state.retry_count + 1
};

Concern: The reset variable is computed from state.reschedule_state.retry_count == 0 which is confusing. If reset_rescheduling was intentionally removed, ensure the behavior is still correct.

5. Destroy Signal Still Sends Stop Command

In mod.rs:509-521, the Destroy signal now sends a CommandStopActor before destroying:

Main::Destroy(_) => {
    // If allocated, send stop actor command
    if let Some(runner_workflow_id) = state.runner_workflow_id {
        ctx.signal(crate::workflows::runner::Command {
            // ...
        })
        // ...
    }
    // Then destroy
}

Question: Is this intentional? Previously with kill: true, the destroy workflow would handle stopping. Now it happens in the main workflow. This seems correct but worth confirming the behavior change is intentional.

Minor Issues 🔍

1. Documentation

The new going_away and stopping flags lack inline documentation explaining when each is set
The StoppedVariant enum could use doc comments explaining each variant

2. Logging

At mod.rs:585:

tracing::debug!(?variant, "actor stopped");

Suggestion: Consider more descriptive logging that includes actor_id for easier debugging:

tracing::debug!(actor_id=?input.actor_id, ?variant, "actor stopped");

3. Formatting Changes

Several formatting changes (like line breaks in else blocks) were included. While not harmful, they make the diff larger. Consider separating pure formatting changes from logic changes in future PRs.

Security Considerations 🔒

No significant security concerns identified. The changes are primarily internal state management improvements.

Performance Considerations ⚡

Positive Impact: This change should reduce unnecessary actor reallocations when runners are gracefully shutting down, which will:

Reduce load on the allocation system
Improve overall system stability
Reduce churn in actor assignments

Test Coverage 📋

Major Concern: No test files were found for the pegboard package. Given the complexity of this state machine and the critical nature of actor lifecycle management:

Strong Recommendation: Add tests covering:

Actor receives GoingAway signal → should stop gracefully without immediate reallocation
Actor receives Lost signal with force_reschedule: true → should reallocate immediately
Actor receives Lost signal with force_reschedule: false → should handle based on crash policy
State transitions: ensure stopping, going_away, sleeping flags are managed correctly
Edge case: rapid signal sequences (e.g., GoingAway followed immediately by Destroy)

Recommendations

High Priority

Add comprehensive tests for the new state transitions
Add inline documentation for the new state flags explaining when they're set/cleared
Verify the Destroy signal behavior change is intentional

Medium Priority

Consider the enum-based state model mentioned in the TODO for future refactoring
Add more structured logging with actor_id in the handle_stopped function
Review the reset logic in handle_stopped to ensure flags are cleared appropriately for all variants

Low Priority

Separate formatting changes from logic changes in future PRs

Overall Assessment

This is a solid improvement that addresses a real issue with unnecessary actor reallocation. The core logic is sound, and the separation of GoingAway vs Lost is the right approach. However, the increasing complexity of the state management flags suggests that a more structured state model (as noted in the TODO) would be beneficial for long-term maintainability.

Recommendation: Approve with the strong suggestion to add tests before merging.

📝 Review generated by Claude Code

claude · 2025-11-13T22:32:14Z

Code Review: PR #3413 - Fix actor reallocation on runner stopping

Summary

This PR introduces a new GoingAway signal to handle the case when a runner is stopping gracefully, preventing immediate reallocation of actors. This is a significant improvement to the actor lifecycle management in Pegboard.

Positive Changes

1. Improved State Modeling ✅

Added stopping and going_away boolean flags to LifecycleState to track different stopping scenarios
Introduces clearer separation between different stop conditions (user-initiated stop vs runner going away vs actor lost)
The new StoppedVariant enum improves type safety and makes the code more self-documenting

2. Cleaner Separation of Concerns ✅

The GoingAway signal properly distinguishes between actors that are stopping due to runner shutdown vs other reasons
Removed the kill parameter from the destroy workflow, simplifying the interface
Signal sending is now properly handled in the main actor workflow instead of in the destroy workflow

3. Better Error Recovery ✅

Properly resets state flags (going_away, stopping) in handle_stopped to prevent state corruption
The refactored logic prevents unnecessary reallocations when a runner is gracefully shutting down

Issues and Concerns

1. Missing Idempotency Guards ⚠️

The GoingAway signal handler at mod.rs:471-502 has an idempotency check (if !state.going_away), but there's a potential race condition:

Main::GoingAway(sig) => {
    // ...
    if !state.going_away {
        // Multiple signal sends could arrive before state is persisted
        state.going_away = true;
        // ... send CommandStopActor signal
    }
}

Issue: If multiple GoingAway signals arrive before the workflow state is persisted, multiple CommandStopActor signals could be sent to the runner.

Recommendation: Consider adding a trace log when the signal is ignored due to state.going_away already being true, similar to the pattern used for stopping and sleeping flags.

2. Generation Mismatch Handling ⚠️

At mod.rs:473-475, the code ignores signals for previous generations:

if sig.generation != state.generation {
    return Ok(Loop::Continue);
}

Concern: What happens if a GoingAway signal arrives for a future generation? This could occur during rapid rescheduling scenarios.

Recommendation: Add a trace warning for generation mismatches to aid debugging:

if sig.generation != state.generation {
    tracing::debug!(?sig.generation, current_generation=?state.generation, "ignoring GoingAway signal for different generation");
    return Ok(Loop::Continue);
}

3. Comment Accuracy 📝

At mod.rs:640-641, the comment has a typo:

// We don't know the state of the previous generation of this actor actor if it becomes lost
//                                                                   ^^^^^^ duplicate word

4. State Cleanup Order 🤔

In handle_stopped at mod.rs:601-606:

state.gc_timeout_ts = None;
state.going_away = false;
state.stopping = false;
state.runner_id = None;
let old_runner_workflow_id = state.runner_workflow_id.take();

Question: Is there a reason to clear these flags before deallocating? If deallocation fails, the state might be inconsistent.

Recommendation: Consider whether cleanup should happen after successful deallocation, or add comments explaining why early cleanup is correct.

5. Missing Test Coverage ⚠️

No test files were found in the pegboard package for this functionality.

Recommendation: Add integration or unit tests covering:

Actors receiving GoingAway signal during normal operation
Multiple actors on a stopping runner
Race conditions between GoingAway and Lost signals
Proper state transitions when reset_rescheduling is true/false

6. Force Reschedule Logic 🤔

At mod.rs:588-599, the force_reschedule logic only extracts the flag from StoppedVariant::Lost:

let force_reschedule = match &variant {
    StoppedVariant::Normal { code: protocol::StopCode::Ok } => {
        state.reschedule_state = Default::default();
        false
    }
    StoppedVariant::Lost { force_reschedule } => *force_reschedule,
    _ => false,
};

Concern: The pattern matching doesn't explicitly handle all StoppedVariant::Normal cases - only the Ok code resets reschedule state.

Recommendation: Make this more explicit:

let force_reschedule = match &variant {
    StoppedVariant::Normal { code } => {
        if matches!(code, protocol::StopCode::Ok) {
            state.reschedule_state = Default::default();
        }
        false
    }
    StoppedVariant::Lost { force_reschedule } => *force_reschedule,
};

7. Potential for Lost CommandStopActor ⚠️

At mod.rs:517-527, when handling the Destroy signal, a CommandStopActor is sent to the runner:

Main::Destroy(_) => {
    if let Some(runner_workflow_id) = state.runner_workflow_id {
        ctx.signal(crate::workflows::runner::Command {
            inner: protocol::Command::CommandStopActor(protocol::CommandStopActor {
                actor_id: input.actor_id.to_string(),
                generation: state.generation,
            }),
        })
        .to_workflow_id(runner_workflow_id)
        .send()
        .await?;
    }
    // ...
}

Concern: If the signal send fails (network partition, runner workflow gone), the actor workflow will error and potentially retry, but the runner might already be stopped.

Recommendation: Consider whether this should use a best-effort send pattern, or if the current error propagation is intentional.

Style and Convention Issues

1. Logging Consistency 📝

At mod.rs:586, the log uses structured logging correctly:

tracing::debug!(?variant, "actor stopped");

Good adherence to CLAUDE.md guidelines! ✅

2. Import Organization ✅

Imports are properly organized at the top of files, following the style guide.

3. Serialization ✅

Proper use of #[serde(default)] for new fields (stopping, going_away) ensures backward compatibility with existing serialized state.

Performance Considerations

1. Signal Storm Prevention ⚠️

When a runner stops, it sends GoingAway signals to all actors at runner.rs:414-422. For runners with many actors (e.g., 100+), this could create a signal storm.

Consideration: The current implementation is fine for moderate actor counts, but might need batching or rate limiting for very large runners.

2. No New Database Queries ✅

The changes don't introduce additional database roundtrips, which is good for performance.

Security Considerations

1. No Security Issues Identified ✅

The changes don't introduce obvious security vulnerabilities. Actor isolation and generation tracking prevent cross-actor interference.

Recommendations Summary

High Priority:

Add test coverage for the new GoingAway signal behavior
Add debug logging for ignored signals (different generations, duplicate signals)
Fix the typo in the comment at line 640

Medium Priority:
4. Review state cleanup ordering in handle_stopped
5. Make force_reschedule pattern matching more explicit
6. Document the expected behavior when CommandStopActor signal fails

Low Priority:
7. Consider adding metrics for GoingAway signal processing
8. Add a TODO or comment about potential signal storm mitigation for large runners

Overall Assessment

This is a solid improvement to the actor lifecycle management system. The code is well-structured and addresses a real issue with unnecessary reallocations. The main concerns are around edge cases, test coverage, and operational observability. The changes follow the project's style guidelines and maintain backward compatibility well.

Recommendation: Approve with minor changes requested for test coverage and logging improvements.

Generated with Claude Code

claude · 2025-11-13T22:35:51Z

Pull Request Review

Summary

This PR introduces a new GoingAway signal to distinguish between actors that become lost (due to crashes/timeouts) and actors on runners that are gracefully stopping. This prevents immediate reallocation of actors when a runner is draining, allowing the runner to cleanly stop its actors first.

Code Quality & Best Practices

✅ Strengths

Clear separation of concerns: The new GoingAway signal properly distinguishes between different actor termination scenarios:
- Lost: Actor lost due to timeout/crash (may need immediate reallocation)
- GoingAway: Runner is gracefully stopping (should wait for clean shutdown)
- Destroy: Explicit actor destruction
State management improvements: Added stopping and going_away boolean flags to LifecycleState to track different shutdown states, improving clarity (runtime.rs:32-34).
Idempotency: Properly checks state flags before sending signals to prevent duplicate operations:
```
if !state.stopping { /* ... */ }
if !state.going_away { /* ... */ }
```
Code cleanup: Removed the kill parameter from the destroy workflow, simplifying the interface and removing unnecessary complexity.
Better error handling: The refactored handle_stopped function uses enums (StoppedVariant, StoppedResult) instead of Option<Result>, making control flow clearer.

⚠️ Potential Issues

State complexity (acknowledged by TODO): The LifecycleState struct has a TODO comment noting it should be rewritten as nested structs/enums (runtime.rs:20-21). The current design with multiple boolean flags (sleeping, stopping, going_away, will_wake) can lead to invalid state combinations.
Missing #[serde(default)] on new fields: While stopping and going_away have #[serde(default)] for backward compatibility (runtime.rs:31-34), ensure this is tested with existing workflows that may have serialized state without these fields.

Comment typo: Line 640 in mod.rs has "actor actor" (should be just "actor"):

// We don't know the state of the previous generation of this actor actor if it becomes lost

Potential race condition: In the Destroy signal handler (mod.rs:509-526), if runner_workflow_id is Some, a stop command is sent. However, there's no timeout or confirmation that the command was received before the workflow breaks the loop. If the runner is already gone, this could delay cleanup.

Performance Considerations

✅ Good

Reduced unnecessary reallocations: By not immediately marking actors as Lost when a runner stops gracefully, this avoids churn in actor allocation.
Efficient state updates: The changes don't introduce additional database queries or activities in hot paths.

💡 Suggestions

Consider batch signal sending: In handle_stopping (runner.rs:413-423), actors are signaled in a loop with await? inside. This is sequential. If there are many actors, this could be slow. Consider:

// Current (sequential)
for (actor_id, generation) in &actors {
    ctx.signal(...).send().await?;
}

// Potential optimization (if order doesn't matter)
let futures = actors.iter().map(|(actor_id, generation)| {
    ctx.signal(...).send()
});
futures_util::future::try_join_all(futures).await?;

Security Concerns

✅ No major security issues identified

The changes are primarily around workflow orchestration and don't introduce new attack surfaces. The signal-based architecture is already in place.

💡 Minor observations

The reset_rescheduling flag in GoingAway signal allows resetting retry counts. Ensure this cannot be abused if signals can be sent from untrusted sources.

Test Coverage

⚠️ Missing test updates

The PR doesn't include test updates for the new GoingAway signal path. Recommended test cases:

Graceful runner shutdown: Verify actors receive GoingAway signal and transition correctly
Actor state transitions: Test going_away flag is set/cleared appropriately
Backward compatibility: Test that workflows with old serialized state (missing new boolean fields) work correctly due to #[serde(default)]
Race conditions: Test Destroy signal during GoingAway state
Multiple state flags: Test combinations of sleeping, stopping, and going_away flags

Architecture & Design

✅ Well-aligned with existing patterns

The changes follow the existing signal-based workflow pattern and integrate cleanly with the actor lifecycle management.

💡 Future improvements

The TODO comment (runtime.rs:20-21) about rewriting state as nested enums is valid. Consider this refactor:

enum ActorLifecycleState {
    Running { runner_id: Id, runner_workflow_id: Id },
    Stopping { runner_id: Id, runner_workflow_id: Id },
    GoingAway { runner_id: Id, runner_workflow_id: Id },
    Sleeping { alarm_ts: Option<i64> },
    WillWake { /* ... */ },
}

This would make invalid states unrepresentable and simplify logic.

Recommendations

High Priority

✅ Fix typo on line 640: "actor actor" → "actor"
⚠️ Add tests for the new GoingAway signal path
⚠️ Test backward compatibility with existing serialized workflow state

Medium Priority

💡 Consider batching signal sends in handle_stopping for performance
💡 Add timeout or error handling for the stop command in Destroy handler
💡 Document the state machine transitions in a comment or diagram

Low Priority (Future Work)

💡 Consider the state machine refactor suggested in the TODO

Verdict

Overall: Good PR ✅

The core fix is sound and addresses a real issue with unnecessary actor reallocation. The code follows existing patterns and conventions. Main concerns are around test coverage and the existing state complexity (which this PR doesn't worsen, just maintains).

Suggested changes before merge:

Fix the typo
Add test coverage for the new signal path
Verify backward compatibility with existing workflows

🤖 Generated with Claude Code

claude · 2025-11-13T22:38:39Z

PR Review: Fix actor reallocation on runner stopping

Summary

This PR addresses an issue where actors were being immediately reallocated when a runner was stopping, which could lead to unnecessary churn and resource waste. The fix introduces a new GoingAway signal to gracefully handle the transition period when a runner is draining.

Code Quality & Best Practices ✅

Strengths:

Clear separation of concerns: The introduction of GoingAway signal properly distinguishes between different stopping scenarios (normal stop vs runner draining)
Consistent state management: Added stopping and going_away flags with #[serde(default)] for backward compatibility
Proper cleanup: The handle_stopped function now properly resets both going_away and stopping flags (line 603-604 in mod.rs)
Follows existing patterns: The new signal follows the same pattern as Lost with generation tracking and reset_rescheduling support

Code style observations:

Formatting changes are consistent with the codebase
Log messages follow the convention (lowercase, structured logging)
Import cleanup is appropriate

Architecture & Logic 🎯

Key improvements:

Removed premature destruction logic: The kill parameter has been removed from the destroy workflow, preventing premature stop commands during destruction
New GoingAway signal: Properly handles runner draining scenario with idempotency via !state.going_away check
Simplified LifecycleResult: Removed the kill boolean which was causing complexity

Potential concerns:

Race condition handling: Consider adding a log message when GoingAway signal is received but actor is not allocated
State transition clarity: With sleeping, stopping, and going_away flags, the state space is becoming complex. Consider documenting valid state combinations.

Security Concerns 🔒

No security issues identified. The changes maintain proper generation tracking and idempotency checks.

Performance Considerations ⚡

Positive impacts:

Reduced signal churn by not immediately reallocating actors when runner is stopping
Better resource utilization with graceful transitions

Potential Bugs 🐛

Minor issue found:

Line 640 comment has duplicate word: "this actor actor if" should be "this actor if"

All other checks passed:

Constructor initialization ✅
Signal handling for previous generations ✅
Destroy signal handling ✅

Test Coverage 🧪

Missing from this PR:

No test cases added for the new GoingAway signal
No tests verifying the behavior when a runner stops vs when it's draining
No tests for the idempotency of stopping and going_away flags

Documentation 📝

Suggestions:

Add doc comment to GoingAway signal explaining when it's used vs Lost
Document the state flags in LifecycleState to clarify their purpose

Summary & Recommendation

Overall assessment: This is a solid fix that addresses a real problem with actor reallocation during runner shutdown. The code quality is good and follows existing patterns.

Recommendation: ✅ Approve with minor suggestions

Before merging:

Fix the duplicate "actor actor" typo in comment at line 640
Consider adding test coverage for the new GoingAway signal behavior
Consider adding documentation comments

Nice to have (follow-up):

Address the TODO at runtime.rs:20 about refactoring state management into nested structs/enums for v2

Great work on identifying and fixing this issue! 🎉

claude · 2025-11-13T22:39:18Z

Code Review

This PR addresses an important issue where actors were being immediately reallocated when a runner was stopping, which could cause unnecessary churn. The solution introduces a new GoingAway signal to gracefully handle the runner shutdown scenario. Overall, the changes are well-structured and improve the actor lifecycle management.

✅ Strengths

Clear separation of concerns: The new GoingAway signal appropriately distinguishes between a runner stopping (graceful) vs an actor being lost (unexpected failure).
Simplified destroy workflow: Removing the kill parameter and associated logic from the destroy workflow is a good cleanup - the actor workflow now handles sending stop commands directly.
Proper state management: Added stopping and going_away flags to LifecycleState to track different shutdown scenarios, with appropriate #[serde(default)] for backward compatibility.
Consistent timeout handling: All stop scenarios (sleep, stop intent, going away) now consistently set gc_timeout_ts and mark the actor as not connectable.

🔍 Potential Issues

Missing Debug derive on StoppedResult (mod.rs:575)
- StoppedVariant has #[derive(Debug)] but StoppedResult doesn't. For consistency and debugging, consider adding it.
State cleanup in handle_stopped (mod.rs:602-604)
- The function clears going_away and stopping flags together. Ensure this is correct for all code paths - if an actor is in stopping state and receives a GoingAway signal, both flags would be set, which seems correct.
Race condition consideration (mod.rs:510-521)
- In the Destroy signal handler, if runner_workflow_id is Some, a stop command is sent. However, if the actor is already in going_away or stopping state, this could result in duplicate stop commands being sent. While likely harmless, consider checking these flags before sending.
Potential duplicate stop commands (mod.rs:498-503)
- The GoingAway signal sends a stop command if !state.going_away. However, if the actor was already in stopping state (from ActorIntentStop), this would be a second stop command. Verify the runner handles duplicate stop commands gracefully.

💡 Suggestions

Documentation (runtime.rs:20-21)
- The TODO comment mentions rewriting as nested structs/enums for better state transparency. Consider filing an issue to track this if one doesn't exist.
Code clarity (mod.rs:640-654)
- The comment says "We don't know the state of the previous generation of this actor if it becomes lost" but this logic only applies to Lost variant, not GoingAway. Consider clarifying that for GoingAway, we DO know the state and gracefully handle it.
Consistent formatting (mod.rs:499-502)
- The CommandStopActor instantiation here uses inline formatting, while at mod.rs:512-514 it's on separate lines. Consider consistent formatting (the CLAUDE.md mentions following existing patterns).
Signal naming (mod.rs:780)
- GoingAway is a good descriptive name. Consider documenting in a comment that this specifically handles the case where a runner is gracefully shutting down vs being lost.

🧪 Testing Recommendations

Test the scenario where a runner stops gracefully with multiple actors allocated
Test that actors are properly rescheduled after GoingAway without immediate retry
Verify that duplicate stop commands don't cause issues
Test race condition: Destroy signal arrives while processing GoingAway
Test state transitions: actor in stopping state receives GoingAway signal

🔒 Security & Performance

✅ No security concerns identified
✅ Performance should improve by reducing unnecessary reallocations
✅ Proper use of UDB transactions and state management

📝 Minor Code Quality Notes

Imports (destroy.rs:2): The import rivet_runner_protocol was removed but is still being used elsewhere - confirm this wasn't needed.
Error handling: All error paths use proper Result types - good adherence to CLAUDE.md guidelines.
Logging: The structured logging follows the pattern from CLAUDE.md (e.g., tracing::debug!(?variant, "actor stopped")).

Summary

This is a solid improvement to the actor lifecycle management. The main concerns are potential duplicate stop commands and ensuring the state machine handles all edge cases correctly. The refactoring simplifies the destroy workflow and adds clearer semantics for different shutdown scenarios.

Recommendation: Approve with minor suggestions. The potential duplicate stop command issue should be verified, but likely won't cause functional problems.

graphite-app · 2025-11-13T22:43:28Z

Merge activity

Nov 13, 10:43 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Nov 13, 10:43 PM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #3400.

claude · 2025-11-13T22:46:45Z

PR Review: WebSocket Hibernation & Runner Allocation Fix

Summary

This PR introduces WebSocket hibernation for the gateway and fixes an issue where actors were immediately reallocated when their runner was stopping. The changes span multiple components including Guard, Pegboard Gateway, Pegboard actor lifecycle, and the Gasoline workflow engine.

Code Quality & Best Practices ✅

Strengths:

Excellent naming improvements: The rename from wake_sub → bump_sub and wake_worker → bump_workers is more semantically accurate and reduces confusion
Good error handling patterns: Uses custom RivetError types appropriately (WebSocketServiceHibernate, WebsocketPendingLimitReached)
Comprehensive test coverage: Added test_custom_serve_websocket_hibernation which validates the hibernation flow
Proper adherence to CLAUDE.md:
- Updated anyhow import guidance from glob to explicit imports ✅
- Uses structured logging correctly (e.g., tracing::debug!(?err, "..."))
- Error definitions follow the RivetError derive macro pattern
Good use of async patterns: Proper use of tokio::select!, tokio::join!, watch channels for abort signaling

Code Structure:

Separation of concerns: handle_websocket_hibernation is cleanly separated from handle_websocket
Type safety: Good use of enums (HibernationResult, LifecycleResult) to represent state
Resource management: Proper cleanup and abort signaling between concurrent tasks

Potential Issues & Concerns 🔍

1. Critical: Unreachable Error Check (packages/guard-core/src/proxy_service.rs:1911-1920)

ensure!(
    !ws_hibernation_close,
    "should not be hibernating again after receiving a close frame during hibernation"
);

Issue: The comment says "This should be unreachable" but uses ensure! which will panic. If this is truly unreachable, consider using debug_assert! or removing it. If it's a safety check for a potential bug, the current approach is fine but the comment is misleading.

Recommendation: Either change to debug_assert!(!ws_hibernation_close) or update the comment to clarify this is a runtime safety check.

2. Potential Race Condition (packages/pegboard/src/workflows/actor/mod.rs:472-508)

The GoingAway signal handling:

Main::GoingAway(sig) => {
    if !state.going_away {
        let Some(runner_workflow_id) = state.runner_workflow_id else {
            return Ok(Loop::Continue);
        };
        // ... sets state.going_away = true and sends stop command
    }
}

Question: What happens if multiple GoingAway signals arrive before the first one sets state.going_away = true? The check prevents duplicate stop commands, but should we log when we receive duplicate signals?

Recommendation: Add debug logging when ignoring duplicate GoingAway signals:

} else {
    tracing::debug!("ignoring duplicate GoingAway signal");
}

3. Missing Error Context (packages/pegboard-gateway/src/lib.rs:529-535)

if let Err(err) = self
    .shared_state
    .send_message(request_id, close_message)
    .await
{
    tracing::error!(?err, "error sending close message");
}

Issue: Error is logged but swallowed. Is this intentional? Should this affect the return value?

Recommendation: Add context about why it's safe to ignore this error, or return early if it's critical.

4. Configuration Value Change (packages/gasoline/src/worker.rs:17)

-const PING_INTERVAL: Duration = Duration::from_secs(20);
+const PING_INTERVAL: Duration = Duration::from_secs(10);

Question: Why was ping interval reduced from 20s to 10s? This doubles the ping frequency. Is this related to the hibernation feature or a separate optimization?

Recommendation: Add a comment explaining the rationale or mention in PR description.

Performance Considerations 🚀

Positive:

Efficient hibernation: Instead of keeping connections alive during actor downtime, hibernation reduces resource usage
Proper backoff: Retry logic with calculate_backoff prevents thundering herd
Peekable stream: Good use of Peekable on WebSocket receiver to avoid consuming messages during hibernation

Concerns:

Ping interval: Halving PING_INTERVAL (20s → 10s) doubles the ping traffic. Ensure this doesn't negatively impact system load at scale.

Security Concerns 🔒

Generally Secure:

Input validation: Headers and request data are properly validated
CORS handling: Proper CORS implementation with origin echoing and credentials support
Timeout protection: TUNNEL_ACK_TIMEOUT and HIBERNATION_TIMEOUT prevent indefinite hangs

Minor Concerns:

Origin validation: Current CORS implementation accepts any origin ("*" fallback). Consider if this is intended for all use cases.
Close frame handling: The code trusts close frames from both client and server without additional validation (likely fine, but worth noting).

Test Coverage ✅

Excellent Coverage:

✅ Basic WebSocket functionality
✅ Hibernation flow with message queueing
✅ Multiple concurrent requests/connections
✅ Custom serve HTTP and WebSocket paths

Gaps:

❌ No test for hibernation with close frame: The code handles ws_hibernation_close but there's no test validating this path
❌ No test for error cases: Timeout scenarios, connection failures during hibernation
❌ No test for actor lifecycle integration: Tests are isolated to guard-core, missing integration with pegboard

Recommendation: Add tests for:

#[tokio::test]
async fn test_websocket_hibernation_with_client_close() {
    // Send close during hibernation, verify proper cleanup
}

#[tokio::test]
async fn test_websocket_hibernation_timeout() {
    // Verify timeout handling during hibernation
}

Specific File Comments

`packages/pegboard/src/workflows/actor/mod.rs`

Line 472-508: Good defensive programming with generation checks
Line 580-735: handle_stopped is well-structured but quite long. Consider extracting sub-functions for readability (not blocking, minor suggestion)

`packages/guard-core/src/proxy_service.rs`

Line 1833-2038: The custom serve WebSocket handling is complex but well-documented with tracing
Line 1898-1959: Retry/hibernation logic is intricate - excellent candidate for unit tests

`packages/pegboard-gateway/src/lib.rs`

Line 554-587: handle_websocket_hibernation is clean and focused
Line 590-612: hibernate_ws helper is well-designed with proper stream handling

Recommendations Summary

Must Fix:

Clarify the "unreachable" check in proxy_service.rs:1911-1920

Should Consider:

Add debug logging for duplicate GoingAway signals
Document why the ping interval was halved
Add test coverage for hibernation edge cases (close during hibernation, timeouts)
Add comment explaining why close message send errors are safely ignored

Nice to Have:

Extract sub-functions from handle_stopped for improved readability
Add integration tests between guard and pegboard components

Overall Assessment ⭐

Quality: 8.5/10
Test Coverage: 7/10
Documentation: 7.5/10

This is a well-architected PR that introduces a valuable feature (WebSocket hibernation) while fixing an important bug (actor reallocation). The code follows project conventions, has good error handling, and includes tests. The main areas for improvement are around edge case testing and clarifying a few implementation details.

Recommendation: ✅ Approve with minor changes

Great work on the hibernation feature - this is a non-trivial addition that's been implemented thoughtfully!

claude · 2025-11-13T22:48:08Z

Pull Request Review

Overview

This PR addresses a critical issue where actors were being immediately reallocated when their runner was stopping, which could lead to unnecessary churn and resource waste. The fix introduces a websocket hibernation mechanism to gracefully handle runner transitions.

Code Quality & Architecture

✅ Strong Points

Excellent naming refactor: The rename from wake_* to bump_* throughout the gasoline package is much clearer and better communicates intent:
- wake_worker → bump_workers
- wake_sub → bump_sub
- WORKER_WAKE_SUBJECT → WORKER_BUMP_SUBJECT
Well-designed hibernation pattern: The new handle_websocket_hibernation method provides a clean extension point for custom WebSocket lifecycle management.
Comprehensive test coverage: The new test test_custom_serve_websocket_hibernation properly validates the hibernation flow (lines 260-320 in custom_serve.rs).
Good error handling improvements: Changed from generic retry error to specific WebSocketServiceHibernate error provides clearer semantics.

Potential Issues & Concerns

🔴 Critical

1. Race condition in hibernation logic (proxy_service.rs:1914-1916)

ensure!(
    !ws_hibernation_close,
    "should not be hibernating again after receiving a close frame during hibernation"
);

This ensure! will panic if triggered. While the comment suggests it's unreachable, race conditions in distributed systems can make "unreachable" states reachable. Consider:

Using a warning log + graceful handling instead of panic
Or adding telemetry to monitor if this ever triggers in production

2. Missing timeout on hibernation (custom_serve.rs:115)

tokio::time::sleep(HIBERNATION_TIMEOUT).await;

The test implementation sleeps for 2 seconds, but there's no timeout enforcement in the actual handle_websocket_hibernation trait. A misbehaving implementation could hang indefinitely. Consider adding a timeout wrapper in proxy_service.rs.

3. Unbounded retry loop potential (proxy_service.rs:1850-2032)
The hibernation path resets attempts = 0 on line 1905, which could theoretically lead to infinite loops if hibernation repeatedly fails. Consider adding a separate hibernation attempt counter.

⚠️ Medium Priority

4. Peekable stream overhead (websocket_handle.rs:11, 30)

pub type WebSocketReceiver =
    Peekable<futures_util::stream::SplitStream<WebSocketStream<TokioIo<Upgraded>>>>;

The change to Peekable adds buffering overhead. While this enables hibernation, consider:

Documenting why this change was necessary
Measuring the performance impact on high-throughput WebSocket connections

5. PING_INTERVAL reduction (worker.rs:17)

-const PING_INTERVAL: Duration = Duration::from_secs(20);
+const PING_INTERVAL: Duration = Duration::from_secs(10);

Halving the ping interval doubles the ping traffic. This change isn't mentioned in the PR description. Was this intentional? What problem does it solve?

6. Deallocate timing in actor lifecycle (actor/mod.rs:608-638)
The handle_stopped function now deallocates before allocating pending actors. While this makes logical sense, the ordering change could affect race conditions. Consider:

Adding a comment explaining why this ordering is important
Verifying this doesn't cause issues with serverless autoscaling

💡 Minor Suggestions

7. Default trait implementation could be more ergonomic (custom_serve.rs:43-47)

async fn handle_websocket_hibernation(
    &self,
    _websocket: WebSocketHandle,
) -> Result<HibernationResult> {
    bail!("service does not support websocket hibernation");
}

Consider returning a more specific error type that downstream can match on, rather than a string-based bail.

8. Inconsistent error handling in test (custom_serve.rs:89-91)

if text == "hibernate" {
    return Err(WebSocketServiceHibernate.build());
}

Using errors for control flow is idiomatic in Rust, but consider if a dedicated ControlFlow return type might be clearer.

9. CLAUDE.md update (CLAUDE.md:128)
The anyhow import style change is good, but unrelated to the PR's main purpose. Consider separating documentation updates into their own commits.

Performance Considerations

Reduced GC timeout pressure: By not immediately reallocating actors, this should reduce unnecessary work during runner transitions. ✅
Increased ping frequency: The PING_INTERVAL reduction will increase network traffic by 2x. Monitor this in production.
Peekable stream buffering: Minor overhead added. Measure impact on high-throughput scenarios.

Security Concerns

No obvious security issues ✅
Error messages don't leak sensitive info ✅
Timeout handling: Consider adding max hibernation duration to prevent resource exhaustion

Testing

✅ Good Coverage

New hibernation test validates happy path
Existing tests appear to still pass

❌ Missing Coverage

No test for the ws_hibernation_close edge case
No test for hibernation timeout behavior
No test for multiple consecutive hibernations
No load testing for the reduced PING_INTERVAL

Recommendations

Before Merge:

Add timeout enforcement for handle_websocket_hibernation calls
Add bounded retry counter for hibernation attempts
Document the PING_INTERVAL change in commit message
Consider changing the ensure! at line 1914 to a logged warning

Post-Merge:

Monitor ping traffic increase in production
Add metrics for hibernation events
Consider adding alerts for the "unreachable" hibernation close case

Summary

This is a well-architected solution to a real problem. The hibernation pattern is clean and extensible. Main concerns are around edge case handling and the lack of hibernation timeouts. With a few safety improvements, this should be good to merge.

Overall Rating: 7/10 - Good solution, needs minor safety improvements

Review generated with assistance from Claude Code

MasterPtato requested a review from NathanFlurry November 7, 2025 22:49

MasterPtato mentioned this pull request Nov 7, 2025

feat(gateway): websocket hibernation #3400

Merged

MasterPtato mentioned this pull request Nov 7, 2025

chore(gas): rename wake sub to bump sub #3408

Merged

vercel bot deployed to Preview – rivetkit-serverless November 7, 2025 22:51 View deployment

MasterPtato mentioned this pull request Nov 8, 2025

feat(gas): implement smart pulling for horizontally scaled workers #3417

Merged

NathanFlurry approved these changes Nov 8, 2025

View reviewed changes

MasterPtato force-pushed the 11-07-fix_pb_dont_immediately_reallocate_actors_on_runner_stopping branch from 79bbca9 to 87f1c1a Compare November 10, 2025 22:49

MasterPtato force-pushed the 11-07-chore_gas_rename_wake_sub_to_bump_sub branch from c0c9897 to 44cfde4 Compare November 10, 2025 22:50

MasterPtato requested a review from NathanFlurry November 10, 2025 22:50

MasterPtato mentioned this pull request Nov 10, 2025

fix: improve sigterm handling for the entire runtime #3443

Merged

vercel bot deployed to Preview – rivetkit-serverless November 10, 2025 22:51 View deployment

This was referenced Nov 11, 2025

fix(guard): handle websocket tasks during shutdown #3448

Merged

fix(guard): dont send open msg for hibernating ws, hibernating ws keepalive #3449

Merged

NathanFlurry force-pushed the 11-07-chore_gas_rename_wake_sub_to_bump_sub branch from 44cfde4 to 851e83c Compare November 13, 2025 17:20

NathanFlurry force-pushed the 11-07-fix_pb_dont_immediately_reallocate_actors_on_runner_stopping branch from 87f1c1a to 378e6a2 Compare November 13, 2025 17:20

vercel bot had a problem deploying to Preview – rivetkit-serverless November 13, 2025 17:20 Failure

NathanFlurry mentioned this pull request Nov 13, 2025

chore(rivetkit): fix type checks #3462

Merged

NathanFlurry force-pushed the 11-07-fix_pb_dont_immediately_reallocate_actors_on_runner_stopping branch from 378e6a2 to f5922d4 Compare November 13, 2025 20:53

vercel bot had a problem deploying to Preview – rivetkit-serverless November 13, 2025 20:54 Failure

NathanFlurry mentioned this pull request Nov 13, 2025

chore(rivetkit): implement new hibernating ws protocol #3464

Open

NathanFlurry force-pushed the 11-07-fix_pb_dont_immediately_reallocate_actors_on_runner_stopping branch from f5922d4 to 0bfb5d7 Compare November 13, 2025 21:39

NathanFlurry force-pushed the 11-07-chore_gas_rename_wake_sub_to_bump_sub branch from 47e1dfc to 05fd9f6 Compare November 13, 2025 21:39

vercel bot had a problem deploying to Preview – rivetkit-serverless November 13, 2025 21:40 Failure

NathanFlurry force-pushed the 11-07-fix_pb_dont_immediately_reallocate_actors_on_runner_stopping branch from 0bfb5d7 to 7ab4b5a Compare November 13, 2025 22:27

NathanFlurry force-pushed the 11-07-chore_gas_rename_wake_sub_to_bump_sub branch from 05fd9f6 to 4b7b6b5 Compare November 13, 2025 22:27

vercel bot had a problem deploying to Preview – rivetkit-serverless November 13, 2025 22:28 Failure

NathanFlurry force-pushed the 11-07-fix_pb_dont_immediately_reallocate_actors_on_runner_stopping branch from 7ab4b5a to ca5973c Compare November 13, 2025 22:35

NathanFlurry force-pushed the 11-07-chore_gas_rename_wake_sub_to_bump_sub branch from 4b7b6b5 to 70e0557 Compare November 13, 2025 22:35

vercel bot had a problem deploying to Preview – rivetkit-serverless November 13, 2025 22:35 Failure

MasterPtato added 3 commits November 13, 2025 14:40

feat(gateway): websocket hibernation

657d42e

chore(gas): rename wake sub to bump sub

277e2db

fix(pb): dont immediately reallocate actors on runner stopping

86c0f99

NathanFlurry force-pushed the 11-07-fix_pb_dont_immediately_reallocate_actors_on_runner_stopping branch from ca5973c to 86c0f99 Compare November 13, 2025 22:42

NathanFlurry force-pushed the 11-07-chore_gas_rename_wake_sub_to_bump_sub branch from 70e0557 to 277e2db Compare November 13, 2025 22:42

vercel bot had a problem deploying to Preview – rivetkit-serverless November 13, 2025 22:42 Failure

Base automatically changed from 11-07-chore_gas_rename_wake_sub_to_bump_sub to main November 13, 2025 22:44

NathanFlurry merged commit f27177c into main Nov 13, 2025
7 of 10 checks passed

NathanFlurry deleted the 11-07-fix_pb_dont_immediately_reallocate_actors_on_runner_stopping branch November 13, 2025 22:45

fix(pb): dont immediately reallocate actors on runner stopping #3413

fix(pb): dont immediately reallocate actors on runner stopping #3413

Uh oh!

Conversation

MasterPtato commented Nov 7, 2025

Uh oh!

vercel bot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MasterPtato commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

claude bot commented Nov 7, 2025

PR Review: Fix actor reallocation on runner stopping

Summary

Positive Changes

Architecture Improvements

Code Quality

Issues and Concerns

1. Potential Race Condition in GoingAway Handler (High Priority)

2. Missing State Transitions (Medium Priority)

3. Generation Mismatch Edge Case (Low Priority)

4. Typo in Comment (Trivial)

Performance Considerations

Security Concerns

Test Coverage

Code Style

Summary

Recommendations:

Uh oh!

pkg-pr-new bot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Nov 10, 2025

PR Review: fix(pb): dont immediately reallocate actors on runner stopping

Summary

Code Quality & Best Practices

Potential Bugs & Issues

Logic & Correctness

Performance Considerations

Security Concerns

Test Coverage

Specific Code Issues

mod.rs:637-640 - Unclear Logic

mod.rs:680-686 - Inconsistent Error Handling

runtime.rs:32-34 - State Flag Proliferation

Recommendations

Verdict

Uh oh!

claude bot commented Nov 10, 2025

Code Review: Fix Pegboard Actor Immediate Reallocation on Runner Stopping

Overview

Positive Changes ✅

Concerns & Issues 🔍

Potential Bugs 🐛

Performance Considerations ⚡

Security Concerns 🔒

Recommendations 📋

Architecture Suggestion 💡

Verdict

Uh oh!

claude bot commented Nov 13, 2025

Code Review

Overview

Key Changes Summary

Positive Aspects

Architecture & Design

Code Quality

Concerns & Suggestions

1. State Management Complexity ⚠️

2. Missing State Reset Logic ⚠️

3. Race Condition Potential 🔴

4. Destroy Signal Behavior 📝

5. Lost Actor Stop Command Logic 🤔

6. Force Reschedule Logic Simplification ✅

7. Test Coverage ❓

8. Documentation 📖

9. Potential Cleanup Opportunities 🧹

Security Considerations

vercel bot commented Nov 7, 2025 •

edited

Loading

MasterPtato commented Nov 7, 2025 •

edited

Loading

1. Potential Race Condition in `GoingAway` Handler (High Priority)

pkg-pr-new bot commented Nov 7, 2025 •

edited

Loading

`mod.rs:637-640` - Unclear Logic

`mod.rs:680-686` - Inconsistent Error Handling

`runtime.rs:32-34` - State Flag Proliferation

1. Potential Race Condition in `GoingAway` Handler ⚠️

4. Inconsistent `reset_rescheduling` Handling