fix(run-engine): correct park deadline and snapshot state for debounced parked runs - #4708
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
0ski
changed the title
7aab9b9 to
b40b22a
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…ed parked runs Two defects surface when a run parked on an external deployment id is pushed by a debounce key. Both were reproduced against a local instance before fixing. 1. The run is expired before it is due. The park deadline is armed once, when the run is first parked, from max(now, delayUntil) + deadline. Debounce pushes delayUntil out afterwards: rescheduleDelayedRun reschedules enqueueDelayedRun:<id>, and the redis-worker reschedule is an update-only ZADD, so expireParkedExternalDeploymentRun:<id> is never re-armed. Repeat triggers on one key walk delayUntil past a deadline that no longer moves, and the run is expired with EXTERNAL_DEPLOYMENT_NOT_FOUND before it was ever due to start. Observed: a run due at 14:01:37 expired at 13:57:02. The expiry job already loads delayUntil, so it now re-arms from the current value and returns instead of expiring a run that is not due. Putting the guard there rather than in the debounce path covers every caller that moves delayUntil, and it stays bounded by the debounce max-duration contract. 2. The run reports itself as delayed while it is parked. rescheduleRun hardcoded a DELAYED/DELAYED execution snapshot, so a debounce push left the run row on PENDING_VERSION while its latest snapshot claimed DELAYED, and the run page described a parked run as delayed. The snapshot statuses are now supplied by the caller and default to DELAYED, so the ordinary delayed path is unchanged, and rescheduleDelayedRun passes the parked statuses through when the run is parked.
b40b22a to
f62294a
Compare
2 participants
Two defects that surface when a run parked on an external deployment id gets pushed by a debounce key. Both were reproduced against a local instance before being fixed.
1. The run is expired before it is due
Killed 4m35s before its own scheduled start, blaming a missing deployment.
Why. The park deadline is armed once, when the run is first parked, from
max(now, delayUntil) + deadline. Debounce pushesdelayUntilout afterwards and nothing re-arms it:rescheduleDelayedRunreschedulesenqueueDelayedRun:<id>, notexpireParkedExternalDeploymentRun:<id>ZADD … XX, and a parked run has noenqueueDelayedRunjob, so that call is a silent no-opRepeat triggers on one key walk
delayUntilaway from a deadline that no longer moves. Once it crosses, the run dies while parked and not yet due.Fix. The expiry job already loads
delayUntil, so it re-arms from the current value and returns instead of expiring a run that is not due.The guard lives in the expiry job rather than the debounce path deliberately: it covers every caller that moves
delayUntil, so a future call site can't reintroduce this by forgetting to re-arm. It stays bounded by the debounce max-duration contract, so a hot key can't postpone expiry indefinitely.2. The run reports itself as delayed while it is parked
The row stays
PENDING_VERSION; the latest snapshot claimsDELAYED, so the run page describes a parked run as delayed. Happens on the first push.Fix.
rescheduleRunhardcodedDELAYED/DELAYED. The snapshot statuses are now supplied by the caller and default toDELAYED, so the ordinary delayed path is byte-identical, andrescheduleDelayedRunpasses the parked statuses through when the run is parked.Reproducing
Repeated triggers on one debounce key against an id that hasn't landed:
curl … -d '{"options":{"externalDeploymentId":"x","debounce":{"key":"k","delay":"5m"}}}'Three triggers correctly fold into one parked run; the defects show up on the pushes.
Testing
Two tests, each verified red before green and failing alone:
PENDING_VERSIONinstead of expiringRUN_CREATED/PENDING_VERSIONsnapshot, notDELAYED56 passedacross parking, pendingVersion, delayedRunSystem and debounce;43 passedinPostgresRunStore. Typecheck, lint, format clean.Notes