fix(jobs): let a requested cancel survive a fresh read of the run

[chenmingwei23]

What is the problem?

JobSDK.cancel sets an in-memory threading.Event on the live handle and
writes nothing (src/kiro_crew/apps/job_sdk.py). _public_view served no field
for that request (src/kiro_crew/apps/job_routes.py), so the ONLY report of a
requested cancel was the body of the cancel call itself: _handle_cancel's
{"cancelling": true, ...}.

Every later read said nothing. GET /_jobs/{run_id} and GET /_jobs/active
both returned the run's status unchanged - still running - until the worker
reached its next checkpoint. For a runner that checkpoints minutes apart, that
window is minutes.

Why this issue matters to the user

Surviving navigation is the Job SDK's entire thesis. Its own module docstring
says the SDK exists because "a task of mine is running" had no server-side
representation: the fact lived in the React component that started it, so
navigating away destroyed the fact while the work kept going.

cancelling was the one fact still living only in that component. A UI could
show "cancelling..." for exactly as long as the tab that asked stayed mounted. A
reload, a navigation away and back, a second tab, or the fresh mount the SDK was
built to serve all saw running and no evidence the button had worked - so the
user's own action was the one thing the durable record could not tell them about.

How our fix solves it

The obvious fix is the wrong one, and that is the whole shape of this change.
Writing cancel_requested_at from cancel() would make it a second writer
on that run's file. One-writer-per-run is load-bearing rather than incidental:
atomic_write gives crash-safety but not mutual exclusion, so two writers on
one path silently drop the loser's update. That would be a worse bug than the
one being fixed.

So the fact is derived on read instead of written down:

• JobSDK.cancelling_ids() returns the run ids whose handle has its cancelled
  event set, read from the live table under the SDK's own lock. This is the same
  table reconcile already derives liveness from - no new state, no new writer,
  single-writer rule untouched.
• _public_view serves cancelling: bool from that snapshot, so every read path
  (start, active, recent, {run_id}, cancel) carries it.

Chained from symptom to cause: the symptom is a re-read that shows running; the
cause is that the request was never readable anywhere but the cancel response;
the constraint is that it must not become writable; the remedy is a read-side
derivation from the process that already holds the fact.

Three details the chain forced:

• A terminal run is never cancelling. The worker writes the record and then
  _execute drops the live entry, so a read landing between those two would
  otherwise serve status: cancelled together with cancelling: true - the same
  cancel both finished and still outstanding.
• The snapshot is taken off the event loop. _persist holds that same lock
  across a disk write, so acquiring it on the loop could park the gateway for the
  length of that write. _read_with_cancelling pairs the snapshot with the store
  read that was already off-loop work, so no hop is added.
• One snapshot per response, taken after the record read. One per row would
  let two rows of a list disagree about a cancel that landed mid-render; taking
  it after the record means a cancel arriving during the request is reported
  rather than missed.

cancelling is a required argument to _public_view, not a defaulted one:
a future call site that forgets it is a type error rather than a response that
quietly says false.

Not durable across a restart, deliberately - a restarted gateway has no live
table and reconcile has already resolved the run to interrupted, so there is
no pending cancel left to report.

What tests we did

Six new tests, all verified red on base (KeyError: 'cancelling' on the
routes; AttributeError: 'JobSDK' object has no attribute 'cancelling_ids' on
the SDK) by reverting only the two source files and re-running:

Test	What it pins
test_requested_cancel_is_visible_to_a_later_read_of_the_run	a fresh GET {run_id} after the cancel reports cancelling: true AND status: running - the reload / second-tab case
test_requested_cancel_is_visible_in_the_active_list	GET /active, the list a fresh mount adopts, carries it too
test_a_run_nobody_cancelled_reads_not_cancelling	the flag is derived, not decoration - an untouched run reads false
test_a_recorded_cancel_is_no_longer_cancelling	once the status carries the answer, cancelling is false (the terminal guard)
TestCancellingIds::test_request_is_reported_until_the_worker_records_it	SDK level: visible while the record still says running, gone once the worker records CANCELLED
TestCancellingIds::test_a_refused_cancel_reports_nothing	the snapshot follows the ACCEPTED request, not the attempt (live but not declared cancellable)

The route tests use a runner whose next checkpoint is deliberately far away -
it does not poll handle.cancelled until the test releases it - so the window
under test is held open rather than raced. Each release happens in a finally,
so an assertion failure cannot park a worker for the rest of the session.

Gates run locally: test_job_routes.py + test_job_sdk.py 114 passed,
flake8 clean, mypy clean on both changed modules, black --check clean on
all four files, and scripts/check_black_formatting.py passes in scope.

Any other suggestions on the work

• A frontend consumer is still needed to make this visible to a human. P1
  ships with no _jobs consumer (grep finds none under website/src), so this
  makes the fact available; the "cancelling..." pill is the consumer's job.
  Code Review Sage already renders exactly that shape from its own
  cancel_requested_at, which is the pattern to copy.
• docs/system-specs/features/app-sdk-durable-jobs-and-view-state.md was
  deliberately not touched. It is the pre-feat(apps): durable job runtime and the shared _jobs surface #6682 problem statement and still
  says the App SDK "has no job service" - already stale from that merge, not
  from this change, and correcting it belongs with whoever owns that spec rather
  than in a bug fix's diff. The authoritative field list is the _public_view
  docstring, which this change updates, along with the module docstring's
  one-writer-per-run section so it names the derived read.
• Worth noting for P2: if a progress channel returns, the worker stops being
  the sole writer and this derivation's terminal guard is one of the places that
  will need re-reading against the new write pattern.

Pattern harvest

Rule candidate: review-prompt

Pattern: a mutation handler reports state it just changed with a response key
that the same resource's READ serializer does not carry. The acknowledgement
then lives only in the caller that made the call, so any client that re-reads
instead of holding that response cannot see the change. Here it was
_handle_cancel returning cancelling: true while _public_view had no such
field.

Detectable signature, cheap enough for a reviewer to run: a key that appears in
a mutation response body and in no read serializer for the same resource. It is
a review-prompt rather than a semgrep rule because deciding "same resource"
needs the route family, which the AST does not carry. The narrower half IS
mechanizable and worth filing separately if the class recurs: within one routes
module, flag a response literal key that no _public_view-style function emits.

Generalizes in this repo rather than being one file's accident: the SAME question
has three different answers on main today. Code Review Sage persists it
(cancel_requested_at on the run record, which it can afford because it owns
both writers). Dev Fleet overlays it at request time
(_with_live_run_pointers). The Job SDK did neither, which is the defect. Any
new durable-run surface faces the same fork, and the constraint that picks the
answer is who is allowed to write the record -- worth asking at review time
rather than after the fact.

Closes #7589

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 8eee286d739fd8d8ad907b843893306a7a397cc3 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A read-side derivation from state the process already holds, correctly refusing the second-writer alternative; additive contract, coherent restart story, races reasoned in the safe direction.

[DESIGN-REVIEWED] 8eee286

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 8eee286d739fd8d8ad907b843893306a7a397cc3 — this comment is updated in place on each push.

Review details

No candidates were produced by the discovery pass, and my own inspection confirms the change is sound: the not run.is_terminal guard closes the write-terminal-then-drop-live window (_write_terminal runs before _live.pop at job_sdk.py:692-694), cancelling_ids reads the live table under self._lock without writing (single-writer rule intact), and the read-before-snapshot ordering in _read_with_cancelling fails toward reporting a just-requested cancel rather than missing it. No reachable defect on the changed lines.

No findings.

[OPUS-REVIEWED] 8eee286

Verdict parsed from the review's SHA-scoped output markers for commit 8eee286d739fd8d8ad907b843893306a7a397cc3.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 8eee286d739fd8d8ad907b843893306a7a397cc3: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 8eee286d739fd8d8ad907b843893306a7a397cc3 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All mechanical checks are done: the _jobs HTTP surface has zero frontend callers (grepped /_jobs under website/src: 0 matches), cancelling_ids is genuinely new (no prior accessor exposes the handle's cancelled event), and Code Review Sage's cancel_requested_at lives in its own per-app store, not a reusable shared mechanism. Final review follows.

First-Principles-Verdict: CONCERNS

The fix is real and cause-level, but its one new public field has zero counted consumers — the defect stays human-invisible until an unbuilt pill lands.

What this change ships

Intent: make a requested cancel visible to any later read of a job run, not just the cancel response — a FIX.

1. Every _jobs read (start/active/recent/get/cancel) now reports cancelling — justified (reported defect Job SDK: a requested cancel does not survive navigation #7589; derived on read, not a second writer).
2. New SDK method cancelling_ids() — justified; 1 consumer, but the set shape serves the two list endpoints, not generalization.
3. _public_view now requires the snapshot argument — justified (forgetting it is a type error, not a silent false).
4. One off-loop snapshot per response, taken after the record read — justified (documented _persist lock hazard).
5. A terminal run never reports cancelling — justified (blocks a self-contradictory response).
6. Module docstring's one-writer section now names the derived read — justified (same-commit spec rule).
7. Six tests, red on base — tests.

Watch

• cancelling ships with ZERO consumers (grepped /_jobs under website/src: 0 matches; no builtin app uses JobSDK either). The author declares this — "the 'cancelling…' pill is the consumer's job" — so the reported harm (a user seeing no evidence their button worked) is made solvable, not solved. A human should decide whether the pill lands with this or immediately after; the field is one-way-door API surface once any App Kit app reads it.

[FIRST-PRINCIPLES-REVIEWED] 8eee286

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 8eee286d739fd8d8ad907b843893306a7a397cc3 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/apps/job_routes.py:149 -- "read(), sdk.cancelling_ids()" can return stale running with cancelling: false when cancellation completes between them -> Fix: snapshot cancelling IDs before reading the record. (origin: validation)
[GPT-REVIEWED] 8eee286

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 8eee286d739fd8d8ad907b843893306a7a397cc3: <one-sentence reason>

[bolichen97]

Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix with a clear root cause -- a requested cancel must survive a fresh read of the run, derived on read from the live table rather than persisted, preserving the single-writer rule.

[iamwhatever]

Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix -- a requested cancel was invisible to any later read because cancel deliberately writes nothing (single-writer rule), so the fix derives it on read via the new cancelling_ids() snapshot and surfaces it as a cancelling field, with one snapshot per response and terminal runs masked; 2 source files plus their 2 test files, no auth/input-parsing/trust-boundary/sandbox/gate/secret surface touched.