test(acp): pin the abandoned-turn settle await; retrieve a dropped driver's late failure

[mabry1985]

Summary

Follow-ups from the adversarial review of #3858:

1. Tests now pin the settle await (tests/test_acp_abandoned_turn.py)
   • test_cancelling_the_consumer_mid_stream_stops_the_driver records _driver_stopped(rt) in the consumer's own finally — the moment a caller runs _acp_release — instead of after await consumer (by which point a few loop turns had let the driver finish anyway).
   • test_a_runtime_that_ignores_the_cancel_is_bounded asserts the close actually waited the bound (0.15 <= elapsed < 2.0 at a 0.2s bound) and the warning reads within 0.2s.
2. Late failure retrieved (server/chat_acp.py) — a driver still running after _ACP_CANCEL_SETTLE_S gets a done-callback (_late_outcome_logger) that calls task.exception() and debug-logs it, so a later failure no longer ends as asyncio's "Task exception was never retrieved" at GC. New test test_a_driver_dropped_past_the_bound_has_its_late_failure_retrieved installs a loop exception handler, lets the dropped driver fail after the bound, gc.collect()s, and asserts no "never retrieved" report + the debug line. Also %.0fs → %gs so sub-second bounds don't print "0s".
3. Doc note (a2a_impl/executor.py) — one-line comment at _stall_guarded's wait_for: on a wedged ACP runtime TurnStalled can surface up to _ACP_CANCEL_SETTLE_S later.

Labelled skip-changelog: the production change is debug-level logging for an already-abandoned turn (no user-visible behaviour).

Mutation proofs

M1 — _stop_abandoned_driver cancels without awaiting (done, _ = await asyncio.wait({driver}, timeout=…) → done = {driver} if driver.done() else set()):

• origin/main test file: 4 failed / 3 passed — test_cancelling_the_consumer_mid_stream_stops_the_driver and test_a_runtime_that_ignores_the_cancel_is_bounded stayed green (the gap).
• This PR: both now killed — assert [False] == [True] ("the consumer finished while the driver task was still running") and assert 0.15 <= 0.00046. (6 failed / 2 passed overall.)

M2 — drop the add_done_callback: test_a_driver_dropped_past_the_bound_has_its_late_failure_retrieved killed — the handler received {'message': 'Task exception was never retrieved', 'exception': RuntimeError('boom after the bound'), …}.

Unmutated: ruff check . clean, lint-imports 4 kept / 0 broken, full pytest tests/ 11161 passed / 17 skipped. (server/chat.py untouched.)

Fixes #3860

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

Next included review available in 44 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: d4eecaea-95ec-468b-ac3e-b3e0b477952d

📥 Commits

Reviewing files that changed from the base of the PR and between eedcfc1 and 3b1b4b2.

📒 Files selected for processing (3)

• a2a_impl/executor.py
• server/chat_acp.py
• tests/test_acp_abandoned_turn.py

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[protoreview]

QA panel review — PASS

code-review · head 3b1b4b2e3e01 · formal

Low-risk PR: adds a late-outcome logger for abandoned ACP driver turns and a clarifying comment in the executor. No correctness concerns surfaced across the panel. The one actionable item is the missing changelog.d/ fragment, which PROTO.md lists as a must-pass gate for non-test-only changes. The verifier confirmed the fragment is absent from the diff; the only open question is whether a skip-changelog label is applied on the PR (not verifiable from the diff alone). If the label is present, this finding is a false positive and can be dropped.

Findings

	Severity	Location	Finding	Verified
🟡	minor	changelog.d/3887.fixed.md	The PR changes production code in server/chat_acp.py (adds _late_outcome_logger and calls it in _stop_abandoned_driver) but ships no changelog.d/ fragment, whi…	confirmed

findings JSON (machine-readable)

[
  {
    "file": "changelog.d/3887.fixed.md",
    "line": 0,
    "severity": "minor",
    "category": "conventions",
    "claim": "The PR changes production code in server/chat_acp.py (adds _late_outcome_logger and calls it in _stop_abandoned_driver) but ships no changelog.d/ fragment, which PROTO.md requires for non-test-only changes.",
    "evidence": "The diff touches server/chat_acp.py (adding _late_outcome_logger and driver.add_done_callback(...)) and a2a_impl/executor.py (comment), but no changelog.d/ file is present. PROTO.md: 'a changelog.d/<pr>.<kind>.md fragment \u2014 shape and kinds in changelog.d/README.md' is listed as a must-pass gate.",
    "verdict": "confirmed",
    "note": "Diff confirms no changelog.d/ fragment; PROTO.md gate table explicitly requires one. Caveat: skip-changelog label status unverified (would need API call), so this is confirmed on the fragment-absence alone."
  }
]

---

1 finding(s) excluded from the verdict by in-diff confinement (file not among this PR's changed paths):

• changelog.d/3887.fixed.md (minor) — The PR changes production code in server/chat_acp.py (adds _late_outcome_logger and calls it in _stop_abandoned_driver) but ships no changelog.d/ fragment, whic

[protoreview]

Promoting the PASS verdict for head 3b1b4b2e3e01: all checks terminal-green, zero unresolved review threads. (approve-on-green)

Open findings carried by this approval — non-blocking, but they did not go away:

• minor changelog.d/3887.fixed.md:0 — The PR changes production code in server/chat_acp.py (adds _late_outcome_logger and calls it in _stop_abandoned_driver) but ships no changelog.d/ fragment, which PROTO.md requires for non-test-only ch

Approving a WARN does not resolve its findings (issue #22).