Skip to content

ADFA-1798 | Add proactive memory warning for custom model loading - #63

Merged
jatezzz merged 1 commit into
mainfrom
feat/ADFA-1798-model-memory-preflight
Aug 12, 2026
Merged

ADFA-1798 | Add proactive memory warning for custom model loading#63
jatezzz merged 1 commit into
mainfrom
feat/ADFA-1798-model-memory-preflight

Conversation

@jatezzz

@jatezzz jatezzz commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Description

This PR introduces a "pre-flight" memory check to prevent application crashes caused by loading oversized custom .gguf models into memory. Before attempting to load a selected model, the application now reads the file's GGUF header to estimate its required footprint (weights plus KV cache and compute buffers) and compares it against the device's currently available RAM.

If the model is deemed too large and poses a crash risk, a non-blocking warning dialog alerts the user. The dialog provides a clear explanation of the memory shortfall and offers two choices: "Cancel" (aborting the load safely, which is the default) or "Proceed anyway" (empowering power users to attempt the load if they plan to free up RAM). This proactively enhances app stability and builds user trust.

Details

  • Memory Estimation: Added GgufHeaderReader to parse metadata blocks and ModelMemoryEstimator to calculate load and runtime byte requirements.
  • System RAM Check: Added DeviceMemory interface and SystemDeviceMemory implementation to read real-time available RAM from ActivityManager.
  • Decision Logic: Introduced ModelMemoryGate to evaluate estimates against available RAM, assigning a risk severity of TIGHT or INSUFFICIENT.
  • User Interface: Implemented MemoryWarningDialogFragment to present the warning, utilizing a new UserConfirmation coroutine primitive to gracefully handle async user decisions and process death.
  • Documentation: Updated index.html and tooltips to explain the memory requirements and warning behaviors to users.
document_5161635105843709586.mp4

Ticket

ADFA-1798

Observation

  • The feature is designed to fail open: if the model's file size or the device's free RAM cannot be read, or if the GGUF header is unparseable, the app defaults to loading the model without a warning to avoid blocking legitimate user flows.
  • All file reading and ContentResolver queries during the pre-flight check are dispatched to the IO thread to avoid blocking the main UI thread.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Base automatically changed from feat/ADFA-5018-agent-settings-in-preferences to main August 7, 2026 14:28
@jatezzz
jatezzz force-pushed the feat/ADFA-1798-model-memory-preflight branch from 8501e64 to 17a4173 Compare August 7, 2026 14:37
@hal-eisen-adfa

hal-eisen-adfa commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Code review

Found 1 issue:

  1. Unbounded Long multiplication of header-supplied values overflows silently, producing a wrong memory estimate (bug due to kvCacheBytes multiplying five values that are only floor-checked with takeIf { it > 0L }). GgufHeaderReader bounds entry counts, array lengths, and string lengths, but never the value of a scalar shape field, so a corrupt or crafted .gguf can wrap the product. Two outcomes, both executed against this branch:

    • block_count = 2^61, key_length = value_length = 288 — the KV term wraps to exactly 0, so runBytes collapses to the flat 256 MB compute-buffer allowance. The estimate is understated and the severity is downgraded (TIGHT where the real shape would be INSUFFICIENT).
    • block_count = 2^49, key_length = value_length = 1 — the product is exactly 2^63, i.e. Long.MIN_VALUE, so totalBytes goes negative (-9223372032586340352). availableBytes >= estimate.totalBytes then holds for any device and the gate returns Verdict.Safe, skipping the warning entirely.

    That is the "wrong estimate" the reader's own KDoc says must never happen — "fails closed to null: an unreadable header must mean 'no estimate', never a wrong one". A cheap fix is an upper bound on the shape fields (or Math.multiplyHigh/Math.*Exact with a catch) returning null on overflow, so the estimate stays absent rather than wrong. GgufHeaderReaderTest covers corrupt entry counts and string/array lengths, but no corrupt scalar shape value.

val keyWidth = header.keyLength?.takeIf { it > 0L } ?: defaultHeadWidth(header) ?: return null
val valueWidth = header.valueLength?.takeIf { it > 0L } ?: defaultHeadWidth(header) ?: return null
return KV_BYTES_PER_ELEMENT * layers * RUNTIME_CONTEXT_TOKENS * kvHeads * (keyWidth + valueWidth)
}

if (estimate == null || availableBytes == null) return Verdict.Unknown
return when {
availableBytes >= estimate.totalBytes -> Verdict.Safe
availableBytes >= estimate.runBytes -> Verdict.Risky(estimate, availableBytes, Severity.TIGHT)
else -> Verdict.Risky(estimate, availableBytes, Severity.INSUFFICIENT)
}

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@jatezzz
jatezzz force-pushed the feat/ADFA-1798-model-memory-preflight branch from 17a4173 to 3d873f6 Compare August 11, 2026 13:09
@hal-eisen-adfa

Copy link
Copy Markdown
Contributor

Code review

The overflow finding from the previous review is fully addressed by the ceilings and the saturating totalBytes in 11dfc00, including tests for both repro cases. Two new issues, both in the 5 lines that commit added to ai-core:

Found 2 issues:

  1. The new pre-load gate hard-refuses loads that this PR's own "Proceed anyway" dialog just authorized. ModelLoadDiagnostics.diagnose was a post-mortem attributor — on main it is called only from the catch around llama.load(), and its KDoc explains the max(256 MB, size/4) headroom is deliberately conservative "because overestimating it would blame a corrupt model on memory". Promoting it to a gate makes it disagree with the ai-assistant estimate that drove the dialog: for a 8.5 GB Q8_0 with a parsed header (runBytes = 768 MB) on a device with 1.5 GB free, the gate returns TIGHT, the user proceeds, the path is persisted and settings shows "Loaded" — then the first message throws ModelLoadException quoting 2.1 GB, a number the user never saw, with no override and without llama.load() ever being attempted. ModelLoadException is not caught on the send path, so the model becomes permanently unusable. This contradicts ModelMemoryGate's stated invariant.

val headroom = ModelLoadDiagnostics.diagnose(resolvedPath, availableMemoryBytes())
if (headroom is ModelLoadDiagnostics.Diagnosis.LowMemory) {
throw ModelLoadException(loadMessages.describe(headroom), headroom)
}

* Decides whether a selected model is worth warning about, from its estimate and the free RAM.
*
* Pure, so every boundary is unit-testable. It never refuses a model: the caller asks the user,
* who may proceed anyway (ADFA-1798).
*/

  1. The gate reads free RAM before the previous model is unloaded, so a switch is measured against memory the switch itself would release — and a refusal never unloads, so retries fail identically. The check sits between the embedding-model classification and the unload block. The classification is placed there for a documented reason (ADFA-4388: "Classify BEFORE unloading any working chat model"), but that reasoning holds because classify is a static property of the file; free RAM is not. LLamaAndroid.unload() frees the context, batch and sampler — anonymous, non-reclaimable allocations (roughly 1 GB for a 7B at the hardcoded context) that ActivityManager.MemoryInfo.availMem excludes while resident. Switching from a large resident model to another can therefore trip LowMemory, and since the throw returns before the unload block, modelLoaded/currentModelPath are untouched and no user-facing path reclaims that memory — only close() at plugin disposal unloads. Every retry reproduces the verdict. Pre-PR, the same switch unloaded first and had a real chance of succeeding.

val headroom = ModelLoadDiagnostics.diagnose(resolvedPath, availableMemoryBytes())
if (headroom is ModelLoadDiagnostics.Diagnosis.LowMemory) {
throw ModelLoadException(loadMessages.describe(headroom), headroom)
}
// Unload old model if loaded
if (modelLoaded) {
context.logger.info("Unloading previous model: $currentModelPath")
llama.unload()
modelLoaded = false
currentModelPath = null
}

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Selecting a .gguf now runs a pre-flight check before the path is persisted, which is what makes ai-core load it. The model's size and its declared shape are weighed against free RAM; if it looks too large the user is asked whether to go ahead, with Cancel as the default and the figures behind the judgement on screen. Declining leaves the previously selected model untouched.

The check fails open throughout: an unreadable size, header or memory reading means no warning rather than a wrong one.

- ModelMemoryEstimator splits mmap'd weights from the KV cache and compute buffers, sized from the model's own shape when the header provides it.
- ModelMemoryGate decides Safe / Risky(TIGHT|INSUFFICIENT) / Unknown.
- GgufHeaderReader reads only the metadata block, bounded by a byte budget, an entry cap and an array cap, and attributes shape keys to their own architecture so a multimodal file's clip.* values stay out.
- UserConfirmation lets the load coroutine await the user's answer, so the flow stays one readable sequence across a rotation.

ADFA-1798
@jatezzz
jatezzz force-pushed the feat/ADFA-1798-model-memory-preflight branch from 3505241 to 3c1b4de Compare August 12, 2026 14:50
@jatezzz
jatezzz merged commit bc41e84 into main Aug 12, 2026
1 check passed
@jatezzz
jatezzz deleted the feat/ADFA-1798-model-memory-preflight branch August 12, 2026 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants