fix(scheduler): handle start-time mismatch ValidationException gracefully - #1042
fix(scheduler): handle start-time mismatch ValidationException gracefully#1042ZainAallii wants to merge 1 commit into
Conversation
…ully When fast-completing actions (e.g. P4Credentials, P4ApplySecrets) report their startedAt after the service already recorded a different timestamp from an earlier heartbeat, the service rejects with ValidationException. Previously this was treated as unrecoverable, causing a death spiral (stale entry never cleared, re-sent every heartbeat, agent terminates). Now the scheduler catches this specific ValidationException, logs a warning, clears the stale action updates from the map, and re-syncs immediately. Other ValidationExceptions and unrecoverable errors still propagate as before. Note: commit_completed_actions() drops all pending updates in the batch. This is correct assuming the service rejects the entire batch (not per-entry). The service error is a single ValidationException with no partial-success semantics, so no entries were processed. Fresh updates will be re-reported on the next heartbeat.
e8089fa to
610a528
Compare
| f"(actions likely completed faster than heartbeat interval): {inner}. " | ||
| f"Dropping stale action updates to recover." | ||
| ) | ||
| commit_completed_actions() |
There was a problem hiding this comment.
The batch UpdateWorkerSchedule request is atomic — when it fails with ValidationException, the service accepted none of the actions in updated_session_actions. But commit_completed_actions() here clears the entire transaction (every action in the batch), while only one action is actually stale (the one named in the error message: Cannot update inactive {action_id} ...).
The other actions in the same batch had valid, non-stale updates that were rejected only as collateral damage of the atomic failure. Committing them means their completion status is silently discarded and never re-sent, so the service never learns their real outcome — potentially leaving those actions/sessions stuck from the service's view.
Consider dropping only the offending action: the error message already contains the action id, so you can parse it out and delete just that key from _action_updates_map, then let the next sync re-send the rest normally (which also avoids the death spiral without discarding good updates). If a fully surgical fix is not feasible, at minimum this trade-off is worth calling out explicitly.
| f"(actions likely completed faster than heartbeat interval): {inner}. " | ||
| f"Dropping stale action updates to recover." | ||
| ) | ||
| commit_completed_actions() |
There was a problem hiding this comment.
I'm not convinced by this. Why would fast action times cause this? Nothing here actually explains that, just presume that it's correct. I think doing this is a mistake and is potentially hiding an instance config issue (two workers on one machine?) or some other issue that isn't being fixed.
Problem
When multiple environment-enter actions complete faster than the heartbeat interval (e.g. fast environment-enter scripts in Perforce/UE workflows), the agent re-reports a
startedAttimestamp that the service already recorded differently from an earlier heartbeat. The service rejects withValidationException: Cannot update inactive sessionaction-... because the provided start time X is different from the original start time Y.Previously this was treated as unrecoverable, causing a death spiral: the stale entry stays in
_action_updates_map(commit never called on failure), gets re-sent every heartbeat, and the agent terminates the session.Fix
In
_sync(), catchDeadlineRequestUnrecoverableErrorwhen the inner exception is aValidationExceptioncontaining the specific service message about start-time mismatch. On match: log WARNING, callcommit_completed_actions()to clear the stale entries, return interval=1 for immediate re-sync. Other ValidationExceptions and unrecoverable errors still propagate as before.Tradeoff: batch-level commit
commit_completed_actions()drops all pending updates in the rejected batch, not just the offending action. This is correct assuming the service rejects the entire batch atomically (the error is a single ValidationException, no partial-success). The service already considers the affected actions inactive/done, and fresh updates will be re-reported on the next heartbeat (~1s later).Testing
Impact
Any customer with multiple fast-completing environment-enter actions (common in Perforce/UE workflows) can hit this. The session is terminated and the job fails despite all actions executing successfully.