267 lines
18 KiB
Markdown
267 lines
18 KiB
Markdown
## 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.
|