Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 3, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Ready Ready Preview Comment Nov 4, 2025 7:25pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 4, 2025 7:25pm
rivet-inspector Ignored Ignored Preview Nov 4, 2025 7:25pm
rivet-site Ignored Ignored Preview Nov 4, 2025 7:25pm

Copy link
Member Author

NathanFlurry commented Nov 3, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


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
Copy link

claude bot commented Nov 3, 2025

PR Review: Fix duplicate onActorStop call

Summary

This PR correctly removes a duplicate onActorStop callback invocation in the forceStopActor method. Good catch!

Analysis

What was happening:
The forceStopActor method at line 162-174 was calling onActorStop twice:

  1. First at line 168: await this.#config.onActorStop(actorId, actor.generation) with proper try-catch
  2. Then again in the removed code (after #sendActorStateUpdate): this.#config.onActorStop(...).catch(...)

Why this is correct:

  • The first call at line 168 is the proper implementation - it awaits the callback and handles errors appropriately
  • The duplicate call would cause the user's onActorStop handler to be invoked twice for the same actor stop event
  • Removing the second call ensures the callback is only invoked once per actor stop

Code Quality: ✅ Excellent

Strengths:

  • Clean, focused fix addressing exactly one issue
  • Maintains the proper error handling in the remaining onActorStop call
  • The #sendActorStateUpdate call correctly remains after onActorStop completes
  • Preserves the comment explaining Pegboard's timeout handling

Observations:

  • Line 170: Uses console.error instead of the structured logger. For consistency with the codebase's logging patterns (per CLAUDE.md), consider:
    logger()?.error({
      msg: "error in onactorstop for actor",
      runnerId: this.runnerId,
      actorId,
      err,
    });
    (This is a minor style suggestion for a future PR, not a blocker for this fix)

Testing Considerations

  • ✅ No test changes needed - this is purely fixing incorrect behavior
  • Consider manual testing: Verify that onActorStop callbacks are only invoked once when actors are stopped
  • Edge case: Ensure the fix works correctly when onActorStop throws an error (should be caught by the try-catch at line 169)

Security: ✅ No concerns

Performance: ✅ Improvement

  • Eliminates redundant async callback invocation
  • Reduces unnecessary work during actor shutdown

Verdict: APPROVE

This is a straightforward bug fix that correctly removes duplicate callback invocation. The remaining implementation properly handles the onActorStop callback with appropriate error handling.

Recommendation: Merge after CI passes.

@NathanFlurry NathanFlurry force-pushed the 11-02-chore_add_rivetkit_test_writer_agent branch from 76fdd5c to e9df1f3 Compare November 4, 2025 00:36
@NathanFlurry NathanFlurry force-pushed the 11-03-fix_runner_dupe_onactorstop_call branch from b62cf5d to 4fb3477 Compare November 4, 2025 00:36
@claude
Copy link

claude bot commented Nov 4, 2025

Code Review for PR #3321: fix(runner): dupe onActorStop call

Summary

This PR removes a duplicate call to onActorStop in the forceStopActor method. The duplicate code was calling the callback after sending the actor state update, but the callback is already called earlier in the same method.

Analysis

The Fix:
The removed code (lines 175-184) was a duplicate invocation of onActorStop that occurred after the state update was sent. Looking at the forceStopActor method at engine/sdks/typescript/runner/src/mod.ts:162-174, we can see the correct flow:

  1. Line 163: Remove the actor from the actors map
  2. Lines 167-171: Call onActorStop (with proper error handling)
  3. Line 173: Send actor state update to "stopped"

The removed duplicate code was incorrectly calling onActorStop again after step 3, which would cause the callback to execute twice for every forced actor stop.

Code Quality: ✅ Excellent

Positive aspects:

  • The fix is surgical and correct - removing only the duplicate code
  • The remaining onActorStop call (lines 167-171) has proper error handling with try/catch
  • The code follows the established pattern in the codebase
  • The fix aligns with the single-responsibility principle

Potential Issues: ✅ None Found

No bugs detected:

  • The remaining onActorStop call is in the correct location (before the state update)
  • Error handling is properly implemented with try/catch
  • The method correctly removes the actor from tracking before calling the callback
  • The state update is sent after cleanup, which is the correct sequence

Performance: ✅ Improved

Impact:

  • Positive: Eliminates redundant callback execution, reducing unnecessary work
  • This fix prevents the callback from running twice on every actor stop, which could have performance implications if the callback performs expensive operations
  • The removal of the duplicate also improves code clarity and maintainability

Security: ✅ No Concerns

No security implications from this change.

Test Coverage: ⚠️ Consider Adding Test

Recommendation:
While the fix is clearly correct, consider adding a test case to ensure onActorStop is called exactly once per actor stop. This would prevent future regressions.

Example test approach (in engine/sdks/typescript/runner/tests/lifecycle.test.ts):

it("calls onActorStop exactly once per actor", async () => {
  let callCount = 0;
  const config: RunnerConfig = {
    // ... config setup
    onActorStop: async (actorId, generation) => {
      callCount++;
    },
  };
  
  // ... create and force stop actor
  expect(callCount).toBe(1);
});

Additional Context

The forceStopActor method is called from:

  1. #stopAllActors() (line 184) - when runner lost threshold is exceeded
  2. #handleCommandStopActor() (line 800) - when receiving stop command from server
  3. #handleCommandStartActor() error handler (line 789) - when actor start fails

All of these call sites would have been affected by the duplicate callback execution.

Conclusion

✅ APPROVED - This is a clean, correct bug fix that removes a duplicate callback invocation. The change improves both performance and code correctness.

Recommendation: Merge after optionally adding test coverage to prevent future regressions.


🤖 Generated with Claude Code

@claude
Copy link

claude bot commented Nov 4, 2025

Code Review

Summary

This PR correctly removes a duplicate call to onActorStop in the forceStopActor method. Good catch!

Analysis

The Issue:
Looking at engine/sdks/typescript/runner/src/mod.ts:162-174, the forceStopActor method was calling onActorStop twice:

  1. First call at line 168 (correctly placed, with proper error handling)
  2. Second call removed by this PR (lines 175-183 in the original code) - after sending the state update

This would cause the callback to be invoked twice for every forced actor stop, which could lead to:

  • Duplicate cleanup operations
  • Confusing logs
  • Potential race conditions if the callback has side effects
  • Unnecessary overhead

The Fix:
The PR removes the second (duplicate) call, keeping only the first one which:

  • Is properly awaited
  • Has appropriate error handling with try-catch
  • Is called in the correct sequence (before sending state update)

Verification

The single remaining onActorStop call at line 168 is correct because:

  1. It's called after the actor is removed from internal state (#removeActor)
  2. It's properly awaited and wrapped in error handling
  3. It's followed by the state update notification to the server
  4. This matches the expected lifecycle: remove → cleanup → notify

Additional Observations

Code Quality: The change is minimal and surgical - only removes the duplicate code
Error Handling: The remaining call has proper error handling
Consistency: The pattern now matches other lifecycle methods
No Breaking Changes: External behavior is unchanged (still calls onActorStop once as expected)

Test Coverage

While the lifecycle tests are currently commented out, when re-enabled they should verify:

  • onActorStop is called exactly once per actor stop
  • The callback timing is correct relative to state updates
  • Error handling works properly

Consider adding a specific test case that tracks the number of onActorStop invocations to prevent regression of this issue.

Verdict

LGTM ✓ - This is a correct bug fix that removes duplicate callback invocations. No issues found.

@claude
Copy link

claude bot commented Nov 4, 2025

Pull Request Review

Summary

This PR fixes a bug where onActorStop was being called twice in the forceStopActor method - once correctly at the beginning (line 168) and once incorrectly at the end (the removed lines).

Code Quality ✅

  • Clean fix: The removal is correct and straightforward
  • Maintains error handling: The proper try-catch block around the first onActorStop call (lines 167-171) is preserved
  • Preserves flow: The method still follows the correct sequence:
    1. Remove actor from internal state (#removeActor)
    2. Call the user-defined onActorStop hook
    3. Send actor state update to server

Bug Analysis ✅

The duplicate call was problematic because:

  1. Double execution: User's onActorStop callback would execute twice for the same actor stop event
  2. State inconsistency: The second call happens after #sendActorStateUpdate, meaning the server would be notified the actor stopped before the second cleanup callback runs
  3. Resource waste: Any cleanup operations in onActorStop (closing connections, releasing resources) would be unnecessarily repeated

Error Handling ✅

The remaining onActorStop call (line 168) has proper error handling with a try-catch block that logs errors appropriately. The removed duplicate used a .catch() pattern which would have been less reliable if the Promise chain was already resolved.

Best Practices ✅

  • Single responsibility: Each lifecycle event should trigger its callback exactly once
  • Proper sequencing: The call happens at the correct point in the actor shutdown sequence (after removal from state, before server notification)
  • Timeout handling: Comment on line 166 correctly notes that Pegboard handles timeouts via ACTOR_STOP_THRESHOLD_DURATION_MS

Testing Recommendations

Consider adding/verifying tests that:

  1. Verify onActorStop is called exactly once per actor stop
  2. Test that errors in onActorStop don't prevent the state update from being sent
  3. Verify the correct sequence: remove actor → stop callback → state update

Verdict: LGTM

This is a clean bug fix that removes duplicate functionality. The remaining code properly handles the actor stop lifecycle.

Reviewed according to repository conventions in CLAUDE.md

@claude
Copy link

claude bot commented Nov 4, 2025

Code Review

Summary

This PR removes a duplicate call to onActorStop in the forceStopActor method at engine/sdks/typescript/runner/src/mod.ts:174-182. The duplicate call was causing the actor stop handler to be invoked twice for the same actor lifecycle event.

Analysis

The Issue:
Looking at the forceStopActor method (lines 162-174), the code flow is:

  1. Remove actor from internal state via #removeActor
  2. Call onActorStop handler (line 168)
  3. Send actor state update to server (line 173)
  4. The removed code was calling onActorStop again (lines 175-182)

This is clearly a duplicate call that would cause the stop handler to execute twice for every force stop operation.

Where forceStopActor is called:

  • Line 184: In #stopAllActors when runner lost threshold is exceeded
  • Line 789: In #handleCommandStartActor when actor start fails
  • Line 800: In #handleCommandStopActor when server sends stop command

Positive Observations

  1. Correct Fix: The removed code is indeed a duplicate. The onActorStop callback should only be invoked once per actor stop event.

  2. Proper Error Handling: The remaining onActorStop call (line 168) has proper try-catch error handling, while the removed duplicate used `.catch()" with only error logging.

  3. Consistent Pattern: After the fix, all actor lifecycle events follow a consistent pattern:

    • Update internal state
    • Call lifecycle hook
    • Send state update to server
  4. Comment Context: The comment at line 166 mentions Pegboard timeout handling, indicating this was a thoughtfully designed section.

Concerns & Questions

  1. Test Coverage: The test file tests/lifecycle.test.ts is entirely commented out. This raises concerns about:

    • How was this duplicate call bug discovered?
    • What prevents this regression in the future?
    • Are there other similar issues lurking?
  2. No Explanation in PR: The PR body is empty. For a bug fix, it would be helpful to know:

    • How was this discovered? (Production issue? Code review? Testing?)
    • What symptoms did users experience?
    • Was there any impact from the duplicate calls?
  3. Logging Inconsistency:

    • The remaining call uses console.error (line 170)
    • The removed duplicate used structured logging via logger()?.error (lines 176-181)
    • Consider standardizing on the logging approach used throughout the file
  4. Potential Side Effects: The removed duplicate had a .catch() that would swallow errors. The remaining call has a try-catch but doesn't re-throw. This is actually fine since these are async lifecycle hooks that shouldn't block the state machine, but worth noting.

Recommendations

  1. High Priority: Uncomment and run the lifecycle tests, or create new tests that specifically verify:

    • onActorStop is called exactly once per stop event
    • Actor stop works correctly in all paths (force stop, stop command, start failure)
  2. Medium Priority: Update the PR description to include:

    • How this bug was discovered
    • What impact it had (if any)
    • Why the duplicate was there (copy-paste error? Refactoring artifact?)
  3. Low Priority: Consider standardizing error logging in the onActorStop call at line 170 to use structured logging like the rest of the file:

    } catch (err) {
        logger()?.error({
            msg: "error in onActorStop for actor",
            runnerId: this.runnerId,
            actorId,
            generation: actor.generation,
            err,
        });
    }

Security Considerations

No security concerns identified. This is a straightforward bug fix.

Performance Considerations

This fix actually improves performance by eliminating unnecessary duplicate work.

Verdict

LGTM with recommendations - This is a correct bug fix that removes duplicate callback invocation. The code change itself is solid, but the lack of test coverage and PR documentation are concerning for long-term maintenance.


Review generated by Claude Code

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 4, 2025

Merge activity

  • Nov 4, 8:09 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 4, 8:11 PM UTC: CI is running for this pull request on a draft pull request (#3349) due to your merge queue CI optimization settings.
  • Nov 4, 8:41 PM UTC: The Graphite merge queue removed this pull request due to removal of a downstack PR #3342.
  • Nov 4, 8:49 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 4, 8:50 PM UTC: CI is running for this pull request on a draft pull request (#3352) due to your merge queue CI optimization settings.
  • Nov 4, 8:52 PM UTC: Merged by the Graphite merge queue via draft PR: #3352.

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025
@NathanFlurry NathanFlurry force-pushed the 11-02-chore_add_rivetkit_test_writer_agent branch from 16142c0 to 22d5ec0 Compare November 4, 2025 20:25
@NathanFlurry NathanFlurry force-pushed the 11-03-fix_runner_dupe_onactorstop_call branch from cadd5cb to 0f73c04 Compare November 4, 2025 20:25
@vercel
Copy link

vercel bot commented Nov 4, 2025

Deployment failed with the following error:

Too many requests - try again in 5 minutes (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

@claude
Copy link

claude bot commented Nov 4, 2025

Code Review

Summary

This PR removes a duplicate call to onActorStop in the forceStopActor method. The change is correct and necessary - it fixes a bug where the actor stop callback was being invoked twice.

Analysis

What was wrong:
The forceStopActor method at line 162-174 was calling onActorStop twice:

  1. First call at line 168: await this.#config.onActorStop(actorId, actor.generation) - properly wrapped in try-catch with error handling
  2. Second call (now removed): Line 175-183 - async fire-and-forget with separate error logging

Why this is correct:

  • The first call (line 168) is the proper implementation - it awaits the callback and handles errors appropriately
  • The duplicate call would cause unintended side effects in user code that expects onActorStop to be called exactly once per actor stop
  • The comment at line 166 clearly indicates this is a deliberate stop operation with timeout handling by Pegboard

Code flow verification:

  • forceStopActor is the only place that actually executes the stop callback
  • stopActor (line 152) and sleepActor (line 141) only send intents but don't call the callback
  • The server sends a StopActor command which triggers #handleCommandStopActor (line 793), which then calls forceStopActor
  • This ensures exactly one callback execution per stop operation

Quality Assessment

Positive:

  • ✅ Fixes a clear bug (duplicate callback invocation)
  • ✅ Clean, simple change with no additional logic needed
  • ✅ Maintains proper error handling flow
  • ✅ The remaining implementation correctly handles the stop lifecycle

Considerations:

  • The error handling differs slightly between the two versions:
    • Kept version: Uses console.error with template string
    • Removed version: Used structured logging with logger()?.error
  • The kept version is actually more consistent with being inside a try-catch block
  • Consider whether structured logging would be preferred (though this is minor)

Test Coverage

The test file at engine/sdks/typescript/runner/tests/lifecycle.test.ts is currently commented out but appears to have comprehensive E2E tests that would catch this issue if uncommented. The tests verify:

  • Actor start/stop lifecycle
  • Sleep/wake cycles
  • Callback invocations

Recommendation: Consider uncommenting and running these tests to ensure no regressions.

Security & Performance

  • ✅ No security concerns
  • ✅ Performance improvement (eliminates unnecessary duplicate callback execution)
  • ✅ No breaking changes - this is a pure bug fix

Verdict

LGTM

This is a straightforward bug fix that correctly removes duplicate callback invocation. The change improves correctness without introducing any risks.


Review generated with Claude Code

@claude
Copy link

claude bot commented Nov 4, 2025

Pull Request Review: fix(runner): dupe onActorStop call

Summary

This PR correctly removes a duplicate call to onActorStop in the forceStopActor method. The fix is small, focused, and addresses a clear bug.

Code Quality ✅

Positive aspects:

  • The change is minimal and surgical, removing exactly 9 lines that were causing the duplicate behavior
  • The removed code block was indeed redundant - onActorStop is already called at line 168 in the forceStopActor method
  • Follows the single responsibility principle - the method now has one clear call to onActorStop followed by the state update

Before the fix:

async forceStopActor(actorId: string, generation?: number) {
    const actor = this.#removeActor(actorId, generation);
    if (!actor) return;

    // First call (correct location)
    await this.#config.onActorStop(actorId, actor.generation);
    
    this.#sendActorStateUpdate(actorId, actor.generation, "stopped");

    // Second call (duplicate - now removed) ❌
    this.#config.onActorStop(actorId, actor.generation).catch((err) => {
        logger()?.error({...});
    });
}

After the fix:

async forceStopActor(actorId: string, generation?: number) {
    const actor = this.#removeActor(actorId, generation);
    if (!actor) return;

    await this.#config.onActorStop(actorId, actor.generation);
    
    this.#sendActorStateUpdate(actorId, actor.generation, "stopped");
}

Potential Issues & Considerations

1. Error Handling Difference ⚠️

The removed code had a .catch() handler that logged errors without throwing them:

.catch((err) => {
    logger()?.error({
        msg: "error in onactorstop for actor",
        runnerId: this.runnerId,
        actorId,
        err,
    });
});

However, the remaining call (line 168) is wrapped in a try-catch block that uses console.error instead of the structured logger:

try {
    await this.#config.onActorStop(actorId, actor.generation);
} catch (err) {
    console.error(`Error in onActorStop for actor ${actorId}:`, err);
}

Suggestion: Consider using the structured logger for consistency with the rest of the codebase:

} catch (err) {
    logger()?.error({
        msg: "error in onActorStop for actor",
        runnerId: this.runnerId,
        actorId,
        err,
    });
}

2. Logging Pattern Inconsistency

The removed code used lowercase "onactorstop" in the log message, while other parts of the codebase use "onActorStop" (line 781). Per the CLAUDE.md guidelines:

Log messages should be lowercase unless mentioning specific code symbols.

The current console.error doesn't follow this pattern. Consider:

logger()?.error({
    msg: "error in actor stop callback",
    runnerId: this.runnerId,
    actorId,
    err,
});

Test Coverage ⚠️

The test file lifecycle.test.ts is currently completely commented out. While this PR fixes a clear bug, there's no active test coverage to:

  • Verify onActorStop is called exactly once per actor stop
  • Ensure no regression of this duplicate call bug
  • Validate the complete actor lifecycle

Recommendation: Consider adding or uncommenting tests that verify onActorStop is called exactly once when an actor is stopped.

Performance Considerations ✅

This fix has positive performance implications:

  • Eliminates redundant callback execution
  • Reduces unnecessary async operations
  • Prevents duplicate logging in error scenarios

Security Concerns ✅

No security concerns identified. The change maintains the same security posture as before while fixing the duplication issue.

Overall Assessment

Verdict:APPROVED - This is a good, focused bug fix.

The PR successfully removes a duplicate callback that could have caused:

  • Confusion in logs (duplicate stop messages)
  • Potential side effects if onActorStop had non-idempotent operations
  • Unnecessary performance overhead

Minor improvement suggestions:

  1. Update error logging to use structured logger instead of console.error
  2. Consider following the lowercase logging convention for non-symbol references
  3. Add test coverage to prevent regression

Great catch on finding this duplicate call! 🎯

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025
@graphite-app graphite-app bot closed this Nov 4, 2025
@graphite-app graphite-app bot deleted the 11-03-fix_runner_dupe_onactorstop_call branch November 4, 2025 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants