fix: clean up worker threads in the event of an error

[MasterPtato]

Changes

[vercel]

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]

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

---

[MasterPtato]

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

• 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.

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.

[cloudflare-workers-and-pages]

Deploying rivet with    Cloudflare Pages

Latest commit:	17d427c
Status:	🚫  Build failed.

View logs

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[graphite-app]

The error message in this line is incorrect - it's logging "metrics task unexpectedly stopped" when handling the result from gc_handle. This should be updated to "gc task unexpectedly stopped" to accurately reflect which task failed.

Suggested change

	tracing::error!(?res, "metrics task unexpectedly stopped");
	tracing::error!(?res, "gc task unexpectedly stopped");

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.

[cloudflare-workers-and-pages]

Deploying rivet-studio with    Cloudflare Pages

Latest commit:	17d427c
Status:	🚫  Build failed.

View logs

[cloudflare-workers-and-pages]

Deploying rivet-hub with    Cloudflare Pages

Latest commit:	17d427c
Status:	✅  Deploy successful!
Preview URL:	https://f0ac695a.rivet-hub-7jb.pages.dev
Branch Preview URL:	https://07-21-fix-clean-up-worker-th.rivet-hub-7jb.pages.dev

View logs