ADFA-5253 | Persist model URI instead of copying multi-GB models

[jatezzz]

Description

Implemented direct-access behavior for LLM models selected via the Storage Access Framework (SAF). The application now retains persistent read permissions for the chosen URI and queries the .gguf file in-place. This prevents copying multi-gigabyte models into the app's internal sandbox, drastically saving device storage and speeding up the setup process. Additionally, a cleanup routine was added to delete legacy model copies, and the plugin's HTML documentation was updated to reflect this direct-access behavior.

Details

• Updated LocalLlmBackend to hand the native loader a file descriptor path (/proc/self/fd/N) for direct reading.
• Added ModelSourceWatcher and NativeModelSource interfaces to safely monitor, open, and evict models if the underlying source file is deleted or unmounted.
• Adjusted LocalLlmSettingsViewModel and LocalLlmSettingsFragment to properly display and handle "Unavailable" model states when a URI becomes unreachable.
• Refactored GgufModelInspector and ModelLoadDiagnostics to operate on InputStream factories rather than static file paths.

document_4956368585724266828.mp4

Ticket

ADFA-5253

Observation

To facilitate mmap operations in the native code without copying the file, ContentNativeModelSource resolves the content:// URI to a file descriptor and passes the /proc/self/fd/ path to the llama.cpp backend. A legacy cache cleanup (deleteLegacyModelCache) is executed in the background upon initialization to reclaim space from older app versions.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

[itsaky-adfa]

Reviewed at 426e5bd. One confirmed defect on a path a user will hit, so requesting changes; the rest are minor. The refactor itself is careful, and the seam design (NativeModelSource / ModelResidencyEngine / ModelSourceWatcher) is what makes the residency rules testable at all - that part is a clear improvement.

Verified locally: ./gradlew testDebugUnitTest in ai-agent-local - 137 tests, 0 failures. git fetch origin first, so the diff is against current origin/main (68abdd6).

Blocking: the isAvailable() memo (LocalLlmBackend.kt:243) makes the first message after a restored model file fail with Backend 'local' is not available. Traced through ai-core's LlmInferenceServiceImpl.generateStreaming, which returns that string verbatim on a false.

Two things only a device can settle, both called out inline:

• A streaming DocumentsProvider hands back a pipe, which the three openStream() calls consume before llama.cpp reads it. Worth picking a .gguf from Drive to see what happens.
• Re-opening /proc/self/fd/N is a fresh path-based open(), checked against path permissions rather than the SAF grant that produced the fd. The attached video covers the happy case; an SD-card and a FUSE-volume load would close it. du -sh .../files/llm-models proves no copy was made, not that the load works everywhere.

Checked and cleared: descriptor lifecycle in ensureModelLoaded (the adopted/finally pairing and the unload-before-close ordering are right, and both header readers .use their streams); double-close() on modelWatch (unreachable - stopWatching nulls it); the close()-vs-onModelSourceGone race on cleanupScope; and no remaining callers anywhere in the repo of the removed or privatised API (engineState, savedModelPath, modelLoadingState, getSavedModelName, fallbackDisplayName, isGguf(String), EngineState.Uninitialized).

Two candidate findings I dropped after checking:

• "The model is permanently unavailable once the memo is set" - false. LlmInferenceServiceImpl.getAvailableBackends() does not filter on isAvailable(), so local stays selectable and the warm-up clears the memo. Only the one-shot stale false survives, which is the blocking finding above.
• "diagnose() losing FileMissing dropped its test" - false. givenMissingFile_whenDiagnosed_thenFileMissing was ported to givenAFilesystemPath_whenDiagnoseUnopenable_thenFileMissing, and diagnose now only runs after a successful open, so the narrowing is deliberate.

No prior review comments on this PR, so there was nothing to re-check from an earlier round. This repo has no written approve/request-changes rule, so the reviewer default applied: any confirmed IMPORTANT blocks. Its CLAUDE.md does require device-level verification over a green build, which is what the two items above are asking for.

Docs are in step with the code - ai-agent-local.html, assets/docs/index.html and the plugin.permissions comment all describe read-in-place rather than the old copy.

[itsaky-adfa]

Round 2, reviewed at e30d9aad. Findings only - the verdict follows separately, once the two unconfirmed items below are settled.

Prior round: all 7 findings fixed

Verified against the code at head, not against the replies. (The reply saying "fixed locally, not pushed yet" was stale by three minutes - 54c95bd landed at 18:25 UTC, the reply at 18:28.)

• isAvailable() memo - unreachableModelRef is gone repo-wide; reachability is answered only on the generation path. It also removes a binder probe from the caller's thread.
• Non-seekable descriptor - refused at LocalLlmBackend.kt:300, before the first openStream(), which is the ordering the finding turned on. Plus EXTRA_LOCAL_ONLY on the picker.
• ModelLoadDiagnostics KDoc - back above refuseBeforeLoad.
• Fragment "(unavailable)" marker - keyed off the engine status, and publishAbandonedSelection covers the stranded Initializing you turned up while testing it.
• Missing tests - openDocument now pins the procfs path and statSize; the 9 LocalLlmSettingsViewModelTest assertions match the grant-lifecycle claims exactly.
• Watch coalescing - sourceCheckInFlight CAS, cleared in a finally outside the lock.
• Watcher KDoc - reworded to name both delivery threads.

Verification

../gradlew testDebugUnitTest: 152 tests, 0 failures. I also stubbed the isSeekable guard to false and confirmed givenAStreamingDocument_whenLoading_thenRefusedAsNotSeekableWithoutReadingIt fails, and only that test - so the new guard is genuinely pinned by its test.

Two findings are NOT confirmed

Both are marked as such inline. Please treat them as questions, not verdicts:

• NativeModelSource.kt:106 - whether /proc/self/fd/N re-opens successfully for a document on removable storage. The code path is confirmed (llama.cpp opens the path by name, never the descriptor); the permission outcome needs a device. Your demo video shows at least one storage location working, so this is scoped to SD/USB.
• ModelSourceWatcher.kt:76 - the URI arithmetic is confirmed against the DocumentsContract contract, but I have not watched a provider fail to notify on a device.

Every other finding is confirmed by reading the code at head. For the eviction-on-transient-failure one, the defect is confirmed and only the trigger's frequency is not.

Checked and dropped

Saying so explicitly, so they are not silently missing: main-thread binder I/O in ensureModelLoaded (every call site is inside a Dispatchers.IO scope); the descriptor being closed while llama.cpp still has pages mapped (engine.unload() drains the native run loop before releaseCurrentModel()); the watch Closeable being double-closed today (stopWatching() nulls the field); and core-testing bypassing a version catalog (this repo has none - every dependency is a hardcoded coordinate).

This repo has no written approve/request-changes rule, so the default one applied.

The seam design continues to be the strength here - NativeModelSource / ModelResidencyEngine / ModelSourceWatcher are what make any of this testable off a device, and the docs were kept in step with the behaviour change.

[itsaky-adfa]

Round 3, reviewed at 913807d3. Findings only - the verdict follows separately.

Prior round: 7 of 9 fixed, 1 not fixed, 1 fixed into a new defect

Verified by reading the code at head, not by reading the replies.

• NativeModelSource.kt:134 (procfs path re-opened by name) - fixed, as a refusal rather than a guess: OpenModelFile.isReopenable() opens nativePath by name before the header read and raises SourceNotReopenable, wired through ModelLoadMessages and strings.xml and documented in assets/docs/index.html. The probe now does exactly what the native loader does. The test pinning it is a finding below.
• LocalLlmBackend.kt:286 (a transient failure evicts a resident model) - NOT fixed. See the reply on that thread: ContentResolver converts provider death into FileNotFoundException, which documentReachability reads as GONE.
• ModelSourceWatcher.kt:78 (a delete is notified on the parent) - fixed: watchDocument registers on parentChildrenUriOf(uri) too, one unregisterContentObserver covers both, and the URI arithmetic is pinned by 4 new tests. Still device-unverified.
• LocalLlmSettingsViewModel.kt:358 (the grant leaks on cancellation) - fixed: a stored flag plus a finally, and the three inline releases are gone. Pinned by 4 new tests, including the decline-at-the-dialog path.
• LocalLlmSettingsViewModel.kt:206 (a stale error on the status line) - fixed for the case raised, into a new defect for the other case. See the reply on that thread.
• LocalLlmBackend.kt:451 (the warm-up re-armed on every isAvailable()) - fixed: warmedUpRef.getAndSet(configuredPath), pinned by two tests (five asks cost one attempt; a different selection re-arms).
• ModelSourceWatcher.kt:82 (Closeable not idempotent) - fixed: both watch paths return closeOnce { } behind an AtomicBoolean CAS.
• LocalLlmSettingsViewModel.kt:507 (update ignored its transform) - fixed: restoreStateBefore composes the two lines a decline actually owns.
• ModelFileSource.kt:60 (releaseAccess reported a no-op as an error) - fixed: SecurityException is caught and returns quietly.

Round 1's 7 findings were re-confirmed fixed in round 2 and are unchanged at head.

Build and tests

./gradlew testDebugUnitTest on a clean worktree at 913807d3: 170 tests, 1 failure, reproduced on two runs including --rerun-tasks. That failure is a finding below. The other 169 pass, and compileDebugKotlin is clean.

Device verification was not possible in this session, so nothing here is device-verified. Per this repo's CLAUDE.md ("Verification"), a green build is necessary and never sufficient - and the two mitigations that turn on how a real DocumentsProvider behaves on removable storage (isReopenable, the parent-children watch) still need a device before anyone calls them settled.

Verdict rule

This repo has no REVIEW.md, CONTRIBUTING.md or PR template, and CLAUDE.md sets no approve/request-changes rule, so the skill default applied: any confirmed CRITICAL or IMPORTANT blocks.

[itsaky-adfa]

Requesting changes at 913807d3. Findings are in the review above and in the two reopened threads.

Two things need to happen before this can go in:

1. The suite is red. givenADocumentThatCannotBeReopenedByPath_whenLoading_thenRefusedWithItsOwnAdvice fails on any host whose JVM holds fd 99, which a Gradle test worker does. 170 tests, 1 failure, reproduced twice.
2. Round-2's eviction finding is still open. ContentResolver converts every provider-death path into FileNotFoundException, so documentReachability reads a dead provider as GONE and a transient failure still costs a resident multi-GB model its pages. The UNKNOWN arm is unreachable for the case it was written for. ModelFileSource.isReadable makes the same collapse on the settings pane.

Also worth fixing in this round: refreshSavedModelAvailability now clears an Error that describes the configured model, so a .gguf that just failed looksLikeGguf reads back as "Model loaded / Engine ready" on the next onResume.

The two MINORs and the NITPICK are yours to judge - none of them blocks.

For the record, the refactor itself continues to hold up well: seven of round 2's nine findings are properly fixed, each with a test that pins the behaviour rather than the implementation, and the NativeModelSource / ModelResidencyEngine / ModelSourceWatcher seams are what made this round reviewable off a device at all. Note that none of this is device-verified - isReopenable and the parent-children watch both turn on real DocumentsProvider behaviour on removable storage, and should be exercised on hardware before they are trusted.

[itsaky-adfa]

Round 4, reviewed at a9d14e66. Findings only - the verdict follows separately.

Prior round: 6 of 7 fixed, 1 partly

Verified by reading the code at head and running the suite, not by reading the replies.

• ModelFileSource.kt:96 - isReadable collapsed the tri-state (IMPORTANT). Fixed. readability() returns SourceReachability; refreshSavedModelAvailability leaves both status lines alone on UNKNOWN (LocalLlmSettingsViewModel.kt:220), and the pick path refuses only a confirmed GONE (line 397). Three new cases in ContentModelFileSourceTest.
• LocalLlmBackend.kt:286 - GONE/UNKNOWN could not separate provider death (IMPORTANT). Fixed. confirmedGone (NativeModelSource.kt:95) re-asks after 250 ms and downgrades an unconfirmed GONE to UNKNOWN; both document probes route through it, and givenAProviderThatServesOnTheSecondAsk_... pins it.
• LocalLlmSettingsViewModel.kt:206 - a re-check cleared an Error about the configured model (IMPORTANT). Fixed. Error carries reference, stamped in publishAbandonedSelection so no call site can forget it, and clearStatusMadeStaleBy keeps an error whose reference is the configured path. The engine line was not carried along with it - that is the new IMPORTANT on line 286.
• LocalLlmBackendTest.kt:340 - the test depended on the worker's fd table (IMPORTANT). Fixed. It names a never-created TemporaryFolder child. The suite is green: 178 tests, 0 failures, 0 errors - that was the first blocker in the round-3 verdict.
• LocalLlmBackend.kt:853 - close() reached evictResidentModel() without generationMutex (MINOR). Fixed. unloadModelInternal() takes the lock and is its only caller; all four evictResidentModel() sites hold it now.
• ModelSourceWatcher.kt:88 - a half-registered observer leaked (NITPICK). Fixed. unregisterQuietly(observer) runs before releaseHandler() and the rethrow.
• LocalLlmBackend.kt:293 - the pre-generation probe was unbounded (MINOR). Partly fixed; thread reopened with the detail. The 5s window bounds a healthy provider, but only a REACHABLE answer arms the memo, so a provider that hangs rather than dies is still re-probed on every message.

This round

Two IMPORTANT, both in the settings pane's status model and both traceable to engineStateFor(Error) = null; two MINOR; three NITPICK. Every one is anchored in the diff - nothing was dropped, degraded, or left for this body.

One lead I checked and am not reporting: close() calling stopWatching() outside generationMutex looks like it can race the startWatching at line 400 and leak a ContentObserver, but it cannot - unloadModelInternal() runs afterwards, takes the lock the load still holds, and its releaseCurrentModel() calls stopWatching() again.

Verification

• ../gradlew testDebugUnitTest - 178 tests, 0 failures, 0 errors. (I had to create ai-agent-local/local.properties; it is not committed.)
• ../gradlew assemblePlugin - BUILD SUCCESSFUL, build/plugin/ai-agent-local.cgp, native libraries limited to arm64-v8a.
• CI on this head: passing.
• No device verification. adb devices is empty here, so the two things left unproven by earlier rounds - whether the native loader can re-open /proc/self/fd/N, and which volumes refuse it - stay unproven. For a plugin that reads a user's file in place through a persisted grant, a green build is necessary and nowhere near sufficient.
• tools/addons check not run: uv is not installed on this machine. This PR touches no addon.json, directory name, or URL, and check-toolchain.yml passed.

This repo has no written approve/request-changes rule - no CONTRIBUTING.md, REVIEW.md, or PR template, and CLAUDE.md covers workflow rather than verdicts - so the default applied.

[itsaky-adfa]

Requesting changes at a9d14e66. Findings are in the review above and in the reopened thread.

The round-3 blocker is cleared: the suite is green (178 tests, 0 failures), assemblePlugin succeeds, CI passes, and 6 of the 7 prior findings hold up when read at head rather than taken from the replies. The reachability work in particular is in good shape now - confirmedGone and the tri-state are the right shape, and the tests pin them.

Two things to settle before this goes in, both in the settings pane's status model and both the same root cause:

1. engineStateFor(Error) = null leaves the engine line describing the state before the refusal (LocalLlmSettingsViewModel.kt:286). Load from saved on a truncated model draws "Engine ready" beside "isn't a valid .gguf", and an engine line already reading ModelUnavailable survives a refusal of a model that is reachable - where nothing can clear it any more, now that an Error about the configured model is (rightly) kept. This is the split LocalLlmSettingsState was introduced to make impossible, so it is worth closing there rather than in the fragment.

2. A failed persistAccess is only logged (LocalLlmSettingsViewModel.kt:383). The code's own comment promises to "say so later" and nothing does: the model is stored, shown as loaded, works all session, and then fails every message after a restart with advice to re-pick a file that never moved. Say it at selection time, while the picker's grant is still live.

The two MINOR and three NITPICK are yours to take or leave; the reopened LocalLlmBackend.kt:293 thread explains why the 5s window does not yet bound the case it was aimed at.

Still unverified on my side, and worth a note in the PR before merge: there is no device on this machine, so whether the native loader can re-open /proc/self/fd/N, and which volumes refuse it, remain unproven from rounds 2 and 3. The SourceNotReopenable message and the "Internal storage always works" line in assets/docs/index.html are both guesses about that behaviour until someone exercises it on hardware.

[itsaky-adfa]

Round 5, reviewed at 05504c9. Findings only - the verdict follows separately.

Prior round: 8 of 8 fixed, one with a residual

Verified by reading the code at head, not by reading the replies. I also ran :testDebugUnitTest in a worktree at 05504c9: 184 tests, 0 failures, which closes out the round-3 "this test fails at head" finding.

• IMPORTANT LocalLlmSettingsViewModel.kt:383, unpersistable grant kept silently - fixed, with a residual. Loaded carries accessPersisted and the pane says so at selection time. The caveat does not survive a pane rebuild, which is a new MINOR below rather than a reopen: the warning does reach the user while they can still act on it, which is what the finding asked for.
• IMPORTANT LocalLlmSettingsViewModel.kt:286, refused configured model leaves the engine line stale - fixed. engineStateFor gives Error an EngineState.Error when its reference is the configured model, and publishAbandonedSelection stamps the reference before deriving it.
• MINOR LocalLlmSettingsViewModel.kt:236, re-check overwrites a rejected pick - fixed. statusGeneration is bumped in update, read before the probe, re-checked under the same lock.
• MINOR NativeModelSource.kt:196, unconfirmed GONE on the file branch - fixed. fileReachability routes through confirmedGone, probeFile checks isFile && canRead().
• MINOR LocalLlmBackend.kt:293, wedged provider re-probed every message - fixed. The window arms on any non-GONE answer.
• NITPICK LocalLlmSettingsFragment.kt:43, dead try/catch - fixed, state_error removed with it.
• NITPICK docs/index.html:62, "internal storage always works" - fixed.
• NITPICK LocalLlmBackend.kt:289, dead ModelNotConfiguredException - fixed. No references anywhere in the tree.

The three questions you raised

• The guarded GONE arm. Keep it guarded. Unguarded, a confirmed GONE overwrites a refused pick's explanation, which is the finding you just fixed; the case the guard loses is one where the state is either already Unavailable or is recomputed on the next onResume.
• The same sentence on both lines. Worth changing - NITPICK below.
• ContentModelFileSource's file branch still unconfirmed. Worth changing - MINOR below.
• The Loaded flag not surviving the pane - yes, but derive it rather than storing it beside KEY_MODEL_PATH; see the MINOR below.

Two things I looked at and am not filing

The trust window arming on UNKNOWN is the option I asked for last round and I still think it is right: it does not collapse GONE and UNKNOWN (UNKNOWN serves, GONE evicts), and the comment that reads like a contradiction is describing the old behaviour in the past tense.

persistAccess(new) running before releaseAccess(replaced) means that at a full grant table the new grant is taken while the slot it is about to free is still held. Not filed, because the fix for the IMPORTANT below needs the replaced grant held longer, not shorter, and the two cannot both be had - but whichever way that lands, it is worth a line in the code saying so.

Verification

No device attached this session, so nothing here is device-verified; per CLAUDE.md a green build and green unit tests are not verification for this repo. Two items stay unconfirmed for that reason, both carried over: whether isReopenable() actually refuses on removable storage, and whether the document watch sees the delete it exists to catch.

This repo has no written approve/request-changes rule - CLAUDE.md and the plugin-review skill cover process and submission readiness, not review outcomes - so the skill default applied.

[itsaky-adfa]

Round 6, at 733128e. Re-checked all 16 open threads from rounds 4 and 5 against the code at head, then reviewed the new work for its own defects.

All 16 prior findings are fixed. Six new ones: 1 IMPORTANT, 3 MINOR, 2 NITPICK.

Re-check of the open threads

Verified by reading the code at head, not from the replies.

Round 4

• LocalLlmBackendTest.kt fd-99 test - fixed. The reopenable test names temporaryFolder.root/never-created/model.gguf; the seekable test still uses /proc/self/fd/7 but passes sizeBytes = -1, so it is refused before anything opens that path.
• ModelFileSource reachability collapse - fixed; readability() returns the tri-state.
• Unbounded probe in front of every generation - fixed, and the partial fix from last round is now complete: lastProbeAnsweredNanos arms on any answer that is not GONE (line 309), so a provider that only ever yields UNKNOWN costs one message rather than every message.
• unloadModelInternal without the mutex - fixed (line 885). See the IMPORTANT finding on that line: closing that hole opened a different one.
• Half-registered observer - fixed; the catch unregisters before releaseHandler().

Round 5

• Grant that could not be persisted - fixed. Loaded carries accessPersisted, re-derived on every visit rather than remembered. One residual case posted inline.
• Refused configured model leaving a stale engine line - fixed; engineStateFor maps Error to EngineState.Error when its reference is the configured model.
• Re-check overwriting a rejected pick's error - fixed via statusGeneration, taken before the probe and re-checked under the same monitor.
• fileReachability reporting an unconfirmed GONE - fixed; routed through confirmedGone, and probeFile checks isFile && canRead().
• Dead try/catch in the picker callback - fixed, and state_error is gone with it.
• "Internal storage always works" - fixed. The claim is gone and llm_load_error_not_reopenable no longer sends a Downloads pick back to Downloads. Still the unverified half: no device here either.
• ModelNotConfiguredException - fixed; git grep finds no reference anywhere in the repo.
• Pick taking the working model's grant - fixed in shape. supersede writes a persisted list and releaseSupersededGrants gives the grants back only after a load is adopted. Two bookkeeping gaps in that new code are posted inline.
• Caveat dropped on the next visit - fixed; derived from persistedUriPermissions, not stored.
• GONE from a single stat - fixed; confirmedGone { probeFile(uriString) }.
• Engine line printing the model line's sentence - fixed; engine_error, and EngineState.Error is now an object.

Checked and cleared, so you do not have to re-derive it

• The upgrade path is safe. deleteLegacyModelCache does not strand an existing user: origin/main stored the picked URI under KEY_MODEL_PATH (the filesDir/llm-models copy was a load-time cache) and already took the persistable grant in the picker callback, so a model configured before this PR stays readable after it.
• No path releases a live model's grant. The configured model is never on the superseded list at write time, and the one window where a cancelled selection can leave it there is covered by the - loadedRef exclusion.
• Manifest, permissions comment, tooltip category and tags, and the Tier-3 docs/index.html wiring are consistent.

Verification I could not do

• The unit tests were not run. The Kotlin compile fails in my sandbox on a classpath problem (Cannot access class 'View'), and it reproduces identically on origin/main, so it is environmental rather than a defect in this PR. I read the new tests; I did not execute them. Please confirm they pass on your machine.
• No device. Per this repo's CLAUDE.md a green build is not verification here. The in-place /proc/self/fd/N load, isReopenable on removable storage, and the parent-children-URI delete watch all still need a device round trip. /plugin-review on ai-agent-local is worth the minute before merge, given the manifest permission change and the new shipped strings.

This repo has no written approve/request-changes rule, so the default applied. The IMPORTANT finding computes to REQUEST_CHANGES; this review is posted as COMMENT so the findings land without waiting, and the verdict is with the reviewer.