[leonlaiyc]

Problem / Motivation

Goal: A transcode that runs while the retired-decoder migration is under way gets the newly installed decoder instead of a "decoder missing" result.

test_stt_decoder.py::TestRetiredDecoderMigration::test_concurrent_callers_share_one_transfer failed on a Linux shard of #23334 (#23360): of two concurrent migrate_retired_decoder() callers, one returned the installed path and the other None. The issue suspected the test's reset of _migration_task. The race is in migrate_retired_decoder itself. A caller that finds no migration started awaits the retired-file probe (asyncio.to_thread(has_retired_decoder)). If another caller starts the migration meanwhile and its transfer installs the pin and removes the retired file before this probe runs, the probe finds no retired file and the caller returns None, even though a migration had just succeeded.

Why it matters

The transcode resolver calls migrate_retired_decoder when nothing resolves, and None means "no decoder". On an install upgrading from a retired pin, a second voice note or dictation that arrives during the first one's migration fails to transcode, even though the decoder it needed was installed moments earlier. On CI the same race shows up as an intermittent red on PRs that do not touch STT.

Not a goal

• Changing when the migration runs, how often, or what DecoderStore.ensure does.
• Changing the other-event-loop behaviour: a caller on a different loop still gets None, as before.

What changed (motivation → approach → change)

• Symptom: of two concurrent callers, one gets None (assert PosixPath('.../ffmpeg-test') == None).
• Root cause: migrate_retired_decoder (stt/decoder.py) checked for a started migration only before the probe's await. A migration that started during that await, and finished and removed the retired file before the probe read the store, left the probe answering "nothing to migrate".
• Change: after the probe, the function checks _migration_task again and joins a migration that was started, whatever the probe found. Both joins go through a new _join_migration helper, which is the existing early-join code moved unchanged: the same-loop guard (None for a task on another loop) and the asyncio.shield that keeps one caller's cancellation from cancelling the shared transfer.

Backwards compatibility

Compatible: a caller that used to get the path still gets it. The only caller whose result changes is one that used to get None while a migration on its own loop had already started, and it now waits for that migration.

Removes nothing: no field, key or path is removed. The function only waits for a transfer it used to ignore.

Tests

Proof: PROVEN test/test_stt_decoder.py::TestRetiredDecoderMigration::test_a_caller_whose_probe_sees_the_migrated_store_joins_the_transfer (head cfc32f6)

• New test_a_caller_whose_probe_sees_the_migrated_store_joins_the_transfer forces the losing order instead of hoping for it. The retired-file probe is wrapped so that whichever call arrives second blocks on a threading.Event (bounded at 10 s). The shared store's ensure sets that event when the first migration finishes. So the second probe always runs on a store where the retired file has already been removed. It asserts that both callers get the installed path and that there is one fetch. On origin/main it fails with assert WindowsPath('.../ffmpeg-test') == None.
• test_concurrent_callers_share_one_transfer, the test that failed in Flaky: test_concurrent_callers_share_one_transfer (test_stt_decoder.py::TestRetiredDecoderMigration) returns None for one caller #23360, is unchanged. The fix removes the order it lost under.
• Local (Windows, Python 3.13): TestRetiredDecoderMigration 9 passed in each of 5 consecutive runs. test_stt_decoder.py 74 passed, 4 skipped. Across test_stt_decoder.py, test_transcribe_ffmpeg.py, test_build_desktop_decoder_requirement.py, test_cli_doctor.py and test_cli_stt_memory_doctor.py: 255 passed, 2 failed. The two failures are TestDoctorStt::test_doctor_stt_transcribe_boto3_missing and ...amazon_transcribe_missing, and they fail the same way with origin/main's decoder.py on this host, so they do not come from this change. test_flake_pattern_ratchet.py 60 passed. black, isort and flake8 are clean on both files, and the comment-history, brand-name and sync-io-in-async gates pass.

Manual verification

N/A — unit coverage sufficient: the new test reproduces the exact interleaving with the real DecoderStore.ensure, the real retired-file probe and a served artifact. Only the network is stubbed.

Related Issues

Fixes #23360

Found on #23334's CI; the test arrived with #23158.

Sibling sweep

Swept: git grep -nE "_task is None|_task = asyncio.ensure_future" -- src/kiro_crew/stt/ (other start-once tasks in stt that probe before starting)
Sibling: src/kiro_crew/stt/decoder.py:604 -- changed here
Sibling: src/kiro_crew/stt/models.py:895 -- not affected -- a detached maybe_autofetch per model download with no start-once global; its probe then store().ensure(), and ensure re-checks the installed file under its own lock

Pattern harvest

Rule candidate: review-prompt
Pattern: a start-once task checked before an await and decided on after it, without re-checking the task, so a task started during the await is missed

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)

• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable): N/A, no spec describes the migration's join behaviour
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

🤖 Generated with Claude Code

[github-actions]

UX Review (Opus 5.5, fork) — ⏭️ skipped

Revision cfc32f67cb41a8f40c52aa26837c73c6e09b758e touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Does not block merge.

[github-actions]

Design Review (Opus 5.5, fork) — ✅ PASS

Design-level review of cfc32f67cb41a8f40c52aa26837c73c6e09b758e via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

Design-Verdict: PASS

Nothing blocks: the fix re-checks the start-once task after the probe's await, which removes the race at its cause and changes no contract.

[DESIGN-REVIEWED] cfc32f6

Description read

[DESCRIPTION-READ] aba6e4a31e5305692c69bc6cdf60ae654e000d9d0c728c9f7c8b79ddad0b2235

That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with .github/scripts/pr-description-capture.sh over the same inputs; if it differs, the description or that evidence changed after this verdict was formed and any finding drawn from either is unproven.

[github-actions]

First Principles Review (Opus 5.5, fork) — ✅ PASS

Premise-level review of cfc32f67cb41a8f40c52aa26837c73c6e09b758e via the fork AI-review pipeline — 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 does not.

First-Principles-Verdict: PASS

Nothing blocks: this fixes the mechanism behind the race seen in #23360, and the new test fails on base.

What this change ships

Inventory (3 items) — 3 justified

Intent: a voice note or dictation sent while the retired-decoder migration is running gets the new decoder instead of "decoder missing". This is a FIX.

1. A caller whose retired-file probe finishes after another caller has already started the migration now waits for that migration instead of returning None. — justified
2. The same-loop guard and shielded wait are moved into _join_migration without changes; the helper has 2 call sites in migrate_retired_decoder. — justified
3. A deterministic test forces the losing order (the second probe blocks until the first ensure has finished). — justified

[FIRST-PRINCIPLES-REVIEWED] cfc32f6

Description read

[DESCRIPTION-READ] 0235991bd706736651d3f5e14a395b044568fc83fa148b519a8aa47780bb6688

That sha256 names the PR description this verdict read. Recompute it with .github/scripts/pr-description-capture.sh; if it differs, the description changed after this verdict was formed and any finding drawn from the description is unproven.

[github-actions]

Opus 5.5 Review (fork) — ✅ no blocking findings

Reviewed cfc32f67cb41a8f40c52aa26837c73c6e09b758e via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] cfc32f6

[github-actions]

GPT 6.1 Review (fork) — ✅ no blocking findings

Reviewed cfc32f67cb41a8f40c52aa26837c73c6e09b758e via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] cfc32f6

[KiroCrewCaptain]

Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, security checklist all-NO, no reviewer-lane findings, readiness signal fresh for this head SHA, declared category matches the diff. Category: the STT decoder migration re-checks for an in-flight migration after the retired-file probe and joins it, closing the race where a second caller reported the decoder missing; covered by a new concurrency test.