Skip to content

fix(apple): ensure log file exists before writing to it - #9597

Merged
jamilbk merged 3 commits into
mainfrom
fix/apple-log-write
Jun 20, 2025
Merged

fix(apple): ensure log file exists before writing to it#9597
jamilbk merged 3 commits into
mainfrom
fix/apple-log-write

Conversation

@jamilbk

@jamilbk jamilbk commented Jun 20, 2025

Copy link
Copy Markdown
Member

Similar to the issue for the gui clients, the log file handle needs to be able to be rolled over after logs are cleared.

related: #6850

Copilot AI review requested due to automatic review settings June 20, 2025 14:24
@vercel

vercel Bot commented Jun 20, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 20, 2025 3:18pm

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures that the log file exists before writing to it, thereby supporting proper log rollover after logs are cleared. Additionally, it reorders several import statements for consistency.

  • Added logic to check and recreate the log file if it’s missing.
  • Reordered SwiftUI (and related) imports in multiple files.
  • Updated file system handling in Log.swift to support log rotation.

Reviewed Changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift Reordered SwiftUI import to maintain consistency.
swift/apple/FirezoneKit/Sources/FirezoneKit/Views/GrantVPNView.swift Reordered SwiftUI import to maintain consistency.
swift/apple/FirezoneKit/Sources/FirezoneKit/Models/SessionNotification.swift Reordered UserNotifications import with no functional change.
swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/Log.swift Introduced file existence checks and log file recreation logic to support log rollover.
swift/apple/Firezone/Application/FirezoneApp.swift Reordered SwiftUI import for consistency.

}

// File was deleted, need to recreate
try? handle.close()

Copilot AI Jun 20, 2025

Copy link

Choose a reason for hiding this comment

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

Consider logging the error from closing the file handle instead of silently ignoring it with 'try?'. Explicit error handling can help diagnose potential issues during log file recreation.

Suggested change
try? handle.close()
do {
try handle.close()
} catch {
logger.error("Could not close logfile during recreation: \(error)")
}

Copilot uses AI. Check for mistakes.
@jamilbk

jamilbk commented Jun 20, 2025

Copy link
Copy Markdown
Member Author

Tested on live device(s).

import OSLog
import SwiftUI
import Sentry
import SwiftUI

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Where are these reformattings coming from?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

swift-format must have decided it wants to do things this way now. Probably a swift minor update.

Comment thread swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/Log.swift Outdated
@jamilbk
jamilbk added this pull request to the merge queue Jun 20, 2025
Merged via the queue into main with commit 5537b8c Jun 20, 2025
@jamilbk
jamilbk deleted the fix/apple-log-write branch June 20, 2025 17:14
if self.ensureFileExists() {
self.handle.write(jsonData)
// Get valid handle, recreating file if necessary
if let handle = self.ensureFileExists() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think a guard would be very clean here.

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.

3 participants