close
Skip to content

fix(dashboard): stop a hung usage fetch parking the credit pill forever - #2498

Merged
iamwhatever merged 1 commit into
kirodotdev:mainfrom
isotope14:fix/usage-fetch-deadline
Aug 10, 2026
Merged

fix(dashboard): stop a hung usage fetch parking the credit pill forever#2498
iamwhatever merged 1 commit into
kirodotdev:mainfrom
isotope14:fix/usage-fetch-deadline

Conversation

@isotope14

Copy link
Copy Markdown
Contributor

Fixes #2364.

The bug

_fetch_usage_bg() is gated by a module-level _usage_fetching flag that is set
before the work and cleared only in the function's finally. That finally is
correct as far as it goes — it does run on CancelledError, so the cancellation
mechanism named in the report is not the cause.

What it cannot survive is a hang, because the refresh has no overall deadline.
fetch_usage_limits is awaited via run_in_executor(subprocess_executor(), ...)
with no wait_for, and the urlopen(timeout=15) inside it does not cover
getaddrinfo — nor the wait for a free worker in a bounded pool. If that call
blocks, the coroutine never reaches its finally, _usage_fetching stays True
for the process lifetime, and every subsequent refresh returns immediately at
the guard.

_usage_cache therefore stays {} forever. The frontend cannot recover on its
own: the useQuery at App.tsx returns null unless credits_plan is finite or
available === false, and !kiroUsage renders the spinner — so a never-populated
cache is indistinguishable from a warming one, and the pill reads
"Checking usage..." indefinitely with nothing logged.

The fix

One ceiling over the whole refresh. Every await inside is either already bounded
(whoami ≤30s, the billed scrape ≤60s) or an executor call that can block
indefinitely; bounding the total is the invariant that actually matters, and it
cannot be defeated by a future await being added inside.

A timeout lands in the existing except asyncio.TimeoutError handler, which
calls _cache_transient_failure() — keeping the last good value as stale, or
caching {"available": False} when there is nothing to show. Either way the pill
resolves instead of spinning.

asyncio.timeout() would express this without restructuring, but it needs 3.11
and requires-python is >=3.10, so the body is lifted into a nested coroutine
for wait_for. Its indentation is unchanged, so the real diff is 22 lines —
git diff -w and git diff agree.

Verification

Two tests in the new TestFetchUsageDeadline:

  • test_hung_api_read_still_clears_the_guardfetch_usage_limits blocks on an
    event; asserts _fetch_usage_bg() returns, _usage_fetching is False, and the
    cache resolves to available: False.
  • test_refresh_after_a_hang_can_still_succeed — proves the guard is not merely
    cleared but that a later refresh populates the cache normally.

Both patch the deadline with raising=False so they do not depend on the new
constant existing, which is what makes them exercise the real hang on unfixed
code rather than erroring on a missing attribute.

Causal proof — with the tests in place, reverting only
src/kiro_crew/dashboard/handlers/sessions.py to origin/main:

$ git checkout origin/main -- src/kiro_crew/dashboard/handlers/sessions.py
$ pytest test/test_session_usage.py::TestFetchUsageDeadline
E   TimeoutError          # the test's own wait_for(10) fires — _fetch_usage_bg never returns
2 failed

With the fix restored: 2 passed in 0.70s.

Local gates: every test file importing this module — test_session_usage.py,
test_usage.py, test_sessions_health_cache.py, test_session_keepalive.py,
test_computer_use_api.py, test_dashboard_sessions_memory.py and 8 more —
737 passed, 0 failures. Plus test_kiro_usage_api.py and
test_kiro_spawn_readiness_gate.py. isort, flake8 clean; mypy clean on
sessions.py (mypy 1.14.1, matching the pyproject.toml pin).

Scope notes

  • The deadline is 180s: above the sum of the bounded inner steps so a healthy
    slow refresh still completes, well below the 600s refresh interval so a wedged
    one always clears before the next attempt.
  • A timestamped guard (so the flag self-heals independently) was considered and
    left out — with the deadline in place there is no path from _usage_fetching = True to a missed finally, so it would be redundant machinery.
  • The report also predicted an API-key-auth readiness asymmetry (KIRO_API_KEY
    appears nowhere in the repo, and the readiness probe's env allowlist omits it,
    which would 503 this endpoint). That is a separate question — it would also
    break /api/models, regenerate and rewind, which the reporter did not observe
    — so it is deliberately not addressed here. This change fixes the wedge that
    makes the pill spin with no error at all.
  • The frontend spinner-vs-error state is untouched: it is a product call whether
    undefined (fetch rejected) should render differently from null.

@isotope14
isotope14 requested a review from a team as a code owner August 10, 2026 06:55
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

I've read the diff, both AUTOSDE rule snapshots, and the surrounding code in sessions.py.

The change wraps the body of _fetch_usage_bg into a nested _refresh() coroutine and applies a single asyncio.wait_for(..., timeout=180) ceiling. Let me verify the key correctness concern: whether cancelling _refresh while it's blocked in run_in_executor(subprocess_executor(), fetch_usage_limits, ...) actually unwinds promptly.

Tracing the semantics: wait_for timeout cancels the inner coroutine's awaited future. For a run_in_executor future, asyncio.Future.cancel() immediately transitions it to CANCELLED and raises CancelledError at the await point — the executor thread keeps running (a leaked worker, which the no-blocking-call-on-event-loop rule explicitly deems "survivable"), but _refresh unwinds, TimeoutError reaches the outer handler, and the finally clears _usage_fetching. The nonlocal proc, sandbox_cleanup bindings mean the outer finally still reaps any spawned subprocess. So the guard is genuinely released on the hang path, which is the stated purpose.

I checked for:

  • nonlocal/global scoping — proc, sandbox_cleanup, kiro_bin, scrape_attempted are reachable by the outer except/finally; _usage_cache/_usage_cache_ts assignments hit the module globals. Correct.
  • Guard leak on the new cancellation path — finally handles it.
  • No security/redaction path was altered (redaction, ARN anchoring, identity matching all unchanged).
  • Test file has no side effects outside its globals reset; uses raising=False so it exercises the real hang on unfixed code.

No blocking or reachable semantic defect on the changed lines.

No findings.

[OPUS-REVIEWED] c8c50d2

@github-actions

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] c8c50d2

@github-actions

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Advisory design-level review of c8c50d28ee2f96c3d3d93aaa23cc75b27dc4a48d via the fork AI-review pipeline — updated in place on each push; does not block merge.

Design-Verdict: PASS

Bounding the whole refresh is the right invariant — root-cause fix, lands in the existing failure handler, and abandoned wedged work sits in the pool built to cap exactly that.

[DESIGN-REVIEWED] c8c50d2

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 10, 2026
@iamwhatever
iamwhatever merged commit ec35a9b into kirodotdev:main Aug 10, 2026
52 checks passed
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 10, 2026
encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
…er (kirodotdev#2498)

Co-authored-by: Junfeng Qiu <junfume@Junfengs-Mac-mini-2.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dashboard Kiro Credits pill stuck on 'Checking usage...' indefinitely with API-key auth

2 participants