From 29a0438e85662feb51dd05b81df377c06bf2dc56 Mon Sep 17 00:00:00 2001 From: Adolfo Marinucci Date: Thu, 15 Oct 2026 12:29:16 +0200 Subject: [PATCH 1/4] Classify synchronization failures and carry the server's reason to the client SyncAgent.SynchronizeAsync still wraps every failure in a SynchronizationException with the same message and inner exception, but now also reports Kind (Network, Unauthorized, Cancelled, AnchorTooOld, SessionExpired, ServerUnavailable, DataError, ConfigurationError, ServerError), Stage, TableName, ErrorCode, TraceId and ServerMessage, mirrored into Exception.Data. CoreSync.Http.Client rebuilds a SyncHttpRequestException (an HttpRequestException) from the X-CoreSync-Error / X-CoreSync-Trace headers and the SyncErrorDetails body a server emits, infers a coarse classification from the status code when it emits neither, and stops retrying bulk downloads only for failures the server marked as permanent. CoreSync.Http.Server answers an expired upload/download session with 410 + session-expired instead of an unhandled InvalidOperationException, and tags unexpected failures with the error code and trace headers while leaving the host's own exception handling untouched. --- .../Implementation/SyncProviderHttpClient.cs | 121 ++- .../SyncHttpRequestException.cs | 120 +++ .../SyncAgentController.cs | 12 +- .../SyncControllerOptions.cs | 22 + .../SyncSessionNotFoundException.cs | 24 + .../WebApplicationExtensions.cs | 158 +++- src/CoreSync.Http/SyncErrorDetails.cs | 54 ++ src/CoreSync.Http/SyncHttpErrorCodes.cs | 122 ++- src/CoreSync.Http/SyncHttpHeaders.cs | 8 + src/CoreSync.MySql/MySqlSyncProvider.cs | 6 +- .../PostgreSQLSyncProvider.cs | 6 +- src/CoreSync.SqlServer/SqlSyncProvider.cs | 6 +- .../SqlServerCTProvider.cs | 15 +- .../SyncErrorClassificationTests.cs | 736 ++++++++++++++++++ src/CoreSync.Tests/SyncTestServer.cs | 60 +- src/CoreSync/ISyncFailureInfo.cs | 44 ++ src/CoreSync/SyncAgent.cs | 154 +++- src/CoreSync/SyncAnchorTooOldException.cs | 14 +- src/CoreSync/SyncFailureKind.cs | 79 ++ src/CoreSync/SynchronizationException.cs | 162 +++- src/CoreSync/SynchronizationStage.cs | 60 ++ 21 files changed, 1935 insertions(+), 48 deletions(-) create mode 100644 src/CoreSync.Http.Client/SyncHttpRequestException.cs create mode 100644 src/CoreSync.Http.Server/SyncSessionNotFoundException.cs create mode 100644 src/CoreSync.Http/SyncErrorDetails.cs create mode 100644 src/CoreSync.Tests/SyncErrorClassificationTests.cs create mode 100644 src/CoreSync/ISyncFailureInfo.cs create mode 100644 src/CoreSync/SyncFailureKind.cs create mode 100644 src/CoreSync/SynchronizationStage.cs diff --git a/src/CoreSync.Http.Client/Implementation/SyncProviderHttpClient.cs b/src/CoreSync.Http.Client/Implementation/SyncProviderHttpClient.cs index 9dfe28f..009c390 100644 --- a/src/CoreSync.Http.Client/Implementation/SyncProviderHttpClient.cs +++ b/src/CoreSync.Http.Client/Implementation/SyncProviderHttpClient.cs @@ -34,18 +34,34 @@ public SyncProviderHttpClient(IHttpClientFactory httpClientProvider, SyncProvide /// /// Throws for an unsuccessful response, rebuilding from the - /// server's 410 Gone signal so that callers see the same typed error they would from a local provider. + /// server's 410 Gone signal and from everything else, so that + /// callers see what the server said instead of a bare status code. /// /// + /// /// Used in place of on every sync endpoint: - /// without it an aged-out anchor arrives as a bare and the only - /// thing an application can tell the user is that synchronization failed. + /// without it a failure arrives as a bare and the only thing an + /// application can tell the user is that synchronization failed. + /// + /// + /// derives from and carries + /// the very message would have produced, so + /// the failure callers used to see is still there; the header + /// and body are read when present and inferred from the status code + /// when the server (an older one) sends neither. + /// /// private static async Task EnsureSyncSuccessAsync(HttpResponseMessage response, CancellationToken cancellationToken) { - if (response.StatusCode == HttpStatusCode.Gone && - response.Headers.TryGetValues(SyncHttpHeaders.ErrorCode, out var errorCodes) && - errorCodes.Contains(SyncHttpErrorCodes.AnchorTooOld)) + if (response.IsSuccessStatusCode) + { + return; + } + + var errorCode = ReadHeader(response, SyncHttpHeaders.ErrorCode); + var traceId = ReadHeader(response, SyncHttpHeaders.TraceId); + + if (response.StatusCode == HttpStatusCode.Gone && errorCode == SyncHttpErrorCodes.AnchorTooOld) { SyncAnchorTooOldError? error = null; try @@ -66,9 +82,62 @@ private static async Task EnsureSyncSuccessAsync(HttpResponseMessage response, C : new SyncAnchorTooOldException(requestedVersion, minValidVersion); } - response.EnsureSuccessStatusCode(); + // The body is only read on the server's word: a JSON body without the header is the host's + // (a ProblemDetails for a 404, an error page), and reading it as SyncErrorDetails would + // invent an error code the server never sent. + SyncErrorDetails? details = null; + if (errorCode != null && IsJson(response.Content.Headers.ContentType)) + { + try + { + details = await response.Content.ReadFromJsonAsync(cancellationToken); + } + catch (Exception ex) when (ex is JsonException || ex is NotSupportedException) + { + // A body in a shape we do not know is not a reason to lose the header or the status code. + } + } + + string message; + try + { + response.EnsureSuccessStatusCode(); + return; + } + catch (HttpRequestException ex) + { + // Reused verbatim so the message callers have been reading (and logging) does not change. + message = ex.Message; + } + + throw new SyncHttpRequestException(message, response.StatusCode, errorCode, traceId, details); } + private static string? ReadHeader(HttpResponseMessage response, string name) + => response.Headers.TryGetValues(name, out var values) ? values.FirstOrDefault() : null; + + private static bool IsJson(MediaTypeHeaderValue? contentType) + => contentType?.MediaType is { } mediaType + && (mediaType.Equals("application/json", StringComparison.OrdinalIgnoreCase) + || mediaType.EndsWith("+json", StringComparison.OrdinalIgnoreCase)); + + /// + /// Decides whether a failed bulk download request is worth repeating. + /// + /// + /// Only a failure the server marked as permanent is not retried: an aged-out anchor, an expired + /// session, a data or configuration error. Everything else - a dropped connection, a timeout, a + /// 5xx without an error code from an older server or a proxy - keeps the retries it always had, + /// because those are what let a synchronization survive a flaky connection in the field. + /// + private static bool IsWorthRetrying(Exception exception) + => exception switch + { + SyncAnchorTooOldException => false, + SyncHttpRequestException http => http.IsTransient, + _ => true + }; + public async Task ApplyChangesAsync([NotNull] SyncChangeSet changeSet, [CanBeNull] Func? onConflictFunc = null, CancellationToken cancellationToken = default) { var httpClient = _httpClientFactory.CreateClient(_options.HttpClientName ?? Options.DefaultName); @@ -177,7 +246,16 @@ private async Task DownloadBulkChangeSetJson(BulkSyncChangeSet bu SyncProgress?.Invoke(this, new SyncProgressEventArgs(stage, 0.0)); ResiliencePipeline pipeline = new ResiliencePipelineBuilder() - .AddRetry(new RetryStrategyOptions { MaxRetryAttempts = _options.MaxRetryAttempts }) // Add retry using the default options + .AddRetry(new RetryStrategyOptions + { + MaxRetryAttempts = _options.MaxRetryAttempts, + // Polly's default handles every exception except a cancellation; keep that and + // additionally stop on failures the server marked as permanent. + ShouldHandle = args => new ValueTask( + args.Outcome.Exception is { } exception + && exception is not OperationCanceledException + && IsWorthRetrying(exception)) + }) .Build(); // Builds the resilience pipeline List items = []; @@ -185,7 +263,11 @@ private async Task DownloadBulkChangeSetJson(BulkSyncChangeSet bu { await pipeline.ExecuteAsync(async (token) => { - var bulkItems = await httpClient.GetFromJsonAsync($"/{_options.SyncControllerRoute}/changes-bulk-item/{bulkSyncChangeSet.SessionId}/{skip}/{_options.BulkItemSize}", cancellationToken: token) + var response = await httpClient.GetAsync($"/{_options.SyncControllerRoute}/changes-bulk-item/{bulkSyncChangeSet.SessionId}/{skip}/{_options.BulkItemSize}", token); + + await EnsureSyncSuccessAsync(response, token); + + var bulkItems = await response.Content.ReadFromJsonAsync(cancellationToken: token) ?? throw new InvalidOperationException(); items.AddRange(bulkItems); @@ -229,10 +311,11 @@ private async Task DownloadBulkChangeSetBinary(BulkSyncChangeSet items.AddRange(bulkItems); break; } - catch (SyncAnchorTooOldException) + catch (Exception ex) when (!IsWorthRetrying(ex)) { - // Permanent for this client: the change history it asked for is gone. Retrying - // burns three round trips and fifteen seconds to reach the same answer. + // Permanent for this client (the change history it asked for is gone, the session + // expired, the server refused the data). Retrying burns three round trips and + // fifteen seconds to reach the same answer. throw; } catch (Exception) @@ -303,10 +386,12 @@ public async Task GetStoreIdAsync(CancellationToken cancellationToken = de var httpClient = _httpClientFactory.CreateClient(_options.HttpClientName ?? Options.DefaultName); var res = await httpClient.GetAsync($"/{_options.SyncControllerRoute}/store-id", cancellationToken); + // Kept ahead of the generic handling on purpose: applications have been catching this exact + // type around SynchronizeAsync to trigger a re-authentication. if (res.StatusCode == HttpStatusCode.Unauthorized) throw new UnauthorizedAccessException(); - res.EnsureSuccessStatusCode(); + await EnsureSyncSuccessAsync(res, cancellationToken); return Guid.Parse(await res.Content.ReadAsStringAsync(cancellationToken)); } @@ -314,13 +399,17 @@ public async Task GetStoreIdAsync(CancellationToken cancellationToken = de public async Task SaveVersionForStoreAsync(Guid otherStoreId, long version, CancellationToken cancellationToken = default) { var httpClient = _httpClientFactory.CreateClient(_options.HttpClientName ?? Options.DefaultName); - (await httpClient.PostAsync($"/{_options.SyncControllerRoute}/save-version/{otherStoreId}/{version}", null, cancellationToken)) - .EnsureSuccessStatusCode(); + await EnsureSyncSuccessAsync( + await httpClient.PostAsync($"/{_options.SyncControllerRoute}/save-version/{otherStoreId}/{version}", null, cancellationToken), + cancellationToken); } public async Task GetSyncVersionAsync(CancellationToken cancellationToken = default) { var httpClient = _httpClientFactory.CreateClient(_options.HttpClientName ?? Options.DefaultName); - return await httpClient.GetFromJsonAsync($"/{_options.SyncControllerRoute}/sync-version", cancellationToken) ?? throw new InvalidOperationException(); + var response = await httpClient.GetAsync($"/{_options.SyncControllerRoute}/sync-version", cancellationToken); + await EnsureSyncSuccessAsync(response, cancellationToken); + + return await response.Content.ReadFromJsonAsync(cancellationToken) ?? throw new InvalidOperationException(); } } diff --git a/src/CoreSync.Http.Client/SyncHttpRequestException.cs b/src/CoreSync.Http.Client/SyncHttpRequestException.cs new file mode 100644 index 0000000..8a252c9 --- /dev/null +++ b/src/CoreSync.Http.Client/SyncHttpRequestException.cs @@ -0,0 +1,120 @@ +using System; +using System.Net; +using System.Net.Http; + +namespace CoreSync.Http.Client; + +/// +/// Raised by when the server answers a synchronization request +/// with a non-success status code, carrying whatever the server said about the failure. +/// +/// +/// +/// It derives from and keeps the message and +/// that +/// produces, so code written against the previous behaviour - typically +/// catch (SynchronizationException ex) when (ex.InnerException is HttpRequestException) - keeps +/// working unchanged. What is new is the classification: , , +/// , and , read from the +/// header and body when the +/// server sends them, inferred from the status code when it does not (an older server). +/// +/// +/// A genuine connectivity failure (no route, DNS, TLS, connection reset) is not this exception: it +/// stays the plain that throws, with no +/// status code, which is how tells "the server was never reached" from "the +/// server answered with an error". +/// +/// +public class SyncHttpRequestException : HttpRequestException, ISyncFailureInfo +{ + /// + /// Initializes a new instance of the class. + /// + /// + /// The message; callers pass the one + /// would have produced, so that it stays identical to the previous behaviour. + /// + /// The response status code. + /// The header value, or null. + /// The header value, or null. + /// The parsed body, or null. + public SyncHttpRequestException( + string message, + HttpStatusCode statusCode, + string? errorCode, + string? traceId, + SyncErrorDetails? details) + : base(message, null, statusCode) + { + ErrorCode = errorCode; + TraceId = traceId ?? details?.TraceId; + TableName = details?.TableName; + ServerMessage = details?.Message; + Detail = details?.Detail; + + // The header is the server's word and wins. Without it the status code is all there is, and + // the mapping is deliberately coarse: the point is to tell "the server answered with an error" + // (so the device is online) from everything else, not to guess at causes. + Kind = ErrorCode != null + ? SyncHttpErrorCodes.ToFailureKind(ErrorCode) + : statusCode switch + { + HttpStatusCode.Unauthorized => SyncFailureKind.Unauthorized, + HttpStatusCode.Forbidden => SyncFailureKind.Unauthorized, + HttpStatusCode.RequestTimeout => SyncFailureKind.ServerUnavailable, + HttpStatusCode.TooManyRequests => SyncFailureKind.ServerUnavailable, + HttpStatusCode.BadGateway => SyncFailureKind.ServerUnavailable, + HttpStatusCode.ServiceUnavailable => SyncFailureKind.ServerUnavailable, + HttpStatusCode.GatewayTimeout => SyncFailureKind.ServerUnavailable, + _ => SyncFailureKind.ServerError + }; + + // Only a server that says "permanent" stops a retry. A server that says nothing (older + // versions, a proxy's own error page) gets the retries it always got, because those are what + // rescue a sync over a flaky connection. + IsTransient = !SyncHttpErrorCodes.IsPermanent(ErrorCode); + + SetData(SynchronizationException.DataKeyKind, Kind.ToString()); + SetData(SynchronizationException.DataKeyErrorCode, ErrorCode); + SetData(SynchronizationException.DataKeyTableName, TableName); + SetData(SynchronizationException.DataKeyTraceId, TraceId); + SetData(SynchronizationException.DataKeyServerMessage, ServerMessage); + SetData("CoreSync.StatusCode", ((int)statusCode).ToString()); + } + + /// + public SyncFailureKind Kind { get; } + + /// + public string? ErrorCode { get; } + + /// + public string? TableName { get; } + + /// + public string? TraceId { get; } + + /// + public string? ServerMessage { get; } + + /// + /// Gets the diagnostic detail the server attached (exception types and messages), when the server + /// is configured to include it; null otherwise. + /// + public string? Detail { get; } + + /// + /// Gets a value indicating whether repeating the same request could succeed. false only + /// when the server marked the failure as permanent through its error code. + /// + public bool IsTransient { get; } + + private void SetData(string key, string? value) + { + if (value != null) + { + Data[key] = value; + } + } +} diff --git a/src/CoreSync.Http.Server/SyncAgentController.cs b/src/CoreSync.Http.Server/SyncAgentController.cs index e7c31e0..9fee9eb 100644 --- a/src/CoreSync.Http.Server/SyncAgentController.cs +++ b/src/CoreSync.Http.Server/SyncAgentController.cs @@ -99,7 +99,7 @@ public IReadOnlyList GetBulkChangeSetItem([FromQuery] BulkChangeSetDow return bufferList; } - throw new InvalidOperationException(); + throw new SyncSessionNotFoundException(item.SessionId); } public byte[] GetBulkChangeSetItemBinary([FromBody] BulkChangeSetDownloadItem item) @@ -123,7 +123,7 @@ public byte[] GetBulkChangeSetItemBinary([FromBody] BulkChangeSetDownloadItem it return CoreSyncMessagePackSerializer.Serialize(bufferList); } - throw new InvalidOperationException(); + throw new SyncSessionNotFoundException(item.SessionId); } public void BeginApplyBulkChanges(BulkSyncChangeSet bulkChangeSet) @@ -142,7 +142,7 @@ public void ApplyBulkChangesItem(BulkChangeSetUploadItem bulkUploadItem) return; } - throw new InvalidOperationException(); + throw new SyncSessionNotFoundException(bulkUploadItem.SessionId); } public async Task ApplyBulkChangesItemBinary(HttpRequest httpRequest) @@ -157,7 +157,7 @@ public async Task ApplyBulkChangesItemBinary(HttpRequest httpRequest) return; } - throw new InvalidOperationException(); + throw new SyncSessionNotFoundException(bulkUploadItem.SessionId); } public async Task CompleteApplyBulkChangesAsync(Guid sessionId) @@ -187,7 +187,7 @@ public async Task CompleteApplyBulkChangesAsync(Guid sessionId) return resAnchor; } - throw new InvalidOperationException(); + throw new SyncSessionNotFoundException(sessionId); } public async Task CompleteApplyBulkChangesBinaryAsync(Guid sessionId) @@ -204,7 +204,7 @@ public async Task CompleteApplyBulkChangesBinaryAsync(Guid sessionId return resAnchor; } - throw new InvalidOperationException(); + throw new SyncSessionNotFoundException(sessionId); } diff --git a/src/CoreSync.Http.Server/SyncControllerOptions.cs b/src/CoreSync.Http.Server/SyncControllerOptions.cs index 2290f56..22ce859 100644 --- a/src/CoreSync.Http.Server/SyncControllerOptions.cs +++ b/src/CoreSync.Http.Server/SyncControllerOptions.cs @@ -65,4 +65,26 @@ public class SyncControllerOptions /// public Action? PostSaveVersionForStoreAsyncEndPoint { get; set; } + /// + /// Gets or sets a value indicating whether the sync endpoints answer unexpected exceptions + /// themselves, with HTTP 500 and a JSON body, instead + /// of letting them propagate to the host's exception handling. Defaults to false. + /// + /// + /// With the default, an unexpected exception reaches the host exactly as before (its exception + /// handler, its logging and its error reporting all see it); the endpoints only add the + /// and + /// headers to whatever response the host produces, so a client can still classify the failure. + /// Set to true for a host that prefers a JSON body over its error page; the exception is then + /// logged at Error level through and does not + /// reach the host's exception handler. + /// + public bool HandleUnexpectedExceptions { get; set; } + + /// + /// Gets or sets a value indicating whether + /// (the exception types and messages behind a failure) is sent to clients. Defaults to false: + /// provider messages can carry SQL text and schema details that do not belong on a device. + /// + public bool IncludeExceptionDetails { get; set; } } diff --git a/src/CoreSync.Http.Server/SyncSessionNotFoundException.cs b/src/CoreSync.Http.Server/SyncSessionNotFoundException.cs new file mode 100644 index 0000000..d603267 --- /dev/null +++ b/src/CoreSync.Http.Server/SyncSessionNotFoundException.cs @@ -0,0 +1,24 @@ +using System; + +namespace CoreSync.Http.Server; + +/// +/// Raised by when a request refers to an upload or download session +/// the memory cache no longer holds: it was evicted, the process restarted, or the client took too long +/// between two requests of the same synchronization. +/// +/// +/// Derives from , which is what the controller threw before the +/// condition had a name, so any host code catching that type keeps working. The endpoint filter turns +/// it into a 410 Gone carrying . +/// +internal sealed class SyncSessionNotFoundException : InvalidOperationException +{ + public SyncSessionNotFoundException(Guid sessionId) + : base($"Synchronization session '{sessionId}' is not available on the server: it expired or the server restarted. Start a new synchronization.") + { + SessionId = sessionId; + } + + public Guid SessionId { get; } +} diff --git a/src/CoreSync.Http.Server/WebApplicationExtensions.cs b/src/CoreSync.Http.Server/WebApplicationExtensions.cs index 180c780..1486a8c 100644 --- a/src/CoreSync.Http.Server/WebApplicationExtensions.cs +++ b/src/CoreSync.Http.Server/WebApplicationExtensions.cs @@ -5,6 +5,8 @@ using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.Formatters; using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Primitives; using System; using System.IO; @@ -50,9 +52,10 @@ public static void MapCoreSyncHttpServerEndpoints(this IEndpointRouteBuilder web void ConfigureEndpoint(T endpoint, Action? specificAction = null) where T : IEndpointConventionBuilder { - // Added before the caller's own conventions so that an aged-out anchor reaches the client - // as a documented 410 rather than whatever the host makes of an unhandled exception. - endpoint.AddSyncErrorEndpointFilter(); + // Added before the caller's own conventions so that a recognised failure (an aged-out + // anchor, an expired session) reaches the client as a documented status with an error + // code rather than whatever the host makes of an unhandled exception. + endpoint.AddSyncErrorEndpointFilter(options); options.AllEndpoints?.Invoke(endpoint); specificAction?.Invoke(endpoint); } @@ -137,26 +140,45 @@ void ConfigureEndpoint(T endpoint, Action? specificAction = null) where T internal static class SyncErrorFilterEndpointExtensions { /// - /// Translates into an HTTP 410 Gone carrying the - /// header and a body. + /// Translates the failures a sync endpoint can raise into responses a client can classify: + /// a status code, the and + /// headers, and a JSON body where the endpoint produces the response itself. /// /// - /// 410 is the honest code here: the change history the client asked for existed once and is - /// permanently gone, so unlike a 5xx there is nothing to retry. Without this the condition - /// surfaced as an unhandled exception, which the client could only report as a generic - /// "unable to synchronize". + /// + /// + /// : 410 Gone, , + /// body. 410 is the honest code: the change history the client + /// asked for existed once and is permanently gone, so unlike a 5xx there is nothing to retry. + /// + /// + /// : 410 Gone, , + /// body. Before it had a name this was an unhandled + /// , i.e. a 500 that clients retried for nothing. + /// + /// + /// Anything else keeps propagating to the host exactly as before - same status, same error page, + /// same logging and error reporting - with the two headers added to the host's response through + /// , which survives the Response.Clear() the exception + /// handler middleware performs. + /// switches this to a 500 with a body produced here. + /// + /// /// - public static TBuilder AddSyncErrorEndpointFilter(this TBuilder builder) where TBuilder : IEndpointConventionBuilder + public static TBuilder AddSyncErrorEndpointFilter(this TBuilder builder, SyncControllerOptions options) where TBuilder : IEndpointConventionBuilder { builder.AddEndpointFilter(async (context, next) => { + var response = context.HttpContext.Response; + try { return await next(context); } catch (SyncAnchorTooOldException ex) { - context.HttpContext.Response.Headers[SyncHttpHeaders.ErrorCode] = SyncHttpErrorCodes.AnchorTooOld; + response.Headers[SyncHttpHeaders.ErrorCode] = SyncHttpErrorCodes.AnchorTooOld; + response.Headers[SyncHttpHeaders.TraceId] = context.HttpContext.TraceIdentifier; return Results.Json( new SyncAnchorTooOldError @@ -167,12 +189,126 @@ public static TBuilder AddSyncErrorEndpointFilter(this TBuilder builde }, statusCode: StatusCodes.Status410Gone); } + catch (SyncSessionNotFoundException ex) + { + var details = SyncErrorResponse.Describe(context.HttpContext, SyncHttpErrorCodes.SessionExpired, ex, tableName: null, options.IncludeExceptionDetails); + return SyncErrorResponse.Create(context.HttpContext, details, StatusCodes.Status410Gone); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + var code = SyncErrorResponse.ClassifyUnexpected(ex, out var tableName); + + if (options.HandleUnexpectedExceptions) + { + // The host's exception handler will not see this one, so make sure its loggers do. + context.HttpContext.RequestServices + .GetRequiredService() + .CreateLogger(typeof(SyncAgentController).Namespace!) + .LogError(ex, "Synchronization request {Method} {Path} failed ({ErrorCode})", context.HttpContext.Request.Method, context.HttpContext.Request.Path, code); + + var details = SyncErrorResponse.Describe(context.HttpContext, code, ex, tableName, options.IncludeExceptionDetails); + return SyncErrorResponse.Create(context.HttpContext, details, StatusCodes.Status500InternalServerError); + } + + // Not handled here: the host decides the status and the body, and keeps logging and + // reporting the exception the way it always did. Only the headers are ours. They are + // attached when the host's response starts because the exception handler middleware + // clears every header set before it runs; OnStarting callbacks are not cleared. + var traceId = context.HttpContext.TraceIdentifier; + response.OnStarting(() => + { + if (!response.Headers.ContainsKey(SyncHttpHeaders.ErrorCode)) + { + response.Headers[SyncHttpHeaders.ErrorCode] = code; + response.Headers[SyncHttpHeaders.TraceId] = traceId; + } + + return Task.CompletedTask; + }); + + throw; + } }); return builder; } } +/// +/// Builds the responses of the sync endpoints. +/// +internal static class SyncErrorResponse +{ + /// + /// Picks the error code for an exception the endpoint did not expect. + /// + /// + /// A anywhere in the chain is a provider telling that the + /// store refused a change (constraint violation, missing table, bad value), and it names the table + /// when it can. is what providers raise for a store they cannot + /// synchronize as configured. Everything else is simply "the server failed", which still tells the + /// client that the device is online. + /// + public static string ClassifyUnexpected(Exception exception, out string? tableName) + { + tableName = null; + + for (var current = exception; current != null; current = current.InnerException) + { + switch (current) + { + case SynchronizationException sync: + tableName = sync.TableName; + return SyncHttpErrorCodes.DataError; + case NotSupportedException: + return SyncHttpErrorCodes.ConfigurationError; + } + } + + return SyncHttpErrorCodes.ServerError; + } + + public static SyncErrorDetails Describe(HttpContext httpContext, string code, Exception exception, string? tableName, bool includeExceptionDetails) + { + var message = code switch + { + SyncHttpErrorCodes.SessionExpired => "The synchronization session is no longer available on the server. Start a new synchronization.", + SyncHttpErrorCodes.DataError when tableName != null => $"The server could not apply the synchronized changes for table '{tableName}'.", + SyncHttpErrorCodes.DataError => "The server could not apply the synchronized changes.", + SyncHttpErrorCodes.ConfigurationError => "The server's data store is not configured in a way that can be synchronized.", + _ => "The server could not process the synchronization request." + }; + + return new SyncErrorDetails + { + Code = code, + Message = message, + TableName = tableName, + TraceId = httpContext.TraceIdentifier, + Detail = includeExceptionDetails ? DescribeChain(exception) : null + }; + } + + public static IResult Create(HttpContext httpContext, SyncErrorDetails details, int statusCode) + { + httpContext.Response.Headers[SyncHttpHeaders.ErrorCode] = details.Code; + httpContext.Response.Headers[SyncHttpHeaders.TraceId] = details.TraceId; + + return Results.Json(details, statusCode: statusCode); + } + + private static string DescribeChain(Exception exception) + { + var parts = new System.Collections.Generic.List(); + for (var current = exception; current != null; current = current.InnerException) + { + parts.Add($"{current.GetType().FullName}: {current.Message}"); + } + + return string.Join(" --> ", parts); + } +} + internal static class EndpointMessagePackFilterEndpointExtensions { public static TBuilder AddMessagePackEndpointFilter(this TBuilder builder) where TBuilder : IEndpointConventionBuilder diff --git a/src/CoreSync.Http/SyncErrorDetails.cs b/src/CoreSync.Http/SyncErrorDetails.cs new file mode 100644 index 0000000..d30d386 --- /dev/null +++ b/src/CoreSync.Http/SyncErrorDetails.cs @@ -0,0 +1,54 @@ +namespace CoreSync.Http; + +/// +/// JSON body a CoreSync server attaches to an unsuccessful response when it recognises the failure, +/// alongside the header carrying the same . +/// +/// +/// +/// The header is the contract and this body is the detail: a client that cannot read the body (an +/// HTML error page from the host, a truncated response) still classifies the failure from the header, +/// and a client that predates both ignores them and sees the status code it always saw. +/// +/// +/// Nothing in here is meant to leak what the server knows: is a short sentence +/// safe to show a user, names the table involved when the server knows it, and +/// is the handle support uses to find the full story in the server's logs. +/// (exception types and messages) is only filled when the server operator opts in. +/// +/// +/// The 410 anchor-too-old response keeps its own body (), which +/// existing clients already parse. +/// +/// +public class SyncErrorDetails +{ + /// + /// Gets or sets the machine-readable error code; one of the values. + /// + public string Code { get; set; } = SyncHttpErrorCodes.ServerError; + + /// + /// Gets or sets a short, user-safe description of the failure. + /// + public string? Message { get; set; } + + /// + /// Gets or sets the table involved in the failure, when the server knows it. + /// + public string? TableName { get; set; } + + /// + /// Gets or sets the identifier under which the server logged the failure + /// (its request trace identifier), so that support can correlate a user report with the server + /// side. Also sent as the header. + /// + public string? TraceId { get; set; } + + /// + /// Gets or sets diagnostic detail (exception types and messages). Only present when the server is + /// configured to include it; never contains it by default because provider messages can carry + /// SQL text and schema details. + /// + public string? Detail { get; set; } +} diff --git a/src/CoreSync.Http/SyncHttpErrorCodes.cs b/src/CoreSync.Http/SyncHttpErrorCodes.cs index ddac2d8..110fcda 100644 --- a/src/CoreSync.Http/SyncHttpErrorCodes.cs +++ b/src/CoreSync.Http/SyncHttpErrorCodes.cs @@ -4,9 +4,16 @@ namespace CoreSync.Http; /// Machine-readable values carried by the response header. /// /// +/// /// The header lets a client branch on a failure without parsing the response body or matching on /// message text, and stays readable even when the body is a message pack payload the client did not -/// expect. +/// expect, or an HTML error page produced by the host. +/// +/// +/// The header is additive to the status code, never a replacement: a client that does not know a +/// code still sees the same non-success status it always did. A client that does know it can tell a +/// permanent failure (reinitialize, fix the data, fix the configuration) from one worth retrying. +/// /// public static class SyncHttpErrorCodes { @@ -20,4 +27,117 @@ public static class SyncHttpErrorCodes /// The client has to be reinitialized from a fresh snapshot. /// public const string AnchorTooOld = "anchor-too-old"; + + /// + /// The upload or download session the request refers to is no longer known to the server: it was + /// evicted from the cache, the server restarted, or the client took too long between requests. + /// Accompanied by a body. + /// + /// + /// Permanent for the current synchronization, harmless for the data: retrying the same request + /// cannot succeed, but starting a new synchronization will. + /// + public const string SessionExpired = "session-expired"; + + /// + /// The synchronization endpoint addressed by the request does not exist or is not published. + /// Accompanied by a body. + /// + /// + /// Permanent until an administrator changes the server configuration or the client its address. + /// + public const string EndpointNotFound = "endpoint-not-found"; + + /// + /// The server is reachable but the component that owns the data store (for example an on-premise + /// agent) is not connected or did not answer in time. Accompanied by a + /// body. + /// + /// + /// Transient: retrying later is appropriate. + /// + public const string AgentUnavailable = "agent-unavailable"; + + /// + /// The store is configured in a way the server cannot synchronize (a table without a primary key, + /// a reserved column name, a table missing from the configuration...). Accompanied by a + /// body. + /// + /// + /// Permanent until an administrator fixes the configuration. + /// + public const string ConfigurationError = "configuration-error"; + + /// + /// The server's store refused the changes or could not produce them: a constraint violation, a + /// missing table or column, a value that could not be converted. Accompanied by a + /// body naming the table when it is known. + /// + /// + /// Permanent for the changes involved: retrying the same request cannot succeed. + /// + public const string DataError = "data-error"; + + /// + /// The server failed for a reason it did not classify further. + /// + /// + /// Treated as transient by the client, which is how an unclassified failure has always been + /// treated: the request is retried where the client retried before. + /// + public const string ServerError = "server-error"; + + /// + /// Returns true when identifies a failure that retrying the same + /// request cannot fix. + /// + /// + /// Unknown codes (from a newer server) and null (no header at all) are reported as not + /// permanent, so a client never stops retrying something it used to retry just because the server + /// learned a new word. + /// + public static bool IsPermanent(string? code) + { + switch (code) + { + case AnchorTooOld: + case SessionExpired: + case EndpointNotFound: + case ConfigurationError: + case DataError: + return true; + default: + return false; + } + } + + /// + /// Maps an error code to the an application should see. + /// + /// The value of the header, or null. + /// + /// The matching kind, for a code this version does not + /// know, and for null. + /// + public static SyncFailureKind ToFailureKind(string? code) + { + switch (code) + { + case null: + return SyncFailureKind.Unknown; + case AnchorTooOld: + return SyncFailureKind.AnchorTooOld; + case SessionExpired: + return SyncFailureKind.SessionExpired; + case EndpointNotFound: + case ConfigurationError: + return SyncFailureKind.ConfigurationError; + case AgentUnavailable: + return SyncFailureKind.ServerUnavailable; + case DataError: + return SyncFailureKind.DataError; + default: + return SyncFailureKind.ServerError; + } + } } diff --git a/src/CoreSync.Http/SyncHttpHeaders.cs b/src/CoreSync.Http/SyncHttpHeaders.cs index 1f2f759..034a6d3 100644 --- a/src/CoreSync.Http/SyncHttpHeaders.cs +++ b/src/CoreSync.Http/SyncHttpHeaders.cs @@ -23,4 +23,12 @@ public static class SyncHttpHeaders /// See for the defined values. /// public const string ErrorCode = "X-CoreSync-Error"; + + /// + /// The identifier under which the server logged a failed request (its request trace identifier), + /// set on unsuccessful responses so that a user report can be matched with the server's logs. + /// Sent as a header, and not only inside , because for unexpected + /// failures the body is whatever the host's error handler produces. + /// + public const string TraceId = "X-CoreSync-Trace"; } diff --git a/src/CoreSync.MySql/MySqlSyncProvider.cs b/src/CoreSync.MySql/MySqlSyncProvider.cs index c967a36..1a33f04 100644 --- a/src/CoreSync.MySql/MySqlSyncProvider.cs +++ b/src/CoreSync.MySql/MySqlSyncProvider.cs @@ -120,7 +120,11 @@ await HasConflictAsync(connection, tr, table, item, changeSet.TargetAnchor.Versi catch (Exception ex) { _logger?.Error($"Unable to {itemChangeType} item {item} to store for table {table}.{Environment.NewLine}{ex}{Environment.NewLine}Generated SQL:{Environment.NewLine}{cmd.CommandText}"); - throw new SynchronizationException($"Unable to {itemChangeType} item {item} to store for table {table}", ex); + throw new SynchronizationException($"Unable to {itemChangeType} item {item} to store for table {table}", ex) + { + Kind = SyncFailureKind.DataError, + TableName = item.TableName + }; } if (affectedRows == 0) diff --git a/src/CoreSync.PostgreSQL/PostgreSQLSyncProvider.cs b/src/CoreSync.PostgreSQL/PostgreSQLSyncProvider.cs index 5a748cf..97d1e3c 100644 --- a/src/CoreSync.PostgreSQL/PostgreSQLSyncProvider.cs +++ b/src/CoreSync.PostgreSQL/PostgreSQLSyncProvider.cs @@ -121,7 +121,11 @@ public async Task ApplyChangesAsync([NotNull] SyncChangeSet changeSe catch (Exception ex) { _logger?.Error($"Unable to {itemChangeType} item {item} to store for table {table}.{Environment.NewLine}{ex}{Environment.NewLine}Generated SQL:{Environment.NewLine}{cmd.CommandText}"); - throw new SynchronizationException($"Unable to {itemChangeType} item {item} to store for table {table}", ex); + throw new SynchronizationException($"Unable to {itemChangeType} item {item} to store for table {table}", ex) + { + Kind = SyncFailureKind.DataError, + TableName = item.TableName + }; } if (affectedRows == 0) diff --git a/src/CoreSync.SqlServer/SqlSyncProvider.cs b/src/CoreSync.SqlServer/SqlSyncProvider.cs index cb4b1a9..621cdd4 100644 --- a/src/CoreSync.SqlServer/SqlSyncProvider.cs +++ b/src/CoreSync.SqlServer/SqlSyncProvider.cs @@ -121,7 +121,11 @@ public async Task ApplyChangesAsync([NotNull] SyncChangeSet changeSe catch (Exception ex) { _logger?.Error($"Unable to {itemChangeType} item {item} to store for table {table}.{Environment.NewLine}Generated SQL:{Environment.NewLine}{cmd.CommandText}"); - throw new SynchronizationException($"Unable to {itemChangeType} item {item} to store for table {table}", ex); + throw new SynchronizationException($"Unable to {itemChangeType} item {item} to store for table {table}", ex) + { + Kind = SyncFailureKind.DataError, + TableName = item.TableName + }; } if (affectedRows == 0) diff --git a/src/CoreSync.SqlServerCT/SqlServerCTProvider.cs b/src/CoreSync.SqlServerCT/SqlServerCTProvider.cs index 1f1a432..5b23444 100644 --- a/src/CoreSync.SqlServerCT/SqlServerCTProvider.cs +++ b/src/CoreSync.SqlServerCT/SqlServerCTProvider.cs @@ -90,6 +90,7 @@ public async Task ApplyChangesAsync([NotNull] SyncChangeSet changeSe var failedItems = new List(); int appliedInPass = 0; SqlException? lastFailure = null; + string? lastFailureTable = null; foreach (var item in remainingItems) { @@ -151,13 +152,18 @@ public async Task ApplyChangesAsync([NotNull] SyncChangeSet changeSe // than dropped. _logger?.Trace($"[{_storeId}] Deferring {itemChangeType} of {item} on table {table} (pass {pass}): {ex.Message}"); lastFailure = ex; + lastFailureTable = item.TableName; failedItems.Add(item); continue; } catch (Exception ex) { _logger?.Error(DescribeItemFailure(itemChangeType, item, table, cmd.CommandText, messageLog, ex)); - throw new SynchronizationException($"Unable to {itemChangeType} item {item} to store for table {table}", ex); + throw new SynchronizationException($"Unable to {itemChangeType} item {item} to store for table {table}", ex) + { + Kind = SyncFailureKind.DataError, + TableName = item.TableName + }; } if (affectedRows == 0) @@ -235,6 +241,7 @@ public async Task ApplyChangesAsync([NotNull] SyncChangeSet changeSe if (itemFailure != null) { lastFailure = itemFailure; + lastFailureTable = item.TableName; } } } @@ -251,7 +258,11 @@ public async Task ApplyChangesAsync([NotNull] SyncChangeSet changeSe _logger?.Error($"[{_storeId}] {failedItems.Count} item(s) could not be applied after {pass} pass(es): {lastFailure.Message}"); throw new SynchronizationException( $"Unable to apply {failedItems.Count} item(s) after {pass} pass(es); the change set does not resolve the last failure", - lastFailure); + lastFailure) + { + Kind = SyncFailureKind.DataError, + TableName = lastFailureTable + }; } _logger?.Warning($"[{_storeId}] {failedItems.Count} item(s) could not be applied after {pass} pass(es)"); diff --git a/src/CoreSync.Tests/SyncErrorClassificationTests.cs b/src/CoreSync.Tests/SyncErrorClassificationTests.cs new file mode 100644 index 0000000..a70a07c --- /dev/null +++ b/src/CoreSync.Tests/SyncErrorClassificationTests.cs @@ -0,0 +1,736 @@ +using CoreSync.Http; +using CoreSync.Http.Client; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Shouldly; +using System; +using System.Collections.Generic; +using System.Linq; +using System.Net; +using System.Net.Http; +using System.Net.Http.Json; +using System.Text; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; + +namespace CoreSync.Tests; + +/// +/// Covers how a synchronization failure is classified on its way from the server (or the network) to +/// the application: the X-CoreSync-Error header and body the +/// server emits, the the client rebuilds from them, the retry +/// decisions that depend on it, and the / +/// that finally reports. +/// +/// +/// Every test here also guards the compatibility promise: the status codes, exception types and +/// messages an application saw before the classification existed are still what it sees. An +/// application written against CoreSync 1.0.1 must keep working against this server, and this client +/// must keep working against a 1.0.1 server, which sends none of the new signals. +/// +[TestClass] +public class SyncErrorClassificationTests +{ + #region Server: CoreSync.Http.Server responses + + [TestMethod] + public async Task HttpServer_UnknownDownloadSession_RespondsGoneWithSessionExpired() + { + using var server = SyncTestServer.Create(new ScriptedSyncProvider()); + + using var response = await server.RawHttpClient.GetAsync($"/api/sync-agent/changes-bulk-item/{Guid.NewGuid()}/0/50"); + + // Before it had a name this was an unhandled InvalidOperationException, a bodiless 500 the + // client retried three times for nothing. 410: the session existed and is gone for good. + response.StatusCode.ShouldBe(HttpStatusCode.Gone); + response.Headers.GetValues(SyncHttpHeaders.ErrorCode).ShouldContain(SyncHttpErrorCodes.SessionExpired); + response.Headers.GetValues(SyncHttpHeaders.TraceId).Single().ShouldNotBeNullOrWhiteSpace(); + + var details = await response.Content.ReadFromJsonAsync(); + details.ShouldNotBeNull(); + details!.Code.ShouldBe(SyncHttpErrorCodes.SessionExpired); + details.Message.ShouldNotBeNullOrWhiteSpace(); + details.TraceId.ShouldBe(response.Headers.GetValues(SyncHttpHeaders.TraceId).Single()); + details.Detail.ShouldBeNull(); + + // An old client calls EnsureSuccessStatusCode and nothing else: it must still see a failure. + Should.Throw(() => response.EnsureSuccessStatusCode()); + } + + [TestMethod] + public async Task HttpServer_UnknownUploadSession_RespondsGoneWithSessionExpired() + { + using var server = SyncTestServer.Create(new ScriptedSyncProvider()); + + using var response = await server.RawHttpClient.PostAsync($"/api/sync-agent/changes-bulk-complete/{Guid.NewGuid()}", null); + + response.StatusCode.ShouldBe(HttpStatusCode.Gone); + response.Headers.GetValues(SyncHttpHeaders.ErrorCode).ShouldContain(SyncHttpErrorCodes.SessionExpired); + } + + [TestMethod] + public async Task HttpServer_UnexpectedFailure_KeepsHostResponseAndAddsHeaders() + { + var failure = new SynchronizationException("Unable to Insert item to store for table Posts", new InvalidOperationException("FK")) + { + Kind = SyncFailureKind.DataError, + TableName = "Posts" + }; + + using var server = SyncTestServer.Create(new ScriptedSyncProvider { ApplyChanges = _ => throw failure }, useHostExceptionHandler: true); + + using var response = await server.RawHttpClient.PostAsync($"/api/sync-agent/changes-bulk-complete/{await BeginUploadAsync(server)}", null); + + // The host still owns the response - same 500, same error page, same logging and reporting - + // only the headers are added, surviving the Response.Clear() the exception handler performs. + response.StatusCode.ShouldBe(HttpStatusCode.InternalServerError); + (await response.Content.ReadAsStringAsync()).ShouldBe(SyncTestServer.HostErrorPageBody); + response.Headers.GetValues(SyncHttpHeaders.ErrorCode).ShouldContain(SyncHttpErrorCodes.DataError); + response.Headers.GetValues(SyncHttpHeaders.TraceId).Single().ShouldNotBeNullOrWhiteSpace(); + } + + [TestMethod] + public async Task HttpServer_UnexpectedFailure_ClassifiesConfigurationAndServerErrors() + { + using (var server = SyncTestServer.Create(new ScriptedSyncProvider { GetChanges = _ => throw new NotSupportedException("no primary key") }, useHostExceptionHandler: true)) + { + using var response = await server.RawHttpClient.GetAsync($"/api/sync-agent/changes-bulk/{Guid.NewGuid()}"); + response.StatusCode.ShouldBe(HttpStatusCode.InternalServerError); + response.Headers.GetValues(SyncHttpHeaders.ErrorCode).ShouldContain(SyncHttpErrorCodes.ConfigurationError); + } + + using (var server = SyncTestServer.Create(new ScriptedSyncProvider { GetChanges = _ => throw new InvalidOperationException("something else") }, useHostExceptionHandler: true)) + { + using var response = await server.RawHttpClient.GetAsync($"/api/sync-agent/changes-bulk/{Guid.NewGuid()}"); + response.StatusCode.ShouldBe(HttpStatusCode.InternalServerError); + response.Headers.GetValues(SyncHttpHeaders.ErrorCode).ShouldContain(SyncHttpErrorCodes.ServerError); + } + } + + [TestMethod] + public async Task HttpServer_HandleUnexpectedExceptions_AnswersWithDetailsBody() + { + var failure = new SynchronizationException("Unable to Insert item to store for table Posts", new InvalidOperationException("FK_Posts_Users")) + { + Kind = SyncFailureKind.DataError, + TableName = "Posts" + }; + + using var server = SyncTestServer.Create( + new ScriptedSyncProvider { ApplyChanges = _ => throw failure }, + configureOptions: options => options.HandleUnexpectedExceptions = true, + useHostExceptionHandler: true); + + using var response = await server.RawHttpClient.PostAsync($"/api/sync-agent/changes-bulk-complete/{await BeginUploadAsync(server)}", null); + + response.StatusCode.ShouldBe(HttpStatusCode.InternalServerError); + response.Headers.GetValues(SyncHttpHeaders.ErrorCode).ShouldContain(SyncHttpErrorCodes.DataError); + + var details = await response.Content.ReadFromJsonAsync(); + details.ShouldNotBeNull(); + details!.Code.ShouldBe(SyncHttpErrorCodes.DataError); + details.TableName.ShouldBe("Posts"); + details.Message!.ShouldContain("Posts"); + + // The provider's message names the constraint; that stays on the server unless asked for. + details.Detail.ShouldBeNull(); + details.Message!.ShouldNotContain("FK_Posts_Users"); + } + + [TestMethod] + public async Task HttpServer_IncludeExceptionDetails_IsOptIn() + { + var failure = new SynchronizationException("Unable to Insert item", new InvalidOperationException("FK_Posts_Users")); + + using var server = SyncTestServer.Create( + new ScriptedSyncProvider { ApplyChanges = _ => throw failure }, + configureOptions: options => + { + options.HandleUnexpectedExceptions = true; + options.IncludeExceptionDetails = true; + }); + + using var response = await server.RawHttpClient.PostAsync($"/api/sync-agent/changes-bulk-complete/{await BeginUploadAsync(server)}", null); + + var details = await response.Content.ReadFromJsonAsync(); + details.ShouldNotBeNull(); + details!.Detail.ShouldNotBeNull(); + details.Detail!.ShouldContain("FK_Posts_Users"); + } + + [TestMethod] + public async Task HttpServer_AnchorTooOld_IsUnchanged() + { + using var server = SyncTestServer.Create(new ScriptedSyncProvider { GetChanges = _ => throw new SyncAnchorTooOldException("[dbo].[Posts]", 10, 20) }); + + using var response = await server.RawHttpClient.GetAsync($"/api/sync-agent/changes-bulk/{Guid.NewGuid()}"); + + // Clients on 1.0.1 parse exactly this: 410, this header value, this body shape. + response.StatusCode.ShouldBe(HttpStatusCode.Gone); + response.Headers.GetValues(SyncHttpHeaders.ErrorCode).Single().ShouldBe(SyncHttpErrorCodes.AnchorTooOld); + + var error = await response.Content.ReadFromJsonAsync(); + error.ShouldNotBeNull(); + error!.TableName.ShouldBe("[dbo].[Posts]"); + error.RequestedVersion.ShouldBe(10); + error.MinValidVersion.ShouldBe(20); + } + + #endregion + + #region Client: SyncHttpRequestException from a new server + + [TestMethod] + public async Task HttpClient_RebuildsSessionExpiredFromServerSignal() + { + // A server whose cache forgets every upload session the moment it is created: what a client + // meets after a server restart or a cache eviction between two requests of the same sync + using var server = SyncTestServer.Create( + new ScriptedSyncProvider(), + configureServices: services => services.AddSingleton()); + + var changeSet = new SyncChangeSet(new SyncAnchor(Guid.NewGuid(), 1), SyncAnchor.Null, new List()); + + var ex = await Should.ThrowAsync(() => server.HttpSyncProvider.ApplyChangesAsync(changeSet)); + + ex.StatusCode.ShouldBe(HttpStatusCode.Gone); + ex.ErrorCode.ShouldBe(SyncHttpErrorCodes.SessionExpired); + ex.Kind.ShouldBe(SyncFailureKind.SessionExpired); + ex.IsTransient.ShouldBeFalse(); + ex.TraceId.ShouldNotBeNullOrWhiteSpace(); + ex.ServerMessage.ShouldNotBeNullOrWhiteSpace(); + + // Still the exception type and message callers have always received for a failed request + ex.ShouldBeAssignableTo(); + ex.Message.ShouldStartWith("Response status code does not indicate success: 410"); + } + + [TestMethod] + public async Task HttpClient_RebuildsDataErrorWithTableFromServerBody() + { + var failure = new SynchronizationException("Unable to Insert item", new InvalidOperationException("FK")) + { + Kind = SyncFailureKind.DataError, + TableName = "Posts" + }; + + using var server = SyncTestServer.Create( + new ScriptedSyncProvider { ApplyChanges = _ => throw failure }, + configureOptions: options => options.HandleUnexpectedExceptions = true); + + var changeSet = new SyncChangeSet(new SyncAnchor(Guid.NewGuid(), 1), SyncAnchor.Null, new List()); + + var ex = await Should.ThrowAsync(() => server.HttpSyncProvider.ApplyChangesAsync(changeSet)); + + ex.StatusCode.ShouldBe(HttpStatusCode.InternalServerError); + ex.Kind.ShouldBe(SyncFailureKind.DataError); + ex.ErrorCode.ShouldBe(SyncHttpErrorCodes.DataError); + ex.TableName.ShouldBe("Posts"); + ex.IsTransient.ShouldBeFalse(); + ex.Message.ShouldStartWith("Response status code does not indicate success: 500"); + } + + [TestMethod] + public async Task HttpClient_RebuildsDataErrorFromHeadersAloneWhenBodyIsTheHostErrorPage() + { + var failure = new SynchronizationException("Unable to Insert item", new InvalidOperationException("FK")) { TableName = "Posts" }; + + using var server = SyncTestServer.Create(new ScriptedSyncProvider { ApplyChanges = _ => throw failure }, useHostExceptionHandler: true); + + var changeSet = new SyncChangeSet(new SyncAnchor(Guid.NewGuid(), 1), SyncAnchor.Null, new List()); + + var ex = await Should.ThrowAsync(() => server.HttpSyncProvider.ApplyChangesAsync(changeSet)); + + // text/plain body: nothing to parse, the header alone has to carry the classification + ex.Kind.ShouldBe(SyncFailureKind.DataError); + ex.ErrorCode.ShouldBe(SyncHttpErrorCodes.DataError); + ex.TraceId.ShouldNotBeNullOrWhiteSpace(); + ex.TableName.ShouldBeNull(); + } + + [TestMethod] + public async Task HttpClient_StoreId_StillThrowsUnauthorizedAccessException() + { + using var handler = new ScriptedHandler(_ => new HttpResponseMessage(HttpStatusCode.Unauthorized)); + var client = SyncTestServer.CreateClient(handler); + + // Applications re-authenticate on exactly this type; it must not become a SyncHttpRequestException. + var ex = await Should.ThrowAsync(() => client.GetStoreIdAsync()); + ex.GetType().ShouldBe(typeof(UnauthorizedAccessException)); + } + + #endregion + + #region Client: classification against an old server (no X-CoreSync-Error) + + [TestMethod] + public async Task HttpClient_OldServer_BodilessFailureIsStillHttpRequestExceptionClassifiedAsServerError() + { + using var handler = new ScriptedHandler(_ => new HttpResponseMessage(HttpStatusCode.InternalServerError)); + var client = SyncTestServer.CreateClient(handler); + + var ex = await Should.ThrowAsync(() => client.SaveVersionForStoreAsync(Guid.NewGuid(), 1)); + + var sync = ex.ShouldBeOfType(); + sync.Kind.ShouldBe(SyncFailureKind.ServerError); + sync.ErrorCode.ShouldBeNull(); + sync.IsTransient.ShouldBeTrue(); + sync.StatusCode.ShouldBe(HttpStatusCode.InternalServerError); + } + + [TestMethod] + public async Task HttpClient_OldServer_ServiceUnavailableIsServerUnavailable() + { + using var handler = new ScriptedHandler(_ => new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) + { + Content = new StringContent("No agent is currently connected for DataStore 12.", Encoding.UTF8, "text/plain") + }); + var client = SyncTestServer.CreateClient(handler); + + var ex = await Should.ThrowAsync(() => client.SaveVersionForStoreAsync(Guid.NewGuid(), 1)); + + ex.Kind.ShouldBe(SyncFailureKind.ServerUnavailable); + ex.IsTransient.ShouldBeTrue(); + ex.ServerMessage.ShouldBeNull(); + } + + [TestMethod] + public async Task HttpClient_OldServer_HostProblemDetailsBodyDoesNotBreakClassification() + { + using var handler = new ScriptedHandler(_ => new HttpResponseMessage(HttpStatusCode.NotFound) + { + Content = new StringContent("""{"type":"https://tools.ietf.org/html/rfc9110#section-15.5.5","title":"Not Found","status":404}""", Encoding.UTF8, "application/problem+json") + }); + var client = SyncTestServer.CreateClient(handler); + + var ex = await Should.ThrowAsync(() => client.SaveVersionForStoreAsync(Guid.NewGuid(), 1)); + + // A ProblemDetails body deserializes into SyncErrorDetails with defaults; the code must come + // from the header (absent here), not from a body that never meant to carry one. + ex.Kind.ShouldBe(SyncFailureKind.ServerError); + ex.ErrorCode.ShouldBeNull(); + ex.IsTransient.ShouldBeTrue(); + } + + [TestMethod] + public async Task HttpClient_NetworkFailure_StaysAPlainHttpRequestException() + { + using var handler = new ScriptedHandler(_ => throw new HttpRequestException("No such host is known.", new System.Net.Sockets.SocketException(11001))); + var client = SyncTestServer.CreateClient(handler); + + var ex = await Should.ThrowAsync(() => client.GetStoreIdAsync()); + + // Never reached the server: no status, no classification layer, exactly as before + ex.ShouldNotBeOfType(); + ex.StatusCode.ShouldBeNull(); + } + + #endregion + + #region Client: retries + + [TestMethod] + public async Task HttpClient_Download_StopsRetryingWhenServerSaysPermanent() + { + foreach (var binary in new[] { false, true }) + { + var itemRequests = 0; + using var handler = new ScriptedHandler(request => + { + if (request.RequestUri!.AbsolutePath.Contains("/changes-bulk-item")) + { + itemRequests++; + var response = new HttpResponseMessage(HttpStatusCode.Gone); + response.Headers.Add(SyncHttpHeaders.ErrorCode, SyncHttpErrorCodes.SessionExpired); + return response; + } + + return BulkChangeSetResponse(totalChanges: 10); + }); + + var client = SyncTestServer.CreateClient(handler, useBinaryFormat: binary); + + var ex = await Should.ThrowAsync( + () => client.GetChangesAsync(Guid.NewGuid(), null, SyncDirection.DownloadOnly)); + + ex.Kind.ShouldBe(SyncFailureKind.SessionExpired); + itemRequests.ShouldBe(1, $"binary={binary}: a permanent failure must not be retried"); + } + } + + [TestMethod] + public async Task HttpClient_JsonDownload_StillRetriesFailuresWithoutAnErrorCode() + { + var itemRequests = 0; + using var handler = new ScriptedHandler(request => + { + if (request.RequestUri!.AbsolutePath.Contains("/changes-bulk-item")) + { + itemRequests++; + // What a 1.0.1 server (or a proxy) sends for anything unexpected + return new HttpResponseMessage(HttpStatusCode.InternalServerError); + } + + return BulkChangeSetResponse(totalChanges: 10); + }); + + // Polly retries: 1 attempt + MaxRetryAttempts; the delay is Polly's default so keep the count small + var client = SyncTestServer.CreateClient(handler, useBinaryFormat: false, maxRetryAttempts: 1); + + await Should.ThrowAsync(() => client.GetChangesAsync(Guid.NewGuid(), null, SyncDirection.DownloadOnly)); + + itemRequests.ShouldBe(2, "a failure the server did not classify keeps the retries it always had"); + } + + [TestMethod] + public async Task HttpClient_JsonDownload_DoesNotRetryAnchorTooOld() + { + var itemRequests = 0; + using var handler = new ScriptedHandler(request => + { + if (request.RequestUri!.AbsolutePath.Contains("/changes-bulk-item")) + { + itemRequests++; + var response = new HttpResponseMessage(HttpStatusCode.Gone) + { + Content = JsonContent.Create(new SyncAnchorTooOldError { RequestedVersion = 1, MinValidVersion = 5 }) + }; + response.Headers.Add(SyncHttpHeaders.ErrorCode, SyncHttpErrorCodes.AnchorTooOld); + return response; + } + + return BulkChangeSetResponse(totalChanges: 10); + }); + + var client = SyncTestServer.CreateClient(handler, useBinaryFormat: false, maxRetryAttempts: 1); + + await Should.ThrowAsync(() => client.GetChangesAsync(Guid.NewGuid(), null, SyncDirection.DownloadOnly)); + + itemRequests.ShouldBe(1); + } + + #endregion + + #region SyncAgent: Kind and Stage + + [TestMethod] + public async Task SyncAgent_ReportsStageAndKindFromTheHttpClient() + { + using var handler = new ScriptedHandler(request => + { + if (request.RequestUri!.AbsolutePath.EndsWith("/store-id")) + return new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent(Guid.NewGuid().ToString()) }; + + if (request.RequestUri.AbsolutePath.EndsWith("/changes-bulk-begin")) + { + var response = new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) + { + Content = JsonContent.Create(new SyncErrorDetails + { + Code = SyncHttpErrorCodes.AgentUnavailable, + Message = "No agent is currently connected for DataStore 12.", + TraceId = "trace-123" + }) + }; + response.Headers.Add(SyncHttpHeaders.ErrorCode, SyncHttpErrorCodes.AgentUnavailable); + response.Headers.Add(SyncHttpHeaders.TraceId, "trace-123"); + return response; + } + + throw new InvalidOperationException($"unexpected request {request.RequestUri}"); + }); + + var agent = new SyncAgent(new ScriptedSyncProvider(), SyncTestServer.CreateClient(handler)); + + var ex = await Should.ThrowAsync(() => agent.SynchronizeAsync()); + + ex.Stage.ShouldBe(SynchronizationStage.UploadChanges); + ex.Kind.ShouldBe(SyncFailureKind.ServerUnavailable); + ex.ErrorCode.ShouldBe(SyncHttpErrorCodes.AgentUnavailable); + ex.TraceId.ShouldBe("trace-123"); + ex.ServerMessage.ShouldBe("No agent is currently connected for DataStore 12."); + + // Unchanged contract: the inner exception is still an HttpRequestException and the message is + // still built the same way + ex.InnerException.ShouldBeAssignableTo(); + ex.Message.ShouldBe($"Unable to synchronize stores: {ex.InnerException!.Message}"); + + // Mirrored into Data for error reporters that capture it + ex.Data[SynchronizationException.DataKeyKind].ShouldBe("ServerUnavailable"); + ex.Data[SynchronizationException.DataKeyStage].ShouldBe("UploadChanges"); + ex.Data[SynchronizationException.DataKeyTraceId].ShouldBe("trace-123"); + } + + [TestMethod] + public async Task SyncAgent_ClassifiesAnUnreachableServerAsNetwork() + { + using var handler = new ScriptedHandler(_ => throw new HttpRequestException("Connection refused", new System.Net.Sockets.SocketException(10061))); + + var agent = new SyncAgent(new ScriptedSyncProvider(), SyncTestServer.CreateClient(handler)); + + var ex = await Should.ThrowAsync(() => agent.SynchronizeAsync()); + + ex.Stage.ShouldBe(SynchronizationStage.RemoteStoreId); + ex.Kind.ShouldBe(SyncFailureKind.Network); + ex.InnerException.ShouldBeOfType(); + } + + [TestMethod] + public async Task SyncAgent_ClassifiesARequestTimeoutAsNetwork() + { + // HttpClient reports an expired request timeout as TaskCanceledException while the caller's + // token is untouched; that is a connectivity problem, not a cancellation. + using var handler = new ScriptedHandler(_ => throw new TaskCanceledException("The request was canceled due to the configured HttpClient.Timeout of 100 seconds elapsing.", new TimeoutException())); + + var agent = new SyncAgent(new ScriptedSyncProvider(), SyncTestServer.CreateClient(handler)); + + var ex = await Should.ThrowAsync(() => agent.SynchronizeAsync()); + + ex.Kind.ShouldBe(SyncFailureKind.Network); + ex.InnerException.ShouldBeOfType(); + } + + [TestMethod] + public async Task SyncAgent_ClassifiesTheCallersCancellationAsCancelled() + { + using var cts = new CancellationTokenSource(); + var remote = new ScriptedSyncProvider + { + GetStoreId = () => + { + cts.Cancel(); + throw new OperationCanceledException(cts.Token); + } + }; + + var agent = new SyncAgent(new ScriptedSyncProvider(), remote); + + var ex = await Should.ThrowAsync(() => agent.SynchronizeAsync(cancellationToken: cts.Token)); + + ex.Kind.ShouldBe(SyncFailureKind.Cancelled); + ex.Stage.ShouldBe(SynchronizationStage.RemoteStoreId); + } + + [TestMethod] + public async Task SyncAgent_ClassifiesUnauthorizedAndKeepsTheInnerType() + { + var remote = new ScriptedSyncProvider { GetStoreId = () => throw new UnauthorizedAccessException() }; + + var agent = new SyncAgent(new ScriptedSyncProvider(), remote); + + var ex = await Should.ThrowAsync(() => agent.SynchronizeAsync()); + + // This is the branch existing applications take to re-authenticate + ex.InnerException.ShouldBeOfType(); + ex.Kind.ShouldBe(SyncFailureKind.Unauthorized); + } + + [TestMethod] + public async Task SyncAgent_ClassifiesAProviderFailureAsDataErrorWithTable() + { + // What SqlServer/MySql/PostgreSQL providers throw when a change cannot be applied + var local = new ScriptedSyncProvider + { + ApplyChanges = _ => throw new SyncErrorException("An exception occurred during synchronization", + new SynchronizationException("Unable to Insert item to store for table Comments", new InvalidOperationException("FK")) + { + Kind = SyncFailureKind.DataError, + TableName = "Comments" + }) + }; + + var agent = new SyncAgent(local, new ScriptedSyncProvider()); + + var ex = await Should.ThrowAsync(() => agent.SynchronizeAsync()); + + ex.Stage.ShouldBe(SynchronizationStage.ApplyChangesLocally); + ex.Kind.ShouldBe(SyncFailureKind.DataError); + ex.TableName.ShouldBe("Comments"); + ex.InnerException.ShouldBeOfType(); + } + + [TestMethod] + public async Task SyncAgent_TreatsAnOlderProvidersSynchronizationExceptionAsDataError() + { + // A provider package that predates Kind leaves it Unknown; the type alone has to count + var local = new ScriptedSyncProvider + { + ApplyChanges = _ => throw new SynchronizationException("Unable to Insert item to store for table Comments", new InvalidOperationException("FK")) + }; + + var agent = new SyncAgent(local, new ScriptedSyncProvider()); + + var ex = await Should.ThrowAsync(() => agent.SynchronizeAsync()); + + ex.Kind.ShouldBe(SyncFailureKind.DataError); + ex.TableName.ShouldBeNull(); + } + + [TestMethod] + public async Task SyncAgent_ClassifiesAnchorTooOldThroughTheInterface() + { + var remote = new ScriptedSyncProvider { GetChanges = _ => throw new SyncAnchorTooOldException("[dbo].[Posts]", 1, 5) }; + + var agent = new SyncAgent(new ScriptedSyncProvider(), remote); + + var ex = await Should.ThrowAsync(() => agent.SynchronizeAsync()); + + ex.Kind.ShouldBe(SyncFailureKind.AnchorTooOld); + ex.Stage.ShouldBe(SynchronizationStage.DownloadChanges); + ex.TableName.ShouldBe("[dbo].[Posts]"); + ex.InnerException.ShouldBeOfType(); + } + + [TestMethod] + public async Task SyncAgent_LeavesAnUnrecognisedFailureUnknownButStaged() + { + var local = new ScriptedSyncProvider { GetChanges = _ => throw new InvalidOperationException("database is locked") }; + + var agent = new SyncAgent(local, new ScriptedSyncProvider()); + + var ex = await Should.ThrowAsync(() => agent.SynchronizeAsync()); + + ex.Kind.ShouldBe(SyncFailureKind.Unknown); + ex.Stage.ShouldBe(SynchronizationStage.LocalChanges); + ex.Data.Contains(SynchronizationException.DataKeyKind).ShouldBeFalse(); + ex.Data[SynchronizationException.DataKeyStage].ShouldBe("LocalChanges"); + } + + #endregion + + #region Error code table + + [TestMethod] + public void ErrorCodes_PermanentAndKindMappingsAreStable() + { + SyncHttpErrorCodes.IsPermanent(SyncHttpErrorCodes.AnchorTooOld).ShouldBeTrue(); + SyncHttpErrorCodes.IsPermanent(SyncHttpErrorCodes.SessionExpired).ShouldBeTrue(); + SyncHttpErrorCodes.IsPermanent(SyncHttpErrorCodes.DataError).ShouldBeTrue(); + SyncHttpErrorCodes.IsPermanent(SyncHttpErrorCodes.ConfigurationError).ShouldBeTrue(); + SyncHttpErrorCodes.IsPermanent(SyncHttpErrorCodes.EndpointNotFound).ShouldBeTrue(); + SyncHttpErrorCodes.IsPermanent(SyncHttpErrorCodes.AgentUnavailable).ShouldBeFalse(); + SyncHttpErrorCodes.IsPermanent(SyncHttpErrorCodes.ServerError).ShouldBeFalse(); + + // A code this version does not know, or no code at all, must never stop a retry + SyncHttpErrorCodes.IsPermanent("something-newer").ShouldBeFalse(); + SyncHttpErrorCodes.IsPermanent(null).ShouldBeFalse(); + + SyncHttpErrorCodes.ToFailureKind(null).ShouldBe(SyncFailureKind.Unknown); + SyncHttpErrorCodes.ToFailureKind("something-newer").ShouldBe(SyncFailureKind.ServerError); + SyncHttpErrorCodes.ToFailureKind(SyncHttpErrorCodes.AgentUnavailable).ShouldBe(SyncFailureKind.ServerUnavailable); + SyncHttpErrorCodes.ToFailureKind(SyncHttpErrorCodes.EndpointNotFound).ShouldBe(SyncFailureKind.ConfigurationError); + } + + #endregion + + #region Helpers + + private static async Task BeginUploadAsync(SyncTestServer server) + { + var sessionId = Guid.NewGuid(); + var begin = await server.RawHttpClient.PostAsJsonAsync("/api/sync-agent/changes-bulk-begin", new BulkSyncChangeSet + { + SessionId = sessionId, + SourceAnchor = new SyncAnchor(Guid.NewGuid(), 1), + TargetAnchor = SyncAnchor.Null + }); + begin.EnsureSuccessStatusCode(); + return sessionId; + } + + /// + /// A cache that accepts every entry and remembers none, so that the very next request of a + /// synchronization finds its session gone. + /// + private sealed class ForgetfulMemoryCache : Microsoft.Extensions.Caching.Memory.IMemoryCache + { + private readonly Microsoft.Extensions.Caching.Memory.MemoryCache _entries = new(new Microsoft.Extensions.Caching.Memory.MemoryCacheOptions()); + + public Microsoft.Extensions.Caching.Memory.ICacheEntry CreateEntry(object key) => _entries.CreateEntry(key); + + public void Remove(object key) => _entries.Remove(key); + + public bool TryGetValue(object key, out object? value) + { + value = null; + return false; + } + + public void Dispose() => _entries.Dispose(); + } + + private static HttpResponseMessage BulkChangeSetResponse(int totalChanges) + => new(HttpStatusCode.OK) + { + Content = JsonContent.Create(new BulkSyncChangeSet + { + SessionId = Guid.NewGuid(), + TotalChanges = totalChanges, + SourceAnchor = new SyncAnchor(Guid.NewGuid(), 1), + TargetAnchor = new SyncAnchor(Guid.NewGuid(), 1) + }) + }; + + /// + /// An that answers from a delegate: the stand-in for servers this + /// code base does not host (older versions, proxies) and for the network itself. + /// + private sealed class ScriptedHandler : HttpMessageHandler + { + private readonly Func _respond; + + public ScriptedHandler(Func respond) => _respond = respond; + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + => Task.FromResult(_respond(request)); + } + + /// + /// A provider whose every operation succeeds trivially unless a test scripts it to fail, so the + /// HTTP layer and the agent can be driven through each stage without a database. + /// + private sealed class ScriptedSyncProvider : ISyncProvider + { + private readonly Guid _storeId = Guid.NewGuid(); + + public Func? GetStoreId { get; set; } + public Func? GetChanges { get; set; } + public Func? ApplyChanges { get; set; } + + public string[]? SyncTableNames => null; + + public Task GetStoreIdAsync(CancellationToken cancellationToken = default) + => Task.FromResult(GetStoreId?.Invoke() ?? _storeId); + + public Task GetChangesAsync(Guid otherStoreId, SyncFilterParameter[]? syncFilterParameters = null, SyncDirection syncDirection = SyncDirection.UploadAndDownload, string[]? tables = null, CancellationToken cancellationToken = default) + => Task.FromResult(GetChanges?.Invoke(otherStoreId) + ?? new SyncChangeSet(new SyncAnchor(_storeId, 1), new SyncAnchor(otherStoreId, 1), new List())); + + public Task ApplyChangesAsync(SyncChangeSet changeSet, Func? onConflictFunc = null, CancellationToken cancellationToken = default) + => Task.FromResult(ApplyChanges?.Invoke(changeSet) ?? new SyncAnchor(_storeId, 1)); + + public Task SaveVersionForStoreAsync(Guid otherStoreId, long version, CancellationToken cancellationToken = default) + => Task.CompletedTask; + + public Task ApplyProvisionAsync(CancellationToken cancellationToken = default) + => Task.CompletedTask; + + public Task RemoveProvisionAsync(CancellationToken cancellationToken = default) + => Task.CompletedTask; + + public Task GetSyncVersionAsync(CancellationToken cancellationToken = default) + => Task.FromResult(new SyncVersion(1, 0)); + + public Task ApplyRetentionPolicyAsync(int minVersion, CancellationToken cancellationToken = default) + => throw new NotSupportedException(); + + public Task EnableChangeTrackingForTable(string tableName, CancellationToken cancellationToken = default) + => Task.CompletedTask; + + public Task DisableChangeTrackingForTable(string tableName, CancellationToken cancellationToken = default) + => Task.CompletedTask; + } + + #endregion +} diff --git a/src/CoreSync.Tests/SyncTestServer.cs b/src/CoreSync.Tests/SyncTestServer.cs index 03eeee1..8a5ec5b 100644 --- a/src/CoreSync.Tests/SyncTestServer.cs +++ b/src/CoreSync.Tests/SyncTestServer.cs @@ -2,6 +2,7 @@ using CoreSync.Http.Server; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.TestHost; using Microsoft.Extensions.DependencyInjection; using System; @@ -16,6 +17,12 @@ namespace CoreSync.Tests; /// internal sealed class SyncTestServer : IDisposable { + /// + /// Body the simulated host error page writes for an unhandled exception, so that tests can tell + /// a response produced by the host from one produced by the sync endpoints. + /// + public const string HostErrorPageBody = "host error page"; + private readonly WebApplication _app; private readonly HttpClient _httpClient; private readonly ServiceProvider _clientServiceProvider; @@ -36,7 +43,24 @@ private SyncTestServer(WebApplication app, HttpClient httpClient, ServiceProvide HttpSyncProvider = httpSyncProvider; } - public static SyncTestServer Create(ISyncProvider remoteSyncProvider, bool useBinaryFormat = false) + /// The provider the endpoints are backed by. + /// Whether the client uses the message pack endpoints. + /// Optional configuration. + /// + /// When true the app gets an exception handler that answers 500 with , + /// the way a production host does; without it the test server surfaces unhandled exceptions + /// straight to the calling test, which is convenient but is not what a client ever sees. + /// + /// + /// Optional hook run after the CoreSync services are registered, for tests that need to replace + /// one of them (for example the memory cache that holds upload/download sessions). + /// + public static SyncTestServer Create( + ISyncProvider remoteSyncProvider, + bool useBinaryFormat = false, + Action? configureOptions = null, + bool useHostExceptionHandler = false, + Action? configureServices = null) { // Build and start the server var builder = WebApplication.CreateBuilder(); @@ -44,9 +68,21 @@ public static SyncTestServer Create(ISyncProvider remoteSyncProvider, bool useBi builder.Services.AddCoreSyncHttpServer(); builder.Services.AddSingleton(remoteSyncProvider); + configureServices?.Invoke(builder.Services); var app = builder.Build(); - app.UseCoreSyncHttpServer(); + + if (useHostExceptionHandler) + { + app.UseExceptionHandler(errorApp => errorApp.Run(context => + { + context.Response.StatusCode = StatusCodes.Status500InternalServerError; + context.Response.ContentType = "text/plain"; + return context.Response.WriteAsync(HostErrorPageBody); + })); + } + + app.UseCoreSyncHttpServer(optionsConfigure: configureOptions); app.StartAsync().GetAwaiter().GetResult(); var httpClient = app.GetTestServer().CreateClient(); @@ -65,6 +101,26 @@ public static SyncTestServer Create(ISyncProvider remoteSyncProvider, bool useBi return new SyncTestServer(app, httpClient, clientServiceProvider, httpSyncProvider); } + /// + /// Builds an over a scripted + /// instead of a real server, to simulate servers this code base does not host: older versions that + /// send no error signal, proxies, an unreachable network. + /// + public static ISyncProviderHttpClient CreateClient(HttpMessageHandler handler, bool useBinaryFormat = false, int maxRetryAttempts = 3) + { + var httpClient = new HttpClient(handler) { BaseAddress = new Uri("http://sync.test/") }; + + var clientServices = new ServiceCollection(); + clientServices.AddSingleton(new SingleClientFactory(httpClient)); + clientServices.AddCoreSyncHttpClient(options => + { + options.UseBinaryFormat = useBinaryFormat; + options.MaxRetryAttempts = maxRetryAttempts; + }); + + return clientServices.BuildServiceProvider().GetRequiredService(); + } + public void Dispose() { _clientServiceProvider.Dispose(); diff --git a/src/CoreSync/ISyncFailureInfo.cs b/src/CoreSync/ISyncFailureInfo.cs new file mode 100644 index 0000000..7ffe206 --- /dev/null +++ b/src/CoreSync/ISyncFailureInfo.cs @@ -0,0 +1,44 @@ +namespace CoreSync +{ + /// + /// Implemented by exceptions that know why a synchronization step failed, so that + /// can copy the classification onto the + /// it raises without depending on the transport or provider that detected the failure. + /// + /// + /// walks the inner exception chain of whatever it catches + /// and uses the first implementation it finds. Transport packages implement it on their own + /// exception types (for example the HTTP client's SyncHttpRequestException) so the agent, + /// which targets .NET Standard 2.0, never has to inspect HTTP status codes itself. + /// + public interface ISyncFailureInfo + { + /// + /// Gets the classification of the failure. + /// + SyncFailureKind Kind { get; } + + /// + /// Gets the machine-readable error code reported by the remote side, when it reported one + /// (see CoreSync.Http.SyncHttpErrorCodes), otherwise null. + /// + string? ErrorCode { get; } + + /// + /// Gets the table involved in the failure, when known, otherwise null. + /// + string? TableName { get; } + + /// + /// Gets an identifier the remote side attached to the failure so that it can be found in its + /// logs, when it attached one, otherwise null. + /// + string? TraceId { get; } + + /// + /// Gets a short, user-safe message the remote side attached to the failure, when it attached + /// one, otherwise null. + /// + string? ServerMessage { get; } + } +} diff --git a/src/CoreSync/SyncAgent.cs b/src/CoreSync/SyncAgent.cs index d3bb615..ff143e0 100644 --- a/src/CoreSync/SyncAgent.cs +++ b/src/CoreSync/SyncAgent.cs @@ -65,10 +65,26 @@ public SyncAgent(ISyncProviderBase localSyncProvider, ISyncProviderBase remoteSy /// Thrown when synchronization fails, with the underlying failure as /// . /// - /// An inner means one of the stores can no longer - /// resolve changes from the other's anchor, because the change history covering it has been - /// discarded by its retention policy. That is permanent: the affected store has to be - /// reinitialized from a fresh snapshot, and retrying will not help. + /// says why it failed as far as the failing component + /// could tell (offline, unauthorized, server-side data error, ...) and + /// says which step was running. Both are additive: + /// the message and the inner exception are the same as before they existed. + /// + /// catch (SynchronizationException ex) when (ex.Kind == SyncFailureKind.Network) + /// { + /// // the device is offline: ask the user to check connectivity and try again + /// } + /// catch (SynchronizationException ex) + /// { + /// // ex.Kind, ex.Stage, ex.TableName, ex.ErrorCode, ex.TraceId, ex.ServerMessage + /// } + /// + /// + /// + /// An inner () + /// means one of the stores can no longer resolve changes from the other's anchor, because the + /// change history covering it has been discarded by its retention policy. That is permanent: + /// the affected store has to be reinitialized from a fresh snapshot, and retrying will not help. /// /// catch (SynchronizationException ex) /// when (ex.InnerException is SyncAnchorTooOldException tooOld) @@ -85,20 +101,35 @@ public async Task SynchronizeAsync( SyncFilterParameter[]? remoteSyncFilterParameters = null, SyncFilterParameter[]? localSyncFilterParameters = null) { + var stage = SynchronizationStage.Unknown; + try { remoteConflictResolutionFunc ??= ((_) => ConflictResolution.Skip); localConflictResolutionFunc ??= ((_) => ConflictResolution.ForceWrite); + stage = SynchronizationStage.LocalStoreId; var localStoreId = await LocalSyncProvider.GetStoreIdAsync(cancellationToken); + + stage = SynchronizationStage.RemoteStoreId; var remoteStoreId = await RemoteSyncProvider.GetStoreIdAsync(cancellationToken); + stage = SynchronizationStage.LocalChanges; var localChangeSet = await LocalSyncProvider.GetChangesAsync(remoteStoreId, localSyncFilterParameters, SyncDirection.UploadOnly, RemoteSyncProvider.SyncTableNames, cancellationToken: cancellationToken); + + stage = SynchronizationStage.UploadChanges; await RemoteSyncProvider.ApplyChangesAsync(localChangeSet, remoteConflictResolutionFunc, cancellationToken: cancellationToken); + + stage = SynchronizationStage.SaveLocalVersion; await LocalSyncProvider.SaveVersionForStoreAsync(remoteStoreId, localChangeSet.SourceAnchor.Version, cancellationToken: cancellationToken); + stage = SynchronizationStage.DownloadChanges; var remoteChangeSet = await RemoteSyncProvider.GetChangesAsync(localStoreId, remoteSyncFilterParameters, SyncDirection.DownloadOnly, LocalSyncProvider.SyncTableNames, cancellationToken: cancellationToken); + + stage = SynchronizationStage.ApplyChangesLocally; await LocalSyncProvider.ApplyChangesAsync(remoteChangeSet, localConflictResolutionFunc, cancellationToken: cancellationToken); + + stage = SynchronizationStage.SaveRemoteVersion; await RemoteSyncProvider.SaveVersionForStoreAsync(localStoreId, remoteChangeSet.SourceAnchor.Version, cancellationToken: cancellationToken); //await LocalSyncProvider.ApplyProvisionAsync(cancellationToken: cancellationToken); @@ -112,7 +143,120 @@ public async Task SynchronizeAsync( // escape unwrapped would break them at runtime with nothing to catch it at compile // time. The typed exception stays reachable as the inner exception, which is what // callers branch on to tell "reinitialize this client" from a transient failure. - throw new SynchronizationException("Unable to synchronize stores", ex); + // The message and the inner exception are unchanged; the classification is additive. + throw Classify(new SynchronizationException("Unable to synchronize stores", ex), ex, stage, cancellationToken); + } + } + + /// + /// Fills the classification properties of from what the failing + /// component was able to say about . + /// + /// + /// Best effort by design: anything implementing in the inner + /// exception chain wins, because it was produced by the component that saw the failure (the + /// HTTP client reading the server's error code, a provider naming the table it could not + /// write). Without one, the exception types that reliably mean one thing are mapped - a + /// cancelled token, an unreachable server, refused credentials - and everything else stays + /// , which is what callers had before. + /// + private static SynchronizationException Classify( + SynchronizationException result, + Exception failure, + SynchronizationStage stage, + CancellationToken cancellationToken) + { + result.Stage = stage; + + ISyncFailureInfo? info = null; + var sawProviderFailure = false; + var sawCancellation = false; + var sawHttpFailure = false; + var sawUnauthorized = false; + + foreach (var inner in Flatten(failure)) + { + if (info == null && inner is ISyncFailureInfo candidate && candidate.Kind != SyncFailureKind.Unknown) + { + info = candidate; + } + + // A provider raises SynchronizationException for a change it could not apply (constraint + // violation, missing table...). Older provider packages do not set Kind on it, so the type + // alone has to count as a data error. + sawProviderFailure |= inner is SynchronizationException; + sawCancellation |= inner is OperationCanceledException; + sawHttpFailure |= inner is System.Net.Http.HttpRequestException; + sawUnauthorized |= inner is UnauthorizedAccessException; + + if (result.TableName == null && inner is ISyncFailureInfo withTable && withTable.TableName != null) + { + result.TableName = withTable.TableName; + } + } + + if (info != null) + { + result.Kind = info.Kind; + result.ErrorCode = info.ErrorCode; + result.TraceId = info.TraceId; + result.ServerMessage = info.ServerMessage; + return result; + } + + if (sawCancellation) + { + // The caller asked to stop. Otherwise a cancellation nobody requested is a timeout: + // HttpClient reports an expired request timeout as TaskCanceledException. + result.Kind = cancellationToken.IsCancellationRequested + ? SyncFailureKind.Cancelled + : IsRemoteStage(stage) ? SyncFailureKind.Network : SyncFailureKind.Unknown; + } + else if (sawUnauthorized) + { + result.Kind = SyncFailureKind.Unauthorized; + } + else if (sawHttpFailure) + { + // An HttpRequestException that does not implement ISyncFailureInfo never carried a + // response: the connection failed, the name did not resolve, TLS failed. The server + // was not reached. + result.Kind = SyncFailureKind.Network; + } + else if (sawProviderFailure) + { + result.Kind = SyncFailureKind.DataError; + } + + return result; + } + + private static bool IsRemoteStage(SynchronizationStage stage) + => stage == SynchronizationStage.RemoteStoreId + || stage == SynchronizationStage.UploadChanges + || stage == SynchronizationStage.DownloadChanges + || stage == SynchronizationStage.SaveRemoteVersion; + + private static IEnumerable Flatten(Exception exception) + { + var current = exception; + while (current != null) + { + if (current is AggregateException aggregate) + { + foreach (var inner in aggregate.InnerExceptions) + { + foreach (var nested in Flatten(inner)) + { + yield return nested; + } + } + + yield break; + } + + yield return current; + current = current.InnerException; } } } diff --git a/src/CoreSync/SyncAnchorTooOldException.cs b/src/CoreSync/SyncAnchorTooOldException.cs index b105954..7fa1124 100644 --- a/src/CoreSync/SyncAnchorTooOldException.cs +++ b/src/CoreSync/SyncAnchorTooOldException.cs @@ -25,7 +25,7 @@ namespace CoreSync /// on rather than catching this type directly. /// /// - public class SyncAnchorTooOldException : InvalidOperationException + public class SyncAnchorTooOldException : InvalidOperationException, ISyncFailureInfo { /// /// Initializes a new instance of the class. @@ -94,6 +94,18 @@ public SyncAnchorTooOldException(long requestedVersion, long minValidVersion) /// public long MinValidVersion { get; } + /// + /// Always : lets classify the + /// failure without a type check. + /// + SyncFailureKind ISyncFailureInfo.Kind => SyncFailureKind.AnchorTooOld; + + string? ISyncFailureInfo.ErrorCode => null; + + string? ISyncFailureInfo.TraceId => null; + + string? ISyncFailureInfo.ServerMessage => null; + private static string BuildMessage(string? tableName, long requestedVersion, long minValidVersion) { var requested = requestedVersion < 0 diff --git a/src/CoreSync/SyncFailureKind.cs b/src/CoreSync/SyncFailureKind.cs new file mode 100644 index 0000000..7834926 --- /dev/null +++ b/src/CoreSync/SyncFailureKind.cs @@ -0,0 +1,79 @@ +namespace CoreSync +{ + /// + /// Broad classification of why a synchronization failed, meant to drive what an application tells + /// the user and whether retrying makes sense. + /// + /// + /// The value is best effort: it is derived from whatever the failing component could tell + /// (a typed exception, an X-CoreSync-Error header from a server that emits it, or the HTTP + /// status code and exception type when talking to an older server). means + /// nothing better than "it failed" is known, which is exactly what callers had before this + /// classification existed. + /// + public enum SyncFailureKind + { + /// + /// No classification is available. + /// + Unknown = 0, + + /// + /// The remote store could not be reached or the connection dropped or timed out before a + /// response arrived. Typically the device is offline; retrying later is appropriate. + /// + Network, + + /// + /// The server refused the credentials (HTTP 401/403). Retrying without re-authenticating + /// cannot succeed. + /// + Unauthorized, + + /// + /// The caller cancelled the operation through its . + /// + Cancelled, + + /// + /// One of the stores can no longer resolve changes from the other's anchor + /// (). The affected store must be reinitialized from a + /// fresh snapshot; retrying cannot help. + /// + AnchorTooOld, + + /// + /// The server no longer holds the upload or download session this synchronization was using + /// (it was evicted, the server restarted, or the client took too long). Nothing is wrong with + /// the data: starting a new synchronization is the fix. + /// + SessionExpired, + + /// + /// The server was reached but cannot serve requests right now (HTTP 502/503/504, or the + /// server's data store agent is offline). Retrying later is appropriate. + /// + ServerUnavailable, + + /// + /// A store refused the changes: a constraint violation, a missing table or column, a value + /// that could not be converted. Retrying the same changes cannot succeed; the problem needs + /// someone to look at the data or the schema. + /// + DataError, + + /// + /// The synchronization endpoint or store is not configured in a way that can be served (the + /// endpoint does not exist or is unpublished, a table has no primary key, ...). Needs an + /// administrator; retrying cannot help. + /// + ConfigurationError, + + /// + /// The server answered with an error it did not classify further (an unexpected HTTP 5xx, or + /// a 4xx from a server that does not emit X-CoreSync-Error). The device is online and + /// the problem is on the server side. + /// + ServerError + } +} diff --git a/src/CoreSync/SynchronizationException.cs b/src/CoreSync/SynchronizationException.cs index 00d6cdb..145ff41 100644 --- a/src/CoreSync/SynchronizationException.cs +++ b/src/CoreSync/SynchronizationException.cs @@ -5,8 +5,65 @@ namespace CoreSync { - public class SynchronizationException : Exception + /// + /// Raised by for every failure, with the underlying error + /// as , and by providers when a single change cannot be + /// applied to a store. + /// + /// + /// + /// The message and the inner exception are what they have always been; applications that branch on + /// (for example is UnauthorizedAccessException) keep + /// working. The classification properties (, , + /// , , , + /// ) are additional and default to "not known". + /// + /// + /// Every classification value is mirrored into under a + /// CoreSync.* key ( and siblings), so error reporters that capture + /// exception data (Sentry, Application Insights, structured loggers) show it without any + /// application code. + /// + /// + public class SynchronizationException : Exception, ISyncFailureInfo { + /// + /// Key under which is mirrored into . + /// + public const string DataKeyKind = "CoreSync.Kind"; + + /// + /// Key under which is mirrored into . + /// + public const string DataKeyStage = "CoreSync.Stage"; + + /// + /// Key under which is mirrored into . + /// + public const string DataKeyErrorCode = "CoreSync.ErrorCode"; + + /// + /// Key under which is mirrored into . + /// + public const string DataKeyTableName = "CoreSync.TableName"; + + /// + /// Key under which is mirrored into . + /// + public const string DataKeyTraceId = "CoreSync.TraceId"; + + /// + /// Key under which is mirrored into . + /// + public const string DataKeyServerMessage = "CoreSync.ServerMessage"; + + private SyncFailureKind _kind; + private SynchronizationStage _stage; + private string? _errorCode; + private string? _tableName; + private string? _traceId; + private string? _serverMessage; + public SynchronizationException() { } @@ -22,5 +79,108 @@ public SynchronizationException(string message, Exception innerException) : base protected SynchronizationException(SerializationInfo info, StreamingContext context) : base(info, context) { } + + /// + /// Gets or sets why the synchronization failed. when the + /// failing component did not say. + /// + public SyncFailureKind Kind + { + get => _kind; + set + { + _kind = value; + SetData(DataKeyKind, value == SyncFailureKind.Unknown ? null : value.ToString()); + } + } + + /// + /// Gets or sets the step of that was executing when + /// the failure occurred. when the exception was not + /// raised by the agent. + /// + public SynchronizationStage Stage + { + get => _stage; + set + { + _stage = value; + SetData(DataKeyStage, value == SynchronizationStage.Unknown ? null : value.ToString()); + } + } + + /// + /// Gets or sets the machine-readable error code reported by the remote side, when any + /// (see CoreSync.Http.SyncHttpErrorCodes). + /// + public string? ErrorCode + { + get => _errorCode; + set + { + _errorCode = value; + SetData(DataKeyErrorCode, value); + } + } + + /// + /// Gets or sets the table involved in the failure, when known. + /// + public string? TableName + { + get => _tableName; + set + { + _tableName = value; + SetData(DataKeyTableName, value); + } + } + + /// + /// Gets or sets an identifier the remote side attached to the failure so that support can find + /// it in the server's logs, when any. + /// + public string? TraceId + { + get => _traceId; + set + { + _traceId = value; + SetData(DataKeyTraceId, value); + } + } + + /// + /// Gets or sets a short, user-safe message the remote side attached to the failure, when any. + /// + public string? ServerMessage + { + get => _serverMessage; + set + { + _serverMessage = value; + SetData(DataKeyServerMessage, value); + } + } + + private void SetData(string key, string? value) + { + // Exception.Data can legitimately be unavailable (a derived type overriding it with null); + // the classification must never be the thing that throws while reporting a failure. + var data = Data; + if (data == null || data.IsReadOnly) + { + return; + } + + if (value == null) + { + data.Remove(key); + } + else + { + data[key] = value; + } + } } } diff --git a/src/CoreSync/SynchronizationStage.cs b/src/CoreSync/SynchronizationStage.cs new file mode 100644 index 0000000..9e4380f --- /dev/null +++ b/src/CoreSync/SynchronizationStage.cs @@ -0,0 +1,60 @@ +namespace CoreSync +{ + /// + /// The step of that was executing when a synchronization + /// failed. Reported through . + /// + /// + /// Named SynchronizationStage rather than SyncStage on purpose: CoreSync.Http.Client + /// already has a SyncStage progress enum, and applications commonly import both namespaces. + /// + public enum SynchronizationStage + { + /// + /// The stage is not known (the failure was raised outside , or before + /// the first step started). + /// + Unknown = 0, + + /// + /// Reading the local store's identifier. + /// + LocalStoreId, + + /// + /// Reading the remote store's identifier. Over HTTP this is the first request of a + /// synchronization, so connectivity and authentication problems usually surface here. + /// + RemoteStoreId, + + /// + /// Computing the local changes to upload. + /// + LocalChanges, + + /// + /// Applying the local changes to the remote store (upload). + /// + UploadChanges, + + /// + /// Recording on the local store the version that has been uploaded. + /// + SaveLocalVersion, + + /// + /// Retrieving the remote changes (download). + /// + DownloadChanges, + + /// + /// Applying the downloaded changes to the local store. + /// + ApplyChangesLocally, + + /// + /// Recording on the remote store the version that has been downloaded. + /// + SaveRemoteVersion + } +} From 2cc313db0d80f8bd35cab8baf249b17a8cf0be19 Mon Sep 17 00:00:00 2001 From: Adolfo Marinucci Date: Thu, 15 Oct 2026 15:14:04 +0200 Subject: [PATCH 2/4] Tighten failure classification for mixed versions and tidy HTTP response handling - SyncAgent no longer reports every plain HttpRequestException as Network: an older CoreSync.Http.Client throws that type from EnsureSuccessStatusCode for a 401/404/500 too, so the kind is Network only when the exception carries no status (read through reflection, since .NET Standard 2.0 has no HttpRequestException.StatusCode) and Unknown when it does or when the runtime cannot tell. - UnauthorizedAccessException maps to Unauthorized only on remote stages; a local store raising it for a read-only database file stays Unknown. - New agent-error code (not permanent, kind ServerError) for a data store agent that answered a request with a failure. - The HTTP client disposes every response it reads and asks for headers first where it used to rely on GetFromJsonAsync; the server registers its OnStarting headers only while the response has not started and documents that a bare Kestrel 500 carries none. --- .../Implementation/SyncProviderHttpClient.cs | 19 ++--- .../WebApplicationExtensions.cs | 37 +++++---- src/CoreSync.Http/SyncHttpErrorCodes.cs | 13 ++++ .../SyncErrorClassificationTests.cs | 78 +++++++++++++++++++ src/CoreSync/SyncAgent.cs | 48 ++++++++++-- 5 files changed, 164 insertions(+), 31 deletions(-) diff --git a/src/CoreSync.Http.Client/Implementation/SyncProviderHttpClient.cs b/src/CoreSync.Http.Client/Implementation/SyncProviderHttpClient.cs index 009c390..7cfd96e 100644 --- a/src/CoreSync.Http.Client/Implementation/SyncProviderHttpClient.cs +++ b/src/CoreSync.Http.Client/Implementation/SyncProviderHttpClient.cs @@ -201,7 +201,7 @@ await httpClient.PostAsync($"{_options.SyncControllerRoute}/changes-bulk-item", SyncProgress?.Invoke(this, new SyncProgressEventArgs(SyncStage.ApplyChanges, skip / (double)changeSet.Items.Count)); } - var remoteChangeSetResponse = await httpClient.PostAsync( + using var remoteChangeSetResponse = await httpClient.PostAsync( $"{_options.SyncControllerRoute}/changes-bulk-complete{(_options.UseBinaryFormat ? "-binary" : string.Empty)}/{sessionId}", null, cancellationToken); await EnsureSyncSuccessAsync(remoteChangeSetResponse, cancellationToken); @@ -223,7 +223,7 @@ public async Task GetChangesAsync(Guid otherStoreId, SyncFilterPa request.Headers.Add(SyncHttpHeaders.TablesCount, tables.Length.ToString()); } - var response = await httpClient.SendAsync(request, cancellationToken); + using var response = await httpClient.SendAsync(request, cancellationToken); await EnsureSyncSuccessAsync(response, cancellationToken); var bulkSyncChangeSet = await response.Content.ReadFromJsonAsync(cancellationToken) @@ -263,7 +263,9 @@ args.Outcome.Exception is { } exception { await pipeline.ExecuteAsync(async (token) => { - var response = await httpClient.GetAsync($"/{_options.SyncControllerRoute}/changes-bulk-item/{bulkSyncChangeSet.SessionId}/{skip}/{_options.BulkItemSize}", token); + // Headers first and disposed when done, as GetFromJsonAsync did before the response had + // to be inspected for the error signal. + using var response = await httpClient.GetAsync($"/{_options.SyncControllerRoute}/changes-bulk-item/{bulkSyncChangeSet.SessionId}/{skip}/{_options.BulkItemSize}", HttpCompletionOption.ResponseHeadersRead, token); await EnsureSyncSuccessAsync(response, token); @@ -301,7 +303,7 @@ private async Task DownloadBulkChangeSetBinary(BulkSyncChangeSet try { - var response = await httpClient.GetAsync($"{_options.SyncControllerRoute}/changes-bulk-item-binary/{bulkSyncChangeSet.SessionId}/{skip}/{_options.BulkItemSize}", cancellationToken); + using var response = await httpClient.GetAsync($"{_options.SyncControllerRoute}/changes-bulk-item-binary/{bulkSyncChangeSet.SessionId}/{skip}/{_options.BulkItemSize}", HttpCompletionOption.ResponseHeadersRead, cancellationToken); await EnsureSyncSuccessAsync(response, cancellationToken); @@ -384,7 +386,7 @@ private static void ConvertJsonValueToNetObject(SyncChangeSet changeSet) public async Task GetStoreIdAsync(CancellationToken cancellationToken = default) { var httpClient = _httpClientFactory.CreateClient(_options.HttpClientName ?? Options.DefaultName); - var res = await httpClient.GetAsync($"/{_options.SyncControllerRoute}/store-id", cancellationToken); + using var res = await httpClient.GetAsync($"/{_options.SyncControllerRoute}/store-id", cancellationToken); // Kept ahead of the generic handling on purpose: applications have been catching this exact // type around SynchronizeAsync to trigger a re-authentication. @@ -399,15 +401,14 @@ public async Task GetStoreIdAsync(CancellationToken cancellationToken = de public async Task SaveVersionForStoreAsync(Guid otherStoreId, long version, CancellationToken cancellationToken = default) { var httpClient = _httpClientFactory.CreateClient(_options.HttpClientName ?? Options.DefaultName); - await EnsureSyncSuccessAsync( - await httpClient.PostAsync($"/{_options.SyncControllerRoute}/save-version/{otherStoreId}/{version}", null, cancellationToken), - cancellationToken); + using var response = await httpClient.PostAsync($"/{_options.SyncControllerRoute}/save-version/{otherStoreId}/{version}", null, cancellationToken); + await EnsureSyncSuccessAsync(response, cancellationToken); } public async Task GetSyncVersionAsync(CancellationToken cancellationToken = default) { var httpClient = _httpClientFactory.CreateClient(_options.HttpClientName ?? Options.DefaultName); - var response = await httpClient.GetAsync($"/{_options.SyncControllerRoute}/sync-version", cancellationToken); + using var response = await httpClient.GetAsync($"/{_options.SyncControllerRoute}/sync-version", HttpCompletionOption.ResponseHeadersRead, cancellationToken); await EnsureSyncSuccessAsync(response, cancellationToken); return await response.Content.ReadFromJsonAsync(cancellationToken) ?? throw new InvalidOperationException(); diff --git a/src/CoreSync.Http.Server/WebApplicationExtensions.cs b/src/CoreSync.Http.Server/WebApplicationExtensions.cs index 1486a8c..98bd227 100644 --- a/src/CoreSync.Http.Server/WebApplicationExtensions.cs +++ b/src/CoreSync.Http.Server/WebApplicationExtensions.cs @@ -158,10 +158,14 @@ internal static class SyncErrorFilterEndpointExtensions /// /// /// Anything else keeps propagating to the host exactly as before - same status, same error page, - /// same logging and error reporting - with the two headers added to the host's response through + /// same logging and error reporting. The two headers are registered through /// , which survives the Response.Clear() the exception - /// handler middleware performs. - /// switches this to a 500 with a body produced here. + /// handler middleware performs, so they appear on the response an exception handler + /// (UseExceptionHandler, the developer exception page) produces. A host with no handler at + /// all lets Kestrel write a bare 500, which runs no OnStarting callbacks: that response carries no + /// header, and the status code alone still tells the client that the server answered. + /// switches this to a 500 with a + /// body produced here, headers guaranteed. /// /// /// @@ -212,19 +216,24 @@ public static TBuilder AddSyncErrorEndpointFilter(this TBuilder builde // Not handled here: the host decides the status and the body, and keeps logging and // reporting the exception the way it always did. Only the headers are ours. They are - // attached when the host's response starts because the exception handler middleware - // clears every header set before it runs; OnStarting callbacks are not cleared. - var traceId = context.HttpContext.TraceIdentifier; - response.OnStarting(() => + // attached when the host's error response starts because the exception handler + // middleware clears every header set before it runs; OnStarting callbacks are not + // cleared. Once the response has started nothing can be added, and registering would + // throw and replace the real failure, so it is skipped. + if (!response.HasStarted) { - if (!response.Headers.ContainsKey(SyncHttpHeaders.ErrorCode)) + var traceId = context.HttpContext.TraceIdentifier; + response.OnStarting(() => { - response.Headers[SyncHttpHeaders.ErrorCode] = code; - response.Headers[SyncHttpHeaders.TraceId] = traceId; - } - - return Task.CompletedTask; - }); + if (!response.Headers.ContainsKey(SyncHttpHeaders.ErrorCode)) + { + response.Headers[SyncHttpHeaders.ErrorCode] = code; + response.Headers[SyncHttpHeaders.TraceId] = traceId; + } + + return Task.CompletedTask; + }); + } throw; } diff --git a/src/CoreSync.Http/SyncHttpErrorCodes.cs b/src/CoreSync.Http/SyncHttpErrorCodes.cs index 110fcda..1a8ff9c 100644 --- a/src/CoreSync.Http/SyncHttpErrorCodes.cs +++ b/src/CoreSync.Http/SyncHttpErrorCodes.cs @@ -58,6 +58,17 @@ public static class SyncHttpErrorCodes /// public const string AgentUnavailable = "agent-unavailable"; + /// + /// The component that owns the data store (for example an on-premise agent) was reached and + /// executed the request, but reported that it failed. Accompanied by a + /// body. + /// + /// + /// Not marked permanent: the agent's message is not structured enough to tell a data error from + /// a database that was briefly unreachable from the agent, so the client keeps the retries it had. + /// + public const string AgentError = "agent-error"; + /// /// The store is configured in a way the server cannot synchronize (a table without a primary key, /// a reserved column name, a table missing from the configuration...). Accompanied by a @@ -134,6 +145,8 @@ public static SyncFailureKind ToFailureKind(string? code) return SyncFailureKind.ConfigurationError; case AgentUnavailable: return SyncFailureKind.ServerUnavailable; + case AgentError: + return SyncFailureKind.ServerError; case DataError: return SyncFailureKind.DataError; default: diff --git a/src/CoreSync.Tests/SyncErrorClassificationTests.cs b/src/CoreSync.Tests/SyncErrorClassificationTests.cs index a70a07c..222c044 100644 --- a/src/CoreSync.Tests/SyncErrorClassificationTests.cs +++ b/src/CoreSync.Tests/SyncErrorClassificationTests.cs @@ -383,6 +383,32 @@ public async Task HttpClient_JsonDownload_StillRetriesFailuresWithoutAnErrorCode itemRequests.ShouldBe(2, "a failure the server did not classify keeps the retries it always had"); } + [TestMethod] + public async Task HttpClient_BinaryDownload_StillRetriesFailuresWithoutAnErrorCode() + { + var itemRequests = 0; + using var handler = new ScriptedHandler(request => + { + if (request.RequestUri!.AbsolutePath.Contains("/changes-bulk-item")) + { + itemRequests++; + // What a 1.0.1 server (or a proxy) sends for anything unexpected + return new HttpResponseMessage(HttpStatusCode.InternalServerError); + } + + return BulkChangeSetResponse(totalChanges: 10); + }); + + var client = SyncTestServer.CreateClient(handler, useBinaryFormat: true); + + var ex = await Should.ThrowAsync(() => client.GetChangesAsync(Guid.NewGuid(), null, SyncDirection.DownloadOnly)); + + // The binary loop has always made three attempts five seconds apart; a failure the server did + // not classify keeps every one of them + itemRequests.ShouldBe(3); + ex.ShouldBeOfType().Kind.ShouldBe(SyncFailureKind.ServerError); + } + [TestMethod] public async Task HttpClient_JsonDownload_DoesNotRetryAnchorTooOld() { @@ -512,6 +538,56 @@ public async Task SyncAgent_ClassifiesTheCallersCancellationAsCancelled() ex.Stage.ShouldBe(SynchronizationStage.RemoteStoreId); } + [TestMethod] + public async Task SyncAgent_OldClient_PlainHttpRequestExceptionWithAStatusIsNotNetwork() + { + // A CoreSync.Http.Client that predates SyncHttpRequestException throws what + // EnsureSuccessStatusCode throws: a plain HttpRequestException that does carry the status. + // The server answered, so this must not be reported as "you are offline". + foreach (var status in new[] { HttpStatusCode.Unauthorized, HttpStatusCode.NotFound, HttpStatusCode.InternalServerError, HttpStatusCode.ServiceUnavailable }) + { + var remote = new ScriptedSyncProvider + { + GetStoreId = () => throw new HttpRequestException($"Response status code does not indicate success: {(int)status} ({status}).", null, status) + }; + + var ex = await Should.ThrowAsync(() => new SyncAgent(new ScriptedSyncProvider(), remote).SynchronizeAsync()); + + ex.Kind.ShouldBe(SyncFailureKind.Unknown, $"status {status}"); + ex.Stage.ShouldBe(SynchronizationStage.RemoteStoreId); + ex.InnerException.ShouldBeOfType(); + } + } + + [TestMethod] + public async Task SyncAgent_OldClient_PlainHttpRequestExceptionWithoutAStatusIsNetwork() + { + // ...while the same type with no status is what HttpClient itself throws when the server was + // never reached, on an old client and a new one alike. + var remote = new ScriptedSyncProvider + { + GetStoreId = () => throw new HttpRequestException("No such host is known.", new System.Net.Sockets.SocketException(11001)) + }; + + var ex = await Should.ThrowAsync(() => new SyncAgent(new ScriptedSyncProvider(), remote).SynchronizeAsync()); + + ex.Kind.ShouldBe(SyncFailureKind.Network); + } + + [TestMethod] + public async Task SyncAgent_LocalUnauthorizedAccessIsNotACredentialsProblem() + { + // SQLite raises UnauthorizedAccessException for a database file the app may not write (a + // read-only folder, Android scoped storage). Re-authenticating with the server would not help. + var local = new ScriptedSyncProvider { GetChanges = _ => throw new UnauthorizedAccessException("Access to the path is denied.") }; + + var ex = await Should.ThrowAsync(() => new SyncAgent(local, new ScriptedSyncProvider()).SynchronizeAsync()); + + ex.Kind.ShouldBe(SyncFailureKind.Unknown); + ex.Stage.ShouldBe(SynchronizationStage.LocalChanges); + ex.InnerException.ShouldBeOfType(); + } + [TestMethod] public async Task SyncAgent_ClassifiesUnauthorizedAndKeepsTheInnerType() { @@ -610,6 +686,7 @@ public void ErrorCodes_PermanentAndKindMappingsAreStable() SyncHttpErrorCodes.IsPermanent(SyncHttpErrorCodes.ConfigurationError).ShouldBeTrue(); SyncHttpErrorCodes.IsPermanent(SyncHttpErrorCodes.EndpointNotFound).ShouldBeTrue(); SyncHttpErrorCodes.IsPermanent(SyncHttpErrorCodes.AgentUnavailable).ShouldBeFalse(); + SyncHttpErrorCodes.IsPermanent(SyncHttpErrorCodes.AgentError).ShouldBeFalse(); SyncHttpErrorCodes.IsPermanent(SyncHttpErrorCodes.ServerError).ShouldBeFalse(); // A code this version does not know, or no code at all, must never stop a retry @@ -619,6 +696,7 @@ public void ErrorCodes_PermanentAndKindMappingsAreStable() SyncHttpErrorCodes.ToFailureKind(null).ShouldBe(SyncFailureKind.Unknown); SyncHttpErrorCodes.ToFailureKind("something-newer").ShouldBe(SyncFailureKind.ServerError); SyncHttpErrorCodes.ToFailureKind(SyncHttpErrorCodes.AgentUnavailable).ShouldBe(SyncFailureKind.ServerUnavailable); + SyncHttpErrorCodes.ToFailureKind(SyncHttpErrorCodes.AgentError).ShouldBe(SyncFailureKind.ServerError); SyncHttpErrorCodes.ToFailureKind(SyncHttpErrorCodes.EndpointNotFound).ShouldBe(SyncFailureKind.ConfigurationError); } diff --git a/src/CoreSync/SyncAgent.cs b/src/CoreSync/SyncAgent.cs index ff143e0..4692fc9 100644 --- a/src/CoreSync/SyncAgent.cs +++ b/src/CoreSync/SyncAgent.cs @@ -169,9 +169,9 @@ private static SynchronizationException Classify( result.Stage = stage; ISyncFailureInfo? info = null; + System.Net.Http.HttpRequestException? httpFailure = null; var sawProviderFailure = false; var sawCancellation = false; - var sawHttpFailure = false; var sawUnauthorized = false; foreach (var inner in Flatten(failure)) @@ -186,8 +186,8 @@ private static SynchronizationException Classify( // alone has to count as a data error. sawProviderFailure |= inner is SynchronizationException; sawCancellation |= inner is OperationCanceledException; - sawHttpFailure |= inner is System.Net.Http.HttpRequestException; sawUnauthorized |= inner is UnauthorizedAccessException; + httpFailure ??= inner as System.Net.Http.HttpRequestException; if (result.TableName == null && inner is ISyncFailureInfo withTable && withTable.TableName != null) { @@ -214,14 +214,19 @@ private static SynchronizationException Classify( } else if (sawUnauthorized) { - result.Kind = SyncFailureKind.Unauthorized; + // Only the remote side means "credentials refused". A local store raises the same type + // for a file it is not allowed to touch (a read-only folder, Android scoped storage), + // and re-authenticating would not fix that. + result.Kind = IsRemoteStage(stage) ? SyncFailureKind.Unauthorized : SyncFailureKind.Unknown; } - else if (sawHttpFailure) + else if (httpFailure != null) { - // An HttpRequestException that does not implement ISyncFailureInfo never carried a - // response: the connection failed, the name did not resolve, TLS failed. The server - // was not reached. - result.Kind = SyncFailureKind.Network; + // A plain HttpRequestException (one that does not implement ISyncFailureInfo) is what + // HttpClient throws when the server was never reached - but also what an older + // CoreSync.Http.Client throws from EnsureSuccessStatusCode for a 401, 404 or 500. The + // two are told apart by whether the exception carries a status code; when the runtime + // cannot say, nothing is claimed. + result.Kind = CarriesNoResponse(httpFailure) ? SyncFailureKind.Network : SyncFailureKind.Unknown; } else if (sawProviderFailure) { @@ -231,6 +236,33 @@ private static SynchronizationException Classify( return result; } + // HttpRequestException.StatusCode exists from .NET 5 on; this library targets .NET Standard 2.0, + // where the type has no such member, so it is read through reflection on the actual runtime. + private static readonly System.Reflection.PropertyInfo? HttpStatusCodeProperty = + typeof(System.Net.Http.HttpRequestException).GetProperty("StatusCode"); + + /// + /// Returns true only when the runtime exposes the response status on the exception and it + /// is absent, i.e. the request failed before any response arrived. false when a status is + /// present or when the runtime cannot tell. + /// + private static bool CarriesNoResponse(System.Net.Http.HttpRequestException exception) + { + if (HttpStatusCodeProperty == null) + { + return false; + } + + try + { + return HttpStatusCodeProperty.GetValue(exception) == null; + } + catch + { + return false; + } + } + private static bool IsRemoteStage(SynchronizationStage stage) => stage == SynchronizationStage.RemoteStoreId || stage == SynchronizationStage.UploadChanges From 9894eb1c35e531af1dcc787b4b584109bd9bafc9 Mon Sep 17 00:00:00 2001 From: Adolfo Marinucci Date: Fri, 16 Oct 2026 18:15:28 +0200 Subject: [PATCH 3/4] Update MessagePack to 3.1.12 3.1.7 is affected by GHSA-qhrr-8q5h-9q3h (nested arrays can exceed buffer length limits), fixed in 3.1.9. The NuGet audit fails restore on it with warnings as errors. Patch release: the serialization format is unchanged. --- src/CoreSync.Http.Client/CoreSync.Http.Client.csproj | 2 +- src/CoreSync.Http.Server/CoreSync.Http.Server.csproj | 2 +- src/CoreSync.Http/CoreSync.Http.csproj | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/CoreSync.Http.Client/CoreSync.Http.Client.csproj b/src/CoreSync.Http.Client/CoreSync.Http.Client.csproj index 18982d6..c742876 100644 --- a/src/CoreSync.Http.Client/CoreSync.Http.Client.csproj +++ b/src/CoreSync.Http.Client/CoreSync.Http.Client.csproj @@ -26,7 +26,7 @@ - + diff --git a/src/CoreSync.Http.Server/CoreSync.Http.Server.csproj b/src/CoreSync.Http.Server/CoreSync.Http.Server.csproj index e038ad3..013b744 100644 --- a/src/CoreSync.Http.Server/CoreSync.Http.Server.csproj +++ b/src/CoreSync.Http.Server/CoreSync.Http.Server.csproj @@ -26,7 +26,7 @@ - + diff --git a/src/CoreSync.Http/CoreSync.Http.csproj b/src/CoreSync.Http/CoreSync.Http.csproj index dc25063..3a19b46 100644 --- a/src/CoreSync.Http/CoreSync.Http.csproj +++ b/src/CoreSync.Http/CoreSync.Http.csproj @@ -25,7 +25,7 @@ - + From e8434084b4c0908a3582c3ed4ca7f01367858c57 Mon Sep 17 00:00:00 2001 From: Adolfo Marinucci Date: Mon, 19 Oct 2026 12:53:18 +0200 Subject: [PATCH 4/4] Disambiguate the OnStarting cref in the server doc comment CS0419: HttpResponse.OnStarting has two overloads, and the Release build treats the warning as an error. --- src/CoreSync.Http.Server/WebApplicationExtensions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/CoreSync.Http.Server/WebApplicationExtensions.cs b/src/CoreSync.Http.Server/WebApplicationExtensions.cs index 98bd227..baf2ff2 100644 --- a/src/CoreSync.Http.Server/WebApplicationExtensions.cs +++ b/src/CoreSync.Http.Server/WebApplicationExtensions.cs @@ -159,7 +159,7 @@ internal static class SyncErrorFilterEndpointExtensions /// /// Anything else keeps propagating to the host exactly as before - same status, same error page, /// same logging and error reporting. The two headers are registered through - /// , which survives the Response.Clear() the exception + /// , which survives the Response.Clear() the exception /// handler middleware performs, so they appear on the response an exception handler /// (UseExceptionHandler, the developer exception page) produces. A host with no handler at /// all lets Kestrel write a bare 500, which runs no OnStarting callbacks: that response carries no