Verbindung: Zeitlimit je Zugriffsart, binaere API strikt nacheinander, Trap/Done-Fix (#40, #41)

- RouterOsTimeouts: Lesen 15 s, Schreiben 30 s, Befehle (Backup/Restore/Update) 120 s, Verbindungstest 8 s; vorher global 8 s (Fehlalarme auf schwacher Hardware, Mac v1.3/v1.6) - BinaryRouterOsClient: Zugriffe per SemaphoreSlim serialisiert, nach !trap wird das folgende !done mitgelesen (sonst las der naechste Befehl eine fremde Antwort) - REST-Fehler nennen RouterOS' 'detail' - IRouterOsClient.RunAsync liefert die Ausgabe eines Befehls (Basis fuer Ping/Traceroute) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-16 06:21:40 +02:00
1 parent 49a73ceb2b
commit bd65ffbc3c
@@ -0,0 +1,66 @@
+using MikrotikInstaller.Core.Connectivity;
+using MikrotikInstaller.Core.Connectivity.Binary;
+
+namespace MikrotikInstaller.Core.Tests.Connectivity;
+
+public class BinaryRouterOsClientTests
+{
+    [Fact]
+    public async Task Trap_ConsumesTrailingDone_SoNextCommandReadsItsOwnReply()
+    {
+        var replies = await EncodeAsync(
+            ["!trap", "=message=failure: already have such entry"],
+            ["!done"],
+            ["!done", "=ret=*5"]);
+        await using var client = new BinaryRouterOsClient(new ReplayStream(replies));
+
+        var error = await Assert.ThrowsAsync<RouterOsException>(() =>
+            client.AddAsync("/ip/pool", new Dictionary<string, string> { ["name"] = "x" }));
+        Assert.Contains("already have such entry", error.Message);
+
+        var id = await client.AddAsync("/ip/pool", new Dictionary<string, string> { ["name"] = "y" });
+        Assert.Equal("*5", id);
+    }
+
+    [Fact]
+    public async Task RunAsync_ReturnsRowsOfCommand()
+    {
+        var replies = await EncodeAsync(
+            ["!re", "=host=1.1.1.1", "=time=12ms"],
+            ["!re", "=host=1.1.1.1", "=time=11ms"],
+            ["!done"]);
+        await using var client = new BinaryRouterOsClient(new ReplayStream(replies));
+
+        var rows = await client.RunAsync("/ping", new Dictionary<string, string> { ["address"] = "1.1.1.1", ["count"] = "2" });
+
+        Assert.Equal(["12ms", "11ms"], rows.Select(r => r["time"]));
+    }
+
+    private static async Task<byte[]> EncodeAsync(params string[][] sentences)
+    {
+        await using var buffer = new MemoryStream();
+        foreach (var sentence in sentences)
+        {
+            await RouterOsWireFormat.WriteSentenceAsync(buffer, sentence, CancellationToken.None);
+        }
+
+        return buffer.ToArray();
+    }
+
+    /// <summary>Liest vorbereitete Antworten, verwirft alles Geschriebene.</summary>
+    private sealed class ReplayStream(byte[] replies) : Stream
+    {
+        private readonly MemoryStream _replies = new(replies);
+
+        public override bool CanRead => true;
+        public override bool CanSeek => false;
+        public override bool CanWrite => true;
+        public override long Length => throw new NotSupportedException();
+        public override long Position { get => throw new NotSupportedException(); set => throw new NotSupportedException(); }
+        public override void Flush() { }
+        public override int Read(byte[] buffer, int offset, int count) => _replies.Read(buffer, offset, count);
+        public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException();
+        public override void SetLength(long value) => throw new NotSupportedException();
+        public override void Write(byte[] buffer, int offset, int count) { }
+    }
+}
@@ -12,12 +12,23 @@ namespace MikrotikInstaller.Core.Connectivity.Binary;
 /// </summary>
 public sealed class BinaryRouterOsClient : IRouterOsClient
 {
-    private readonly TcpClient _tcpClient;
+    private readonly TcpClient? _tcpClient;
    private readonly Stream _stream;
+    private readonly SemaphoreSlim _gate = new(1, 1);
+    private bool _isBroken;
+
+    /// <summary>Nach Zeitüberschreitung oder Abbruch mitten in einer Antwort ist diese Verbindung nicht mehr nutzbar.</summary>
+    public bool IsBroken => _isBroken;

    public RouterOsProtocol Protocol => RouterOsProtocol.Binary;

-    private BinaryRouterOsClient(TcpClient tcpClient, Stream stream)
+    /// <summary>Für Tests: Client über einen bereits geöffneten Datenstrom, ohne Login.</summary>
+    internal BinaryRouterOsClient(Stream stream)
+        : this(null, stream)
+    {
+    }
+
+    private BinaryRouterOsClient(TcpClient? tcpClient, Stream stream)
    {
        _tcpClient = tcpClient;
        _stream = stream;
@@ -89,77 +100,51 @@ public sealed class BinaryRouterOsClient : IRouterOsClient
            ],
            cancellationToken);

-        var reply = await ReadReplyAsync(cancellationToken);
+        using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
+        timeoutCts.CancelAfter(RouterOsTimeouts.ConnectProbe);
+        RouterOsReply reply;
+        try
+        {
+            reply = await ReadReplyAsync(timeoutCts.Token);
+        }
+        catch (Exception ex) when (ex is OperationCanceledException or IOException)
+        {
+            throw new RouterOsException("Das Gerät hat auf die Anmeldung nicht geantwortet.", ex);
+        }
+
        if (reply.Status == RouterOsReplyStatus.Trap)
        {
            throw new RouterOsException("Benutzername oder Passwort ist falsch.");
        }
    }

-    public async Task<IReadOnlyList<IReadOnlyDictionary<string, string>>> GetAsync(string path, CancellationToken cancellationToken = default)
-    {
-        await RouterOsWireFormat.WriteSentenceAsync(_stream, [NormalizeCommand(path, "print")], cancellationToken);
-
-        var rows = new List<IReadOnlyDictionary<string, string>>();
-        while (true)
-        {
-            var reply = await ReadReplyAsync(cancellationToken);
-            switch (reply.Status)
-            {
-                case RouterOsReplyStatus.Row:
-                    rows.Add(reply.Attributes);
-                    break;
-                case RouterOsReplyStatus.Done:
-                    return rows;
-                case RouterOsReplyStatus.Trap:
-                    throw new RouterOsException(DescribeError(reply));
-            }
-        }
-    }
+    public Task<IReadOnlyList<IReadOnlyDictionary<string, string>>> GetAsync(string path, CancellationToken cancellationToken = default) =>
+        SendAsync([NormalizeCommand(path, "print")], RouterOsTimeouts.Read, cancellationToken);

    public async Task<string> AddAsync(string path, IReadOnlyDictionary<string, string> parameters, CancellationToken cancellationToken = default)
    {
        var words = new List<string> { NormalizeCommand(path, "add") };
        words.AddRange(parameters.Select(kv => $"={kv.Key}={kv.Value}"));

-        await RouterOsWireFormat.WriteSentenceAsync(_stream, words, cancellationToken);
-        var reply = await ReadReplyAsync(cancellationToken);
-        if (reply.Status == RouterOsReplyStatus.Trap)
-        {
-            throw new RouterOsException(DescribeError(reply));
-        }
-
-        return reply.Attributes.GetValueOrDefault("ret", string.Empty);
+        var rows = await SendAsync(words, RouterOsTimeouts.Write, cancellationToken);
+        return rows.Count > 0 ? rows[^1].GetValueOrDefault("ret", string.Empty) : string.Empty;
    }

    public async Task SetAsync(string path, string id, IReadOnlyDictionary<string, string> parameters, CancellationToken cancellationToken = default)
    {
        var words = new List<string> { NormalizeCommand(path, "set"), $"=.id={id}" };
        words.AddRange(parameters.Select(kv => $"={kv.Key}={kv.Value}"));
-
-        await RouterOsWireFormat.WriteSentenceAsync(_stream, words, cancellationToken);
-        var reply = await ReadReplyAsync(cancellationToken);
-        if (reply.Status == RouterOsReplyStatus.Trap)
-        {
-            throw new RouterOsException(DescribeError(reply));
-        }
+        await SendAsync(words, RouterOsTimeouts.Write, cancellationToken);
    }

-    public async Task RemoveAsync(string path, string id, CancellationToken cancellationToken = default)
-    {
-        await RouterOsWireFormat.WriteSentenceAsync(
-            _stream,
-            [NormalizeCommand(path, "remove"), $"=.id={id}"],
-            cancellationToken);
+    public async Task RemoveAsync(string path, string id, CancellationToken cancellationToken = default) =>
+        await SendAsync([NormalizeCommand(path, "remove"), $"=.id={id}"], RouterOsTimeouts.Write, cancellationToken);

-        var reply = await ReadReplyAsync(cancellationToken);
-        if (reply.Status == RouterOsReplyStatus.Trap)
-        {
-            throw new RouterOsException(DescribeError(reply));
-        }
-    }
+    public async Task ExecuteAsync(string path, IReadOnlyDictionary<string, string>? parameters = null, CancellationToken cancellationToken = default) =>
+        await RunAsync(path, parameters, cancellationToken);

-    public async Task ExecuteAsync(string path, IReadOnlyDictionary<string, string>? parameters = null, CancellationToken cancellationToken = default)
+    public Task<IReadOnlyList<IReadOnlyDictionary<string, string>>> RunAsync(
+        string path, IReadOnlyDictionary<string, string>? parameters = null, CancellationToken cancellationToken = default)
    {
        var words = new List<string> { path.StartsWith('/') ? path : "/" + path };
        if (parameters is not null)
@@ -167,21 +152,75 @@ public sealed class BinaryRouterOsClient : IRouterOsClient
            words.AddRange(parameters.Select(kv => $"={kv.Key}={kv.Value}"));
        }

-        await RouterOsWireFormat.WriteSentenceAsync(_stream, words, cancellationToken);
+        return SendAsync(words, RouterOsTimeouts.Command, cancellationToken);
+    }

-        while (true)
+    /// <summary>
+    /// Schickt einen Satz und liest bis einschließlich "!done" — auch nach einem "!trap", denn RouterOS
+    /// schickt danach noch ein "!done", das sonst als Antwort des nächsten Befehls gelesen würde.
+    /// Zugriffe laufen strikt nacheinander: die binäre API ordnet Antworten nur über die Reihenfolge
+    /// zu, ein paralleler Zugriff (z. B. Update-Prüfung im Hintergrund) würde sonst fremde Antworten lesen.
+    /// "!done" mit Attributen (z. B. "ret" bei "add") wird als letzte Zeile mitgeliefert.
+    /// </summary>
+    private async Task<IReadOnlyList<IReadOnlyDictionary<string, string>>> SendAsync(
+        IReadOnlyList<string> words, TimeSpan timeout, CancellationToken cancellationToken)
+    {
+        using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
+        timeoutCts.CancelAfter(timeout);
+
+        await _gate.WaitAsync(cancellationToken);
+        try
        {
-            var reply = await ReadReplyAsync(cancellationToken);
-            if (reply.Status == RouterOsReplyStatus.Trap)
-            {
-                throw new RouterOsException(DescribeError(reply));
-            }
+            await RouterOsWireFormat.WriteSentenceAsync(_stream, words, timeoutCts.Token);

-            if (reply.Status == RouterOsReplyStatus.Done)
+            var rows = new List<IReadOnlyDictionary<string, string>>();
+            string? error = null;
+            while (true)
            {
-                return;
+                var reply = await ReadReplyAsync(timeoutCts.Token);
+                switch (reply.Status)
+                {
+                    case RouterOsReplyStatus.Row:
+                        rows.Add(reply.Attributes);
+                        break;
+                    case RouterOsReplyStatus.Trap:
+                        error ??= DescribeError(reply);
+                        if (reply.IsFatal)
+                        {
+                            throw new RouterOsException(error);
+                        }
+
+                        break;
+                    case RouterOsReplyStatus.Done:
+                        if (error is not null)
+                        {
+                            throw new RouterOsException(error);
+                        }
+
+                        if (reply.Attributes.Count > 0)
+                        {
+                            rows.Add(reply.Attributes);
+                        }
+
+                        return rows;
+                }
            }
        }
+        catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested)
+        {
+            // Nach einem Zeitlimit mitten in einer Antwort ist die Reihenfolge nicht mehr verlässlich.
+            _isBroken = true;
+            throw new RouterOsException("Das Gerät hat nicht rechtzeitig geantwortet.");
+        }
+        catch (IOException ex)
+        {
+            _isBroken = true;
+            throw new RouterOsException("Die Verbindung zum Gerät wurde unterbrochen.", ex);
+        }
+        finally
+        {
+            _gate.Release();
+        }
    }

    private static string NormalizeCommand(string path, string action)
@@ -224,7 +263,7 @@ public sealed class BinaryRouterOsClient : IRouterOsClient
            _ => RouterOsReplyStatus.Done,
        };

-        return new RouterOsReply(status, attributes);
+        return new RouterOsReply(status, attributes) { IsFatal = replyWord == "!fatal" };
    }

    private static string DescribeError(RouterOsReply reply) =>
@@ -235,7 +274,7 @@ public sealed class BinaryRouterOsClient : IRouterOsClient
    public ValueTask DisposeAsync()
    {
        _stream.Dispose();
-        _tcpClient.Dispose();
+        _tcpClient?.Dispose();
        return ValueTask.CompletedTask;
    }
 }
@@ -12,4 +12,8 @@ internal enum RouterOsReplyStatus
    Trap,
 }

-internal sealed record RouterOsReply(RouterOsReplyStatus Status, IReadOnlyDictionary<string, string> Attributes);
+internal sealed record RouterOsReply(RouterOsReplyStatus Status, IReadOnlyDictionary<string, string> Attributes)
+{
+    /// <summary>"!fatal": RouterOS schließt die Verbindung danach, es folgt kein "!done" mehr.</summary>
+    public bool IsFatal { get; init; }
+}
@@ -23,4 +23,15 @@ public interface IRouterOsClient : IAsyncDisposable

    /// <summary>Führt einen eigenständigen Befehl aus, der keinem Listen-Eintrag entspricht (z. B. "/system/backup/save").</summary>
    Task ExecuteAsync(string path, IReadOnlyDictionary<string, string>? parameters = null, CancellationToken cancellationToken = default);
+
+    /// <summary>
+    /// Wie <see cref="ExecuteAsync"/>, liefert aber die Ausgabezeilen des Befehls zurück (z. B. "/ping" mit
+    /// "count", "/tool/traceroute", "/execute"). Standard für Test-Doubles: ausführen, keine Ausgabe.
+    /// </summary>
+    async Task<IReadOnlyList<IReadOnlyDictionary<string, string>>> RunAsync(
+        string path, IReadOnlyDictionary<string, string>? parameters = null, CancellationToken cancellationToken = default)
+    {
+        await ExecuteAsync(path, parameters, cancellationToken);
+        return [];
+    }
 }
@@ -37,7 +37,8 @@ public sealed class RestRouterOsClient : IRouterOsClient
        var httpClient = new HttpClient(handler)
        {
            BaseAddress = new Uri($"{scheme}://{credentials.Host}:{port}/rest/"),
-            Timeout = TimeSpan.FromSeconds(8),
+            // Zeitlimit je Aufruf in SendAsync (siehe RouterOsTimeouts), nicht global.
+            Timeout = Timeout.InfiniteTimeSpan,
        };
        httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue(
            "Basic",
@@ -47,7 +48,7 @@ public sealed class RestRouterOsClient : IRouterOsClient
        try
        {
            // Dient gleichzeitig als Erreichbarkeits- und Login-Test.
-            await client.GetAsync("/system/resource", cancellationToken);
+            using var probe = await client.SendAsync(HttpMethod.Get, "/system/resource", null, RouterOsTimeouts.ConnectProbe, cancellationToken);
        }
        catch (RouterOsException)
        {
@@ -65,9 +66,19 @@ public sealed class RestRouterOsClient : IRouterOsClient

    public async Task<IReadOnlyList<IReadOnlyDictionary<string, string>>> GetAsync(string path, CancellationToken cancellationToken = default)
    {
-        using var response = await SendAsync(HttpMethod.Get, path, body: null, cancellationToken);
-        await using var stream = await response.Content.ReadAsStreamAsync(cancellationToken);
-        using var document = await JsonDocument.ParseAsync(stream, cancellationToken: cancellationToken);
+        using var response = await SendAsync(HttpMethod.Get, path, body: null, RouterOsTimeouts.Read, cancellationToken);
+        return await ReadRowsAsync(response, cancellationToken);
+    }
+
+    private static async Task<IReadOnlyList<IReadOnlyDictionary<string, string>>> ReadRowsAsync(HttpResponseMessage response, CancellationToken cancellationToken)
+    {
+        var content = await response.Content.ReadAsStringAsync(cancellationToken);
+        if (string.IsNullOrWhiteSpace(content))
+        {
+            return [];
+        }
+
+        using var document = JsonDocument.Parse(content);

        var results = new List<IReadOnlyDictionary<string, string>>();
        switch (document.RootElement.ValueKind)
@@ -89,7 +100,7 @@ public sealed class RestRouterOsClient : IRouterOsClient

    public async Task<string> AddAsync(string path, IReadOnlyDictionary<string, string> parameters, CancellationToken cancellationToken = default)
    {
-        using var response = await SendAsync(HttpMethod.Put, path, parameters, cancellationToken);
+        using var response = await SendAsync(HttpMethod.Put, path, parameters, RouterOsTimeouts.Write, cancellationToken);
        await using var stream = await response.Content.ReadAsStreamAsync(cancellationToken);
        using var document = await JsonDocument.ParseAsync(stream, cancellationToken: cancellationToken);
        return document.RootElement.TryGetProperty(".id", out var id) ? id.GetString() ?? string.Empty : string.Empty;
@@ -97,22 +108,31 @@ public sealed class RestRouterOsClient : IRouterOsClient

    public async Task SetAsync(string path, string id, IReadOnlyDictionary<string, string> parameters, CancellationToken cancellationToken = default)
    {
-        using var response = await SendAsync(new HttpMethod("PATCH"), $"{path.TrimEnd('/')}/{id}", parameters, cancellationToken);
+        using var response = await SendAsync(new HttpMethod("PATCH"), $"{path.TrimEnd('/')}/{id}", parameters, RouterOsTimeouts.Write, cancellationToken);
    }

    public async Task RemoveAsync(string path, string id, CancellationToken cancellationToken = default)
    {
-        using var response = await SendAsync(HttpMethod.Delete, $"{path.TrimEnd('/')}/{id}", body: null, cancellationToken);
+        using var response = await SendAsync(HttpMethod.Delete, $"{path.TrimEnd('/')}/{id}", body: null, RouterOsTimeouts.Write, cancellationToken);
    }

-    public async Task ExecuteAsync(string path, IReadOnlyDictionary<string, string>? parameters = null, CancellationToken cancellationToken = default)
+    public async Task ExecuteAsync(string path, IReadOnlyDictionary<string, string>? parameters = null, CancellationToken cancellationToken = default) =>
+        await RunAsync(path, parameters, cancellationToken);
+
+    public async Task<IReadOnlyList<IReadOnlyDictionary<string, string>>> RunAsync(
+        string path, IReadOnlyDictionary<string, string>? parameters = null, CancellationToken cancellationToken = default)
    {
-        using var response = await SendAsync(HttpMethod.Post, path, parameters, cancellationToken);
+        // RouterOS erwartet bei POST immer einen JSON-Körper, auch ohne Parameter.
+        using var response = await SendAsync(HttpMethod.Post, path, parameters ?? new Dictionary<string, string>(), RouterOsTimeouts.Command, cancellationToken);
+        return await ReadRowsAsync(response, cancellationToken);
    }

    private async Task<HttpResponseMessage> SendAsync(
-        HttpMethod method, string path, IReadOnlyDictionary<string, string>? body, CancellationToken cancellationToken)
+        HttpMethod method, string path, IReadOnlyDictionary<string, string>? body, TimeSpan timeout, CancellationToken cancellationToken)
    {
+        using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
+        timeoutCts.CancelAfter(timeout);
+
        var request = new HttpRequestMessage(method, path.TrimStart('/'));
        if (body is not null)
        {
@@ -122,17 +142,22 @@ public sealed class RestRouterOsClient : IRouterOsClient
        HttpResponseMessage response;
        try
        {
-            response = await _httpClient.SendAsync(request, cancellationToken);
+            response = await _httpClient.SendAsync(request, timeoutCts.Token);
        }
-        catch (Exception ex)
+        catch (OperationCanceledException ex) when (!cancellationToken.IsCancellationRequested)
+        {
+            throw new RouterOsException("Das Gerät hat nicht rechtzeitig geantwortet.", ex);
+        }
+        catch (Exception ex) when (ex is not OperationCanceledException)
        {
            throw new RouterOsException("Die Verbindung zum Gerät wurde unterbrochen.", ex);
        }

        if (!response.IsSuccessStatusCode)
        {
+            var detail = await TryReadErrorDetailAsync(response);
            response.Dispose();
-            throw new RouterOsException(DescribeError(response.StatusCode));
+            throw new RouterOsException(DescribeError(response.StatusCode, detail));
        }

        return response;
@@ -151,10 +176,25 @@ public sealed class RestRouterOsClient : IRouterOsClient
        return dict;
    }

-    private static string DescribeError(HttpStatusCode statusCode) => statusCode switch
+    /// <summary>RouterOS liefert bei Fehlern {"error":400,"message":"Bad Request","detail":"…"} — "detail" nennt den eigentlichen Grund.</summary>
+    private static async Task<string?> TryReadErrorDetailAsync(HttpResponseMessage response)
+    {
+        try
+        {
+            using var document = JsonDocument.Parse(await response.Content.ReadAsStringAsync());
+            return document.RootElement.TryGetProperty("detail", out var detail) ? detail.GetString() : null;
+        }
+        catch (Exception ex) when (ex is JsonException or InvalidOperationException or HttpRequestException)
+        {
+            return null;
+        }
+    }
+
+    private static string DescribeError(HttpStatusCode statusCode, string? detail) => statusCode switch
    {
        HttpStatusCode.Unauthorized or HttpStatusCode.Forbidden => "Benutzername oder Passwort ist falsch.",
        HttpStatusCode.NotFound => "Diese Einstellung wird von deinem Gerät nicht unterstützt.",
+        _ when !string.IsNullOrWhiteSpace(detail) => $"Das Gerät hat die Anfrage abgelehnt: {detail}",
        _ => "Das Gerät hat die Anfrage abgelehnt.",
    };

@@ -0,0 +1,19 @@
+namespace MikrotikInstaller.Core.Connectivity;
+
+/// <summary>
+/// Zeitlimits je Art des Zugriffs. Schwache Hardware (z. B. hAP lite) braucht für Schreibvorgänge und
+/// erst recht für Backup/Restore/Update deutlich länger als für ein Auslesen — ein gemeinsames, knappes
+/// Limit meldete dort Fehler, obwohl der Router den Befehl ausgeführt hatte (Mac v1.3.0/v1.6.0).
+/// </summary>
+public static class RouterOsTimeouts
+{
+    /// <summary>Erster Erreichbarkeits-/Login-Test beim Verbinden — kurz, damit ein Fallback zügig greift.</summary>
+    public static readonly TimeSpan ConnectProbe = TimeSpan.FromSeconds(8);
+
+    public static readonly TimeSpan Read = TimeSpan.FromSeconds(15);
+
+    public static readonly TimeSpan Write = TimeSpan.FromSeconds(30);
+
+    /// <summary>Eigenständige Befehle wie Backup, Restore, Update, Ping.</summary>
+    public static readonly TimeSpan Command = TimeSpan.FromSeconds(120);
+}
@@ -9,6 +9,10 @@
    <DebugType>embedded</DebugType>
  </PropertyGroup>

+  <ItemGroup>
+    <InternalsVisibleTo Include="MikrotikInstaller.Core.Tests" />
+  </ItemGroup>
+
  <ItemGroup>
    <PackageReference Include="PDFsharp" Version="6.2.4" />
    <PackageReference Include="QRCoder" Version="1.8.0" />