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

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-site	Ready	Preview	Comment	Aug 21, 2025 8:41pm

Claude finished @MasterPtato's task —— View job

PR Review: Clean up worker threads in the event of an error

Review Checklist

• Read and analyze the changed file
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Consider test coverage needs
• Provide comprehensive feedback

Changes Summary

This PR addresses proper cleanup of background tasks when the worker encounters errors. The main changes are:

1. Added background task cleanup (gc_handle.abort(), metrics_handle.abort()) when subscription becomes unsubscribed
2. Added error handling around self.tick() call with proper cleanup on errors
3. Reordered tokio::select! arms for clearer flow
4. Added clarifying comment about background task cleanup

✅ Positive Aspects

Excellent Bug Fix: This addresses a real resource leak issue where background GC and metrics tasks would continue running after the main worker loop exits due to errors.

Consistent Error Handling: The fix applies the same cleanup pattern across all error exit paths (wake_sub.next() == None, self.tick() errors), ensuring consistent behavior.

Clear Code Organization: Reordering the tokio::select! arms puts the subscription handling before background task monitoring, which is more logical.

Good Documentation: The added comment at packages/common/chirp-workflow/core/src/worker.rs:195 clearly explains why rivet_runtime::shutdown() stops background tasks.

🔍 Code Quality Analysis

Rust Best Practices: ✅

• Proper use of JoinHandle::abort() for task cancellation
• Consistent error propagation with ? operator and explicit Err() returns
• Appropriate use of tokio::select! for concurrent operations

Error Handling: ✅

• All error paths now properly clean up resources
• Error messages are preserved and bubbled up correctly
• No silent failures or resource leaks

🐛 Potential Issues

Minor: Duplication: The cleanup code (gc_handle.abort(); metrics_handle.abort()) is now duplicated in two places. Consider extracting to a helper method:

impl Worker {
    fn abort_background_tasks(&self, gc_handle: &JoinHandle<()>, metrics_handle: &JoinHandle<()>) {
        gc_handle.abort();
        metrics_handle.abort();
    }
}

Edge Case: If gc_handle.abort() or metrics_handle.abort() panic (unlikely but theoretically possible), the second abort wouldn't execute. This is very minor since abort() is typically infallible.

🚀 Performance Considerations

Resource Cleanup: ✅ Improves performance by preventing background tasks from continuing unnecessarily after errors.

No Performance Regression: The changes add minimal overhead (two function calls) only in error paths.

🔒 Security Assessment

No Security Issues: This is defensive cleanup code that improves resource management. No security implications.

🧪 Test Coverage Recommendations

Consider adding tests for:

1. Subscription failure cleanup: Verify that when wake_sub.next() returns None, both background tasks are aborted
2. Tick error cleanup: Verify that when self.tick() returns an error, background tasks are aborted
3. Task handle state: Confirm that after cleanup, the handles report as finished/aborted

Example test structure:

#[tokio::test]
async fn test_background_task_cleanup_on_tick_error() {
    // Setup worker with mocked dependencies that will cause tick() to fail
    // Verify gc_handle.is_finished() and metrics_handle.is_finished() return true
}

📋 Overall Assessment

Strong Approve ✅

This is a well-implemented bug fix that addresses a real resource management issue. The changes are minimal, focused, and follow Rust best practices. The only suggestion is the minor code deduplication opportunity mentioned above.

Files Changed: packages/common/chirp-workflow/core/src/worker.rs:86-116

• fix: get pb manager test working e2e again #2721
• fix: convert runner protocol to protobuf #2702
• fix: even more actor api changes #2714
• chore: flatten actor api spec #2698
• fix: increase default actor kill timeout to 5s #2701
• fix: dont clear ingress ports on reschedule #2700
• fix: add cpu constraints to oci config #2697
• fix: api changes #2692
• fix: fix logs for new actors #2649
• feat: add actor queue #2644
• feat: add actor kv to runners #2638
• feat: get multi actors working e2e on docker compose #2637
• fix: add udp ports to docker compose #2635
• chore: fix merge conflicts #2634
• fix: get new actors working on docker compose #2623
• fix: figure out actor log shipping #2622
• fix(pegboard): convert ws manager socket to unix socket #2478
• fix(pb): get new actor ids working e2e #2471
• feat: implement new actor ids #2467
• feat(pb): get multi-actors working e2e, add resources for builds #2465
• feat(pb): add runner logs #2447
• feat: add runners to pb protocol #2433
• chore(pegboard): add exit code log from container runner #2477
• fix(workflows): fix branch version bug #2475
• chore(pegboard): replace computing image size manually with using tar bytes read #2625
• fix: make pb image downloads random, remove prewarm from pb protocol #2784
• fix: clean up worker threads in the event of an error #2783 👈 (View in Graphite)
• fix: make query_as macros retry #2773
• main

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.

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

Deploying rivet with    Cloudflare Pages

View logs

Claude encountered an error —— View job

I'll analyze this and get back to you.

Deploying rivet-studio with    Cloudflare Pages

View logs

Deploying rivet-hub with    Cloudflare Pages

View logs