Repository navigation
fix(provider): bound weight hash memory growth - #358
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This PR hardens the weight-hashing path against unbounded memory allocation during the whole-file fallback and removes the concurrent parallel-dispatch implementation, but it does not change the coordinator's fail-open enforcement of weight hashes (SEC-007), so T-007, T-012, and T-027 remain open at the system level. Trust Boundaries Touched
Threat AnalysisT-007 · T-012 · T-027 — Weight hash integrity (Tampering / Information Disclosure)The hash value produced by this code is unchanged for all files that fit within the streaming paths (the dominant case for every model shard). The coordinator still receives the same hash and still enforces it fail-open (SEC-007: omitting Specific observations:
T-027 —
|
| Finding | Status after this PR |
|---|---|
| SEC-007 | Still open — coordinator fail-open enforcement is untouched by this PR |
No SEC-* findings are resolved by this change.
Summary for Reviewer
The coordinator-side fix (SEC-007: make weight hash enforcement fail-closed) is the missing half of this work. Until that lands, an operator can still omit weight_hash entirely and be admitted. This PR makes the provider's computation of that hash more memory-safe and reliable, which is necessary but not sufficient to close T-007/T-012/T-027.
🔐 Threat model: docs/threat-model.yaml · Updates on each push to this PR
c43395a to
ac02c27
Compare
ethenotethan
left a comment
ethenotethan
left a comment
There was a problem hiding this comment.
Automated Code Review — Layr-Labs/d-inference#
Verdict: REQUEST_CHANGES
Security — ✅ No issues found
Performance — 3 finding(s) (2 blocking)
- 🔴 [CRITICAL]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:127-142— Semaphore-based concurrency control may cause goroutine buildup under high load- Suggestion: Consider using errgroup.Group with SetLimit() or a worker pool pattern to bound concurrent operations more efficiently
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:182— Data(contentsOf:) loads entire file into memory as fallback- Suggestion: Move this fallback inside the semaphore-controlled section or add file size checks to prevent loading very large files entirely into memory
- 🔵 [INFO]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:115— UnsafeMutablePointer allocation scales with file count without bounds- Suggestion: Add a maximum file count check or use a different data structure to prevent excessive memory allocation for very large manifests
Type_diligence — ✅ No issues found
Additive_complexity — 1 finding(s)
- 🔵 [INFO]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:205-238— hashSingleFileViaInputStream duplicates streaming pattern from hashSingleFileViaHandle- Suggestion: Extract common streaming hash pattern into shared helper function to reduce code duplication and maintenance burden
4 finding(s) total, 2 blocking. Verdict: REQUEST_CHANGES.
🤖 Automated review by Centaur · DAR-186
Gajesh2007
changed the title
ac02c27 to
abd5433
Compare
ethenotethan
left a comment
ethenotethan
left a comment
There was a problem hiding this comment.
Automated Code Review — Layr-Labs/d-inference#
Verdict: REQUEST_CHANGES
Security — ✅ No issues found
Performance — 3 finding(s) (3 blocking)
- 🔴 [CRITICAL]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:110-149— Semaphore-based concurrency control may cause goroutine buildup under high load- Suggestion: Consider using errgroup.Group with SetLimit() or worker pool pattern to bound concurrent operations more efficiently
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:182-184— Data(contentsOf:) loads entire file into memory as fallback- Suggestion: Move this fallback inside the semaphore-controlled section or add file size checks to prevent loading very large files entirely into memory
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:207-208— Fixed-size buffer allocation without size validation- Suggestion: Add validation that bufferSize is reasonable or make buffer size configurable based on available memory
Type_diligence — ✅ No issues found
Additive_complexity — 1 finding(s)
- 🔵 [INFO]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:205-238— hashSingleFileViaInputStream duplicates streaming pattern from hashSingleFileViaHandle- Suggestion: Extract common streaming hash pattern into shared helper function to reduce code duplication and maintenance burden
4 finding(s) total, 3 blocking. Verdict: REQUEST_CHANGES.
🤖 Automated review by Centaur · DAR-186
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac02c27885
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Last-resort compatibility fallback: NSData can handle file-protection | ||
| // cases that raw POSIX open() rejects. Fan-out is capped above so this | ||
| // cannot load every shard at once. | ||
| guard let data = autoreleasepool(invoking: { try? Data(contentsOf: url) }) else { |
There was a problem hiding this comment.
On Linux builds of ProviderCoreFoundation (for example the Linux/GCP darkbloom-publish path used by scripts/publish-model.sh), Swift does not provide autoreleasepool, so this new unguarded call makes the target fail to compile before any hashing runs; I verified with the Linux Swift toolchain error cannot find 'autoreleasepool' in scope. The same call in the read loop below has the same issue, so this needs a Linux-compatible wrapper/no-op path rather than using autoreleasepool directly.
Useful? React with 👍 / 👎.
| private static func maxRSSBytes() throws -> UInt64 { | ||
| #if canImport(Darwin) || canImport(Glibc) | ||
| var usage = rusage() | ||
| guard getrusage(RUSAGE_SELF, &usage) == 0 else { |
There was a problem hiding this comment.
On Linux, Glibc exposes RUSAGE_SELF as __rusage_who while getrusage expects __rusage_who_t/Int32; building this new ProviderCoreFoundationTests file with Swift 6.2.4 fails at this line with cannot convert value of type '__rusage_who' to expected argument type '__rusage_who_t'. Since this test target is meant to stay Linux-buildable, use the raw value/cast under Glibc so the suite can compile there.
Useful? React with 👍 / 👎.
abd5433 to
11c3315
Compare
ethenotethan
left a comment
ethenotethan
left a comment
There was a problem hiding this comment.
Automated Code Review — Layr-Labs/d-inference#
Verdict: REQUEST_CHANGES
Security — ✅ No issues found
Performance — 1 finding(s) (1 blocking)
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:125-145— Worker-based parallelism creates uneven work distribution for file hashing- Suggestion: Use a work-stealing queue or TaskGroup instead of static worker assignment to ensure balanced load distribution across files of varying sizes
Type_diligence — 1 finding(s) (1 blocking)
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:207-225— Multiple unsafe type casts from Any to numeric types without checking cast success- Suggestion: Use guard let with type assertions: guard let size = rawSize as? NSNumber else { return nil }. Current code could panic if FileManager returns unexpected types for .size attribute
Additive_complexity — 3 finding(s)
- 🔵 [INFO]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:194-215— fileSizeBytes function handles multiple type conversions unnecessarily- Suggestion: Use FileManager.default.attributesOfItem(atPath:)[.size] as? UInt64 directly, or simplify with a single NSNumber conversion since all numeric types can convert through NSNumber
- 🔵 [INFO]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:193-196— isWholeFileFallbackAllowed is a trivial one-liner extracted as separate function- Suggestion: Inline this simple comparison directly where used, or keep as private helper if used multiple times
- 🔵 [INFO]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:239-267— hashSingleFileViaInputStream duplicates streaming pattern from FileHandle version- Suggestion: Extract common streaming hash logic into shared helper function that accepts a read closure, reducing duplication between FileHandle and InputStream paths
5 finding(s) total, 2 blocking. Verdict: REQUEST_CHANGES.
🤖 Automated review by Centaur · DAR-186
| static func isWholeFileFallbackAllowed(fileSizeBytes: UInt64) -> Bool { | ||
| fileSizeBytes <= maxWholeFileFallbackBytes | ||
| } | ||
|
|
||
| private static func fileSizeBytes(at url: URL) -> UInt64? { | ||
| let path = url.resolvingSymlinksInPath().path | ||
| guard let rawSize = try? FileManager.default.attributesOfItem(atPath: path)[.size] else { | ||
| return nil | ||
| } | ||
| if let size = rawSize as? NSNumber { | ||
| return size.uint64Value | ||
| } | ||
| if let size = rawSize as? UInt64 { | ||
| return size | ||
| } | ||
| if let size = rawSize as? Int64, size >= 0 { | ||
| return UInt64(size) | ||
| } | ||
| if let size = rawSize as? Int, size >= 0 { | ||
| return UInt64(size) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🔵 [INFO] 🧩 fileSizeBytes function handles multiple type conversions unnecessarily
💡 Suggestion: Use FileManager.default.attributesOfItem(atPath:)[.size] as? UInt64 directly, or simplify with a single NSNumber conversion since all numeric types can convert through NSNumber
📊 Score: 2×3 = 6 · Category: over-abstraction
|
|
||
| static func isWholeFileFallbackAllowed(fileSizeBytes: UInt64) -> Bool { | ||
| fileSizeBytes <= maxWholeFileFallbackBytes | ||
| } |
There was a problem hiding this comment.
🔵 [INFO] 🧩 isWholeFileFallbackAllowed is a trivial one-liner extracted as separate function
💡 Suggestion: Inline this simple comparison directly where used, or keep as private helper if used multiple times
📊 Score: 1×2 = 2 · Category: misplaced responsibility
| private static func hashSingleFileViaInputStream(at url: URL) -> SHA256Digest? { | ||
| guard let stream = InputStream(url: url) else { | ||
| return nil | ||
| } | ||
| stream.open() | ||
| defer { stream.close() } | ||
|
|
||
| var hasher = SHA256() | ||
| var buffer = [UInt8](repeating: 0, count: bufferSize) | ||
|
|
||
| while true { | ||
| let bytesRead = buffer.withUnsafeMutableBufferPointer { ptr -> Int in | ||
| guard let baseAddress = ptr.baseAddress else { return -1 } | ||
| return stream.read(baseAddress, maxLength: bufferSize) | ||
| } | ||
| if bytesRead < 0 { | ||
| return nil | ||
| } | ||
| if bytesRead == 0 { | ||
| break | ||
| } | ||
| buffer.withUnsafeBufferPointer { ptr in | ||
| guard let baseAddress = ptr.baseAddress else { return } | ||
| hasher.update(bufferPointer: UnsafeRawBufferPointer(start: baseAddress, count: bytesRead)) | ||
| } | ||
| } | ||
|
|
||
| return hasher.finalize() | ||
| } |
There was a problem hiding this comment.
🔵 [INFO] 🧩 hashSingleFileViaInputStream duplicates streaming pattern from FileHandle version
💡 Suggestion: Extract common streaming hash logic into shared helper function that accepts a read closure, reducing duplication between FileHandle and InputStream paths
📊 Score: 2×3 = 6 · Category: duplicate logic
11c3315 to
b6fbec1
Compare
ethenotethan
left a comment
ethenotethan
left a comment
There was a problem hiding this comment.
Automated Code Review — Layr-Labs/d-inference#
Verdict: REQUEST_CHANGES
Security — ✅ No issues found
Performance — 3 finding(s) (2 blocking)
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:135-147— Sequential worker dispatch creates uneven load distribution- Suggestion: Use a work-stealing queue or TaskGroup for better load balancing across workers
- 🔵 [INFO]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:216-228— Multiple type checks and casts for file size attribute- Suggestion: Use a single switch statement or guard-let chain to avoid redundant type checking
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:264-266— Fixed-size buffer allocation without validation- Suggestion: Pre-validate bufferSize is within bounds before allocating the buffer array
Type_diligence — 2 finding(s) (1 blocking)
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:116— fileSizeBytes(at:) returns optional UInt64 but rawSize is cast without type assertion- Suggestion: Add proper type assertion with ok-checking: if let size = rawSize as? NSNumber { return size.uint64Value } else if let size = rawSize as? UInt64 { return size } // etc
- 🔵 [INFO]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:205-228— Multiple unsafe type casts in fileSizeBytes without checking cast success- Suggestion: Replace force casts with safe type assertions: guard let size = rawSize as? NSNumber else { /* try next type */ }
Additive_complexity — 5 finding(s) (1 blocking)
- 🔵 [INFO]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:147— Worker count calculation could be simplified- Suggestion: Replace
max(1, min(maxConcurrentFileHashes, count))with justmin(maxConcurrentFileHashes, count)since min() already handles the zero case naturally
- Suggestion: Replace
- 🔵 [INFO]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:196-200— Single-use helper function adds unnecessary indirection- Suggestion: Inline
isWholeFileFallbackAllowedat its single call site since it's just a simple comparison
- Suggestion: Inline
- 🔵 [INFO]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:202-206— Single-use helper function adds unnecessary indirection- Suggestion: Inline
isStreamingBufferSizeAllowedat its single call site since it's just a simple bounds check
- Suggestion: Inline
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:208-237— Complex file size extraction reimplements Foundation functionality- Suggestion: Use
try url.resourceValues(forKeys: [.fileSizeKey]).fileSizeinstead of manual attribute parsing with multiple type checks
- Suggestion: Use
- 🔵 [INFO]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:239-245— Conditional compilation wrapper for single autoreleasepool call- Suggestion: Use autoreleasepool directly since it's available on all Swift platforms; the #if canImport(ObjectiveC) check is unnecessary complexity
10 finding(s) total, 4 blocking. Verdict: REQUEST_CHANGES.
🤖 Automated review by Centaur · DAR-186
| return UInt64(size) | ||
| } | ||
| if let size = rawSize as? Int, size >= 0 { | ||
| return UInt64(size) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| private static func withAutoreleasePool<T>(_ body: () -> T) -> T { | ||
| #if canImport(ObjectiveC) | ||
| return autoreleasepool(invoking: body) | ||
| #else | ||
| return body() |
There was a problem hiding this comment.
🔵 [INFO] ⚡ Multiple type checks and casts for file size attribute
💡 Suggestion: Use a single switch statement or guard-let chain to avoid redundant type checking
📊 Score: 2×3 = 6 · Category: Repeated work
| static func isWholeFileFallbackAllowed(fileSizeBytes: UInt64) -> Bool { | ||
| fileSizeBytes <= maxWholeFileFallbackBytes | ||
| } | ||
|
|
||
| static func isStreamingBufferSizeAllowed(_ bytes: Int) -> Bool { |
There was a problem hiding this comment.
🔵 [INFO] 🧩 Single-use helper function adds unnecessary indirection
💡 Suggestion: Inline isWholeFileFallbackAllowed at its single call site since it's just a simple comparison
📊 Score: 2×2 = 4 · Category: over-abstraction
| } | ||
|
|
||
| private static func fileSizeBytes(at url: URL) -> UInt64? { | ||
| let path = url.resolvingSymlinksInPath().path | ||
| guard let rawSize = try? FileManager.default.attributesOfItem(atPath: path)[.size] else { |
There was a problem hiding this comment.
🔵 [INFO] 🧩 Single-use helper function adds unnecessary indirection
💡 Suggestion: Inline isStreamingBufferSizeAllowed at its single call site since it's just a simple bounds check
📊 Score: 2×2 = 4 · Category: over-abstraction
b6fbec1 to
16fb4d5
Compare
ethenotethan
left a comment
ethenotethan
left a comment
There was a problem hiding this comment.
Automated Code Review — Layr-Labs/d-inference#
Verdict: REQUEST_CHANGES
Security — ✅ No issues found
Performance — 3 finding(s) (3 blocking)
- 🔴 [CRITICAL]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:114-162— Worker goroutines access shared state without proper synchronization- Suggestion: Replace NSLock with atomic operations or use a proper work-stealing queue implementation to avoid lock contention and potential race conditions in the shared index access pattern
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:130-138— Manual unsafe pointer management with complex deallocation logic- Suggestion: Use Swift's built-in concurrent collections or TaskGroup for cleaner memory management and better performance than manual unsafe pointer allocation
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:268-287— Fixed-size buffer allocation in InputStream path without size validation- Suggestion: Validate bufferSize against maxStreamingBufferSize before allocating the buffer array to prevent potential large allocations
Type_diligence — ✅ No issues found
Additive_complexity — 3 finding(s) (1 blocking)
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:125-140— Complex worker pool with manual memory management for bounded parallelism- Suggestion: Use TaskGroup or OperationQueue instead of manual unsafe pointers and locks for cleaner concurrency
- 🔵 [INFO]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:228-236— Custom autoreleasepool wrapper when Foundation already provides this- Suggestion: Use Foundation's autoreleasepool directly instead of custom conditional compilation wrapper
- 🔵 [INFO]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:257-289— InputStream hashing duplicates FileHandle streaming pattern- Suggestion: Extract common streaming hash logic to reduce duplication between FileHandle and InputStream paths
6 finding(s) total, 4 blocking. Verdict: REQUEST_CHANGES.
🤖 Automated review by Centaur · DAR-186
| private static func withAutoreleasePool<T>(_ body: () -> T) -> T { | ||
| #if canImport(ObjectiveC) | ||
| return autoreleasepool(invoking: body) | ||
| #else | ||
| return body() | ||
| #endif | ||
| } | ||
|
|
||
| private static func hashSingleFileViaHandle(at url: URL) -> SHA256Digest? { |
There was a problem hiding this comment.
🔵 [INFO] 🧩 Custom autoreleasepool wrapper when Foundation already provides this
💡 Suggestion: Use Foundation's autoreleasepool directly instead of custom conditional compilation wrapper
📊 Score: 2×3 = 6 · Category: reinventing stdlib
| private static func hashSingleFileViaInputStream(at url: URL) -> SHA256Digest? { | ||
| guard isStreamingBufferSizeAllowed(bufferSize) else { | ||
| return nil | ||
| } | ||
| guard let stream = InputStream(url: url) else { | ||
| return nil | ||
| } | ||
| stream.open() | ||
| defer { stream.close() } | ||
|
|
||
| var hasher = SHA256() | ||
| var buffer = [UInt8](repeating: 0, count: bufferSize) | ||
|
|
||
| while true { | ||
| let bytesRead = buffer.withUnsafeMutableBufferPointer { ptr -> Int in | ||
| guard let baseAddress = ptr.baseAddress else { return -1 } | ||
| return stream.read(baseAddress, maxLength: bufferSize) | ||
| } | ||
| if bytesRead < 0 { | ||
| return nil | ||
| } | ||
| if bytesRead == 0 { | ||
| break | ||
| } | ||
| buffer.withUnsafeBufferPointer { ptr in | ||
| guard let baseAddress = ptr.baseAddress else { return } | ||
| hasher.update(bufferPointer: UnsafeRawBufferPointer(start: baseAddress, count: bytesRead)) | ||
| } | ||
| } | ||
|
|
||
| return hasher.finalize() | ||
| } | ||
| } |
There was a problem hiding this comment.
🔵 [INFO] 🧩 InputStream hashing duplicates FileHandle streaming pattern
💡 Suggestion: Extract common streaming hash logic to reduce duplication between FileHandle and InputStream paths
📊 Score: 2×2 = 4 · Category: duplicate logic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16fb4d584a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard let size = fileSizeBytes(at: url), isWholeFileFallbackAllowed(fileSizeBytes: size) else { | ||
| return nil |
There was a problem hiding this comment.
When a shard larger than 64 MiB hits the documented fallback path where the streaming readers fail, this guard makes WeightHasher.hashSingleFile return nil; however the manifest download verifier still treats that as a signal to run its own Data(contentsOf:) fallback (ModelCatalog.swift:1096-1099 and 1288-1295). That means large URLSession-downloaded model files can still be read wholesale into memory during verification, just one frame higher, so the memory cap added here does not actually protect the download paths that call through sha256Hex.
Useful? React with 👍 / 👎.
16fb4d5 to
ec1590e
Compare
ethenotethan
left a comment
ethenotethan
left a comment
There was a problem hiding this comment.
Automated Code Review — Layr-Labs/d-inference#
Verdict: REQUEST_CHANGES
Security — ✅ No issues found
Performance — 3 finding(s) (3 blocking)
- 🔴 [CRITICAL]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:112-122— Removed parallel file hashing without bounded concurrency control- Suggestion: The original parallel implementation had unbounded goroutines but provided better throughput. Consider adding bounded concurrency (e.g., semaphore with worker pool) instead of removing parallelism entirely for large model manifests.
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:184-195— Autoreleasepool wrapping every FileHandle.read() call adds overhead- Suggestion: Move autoreleasepool outside the read loop to drain accumulated objects periodically (e.g., every 100 iterations) rather than on every single read operation.
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:147-156— Whole-file Data(contentsOf:) fallback can block for large files- Suggestion: The size check helps but Data(contentsOf:) still blocks the calling thread. Consider making this async or moving to a background queue for files approaching the 64MB limit.
Type_diligence — ✅ No issues found
Additive_complexity — 4 finding(s) (1 blocking)
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:112-125— Removed parallel file hashing without clear justification- Suggestion: Consider keeping bounded parallelism (e.g., TaskGroup with limited concurrency) rather than fully sequential processing for large model shards
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Models/ModelCatalog.swift:1497-1503— Unnecessary wrapper function with single use case- Suggestion: Inline sha256HexForVerification calls directly or remove the hasher parameter version if only used in tests
- 🔵 [INFO]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:178-184— Custom autoreleasepool wrapper when Foundation already provides this- Suggestion: Use Foundation's autoreleasepool directly instead of custom wrapper with conditional compilation
- 🔵 [INFO]
provider-swift/Sources/ProviderCoreFoundation/WeightHasher.swift:159-165— Multiple buffer size validation functions for simple bounds check- Suggestion: Combine into single validation function or inline the simple bounds checks
7 finding(s) total, 4 blocking. Verdict: REQUEST_CHANGES.
🤖 Automated review by Centaur · DAR-186
| @@ -180,7 +192,7 @@ public struct WeightHasher: Sendable { | |||
| var hasher = SHA256() | |||
|
|
|||
| while true { | |||
| guard let chunk = try? handle.read(upToCount: bufferSize) else { | |||
| guard let chunk = withAutoreleasePool({ try? handle.read(upToCount: bufferSize) }) else { | |||
There was a problem hiding this comment.
🟡 [MEDIUM] ⚡ Autoreleasepool wrapping every FileHandle.read() call adds overhead
💡 Suggestion: Move autoreleasepool outside the read loop to drain accumulated objects periodically (e.g., every 100 iterations) rather than on every single read operation.
📊 Score: 2×5 = 10 · Category: Blocking I/O on hot paths
| // shard cannot be copied into memory after both streaming paths fail. | ||
| guard let size = fileSizeBytes(at: url), isWholeFileFallbackAllowed(fileSizeBytes: size) else { | ||
| return nil | ||
| } | ||
| guard let data = withAutoreleasePool({ try? Data(contentsOf: url) }) else { | ||
| return nil | ||
| } | ||
| var hasher = SHA256() | ||
| hasher.update(data: data) | ||
| return hasher.finalize() |
There was a problem hiding this comment.
🟡 [MEDIUM] ⚡ Whole-file Data(contentsOf:) fallback can block for large files
💡 Suggestion: The size check helps but Data(contentsOf:) still blocks the calling thread. Consider making this async or moving to a background queue for files approaching the 64MB limit.
📊 Score: 3×3 = 9 · Category: Blocking I/O on hot paths
| static func sha256HexForVerification(of url: URL) -> String { | ||
| sha256HexForVerification(of: url, hasher: sha256Hex) | ||
| } | ||
|
|
||
| static func sha256HexForVerification(of url: URL, hasher: (URL) -> String?) -> String { | ||
| hasher(url) ?? "<unreadable>" | ||
| } |
There was a problem hiding this comment.
🔵 [INFO] 🧩 Unnecessary wrapper function with single use case
💡 Suggestion: Inline sha256HexForVerification calls directly or remove the hasher parameter version if only used in tests
📊 Score: 2×3 = 6 · Category: over-abstraction
| private static func withAutoreleasePool<T>(_ body: () -> T) -> T { | ||
| #if canImport(ObjectiveC) | ||
| return autoreleasepool(invoking: body) | ||
| #else | ||
| return body() | ||
| #endif | ||
| } |
There was a problem hiding this comment.
🔵 [INFO] 🧩 Custom autoreleasepool wrapper when Foundation already provides this
💡 Suggestion: Use Foundation's autoreleasepool directly instead of custom wrapper with conditional compilation
📊 Score: 1×2 = 2 · Category: reinventing stdlib
| static func isWholeFileFallbackAllowed(fileSizeBytes: UInt64) -> Bool { | ||
| fileSizeBytes <= maxWholeFileFallbackBytes | ||
| } | ||
|
|
||
| static func isStreamingBufferSizeAllowed(_ bytes: Int) -> Bool { | ||
| bytes > 0 && bytes <= maxStreamingBufferSize | ||
| } |
There was a problem hiding this comment.
🔵 [INFO] 🧩 Multiple buffer size validation functions for simple bounds check
💡 Suggestion: Combine into single validation function or inline the simple bounds checks
📊 Score: 2×3 = 6 · Category: over-configuration
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec1590e1bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard let size = fileSizeBytes(at: url), isWholeFileFallbackAllowed(fileSizeBytes: size) else { | ||
| return nil |
There was a problem hiding this comment.
When a large model shard reaches the file-protection fallback described above (i.e. both streaming readers fail), this guard returns nil before the Data(contentsOf:) compatibility path can run. With the caller-side Data fallback now removed in ModelCatalog.sha256HexForVerification, that nil becomes <unreadable>, causing manifest verification to delete/retry the shard and ultimately fail for URLSession-downloaded shards larger than 64 MiB that need the Foundation/file-coordinated fallback. This needs a bounded Foundation-compatible streaming path rather than rejecting those large files outright.
Useful? React with 👍 / 👎.
…x ff4bb35b) libs/mlx-swift-lm moves efd38585 -> d4fd516e, mlx-swift-lm #356's head after it merges SDK main 476cb7c0. Main gained #358, which pins Layr-Labs/mlx-swift eedca444 (was 6c31f9f1). That mlx-swift pins mlx ff4bb35b and mlx-c 004c735f, which bring two fixes: - float16 and bfloat16 floor divide computed in float32 (Layr-Labs/mlx#40; CPU and the Metal binary_ops kernel); - a freed compiled closure erased from every thread's compile cache (Layr-Labs/mlx#43). libs/mlx-swift moves to eedca444 and libs/mlx to ff4bb35b in lockstep, so the metallib is rebuilt from the new kernels. No Swift API changed: the SDK's own change is Package.swift only, and mlx-swift changes its pins and regenerated sources.
…358) into perf/decoder-protection-0.10.2 The release branch moved libs/mlx (ff4bb35b), libs/mlx-swift (eedca444) and libs/mlx-swift-lm (d4fd516e: the provider 0.10.2 SDK with main's #358). libs/mlx-swift-lm conflicts with this branch's #359 fold (cad9d811); it now points at 5d3bf208, which merges d4fd516e into that fold, so the SDK is the release branch's d4fd516e plus #359.
Summary
Fixes provider-side weight hash verification memory growth during large model downloads.
The issue was in
WeightHasher:FileHandle.read(upToCount:)returns autoreleasedData, but the loop did not drain an autorelease pool per chunk. During aggregate hash verification, every model shard could be hashed concurrently, so a large manifest-backed model such asgemma-4-26bcould retain shard-scale data until the hash completed and get jetsam-killed after download progress reached 100%.Changes
Datainside the streamingFileHandleread loop.InputStreamstreaming fallback before the last-resortData(contentsOf:)fallback.Validation
swift test --filter ProviderCoreFoundationTestsPassed: 42 tests, 0 failures.
swift test --filter ProviderCoreTests.ModelPrefetchDownloaderTestsPassed: 11 tests, 0 failures.
TDD evidence: before the fix, the new memory regression failed with RSS growth of
404,176,896bytes; after the fix, the same test passed.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.