P1: Complete cancellation propagation outside the established stream path (#225)

kilo commented

2026-10-27 22:54:12 +00:00

Member

Refs #225 (P1, successor of excluded #165; disposition D-083). Completes client-disconnect/cancel propagation on the execution-stream paths outside the already-proven established-stream path. Minimal local fixes; no execution-coordination redesign, no worker-protocol change, no retry of ambiguous execution, and the intentional ambiguous-outcome contract (#165 path 4) is untouched.

Defects reproduced red, then fixed green (deterministic seams, no sleeps)

Chunk-branch cancelled return without cancel send — worker_bridged_adapter.py _await_outcome()/_await_resume(): when the cancel event is set while blocked in pending.take() and a chunk lands in that same call, the branch returned cancelled without informing the worker (the loop never revisits the top-of-loop check). Fix: send the best-effort cancel (honoring cancel_sent, so cancellation effects are never duplicated) before the honest cancelled return; the resume leg keeps dropping its suspension handle.
Tool-suspension emissions uncaught — both legs' suspension branches emitted the tool-call/finish frames outside any ClientDisconnectedError handling, so a disconnect there left the backend turn suspended with no cancel. Fix: same cancel-before-rethrow wrapper as the ordinary chunk branch (matching the established covered-path semantics).
Chat Completions SSE header writes outside disconnect translation — _StreamState._begin(): send_response/send_header/end_headers sat outside _write_frame()'s translation, so a header flush failure escaped as raw OSError and the coordinator's cancel path never engaged. Fix: translated to ClientDisconnectedError.
Same shape in the Responses stream — _ResponsesStreamState._write_events() (added with #181/D-080) duplicated the untranslated pattern. Fix: identical translation, so both SSE surfaces cancel identically.

Red/green evidence

Six new deterministic tests (no timeout/sleep-based correctness):

AdapterUncoveredCancellationTests (4): scripted PendingAttempt seams place the cancel inside the take() window / raise ClientDisconnectedError from the suspension emissions; each failed on the pre-fix source (empty cancel record) and passes post-fix asserting exactly one cancel notice plus the honest outcome. The public loop cannot reach these windows deterministically — the top-of-loop check consumes any cancel set outside take() — hence the seams.
SseHeaderDisconnectTests (2): failing-header handler stubs; pre-fix raised raw BrokenPipeError (tracebacks at gateway_server.py:775/:919), post-fix raises ClientDisconnectedError.

Invariants preserved (verified by the existing suites)

Ambiguous-outcome contract unchanged (worker_connection_lost/interrupted/TOOL_RESULT_AMBIGUOUS → AdapterAmbiguousError, never retried); no duplicate cancellation effects (cancel_sent guard); no false completed/cancelled outcomes; D-062 tool-continuation and streaming contracts green; PR #164's merged test synchronization untouched.

Validation

uv sync --only-dev; make check 3063 tests OK / 16 runtime-probe skips (715.5s — includes the 6 new tests); uv run basedpyright 0 errors/0 warnings/0 notes; git diff --check clean. Synthetic/offline only; no live provider, no credentials.

Refs #225 (P1, successor of excluded #165; disposition D-083). Completes client-disconnect/cancel propagation on the execution-stream paths outside the already-proven established-stream path. Minimal local fixes; no execution-coordination redesign, no worker-protocol change, no retry of ambiguous execution, and the intentional ambiguous-outcome contract (#165 path 4) is untouched. ## Defects reproduced red, then fixed green (deterministic seams, no sleeps) 1. **Chunk-branch cancelled return without cancel send** — `worker_bridged_adapter.py` `_await_outcome()`/`_await_resume()`: when the cancel event is set while blocked in `pending.take()` and a chunk lands in that same call, the branch returned `cancelled` without informing the worker (the loop never revisits the top-of-loop check). Fix: send the best-effort cancel (honoring `cancel_sent`, so cancellation effects are never duplicated) before the honest cancelled return; the resume leg keeps dropping its suspension handle. 2. **Tool-suspension emissions uncaught** — both legs' suspension branches emitted the tool-call/finish frames outside any `ClientDisconnectedError` handling, so a disconnect there left the backend turn suspended with no cancel. Fix: same cancel-before-rethrow wrapper as the ordinary chunk branch (matching the established covered-path semantics). 3. **Chat Completions SSE header writes outside disconnect translation** — `_StreamState._begin()`: `send_response/send_header/end_headers` sat outside `_write_frame()`'s translation, so a header flush failure escaped as raw `OSError` and the coordinator's cancel path never engaged. Fix: translated to `ClientDisconnectedError`. 4. **Same shape in the Responses stream** — `_ResponsesStreamState._write_events()` (added with #181/D-080) duplicated the untranslated pattern. Fix: identical translation, so both SSE surfaces cancel identically. ## Red/green evidence Six new deterministic tests (no timeout/sleep-based correctness): - `AdapterUncoveredCancellationTests` (4): scripted `PendingAttempt` seams place the cancel inside the `take()` window / raise `ClientDisconnectedError` from the suspension emissions; each failed on the pre-fix source (empty cancel record) and passes post-fix asserting exactly one cancel notice plus the honest outcome. The public loop cannot reach these windows deterministically — the top-of-loop check consumes any cancel set outside `take()` — hence the seams. - `SseHeaderDisconnectTests` (2): failing-header handler stubs; pre-fix raised raw `BrokenPipeError` (tracebacks at `gateway_server.py:775`/`:919`), post-fix raises `ClientDisconnectedError`. ## Invariants preserved (verified by the existing suites) Ambiguous-outcome contract unchanged (`worker_connection_lost`/`interrupted`/`TOOL_RESULT_AMBIGUOUS` → `AdapterAmbiguousError`, never retried); no duplicate cancellation effects (`cancel_sent` guard); no false completed/cancelled outcomes; D-062 tool-continuation and streaming contracts green; PR #164's merged test synchronization untouched. ## Validation `uv sync --only-dev`; `make check` 3063 tests OK / 16 runtime-probe skips (715.5s — includes the 6 new tests); `uv run basedpyright` 0 errors/0 warnings/0 notes; `git diff --check` clean. Synthetic/offline only; no live provider, no credentials.

kilo added 1 commit

2026-10-27 22:54:12 +00:00

Complete cancellation propagation outside the established stream path (#225 )

ci / check (pull_request) Failing after 4s

Details

ebb5a4093f

kilo commented

2026-10-27 22:54:29 +00:00

Author

Member

Review ordinal record: #1 (initial independent review) reserved for a fresh native read-only Kilo reviewer session (pr-reviewer role) over frozen HEAD ebb5a4093feda19d4e73e0769ecf4b081596398c, base/merge-base 4a182affa4775a4a38b47860ad7622a099c56c4a. Default finite bound 10 whole-PR reviews for this delivery. Source frozen; no pushes/edits during review.

Review ordinal record: **#1** (initial independent review) reserved for a fresh native read-only Kilo reviewer session (pr-reviewer role) over frozen HEAD `ebb5a4093feda19d4e73e0769ecf4b081596398c`, base/merge-base `4a182affa4775a4a38b47860ad7622a099c56c4a`. Default finite bound 10 whole-PR reviews for this delivery. Source frozen; no pushes/edits during review.

kilo commented

2026-10-28 11:07:28 +00:00

Author

Member

Preserved reviewer dispatch payload (native session creation is currently not processing agent-initiated requests; if a reviewer session is started from the Agent Manager UI, begin with exactly this task — note it must review the reconciled final HEAD/base after PR #229 integrates, re-verified at dispatch time):

Perform an independent, read-only whole-PR correctness AND security review of BioMedical-IT/scarcity-router PR #230 (P1 cancellation fix, successor of excluded #165). Review the exact final HEAD/base recorded at dispatch time (creation-time values: HEAD ebb5a4093feda19d4e73e0769ecf4b081596398c branch issue-225-cancel-paths; base = merge-base = the then-current develop — re-verify both from canonical Forgejo PR metadata and freshly fetched develop before starting; any HEAD/base change invalidates prior validation and requires re-checks).

Scope: worker_bridged_adapter.py chunk-branch cancel-send before cancelled return (_await_outcome/_await_resume, cancel_sent guard, resume leg drops its handle); suspension-emission cancel-before-rethrow in both legs; gateway_server.py _StreamState._begin() and _ResponsesStreamState._write_events() header-write failures translated to ClientDisconnectedError. Six new deterministic seam tests (AdapterUncoveredCancellationTests, SseHeaderDisconnectTests) reproduced each defect red on the pre-fix source.

Verify especially: exactly-once cancel propagation (no duplicate effects, cancel_sent respected); honest outcomes (no false completed/cancelled); the ambiguous-outcome contract unchanged and never retried (worker_connection_lost/interrupted/TOOL_RESULT_AMBIGUOUS → AdapterAmbiguousError); D-062 continuation and streaming contracts untouched; no execution-coordination redesign; no new public contract/credential/network surface; concurrency correctness of the seam tests (deterministic, no sleeps); secret-free diff. Implementer's gate: make check 3063 OK / 16 skips, basedpyright 0/0/0 — targeted reruns allowed via the read-only allowlist.

Deliver: verdict exactly APPROVE / CHANGES_REQUESTED / COMMENT; findings list (empty if none); limitations; a UX impact: line; the frozen refs actually reviewed. Modify nothing; push nothing; read-only Forgejo getters only.

Preserved reviewer dispatch payload (native session creation is currently not processing agent-initiated requests; if a reviewer session is started from the Agent Manager UI, begin with exactly this task — note it must review the reconciled final HEAD/base after PR #229 integrates, re-verified at dispatch time): > Perform an independent, read-only whole-PR correctness AND security review of BioMedical-IT/scarcity-router PR #230 (P1 cancellation fix, successor of excluded #165). Review the exact final HEAD/base recorded at dispatch time (creation-time values: HEAD `ebb5a4093feda19d4e73e0769ecf4b081596398c` branch `issue-225-cancel-paths`; base = merge-base = the then-current `develop` — re-verify both from canonical Forgejo PR metadata and freshly fetched `develop` before starting; any HEAD/base change invalidates prior validation and requires re-checks). > > Scope: `worker_bridged_adapter.py` chunk-branch cancel-send before cancelled return (`_await_outcome`/`_await_resume`, `cancel_sent` guard, resume leg drops its handle); suspension-emission cancel-before-rethrow in both legs; `gateway_server.py` `_StreamState._begin()` and `_ResponsesStreamState._write_events()` header-write failures translated to `ClientDisconnectedError`. Six new deterministic seam tests (`AdapterUncoveredCancellationTests`, `SseHeaderDisconnectTests`) reproduced each defect red on the pre-fix source. > > Verify especially: exactly-once cancel propagation (no duplicate effects, `cancel_sent` respected); honest outcomes (no false completed/cancelled); the ambiguous-outcome contract unchanged and never retried (`worker_connection_lost`/`interrupted`/`TOOL_RESULT_AMBIGUOUS` → `AdapterAmbiguousError`); D-062 continuation and streaming contracts untouched; no execution-coordination redesign; no new public contract/credential/network surface; concurrency correctness of the seam tests (deterministic, no sleeps); secret-free diff. Implementer's gate: make check 3063 OK / 16 skips, basedpyright 0/0/0 — targeted reruns allowed via the read-only allowlist. > > Deliver: verdict exactly APPROVE / CHANGES_REQUESTED / COMMENT; findings list (empty if none); limitations; a `UX impact:` line; the frozen refs actually reviewed. Modify nothing; push nothing; read-only Forgejo getters only.

kilo commented

2026-10-28 11:41:55 +00:00

Author

Member

Reconciled after PR229 integrated at develop2c40e0e1fb79397c97e6fa2b7dac0365d9712c0c. Published HEAD remains ebb5a4093f; merge-base4a182affa4775a4a38b47860ad7622a099c56c4a. No history rewrite, synchronization merge or reimplementation. git merge-tree is conflict-free, tree7b0416513ab3859277ffbcb98e517d2066ce1b07. Validated that integration tree in the SAME checkout using unreferenced detached validation object4c0981c5afaaf89316ee2bc5d3d0130db0f3f7f6 (not published/on a branch), then restored exact published HEAD. Sanitized env-i/synthetic HOME/TMPDIR, locked dev deps: uv sync --only-dev; make check3063 tests OK/18 skips in725.455s; basedpyright0/0/0; whitespace clean. Two additional skips vs ambient prior run are reported honestly, not claimed executed. Existing reservation #1 is now imminent native GLM-5.3/zai-coding-plan pr-reviewer review of full PR against exact base2c40e0e/HEADebb5a40/merge-base4a182af; default maximum10. Source frozen during review. Native Agent Manager creation is now verified working with proper JSON-null arguments; no owner relay.

Reconciled after PR229 integrated at develop2c40e0e1fb79397c97e6fa2b7dac0365d9712c0c. Published HEAD remains ebb5a4093feda19d4e73e0769ecf4b081596398c; merge-base4a182affa4775a4a38b47860ad7622a099c56c4a. No history rewrite, synchronization merge or reimplementation. git merge-tree is conflict-free, tree7b0416513ab3859277ffbcb98e517d2066ce1b07. Validated that integration tree in the SAME checkout using unreferenced detached validation object4c0981c5afaaf89316ee2bc5d3d0130db0f3f7f6 (not published/on a branch), then restored exact published HEAD. Sanitized env-i/synthetic HOME/TMPDIR, locked dev deps: uv sync --only-dev; make check3063 tests OK/18 skips in725.455s; basedpyright0/0/0; whitespace clean. Two additional skips vs ambient prior run are reported honestly, not claimed executed. Existing reservation #1 is now imminent native GLM-5.3/zai-coding-plan pr-reviewer review of full PR against exact base2c40e0e/HEADebb5a40/merge-base4a182af; default maximum10. Source frozen during review. Native Agent Manager creation is now verified working with proper JSON-null arguments; no owner relay.

kilo commented

2026-10-28 11:42:40 +00:00

Author

Member

Actual ordinal1 native reviewer now exists: ses_e7dae7b1cffe7M3L5csT38SQmR (Agent Manager overview busy), requestam-1793187742899-cjvnl6, GLM-5.3/zai-coding-plan, pr-reviewer read-only role. Complete PR correctness/security/UX scope at HEADebb5a4093feda19d4e73e0769ecf4b081596398c, current base2c40e0e1fb79397c97e6fa2b7dac0365d9712c0c, merge-base4a182affa4775a4a38b47860ad7622a099c56c4a. Source frozen; no owner relay, Task/subagents or reviewer environment.

kilo commented

2026-10-28 11:58:01 +00:00

Author

Member

Ordinal1/10 final independent GLM-5.3 correctness/security/UX result retrieved natively from ses_e7dae7b1cffe7M3L5csT38SQmR: APPROVE, findings=[], reviewed HEADebb5a4093feda19d4e73e0769ecf4b081596398c/base2c40e0e1fb79397c97e6fa2b7dac0365d9712c0c/merge-base4a182affa4775a4a38b47860ad7622a099c56c4a. Reviewer inspected complete canonical4-file diff and local exact source; Bash denied in role, so canonical getter refs + frozen-local-source verification used without permission changes. Primary verifies actual Git objects/clean checkout/validated merge tree. Same-source clarification substantiated AC2/4 rather than waiving them: send_cancel writes a frame per call (not write-idempotent); worker_client dispatch sets per-attempt Event, so repeated notices are effect-idempotent. Backend cancellation immediate; old claimed record closes immediately; newly WAITING handles are released through existing absolute-deadline expiry/reaper, callback exactly once per record. Reviewer traced control_server->control_api->registry.expire_due->cancel_suspension and cited existing exactly-once/expiry tests; no material uncertainty, no source change or new lifecycle semantics. UX improves immediate backend stop on disconnect; no new friction. Full sanitized merged-tree gate3063 OK/18 skips,basedpyright0/0/0,whitespace clean. No unresolved findings; proceeding to currentness/authorized merge.

kilo merged commit 04f40ddf59 into develop

2026-10-28 11:58:10 +00:00

kilo referenced this pull request from a commit

2026-10-28 11:58:12 +00:00

Merge pull request #230: P1 cancellation propagation

Rows
Columns

P1: Complete cancellation propagation outside the established stream path (#225) #230

Defects reproduced red, then fixed green (deterministic seams, no sleeps)

Red/green evidence

Invariants preserved (verified by the existing suites)

Validation