feat(tasks): first-class CANCELLED terminal status - #1137
Open
kyle-compute wants to merge 1 commit into
Open
Conversation
A stopped run used to be dishonest about why it ended: user cancellation wrote FAILED (implying the pipeline broke) unless a kept SUCCESS verdict forced COMPLETED (implying all trials finished), and quota cancellation always wrote FAILED — silently polluting failure metrics and anything automated on FAILED. The rule is now: task status reflects the execution outcome, the verdict columns independently reflect the judgment. - TaskStatus.CANCELLED (terminal; PG enum label 'CANCELLED' added by core migration cancel01_add_cancelled via ADD VALUE IF NOT EXISTS in an autocommit block, mirroring skip01/qa01). - cancel_tasks_runs: an active run cancelled in PENDING/RUNNING lands CANCELLED; the failed_by_this_cancel COMPLETED override is deleted — a kept SUCCESS verdict + payload simply stays on the cancelled task. - Cancelling in ANALYZING/VERDICT_PENDING (all trials finished, only QA stopped) lands COMPLETED, same rule as cancel_task_qa_core, whose FAILED arm is dropped for the same reason. - Quota enforcement (_reconcile_cancelled_tasks): exhausted tasks land CANCELLED — quota stopped them, nothing broke. - Consumers: CANCELLED is terminal everywhere FAILED was (append/retry/ import resurrection to RUNNING, post-trial hook guard, cleanup-sweep SQL terminal sets, CLI wait loops and rendering, preview seed); it is sticky in resolve_task_status; browse filter gains a Cancelled option; frontend TaskStatus union extended. Trial-level statuses are untouched. Older installed CLIs parse the status as an opaque string (no client-side enum validation), so they render "cancelled" as plain text and do not crash; their watch/pull wait loops treat only completed/failed as terminal and keep polling a cancelled task until interrupted. Claude-Session: https://claude.ai/code/session_01MzJSdrHKEqJrJHU7RRJ758
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Oddish previewCommit:
Vercel deployment URL: https://oddish-da4i1fx7i.oddish.app Plan:
This comment is updated by the PR Preview workflow. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Semantic rule
Task status reflects the execution outcome; the verdict columns independently reflect the judgment.
CANCELLED= a person or quota stopped the run. It is terminal, and it is notFAILED(pipeline broke) norCOMPLETED(all trials finished).Producers
cancel_tasks_runs(oddish/src/oddish/queue.py), task in PENDING/RUNNINGfailed_by_this_cancelhack)cancel_tasks_runs, task in ANALYZING/VERDICT_PENDINGcancel_task_qa_core(oddish/src/oddish/core/endpoints/qa.py), VERDICT_PENDING task_reconcile_cancelled_tasks(oddish/src/oddish/core/quota_enforcement.py), exhausted taskCancel-path audit:
git grep -n "USER_CANCELLED_MESSAGE\|QUOTA_CANCELLED_MESSAGE" -- '*.py'— every non-test hit audited; the only task-status writers on cancel paths are the three functions above (the qa.py hits are verdict/analysis columns,slack_notifications.pyis a read that already excludes user-cancelled verdicts and is unchanged).QA-cancel decision: a task only reaches VERDICT_PENDING when every trial is terminal (
maybe_start_qa_stage/backfill_task_analysis_coreboth enforcepending_count == 0). Cancelling there stops the judgment, not the run — the run finished. So the execution outcome is COMPLETED, and the cancelled judgment is fully recorded in the verdict columns (verdict_status = FAILED+ "Cancelled by user", or the kept SUCCESS verdict restored bycancel_verdict). Writing CANCELLED would falsely claim the trials were stopped; keeping the old FAILED arm would falsely claim the pipeline broke.cancel_tasks_runsapplies the same stage split sooddish cancelandPOST /tasks/{id}/qa/cancelagree on the identical situation.Quota-cancel decision: quota reaching a cap stops work; nothing broke — CANCELLED. In practice reconcile only sees tasks whose active trials were just cancelled (PENDING/RUNNING); message/error fields are unchanged.
Consumer sweep
oddish/— all 50TaskStatus.references audited. Terminal-set membership extended (mirroring FAILED) in: trial retry (endpoints/trials.pyreset-to-RUNNING), append (queue.py), trial import resurrection (core/ingest/trial_imports.py), post-trial hook guard (workers/queue/trial_handler.py), both cleanup-sweep SQL terminal lists (workers/queue/cleanup.py_reset_orphaned_trial_analysis).resolve_task_status(core/helpers.py) makes CANCELLED sticky — otherwise the all-trials-terminal display rule would flip a cancelled task back to COMPLETED. CLI:format_task_statuscolor map plus every("completed", "failed")terminal check inapi.py/status.py/pull.py. Retrying a trial of a CANCELLED task resurrects it, exactly like FAILED.ACTIVE_TASK_STATUSESdeliberately unchanged (CANCELLED is terminal; test-asserted).backend/—git grep -n "TaskStatus\." backend/→ 0 hits; routers treat status as an opaque string (browsestatusesCSV filter passes uppercase names straight toTaskModel.status.in_, so the new filter value works with no backend change). Uppercase-literal audit found one consumer:preview_seed.py_TERMINAL_TASK_STATUSES(+"CANCELLED").'failed'/'completed'string-literal audit: no task-status SQL in backend outside that file.frontend/—TaskStatusunion insrc/lib/types.ts(+"cancelled"); browse sidebarSTATUS_OPTIONSinsrc/lib/tasks-filters.ts(+CANCELLED, uppercase enum-name form per the values_callable comment there). The dashboardSTATUS_FILTER_OPTIONS"Failures" bucket is deliberately untouched:_experiment_row_passes_status_filterkeys offverdict_failed+failed_trialscounts, which are verdict/trial-level and unchanged by this PR (trial statuses are out of scope).git grep '"failed"' frontend/src→ 24 hits audited; all others are trial/report/verifier contexts. Active-state helpers (job-status.ts,experiment-client.tsxACTIVE_TASK_STATUSES) need no change — cancelled is terminal.Deliberately unchanged, for the record: TrialStatus /
harbor_stage='cancelled'sentinels (out of scope), GitHub PR-comment formatter (renders trial+verdict statuses only), Slack qa-failed alert (already excludes "Cancelled by user" verdict errors),deletion.py_clear_stale_task_pipeline_status(fires only on ANALYZING/VERDICT_PENDING survivors of a scoped deletion — not a cancellation).Migration
oddish/alembic/versions/cancel01_add_cancelled_task_status.py— core stack, notbackend/alembic/: thetaskstatustype is created by core migrations and the repo precedent for adding task-status values is there (m8n9o0p1q2r3added ANALYZING/VERDICT_PENDING/COMPLETED). Mechanism mirrorsskip01/qa01:op.get_context().autocommit_block()+ALTER TYPE taskstatus ADD VALUE IF NOT EXISTS 'CANCELLED'(labels are the uppercase member names — default SQLEnum), value not used in the same migration, no-op downgrade. Verifiedalembic upgrade headfrom an empty Postgres 16 (core then backend, same order as the Migration Upgrade Check), single head preserved (cancel01_add_cancelled).Rolling compat
Investigated: the CLI never parses responses through the
TaskStatuspydantic enum —oddish status/watch/pullconsume raw JSON dicts (response.json()), andTaskStatusResponseis server-side serialization only. Precedent: the SKIPPED trial-status rollout (#574) shipped the same way with no API versioning. Exposure for already-released CLIs against a server that emits"cancelled":oddish run(watch),oddish status --watch,oddish pull --wait, and experiment watch treat onlycompleted/failedas terminal, so if the watched task is cancelled from elsewhere the loop keeps polling every 2s until the user interrupts it; status renders as plain uncoloredcancelledtext via thestyle_mapfallback.ls,status,pullwithout wait) just display the new string; old dashboards count such a task as neither done nor running in the summary line.No versioning system introduced (per scope); the exposure is called out in the CHANGELOG entry.
Tests
test_cancel_task_status.py(new — cancel-active → CANCELLED with kept verdict untouched; cancel VERDICT_PENDING → COMPLETED with verdict FAILED+"Cancelled by user"; cancel PENDING → CANCELLED; ACTIVE sets exclude CANCELLED),test_quota_enforcement.py(exhausted task → CANCELLED, both user- and org-scope),test_retry_trial_core.py(new: retry on CANCELLED task → RUNNING),test_cleanup_orphaned_analysis.py(SQL literal assertions), plus cancel-harvest/CLI-status/verdict-sync/backfill files: 60 passed against Postgres 16.oddish/suite vs a cleanorigin/stagingbaseline run on the same DB: failure set is identical to baseline (3009+ passed; baseline failures are environmental — quota/org tables live in the backend migration stack, absent from a core-only test DB — plus one order-dependent flake intest_local_runner_gate.pythat flip-flops on baseline and branch alike).backend/suite: failure list byte-identical to baseline (1105 passed; 45 pre-existing environmental failures — Clerk/env-dependent).backend/tests/e2e(the repo's only CI pytest gate): 2 passed.pnpm exec tsc --noEmitclean;pnpm lintclean.oddish/scripts/load_only_guard.py: OK (no new columns surfaced on the compact path).https://claude.ai/code/session_01MzJSdrHKEqJrJHU7RRJ758
Note
Medium Risk
Touches core task lifecycle (cancel, quota, status resolution, retries) and enum migration; behavior changes are well-tested but affect many code paths and older CLI polling semantics.
Overview
Introduces a terminal
cancelledtask status so user- or quota-driven stops are no longer encoded asfailed(or the old “failed then COMPLETED if verdict SUCCESS” override). Execution status and verdict columns are treated separately: a kept accept verdict can remain valid on a cancelled task, and metrics/automation that key offfailedno longer count cancellations.Cancel paths now set
CANCELLEDfor active runs (cancel_tasks_runs, quota reconciliation) andCOMPLETEDwhen every trial is already done and only QA/judgment was stopped (ANALYZING/VERDICT_PENDING, including QA cancel). The verdict-success → COMPLETED hack on cancel is removed.resolve_task_statusmakesCANCELLEDsticky so “all trials terminal” cannot flip a cancelled task back to completed.Consumers treat
cancelledlike other terminal statuses: CLI watch/pull/status, trial retry/resurrect (same as failed), imports/appends, post-trial hooks, and orphaned-analysis cleanup SQL. The task browse UI adds a Cancelled filter; types and formatting include the new status.Migration
cancel01_add_cancelledadds the Postgres enum value with no row backfill. Older CLIs keep polling oncancelleduntil upgraded (documented in CHANGELOG).Reviewed by Cursor Bugbot for commit 419f47d. Bugbot is set up for automated code reviews on this repo. Configure here.