fix(ios): guard Metal residency set with supportsResidencySets check#536
fix(ios): guard Metal residency set with supportsResidencySets check#536shubhamsinnh wants to merge 12 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds adaptive-context LLM APIs across C, C++, and Swift, including lifecycle-owned proto entry points and exported symbols. It also applies an iOS-only llama.cpp patch that guards Metal residency-set usage based on device support. ChangesAdaptive Context API
Metal Residency-Set Guard Patch
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant RunAnywhere
participant CppBridgeLLM
participant LifecycleLLM
App->>RunAnywhere: generateFromContext(query, options)
RunAnywhere->>CppBridgeLLM: forward adaptive-context request
CppBridgeLLM->>LifecycleLLM: call native generate-from-context API
LifecycleLLM-->>CppBridgeLLM: generation result
CppBridgeLLM-->>RunAnywhere: RALLMGenerationResult
RunAnywhere-->>App: return result
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e148de5. Configure here.
| const char* thinking = nullptr; | ||
| size_t thinking_len = 0; | ||
| const char* raw_text = raw.text ? raw.text : ""; | ||
| (void)rac_llm_extract_thinking(raw_text, &response, &response_len, &thinking, &thinking_len); |
There was a problem hiding this comment.
Wrong thinking tag extraction
Medium Severity
rac_llm_generate_from_context_proto splits model output with rac_llm_extract_thinking, while rac_llm_generate_proto uses rac_llm_extract_thinking_with_tags after thinking_tags_from_request_or_model. Adaptive-context generation therefore ignores per-request and per-model thinking delimiters, so thinking-mode models can return malformed RALLMGenerationResult fields compared to ordinary generate.
Reviewed by Cursor Bugbot for commit e148de5. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/runanywhere-commons/src/features/llm/llm_module.cpp`:
- Around line 2457-2473: The slices produced by rac_llm_extract_thinking in
llm_module.cpp are passed directly to C-string consumers even though they are
bounded by length, so response/thinking may carry trailing text. Update the flow
around rac_llm_split_thinking_tokens, set_structured_output_if_present, and
publish_generation_event to first copy the extracted response and thinking
slices into null-terminated buffers, then use those buffers for token
accounting, structured-output handling, and telemetry.
In
`@sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge`+LLM.swift:
- Around line 145-153: The throwIfFailed helper in CppBridge+LLM is collapsing
all native adaptive-context failures into .processingFailed, which loses the
native C ABI error meaning. Update throwIfFailed to preserve rac_result_t
semantics by routing failures through the existing native-result-to-SDK-error
mapping used elsewhere in the bridge, or by explicitly translating statuses like
RAC_ERROR_NOT_SUPPORTED and component-not-ready into their corresponding
SDKException codes. Keep the native error message in the thrown exception, but
ensure the status-specific code is not discarded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82a3c929-7c49-4149-9500-9a1ec801a8f9
📒 Files selected for processing (8)
engines/llamacpp/CMakeLists.txtengines/llamacpp/patches/001-metal-residency-set-guard.patchsdk/runanywhere-commons/exports/RACommons.exportssdk/runanywhere-commons/include/rac/features/llm/rac_llm_component.hsdk/runanywhere-commons/include/rac/features/llm/rac_llm_service.hsdk/runanywhere-commons/src/features/llm/llm_module.cppsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+LLM.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Public/Extensions/LLM/RunAnywhere+TextGeneration.swift
| const char* raw_text = raw.text ? raw.text : ""; | ||
| (void)rac_llm_extract_thinking(raw_text, &response, &response_len, &thinking, &thinking_len); | ||
|
|
||
| int32_t thinking_tokens = 0; | ||
| int32_t response_tokens = raw.completion_tokens; | ||
| (void)rac_llm_split_thinking_tokens(raw.completion_tokens, response, thinking, &thinking_tokens, | ||
| &response_tokens); | ||
|
|
||
| LLMGenerationResult result; | ||
| set_result_from_raw(ref, raw, response, response_len, thinking, thinking_len, thinking_tokens, | ||
| response_tokens, options.max_tokens, &result); | ||
| set_structured_output_if_present(response, &result); | ||
|
|
||
| publish_generation_event( | ||
| runanywhere::v1::GENERATION_EVENT_KIND_COMPLETED, request.prompt().c_str(), nullptr, | ||
| response, nullptr, ref.model_id, raw.completion_tokens, | ||
| raw.total_time_ms > 0 ? raw.total_time_ms : elapsed, raw.prompt_tokens); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Copy bounded thinking-split slices before passing them to C-string consumers.
rac_llm_extract_thinking returns pointer/length slices, but rac_llm_split_thinking_tokens, set_structured_output_if_present, and publish_generation_event consume response/thinking as null-terminated strings. A response prefix before a thinking block can leak trailing thinking text into token accounting, structured-output validation, or telemetry.
Proposed fix
const char* raw_text = raw.text ? raw.text : "";
(void)rac_llm_extract_thinking(raw_text, &response, &response_len, &thinking, &thinking_len);
+ const std::string response_text =
+ response != nullptr ? std::string(response, response_len) : std::string();
+ const std::string thinking_text =
+ thinking != nullptr ? std::string(thinking, thinking_len) : std::string();
+ const char* response_cstr = response_text.empty() ? nullptr : response_text.c_str();
+ const char* thinking_cstr = thinking_text.empty() ? nullptr : thinking_text.c_str();
int32_t thinking_tokens = 0;
int32_t response_tokens = raw.completion_tokens;
- (void)rac_llm_split_thinking_tokens(raw.completion_tokens, response, thinking, &thinking_tokens,
- &response_tokens);
+ (void)rac_llm_split_thinking_tokens(raw.completion_tokens, response_cstr, thinking_cstr,
+ &thinking_tokens, &response_tokens);
LLMGenerationResult result;
- set_result_from_raw(ref, raw, response, response_len, thinking, thinking_len, thinking_tokens,
- response_tokens, options.max_tokens, &result);
- set_structured_output_if_present(response, &result);
+ set_result_from_raw(ref, raw, response_cstr, response_text.size(), thinking_cstr,
+ thinking_text.size(), thinking_tokens, response_tokens,
+ options.max_tokens, &result);
+ set_structured_output_if_present(response_cstr, &result);
publish_generation_event(
runanywhere::v1::GENERATION_EVENT_KIND_COMPLETED, request.prompt().c_str(), nullptr,
- response, nullptr, ref.model_id, raw.completion_tokens,
+ response_cstr, nullptr, ref.model_id, raw.completion_tokens,
raw.total_time_ms > 0 ? raw.total_time_ms : elapsed, raw.prompt_tokens);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const char* raw_text = raw.text ? raw.text : ""; | |
| (void)rac_llm_extract_thinking(raw_text, &response, &response_len, &thinking, &thinking_len); | |
| int32_t thinking_tokens = 0; | |
| int32_t response_tokens = raw.completion_tokens; | |
| (void)rac_llm_split_thinking_tokens(raw.completion_tokens, response, thinking, &thinking_tokens, | |
| &response_tokens); | |
| LLMGenerationResult result; | |
| set_result_from_raw(ref, raw, response, response_len, thinking, thinking_len, thinking_tokens, | |
| response_tokens, options.max_tokens, &result); | |
| set_structured_output_if_present(response, &result); | |
| publish_generation_event( | |
| runanywhere::v1::GENERATION_EVENT_KIND_COMPLETED, request.prompt().c_str(), nullptr, | |
| response, nullptr, ref.model_id, raw.completion_tokens, | |
| raw.total_time_ms > 0 ? raw.total_time_ms : elapsed, raw.prompt_tokens); | |
| const char* raw_text = raw.text ? raw.text : ""; | |
| (void)rac_llm_extract_thinking(raw_text, &response, &response_len, &thinking, &thinking_len); | |
| const std::string response_text = | |
| response != nullptr ? std::string(response, response_len) : std::string(); | |
| const std::string thinking_text = | |
| thinking != nullptr ? std::string(thinking, thinking_len) : std::string(); | |
| const char* response_cstr = response_text.empty() ? nullptr : response_text.c_str(); | |
| const char* thinking_cstr = thinking_text.empty() ? nullptr : thinking_text.c_str(); | |
| int32_t thinking_tokens = 0; | |
| int32_t response_tokens = raw.completion_tokens; | |
| (void)rac_llm_split_thinking_tokens(raw.completion_tokens, response_cstr, thinking_cstr, | |
| &thinking_tokens, &response_tokens); | |
| LLMGenerationResult result; | |
| set_result_from_raw(ref, raw, response_cstr, response_text.size(), thinking_cstr, | |
| thinking_text.size(), thinking_tokens, response_tokens, | |
| options.max_tokens, &result); | |
| set_structured_output_if_present(response_cstr, &result); | |
| publish_generation_event( | |
| runanywhere::v1::GENERATION_EVENT_KIND_COMPLETED, request.prompt().c_str(), nullptr, | |
| response_cstr, nullptr, ref.model_id, raw.completion_tokens, | |
| raw.total_time_ms > 0 ? raw.total_time_ms : elapsed, raw.prompt_tokens); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/runanywhere-commons/src/features/llm/llm_module.cpp` around lines 2457 -
2473, The slices produced by rac_llm_extract_thinking in llm_module.cpp are
passed directly to C-string consumers even though they are bounded by length, so
response/thinking may carry trailing text. Update the flow around
rac_llm_split_thinking_tokens, set_structured_output_if_present, and
publish_generation_event to first copy the extracted response and thinking
slices into null-terminated buffers, then use those buffers for token
accounting, structured-output handling, and telemetry.
| /// Convert a failed native adaptive-context status into an SDK error with backend detail. | ||
| private static func throwIfFailed(_ status: rac_result_t, operation: String) throws { | ||
| guard status == RAC_SUCCESS else { | ||
| let nativeMessage = String(cString: rac_error_message(status)) | ||
| throw SDKException( | ||
| code: .processingFailed, | ||
| message: "LLM adaptive context \(operation) failed: \(nativeMessage) (\(status))", | ||
| category: .component | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve native adaptive-context error codes.
throwIfFailed maps every native failure to .processingFailed, including RAC_ERROR_NOT_SUPPORTED and component-not-ready cases. That breaks backend feature probing and loses the C ABI error contract; route through the existing native-result-to-SDK-error mapper or map these statuses explicitly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge`+LLM.swift
around lines 145 - 153, The throwIfFailed helper in CppBridge+LLM is collapsing
all native adaptive-context failures into .processingFailed, which loses the
native C ABI error meaning. Update throwIfFailed to preserve rac_result_t
semantics by routing failures through the existing native-result-to-SDK-error
mapping used elsewhere in the bridge, or by explicitly translating statuses like
RAC_ERROR_NOT_SUPPORTED and component-not-ready into their corresponding
SDKException codes. Keep the native error message in the thrown exception, but
ensure the status-specific code is not discarded.
Ready for ReviewHi @Siddhesh2377 and @shubhammalhotra28 this PR fixes Issue #480: a SIGABRT crash when calling The ProblemThe upstream llama.cpp ggml Metal backend calls The Fix (2 files)
CI StatusAll 18 checks passing: iOS device, macOS debug/release, Linux debug/ASAN, Android arm64, Kotlin, WASM, Swift SPM, Flutter, React Native, Web, and more. Bot CommentsThe bot comments on this PR (Cursor Bugbot, CodeRabbit) are about pre-existing adaptive context code from earlier commits in the branch (issue #500), not this fix. Our 2 files received zero bot findings. Happy to make any changes if required |
|
Hi @shubhammalhotra28 @Siddhesh2377 - following up here. The Metal guard update is in and CI is green. This is still stacked on #512, so I can rebase after #512 if that works better. |
# Conflicts: # sdk/runanywhere-swift/Sources/RunAnywhere/CRACommons/include/rac_llm_component.h # sdk/runanywhere-swift/Sources/RunAnywhere/CRACommons/include/rac_llm_service.h
# Conflicts: # sdk/runanywhere-commons/src/features/llm/llm_module.cpp
604bc89 to
0fb395f
Compare
A12/A13 GPUs (iPhone XS Max, iPhone 11) do not support MTLResidencySet, but the upstream @available(iOS 18.0) guard passes on these devices because they run iOS 18. Calling newResidencySetWithDescriptor: on unsupported hardware triggers: -[MTLDebugDevice newResidencySetWithDescriptor:error:]:2785: failed assertion 'device does not support residency sets.' Fix: add [device supportsResidencySets] check before enabling residency sets in ggml-metal-device.m, applied as a patch via CMake FetchContent PATCH_COMMAND. Fixes RunanywhereAI#480
The committed patch called -[MTLDevice supportsResidencySets], which is not a real Metal API (verified against MTLDevice.h across iOS 17.0-18.6 SDKs) and would fail to compile since mtl_device is id<MTLDevice>. The patch file was also missing a trailing newline, which makes `git apply` reject it with "corrupt patch" before the build even reaches the compiler. Both verified by applying against the real pinned llama.cpp (b9878) source. Swap in supportsFamily:MTLGPUFamilyApple7, the real API this same file already uses for supports_gpu_family_apple7, and add the trailing newline.
0fb395f to
dc71c7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
engines/llamacpp/patches/001-metal-residency-set-guard.patch (1)
27-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPatch may fail to apply if upstream llama.cpp line numbers drift.
The hunk targets
@@ -824,9 +824,12 @@inggml-metal-device.m. Since this patch is maintained out-of-tree and applied viaFetchContent, any upstream change that shifts lines around 824 will causegit applyto fail. Consider using a more resilient apply strategy (e.g.,git apply --3wayor patch fuzz tolerance) in the CMakeLists.txt wiring to reduce maintenance burden. The CMakeLists.txt file is not included in this review cohort, so verify the apply command there.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engines/llamacpp/patches/001-metal-residency-set-guard.patch` around lines 27 - 45, Make the out-of-tree patch application in the relevant CMakeLists.txt wiring resilient to upstream line shifts by using a three-way apply or appropriate patch fuzz strategy instead of strict git apply. Preserve the existing patch application flow and verify it still applies the ggml-metal-device.m change reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@engines/llamacpp/patches/001-metal-residency-set-guard.patch`:
- Around line 5-21: Resolve the contradictory residency-support documentation in
the patch comment: verify whether iPhone 11/A13 hardware supports the capability
or whether the reported crash is simulator-only, then update the explanatory
text to state the correct supported GPU families and affected devices. Keep the
`supportsFamily:MTLGPUFamilyApple6` guard aligned with that verified runtime
behavior.
---
Nitpick comments:
In `@engines/llamacpp/patches/001-metal-residency-set-guard.patch`:
- Around line 27-45: Make the out-of-tree patch application in the relevant
CMakeLists.txt wiring resilient to upstream line shifts by using a three-way
apply or appropriate patch fuzz strategy instead of strict git apply. Preserve
the existing patch application flow and verify it still applies the
ggml-metal-device.m change reliably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12cb280b-c16f-4044-ab4e-d060c4a1f1b2
📒 Files selected for processing (3)
engines/llamacpp/CMakeLists.txtengines/llamacpp/patches/001-metal-residency-set-guard.patchsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+LLM.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- engines/llamacpp/CMakeLists.txt
- sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+LLM.swift
| The @available(iOS 18.0, ...) check passes on A12/A13 devices (iPhone XS | ||
| Max, iPhone 11) because these devices run iOS 18, but their GPU does not | ||
| support MTLResidencySet. Calling newResidencySetWithDescriptor: on these | ||
| devices triggers a Metal debug assertion failure: | ||
|
|
||
| -[MTLDebugDevice newResidencySetWithDescriptor:error:]:2785: | ||
| failed assertion 'device does not support residency sets.' | ||
|
|
||
| Fix: also check [device supportsFamily:MTLGPUFamilyApple6] when | ||
| initializing the use_residency_sets flag in the device properties, so | ||
| the entire residency set code path is skipped on unsupported GPUs. | ||
| (There is no `-[MTLDevice supportsResidencySets]` API; supportsFamily: | ||
| is the real, documented way to query GPU-family-gated capabilities and | ||
| is the real runtime capability check for this GPU-family-gated feature.) | ||
|
|
||
| Residency sets require Apple GPU family 6+ (A13+). | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Patch comment contradicts itself on A13 residency-set support.
Lines 5-8 state A12/A13 GPUs "do not support MTLResidencySet," but line 20 states "Residency sets require Apple GPU family 6+ (A13+)," implying A13 does support them. If A13 truly lacks support, the guard at line 39 (supportsFamily:MTLGPUFamilyApple6) would still enable residency sets on A13 devices, leaving the crash unfixed on iPhone 11. Clarify whether the iPhone 11 crash is real-hardware or simulator-only, and update the comment to resolve the contradiction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engines/llamacpp/patches/001-metal-residency-set-guard.patch` around lines 5
- 21, Resolve the contradictory residency-support documentation in the patch
comment: verify whether iPhone 11/A13 hardware supports the capability or
whether the reported crash is simulator-only, then update the explanatory text
to state the correct supported GPU families and affected devices. Keep the
`supportsFamily:MTLGPUFamilyApple6` guard aligned with that verified runtime
behavior.
1 participant
Fixes #480
Summary
Note
Medium Risk
Metal patch affects all iOS llama.cpp fetches; adaptive context changes KV-cache behavior on inference paths and new exported ABI surface, though unsupported backends return NOT_SUPPORTED.
Overview
iOS llama.cpp Metal crash (issue #480): On iOS builds only,
FetchContentnow applies001-metal-residency-set-guard.patchsouse_residency_setsstays off unless[device supportsResidencySets]is true, avoidingMTLResidencySetSIGABRT on A12/A13 devices running iOS 18.Adaptive LLM context: New component and lifecycle-owned C APIs (
rac_llm_component_*andrac_llm_*_lifecycle/rac_llm_generate_from_context_proto) delegate to existing service vtable ops for incremental KV context (system prompt seed, append, generate without clearing cache, clear). Implementations live inllm_module.cppwith a sharedcall_lifecycle_ophelper for the proto lifecycle path.RACommons.exportsadds the new symbols for dynamic Apple linking.Swift SDK:
CppBridge.LLMbinds the lifecycle symbols viaNativeProtoABI, andRunAnywhere+TextGenerationadds publicinjectSystemPrompt,appendContext,generateFromContext, andclearContextfor RAG-style flows on the loaded on-device model.Reviewed by Cursor Bugbot for commit e148de5. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes