Skip to content

Conversation

@abcxff
Copy link
Contributor

@abcxff abcxff commented Dec 11, 2025

No description provided.

@vercel
Copy link

vercel bot commented Dec 11, 2025

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

4 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-cloud Ignored Ignored Dec 25, 2025 2:49am
rivet-inspector Ignored Ignored Preview Dec 25, 2025 2:49am
rivet-site Ignored Ignored Preview Dec 25, 2025 2:49am
rivetkit-serverless Skipped Skipped Dec 25, 2025 2:49am

@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless December 11, 2025 01:09 Inactive
Copy link
Contributor Author

abcxff commented Dec 11, 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 Dec 11, 2025

PR Review: Actor Alarms Test Suite

This PR adds comprehensive test coverage for the actor alarms feature with 1453 lines of tests and test runner infrastructure improvements.

Strengths

  1. Comprehensive test coverage for alarms, clearing, replacement, edge cases
  2. Well-documented test actor behaviors
  3. Added lifecycle event broadcasting for better test observability
  4. Clear test organization with MARK comments

Critical Issues (BLOCKING)

  1. Import style violation at actors_alarm.rs:1 - uses anyhow::* glob import, prohibited by CLAUDE.md. Fix: use anyhow::{Result, bail};
  2. Race condition in wait_for_actor_wake_from_alarm timeout logic (line 75) - saturating_sub can return Duration::ZERO causing immediate timeout

High Priority

  1. Poor error handling with nested expect() calls - use ? with Context trait
  2. Arc<Mutex<Option>> pattern with unwrap() - simplify or document necessity
  3. Silent error handling - let _ = tx.send() should log failures

Nice to Have

  • Extract magic numbers to constants
  • Document timeout choices
  • Consider removing deprecated polling helpers

Test Coverage

Excellent coverage of basic alarms, clearing, replacement, edge cases, multi-cycle patterns, crash interactions, concurrent alarms, and timing accuracy.

Overall: 4/5 stars

Recommendation: Approve with changes - fix glob import and timeout race condition before merging.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 11, 2025

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 10eacad

@abcxff abcxff requested a review from NathanFlurry December 11, 2025 02:18
@NathanFlurry NathanFlurry force-pushed the 12-08-chore_engine_test_actor_alarms branch from b9ccf81 to 4919fdc Compare December 25, 2025 01:40
@NathanFlurry NathanFlurry force-pushed the 11-17-feat_engine_actors_lifecycle_testing branch from 2fc28aa to 2f65035 Compare December 25, 2025 01:41
@NathanFlurry NathanFlurry force-pushed the 11-17-feat_engine_actors_lifecycle_testing branch from 2f65035 to 270d196 Compare December 25, 2025 02:45
@NathanFlurry NathanFlurry force-pushed the 12-08-chore_engine_test_actor_alarms branch from 4919fdc to 9a6ef9c Compare December 25, 2025 02:45
@graphite-app graphite-app bot changed the base branch from 11-17-feat_engine_actors_lifecycle_testing to graphite-base/3624 December 25, 2025 02:47
@graphite-app graphite-app bot force-pushed the graphite-base/3624 branch from 270d196 to 2855c56 Compare December 25, 2025 02:48
@graphite-app graphite-app bot force-pushed the 12-08-chore_engine_test_actor_alarms branch from 9a6ef9c to da6c8c3 Compare December 25, 2025 02:48
@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless December 25, 2025 02:48 Inactive
@graphite-app graphite-app bot changed the base branch from graphite-base/3624 to main December 25, 2025 02:48
@graphite-app graphite-app bot force-pushed the 12-08-chore_engine_test_actor_alarms branch from da6c8c3 to 10eacad Compare December 25, 2025 02:49
@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless December 25, 2025 02:49 Inactive
@abcxff abcxff marked this pull request as ready for review December 25, 2025 03:00
@graphite-app
Copy link
Contributor

graphite-app bot commented Dec 25, 2025

Merge activity

  • Dec 25, 3:10 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Dec 25, 3:10 AM UTC: CI is running for this pull request on a draft pull request (#3676) due to your merge queue CI optimization settings.
  • Dec 25, 3:11 AM UTC: Merged by the Graphite merge queue via draft PR: #3676.

graphite-app bot pushed a commit that referenced this pull request Dec 25, 2025
@graphite-app graphite-app bot closed this Dec 25, 2025
@graphite-app graphite-app bot deleted the 12-08-chore_engine_test_actor_alarms branch December 25, 2025 03:11
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