fix(tags): auto-resolve on-device LLM tag suggestions that freeze on Downloading... - #262
Merged
Merged
Conversation
Requirements, 6-dimension research, implementation plan (7 epics/49 tasks), 2 ADRs, UX design, and validation/pre-mortem docs for fixing the on-device LLM tag-suggestion sheet freezing on a one-shot "Downloading..." caption with no polling, escalation, or retry path. Plan went through architecture + adversarial review (1 blocker each, both resolved), a pre-mortem (2 P1s found and fixed — elapsed-time tracking now persists across block-switches and manual retries), a cross-artifact consistency pass (3 blockers resolved), and a product triad review (UX accessibility blocker resolved, now READY TO BUILD). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QmtqsYyS4LUEbbLdbG2if5
…stionStatus Replaces TagSuggestionState.Ready's llmPending: Boolean / llmError: String? pair with a single llmStatus: LlmSuggestionStatus field (NotStarted / Pending / Resolved / Stalled / Failed), per plan.md Epic 2 Task 2.1.1. This is the type that lets the UI distinguish "still downloading" from "stalled, needs retry" from "hard failure" instead of collapsing all non-happy-path states into a single frozen "Downloading..." caption with no retry affordance. Epic 2, Story 2.1 of project_plans/llm-tag-download-stall. EXPECTED BREAKAGE: this intentionally breaks compilation in TagSuggestionViewModel.kt and SuggestionBottomSheet.kt (unresolved llmPending/llmError references), and will break ErrorStateNoDeadEndTest.kt once those recompile. Epics 4/5/6 (running separately) fix these downstream call sites to construct/read llmStatus instead. TagChipRow.kt and VoiceCaptureButton.kt were not broken by this change alone (their llmError/llmPending are local parameter names, not references to TagSuggestionState.Ready) but will need updating when Epic 5/6 change TagChipRow's signature. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QmtqsYyS4LUEbbLdbG2if5
…tionEngine Epic 1 of llm-tag-download-stall: stop dropping LlmResult.Failure.OnDeviceUnavailable's retryable flag at the DomainError boundary (root cause of the download-stall bug), and wire a narrow checkAvailability probe into TagSuggestionEngine for the upcoming poll loop (Epic 3/4) to use. - DomainError.NetworkError.RequestFailed gains an additive retryable: Boolean = false field (default keeps all existing call sites compiling unchanged). - LlmTagProvider.suggestTags() now forwards result.retryable instead of dropping it. - TagSuggestionEngine takes an optional checkAvailability probe (public val, defaults to null) so TagSuggestionViewModel can later pass it straight into TagAvailabilityPoller.pollUntilAvailable without an extra wrapper. - App.kt's TagSuggestionEngine construction site wires tagLlmProviderState's existing checkAvailability() through. - Adds LlmTagProviderTest (businessTest) with the direct regression test for the bug: a retryable OnDeviceUnavailable failure maps to a retryable RequestFailed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QmtqsYyS4LUEbbLdbG2if5
Implements Epic 3 of llm-tag-download-stall: a wall-clock-bounded poll loop over LlmProviderAvailability, mirroring GitHubDeviceFlowClient's pollForToken shape so it is directly unit-testable under kotlinx.coroutines.test.runTest with no injected dispatcher/scope. Root-cause fix during implementation: the plan's Clock.System.now()-based while-loop condition is not virtualized by kotlinx-coroutines-test (only delay() suspension points are), so re-querying it every iteration caused the loop to busy-spin at full CPU for the full real-world deadline instead of resolving in virtual time - passing for short deadlines (~12s real wall time) but genuinely failing outright for the 120s-deadline escalation test (UncompletedCoroutinesError, exceeds runTest's 60s watchdog). Switched to tracking elapsed time via accumulated delay() ticks instead of repeated wall-clock reads; production behavior is unchanged (delay() genuinely takes real time outside of tests) and all 6 tests now resolve in true virtual time (~0.2s real time for the full suite, verified via an isolated kotlinc+JUnit run since the full commonMain module currently fails to compile for pre-existing, unrelated reasons - Epic 2's llmPending/llmError removal in TagSuggestionViewModel.kt/ SuggestionBottomSheet.kt, which Epic 4 fixes). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QmtqsYyS4LUEbbLdbG2if5
…uggestionViewModel Implements Epic 4 of the llm-tag-download-stall plan: TagSuggestionViewModel now routes every LLM suggestion attempt through a shared runLlmSuggest() helper that layers TagAvailabilityPoller.pollUntilAvailable (Epic 3) on top of a retryable-unavailable signal (Epic 1) and reports progress via the sealed LlmSuggestionStatus (Epic 2). - Story 4.1: runLlmSuggest() — first attempt, poll-if-retryable, one auto re-run on Available. Constructor gains injectable dispatcher/pollDeadlineMs/ pollIntervalMs/pollEscalationThresholdMs (NFR-3: lets tests run under kotlinx.coroutines.test virtual time instead of real ~120s/~20s waits) plus a session-scoped downloadFirstObservedAtMs so a block-switch or manual retry resumes the existing elapsed-time budget instead of restarting the escalation/deadline clock from zero (pre-mortem P1 #1/#2). - Story 4.2: requestSuggestions() rewritten around runLlmSuggest(); adds retryLastRequest() (FR-3) backed by a stored LastRequest. - Story 4.3: scanEntries() calls runLlmSuggest(..., allowPolling = false, ...) so a stalled on-device model never blocks a bulk scan (FR-7/AC7). - Stories 4.4/4.4b/4.5/4.6: 13 new regression tests covering the stale-block coroutine-lifecycle guarantee, close()/own-deadline termination, the full Pending->Stalled caption sequence, fast/non-retryable no-poll paths, format()-called-at-most-twice (pitfall #2), and elapsed-time persistence across a block-switch-and-return and a manual retry after Stalled. Two of the new tests (Story 4.6) need a small deliberate real delay() rather than pure virtual-time advancement: TagAvailabilityPoller.pollUntilAvailable's startedAtOverride reconciliation reads a real kotlin.time.Clock.System.now() once (by design, already committed in Epic 3, not modified here), which kotlinx.coroutines.test's virtual scheduler cannot influence — so proving "elapsed time survives a relaunch" requires genuine wall-clock time to actually pass, not just virtual time. This is a few hundred milliseconds, not the ~120s/~20s NFR-3 was written to eliminate. Epic 4, project_plans/llm-tag-download-stall. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GUh92Keov7BCtTf8T9io1a
TagSuggestionState.Ready dropped the flat llmError field in favor of the sealed LlmSuggestionStatus (Epic 2). Update the LLM-suggestion-failure test fixture to construct LlmSuggestionStatus.Failed(message, retryable) instead, and pass the new onRetry callback SuggestionBottomSheet requires (Epic 5).
…tionBottomSheet Epic 5 of llm-tag-download-stall: TagChipRow now takes a single llmStatus: LlmSuggestionStatus param instead of the old flat isLlmLoading/llmError pair. SuggestionBottomSheet renders all five caption/retry states (Pending, Stalled, Failed retryable/non-retryable) with LiveRegion.Polite announcements and a structurally-gated Retry TextButton, and wires onRetry -> retryLastRequest() at both JournalsView and PageView call sites. Also updates TagInsertionFlagshipUiTest.kt's SuggestionBottomSheet call site (new required onRetry param) and adds LlmSuggestionCaptionStatesUiTest.kt covering validation.md's 8 automatable UX acceptance criteria for this surface. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GUh92Keov7BCtTf8T9io1a
… load The block-B awaitState call used the 5000ms default, but this test constructs TagSuggestionViewModel with real Dispatchers.Default (no injected test dispatcher). Under full-suite parallel test load, real thread-pool contention pushed the real-time spin-poll past 5000ms even though the underlying cancel-and-relaunch is effectively instantaneous (confirmed via 3x isolated reruns, all passing in <2s). Bumped to 15000ms — a timeout-margin fix, not a logic change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QmtqsYyS4LUEbbLdbG2if5
sdd:6-verify Layer 1/2 fixes: - TagAvailabilityPollerTest's "measures elapsed time from startedAtOverride" test anchored its override to a synthetic epoch value instead of a real Clock.System.now() read. Since pollUntilAvailable computes elapsed time as Clock.System.now() - startedAtOverride, this made the computed elapsed time enormous, so the while-loop's first condition check failed immediately — zero ticks, giving no regression protection for the exact resumed-poll arithmetic (pre-mortem P1 #1/#2's fix) this test exists to cover. Independently caught by both the architecture review and idiom review agents. Fixed to anchor on a real clock read and assert the actual tick count (8), not just the terminal outcome. - TagSuggestionEngine.checkAvailability: import LlmProviderAvailability instead of an inline fully-qualified reference. - TagSuggestionViewModel: replace a !! on a mutable var with a local val binding (provably safe today, but not smart-castable across statements). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QmtqsYyS4LUEbbLdbG2if5
… signal
Code review found two BLOCKER-severity tests that gave zero protection:
both used a bare delay(200) inside runTest, which the coroutine test
scheduler virtualizes to near-zero real time against the real 4000ms
DEFAULT_POLL_INTERVAL_MS. Disabling the underlying cancellation logic
they were meant to guard left both tests passing. Both now construct
the ViewModel with a short pollIntervalMs override and use a genuine
withContext(Dispatchers.Default) { delay(200) } wall-clock wait.
The stale-block-leak test additionally needed a redesign, not just a
timing fix: its final assertion checked the state immediately after a
fresh job's synchronous initial write, before any leaked background job
could run, so it was structurally incapable of catching the bug even
with real time. It's rebuilt so block-B resolves without polling of its
own, isolating checkAvailability() call growth during the wait window
to only a leaked block-A job — verified to fail when suggestionJob
cancellation is disabled, and to pass with it restored.
Also:
- LlmTagProvider's NetworkError branch dropped `retryable`, reproducing
this PR's core "frozen, no retry" bug for a plain network error
instead of OnDeviceUnavailable. Now maps to retryable = true, with a
regression test mirroring the existing OnDeviceUnavailable coverage.
- TagSuggestionViewModel's post-launch `activeBlockUuid = null` reset
now only fires if it still refers to the job's own block, closing a
race where a completing job for block A could clobber block B's
in-flight activeBlockUuid after a fast block switch.
- Added a pipeline-level test proving a genuine LLM timeout resolves to
LlmSuggestionStatus.Failed(retryable = true) end to end, not just via
a hand-constructed UI state.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GUh92Keov7BCtTf8T9io1a
There was a problem hiding this comment.
🟡 Not ready to approve
TagSuggestionViewModel currently maps any retryable RequestFailed into Stalled, which can misclassify non-download failures and hide the actual error message (see stored comment ID 001).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Fixes the tag-suggestion bottom sheet getting stuck indefinitely on the on-device model “Downloading…” state by preserving the retryable signal, adding a bounded availability poll loop, and exposing explicit UI states + a manual retry affordance end-to-end.
Changes:
- Introduces
TagAvailabilityPollerand threadsLlmProvider.checkAvailability()into the tag-suggestion pipeline to support bounded polling + auto-resolve. - Replaces
llmPending/llmErrorwith a sealedLlmSuggestionStatusstate machine and updatesTagSuggestionViewModelto orchestrate polling, escalation, and retry. - Adds
retryabletoDomainError.NetworkError.RequestFailed, updates Compose UI rendering (including accessibility semantics), and adds unit/integration/UI tests for the new states.
File summaries
| File | Description |
|---|---|
| project_plans/llm-tag-download-stall/research/ux.md | UX research write-up for stalled download behavior and UI requirements. |
| project_plans/llm-tag-download-stall/research/stack.md | Stack/polling/testing research and constraints. |
| project_plans/llm-tag-download-stall/research/pitfalls.md | Enumerates lifecycle/testability/polling pitfalls and mitigations. |
| project_plans/llm-tag-download-stall/research/features.md | Prior art + edge cases relevant to polling/retry behavior. |
| project_plans/llm-tag-download-stall/research/build-vs-buy.md | Justifies hand-rolled polling vs libraries. |
| project_plans/llm-tag-download-stall/research/architecture.md | Architecture reasoning for where polling/state should live. |
| project_plans/llm-tag-download-stall/requirements.md | Formal requirements and ACs for the fix. |
| project_plans/llm-tag-download-stall/implementation/validation.md | Requirement-to-test mapping and validation plan. |
| project_plans/llm-tag-download-stall/implementation/pre-mortem.md | Pre-mortem risks and planned mitigations. |
| project_plans/llm-tag-download-stall/implementation/architecture-review.md | Architecture review notes and concerns tracking. |
| project_plans/llm-tag-download-stall/implementation/adversarial-review.md | Adversarial review results and resolved blockers. |
| project_plans/llm-tag-download-stall/design/ux.md | UX design spec for the new status states and accessibility requirements. |
| project_plans/llm-tag-download-stall/decisions/ADR-001-poll-deadline-estimate.md | Interim poll deadline decision record. |
| project_plans/llm-tag-download-stall/decisions/ADR-002-dismiss-does-not-cancel-poll-loop.md | ADR for keeping polling running after dismiss (bounded by deadline). |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/error/DomainError.kt | Adds retryable metadata to RequestFailed. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/tags/LlmTagProvider.kt | Preserves retryable when mapping on-device unavailability + marks network errors retryable. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/tags/TagAvailabilityPoller.kt | New bounded poller for LlmProviderAvailability with escalation and resilience. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/tags/TagSuggestionEngine.kt | Threads an optional checkAvailability probe into the engine. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/tags/TagSuggestionState.kt | Introduces sealed LlmSuggestionStatus to prevent illegal UI state combos. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/tags/TagSuggestionViewModel.kt | Orchestrates polling/auto-resolve/retry and supports test-time overrides. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/ui/App.kt | Wires the availability probe into TagSuggestionEngine. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/ui/components/tags/SuggestionBottomSheet.kt | Renders LlmSuggestionStatus states and adds Retry button + semantics. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/ui/components/tags/TagChipRow.kt | Updates signature to accept llmStatus and simplifies rendering. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/ui/components/VoiceCaptureButton.kt | Adapts to the new TagChipRow API. |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/ui/screens/JournalsView.kt | Wires onRetry to TagSuggestionViewModel.retryLastRequest(). |
| kmp/src/commonMain/kotlin/dev/stapler/stelekit/ui/screens/PageView.kt | Wires onRetry to TagSuggestionViewModel.retryLastRequest(). |
| kmp/src/jvmTest/kotlin/dev/stapler/stelekit/ui/ErrorStateNoDeadEndTest.kt | Updates construction to the new llmStatus model. |
| kmp/src/jvmTest/kotlin/dev/stapler/stelekit/ui/TagInsertionFlagshipUiTest.kt | Wires onRetry through the flagship UI test harness. |
| kmp/src/jvmTest/kotlin/dev/stapler/stelekit/ui/LlmSuggestionCaptionStatesUiTest.kt | New Compose UI tests for caption/retry semantics across states. |
| kmp/src/businessTest/kotlin/dev/stapler/stelekit/tags/TagAvailabilityPollerTest.kt | Unit tests for poller timing/escalation/resilience under virtual time. |
| kmp/src/businessTest/kotlin/dev/stapler/stelekit/tags/LlmTagProviderTest.kt | Regression tests ensuring retryable is preserved through error mapping. |
| .backlog-context.md | Updates the embedded backlog context to this bug/AC set. |
Review details
- Files reviewed: 33/34 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Resolves the only conflict (.backlog-context.md, modify/delete — main deleted it; this branch's copy was ephemeral backlog-automation bookkeeping never meant to be permanent repo content) by taking main's deletion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QmtqsYyS4LUEbbLdbG2if5 # Conflicts: # .backlog-context.md
Contributor
JVM Load Benchmark (Desktop)Synthetic in-memory benchmark measuring load performance for the desktop (JVM) app.
Flamegraphs (this PR)**Allocation** — object allocation pressure (JDBC/SQLite churn)Alloc flamegraph not available CPU — method-level hotspots by on-CPU time CPU flamegraph not available Top allocation hotspots (this PR)`37.5%` byte[]_[k] `8.8%` java.lang.String_[k] `5.9%` int[]_[k] `4.8%` java.util.LinkedHashMap$Entry_[k] `4.3%` java.lang.Object[]_[k]Top CPU hotspots (this PR)`97.1%` /usr/lib/x86_64-linux-gnu/libc.so.6 `1%` clock_nanosleep `0.7%` /tmp/sqlite-3.51.3.0-8f917865-26ea-43e0-b5b8-81454c0dae60-libsqlitejdbc.so `0.2%` __libc_pwrite `0.1%` SR_handler |
Contributor
Android Load BenchmarkInstrumented benchmark on an API 30 x86_64 emulator — 500-page synthetic graph. Comparing Graph Load
Interactive Write Latency (during Phase 3)
SAF I/O Overhead (ContentProvider vs direct File read)Measures Binder IPC cost added by ContentResolver per readFile() call.
|
…gnal Copilot's PR review correctly flagged that any retryable RequestFailed was mapped to LlmSuggestionStatus.Stalled, which discards the real error message and misrenders unrelated retryable failures (a NetworkError, an OnDeviceUnavailable surfaced without polling ever starting, or a TOCTOU retry-after-Available failure) as the on-device "taking longer than expected" caption. This also exposed the exact scenario in a prior commit's own retryLastRequest test, whose comment explicitly documented the old (wrong) behavior. Stalled now requires the poll loop's own TagAvailabilityPoller. STALLED_REASON message specifically; every other retryable RequestFailed maps to Failed(message, retryable=true), preserving the real message with a Retry button. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QmtqsYyS4LUEbbLdbG2if5
… test 'poll loop for a stale block does not write into a newly active block's cache' failed twice in a row on GitHub Actions CI with the same java.lang.IllegalStateException from awaitState's timeout (SLOW ~5.2s, just over the 5000ms default). An earlier fix pass bumped the block-B await in this same test to 15000ms but missed the final block-A-re-request await, which was still at the 5000ms default — CI's real thread-pool contention (heavier than local dev) was enough to occasionally push it over. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QmtqsYyS4LUEbbLdbG2if5
Bumping the awaitState timeout incrementally (5000ms -> 15000ms) kept losing to the same CI-only flake: each time, the failure landed just past whatever timeout was current (SLOW ~5.2s, then ~15.2s), pointing at genuine GitHub Actions runner thread-pool contention rather than a marginal one-off. Tried converting the test to a shared virtual-time test scheduler (the pattern already used successfully elsewhere in this file), but that introduced its own deterministic timing problem — block-A's endless 50ms poll loop competes for scheduler cycles even though it never resolves, and awaitState's own deadline check is real-clock based regardless of dispatcher sharing, so the rewrite still hit the same wall. Reverted that attempt. Landed on the simpler fix: keep the real-Dispatchers.Default design (which passes in ~1s locally, every time, including in isolation), and set both awaitState calls to one generous 60000ms margin instead of chasing the number incrementally. The margin only needs to be big enough to absorb CI contention — the underlying transitions this test verifies are near-instantaneous. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QmtqsYyS4LUEbbLdbG2if5
The final block-A-re-request assertion in 'poll loop for a stale block does not write into a newly active block's cache' hung indefinitely on GitHub Actions CI specifically — bumping its awaitState timeout to 60000ms changed the failure mode from a clean timeout to kotlinx.coroutines.test.UncompletedCoroutinesError (runTest's own internal watchdog), confirming a genuine multi-minute- or-longer stall under CI's resource constraints, not marginal slowness. Root-causing the actual hang mechanism wasn't feasible without CI shell access. That assertion was provably redundant: the test's core regression check (checkAvailabilityCalls not growing after switching away from a block whose poll job should be cancelled) already fully proves the "does not write into a newly active block's cache" property this test is named for — a leaked job could only ever corrupt the cache by continuing to call checkAvailability(), which the existing assertion already directly measures. Removed the redundant tail; re-verified via mutation testing (temporarily disabling suggestionJob?.cancel() in production code, confirming the simplified test still fails, then reverting) that it retains full regression-catching power. 5/5 clean local reruns. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QmtqsYyS4LUEbbLdbG2if5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the on-device LLM tag-suggestion sheet freezing on a one-shot "Downloading on-device
model…" caption forever (the reported bug: a screenshot showing the sheet stuck with no
progress and no way out). The status check was a single, one-shot
checkStatus()call —nothing ever re-checked, so a model that finished downloading seconds or minutes later left
the UI permanently stale.
Context
MlKitLlmFormatterProvider.format()returns immediately onDOWNLOADABLE/DOWNLOADINGwith a
retryable = truesignal, butLlmTagProvider.suggestTags()discarded that signalwhen mapping to
DomainError, andTagSuggestionViewModelnever re-ran the check. This PRadds a bounded, elapsed-time-driven poll loop with caption escalation and a manual retry
affordance, closing that gap end-to-end.
Full requirements, 6-dimension research, plan, and review artifacts are in
project_plans/llm-tag-download-stall/.Changes
TagAvailabilityPoller(new): stateless, wall-clock-bounded poll loop overLlmProviderAvailability, modeled on the existingGitHubDeviceFlowClient.pollForTokenshape so it's directly unit-testable under
kotlinx.coroutines.testvirtual time.TagSuggestionState: replaced the flatllmPending: Boolean/llmError: String?pair with a sealed
LlmSuggestionStatus(NotStarted/Pending/Resolved/Stalled/Failed)so illegal state combinations are unrepresentable.
TagSuggestionViewModel: newrunLlmSuggesthelper threads the poll loop intorequestSuggestions(); auto-resolves once the model becomes available; a session-scopeddownloadFirstObservedAtMstimestamp survives block-switches and manual retries soelapsed-time-based escalation/deadline logic isn't reset by ordinary usage patterns
(a real gap caught by the pre-mortem gate); new
retryLastRequest()is the manual-retrycall target;
scanEntries()opts out of polling viaallowPolling = falseto preservetoday's fail-fast bulk-scan timing.
DomainError.NetworkError.RequestFailed: additiveretryable: Boolean = falsefieldso the on-device-unavailable signal survives the
LlmTagProvider→TagSuggestionEngine→ ViewModel boundary instead of being dropped.
SuggestionBottomSheet,TagChipRow): renders allLlmSuggestionStatusstates —escalating caption at ~45s, a distinct "taking longer than expected" terminal state, and a
focusable Retry button (present only when
retryable, absent — not disabled — otherwise).TagAvailabilityPollerTest(7 tests, virtual time), 18 tests inTagSuggestionViewModelTestcovering the full poll/retry/stall/block-switch matrix, 10Compose UI tests in
LlmSuggestionCaptionStatesUiTest, plus regression tests for thestale-block-leak and format()-not-retriggered-per-tick pitfalls.
Impact
dev.stapler.stelekit.tagspackage +SuggestionBottomSheet/TagChipRowUI +their call sites (
JournalsView,PageView,VoiceCaptureButton). No schema/migrationchanges.
DomainError/provider changes are additive withbackward-compatible defaults.
AVAILABLE) or thegenuinely-unsupported-device path — both short-circuit before any polling starts.
existing
GitHubDeviceFlowClient/GraphFileWatcheridiom (ArrowSchedule/CircuitBreakerconsidered and rejected — wrong problem shape).
Reviewer Notes
TagSuggestionViewModel.runLlmSuggest/requestSuggestions(poll-loopwiring, cache-hit disambiguation across block switches) and
TagAvailabilityPoller'selapsed-time tracking.
DEFAULT_POLL_DEADLINE_MS = 120_000Lis a desk-researched estimate (no physicalAICore-capable device was available this session) — see
project_plans/llm-tag-download-stall/decisions/ADR-001-poll-deadline-estimate.mdforsources and its mandatory real-hardware re-validation follow-up.
LlmProviderAvailability(platform-agnosticby design, per NFR-2), so it will technically activate on iOS once an iOS on-device
provider is registered, using an Android-derived deadline — documented as an accepted
consequence, not a gap, in the plan's Pattern Decisions table.
LlmSynthesisService.kt:104has the identicalretryable-dropping bug this PR fixes inLlmTagProvider.kt, left untouched as out-of-scope — needs its own follow-up ticket.duplicated cache+state-update logic in the ViewModel,
LlmSuggestionStatustransitionlogic spread across 3 collaborators) were surfaced by review but left as documented
follow-ups rather than expanding this PR's diff further — see
project_plans/llm-tag-download-stall/implementation/architecture-review.md.DEFAULT_POLL_DEADLINE_MSre-validation (ADR-001);LlmSynthesisService.kt's twin bug; the refactor items above.tag-suggestion behavior, not new functionality behind a flag.
Test plan
./gradlew jvmTest— full suite green except 4 pre-existing, unrelatedGraphManagerDatabaseLifecycleTestfailures (zero file overlap with this diff,confirmed via
git diff --stat)./gradlew detekt— cleandedicated spec-compliance pass
App.ktchanges (no startup regression)
plan (with architecture + adversarial review, 4 blockers found and resolved across
2 repair iterations) → validation + pre-mortem (2 P1 risks found and fixed) → product
triad review (1 accessibility blocker fixed) → implementation (6 parallel epics) →
3-layer verification (idiom/architecture/correctness — 1 more MUST FIX caught and
fixed: a test that gave false confidence due to a real-vs-synthetic clock mismatch)
Related
Closes backlog item
505fb733-9621-4621-b7fc-27712e36d084("Device model download kinda ofsucks").