[arch/backlog] major-medium + minor architecture/best-practice cleanups (grouped)

[ruhex]

From the architecture + .NET 10/C# best-practices review (2026-07-03, 12 aspect-agents, 83 findings).

Problem (MINOR, low)

`` (backlog)

From the architecture + .NET 10/C# best-practices review (2026-07-03, 12 aspect-agents, 83 findings).

Backlog — 66 findings (major-medium + minor)

aot (3)

• BGPLite.Api/ManagementApi.cs:653 [medium]: The serialize call omits the cached _jsonOpts, so it falls back to the default per-call options which is both a perf regression → Pass the shared options explicitly: JsonSerializer.Serialize(response.Body, _jsonOpts). Better, rout
• BGPLite.Api/ManagementApi.cs:288,377,653,684 [low]: ManagementApi serializes responses and deserializes CreatePeer/UpdatePeer request bodies with reflection-based System.Text.Json an → Introduce a JsonSerializerContext (or per-DTO [JsonSerializable]) for the request DTOs (CreatePeerRe
• BGPLite/BGPLite.csproj [low]: The project ships self-contained single-file but is not NativeAOT, and several dependencies would currently block AOT if attempted → If AOT is desired: (1) add the JsonSerializerContext above, (2) replace YamlDotNet's reflection dese

api-design (1)

• BGPLite.Api/ManagementApi.cs:131-132 POST [low]: REST semantics are inconsistent with HTTP verbs. POST /api/peers is actually an upsert (PeerStore.CreatePeer returns the existing → Make POST strictly create (409 on existing (Ip,Asn), or document and return 200 with a `created:fals

async (4)

• BGPLite.Providers/PrefixSourceService.cs:47,64,85,108 [medium]: The cache layer (PrefixSourceService) and the provider interface accept a CT but never propagate it: LoadCachedAsync(source) calls → Add CancellationToken ct = default to GetAsync/GetDefaultAsync/LoadAllAsync; pass it to `gate.Wait
• BGPLite.Api/ManagementApi.cs:81, 325, 405 [low]: Unobserved fire-and-forget Tasks: _ = HandleAsync(ctx) (81), _ = _sessionManager.RefreshPeerAsync(...) (325, 405). An exceptio → Wrap the two RefreshPeerAsync fire-and-forgets in a small helper that observes the result, e.g. `_ =
• BGPLite.Server/BgpSession.cs:995-1029 [low]: _stream.WriteAsync(buffer.AsMemory(0, written)) (1013) omits the CancellationToken even though the caller (ReceiveMessageAsync pat → Add CancellationToken cancellationToken = default to SendMessageAsync and forward it: `await _stre
• BGPLite.Server/BgpSession.cs:995 [low]: SendMessageAsync returns Task but the common case (SendKeepaliveAsync, End-of-RIB, single UPDATE) completes synchronously once _se → Consider ValueTask for SendMessageAsync and SendKeepaliveAsync: `private async ValueTask SendMessage

bcl (2)

• BGPLite.Server/BgpMetrics.cs:5-26 + BGPLite.Api/Managem [medium]: BgpMetrics is a hand-rolled set of Volatile/Interlocked counters exposed only through the management API's JSON. There is no Syste → Expose via System.Diagnostics.Metrics: a static Meter ("BGPLite.Server"), create Counter updat
• BGPLite.Providers/PrefixSourceProviderFactory.cs:10-14 [medium]: The factory holds a Dictionary<string, IPrefixSourceProvider> built once from the DI provider set and only ever read. This is th → Build FrozenDictionary<string, IPrefixSourceProvider> in the ctor via `_byKind = providers.ToFroze

collections (8)

• BGPLite.Routing/Route.cs:8-9 [high]: Route.AsPath and Route.Communities are declared as uint[] ... = []. Because uint[] is a mutable reference array exposed on a pub → Switch both to ImmutableArray<uint> (or required ImmutableArray<uint> AsPath { get; init; }). It
• BGPLite.Server/BgpSession.cs:584 and :664 [high]: In the ASN/custom-AS send loops, every prefix allocates a fresh uint[1]{asn} via the collection expression [asn] (one array pe → Hoist the singleton: since AsPath is logically the same one-element path for all routes in the loop,
• BGPLite.Routing/ICommunityResolver.cs:37 and BGPLite.Se [medium]: ICommunityResolver.Resolve returns uint[]. This forces every implementation to hand out an array (ConfigCommunityResolver caches → Change the contract to ImmutableArray<uint> Resolve(CommunitySource). NullCommunityResolver return
• BGPLite.Server/BgpSession.cs:619 [low]: On every full send the code builds a HashSet of subscribed list names and then runs _appConfig.PrefixSources.Any(s => s.N → Build one FrozenSetof resolved-as-RIPE names and oneFrozenDictionary<string,PrefixSourc
• BGPLite.Routing/RouteTable.cs:26-30 [low]: GetAll() does _routes.Values.ToList() materializing a full snapshot. The Enumerate() sibling already exists and is correctly pre → Either return IReadOnlyCollection<Route> via _routes.Values directly (ConcurrentDictionary.Value
• BGPLite.Server/BgpSession.cs:884-887 [low]: routes.GroupBy(...).Select(g => g.ToList()).ToList() materializes a List<List>: one inner list per community group plus t → Since this just needs to preserve encounter order by community set, consider grouping in a single pa
• BGPLite.Server/BgpSession.cs:694 and :745 [low]: Two extra full-list materializations on the send path: line 694 reassigns `routes = routes.Where(r => _routeFilter.AcceptOutgoing( → Change SendRoutesAsync(uint nextHop, IReadOnlyList routes) (it already calls routes.Count and
• BGPLite.Protocol/BgpMessageReader.cs:42-49 [low]: BgpConstants.Marker is already exposed as a ReadOnlySpan over a collection-expression (idiomatic), but the marker comparison → Replace the loop with if (!marker.SequenceEqual(BgpConstants.Marker)) throw ... — ReadOnlySpan<byt

concurrency (6)

• BGPLite.Server/IPrefixService.cs:5-10 and BGPLite.Provi [medium]: Cancellation is not threaded into the prefix/RIPEstat layer. IPrefixService declares NO CancellationToken parameter on any method, → Add CancellationToken ct = default to the IPrefixService interface (GetPrefixesAsync, GetPrefixesF
• BGPLite.Providers/PrefixService.cs:63-75 [medium]: The RU-prefix projection cache uses plain instance fields _ruProjected/_ruCachedAt with a non-atomic double-checked-locking pa → Either route RU through PrefixSourceService (it already loads the default source with a proper per-k
• BGPLite.Api/ManagementApi.cs:132,138,148,431,618 [medium]: ManagementApi.Route is synchronous and bridges back into async handlers with .GetAwaiter().GetResult() (HandleCreatePeer, Handle → Make Route and the affected handlers return Task and await them in the async HandleAsyn
• BGPLite.Api/ManagementApi.cs:131-148 Route [medium]: The router Route() is synchronous and bridges into async DB/HTTP work via .GetAwaiter().GetResult(). Several of those async paths → Make Route() and all handlers async (Task) and await throughout, removing every .GetAwa
• BGPLite.Server/BgpSession.cs:35,66-68,1254-1258 [low]: _state is volatile and read by external threads via IsEstablished/State (BgpServer.RefreshPeerAsync:307,311, StopAsync:120, GetA → Add a doc-comment on TransitionTo and on _state stating the single-writer invariant: 'Only the RunAs
• BGPLite.Server/BgpSession.cs:313-323 [low]: RunEstablishedAsync correctly cancels and awaits both loop tasks, so no orphan tasks — good. However the hold-time=0 fast path (li → Offload the ROUTE_REFRESH-triggered refresh off the read loop (e.g. post it to a dedicated single-co

config (2)

• BGPLite.Server/BgpSession.cs:29,130,146 + SendAllRoutes [medium]: BgpSession is constructed with the concrete AppConfig DTO and reaches deep into its structure (RipeStat.AsnLists, PrefixSources, → Promote the policy inputs to an injected abstraction (e.g. an IPeerRoutePolicy or a small Subscripti
• BGPLite.Api/ManagementApi.cs:29 [low]: Unlike every other field on ManagementApi (all readonly) and unlike BgpServer/BgpSession CTS fields (both readonly), Managemen → Mark the field private readonly CancellationTokenSource _cts = new(); to match the surrounding inv

config-binding (1)

• Program.cs:24 [low]: Config is bound straight from YAML into AppConfig and registered as a singleton (Program.cs:33) with no validation. Invalid/malfor → Either wrap AppConfig in IOptions with a Validate(...) rule set (and .ValidateOnStart() s

coupling (2)

• CIDR parse duplicated at BGPLite.Api/ManagementApi.cs:3 [medium]: The IndexOf('/'); IPAddress.Parse(cidr[..slash]); byte.Parse(cidr[(slash+1)..]); BgpConstants.IPAddressToUint(ip) idiom is hand- → Add a PrefixListParser.ParseCidr(string) helper returning (uint prefix, byte length) (with a `Tr
• Peer route-projection duplicated: BGPLite.Server/BgpSes [medium]: The 'given a peer, resolve its subscriptions/custom-prefixes/custom-ASNs into a flat prefix list with communities' algorithm exist → Extract a single IPeerRouteResolver (e.g. ResolvePeerPrefixes(PeerContext) : List<Route>) used b

csharp-idiom (8)

• BGPLite.Configuration/PrefixSourceConfig.cs:9-44 [medium]: All configuration DTOs are declared as public sealed class ... { public T Prop { get; init; } = ...; }. These are pure immutable → Convert these DTOs to positional record types: `public sealed record PrefixSourceConfig(string Kin
• BGPLite.Providers/IPrefixSourceProvider.cs:17, FilePref [medium]: The named tuple (uint Prefix, byte Length) is the de-facto prefix type across ~15 method signatures and several local/collection → Standardize on IpPrefix (rename the field to Length-compatible or add `record struct IpPrefix(ui
• BGPLite.Server/BgpServer.cs:40-64 and BgpSession.cs:119 [low]: Several classes have long constructors that only assign each parameter to a like-named field (_x = x; _y = y; ...). FilePrefixPr → Adopt a primary constructor and convert the assigned fields to either direct parameter usage or `rea
• BGPLite.Server/BgpSession.cs:298-301, 884-922, 902-922 [low]: Two hand-rolled private equality comparers (CommunitySetComparer over uint[], AttributeKey over uint[]) implement the same sequenc → C# 12 collection expressions plus StructuralComparisons.StructuralEqualityComparer or, more cleanl
• BGPLite/Program.cs:165-166 and BGPLite.Api/ManagementAp [low]: Array.Empty<uint>() is used in Program.cs where the codebase elsewhere consistently uses the collection expression [] (e.g. D → Replace Array.Empty()with[]` for consistency. The request records are acceptable as-is; o
• BGPLite.Providers/PrefixSourceService.cs:151-164 [low]: TryGetFresh uses the out var list pattern with list = null!; inside the method to satisfy definite-assignment before the early → Annotate TryGetFresh with [NotNullWhen(true)] out IReadOnlyList<(uint,byte)>? list and drop the
• BGPLite.Protocol/BgpMessage.cs:3-6 [low]: The BGP message hierarchy is abstract class BgpMessage + 5 sealed subclasses, each overriding Type with a constant. These are → Consider public abstract record BgpMessage(BgpMessageType Type); with `public sealed record BgpOpe
• BGPLite.Routing/Route.cs:3-13 [low]: Route is a sealed class with only required/init properties and a computed Key — i.e. a pure value object. The codebase elsewhere u → Consider public sealed record Route { public required uint Prefix { get; init; } ... }. Pair with

data-model (3)

• BGPLite.Api/BgpDbContext.cs:12-32 [medium]: Schema is created via EnsureCreated and then evolved with hand-written ExecuteSqlRaw DDL (DROP INDEX IF EXISTS / CREATE UNIQUE IND → Adopt EF Core migrations: dotnet ef migrations add Initial from the current model, then Migrate()
• BGPLite.Api/PeerStore.cs:138-145 SetCommunities, :162-1 [medium]: Each 'set' mutation deletes all existing child rows then re-inserts the new set within a single SaveChanges but with no explicit t → Introduce a single PeerStore.UpdatePeer(...) method that loads the aggregate once within one DbConte
• BGPLite.Api/PeerStore.cs:82-87 GetPeerByIp vs :94-99 Ge [medium]: Peer identity was migrated to composite (Ip,Asn) ([P0/protocol] Concurrent peers behind one source IP clobber each other — sessions keyed by remote IP only #18-23), but several read paths are still Ip-only and silently pick 'one of' the → Drive the Ip-only lookups to extinction on the production paths: have /api/me and the PeerCommunityF

di (1)

• BGPLite.Providers/PrefixSourceProviderFactory.cs:8-21 + [medium]: PrefixSourceProviderFactory is a manually-built Dictionary<string,IPrefixSourceProvider> that resolves providers by a Kind discrim → Register each provider as a keyed service keyed by its Kind string: builder.Services.AddKeyedSinglet

di-lifecycle (4)

• BGPLite/BGPLite/Program.cs:40-41 [medium]: BgpDbContext is registered TWICE with conflicting lifetime intent: once as a factory (AddDbContextFactory, used correctly by the s → Remove the AddDbContext(..., Scoped) registration (Program.cs:40-41). The one startup
• BGPLite/BGPLite/Program.cs:105,112-124,125 [medium]: BgpServer is constructed by hand inside the AddHostedService factory and assigned to a mutable local BgpServer? bgpServer = null → Register BgpServer as a singleton first: builder.Services.AddSingleton<BgpServer>(); with a factor
• BGPLite.Providers/PrefixService.cs:15 [medium]: PrefixService depends on the CONCRETE RipeStatProvider type, which is sealed and has no interface. The per-ASN caching logic i → Extract an IRipeStatProvider (method GetPrefixesAsync(uint asn, CancellationToken)) implemented
• BGPLite.Server/BgpServer.cs:325-332 [low]: BgpServer.Dispose() cancels _cts but never disposes it (_cts.Dispose() is never called on either the Dispose() path at line 325 or → Add _cts.Dispose() at the end of BgpServer.Dispose(). Since StopAsync already does the graceful tear

errors (3)

• BGPLite.Api/ManagementApi.cs:466 and :478 [medium]: Two bare catch { } blocks with no variable binding and no logging when resolving ASN/custom prefixes for the peer-prefixes API r → Bind the exception and log at Warning with a structured template, e.g. `catch (Exception ex) { _logg
• BGPLite.Providers/PrefixSourceProviderFactory.cs:12-14, [low]: Public/ctor entry points of DI-registered services do not validate arguments with the .NET 6+ static throw helpers. PrefixSourcePr → Add ArgumentNullException.ThrowIfNull(providers) / ThrowIfNullOrWhiteSpace(kind) in the factory,
• BGPLite.Protocol/BgpMessageWriter.cs:16,29 [low]: The default switch arm throws new ArgumentException("Unknown message type: {message.Type}") without nameof and without : base(m → Use throw new NotSupportedException($"Unknown BGP message type: {message.Type}")` for the exhaustiv

json (1)

• BGPLite.Api/ManagementApi.cs:649-657 [high]: Every API response is serialized with JsonSerializer.Serialize(response.Body) where Body is an anonymous object (`new { asn = ..., → Introduce a JsonSerializerContext for the management DTOs. The current ad-hoc anonymous payloads (se

layering (1)

• BGPLite.Routing/PeerCommunityFilter.cs:2,43-46 [medium]: The Routing layer's filter depends on BGP protocol constants (well-known communities live in BGPLite.Protocol) and its IRouteFilte → Define a small routing-owned filter-context record (peer ASN + address) instead of passing PeerConfi

logging (5)

• BGPLite.Server/BgpSession.cs [high]: No LoggerMessage source-generated partial methods anywhere in the repo (grep confirms zero usage of Microsoft.Extensions.Logging.G → Add <PackageReference Include="Microsoft.Extensions.Logging.Generator" /> (inherited via the Micro
• BGPLite.Providers/PrefixService.cs:91,95 and :46 [high]: PrefixService has no ILogger field at all. WarmUpAsync uses Console.WriteLine for both success ("AS{asn} cached") and failure ("AS → Inject ILogger into the constructor (add ThrowIfNull), replace both Console.WriteLine
• BGPLite.Server/BgpSession.cs:503 [medium]: LogDebug("Route added: {Prefix} via {NextHop}", nlri, BgpConstants.UintToIPAddress(nextHop)) eagerly evaluates BgpConstants.UintTo → Either guard with if (_logger.IsEnabled(LogLevel.Debug)) before building the argument, or move to
• BGPLite.Server/BgpSession.cs and BGPLite.Api/Management [medium]: Logging uses the structured ILogger extension methods (templates with named placeholders — good, and no string interpolation is in → Add [LoggerMessage] partial methods in a partial BgpSession (or a dedicated Log class) for the high-
• BGPLite.Server/BgpSession.cs:248,257,269,285,412 [low]: Multiple bare catch { /* best-effort */ } blocks around SendNotificationAsync/keepalive sends. These are intentional fire-and-fo → For the non-RFC-cited catches (248/257/269/285), log at Trace with the exception so a deep-debug ses

nullable (1)

• BGPLite.Server/BgpSession.cs:621 [medium]: Uses _appConfig!.PrefixSources.Any(...) with the null-forgiving operator even though _appConfig is declared AppConfig? (line → Capture var appConfig = _appConfig; (or assign a non-null local inside the is not null guard at

resilience (6)

• BGPLite.Providers/PrefixService.cs:81-101 [medium]: Warmup failure handling is inconsistent and operationally invisible. PrefixService.WarmUpAsync catches per-ASN RIPEstat failures a → Route warmup output through ILogger with a Warning/Error level. Track a failed-load count and log an
• BGPLite.Api/ManagementApi.cs:131 [medium]: There is no resource cap: any client can register an unbounded number of peers, and each peer can carry unbounded custom prefixes → Add configurable caps enforced in the handler before store mutation: MaxPeers (reject POST when GetA
• BGPLite.Providers/RipeStatProvider.cs:32-50 [medium]: Transient-failure retry is hand-rolled: a for-loop with manual exponential backoff (TimeSpan.FromSeconds(RetryDelaySeconds * 2^att → Use Microsoft.Extensions.Http.Resilience (Polly v8) on the named HttpClient in Program.cs: AddHttpCl
• BGPLite.Api/ManagementApi.cs:325,405 [low]: CreatePeer/UpdatePeer fire RefreshPeerAsync with discard (_ = ...) and never observe the task. RefreshPeerAsync -> BgpServer.Ref → Either await RefreshPeerAsync in the handler (so failures are reported), or explicitly attach a cont
• BGPLite.Providers/RipeStatProvider.cs:26-51 vs HttpPref [medium]: Inconsistent resilience between providers. RipeStatProvider has explicit retry + exponential backoff + transient classification (4 → Give HttpPrefixProvider the same retry/backoff policy as RipeStatProvider (extract a shared IHttpCli
• BGPLite.Server/BgpSession.cs:46-49 [low]: Several catch blocks swallow exceptions with no log at all. PrefixService.GetPrefixesForAsns:46-49 catches catch { } with an emp → Replace catch { } in GetPrefixesForAsns with `catch (Exception ex) { _logger.LogWarning(..., asn,

security (1)

• BGPLite.Api/ManagementApi.cs:108-117 [medium]: The generic handler catches every exception and returns ApiResponse.Error(ex.InnerException?.Message ?? ex.Message, 500) — leaki → Log the full exception server-side; return a generic 'internal error' message to the client for 500s

testing (3)

• BGPLite.Server/BgpSession.cs internal static helpers:79 [medium]: Eight pure functions (AS_PATH/AS4_PATH merge, mandatory-attribute validation, UPDATE attribute building, community-set grouping, r → Move these into Protocol/codec and Routing types where they naturally belong (they are stateless RFC
• BGPLite.Tests/BgpSessionShutdownTests.cs:24-53 [low]: BgpSession depends on a concrete System.Net.Sockets.Socket, so integration tests must spin up real loopback TCP pairs (ConnectedPa → This is an acceptable design choice for a raw-TCP/179 server and the tests are well-written (ReadExa
• BGPLite.Tests/BGPLite.Tests.csproj:23-25 [low]: xUnit packages are pinned to 2.9.x / 2.8.x while the rest of the stack is on .NET 10 / Microsoft.NET.Test.Sdk 17.14.1. xUnit v3 (a → Optionally upgrade to xUnit v3 (xunit.v3 + xunit.v3.core) and its runner package to align with the .

Fix

See grouped items above.