fix(pb): clear actor reschedule ts on alloc#4338
fix(pb): clear actor reschedule ts on alloc#4338MasterPtato wants to merge 1 commit intofix-api-properly-dedup-name-listfrom
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
|
🚅 Deployed to the rivet-pr-4338 environment in rivet-frontend
|
PR Review: fix(pb): clear actor reschedule ts on allocSummary: This is a minimal, focused bug fix: one line added in the actual logic, plus a Cargo.lock cleanup. The Fix (engine/packages/pegboard/src/workflows/actor/runtime.rs:481)Correctness: This is the right fix. Without it, reschedule_ts would persist in the actor State after a successful allocation even though the actor is no longer waiting to be rescheduled. The reschedule_ts field is surfaced externally via ops/actor/util.rs (line 75), so a stale non-None value would give callers a misleading view of actor state. The pattern matches how other timestamps are handled: sleep_ts, pending_allocation_ts, and failure_reason are all cleared in the AllocateActorStatus::Allocated arm of the match below (lines 489-493). Clearing reschedule_ts before branching into the match is appropriate since it should be None regardless of whether allocation succeeded, is pending, or put the actor to sleep — the reschedule delay has been consumed once allocation is attempted. One minor observation: state.sleep_ts, state.pending_allocation_ts, and state.failure_reason are only cleared in the Allocated arm, not for Pending or Sleep. The new reschedule_ts = None is unconditional (applies to all three match arms). This is likely intentional — the reschedule window has passed once allocate_actor_v2 is called — but worth confirming that Pending and Sleep paths do not need reschedule_ts preserved for any downstream logic. Cargo.lock changeRemoval of the indexmap 2.10.0 dependency is a straightforward cleanup from removing an unused transitive dep. No concerns here. OverallThe fix is correct, minimal, and consistent with surrounding code patterns. No test coverage is included (the PR checklist is unchecked), which is understandable given that reproducing this requires a specific multi-step actor lifecycle scenario. If there is an existing integration test for actor rescheduling, it would be worth adding an assertion that reschedule_ts is null after re-allocation. |
1 participant
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: