[adapters] Fix a bug + a race in suspend processing.#6615
Conversation
d7f7624 to
31fcff5
Compare
mythical-fred
left a comment
There was a problem hiding this comment.
Clean fix for a real race: during a suspend the circuit thread flips to Terminated before the server has installed PipelinePhase::Suspended, so a /status poll landing in that window observed Terminated and returned a 500 PipelineTerminated that the runner then recorded as a fatal error. The new terminated_status helper turns that transient window into a clean Suspended reading iff runtime_desired_status == Suspended; every other terminated case stays a PipelineTerminated error. Doc comment on the helper is precise and points a future reader at the causing invariant.
The unit test covers both branches (suspend-requested → Suspended; anything else → PipelineTerminated), which is exactly what matters here. The runtime_status_details: json!("") for the synthesized Suspended reading matches the shape used elsewhere in the same file.
Question (non-blocking): the Suspended-desired-status guard is a proxy for "a suspend has been requested." Is it possible for a pipeline to get into Terminated while runtime_desired_status == Suspended for reasons other than the intended suspend (e.g. an internal error that happens to race with a suspend request)? If so this would mask a real fatal termination as a clean suspend. If the answer is "the pipeline manager treats Suspended as the terminal desired state and any error path clears/rewrites it first," then this is fine — worth a one-line comment noting that assumption. If not, we might want to also check some "suspend in progress" flag on state before returning the Suspended reading.
LGTM as-is.
|
Needs more work, will reopen soon |
This commit fixes a severe bug and a less severe race. The bug: when we fail to make a checkpoint while suspending, we log the error, but still set pipeline status to Suspended, making it look like the Suspend succeeded. There are two options for handling this better: (1) go back to the Running state, giving the user a chance to fix the issue and retry (not sure what they can do if the disk runs out of space though...), (2) stop the pipeline and set state to Failed. Like with any other failure, some progress is lost, but at least we don't mask the error. The race: when the checkpoint succeeded, there was a brief window after the checkpoint completes and before we set phase to Suspended when the pipeline's state was Stopped. This caused tests waiting for the Suspended state to fail, since they don't expect the Stopped status. This flaked the cloud `suspend_and_resume_demos` test: a demo reached `Stopped` carrying `PipelineTerminated` instead of suspending cleanly. Signed-off-by: Leonid Ryzhyk <ryzhyk@gmail.com>
31fcff5 to
87436fe
Compare
Make sure a failed suspend is reported as a pipeline failure. Signed-off-by: Leonid Ryzhyk <ryzhyk@gmail.com>
blp
left a comment
There was a problem hiding this comment.
This seems reasonable. Thank you for catching it.
This commit fixes a severe bug and a less severe race.
The bug: when we fail to make a checkpoint while suspending, we log the
error, but still set pipeline status to Suspended, making it look like the
Suspend succeeded. There are two options for handling this better: (1) go back
to the Running state, giving the user a chance to fix the issue and retry
(not sure what they can do if the disk runs out of space though...),
(2) stop the pipeline and set state to Failed. Like with any other failure,
some progress is lost, but at least we don't mask the error.
The race: when the checkpoint succeeded, there was a brief window after the
checkpoint completes and before we set phase to Suspended when the pipeline's
state was Stopped. This caused tests waiting for the Suspended state to fail,
since they don't expect the Stopped status.
This flaked the cloud
suspend_and_resume_demostest: a demo reachedStoppedcarryingPipelineTerminatedinstead of suspending cleanly.