docs(openspec): add task-cancellation proposal, design, specs, tasks
This commit is contained in:
@@ -0,0 +1,266 @@
|
||||
## Context
|
||||
|
||||
Task cancellation has no implementation anywhere in the stack today:
|
||||
|
||||
- `core/models.py`'s `TaskStatus` reserves `"cancelled"` but no code path assigns it.
|
||||
`runtime/task.py::TaskRunner._interrupt_task()` marks `should_stop`-triggered
|
||||
interruption as `status="failed", failure_reason="execution interrupted"` — this is the
|
||||
only existing consumer of `should_stop`, and it conflates "lease was lost" with "user
|
||||
asked to stop" (both produce `failed`).
|
||||
- `cloud/scheduler.py`'s `ScheduledTaskStatus` is `queued | assigned | dispatched | done |
|
||||
failed` — no `cancelled`, no cancel operation on `TaskScheduler` or `CloudRepository`.
|
||||
- The internal Host↔Cloud protocol (`cloud/internal_api/models.py`) has exactly one
|
||||
channel from Cloud back to Host during execution: `LeaseRenewalResponse`, currently
|
||||
`{status: Literal["renewed"], lease_expires_at: datetime}`. There is no push channel;
|
||||
the Host Agent is purely outbound (heartbeat, claim long-poll, renew, report result).
|
||||
- `workflow/` already has the target shape: `WorkflowRunStatus` includes `cancelled`,
|
||||
`WorkflowRunner._drive()` checks `should_stop()` at each step boundary and calls
|
||||
`self._checkpoint(run, ..., "cancelled")`. This design generalizes that pattern to
|
||||
`TaskRunner`/`ScheduledTask` rather than inventing a new one.
|
||||
- Three prior changes (`cloud-console`, `cloud-control-plane-integration`,
|
||||
`cloud-console-governance`) explicitly deferred cancellation, citing "no
|
||||
scheduler/repository operation for this exists." This change adds that operation.
|
||||
|
||||
Existing collaborative-stop infrastructure this design reuses instead of replacing:
|
||||
`ActiveAssignmentRunner.run()` (`apps/device-host-agent/host_agent/lease.py`) races
|
||||
execution against a renewal loop; the renewal loop is the only place the Host Agent
|
||||
talks to Cloud while a task is running. `should_stop` already flows
|
||||
`ActiveAssignmentRunner` → `AssignmentExecutor.execute()` →
|
||||
`TaskRunner.run()`/`WorkflowRunner.run()`, checked at step boundaries (never mid-step).
|
||||
|
||||
## Goals / Non-Goals
|
||||
|
||||
**Goals:**
|
||||
- Let an authorized caller cancel a task in `queued`, `assigned`, or `dispatched` state
|
||||
through the public SDK.
|
||||
- For `queued` tasks (never dispatched to a Host), cancellation is synchronous and
|
||||
immediate — no Host round-trip needed.
|
||||
- For `assigned`/`dispatched` tasks (a Host may be actively executing), cancellation is
|
||||
collaborative: the Host learns about it at its next lease renewal (≤ ~1/3 of
|
||||
`lease_duration_seconds`, matching the existing lease-loss detection latency) and stops
|
||||
at the next step boundary, exactly like a lost lease does today.
|
||||
- Make `cancelled` a real, reachable terminal state end-to-end: `core/models.py`'s
|
||||
`TaskStatus`, `cloud/scheduler.py`'s `ScheduledTaskStatus`, the Cloud Console, and the
|
||||
Host Agent local console.
|
||||
- Distinguish "cancelled by request" from "failed" in every layer that currently reports
|
||||
`should_stop`-triggered stops as generic failure, so operators can tell the two apart.
|
||||
- Cancellation is idempotent: cancelling an already-cancelled or already-terminal task is
|
||||
a no-op with a clear response, not an error that implies something changed.
|
||||
|
||||
**Non-Goals:**
|
||||
- No mid-step interruption. A single in-flight action (a tap, an LLM planning call) is
|
||||
never aborted mid-flight; the existing step-boundary granularity of `should_stop` is
|
||||
unchanged. A slow single step still finishes before a `dispatched` task's cancellation
|
||||
takes effect.
|
||||
- No forced/instant kill of a Host Agent process or its OS-level subprocess tree. This is
|
||||
cooperative cancellation only, consistent with the project's existing lease-loss
|
||||
handling — not a new capability class.
|
||||
- No per-task ownership/ACL model. The Cloud repository does not track which principal
|
||||
submitted a task; cancel authorization reuses the existing `tasks:submit` scope
|
||||
(whoever can submit a task can cancel any task). Adding submitter-scoped authorization
|
||||
is a distinct governance change, out of scope here (parallels
|
||||
`cloud-console-governance`'s existing target-based, not ownership-based, model).
|
||||
- No retry-from-UI or task editing. Only stopping a task early; resubmission remains a
|
||||
separate, already-existing "submit a new task" action.
|
||||
- No cancellation of individual workflow steps independent of the whole run; workflow
|
||||
cancellation continues to mean "stop the run," which `workflow/` already implements.
|
||||
- No change to `TaskDispatcher` (`cloud/dispatch.py`) — confirmed dead/dev-only code per
|
||||
prior investigation; not worth extending for cancellation.
|
||||
|
||||
## Decisions
|
||||
|
||||
### D1: `LeaseRenewalResponse.cancel_requested: bool` is the only new wire signal
|
||||
|
||||
Rejected alternatives: (a) a new dedicated Host-polled "check cancellation" endpoint —
|
||||
adds a second polling loop and a second latency bound to reason about, when renewal
|
||||
already runs on a well-understood cadence; (b) a push/webhook mechanism — breaks the
|
||||
explicit "Host Agent operation requires no inbound cloud connection" requirement in
|
||||
`host-agent-protocol`'s spec, which is a hard architectural constraint, not just current
|
||||
practice.
|
||||
|
||||
`cancel_requested` defaults to `False`. When the Cloud repository's `renew_lease` finds
|
||||
the active `ScheduledTask` has a pending cancellation record, it still renews the lease
|
||||
normally (a task must keep a live lease while the Host works through shutdown) but flags
|
||||
`cancel_requested=True` in the response. The Host Agent's `_renew_while_running` loop
|
||||
treats this the same way it treats `StaleLeaseError`: mark the `LeaseGuard` (extended
|
||||
with a distinct reason string, e.g. `"cancellation requested by control plane"`) and let
|
||||
the existing `should_stop` composite (`guard.is_lost() or self._stop_requested.is_set()`)
|
||||
do the rest — no new boolean plumbed through `ActiveAssignmentRunner`/`AssignmentExecutor`
|
||||
signatures.
|
||||
|
||||
### D2: Cancellation request is durable state, not a fire-and-forget signal
|
||||
|
||||
A `POST .../cancel` on an `assigned`/`dispatched` task writes a `cancel_requested_at`
|
||||
timestamp (see D5 schema) rather than only flipping an in-memory flag or directly setting
|
||||
`status="cancelled"`. Rationale: the Host may not renew again for up to
|
||||
`lease_duration_seconds / 3`; if the Cloud process restarts in that window, an in-memory
|
||||
signal would be lost silently, defeating the cancellation with no user-visible error. A
|
||||
durable row survives restarts and is what `renew_lease` reads.
|
||||
|
||||
The scheduled task's `status` only transitions to `cancelled` once the Host has actually
|
||||
stopped and reported it (D3) — or, for the immediate `queued` case, synchronously in the
|
||||
same request. This means `assigned`/`dispatched` tasks pass through an intermediate,
|
||||
observable "cancellation pending" state (surfaced to callers as the existing `status`
|
||||
value plus a non-null `cancel_requested_at`, not a new status literal — see D6) rather
|
||||
than jumping straight to `cancelled` before the Host has confirmed.
|
||||
|
||||
### D3: Host reports `cancelled` as a new terminal outcome via the existing result-report endpoint
|
||||
|
||||
`TerminalResultRequest.status` (`internal_api/models.py`) is `Literal["done", "failed"]`.
|
||||
This design adds `"cancelled"` to that literal rather than inventing a parallel
|
||||
cancellation-report endpoint, because `report_result`'s idempotency/lease-validation
|
||||
logic (`record_task_result` in `sql_repository.py`) already handles exactly the
|
||||
concurrency shape needed (attempt/lease-id conflict checks, `already_recorded` replay
|
||||
safety) — duplicating it for a cancel-specific endpoint would be pure risk with no
|
||||
benefit. `AssignmentExecutionResult.status` (`host_agent/assignment.py`) similarly gains
|
||||
a `"cancelled"` value alongside `"done"`/`"failed"`, set when `TaskRunner.run()` /
|
||||
`WorkflowRunner.run()` returns because of a cancellation-flavored stop rather than a lost
|
||||
lease or genuine step failure (see D4).
|
||||
|
||||
### D4: `should_stop` becomes a richer signal than a bare boolean at the `TaskRunner` boundary
|
||||
|
||||
Today `StopRequested = Callable[[], bool]`. Distinguishing "cancelled" from "lease
|
||||
lost"/"shutdown requested" only matters for the *reason recorded on the terminal
|
||||
status* — the control-flow behavior (stop at the next step boundary) is identical. Rather
|
||||
than changing the `should_stop` callable's signature (which would ripple through
|
||||
`workflow/runner.py`, tests, and every caller), `TaskRunner._interrupt_task()` and
|
||||
`WorkflowRunner._drive()`'s stop branch gain an optional second callable,
|
||||
`stop_reason: Callable[[], str] | None`, defaulting to the existing generic
|
||||
`"execution interrupted"` string when absent. `ActiveAssignmentRunner` supplies a
|
||||
`stop_reason` that reads `LeaseGuard.reason` (already a string field) so `"cancellation
|
||||
requested by control plane"` vs `"lease rejected by control plane"` flows through
|
||||
unchanged plumbing. `TaskRunner._interrupt_task()` sets `status="cancelled"` when the
|
||||
reason string indicates cancellation, else keeps `status="failed"` (lease loss remains a
|
||||
failure, not a cancellation, matching current behavior for that case). This is the
|
||||
narrowest change that gets a real `cancelled` status without touching every
|
||||
`should_stop`-typed parameter across the codebase.
|
||||
|
||||
Alternative considered and rejected: add a third `CancelRequested` callable parallel to
|
||||
`should_stop`. Rejected because it doubles the number of callables threaded through
|
||||
`ActiveAssignmentRunner → AssignmentExecutor → TaskRunner/WorkflowRunner` for
|
||||
information that's only needed at the one moment execution actually stops.
|
||||
|
||||
### D5: Schema — one new nullable column pair on `scheduled_tasks`, no new table
|
||||
|
||||
`packages/cloud-platform/cloud/migrations/versions/0012_task_cancellation.py` adds to
|
||||
`ScheduledTaskRow`/`scheduled_tasks`:
|
||||
- `cancel_requested_at: str | None` (ISO datetime, nullable) — set when a cancel request
|
||||
is recorded against an `assigned`/`dispatched` task; cleared (`NULL`) when the task
|
||||
reaches any terminal status, so a subsequent, different attempt of the same task id (if
|
||||
retry-on-lease-expiry logic in `reap_expired_leases` requeues it) doesn't inherit a
|
||||
stale cancellation.
|
||||
|
||||
No new table: the volume and access pattern (one pending value per task, read on every
|
||||
renewal, written once per cancel call) don't justify the join/joinless-read trade-off a
|
||||
separate `task_cancellation_requests` table would add, and there is exactly one prior
|
||||
migration precedent for this shape — `0008_task_progress_columns` added nullable
|
||||
scalar columns directly to `scheduled_tasks` for the same reason (frequently-read,
|
||||
single-value-per-task state). `TaskAttemptRow`/`task_attempts` needs no schema change:
|
||||
its `status` column already accepts free-form strings and gains `"cancelled"` as a value
|
||||
alongside `"assigned"/"dispatched"/"expired"/"done"/"failed"`.
|
||||
|
||||
`CloudRepository` Protocol gains:
|
||||
- `request_task_cancellation(task_id, *, requested_at) -> CancellationRequestStatus`
|
||||
where `CancellationRequestStatus = Literal["requested", "already_terminal",
|
||||
"already_requested", "not_found"]` — synchronously transitions a `queued` task straight
|
||||
to `cancelled` (there's no Host attempt in flight to notify) and otherwise sets
|
||||
`cancel_requested_at` on an `assigned`/`dispatched` task.
|
||||
- `renew_lease` gains a `cancel_requested` boolean in its return path (or the caller
|
||||
re-reads the row — implementation detail left to tasks.md) so `internal_api/api.py`'s
|
||||
`renew_assignment` handler can populate `LeaseRenewalResponse.cancel_requested`.
|
||||
- `record_task_result` accepts `status: Literal["done", "failed", "cancelled"]`
|
||||
(widened from today's `TerminalTaskStatus = Literal["done", "failed"]`) and clears
|
||||
`cancel_requested_at` on write.
|
||||
|
||||
### D6: No new `ScheduledTaskStatus` value for "cancellation pending"
|
||||
|
||||
Considered adding `"cancelling"` as a distinct status between `assigned`/`dispatched` and
|
||||
`cancelled`. Rejected: it would ripple into every place that already pattern-matches the
|
||||
existing five-value `ScheduledTaskStatus` (scheduler assignment logic, device
|
||||
reservation/`list_reserved_device_ids`, Cloud Console status filter, SDK response
|
||||
`Literal`), each needing to decide whether "cancelling" behaves like "assigned" (device
|
||||
still reserved, task still excluded from re-assignment) — which it always would, making
|
||||
the new status a strict synonym with an extra bit of information. Instead,
|
||||
"cancellation pending" is expressed as `status="assigned"` (or `"dispatched"`) plus
|
||||
non-null `cancel_requested_at` — every existing status-based code path (assignment
|
||||
matching, device reservation, retry-on-expiry) keeps working unmodified, and callers
|
||||
that want to show "cancelling…" in a UI check the extra field.
|
||||
|
||||
### D7: Public cancel endpoint shape
|
||||
|
||||
`POST /v1/tasks/{task_id}/cancel`, `202 Accepted` for the pending (assigned/dispatched)
|
||||
case and `200 OK` for the immediate (queued) case, both returning a small
|
||||
`TaskCancellationResponse {task_id, status}` reflecting the resulting `ScheduledTask`
|
||||
status. Repeated calls against an already-cancelled or already-`cancel_requested_at`-set
|
||||
task return the same response idempotently (HTTP 200, not a conflict) — matching the
|
||||
project's established idempotency style for `report_result`/`renew_assignment`, which
|
||||
return `already_recorded`/success on replay rather than erroring. Calling cancel on a
|
||||
task already `done`/`failed` returns `409 Conflict` with a clear "task is already
|
||||
terminal" detail, mirroring `_stale_lease_conflict`'s existing shape.
|
||||
|
||||
## Risks / Trade-offs
|
||||
|
||||
- [Operators expect cancellation to be instant] → It isn't, by design (D1's latency
|
||||
bound). The Cloud Console surfaces the `cancel_requested_at` pending state distinctly
|
||||
(D6) so operators see "cancellation requested" rather than a UI that looks stuck; docs
|
||||
(`docs/CLOUD_DEPLOYMENT.md`) get an explicit latency note per the proposal's Impact
|
||||
section.
|
||||
- [A Host Agent that never renews again (already offline, or wedged past its lease) never
|
||||
learns about the cancellation] → Existing `reap_expired_leases` already requeues or
|
||||
fails such tasks once the lease expires; this change doesn't need new machinery for
|
||||
that case — an offline Host's task eventually reaches a terminal state via the existing
|
||||
reaper, at which point `cancel_requested_at` being non-null is irrelevant (task is
|
||||
already terminal). Worth noting in tasks.md that the reaper's requeue path should NOT
|
||||
requeue (re-assign) a task with `cancel_requested_at` set — it should mark it
|
||||
`cancelled` instead of `queued`, or the cancellation would be silently dropped on
|
||||
retry.
|
||||
- [`tasks:submit`-scoped cancel with no per-task ownership means any authorized submitter
|
||||
can cancel any other submitter's task] → Accepted for this change (Non-Goals); this
|
||||
matches the existing coarse-grained scope model everywhere else in the SDK (task
|
||||
reading is likewise not ownership-scoped) and narrowing it is `cloud-console-governance`
|
||||
territory, not this change's.
|
||||
- [Widening `TaskStatus`/`ScheduledTaskStatus`/`TaskAttemptRow.status` literals is a
|
||||
backward-compatible additive change in Python, but any exhaustive `switch`/discriminated
|
||||
union in the TypeScript Console (`types.ts`'s `TaskStatus`) will fail to compile until
|
||||
updated] → Caught at Console build time (Vite/`vue-tsc`), not runtime; tasks.md includes
|
||||
updating `cloud-console/src/types.ts` and any exhaustiveness-checked switch in the same
|
||||
commit as the backend change.
|
||||
- [Race: cancel request arrives between the Host's `claim_assignment` (queued→dispatched
|
||||
transition happens inside `claim_assignment`, not `assign_task`) and its first renewal]
|
||||
→ Already handled by D2's durable-row design: the cancel call checks current `status`
|
||||
regardless of exactly when the Host claims, and `renew_lease` always re-reads the
|
||||
current row, so there's no window where the signal is lost — only a window (bounded by
|
||||
D1's latency) where it hasn't been observed yet.
|
||||
|
||||
## Migration Plan
|
||||
|
||||
1. Ship the Alembic migration (0012) — additive nullable column, no backfill needed, safe
|
||||
to apply with the application running (existing tasks get `cancel_requested_at = NULL`,
|
||||
behaviorally identical to today).
|
||||
2. Deploy Cloud API with the new repository methods, internal `renew_assignment` response
|
||||
field (`cancel_requested`, defaults `False` — old Host Agents ignore unknown fields),
|
||||
and the new public cancel endpoint. This step alone is a no-op for existing Hosts:
|
||||
nothing calls the new endpoint yet, and `LeaseRenewalResponse` gaining an optional
|
||||
field is backward-compatible with any Host Agent version already deployed (Pydantic
|
||||
response models are additive-safe for JSON-decoding clients that only read known
|
||||
fields).
|
||||
3. Deploy Host Agents with the updated `lease.py`/`assignment.py`/`client.py` that read
|
||||
and act on `cancel_requested`. Hosts not yet updated simply never observe cancellation
|
||||
requests (task stays "pending cancellation" until its lease naturally expires and the
|
||||
reaper marks it `cancelled` per the Risks section) — a soft-fail, not a hard error.
|
||||
4. Ship the Cloud Console and Host Agent local console UI changes last, once the backend
|
||||
contract is stable.
|
||||
5. Rollback: the migration is purely additive and safe to leave in place even if the
|
||||
feature is disabled; no rollback migration is required beyond the standard Alembic
|
||||
downgrade (drop the column) if a full revert is ever needed.
|
||||
|
||||
## Open Questions
|
||||
|
||||
- Whether the public cancel endpoint should also accept an optional caller-supplied
|
||||
reason string (for audit/UI display) — left to tasks.md to decide during
|
||||
implementation; not a design-blocking question since it's purely additive to
|
||||
`TaskCancellationRequest` if added later.
|
||||
- Whether `reap_expired_leases`' "mark cancelled instead of requeue when
|
||||
`cancel_requested_at` is set" behavior (noted in Risks) needs its own explicit
|
||||
requirement in the `task-scheduler` delta spec or can be covered as an implementation
|
||||
detail of the existing restart-recovery requirement — resolve when writing specs.md.
|
||||
Reference in New Issue
Block a user