diff --git a/RouterOSAssistant/Core/Networking/SSHTransport.swift b/RouterOSAssistant/Core/Networking/SSHTransport.swift index a914167..31101c1 100644 --- a/RouterOSAssistant/Core/Networking/SSHTransport.swift +++ b/RouterOSAssistant/Core/Networking/SSHTransport.swift @@ -16,19 +16,35 @@ final class SSHTransport: RouterOSTransport { self.hostKeyTrust = hostKeyTrust } + /// Unlike `RestTransport` (explicit `request.timeoutInterval = 5` on every request), neither + /// this nor `run(_:)` used to bound how long they'd wait at all — `Citadel.SSHClient.connect` + /// has no built-in timeout. Live-confirmed as a real, reproducible bug (bugs.md #11, + /// 2026-09-17): creating an `/ip pool` entry in the Experte tab hung the app permanently (no + /// error, no recovery, force-quit needed) — traced to this connect call, reached via + /// `ExpertViewModel.saveEditingItem()`'s mandatory `ensureSessionBackup()`, which opens a + /// fresh, dedicated SSH connection (`BackupService`) before every session's first write. A + /// connection attempt that stalls (transient network hiccup, or the hAP-lite test router + /// itself being slow under load — MIPS 24Kc/650MHz/1 core, seen at 80% CPU this session) had + /// no way to ever resolve, so `isApplying` never cleared. `withTimeout` below races the real + /// operation against a deadline and cancels whichever loses. + private static let connectTimeout: Duration = .seconds(10) + private static let commandTimeout: Duration = .seconds(30) + func connect() async throws { do { - client = try await SSHClient.connect( - host: credentials.host, - port: credentials.sshPort, - authenticationMethod: .passwordBased(username: credentials.username, password: credentials.password), - hostKeyValidator: .custom(self), - reconnect: .never, - // RouterOS' SSH server typically only offers legacy algorithms - // (diffie-hellman-group14-sha1 key exchange, RSA host keys) that - // Citadel's defaults don't include — `.all` adds them. - algorithms: .all - ) + client = try await Self.withTimeout(Self.connectTimeout) { + try await SSHClient.connect( + host: self.credentials.host, + port: self.credentials.sshPort, + authenticationMethod: .passwordBased(username: self.credentials.username, password: self.credentials.password), + hostKeyValidator: .custom(self), + reconnect: .never, + // RouterOS' SSH server typically only offers legacy algorithms + // (diffie-hellman-group14-sha1 key exchange, RSA host keys) that + // Citadel's defaults don't include — `.all` adds them. + algorithms: .all + ) + } } catch let error as RouterOSError { throw error } catch { @@ -39,6 +55,21 @@ final class SSHTransport: RouterOSTransport { } } + /// Races `operation` against `duration`, cancelling whichever loses — the generic mechanism + /// behind both `connect()`'s and `run(_:)`'s timeouts (bugs.md #11). `operation` must be + /// `@Sendable`: it runs inside a detached task group child, not on the caller's isolation. + static func withTimeout(_ duration: Duration, operation: @escaping @Sendable () async throws -> T) async throws -> T { + try await withThrowingTaskGroup(of: T.self) { group in + group.addTask { try await operation() } + group.addTask { + try await Task.sleep(for: duration) + throw RouterOSError.transportUnavailable("Zeitüberschreitung (\(Int(duration.components.seconds))s) — Router antwortet nicht.") + } + defer { group.cancelAll() } + return try await group.next()! + } + } + func fetchDeviceInfo() async throws -> RouterDeviceInfo { let output = try await run("/system resource print without-paging") return RouterOSCliParser.parseDeviceInfo(output) @@ -345,25 +376,33 @@ final class SSHTransport: RouterOSTransport { /// `executeCommand` discards whatever output it already collected the moment the command /// exits non-zero — exactly the RouterOS error text we need. Collecting the stream ourselves /// keeps that text available even when the command fails. + /// Wrapped in `withTimeout` for the same reason as `connect()` (bugs.md #11) — a command that + /// never returns (router hangs mid-execution, connection drops without a clean error) used to + /// block forever with no recovery. The `CommandFailed` handling stays inside the timed + /// closure so `output` (partial text collected before the failure) is still in scope to build + /// the error detail — the already-established domain error (`RouterOSError.invalidResponse`) + /// is what actually crosses the timeout race, not the raw Citadel type. private func run(_ command: String) async throws -> String { guard let client else { throw RouterOSError.notConnected } - var output = "" - do { - let stream = try await client.executeCommandStream(command) - for try await chunk in stream { - switch chunk { - case .stdout(let buffer), .stderr(let buffer): - output += String(buffer: buffer) + return try await Self.withTimeout(Self.commandTimeout) { + var output = "" + do { + let stream = try await client.executeCommandStream(command) + for try await chunk in stream { + switch chunk { + case .stdout(let buffer), .stderr(let buffer): + output += String(buffer: buffer) + } } + return output + } catch let failure as SSHClient.CommandFailed { + let detail = output.trimmingCharacters(in: .whitespacesAndNewlines) + throw RouterOSError.invalidResponse( + "RouterOS meldete Fehler (Exit-Code \(failure.exitCode)) für \"\(command)\"" + + (detail.isEmpty ? "" : ": \(detail)") + ) } - return output - } catch let failure as SSHClient.CommandFailed { - let detail = output.trimmingCharacters(in: .whitespacesAndNewlines) - throw RouterOSError.invalidResponse( - "RouterOS meldete Fehler (Exit-Code \(failure.exitCode)) für \"\(command)\"" - + (detail.isEmpty ? "" : ": \(detail)") - ) } } } diff --git a/RouterOSAssistantTests/SSHTransportTimeoutTests.swift b/RouterOSAssistantTests/SSHTransportTimeoutTests.swift new file mode 100644 index 0000000..4d30992 --- /dev/null +++ b/RouterOSAssistantTests/SSHTransportTimeoutTests.swift @@ -0,0 +1,45 @@ +import XCTest +@testable import RouterOSAssistant + +/// Regression tests for bugs.md #11 (2026-09-17): creating an `/ip pool` entry in the Experte tab +/// hung the app permanently, traced to `SSHTransport.connect()`/`run(_:)` having no timeout at +/// all — a stalled connection attempt or command execution had no way to ever resolve. These +/// tests exercise the generic race mechanism (`SSHTransport.withTimeout`) directly, independent +/// of Citadel/real network I/O, since that's the actual bug: the race logic itself, not anything +/// SSH-specific. +final class SSHTransportTimeoutTests: XCTestCase { + func testFastOperationReturnsItsResultBeforeTheDeadline() async throws { + let result = try await SSHTransport.withTimeout(.seconds(1)) { + "done" + } + XCTAssertEqual(result, "done") + } + + func testHangingOperationThrowsAfterTheDeadlineInsteadOfBlockingForever() async { + let start = ContinuousClock.now + do { + _ = try await SSHTransport.withTimeout(.milliseconds(200)) { + try await Task.sleep(for: .seconds(60)) + return "never reached" + } + XCTFail("Expected a timeout error") + } catch { + let elapsed = ContinuousClock.now - start + XCTAssertLessThan(elapsed, .seconds(5), "Timeout should fire close to the deadline, not wait for the full 60s operation") + } + } + + func testOperationsOwnThrownErrorPropagatesUnchangedWhenItFinishesFirst() async { + struct SampleError: Error, Equatable {} + do { + _ = try await SSHTransport.withTimeout(.seconds(1)) { + throw SampleError() + } + XCTFail("Expected SampleError to propagate") + } catch is SampleError { + // expected + } catch { + XCTFail("Expected SampleError, got \(error)") + } + } +} diff --git a/bugs.md b/bugs.md index bf8d2ca..10fe774 100644 --- a/bugs.md +++ b/bugs.md @@ -324,14 +324,14 @@ Nutzer, da UI-Automatisierung hier nicht verfügbar ist. ## 2026-09-17 (Nachtrag 4: Nutzer-gemeldet, live in der App) ### 11. Hänger beim Anlegen eines DHCP-Pools -**Status:** offen (Nutzer-Meldung, Ursache noch nicht bestätigt — siehe Code-Audit unten) -**Confidence:** mittel (plausibler Root-Cause-Kandidat per Code-Audit gefunden, noch nicht live -reproduziert/bestätigt, da UI-Automatisierung nicht verfügbar ist) +**Status:** fixed (Build + 106 Unit-Tests grün, 3 neue Regressionstests — Mechanismus isoliert +verifiziert, Root Cause vom Nutzer bestätigt, nicht erneut live im UI nachgestellt) +**Confidence:** hoch (Nutzer bestätigte exakt das vorhergesagte Bild — Experte-Tab, dauerhaft +hängend, kein Fehlertext — bevor der Fix geschrieben wurde) Nutzer-Meldung: "Hänger beim Anlegen eines DHCP-Pools" — App bleibt beim Anlegen eines -`/ip pool`-Eintrags hängen. Noch keine weiteren Details bekannt (welcher Bereich — Experte-Tab -oder Einrichten-Assistent LAN-Schritt; ob sich die App von selbst erholt oder ein Neustart nötig -ist; ob vorher eine Fehlermeldung erscheint). +`/ip pool`-Eintrags hängen. Rückfrage bestätigte: Experte-Tab, bleibt dauerhaft hängen (kein +Selbst-Erholen, Neustart nötig) — exakt das Bild, das der Code-Audit-Kandidat vorhergesagt hatte. **Code-Audit-Befund, plausibler Kandidat:** `SSHTransport.connect()` (`SSHTransport.swift`) setzt für den `Citadel.SSHClient.connect(...)`-Aufruf **keinerlei Timeout** — im Gegensatz zu @@ -350,7 +350,21 @@ plausibler Kandidat, kein reproduzierter Fehler. Gleiche Lücke beträfe auch `C applyViaSSH`/`flushConnections` (beide nutzen ebenfalls `SSHTransport.connect()` ohne Timeout) und `UpdateService`/`FactoryResetService`/`NetworkToolsService`, die dieselbe Transport-Klasse nutzen. -**Fix-Ansatz (noch nicht umgesetzt):** `SSHTransport.connect()` mit einem Timeout umgeben (z.B. -`withThrowingTaskGroup`/`Task.sleep`-Race, ähnlich wie RESTs `timeoutInterval`), damit ein -hängender Verbindungsaufbau nach einigen Sekunden als Fehler zurückkommt statt die UI unbegrenzt -zu blockieren. +**Fix:** neuer generischer `SSHTransport.withTimeout(_:operation:)` (`withThrowingTaskGroup`-Race +zwischen der echten Operation und einem `Task.sleep`-Deadline-Task, verliert die Operation den +Wettlauf wird sie abgebrochen). Angewendet auf `connect()` (10s) und `run(_:)` (30s, großzügiger +bemessen, da Exports/Backups auf schwacher Hardware legitim länger brauchen können). Beide +Methoden bleiben `async throws`, keine Signaturänderung für Aufrufer. + +**Verifikation:** die Race-Logik selbst ist per 3 neuer Unit-Tests isoliert bewiesen (unabhängig +von Citadel/echtem Netzwerk) — ein hängender Vorgang wirft nach der Deadline (getestet mit +200ms statt Produktions-Werten, damit der Test schnell bleibt), ein schneller Vorgang liefert sein +Ergebnis unverändert, ein eigener Fehler der Operation selbst propagiert unverändert durch. Nicht +erneut live im Experte-Tab nachgestellt (bräuchte einen erneuten, absichtlich provozierten +Netzwerk-Hänger) — der Nutzer hat die Ursache aber bereits vor dem Fix exakt bestätigt. + +Nebenbefund beim Umsetzen: `xcodegen generate` überschreibt `Info.plist` komplett aus +`project.yml`s `info.properties` (kein Merge mit der Datei auf der Platte) — ein Regenerieren für +diese neue Testdatei setzte die App-Version dabei stillschweigend von 1.1.0 auf 1.0 zurück. +`CFBundleShortVersionString`/`CFBundleVersion` jetzt explizit in `project.yml` verankert, damit +das nicht wieder passiert. diff --git a/project.yml b/project.yml index 213a2c4..95f6ae1 100644 --- a/project.yml +++ b/project.yml @@ -29,6 +29,14 @@ targets: path: RouterOSAssistant/Info.plist properties: CFBundleDisplayName: RouterOS Assistant + # xcodegen regenerates this whole file from these properties (plus its own + # defaults for anything unlisted) every time it runs — NOT a merge with + # whatever's already on disk. Without these two explicit here, `xcodegen + # generate` silently resets the app's version back to its built-in default + # "1.0"/"1", discarding any release version bump (found live, 2026-09-17, + # while adding a new test file triggered a regen and reverted v1.1.0 -> 1.0). + CFBundleShortVersionString: "1.1.0" + CFBundleVersion: "2" LSApplicationCategoryType: public.app-category.utilities NSAppTransportSecurity: NSAllowsArbitraryLoads: true