Skip to content

fix: Job cancellation can be overwritten by a later successful/failed finish#790

Closed
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-a58011d4
Closed

fix: Job cancellation can be overwritten by a later successful/failed finish#790
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-a58011d4

Conversation

@sam-saffron-jarvis

Copy link
Copy Markdown
Contributor

What changed

  • Made finishRun perform a guarded terminal-state update instead of blindly overwriting whatever status is already in job_runs_v2.
  • If a run has already been marked cancel_requested, finishRun now persists it as cancelled and keeps cancellation authoritative even if the worker later reports succeeded, failed, or timed_out.
  • Limited terminal persistence to still-active run states so already-terminal rows are left alone.
  • Added a regression test covering the real failure mode: a running job is cancelled, then a later failed finish attempt must not overwrite the cancellation or schedule a retry.

Why this is high-value

Cancellation is a real user-facing control path. Before this change, there was a race where CancelRun could acknowledge cancellation in the DB, but a slightly later worker finish would overwrite that state back to success/failure/timeout. That could:

  • show misleading final state to users,
  • fire the wrong completion notifications,
  • schedule retries for work the user explicitly cancelled.

This fix makes cancellation authoritative at the persistence boundary, which is the smallest safe place to close the race.

Validation

  • Added TestJobsV2FinishRunDoesNotOverrideCancelRequested
  • gofmt -w cmd/serve_jobs_v2.go cmd/serve_jobs_v2_test.go
  • go build ./...
  • go test ./...
  • git diff --check

@SamSaffron

Copy link
Copy Markdown
Owner

Applied to the working tree locally (along with #789/#790/#791 as a combined cancellation-correctness pass); will land in a direct commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants