fix: replace Zip dependency with native ZipService#526
Conversation
This comment has been minimized.
This comment has been minimized.
| ## [2.2.0] - 2026-04-07 | ||
|
|
||
| ### Fixed | ||
| - Replace third-party Zip dependency with native ZipService using NSFileCoordinator #526 |
There was a problem hiding this comment.
The changelog entry was added under ## [2.2.0] - 2026-04-07 (a released version section) instead of ## [Unreleased].
Per CLAUDE.md:
ALWAYS add exactly ONE entry per PR under
## [Unreleased]
NEVER modify released version sections — only edit## [Unreleased]
Move this entry into the ### Fixed category under ## [Unreleased] and remove it from the [2.2.0] section.
| try FileManager.default.createDirectory(at: directoryToZipURL, withIntermediateDirectories: true, attributes: [:]) | ||
| var usedFilenames = Set<String>() | ||
| for fileToZip in filesToZip { | ||
| try fileToZip.prepareInDirectory(directoryURL: directoryToZipURL, usedFilenames: &usedFilenames) | ||
| } | ||
| defer { | ||
| try? FileManager.default.removeItem(at: directoryToZipURL) | ||
| } |
There was a problem hiding this comment.
The defer cleanup is placed after the for loop that can throw. In Swift, defer only registers when execution reaches that statement — if prepareInDirectory throws on any iteration, the defer never runs and directoryToZipURL leaks.
See context at
bitkit-ios/Bitkit/Services/ZipService.swift
Lines 142 to 152 in 474453e
Move the defer to immediately after createDirectory succeeds so it always runs:
| try FileManager.default.createDirectory(at: directoryToZipURL, withIntermediateDirectories: true, attributes: [:]) | |
| var usedFilenames = Set<String>() | |
| for fileToZip in filesToZip { | |
| try fileToZip.prepareInDirectory(directoryURL: directoryToZipURL, usedFilenames: &usedFilenames) | |
| } | |
| defer { | |
| try? FileManager.default.removeItem(at: directoryToZipURL) | |
| } | |
| try FileManager.default.createDirectory(at: directoryToZipURL, withIntermediateDirectories: true, attributes: [:]) | |
| defer { | |
| try? FileManager.default.removeItem(at: directoryToZipURL) | |
| } | |
| var usedFilenames = Set<String>() | |
| for fileToZip in filesToZip { | |
| try fileToZip.prepareInDirectory(directoryURL: directoryToZipURL, usedFilenames: &usedFilenames) | |
| } |
| let directoryToZipURL = FileManager.default.temporaryDirectory | ||
| .appending(path: UUID().uuidString) | ||
| .appending(path: zipFilename) |
There was a problem hiding this comment.
The staging directory is constructed two levels deep: tmp/<UUID>/<zipFilename>/. The defer cleanup calls removeItem(at: directoryToZipURL) which deletes only the inner <zipFilename> directory — the <UUID> parent is left behind as an empty orphan in tmp/ on every call.
Fix: capture the UUID directory URL separately and remove it in the defer instead:
let uuidDirURL = FileManager.default.temporaryDirectory.appending(path: UUID().uuidString)
let directoryToZipURL = uuidDirURL.appending(path: zipFilename)
// ...
defer { try? FileManager.default.removeItem(at: uuidDirURL) }| extension FileToZip { | ||
| static func text(_ text: String, filename: String) -> FileToZip { | ||
| .data(text.data(using: .utf8) ?? Data(), filename: filename) | ||
| } | ||
| } |
There was a problem hiding this comment.
FileToZip.text(_:filename:) is not called anywhere in this PR (not in LogService.swift or ZipServiceTests.swift).\n\nPer CLAUDE.md:\n> Don't add features, refactor, or introduce abstractions beyond what the task requires.\n\nRemove the unused factory.
Description
Switch log zipping to an internal NSFileCoordinator-based service, add safer filename/overwrite/temp-file handling, and cover key zip edge cases with tests.
QA Notes
Zipping logs works as before with same compression level.