fix(adk): preserve natural subagent completion during drain

[N3kox]

What type of PR is this?

fix

Check the PR title.

• This PR title match the format: <type>(optional scope): <description>
• The description of this PR title is user-oriented and clear enough for others to understand.
• This PR does not require user-facing documentation updates.

(Optional) Translate the PR title into Chinese.

(Optional) More detailed description for this PR(en: English/zh: Chinese).

en:

When a drain request raced with a subagent's natural completion, the background task executor queued the drain control before cancellation completed and ignored the result of CancelHandle.Wait.

If the subagent completed before cancellation took effect, CancelHandle.Wait returned ErrExecutionEnded. However, the queued drain control still overrode the completed result and required a resumable checkpoint. Since natural completion does not produce a checkpoint, the executor returned ErrDrainCheckpointUnavailable, leaving the task running until lease recovery incorrectly marked it as failed.

This PR preserves the cancellation outcome and lets the natural completed result win when a drain loses the race with execution completion. Drains that take effect continue to require a resumable checkpoint and remain fail-closed if that checkpoint is unavailable.

A regression test verifies that the task is committed as completed, its final result is retained, and no checkpoint is required when execution completes naturally before drain cancellation takes effect.

zh(optional):

(Optional) Which issue(s) this PR fixes:

N/A

(optional) The PR that updates user documentation:

N/A

[codecov]

Codecov Report

❌ Patch coverage is 96.29630% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (alpha/10@24279d8). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
adk/backgroundtask/subagent/subagent.go	96.29%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             alpha/10    #1267   +/-   ##
===========================================
  Coverage            ?   82.03%           
===========================================
  Files               ?      219           
  Lines               ?    37452           
  Branches            ?        0           
===========================================
  Hits                ?    30724           
  Misses              ?     4526           
  Partials            ?     2202           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[shentongmartin]

Root cause diagnosis is precise, and consuming CancelHandle.Wait() is the right mechanism. The subtle part — that the old if accepted { handle.Wait() } was itself what discarded the outcome in exactly the racing case — is easy to miss.

Verified locally: new test red on 24279d81 (3/3), green on c744606a (5/5); 40 jittered race iterations deterministic; -race clean; ./adk/backgroundtask/... green; lint 0 issues. The fail-closed invariant for drains that do take effect holds.

Left line comments. One needs resolving before merge (the comment at :514 asserts an invariant that holds for only one of two paths); the rest are nits.

Two items I couldn't anchor inline because they're outside this diff:

• subagent.go:265-267 — SupportsDrain's doc still says drain "captures an ADK Runner checkpoint before returning a suspended result." That's now conditional; a drain that loses the race returns completed with no checkpoint.
• subagent.go:622-654 — handleRunError polls the same controlRequests channel at :629 and calls controlResult at :644, but never consults cancelOutcomes. See the :511-522 comment for why this matters.

[shentongmartin]

Re-reviewed at 1bfe2ea6 (base 24279d81 unchanged).

The blocker is fixed, and fixed the more thorough way. You took the invasive option — wiring the guard into handleRunError rather than just narrowing the comment — and the resolveRunOutcome extraction is better structured than what I proposed: unifying the completion, interrupt and error paths through one decision point removes the whole "two paths disagree" class rather than the single instance I found. Confirmed via the v1 attack suite:

TestAttack_DrainLosesRaceToFatalError
  before: status="running" + ErrDrainCheckpointUnavailable
  now:    status="failed"  resultError="model backend exploded"

The trap I flagged was avoided — the crash isn't laundered into StatusCompleted. All 5 nits addressed; dropping the dead handle == nil branch outright is better than the comment I suggested (verified: all 7 AgentCancelFunc return paths in cancel.go construct a non-nil handle). ./adk/backgroundtask/... green across 7 packages, lint 0 issues, other two SupportsDrain implementers unaffected.

One new blocker introduced by the update — the deleted E2E test. Details on the test file; it's a test-shape regression, not a code defect, and ~40 lines closes it.

Re your reply on the NotErrorIs nit: you're right that require.NoError detects it — that was about diagnostic intent, not detection, and it's a fair judgment call. I accept it. Noting only that the assertion in question no longer exists on this branch, so the point resurfaces inside the E2E comment below; if you restore the test with plain require.NoError, that's fine by me.

Two smaller notes:

• ErrSessionBusy relocation — moving yield handling from handleRunError into resolveRunOutcome (subagent.go:534-541) means it's now evaluated on the Execute path too. Unreachable there in practice (run.err == nil), so behavior is preserved and the suite agrees — but worth a line in the PR description, since someone diffing handleRunError will wonder where the logic went.
• Scope — 2 files/+83 grew to 3 files/+174, all in service of the blocker plus the suggested refactor. Justified, but the description still describes the narrower original change.