Fix Haenger beim Anlegen eines DHCP-Pools (bugs.md #11)
SSHTransport.connect()/run() hatten keinen Timeout - im Gegensatz zu RestTransport (timeoutInterval=5). Jeder erste Schreibvorgang einer Session loest ueber ensureSessionBackup() eine dedizierte SSH- Verbindung aus; haengt die, blieb isApplying unbegrenzt aktiv (kein Fehler, kein Recovery, nur Force-Quit). Live vom Nutzer bestaetigt (Experte-Tab, dauerhaft haengend) bevor der Fix geschrieben wurde. Neuer genererischer SSHTransport.withTimeout(_:operation:) (Task- Group-Race gegen eine Deadline), angewendet auf connect() (10s) und run() (30s). 3 neue Regressionstests fuer die Race-Logik isoliert. Nebenbefund: xcodegen generate ueberschreibt Info.plist komplett aus project.yml (kein Merge) - ein Regenerieren fuer die neue Testdatei setzte die Version stillschweigend von 1.1.0 auf 1.0 zurueck. Version jetzt explizit in project.yml verankert. Build + alle 106 Unit-Tests gruen. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -16,19 +16,35 @@ final class SSHTransport: RouterOSTransport {
|
|||||||
self.hostKeyTrust = hostKeyTrust
|
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 {
|
func connect() async throws {
|
||||||
do {
|
do {
|
||||||
client = try await SSHClient.connect(
|
client = try await Self.withTimeout(Self.connectTimeout) {
|
||||||
host: credentials.host,
|
try await SSHClient.connect(
|
||||||
port: credentials.sshPort,
|
host: self.credentials.host,
|
||||||
authenticationMethod: .passwordBased(username: credentials.username, password: credentials.password),
|
port: self.credentials.sshPort,
|
||||||
hostKeyValidator: .custom(self),
|
authenticationMethod: .passwordBased(username: self.credentials.username, password: self.credentials.password),
|
||||||
reconnect: .never,
|
hostKeyValidator: .custom(self),
|
||||||
// RouterOS' SSH server typically only offers legacy algorithms
|
reconnect: .never,
|
||||||
// (diffie-hellman-group14-sha1 key exchange, RSA host keys) that
|
// RouterOS' SSH server typically only offers legacy algorithms
|
||||||
// Citadel's defaults don't include — `.all` adds them.
|
// (diffie-hellman-group14-sha1 key exchange, RSA host keys) that
|
||||||
algorithms: .all
|
// Citadel's defaults don't include — `.all` adds them.
|
||||||
)
|
algorithms: .all
|
||||||
|
)
|
||||||
|
}
|
||||||
} catch let error as RouterOSError {
|
} catch let error as RouterOSError {
|
||||||
throw error
|
throw error
|
||||||
} catch {
|
} 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<T: Sendable>(_ 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 {
|
func fetchDeviceInfo() async throws -> RouterDeviceInfo {
|
||||||
let output = try await run("/system resource print without-paging")
|
let output = try await run("/system resource print without-paging")
|
||||||
return RouterOSCliParser.parseDeviceInfo(output)
|
return RouterOSCliParser.parseDeviceInfo(output)
|
||||||
@@ -345,25 +376,33 @@ final class SSHTransport: RouterOSTransport {
|
|||||||
/// `executeCommand` discards whatever output it already collected the moment the command
|
/// `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
|
/// exits non-zero — exactly the RouterOS error text we need. Collecting the stream ourselves
|
||||||
/// keeps that text available even when the command fails.
|
/// 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 {
|
private func run(_ command: String) async throws -> String {
|
||||||
guard let client else { throw RouterOSError.notConnected }
|
guard let client else { throw RouterOSError.notConnected }
|
||||||
|
|
||||||
var output = ""
|
return try await Self.withTimeout(Self.commandTimeout) {
|
||||||
do {
|
var output = ""
|
||||||
let stream = try await client.executeCommandStream(command)
|
do {
|
||||||
for try await chunk in stream {
|
let stream = try await client.executeCommandStream(command)
|
||||||
switch chunk {
|
for try await chunk in stream {
|
||||||
case .stdout(let buffer), .stderr(let buffer):
|
switch chunk {
|
||||||
output += String(buffer: buffer)
|
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)")
|
|
||||||
)
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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)")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -324,14 +324,14 @@ Nutzer, da UI-Automatisierung hier nicht verfügbar ist.
|
|||||||
## 2026-09-17 (Nachtrag 4: Nutzer-gemeldet, live in der App)
|
## 2026-09-17 (Nachtrag 4: Nutzer-gemeldet, live in der App)
|
||||||
|
|
||||||
### 11. Hänger beim Anlegen eines DHCP-Pools
|
### 11. Hänger beim Anlegen eines DHCP-Pools
|
||||||
**Status:** offen (Nutzer-Meldung, Ursache noch nicht bestätigt — siehe Code-Audit unten)
|
**Status:** fixed (Build + 106 Unit-Tests grün, 3 neue Regressionstests — Mechanismus isoliert
|
||||||
**Confidence:** mittel (plausibler Root-Cause-Kandidat per Code-Audit gefunden, noch nicht live
|
verifiziert, Root Cause vom Nutzer bestätigt, nicht erneut live im UI nachgestellt)
|
||||||
reproduziert/bestätigt, da UI-Automatisierung nicht verfügbar ist)
|
**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
|
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
|
`/ip pool`-Eintrags hängen. Rückfrage bestätigte: Experte-Tab, bleibt dauerhaft hängen (kein
|
||||||
oder Einrichten-Assistent LAN-Schritt; ob sich die App von selbst erholt oder ein Neustart nötig
|
Selbst-Erholen, Neustart nötig) — exakt das Bild, das der Code-Audit-Kandidat vorhergesagt hatte.
|
||||||
ist; ob vorher eine Fehlermeldung erscheint).
|
|
||||||
|
|
||||||
**Code-Audit-Befund, plausibler Kandidat:** `SSHTransport.connect()` (`SSHTransport.swift`) setzt
|
**Code-Audit-Befund, plausibler Kandidat:** `SSHTransport.connect()` (`SSHTransport.swift`) setzt
|
||||||
für den `Citadel.SSHClient.connect(...)`-Aufruf **keinerlei Timeout** — im Gegensatz zu
|
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
|
applyViaSSH`/`flushConnections` (beide nutzen ebenfalls `SSHTransport.connect()` ohne Timeout) und
|
||||||
`UpdateService`/`FactoryResetService`/`NetworkToolsService`, die dieselbe Transport-Klasse nutzen.
|
`UpdateService`/`FactoryResetService`/`NetworkToolsService`, die dieselbe Transport-Klasse nutzen.
|
||||||
|
|
||||||
**Fix-Ansatz (noch nicht umgesetzt):** `SSHTransport.connect()` mit einem Timeout umgeben (z.B.
|
**Fix:** neuer generischer `SSHTransport.withTimeout(_:operation:)` (`withThrowingTaskGroup`-Race
|
||||||
`withThrowingTaskGroup`/`Task.sleep`-Race, ähnlich wie RESTs `timeoutInterval`), damit ein
|
zwischen der echten Operation und einem `Task.sleep`-Deadline-Task, verliert die Operation den
|
||||||
hängender Verbindungsaufbau nach einigen Sekunden als Fehler zurückkommt statt die UI unbegrenzt
|
Wettlauf wird sie abgebrochen). Angewendet auf `connect()` (10s) und `run(_:)` (30s, großzügiger
|
||||||
zu blockieren.
|
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.
|
||||||
|
|||||||
@@ -29,6 +29,14 @@ targets:
|
|||||||
path: RouterOSAssistant/Info.plist
|
path: RouterOSAssistant/Info.plist
|
||||||
properties:
|
properties:
|
||||||
CFBundleDisplayName: RouterOS Assistant
|
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
|
LSApplicationCategoryType: public.app-category.utilities
|
||||||
NSAppTransportSecurity:
|
NSAppTransportSecurity:
|
||||||
NSAllowsArbitraryLoads: true
|
NSAllowsArbitraryLoads: true
|
||||||
|
|||||||
Reference in New Issue
Block a user