From c5dc8adacf4e5f7f193e3eda777452ce50bda17d Mon Sep 17 00:00:00 2001 From: Ryan Gonzalez Date: Fri, 24 Jul 2026 18:08:39 -0500 Subject: [PATCH] Treat all 403s as cancellation, regardless of Job-Status Back when cancellation was first added to gitlab-runner, GitLab would include the current job status in the response's `Job-Status` header, so we could check if the job was actually cancelled. This situation changed all the way back in 2023: https://gitlab.com/gitlab-org/gitlab/-/work_items/386871 In theory, GitLab does set Job-Status when the job is not in a state for the request to go through, but now it will also *invalidate the job token after the job reaches a completed state*. When a job token is invalidated, it cannot be used for authentication anymore, and thus we get a 403 in the sense of "you do not have permission to access this", rather than "the modification you're trying to perform is forbidden". Thus, job cancellation has actually been broken for several years. For gitlab-runner itself, this isn't an issue for two reasons: - All 403s result in job termination, regardless of job status: https://gitlab.com/gitlab-org/gitlab-runner/-/blob/241271310f4c357edd09ade516512a5b8b709c10/network/remote_job_state_response.go#L32 - gitlab-runner implements an additional runner feature, `cancel_gracefully`, that lets the after_script run after cancellation: https://gitlab.com/gitlab-org/gitlab-runner/-/merge_requests/4578 The simplest fix for now is to just treat 403s as the single "incorrect job status" indicator and remove the code related to checking `Job-Status`. If/when we decide to add `cancel_gracefully` support, we can bring parts of it back to check for the `canceling` status as needed. Potentially related to the lava-gitlab-runner hangs: https://github.com/collabora/lava-gitlab-runner/issues/129 --- gitlab-runner-mock/src/api/request.rs | 6 ++--- gitlab-runner-mock/src/api/trace.rs | 9 +++----- gitlab-runner-mock/src/api/update.rs | 33 +++++++++++---------------- gitlab-runner/src/client.rs | 20 +++++----------- gitlab-runner/src/run.rs | 4 ++-- 5 files changed, 26 insertions(+), 46 deletions(-) diff --git a/gitlab-runner-mock/src/api/request.rs b/gitlab-runner-mock/src/api/request.rs index 3afd191..5ed19f4 100644 --- a/gitlab-runner-mock/src/api/request.rs +++ b/gitlab-runner-mock/src/api/request.rs @@ -8,11 +8,9 @@ use wiremock::{Request, Respond}; use crate::{GitlabRunnerMock, MockJobState}; /* - jobs/id => 200 if ok; + jobs/id => 200 if ok; - 403 if cancelled - < job-status: canceled in header -} + 403 if job token is invalid or the job was cancelled */ #[derive(Default, Deserialize)] diff --git a/gitlab-runner-mock/src/api/trace.rs b/gitlab-runner-mock/src/api/trace.rs index 0d71f44..21a8bc7 100644 --- a/gitlab-runner-mock/src/api/trace.rs +++ b/gitlab-runner-mock/src/api/trace.rs @@ -53,19 +53,16 @@ impl Respond for JobTraceResponder { }; if let Some(job) = self.mock.get_job(id) { - if token != job.token() { + if token != job.token() || job.state() != MockJobState::Running { ResponseTemplate::new(StatusCode::FORBIDDEN) - } else if job.state() != MockJobState::Running { - ResponseTemplate::new(StatusCode::FORBIDDEN) - .insert_header("Job-Status", &*job.state().to_string()) } else { match job.append_log(request.body.clone(), start, end) { Ok(()) => ResponseTemplate::new(StatusCode::ACCEPTED) .insert_header( "X-GitLab-Trace-Update-Interval", - &*self.mock.update_interval().to_string(), + self.mock.update_interval().to_string(), ) - .insert_header("Job-Status", &*job.state().to_string()), + .insert_header("Job-Status", job.state().to_string()), Err(e) => ResponseTemplate::new(StatusCode::RANGE_NOT_SATISFIABLE) .set_body_string(format!("{e:?}")), } diff --git a/gitlab-runner-mock/src/api/update.rs b/gitlab-runner-mock/src/api/update.rs index cd8acf4..0113e70 100644 --- a/gitlab-runner-mock/src/api/update.rs +++ b/gitlab-runner-mock/src/api/update.rs @@ -35,29 +35,22 @@ impl Respond for JobUpdateResponder { .unwrap(); if let Some(job) = self.mock.get_job(id) { - if r.token != job.token() { + if r.token != job.token() || job.state() != MockJobState::Running { ResponseTemplate::new(StatusCode::FORBIDDEN) } else { - let r = match (job.state(), r.state) { - (MockJobState::Running, MockJobState::Success) => { - job.update_state(r.state); - ResponseTemplate::new(StatusCode::OK) - } - (MockJobState::Running, MockJobState::Failed) => { - job.update_state(r.state); - ResponseTemplate::new(StatusCode::OK) - } - (MockJobState::Running, MockJobState::Running) => { - job.update_state(r.state); - ResponseTemplate::new(StatusCode::OK) - } - (current_state, _) if current_state != MockJobState::Running => { - ResponseTemplate::new(StatusCode::FORBIDDEN) - } - _ => panic!("Invalid state change"), - }; + assert!( + matches!( + r.state, + MockJobState::Success | MockJobState::Failed | MockJobState::Running + ), + "Invalid state change from {} -> {}", + job.state(), + r.state, + ); - r.append_header("Job-Status", &*job.state().to_string()) + job.update_state(r.state); + ResponseTemplate::new(StatusCode::OK) + .append_header("Job-Status", job.state().to_string()) } } else { ResponseTemplate::new(StatusCode::NOT_FOUND) diff --git a/gitlab-runner/src/client.rs b/gitlab-runner/src/client.rs index 365583d..9925054 100644 --- a/gitlab-runner/src/client.rs +++ b/gitlab-runner/src/client.rs @@ -21,7 +21,6 @@ where } const GITLAB_TRACE_UPDATE_INTERVAL: &str = "X-GitLab-Trace-Update-Interval"; -const JOB_STATUS: &str = "Job-Status"; #[derive(Debug, Default, Clone, Serialize)] struct FeaturesInfo { @@ -250,8 +249,10 @@ impl JobResponse { pub enum Error { #[error("Unexpected reply code {0}")] UnexpectedStatus(StatusCode), - #[error("Job cancelled")] - JobCancelled, + // May occur if the job was cancelled, in which case the job token we're + // using gets invalidated. + #[error("Forbidden")] + Forbidden, #[error("Request failure {0}")] Request(#[from] reqwest::Error), #[error("Failed to write to destination {0}")] @@ -336,13 +337,6 @@ impl Client { } } - fn check_for_job_cancellation(&self, response: &reqwest::Response) -> Result<(), Error> { - match response.headers().get(JOB_STATUS) { - Some(header) if header == "canceled" => Err(Error::JobCancelled), - _ => Ok(()), - } - } - pub async fn update_job( &self, id: u64, @@ -359,8 +353,6 @@ impl Client { let r = self.client.put(url).json(&update).send().await?; - self.check_for_job_cancellation(&r)?; - let trace_update_interval = r .headers() .get(GITLAB_TRACE_UPDATE_INTERVAL) @@ -369,6 +361,7 @@ impl Client { StatusCode::OK => Ok(JobUpdateReply { trace_update_interval, }), + StatusCode::FORBIDDEN => Err(Error::Forbidden), _ => Err(Error::UnexpectedStatus(r.status())), } } @@ -406,8 +399,6 @@ impl Client { .send() .await?; - self.check_for_job_cancellation(&r)?; - let trace_update_interval = r .headers() .get(GITLAB_TRACE_UPDATE_INTERVAL) @@ -417,6 +408,7 @@ impl Client { StatusCode::ACCEPTED => Ok(TraceReply { trace_update_interval, }), + StatusCode::FORBIDDEN => Err(Error::Forbidden), _ => Err(Error::UnexpectedStatus(r.status())), } } diff --git a/gitlab-runner/src/run.rs b/gitlab-runner/src/run.rs index 4cbdb2a..519b3cc 100644 --- a/gitlab-runner/src/run.rs +++ b/gitlab-runner/src/run.rs @@ -248,7 +248,7 @@ impl Run { .await { Ok(_reply) => (), - Err(crate::client::Error::JobCancelled) => cancel_token.cancel(), + Err(crate::client::Error::Forbidden) => cancel_token.cancel(), Err(err) => warn!("Failed to update job status: {:?}", err), } } @@ -276,7 +276,7 @@ impl Run { self.log_offset += len; reply.trace_update_interval } - Err(crate::client::Error::JobCancelled) => { + Err(crate::client::Error::Forbidden) => { cancel_token.cancel(); None }