From cc88fd70ca4c18e5d40e806b1521d6fcaec954a2 Mon Sep 17 00:00:00 2001 From: Hesham Salman Date: Wed, 7 Jan 2026 13:51:09 -0500 Subject: [PATCH] Add timeout and fix resource leaks in Git client The runGit() function had no timeout on pipe reads or process exit, which could cause hangs on large diffs. Pipe file handles were also never explicitly closed, potentially leaking file descriptors across multiple runs. Changes: - Add 60s timeout using DispatchGroup with wait(timeout:) - Add defer block to always close pipe file handles - Add gitCommandTimedOut error case for timeout failures - Use DataBox wrapper to avoid Swift concurrency warnings - Fix unrelated test checking for non-existent prompt text Fixes #17 --- .../SwiftCommitGen/Core/CommitGenError.swift | 3 ++ Sources/SwiftCommitGen/Core/GitClient.swift | 52 ++++++++++++++++--- .../BatchCombinationPromptBuilderTests.swift | 2 +- 3 files changed, 48 insertions(+), 9 deletions(-) diff --git a/Sources/SwiftCommitGen/Core/CommitGenError.swift b/Sources/SwiftCommitGen/Core/CommitGenError.swift index d3fdcfa..f735765 100644 --- a/Sources/SwiftCommitGen/Core/CommitGenError.swift +++ b/Sources/SwiftCommitGen/Core/CommitGenError.swift @@ -4,6 +4,7 @@ import Foundation enum CommitGenError: Error { case gitRepositoryUnavailable case gitCommandFailed(message: String) + case gitCommandTimedOut(command: String, timeout: TimeInterval) case cleanWorkingTree case modelUnavailable(reason: String) case modelTimedOut(timeout: TimeInterval) @@ -18,6 +19,8 @@ extension CommitGenError: LocalizedError { "Failed to locate a Git repository in the current directory hierarchy." case .gitCommandFailed(let message): message.isEmpty ? "Git command failed for an unknown reason." : message + case .gitCommandTimedOut(let command, let timeout): + "Git command '\(command)' did not complete within \(formatSeconds(timeout))." case .cleanWorkingTree: "No pending changes detected; nothing to summarize." case .modelUnavailable(let reason): diff --git a/Sources/SwiftCommitGen/Core/GitClient.swift b/Sources/SwiftCommitGen/Core/GitClient.swift index 1523218..69d7443 100644 --- a/Sources/SwiftCommitGen/Core/GitClient.swift +++ b/Sources/SwiftCommitGen/Core/GitClient.swift @@ -254,7 +254,10 @@ struct SystemGitClient: GitClient { return result } - private func runGit(_ arguments: [String]) throws -> String { + /// Default timeout for git operations in seconds. + private static let gitCommandTimeout: TimeInterval = 60 + + private func runGit(_ arguments: [String], timeout: TimeInterval = gitCommandTimeout) throws -> String { let process = Process() process.executableURL = URL(fileURLWithPath: "/usr/bin/env") process.arguments = ["git"] + arguments @@ -268,30 +271,63 @@ struct SystemGitClient: GitClient { environment["LC_ALL"] = "C" process.environment = environment + // Ensure file handles are always closed to prevent descriptor leaks + defer { + try? stdoutPipe.fileHandleForReading.close() + try? stderrPipe.fileHandleForReading.close() + } + do { try process.run() } catch { throw CommitGenError.gitCommandFailed(message: error.localizedDescription) } - // IMPORTANT: Read from pipes BEFORE waiting for exit to avoid deadlock. - // If the output exceeds the pipe buffer (~64KB), the process will block - // waiting to write more data, causing a deadlock if we wait for exit first. - let stdoutData = stdoutPipe.fileHandleForReading.readDataToEndOfFile() - let stderrData = stderrPipe.fileHandleForReading.readDataToEndOfFile() + // Read from pipes with timeout using DispatchGroup. + // IMPORTANT: Read BEFORE waiting for exit to avoid deadlock when output + // exceeds the pipe buffer (~64KB). + let readGroup = DispatchGroup() + let stdoutBox = DataBox() + let stderrBox = DataBox() + + readGroup.enter() + DispatchQueue.global(qos: .userInitiated).async { + stdoutBox.value = stdoutPipe.fileHandleForReading.readDataToEndOfFile() + readGroup.leave() + } + + readGroup.enter() + DispatchQueue.global(qos: .userInitiated).async { + stderrBox.value = stderrPipe.fileHandleForReading.readDataToEndOfFile() + readGroup.leave() + } + + let waitResult = readGroup.wait(timeout: .now() + timeout) + + if waitResult == .timedOut { + process.terminate() + let command = arguments.first ?? "unknown" + throw CommitGenError.gitCommandTimedOut(command: command, timeout: timeout) + } process.waitUntilExit() guard process.terminationStatus == 0 else { - let stderr = String(data: stderrData, encoding: .utf8) ?? "" + let stderr = String(data: stderrBox.value ?? Data(), encoding: .utf8) ?? "" let message = stderr.trimmingCharacters(in: .whitespacesAndNewlines) throw CommitGenError.gitCommandFailed(message: message) } - return String(data: stdoutData, encoding: .utf8) ?? "" + return String(data: stdoutBox.value ?? Data(), encoding: .utf8) ?? "" } } +/// Thread-safe container for passing data across dispatch queues. +/// Allows concurrent read/write without Swift concurrency warnings. +private final class DataBox: @unchecked Sendable { + var value: Data? +} + struct GitStatusParser { static func parse(_ raw: String) -> GitStatus { var staged: [GitFileChange] = [] diff --git a/Tests/SwiftCommitGenTests/BatchCombinationPromptBuilderTests.swift b/Tests/SwiftCommitGenTests/BatchCombinationPromptBuilderTests.swift index 69159ca..3b83187 100644 --- a/Tests/SwiftCommitGenTests/BatchCombinationPromptBuilderTests.swift +++ b/Tests/SwiftCommitGenTests/BatchCombinationPromptBuilderTests.swift @@ -93,7 +93,7 @@ struct BatchCombinationPromptBuilderTests { #expect(package.diagnostics.estimatedLineCount >= 14) let instructions = String(describing: package.systemPrompt) - #expect(instructions.contains("You are an AI assistant")) + #expect(instructions.contains("Combine these partial commit drafts")) #expect(instructions.contains("Style: use the body for a few short sentences")) }