Skip to content

fix(pb): handle going away signals gracefully#4104

Closed
MasterPtato wants to merge 1 commit into02-03-fix_pb_fix_metrics_wf_inconsistent_calculationfrom
02-03-fix_pb_handle_going_away_signals_gracefully
Closed

fix(pb): handle going away signals gracefully#4104
MasterPtato wants to merge 1 commit into02-03-fix_pb_fix_metrics_wf_inconsistent_calculationfrom
02-03-fix_pb_handle_going_away_signals_gracefully

Conversation

@MasterPtato
Copy link
Contributor

@MasterPtato MasterPtato commented Feb 3, 2026

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

This was referenced Feb 3, 2026
Closed
Closed
Closed
Closed
Closed
Closed
@MasterPtato Graphite App
Copy link
Contributor Author

MasterPtato commented Feb 3, 2026 •
edited
Loading

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.

@MasterPtato MasterPtato mentioned this pull request Feb 3, 2026
Closed
11 tasks
@claude
Copy link

claude bot commented Feb 3, 2026 •
edited
Loading

Code Review - PR #4104: fix(pb): handle going away signals gracefully

Summary

This PR improves error handling in the Pegboard runner workflows by using graceful_not_found() when sending GoingAway signals to actor workflows during runner shutdown. The changes handle race conditions where actor workflows may have already stopped before receiving the signal.

✅ Positive Aspects

  1. Addresses Real Race Condition: The change properly handles the scenario where an actor workflow completes/stops before the runner sends the GoingAway signal during shutdown.

  2. Consistent Pattern: Uses the same .graceful_not_found() pattern that is already used throughout the codebase (I found 20+ other usages in pegboard, guard, and api-peer packages).

  3. Appropriate Logging: Adds warning logs when workflows are not found, which helps with debugging while not treating this as an error condition.

  4. Follows Structured Logging Convention: The log message correctly uses ?actor_id for structured logging rather than formatting it into the message string (per CLAUDE.md guidelines).

  5. Identical Fix in Both Files: The fix is consistently applied to both runner.rs and runner2.rs files.

🔍 Observations

Code Quality: The implementation is clean and follows existing patterns in the codebase.

Performance: Negligible impact. The only change is error handling behavior.

Security: No security concerns.

Test Coverage: The PR description checklist indicates tests may not have been added. Consider adding a test case that:

  • Creates a runner with actors
  • Has some actors complete before the runner stops
  • Verifies that the GoingAway signals are sent gracefully without errors

📝 Minor Suggestions

  1. PR Description: The PR description template was not fully filled out. For future PRs, it would be helpful to:

    • Check the appropriate "Type of change" boxes
    • Describe how the change was tested
    • Complete the checklist

  2. Comment Clarity: Consider slightly improving the comment to explain why graceful_not_found() is necessary:

    // Set all remaining actors as going away immediately. Use graceful_not_found since
// actors may have already stopped before we send the signal.

    This helps future developers understand the race condition being handled.

✅ Verdict

Approved - This is a solid bug fix that improves the robustness of the shutdown process. The changes are minimal, focused, and follow established patterns in the codebase. The fix correctly handles the race condition where actor workflows may complete before receiving the shutdown signal.

The code follows the project conventions (structured logging, error handling patterns) and makes appropriate use of the workflow engine graceful_not_found feature.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Feb 3, 2026

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@4104

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 4f499de

@MasterPtato MasterPtato force-pushed the 02-03-fix_pb_fix_metrics_wf_inconsistent_calculation branch from 28d418c to b549fa5 Compare February 3, 2026 21:30
@MasterPtato MasterPtato force-pushed the 02-03-fix_pb_handle_going_away_signals_gracefully branch from 4f499de to 2179979 Compare February 3, 2026 21:30
This was referenced Feb 3, 2026
Closed
Closed
@MasterPtato MasterPtato force-pushed the 02-03-fix_pb_fix_metrics_wf_inconsistent_calculation branch from b549fa5 to 3b2e149 Compare February 4, 2026 18:49
@MasterPtato MasterPtato force-pushed the 02-03-fix_pb_handle_going_away_signals_gracefully branch from 2179979 to 3c851e9 Compare February 4, 2026 18:49
@graphite-app
Copy link
Contributor

graphite-app bot commented Feb 4, 2026 •
edited
Loading

Merge activity

  • Feb 4, 6:50 PM UTC: MasterPtato added this pull request to the Graphite merge queue.
  • Feb 4, 6:51 PM UTC: CI is running for this pull request on a draft pull request (#4113) due to your merge queue CI optimization settings.
  • Feb 4, 6:51 PM UTC: Merged by the Graphite merge queue via draft PR: #4113.

graphite-app bot pushed a commit that referenced this pull request Feb 4, 2026
@MasterPtato
fix(pb): handle going away signals gracefully (#4104)
a3a7dcd 
# Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

## Type of change

- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update

## How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

## Checklist:

- [ ] My code follows the style guidelines of this project
- [ ] I have performed a self-review of my code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] New and existing unit tests pass locally with my changes
@graphite-app graphite-app bot closed this Feb 4, 2026
@graphite-app graphite-app bot deleted the 02-03-fix_pb_handle_going_away_signals_gracefully branch February 4, 2026 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NathanFlurry NathanFlurry Awaiting requested review from NathanFlurry

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@MasterPtato