From 461530b0af7a70838ec8487d87096c07a45ab392 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 19 Sep 2026 20:36:01 +0000 Subject: [PATCH 1/3] perf(routeinstance): stop the save rebuilding every route's geometry Saving a trip imported from Traewelling took seconds before the list came back with its delay display, and almost none of it was the save. The station suggestions the import computes go through the matcher, whose index was one object fingerprinted on the route count *and* the station count. Importing onto a new route bumps the route count, so the very next save found the index stale and rebuilt it - streaming all 12,809 routes' geometry, 382 MB and 9.9M coordinates, through Douglas-Peucker, inside the request. The lookup that triggered it only ever reads the station tree. So the index is now two, counted and cached apart: matching an operator's calling pattern builds the station points and never touches route geometry. Finding a trip's stations reads its endpoints off the geometry already on the stack rather than asking the route index for them, which leaves FindStationTripsAsync as the only caller that builds the expensive half. Three more costs on the same path: - Matching stops asked the database whether the names agreed once per stop, and a calling pattern runs to dozens of stops. Geometry first, then one query for the names. - The save loaded the route with every trip it has ever had, joined across properties and maps, to find the one trip being edited - repeating the route's geometry column on every row. It now checks ownership, loads that one trip, and reads the geometry only when a duration needs computing. - The reload the screen waits on had the same joined shape. Split queries, so the geometry is fetched once instead of once per row. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012EGxqy7LB2FpCSns4JiNgZ --- OV_DB.Tests/StationTripMatcherTests.cs | 60 ++++++++-- OV_DB/Controllers/RoutesController.cs | 52 +++++++-- OV_DB/Services/MatcherIndex.cs | 156 ++++++++++++++++--------- OV_DB/Services/StationTripMatcher.cs | 119 +++++++++++-------- 4 files changed, 261 insertions(+), 126 deletions(-) diff --git a/OV_DB.Tests/StationTripMatcherTests.cs b/OV_DB.Tests/StationTripMatcherTests.cs index d2d770dd..6e7bd024 100644 --- a/OV_DB.Tests/StationTripMatcherTests.cs +++ b/OV_DB.Tests/StationTripMatcherTests.cs @@ -341,7 +341,7 @@ public async Task ARouteAddedAfterTheIndexWasBuiltIsStillFound() var candidates = await matcher.FindTripsForStationAsync(1, 10); - Assert.Equal(2, cache.Builds); + Assert.Equal(2, cache.RouteBuilds); Assert.Contains(90, candidates.Select(c => c.RouteInstanceId)); } @@ -576,7 +576,44 @@ public async Task TheIndexIsBuiltOnceAndSharedAcrossCalls() await matcher.FindStationsForTripAsync(1, 1); await matcher.FindTripsForStationAsync(1, 10); - Assert.Equal(1, cache.Builds); + Assert.Equal(1, cache.RouteBuilds); + Assert.Equal(1, cache.StationBuilds); + } + + [Fact] + public async Task MatchingStopsNeverBuildsTheRouteIndex() + { + // The import path. Route geometry is hundreds of megabytes and a calling pattern says + // nothing about it, so a trip on a freshly imported line must not pay to rebuild it - which + // is exactly what a shared fingerprint made every Traewelling import do. + using var context = await SeedAsync(); + await AddStationAsync(context, 10, "Halt", BaseLat, BaseLon); + var cache = new CountingCache(); + var matcher = new StationTripMatcher(context, cache); + + var matched = await matcher.MatchStopsAsync([new StopPoint("Halt", BaseLat, BaseLon)]); + + Assert.Equal(10, Assert.Single(matched).StationId); + Assert.Equal(0, cache.RouteBuilds); + Assert.Equal(1, cache.StationBuilds); + } + + [Fact] + public async Task AStationAddedAfterTheIndexWasBuiltIsStillMatched() + { + // The station half of the same guarantee: the counts have to catch a new station on their + // own now that stations no longer ride along on the route fingerprint. + using var context = await SeedAsync(); + var cache = new CountingCache(); + var matcher = new StationTripMatcher(context, cache); + Assert.Empty(await matcher.MatchStopsAsync([new StopPoint("Halt", BaseLat, BaseLon)])); + + await AddStationAsync(context, 10, "Halt", BaseLat, BaseLon); + + var matched = await matcher.MatchStopsAsync([new StopPoint("Halt", BaseLat, BaseLon)]); + + Assert.Equal(10, Assert.Single(matched).StationId); + Assert.Equal(2, cache.StationBuilds); } [Fact] @@ -591,18 +628,27 @@ public async Task InvalidatingForcesTheIndexToBeRebuilt() cache.Invalidate(); await matcher.FindTripsForStationAsync(1, 10); - Assert.Equal(2, cache.Builds); + Assert.Equal(2, cache.RouteBuilds); } private sealed class CountingCache : IMatcherIndexCache { private readonly MatcherIndexCache _inner = new(); - public int Builds { get; private set; } + public int RouteBuilds { get; private set; } + public int StationBuilds { get; private set; } + public int Builds => RouteBuilds + StationBuilds; + + public Task GetRoutesAsync(int routeCount, Func> build, CancellationToken cancellationToken = default) => + _inner.GetRoutesAsync(routeCount, ct => + { + RouteBuilds++; + return build(ct); + }, cancellationToken); - public Task GetAsync(int routeCount, int stationCount, Func> build, CancellationToken cancellationToken = default) => - _inner.GetAsync(routeCount, stationCount, ct => + public Task GetStationsAsync(int stationCount, Func> build, CancellationToken cancellationToken = default) => + _inner.GetStationsAsync(stationCount, ct => { - Builds++; + StationBuilds++; return build(ct); }, cancellationToken); diff --git a/OV_DB/Controllers/RoutesController.cs b/OV_DB/Controllers/RoutesController.cs index a6a7793e..b0a6b91d 100644 --- a/OV_DB/Controllers/RoutesController.cs +++ b/OV_DB/Controllers/RoutesController.cs @@ -965,6 +965,9 @@ public async Task> GetRouteInstances(int id, .ThenInclude(ri => ri.RouteInstanceMaps) .ThenInclude(rim => rim.Map) .Where(r => r.RouteMaps.Any(rm => rm.Map.UserId == userIdClaim)) + // One query per collection instead of one join across all of them: joined, the route + // row — geometry and all — is repeated once per trip per property per map. + .AsSplitQuery() .SingleOrDefaultAsync(); if (route == null) { @@ -999,6 +1002,8 @@ public async Task> GetRouteInstances(Guid ma // shared map anywhere would be readable by id enumeration. .Where(r => r.RouteMaps.Any(rm => rm.Map.MapGuid == mapGuid && (!string.IsNullOrWhiteSpace(rm.Map.SharingLinkName) || rm.Map.UserId == userIdClaim))) + // See above: the joined shape repeats the route's geometry on every row. + .AsSplitQuery() .SingleOrDefaultAsync(); if (route == null) { @@ -1026,26 +1031,48 @@ public async Task UpdateInstance([FromBody] RouteInstanceUpdate up { return Forbid(); } - var route = await _context.Routes - .Where(r => r.RouteId == update.RouteId && r.RouteMaps.Any(rm => rm.Map.UserId == userIdClaim)) - .Include(r => r.RouteInstances) - .ThenInclude(ri => ri.RouteInstanceProperties) - .Include(r => r.RouteInstances) - .ThenInclude(ri => ri.RouteInstanceMaps) - .SingleOrDefaultAsync(); + // Ownership only. Pulling the route in with every trip it has ever had attached — each + // row carrying a copy of the route's geometry, which is megabytes on a long line — was + // most of what made saving one trip feel slow. + var routeExists = await _context.Routes + .AnyAsync(r => r.RouteId == update.RouteId && r.RouteMaps.Any(rm => rm.Map.UserId == userIdClaim)); - if (route == null) + if (!routeExists) { return NotFound(); } + // Read at most once, and only if a duration is actually computed: the timezone lookup + // wants the first and last coordinate, and the geometry is expensive to fetch for + // nothing. + // Fully qualified: SharpKml.Dom is in scope here and has a LineString of its own. + NetTopologySuite.Geometries.LineString geometry = null; + var geometryRead = false; + async Task RouteGeometryAsync() + { + if (!geometryRead) + { + geometry = await _context.Routes + .Where(r => r.RouteId == update.RouteId) + .Select(r => r.LineString) + .SingleOrDefaultAsync(); + geometryRead = true; + } + return geometry; + } + // A newly created instance only gets its id once saved, and the caller needs it to date // any station suggestions against the trip it just imported. RouteInstance savedInstance = null; if (update.RouteInstanceId.HasValue) { - var current = route.RouteInstances.SingleOrDefault(ri => ri.RouteInstanceId == update.RouteInstanceId); + var current = await _context.RouteInstances + .Where(ri => ri.RouteInstanceId == update.RouteInstanceId.Value && ri.RouteId == update.RouteId) + .Include(ri => ri.RouteInstanceProperties) + .Include(ri => ri.RouteInstanceMaps) + .AsSplitQuery() + .SingleOrDefaultAsync(); savedInstance = current; if (current != null) { @@ -1061,7 +1088,7 @@ public async Task UpdateInstance([FromBody] RouteInstanceUpdate up current.DurationHours = _timezoneService.CalculateDurationInHours( current.StartTime.Value, current.EndTime.Value, - route.LineString); + await RouteGeometryAsync()); } else { @@ -1108,6 +1135,7 @@ public async Task UpdateInstance([FromBody] RouteInstanceUpdate up { var newInstance = new RouteInstance { + RouteId = update.RouteId, Date = update.Date, StartTime = update.StartTime, EndTime = update.EndTime, @@ -1122,7 +1150,7 @@ public async Task UpdateInstance([FromBody] RouteInstanceUpdate up newInstance.DurationHours = _timezoneService.CalculateDurationInHours( newInstance.StartTime.Value, newInstance.EndTime.Value, - route.LineString); + await RouteGeometryAsync()); } newInstance.RouteInstanceProperties = new List(); @@ -1136,7 +1164,7 @@ public async Task UpdateInstance([FromBody] RouteInstanceUpdate up Value = rip.Value }); }); - route.RouteInstances.Add(newInstance); + _context.RouteInstances.Add(newInstance); savedInstance = newInstance; } await _context.SaveChangesAsync(); diff --git a/OV_DB/Services/MatcherIndex.cs b/OV_DB/Services/MatcherIndex.cs index 7176c4a7..d74f13a0 100644 --- a/OV_DB/Services/MatcherIndex.cs +++ b/OV_DB/Services/MatcherIndex.cs @@ -17,8 +17,8 @@ namespace OV_DB.Services; public readonly record struct StationPoint(int StationId, double Longitude, double Lattitude); /// -/// The spatial half of the matcher: every route broken into segments, and every active station as a -/// point. Built once and shared, because building it is the expensive part. +/// Every route broken into segments, plus where each one begins and ends. Built once and shared, +/// because building it is the expensive part. /// /// /// Measured against production data: 12,809 routes hold 9.9M coordinates and 382 MB if kept whole. @@ -27,32 +27,46 @@ namespace OV_DB.Services; /// human confirms. Indexing is global rather than per user; results are scoped to the caller's maps /// by the database query that turns route ids into trips, so an unowned route simply yields nothing. /// -public sealed class MatcherIndex +public sealed class RouteIndex { public required STRtree Segments { get; init; } - public required STRtree Stations { get; init; } /// First and last coordinate of each route, for the "you started or ended here" test. - public required IReadOnlyDictionary RouteEndpoints { get; init; } + public required IReadOnlyDictionary Endpoints { get; init; } /// /// What the index was built from. Compared on every read so a route imported a minute ago cannot /// be invisible until the cache happens to expire — which it was, because nothing remembered to /// invalidate. A counted fingerprint needs nobody to remember anything. /// public required int RouteCount { get; init; } +} + +/// +/// Every active station as a point. Kept apart from deliberately: reading +/// it costs one small non-spatial query, while route geometry is hundreds of megabytes, and the two +/// go stale for entirely unrelated reasons. Sharing one fingerprint meant importing a route — which +/// every Träwelling trip on a new line does — threw away the station index too and rebuilt all of +/// that geometry inside the save request, for a lookup that never touches a route. +/// +public sealed class StationIndex +{ + public required STRtree Stations { get; init; } + /// See : the same trick, counted separately. public required int StationCount { get; init; } } public interface IMatcherIndexCache { /// - /// The cached index if it was built from these same counts, otherwise a freshly built one. + /// The cached route index if it was built from this same count, otherwise a freshly built one. /// - Task GetAsync(int routeCount, int stationCount, Func> build, CancellationToken cancellationToken = default); + Task GetRoutesAsync(int routeCount, Func> build, CancellationToken cancellationToken = default); + /// The same, for stations. + Task GetStationsAsync(int stationCount, Func> build, CancellationToken cancellationToken = default); void Invalidate(); } /// -/// Holds the index between requests and drops it once nobody is using it. +/// Holds the indexes between requests and drops them once nobody is using them. /// /// /// The work this serves is bursty — a backfill session hammers it, then nothing touches it for days @@ -63,78 +77,106 @@ public sealed class MatcherIndexCache : IMatcherIndexCache, IDisposable { private static readonly TimeSpan IdleTimeout = TimeSpan.FromMinutes(30); - private readonly SemaphoreSlim _gate = new(1, 1); + private readonly Slot _routes = new(index => index.RouteCount); + private readonly Slot _stations = new(index => index.StationCount); private readonly Timer _sweep; - private MatcherIndex _index; - private DateTime _lastUsedUtc; public MatcherIndexCache() { _sweep = new Timer(_ => DropIfIdle(), null, IdleTimeout, IdleTimeout); } - public async Task GetAsync(int routeCount, int stationCount, Func> build, CancellationToken cancellationToken = default) - { - // Under the gate so a burst of concurrent callers pays the build cost once, not once each. - await _gate.WaitAsync(cancellationToken); - try - { - if (_index != null && (_index.RouteCount != routeCount || _index.StationCount != stationCount)) - { - // Something was imported or removed since this was built, so it is answering about a - // world that no longer exists. - _index = null; - } - _index ??= await build(cancellationToken); - _lastUsedUtc = DateTime.UtcNow; - return _index; - } - finally - { - _gate.Release(); - } - } + public Task GetRoutesAsync(int routeCount, Func> build, CancellationToken cancellationToken = default) => + _routes.GetAsync(routeCount, build, cancellationToken); + + public Task GetStationsAsync(int stationCount, Func> build, CancellationToken cancellationToken = default) => + _stations.GetAsync(stationCount, build, cancellationToken); public void Invalidate() { - if (!_gate.Wait(TimeSpan.FromSeconds(5))) - { - // A build is in flight. It will produce a fresh index anyway, so dropping is pointless - // and blocking an import on it would be worse. - return; - } - try - { - _index = null; - } - finally - { - _gate.Release(); - } + _routes.Invalidate(); + _stations.Invalidate(); } private void DropIfIdle() { - if (!_gate.Wait(TimeSpan.Zero)) + _routes.DropIfIdle(IdleTimeout); + _stations.DropIfIdle(IdleTimeout); + } + + public void Dispose() + { + _sweep.Dispose(); + _routes.Dispose(); + _stations.Dispose(); + } + + /// One cached index and the gate that keeps a burst of callers from all building it. + private sealed class Slot(Func fingerprint) : IDisposable where T : class + { + private readonly SemaphoreSlim _gate = new(1, 1); + private T _index; + private DateTime _lastUsedUtc; + + public async Task GetAsync(int count, Func> build, CancellationToken cancellationToken) { - return; + // Under the gate so a burst of concurrent callers pays the build cost once, not once each. + await _gate.WaitAsync(cancellationToken); + try + { + if (_index != null && fingerprint(_index) != count) + { + // Something was imported or removed since this was built, so it is answering + // about a world that no longer exists. + _index = null; + } + _index ??= await build(cancellationToken); + _lastUsedUtc = DateTime.UtcNow; + return _index; + } + finally + { + _gate.Release(); + } } - try + + public void Invalidate() { - if (_index != null && DateTime.UtcNow - _lastUsedUtc >= IdleTimeout) + if (!_gate.Wait(TimeSpan.FromSeconds(5))) + { + // A build is in flight. It will produce a fresh index anyway, so dropping is + // pointless and blocking an import on it would be worse. + return; + } + try { _index = null; } + finally + { + _gate.Release(); + } } - finally + + public void DropIfIdle(TimeSpan idleTimeout) { - _gate.Release(); + if (!_gate.Wait(TimeSpan.Zero)) + { + return; + } + try + { + if (_index != null && DateTime.UtcNow - _lastUsedUtc >= idleTimeout) + { + _index = null; + } + } + finally + { + _gate.Release(); + } } - } - public void Dispose() - { - _sweep.Dispose(); - _gate.Dispose(); + public void Dispose() => _gate.Dispose(); } } diff --git a/OV_DB/Services/StationTripMatcher.cs b/OV_DB/Services/StationTripMatcher.cs index 39e72d9a..e8c8bf83 100644 --- a/OV_DB/Services/StationTripMatcher.cs +++ b/OV_DB/Services/StationTripMatcher.cs @@ -139,7 +139,7 @@ public async Task FindStationTripsAsync(int userId, int stationId, return NoTrips; } - var index = await GetIndexAsync(cancellationToken); + var index = await GetRouteIndexAsync(cancellationToken); // Nearest approach per route, not per segment: a line that runs alongside a station for a // while would otherwise be reported many times over. @@ -247,7 +247,7 @@ public async Task> FindStationsForTripAsync(int return []; } - var index = await GetIndexAsync(cancellationToken); + var index = await GetStationIndexAsync(cancellationToken); var line = Simplify(trip.LineString); var coordinates = line.Coordinates; @@ -280,7 +280,12 @@ public async Task> FindStationsForTripAsync(int .Select(s => new { s.Id, s.Name, s.Lattitude, s.Longitude }) .ToListAsync(cancellationToken); - var endpoints = index.RouteEndpoints.TryGetValue(trip.RouteId, out var ends) ? ends : default; + // Taken from the geometry in hand rather than from the route index: this is the same + // simplified line that index would have recorded the ends of, so asking for it would mean + // building every other route's geometry to learn what is already on this stack. + var endpoints = coordinates.Length < 2 + ? default((double X1, double Y1, double X2, double Y2)) + : (coordinates[0].X, coordinates[0].Y, coordinates[^1].X, coordinates[^1].Y); var results = stations.Select(s => new StationCandidate( s.Id, s.Name, @@ -304,12 +309,13 @@ public async Task> FindStationsForTripAsync(int /// public async Task> MatchStopsAsync(IEnumerable stops, CancellationToken cancellationToken = default) { - var index = await GetIndexAsync(cancellationToken); - var matched = new Dictionary(); - // Stops arrive in the order the relation lists them, which is the order they are ridden in. - // Keeping it means the suggestion list reads like the journey instead of like an index. - var order = new List(); + // Only the station index: a calling pattern says nothing about route geometry, so building + // it here would be hundreds of megabytes read to answer a question about points. + var index = await GetStationIndexAsync(cancellationToken); + // Geometry first, names afterwards. The name rule needs the database and a calling pattern + // runs to dozens of stops, so this is one query where it used to be one per stop. + var picks = new List<(StopPoint Stop, int StationId, double Distance)>(); foreach (var stop in stops) { StationPoint? best = null; @@ -326,73 +332,79 @@ public async Task> MatchStopsAsync(IEnumerable p.StationId).Distinct().ToList(); + var names = await dbContext.Stations.AsNoTracking() + .Where(s => pickedIds.Contains(s.Id)) + .Select(s => new { s.Id, s.Name }) + .ToDictionaryAsync(s => s.Id, s => s.Name, cancellationToken); + + var matched = new Dictionary(); + // Stops arrive in the order the relation lists them, which is the order they are ridden in. + // Keeping it means the suggestion list reads like the journey instead of like an index. + var order = new List(); + + foreach (var (stop, stationId, distance) in picks) + { + if (!names.TryGetValue(stationId, out var name)) { continue; } // Beyond the tight radius, only take it if the names agree. - if (bestDistance > StopMatchMetres && !await NameAgreesAsync(best.Value.StationId, stop.Name, cancellationToken)) + if (distance > StopMatchMetres && !NameMatches(name, stop.Name)) { continue; } - if (!matched.TryGetValue(best.Value.StationId, out var existing)) + if (!matched.TryGetValue(stationId, out var existing)) { - matched[best.Value.StationId] = bestDistance; - order.Add(best.Value.StationId); + matched[stationId] = distance; + order.Add(stationId); } - else if (bestDistance < existing) + else if (distance < existing) { - matched[best.Value.StationId] = bestDistance; + matched[stationId] = distance; } } - if (order.Count == 0) - { - return []; - } - - var names = await dbContext.Stations.AsNoTracking() - .Where(s => order.Contains(s.Id)) - .Select(s => new { s.Id, s.Name }) - .ToDictionaryAsync(s => s.Id, s => s.Name, cancellationToken); - return order - .Where(names.ContainsKey) .Select(id => new StationCandidate(id, names[id], VisitEvidence.Stopover, matched[id])) .ToList(); } - private async Task NameAgreesAsync(int stationId, string stopName, CancellationToken cancellationToken) - { - if (string.IsNullOrWhiteSpace(stopName)) - { - return false; - } - var name = await dbContext.Stations.AsNoTracking() - .Where(s => s.Id == stationId) - .Select(s => s.Name) - .SingleOrDefaultAsync(cancellationToken); - return NameMatches(name, stopName); - } - /// - /// Counts first, then the index: two cheap queries are worth paying so a route imported a minute - /// ago is never invisible because a cached index predates it. + /// Count first, then the index: a cheap query is worth paying so a route imported a minute ago + /// is never invisible because a cached index predates it. /// - private async Task GetIndexAsync(CancellationToken cancellationToken) + private async Task GetRouteIndexAsync(CancellationToken cancellationToken) { // Counted without a predicate on the geometry. "WHERE LineString IS NOT NULL" measured at // 1.16 s against 10 ms for a plain count — MariaDB reads the geometry blobs to evaluate it — // and it ran on every station in the backfill. The fingerprint only has to notice change, so // rows are as good a signal as rows-with-geometry. var routeCount = await dbContext.Routes.CountAsync(cancellationToken); + return await indexCache.GetRoutesAsync(routeCount, BuildRouteIndexAsync, cancellationToken); + } + + /// The same, for stations — counted and cached apart from the routes. + private async Task GetStationIndexAsync(CancellationToken cancellationToken) + { var stationCount = await dbContext.Stations.CountAsync(s => !s.Hidden && !s.Special, cancellationToken); - return await indexCache.GetAsync(routeCount, stationCount, BuildIndexAsync, cancellationToken); + return await indexCache.GetStationsAsync(stationCount, BuildStationIndexAsync, cancellationToken); } - private async Task BuildIndexAsync(CancellationToken cancellationToken) + private async Task BuildRouteIndexAsync(CancellationToken cancellationToken) { var segments = new STRtree(); var endpoints = new Dictionary(); @@ -438,6 +450,16 @@ private async Task BuildIndexAsync(CancellationToken cancellationT } segments.Build(); + return new RouteIndex + { + Segments = segments, + Endpoints = endpoints, + RouteCount = routeRowCount + }; + } + + private async Task BuildStationIndexAsync(CancellationToken cancellationToken) + { var stations = new STRtree(); var stationRows = dbContext.Stations.AsNoTracking() .Where(s => !s.Hidden && !s.Special) @@ -454,12 +476,9 @@ private async Task BuildIndexAsync(CancellationToken cancellationT } stations.Build(); - return new MatcherIndex + return new StationIndex { - Segments = segments, Stations = stations, - RouteEndpoints = endpoints, - RouteCount = routeRowCount, StationCount = stationRowCount }; } @@ -467,9 +486,9 @@ private async Task BuildIndexAsync(CancellationToken cancellationT private static Geometry Simplify(Geometry line) => DouglasPeuckerSimplifier.Simplify(line, SimplifyMetres / MetresPerDegreeLatitude); - private VisitEvidence EvidenceFor(MatcherIndex index, int routeId, string stationName, string from, string to, double lattitude, double longitude) + private VisitEvidence EvidenceFor(RouteIndex index, int routeId, string stationName, string from, string to, double lattitude, double longitude) { - var endpoints = index.RouteEndpoints.TryGetValue(routeId, out var ends) ? ends : default; + var endpoints = index.Endpoints.TryGetValue(routeId, out var ends) ? ends : default; return EvidenceFrom(endpoints, stationName, from, to, lattitude, longitude); } From c2355adb872db2b1c9731e59371d98732083bfbf Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 19 Sep 2026 21:26:00 +0000 Subject: [PATCH 2/3] feat(backfill): build the route index before the queue needs it Opening the dating queue used to fire the first station request straight into a cold route index. The build reads every route's geometry and takes seconds, so the page sat on an unexplained spinner, and the request was long enough to be at a proxy's mercy. Opening the page is the moment we know the build is coming, so it is started there instead: POST /api/stationbackfill/prepare answers whether the index is warm and, if not, queues the build on MatcherWarmupService. The page waits on StationsHub rather than on a blocked request, showing a determinate bar - routes indexed out of routes to index - because the builder streams rows and the count is known up front. The queue/hosted-service/hub arrangement mirrors the Traewelling resync, which solves the same problem: an endpoint that can only queue, and the work reporting to the page it outlived. Details worth knowing: - The page subscribes before it asks, because asking is what starts the build; connecting after would lose the event of a build that finished in between, and it would then wait out the watchdog for something already done. - Starting the queue is a one-shot claim, so the finished event, the watchdog and a ready answer cannot each load the first station. - Preparing is an optimisation, never a gate: any failure falls through to the request that builds the index itself, which is what this page did before. - IMatcherIndexCache.HasRoutes answers outside the gate, so asking whether a build is needed cannot block behind the build. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012EGxqy7LB2FpCSns4JiNgZ --- OV_DB.Tests/StationTripMatcherTests.cs | 44 ++++++++ .../Controllers/StationBackfillController.cs | 30 +++++ OV_DB/Hubs/StationsHub.cs | 18 +++ .../src/app/models/stationView.model.ts | 16 +++ .../src/app/services/api.service.ts | 11 ++ .../services/stations-live.service.ts | 66 +++++++++++ .../station-backfill.component.html | 20 +++- .../station-backfill.component.scss | 18 +++ .../station-backfill.component.ts | 104 ++++++++++++++++++ OV_DB/OVDBFrontend/src/assets/i18n/en.json | 3 + OV_DB/OVDBFrontend/src/assets/i18n/nl.json | 3 + OV_DB/Services/MatcherIndex.cs | 19 ++++ OV_DB/Services/MatcherWarmupQueue.cs | 53 +++++++++ OV_DB/Services/MatcherWarmupService.cs | 104 ++++++++++++++++++ OV_DB/Services/StationTripMatcher.cs | 41 ++++++- OV_DB/Startup.cs | 9 +- 16 files changed, 555 insertions(+), 4 deletions(-) create mode 100644 OV_DB/Hubs/StationsHub.cs create mode 100644 OV_DB/OVDBFrontend/src/app/stations/services/stations-live.service.ts create mode 100644 OV_DB/Services/MatcherWarmupQueue.cs create mode 100644 OV_DB/Services/MatcherWarmupService.cs diff --git a/OV_DB.Tests/StationTripMatcherTests.cs b/OV_DB.Tests/StationTripMatcherTests.cs index 6e7bd024..6fc68228 100644 --- a/OV_DB.Tests/StationTripMatcherTests.cs +++ b/OV_DB.Tests/StationTripMatcherTests.cs @@ -580,6 +580,48 @@ public async Task TheIndexIsBuiltOnceAndSharedAcrossCalls() Assert.Equal(1, cache.StationBuilds); } + [Fact] + public async Task WarmingBuildsTheRouteIndexUpFrontAndOnlyOnce() + { + // What the backfill page asks for on the way in, so the first station does not sit through + // the build with nothing to show for it. + using var context = await SeedAsync(); + await AddStationAsync(context, 10, "Halt", BaseLat, BaseLon); + var cache = new CountingCache(); + var matcher = new StationTripMatcher(context, cache); + + Assert.False(await matcher.IsRouteIndexWarmAsync()); + + await matcher.WarmRouteIndexAsync(); + + Assert.True(await matcher.IsRouteIndexWarmAsync()); + Assert.Equal(1, cache.RouteBuilds); + + // And the work it was warmed for now costs nothing more. + Assert.Single(await matcher.FindTripsForStationAsync(1, 10)); + Assert.Equal(1, cache.RouteBuilds); + } + + [Fact] + public async Task WarmingReportsEveryRouteRowItPassed() + { + using var context = await SeedAsync(); + var reports = new List(); + var matcher = NewMatcher(context); + + await matcher.WarmRouteIndexAsync(new CollectingProgress(reports.Add)); + + // One route in the seed, so the bar has somewhere to end up rather than a total of zero. + Assert.NotEmpty(reports); + Assert.Equal(1, reports[^1].Processed); + Assert.Equal(1, reports[^1].Total); + } + + private sealed class CollectingProgress(Action report) : IProgress + { + public void Report(MatcherWarmupProgress value) => report(value); + } + [Fact] public async Task MatchingStopsNeverBuildsTheRouteIndex() { @@ -652,6 +694,8 @@ public Task GetStationsAsync(int stationCount, Func _inner.HasRoutes(routeCount); + public void Invalidate() => _inner.Invalidate(); } } diff --git a/OV_DB/Controllers/StationBackfillController.cs b/OV_DB/Controllers/StationBackfillController.cs index 266c8c1c..18757368 100644 --- a/OV_DB/Controllers/StationBackfillController.cs +++ b/OV_DB/Controllers/StationBackfillController.cs @@ -51,6 +51,36 @@ public class StationBackfillController( /// private const double BandDegrees = 0.25; + /// + /// Says whether the queue can be worked straight away, and starts getting ready if not. + /// + /// + /// Every station in the queue is answered by the same route index, and building it reads + /// all of the route geometry. Cold, that used to happen inside the first station request: + /// seconds of unexplained spinner, and a request long enough to be at a proxy's mercy. + /// The page asks here first, and on ready: false waits on the hub instead, watching + /// the build rather than a blocked request. + /// + [HttpPost("prepare")] + public async Task Prepare([FromServices] IMatcherWarmupQueue warmupQueue) + { + var userId = User.GetUserId(); + if (userId < 0) + { + return Forbid(); + } + + if (await matcher.IsRouteIndexWarmAsync()) + { + return Ok(new { ready = true }); + } + + // Already queued from another tab is just as good: that build warms the one index + // everything here shares, and its finished event goes to every connection this user has. + warmupQueue.TryEnqueue(userId); + return Ok(new { ready = false }); + } + /// /// The next undated visit to work on. steps past ones the user has /// looked at and left alone, without recording anything about them. diff --git a/OV_DB/Hubs/StationsHub.cs b/OV_DB/Hubs/StationsHub.cs new file mode 100644 index 00000000..3d31fcaf --- /dev/null +++ b/OV_DB/Hubs/StationsHub.cs @@ -0,0 +1,18 @@ +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.SignalR; + +namespace OV_DB.Hubs; + +/// +/// Per-user live updates for the station screens. Authenticated with the regular JWT +/// (access_token query parameter on the handshake); events go through Clients.User so they +/// only reach the user who asked for the work. +/// +[Authorize] +public class StationsHub : Hub +{ + // Building the route index takes seconds and outlives the request that asked for it, so + // the backfill page hears how it is getting on only here. + public const string WarmupProgressMethod = "BackfillWarmupProgress"; + public const string WarmupFinishedMethod = "BackfillWarmupFinished"; +} diff --git a/OV_DB/OVDBFrontend/src/app/models/stationView.model.ts b/OV_DB/OVDBFrontend/src/app/models/stationView.model.ts index a47a0924..bdf8de35 100644 --- a/OV_DB/OVDBFrontend/src/app/models/stationView.model.ts +++ b/OV_DB/OVDBFrontend/src/app/models/stationView.model.ts @@ -157,3 +157,19 @@ export interface StationSuggestionsForRoute { routeInstanceId: number | null; stations: StationSuggestion[]; } + +/** Whether the backfill queue can be worked straight away, or is still getting ready. */ +export interface BackfillReadiness { + ready: boolean; +} + +/** How far the route index has got, while the backfill page waits for it. */ +export interface BackfillWarmupProgress { + processed: number; + total: number; +} + +/** The end of that wait. `ready: false` means the build failed rather than finished. */ +export interface BackfillWarmupResult { + ready: boolean; +} diff --git a/OV_DB/OVDBFrontend/src/app/services/api.service.ts b/OV_DB/OVDBFrontend/src/app/services/api.service.ts index c3ab8d69..013a8938 100644 --- a/OV_DB/OVDBFrontend/src/app/services/api.service.ts +++ b/OV_DB/OVDBFrontend/src/app/services/api.service.ts @@ -15,6 +15,7 @@ import { MultipleEdit } from "../models/multipleEdit.model"; import { RouteInstance } from "../models/routeInstance.model"; import { StationMap } from "../models/stationMap.model"; import { + BackfillReadiness, OSMStop, RouteGeometry, StationBackfillItem, @@ -583,6 +584,16 @@ export class ApiService { return this.httpClient.get(url); } + /** + * Asks whether the dating queue is ready to work, and starts getting it ready if not. A false + * answer means the route index is being built; the page waits for the hub rather than firing + * the first station request into a build it would sit through unexplained. + */ + prepareBackfill(): Observable { + const url = environment.backend + "api/stationbackfill/prepare"; + return this.httpClient.post(url, {}); + } + /** Retires a station from the dating queue. Says nothing about whether it was visited. */ skipBackfillStation(stationId: number): Observable { const url = environment.backend + "api/stationbackfill/" + stationId + "/skip"; diff --git a/OV_DB/OVDBFrontend/src/app/stations/services/stations-live.service.ts b/OV_DB/OVDBFrontend/src/app/stations/services/stations-live.service.ts new file mode 100644 index 00000000..97eadc36 --- /dev/null +++ b/OV_DB/OVDBFrontend/src/app/stations/services/stations-live.service.ts @@ -0,0 +1,66 @@ +import { Injectable, inject } from '@angular/core'; +import { HubConnection, HubConnectionBuilder } from '@microsoft/signalr'; +import { Subject } from 'rxjs'; +import { environment } from 'src/environments/environment'; +import { AuthenticationService } from '../../services/authentication.service'; +import { + BackfillWarmupProgress, + BackfillWarmupResult, +} from '../../models/stationView.model'; + +/** + * Live updates for the station screens. Today that is one thing: the route index the backfill + * queue runs on being built, which takes seconds and outlives the request that asked for it. + * The hub is JWT-authenticated and only delivers the current user's own events. + */ +@Injectable({ providedIn: 'root' }) +export class StationsLiveService { + private authService = inject(AuthenticationService); + private connection?: HubConnection; + private started?: Promise; + + warmupProgress$ = new Subject(); + warmupFinished$ = new Subject(); + + /** + * Resolves once the connection is up, so a caller can be listening before it asks for work that + * reports here. Queueing first and connecting after loses the event of a build that finishes in + * the meantime, and the page would then wait for something already done. + */ + connect(): Promise { + if (this.started) { + return this.started; + } + const connection = new HubConnectionBuilder() + .withUrl(environment.backend + 'stationsHub', { + accessTokenFactory: () => this.authService.token ?? '', + }) + .withAutomaticReconnect() + .build(); + this.connection = connection; + + connection.on('BackfillWarmupProgress', (progressJson: string) => { + try { + this.warmupProgress$.next(JSON.parse(progressJson) as BackfillWarmupProgress); + } catch (err) { + console.error('Could not parse backfill warm-up progress payload', err); + } + }); + connection.on('BackfillWarmupFinished', (resultJson: string) => { + try { + this.warmupFinished$.next(JSON.parse(resultJson) as BackfillWarmupResult); + } catch (err) { + console.error('Could not parse backfill warm-up result payload', err); + } + }); + + this.started = connection.start(); + return this.started; + } + + disconnect(): void { + this.connection?.stop(); + this.connection = undefined; + this.started = undefined; + } +} diff --git a/OV_DB/OVDBFrontend/src/app/stations/station-backfill/station-backfill.component.html b/OV_DB/OVDBFrontend/src/app/stations/station-backfill/station-backfill.component.html index e41de039..28b6c0a7 100644 --- a/OV_DB/OVDBFrontend/src/app/stations/station-backfill/station-backfill.component.html +++ b/OV_DB/OVDBFrontend/src/app/stations/station-backfill/station-backfill.component.html @@ -1,5 +1,23 @@
- @if (firstLoad()) { + + @if (preparing()) { +
+

{{ 'STATIONS.BACKFILL.PREPARING_TITLE' | translate }}

+

{{ 'STATIONS.BACKFILL.PREPARING' | translate }}

+ + @if (warmupTotal() > 0) { + +

+ {{ 'STATIONS.BACKFILL.PREPARING_PROGRESS' | translate: { processed: warmupProcessed(), total: warmupTotal() } }} +

+ } @else { + + } +
+ } @else if (firstLoad()) { } @else if (!hasWork()) {
diff --git a/OV_DB/OVDBFrontend/src/app/stations/station-backfill/station-backfill.component.scss b/OV_DB/OVDBFrontend/src/app/stations/station-backfill/station-backfill.component.scss index a6eea6e3..7dbaf80c 100644 --- a/OV_DB/OVDBFrontend/src/app/stations/station-backfill/station-backfill.component.scss +++ b/OV_DB/OVDBFrontend/src/app/stations/station-backfill/station-backfill.component.scss @@ -227,6 +227,24 @@ header { padding: 32px 0; } +// The one-off wait at the start of a session. Same centred shape as the all-done panel, with the +// bar held to a readable width rather than stretched across the page. +.preparing { + text-align: center; + padding: 32px 0; + max-width: 420px; + margin: 0 auto; + + mat-progress-bar { + margin-top: 16px; + } + + .remaining { + margin-top: 8px; + opacity: 0.7; + } +} + // Dimmed while the next station is fetched. The map deliberately stays mounted underneath, so the // tile layer and the chosen base layer survive from one station to the next. .work.busy { diff --git a/OV_DB/OVDBFrontend/src/app/stations/station-backfill/station-backfill.component.ts b/OV_DB/OVDBFrontend/src/app/stations/station-backfill/station-backfill.component.ts index 0f69b212..5ef778a0 100644 --- a/OV_DB/OVDBFrontend/src/app/stations/station-backfill/station-backfill.component.ts +++ b/OV_DB/OVDBFrontend/src/app/stations/station-backfill/station-backfill.component.ts @@ -1,6 +1,7 @@ import { ChangeDetectionStrategy, Component, + DestroyRef, ElementRef, Injector, OnDestroy, @@ -10,6 +11,7 @@ import { inject, signal, } from "@angular/core"; +import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; import { LatLng, LatLngBounds, Layer, Map as LeafletMap, circleMarker, polyline } from "leaflet"; import { LeafletModule } from "@bluehalo/ngx-leaflet"; import { MatButton } from "@angular/material/button"; @@ -21,9 +23,11 @@ import { DatePipe, DecimalPipe } from "@angular/common"; import { TranslateModule } from "@ngx-translate/core"; import { firstValueFrom } from "rxjs"; import { ApiService } from "src/app/services/api.service"; +import { StationsLiveService } from "../services/stations-live.service"; import { MapTileLayersService } from "src/app/services/map-tile-layers.service"; import { TranslationService } from "src/app/services/translation.service"; import { + BackfillWarmupProgress, StationBackfillItem, StationVisitLevel, TripCandidateGroup, @@ -59,6 +63,8 @@ import { }) export class StationBackfillComponent implements OnInit, OnDestroy { private apiService = inject(ApiService); + private live = inject(StationsLiveService); + private destroyRef = inject(DestroyRef); private mapTileLayersService = inject(MapTileLayersService); private translationService = inject(TranslationService); private host = inject>(ElementRef); @@ -73,6 +79,19 @@ export class StationBackfillComponent implements OnInit, OnDestroy { item = signal(null); loading = signal(true); + /** + * True while the route index every station in this queue is answered from is being built. It is + * a one-off of a few seconds at the start of a session, and it is shown rather than hidden + * behind the first station's spinner because it is long enough to look like something is wrong. + */ + preparing = signal(false); + private warmup = signal(null); + warmupPercent = computed(() => { + const progress = this.warmup(); + return !progress || progress.total === 0 ? 0 : (progress.processed / progress.total) * 100; + }); + warmupProcessed = computed(() => this.warmup()?.processed ?? 0); + warmupTotal = computed(() => this.warmup()?.total ?? 0); saving = signal(false); selected = signal(null); layers = signal([]); @@ -240,6 +259,87 @@ export class StationBackfillComponent implements OnInit, OnDestroy { expanded = signal(null); ngOnInit(): void { + void this.start(); + } + + /** + * How long to wait for the build to report back before loading anyway. The finished event is + * the normal path; this only catches a connection dropped mid-build, where waiting forever + * would be the worst of the options. Loading anyway is no worse than the old behaviour: the + * request either finds the index warm or sits through the rest of the build. + */ + private static readonly WARMUP_TIMEOUT_MS = 120_000; + private warmupTimeout: ReturnType | null = null; + /** Whether starting the queue is still this page's to do. See claimStart. */ + private pendingStart = true; + + /** + * Gets the route index built before asking for the first station, rather than having that + * request sit through the build with nothing to show for it. + * + * Subscribed before asking, because asking is what starts the build: an event that lands + * between the two would be emitted to nobody, and the page would wait out the timeout for + * something already finished. + */ + private async start(): Promise { + try { + await this.live.connect(); + this.live.warmupProgress$ + .pipe(takeUntilDestroyed(this.destroyRef)) + .subscribe((progress) => this.warmup.set(progress)); + this.live.warmupFinished$ + .pipe(takeUntilDestroyed(this.destroyRef)) + .subscribe(() => this.finishPreparing()); + + const status = await firstValueFrom(this.apiService.prepareBackfill()); + if (status.ready) { + if (this.claimStart()) { + void this.load(); + } + return; + } + } catch { + // Preparing is an optimisation, never a gate. If any of it fails, fall through to the + // request that builds the index itself - which is what this page did before. + if (this.claimStart()) { + void this.load(); + } + return; + } + + // A build already running from an earlier visit may have finished while the answer above was + // in flight, in which case the queue is already on its way and there is nothing to show. + if (!this.pendingStart) { + return; + } + this.preparing.set(true); + this.warmupTimeout = setTimeout( + () => this.finishPreparing(), + StationBackfillComponent.WARMUP_TIMEOUT_MS + ); + } + + /** + * The queue is started exactly once, by whichever of the finished event, the watchdog or the + * answer to prepare gets there first. + */ + private claimStart(): boolean { + if (!this.pendingStart) { + return false; + } + this.pendingStart = false; + return true; + } + + private finishPreparing(): void { + if (!this.claimStart()) { + return; + } + if (this.warmupTimeout !== null) { + clearTimeout(this.warmupTimeout); + this.warmupTimeout = null; + } + this.preparing.set(false); void this.load(); } @@ -310,6 +410,10 @@ export class StationBackfillComponent implements OnInit, OnDestroy { ngOnDestroy(): void { this.mapResize?.disconnect(); + if (this.warmupTimeout !== null) { + clearTimeout(this.warmupTimeout); + } + this.live.disconnect(); } private mapResize: ResizeObserver | null = null; diff --git a/OV_DB/OVDBFrontend/src/assets/i18n/en.json b/OV_DB/OVDBFrontend/src/assets/i18n/en.json index 487783f9..b8716387 100644 --- a/OV_DB/OVDBFrontend/src/assets/i18n/en.json +++ b/OV_DB/OVDBFrontend/src/assets/i18n/en.json @@ -47,6 +47,9 @@ "FEWER_TRIPS": "Show fewer" }, "BACKFILL": { + "PREPARING_TITLE": "Getting the route index ready", + "PREPARING": "Every station here is matched against your route geometry, which is loaded once per session. This takes a few seconds; the queue starts as soon as it is done.", + "PREPARING_PROGRESS": "{{processed}} of {{total}} routes indexed", "REMAINING": "{{count}} still undated", "REGION_PROGRESS": "{{region}}: {{dated}} of {{total}} dated", "NO_CANDIDATES": "No trip of yours passes this station, so there is nothing to date it from yet. Importing more trips may change that.", diff --git a/OV_DB/OVDBFrontend/src/assets/i18n/nl.json b/OV_DB/OVDBFrontend/src/assets/i18n/nl.json index d94eea69..c9ee71bd 100644 --- a/OV_DB/OVDBFrontend/src/assets/i18n/nl.json +++ b/OV_DB/OVDBFrontend/src/assets/i18n/nl.json @@ -46,6 +46,9 @@ "FEWER_TRIPS": "Toon minder" }, "BACKFILL": { + "PREPARING_TITLE": "Route-index wordt opgebouwd", + "PREPARING": "Elk station hier wordt vergeleken met de geometrie van je routes, die één keer per sessie wordt ingeladen. Dit duurt een paar seconden; daarna begint de wachtrij vanzelf.", + "PREPARING_PROGRESS": "{{processed}} van {{total}} routes geïndexeerd", "REMAINING": "Nog {{count}} zonder datum", "REGION_PROGRESS": "{{region}}: {{dated}} van {{total}} gedateerd", "NO_CANDIDATES": "Geen van je ritten komt langs dit station, dus er valt nog niets aan te dateren. Meer ritten importeren kan dat veranderen.", diff --git a/OV_DB/Services/MatcherIndex.cs b/OV_DB/Services/MatcherIndex.cs index d74f13a0..bccd0a26 100644 --- a/OV_DB/Services/MatcherIndex.cs +++ b/OV_DB/Services/MatcherIndex.cs @@ -62,6 +62,11 @@ public interface IMatcherIndexCache Task GetRoutesAsync(int routeCount, Func> build, CancellationToken cancellationToken = default); /// The same, for stations. Task GetStationsAsync(int stationCount, Func> build, CancellationToken cancellationToken = default); + /// + /// Whether a route index built from this count is already in hand — asked, unlike + /// , without building one if it is not. + /// + bool HasRoutes(int routeCount); void Invalidate(); } @@ -92,6 +97,8 @@ public Task GetRoutesAsync(int routeCount, Func GetStationsAsync(int stationCount, Func> build, CancellationToken cancellationToken = default) => _stations.GetAsync(stationCount, build, cancellationToken); + public bool HasRoutes(int routeCount) => _routes.Holds(routeCount); + public void Invalidate() { _routes.Invalidate(); @@ -140,6 +147,18 @@ public async Task GetAsync(int count, Func> build, } } + /// + /// Whether this slot already holds an index for that count. Deliberately outside the + /// gate: a build in flight would otherwise make the caller wait for the very thing it + /// is asking whether it needs to wait for. A build finishing under the read only costs + /// a stale "no", which asks for a warm-up that finds the work already done. + /// + public bool Holds(int count) + { + var index = _index; + return index != null && fingerprint(index) == count; + } + public void Invalidate() { if (!_gate.Wait(TimeSpan.FromSeconds(5))) diff --git a/OV_DB/Services/MatcherWarmupQueue.cs b/OV_DB/Services/MatcherWarmupQueue.cs new file mode 100644 index 00000000..ad19c0ca --- /dev/null +++ b/OV_DB/Services/MatcherWarmupQueue.cs @@ -0,0 +1,53 @@ +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Channels; + +namespace OV_DB.Services +{ + /// + /// The users waiting for the route index to be built. One at a time per user: the build + /// takes seconds, and a second request from the same page would only queue behind the + /// first and report the same thing twice. + /// + /// + /// Mirrors on purpose — same problem, same shape: an + /// endpoint that can only queue, and a hosted service that does the work and reports it. + /// + public interface IMatcherWarmupQueue + { + /// False when this user already has a warm-up queued or running. + bool TryEnqueue(int userId); + + bool IsRunning(int userId); + + IAsyncEnumerable DequeueAllAsync(CancellationToken cancellationToken); + + void MarkFinished(int userId); + } + + public class MatcherWarmupQueue : IMatcherWarmupQueue + { + private readonly Channel _queue = Channel.CreateUnbounded(); + private readonly ConcurrentDictionary _pending = new(); + + public bool TryEnqueue(int userId) + { + if (!_pending.TryAdd(userId, 0)) + return false; + + if (_queue.Writer.TryWrite(userId)) + return true; + + _pending.TryRemove(userId, out _); + return false; + } + + public bool IsRunning(int userId) => _pending.ContainsKey(userId); + + public IAsyncEnumerable DequeueAllAsync(CancellationToken cancellationToken) => + _queue.Reader.ReadAllAsync(cancellationToken); + + public void MarkFinished(int userId) => _pending.TryRemove(userId, out _); + } +} diff --git a/OV_DB/Services/MatcherWarmupService.cs b/OV_DB/Services/MatcherWarmupService.cs new file mode 100644 index 00000000..be193e2a --- /dev/null +++ b/OV_DB/Services/MatcherWarmupService.cs @@ -0,0 +1,104 @@ +using System; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.AspNetCore.SignalR; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; +using Newtonsoft.Json; +using OV_DB.Hubs; + +namespace OV_DB.Services +{ + /// + /// Builds the route index the backfill queue is about to lean on, before the first station + /// is asked for. + /// + /// + /// The build reads every route's geometry and takes seconds, and it used to happen inside + /// whichever request hit a cold index first — an unexplained stall, and long enough to be at + /// the mercy of a proxy timeout. Opening the backfill page is the moment we know it is coming, + /// so it is started here instead and the page watches it fill. + /// + public class MatcherWarmupService( + IServiceProvider serviceProvider, + IMatcherWarmupQueue queue, + IHubContext hubContext, + ILogger logger) : BackgroundService + { + /// Every route would be thousands of messages to say the same thing. + private const int ProgressEveryRoutes = 250; + + /// + /// Not : that posts each report to the thread pool, which + /// reorders them and races the counter this throttles on. + /// + private sealed class InlineProgress(Action report) : IProgress + { + public void Report(T value) => report(value); + } + + protected override async Task ExecuteAsync(CancellationToken stoppingToken) + { + await foreach (var userId in queue.DequeueAllAsync(stoppingToken)) + { + try + { + await WarmAsync(userId, stoppingToken); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + logger.LogError(ex, "Error warming the route index for user {UserId}", userId); + // Told either way: the page waits on this message to start loading, and a + // failed build must not leave it waiting forever. The first station request + // will try the build again and, if it fails too, fail honestly. + await PublishAsync(userId, StationsHub.WarmupFinishedMethod, new { ready = false }); + } + finally + { + queue.MarkFinished(userId); + } + } + } + + private async Task WarmAsync(int userId, CancellationToken cancellationToken) + { + using var scope = serviceProvider.CreateScope(); + var matcher = scope.ServiceProvider.GetRequiredService(); + + var lastReported = 0; + var progress = new InlineProgress(p => + { + // The last report always goes through, so the bar reaches its end rather than + // stopping a few hundred routes short and jumping. + if (p.Processed < p.Total && p.Processed - lastReported < ProgressEveryRoutes) + return; + lastReported = p.Processed; + // Not awaited: the build should not wait on a hub delivery, and a dropped + // progress message costs nothing — the finished event is what the page acts on. + _ = PublishAsync(userId, StationsHub.WarmupProgressMethod, + new { processed = p.Processed, total = p.Total }); + }); + + var started = DateTime.UtcNow; + await matcher.WarmRouteIndexAsync(progress, cancellationToken); + logger.LogInformation("Route index warmed for user {UserId} in {Seconds:F1}s", + userId, (DateTime.UtcNow - started).TotalSeconds); + + await PublishAsync(userId, StationsHub.WarmupFinishedMethod, new { ready = true }); + } + + private async Task PublishAsync(int userId, string method, object payload) + { + try + { + await hubContext.Clients.User(userId.ToString()) + .SendAsync(method, JsonConvert.SerializeObject(payload)); + } + catch (Exception ex) + { + logger.LogError(ex, "Error publishing route index {Method} for user {UserId}", method, userId); + } + } + } +} diff --git a/OV_DB/Services/StationTripMatcher.cs b/OV_DB/Services/StationTripMatcher.cs index e8c8bf83..6ace52bc 100644 --- a/OV_DB/Services/StationTripMatcher.cs +++ b/OV_DB/Services/StationTripMatcher.cs @@ -77,6 +77,9 @@ public sealed record StationCandidate( /// A station an operator says a train calls at, as named by the source it came from. public readonly record struct StopPoint(string Name, double Lattitude, double Longitude); +/// How far the route index has got, for a caller that warmed it deliberately. +public readonly record struct MatcherWarmupProgress(int Processed, int Total); + public interface IStationTripMatcher { Task> FindTripsForStationAsync(int userId, int stationId, CancellationToken cancellationToken = default); @@ -87,6 +90,18 @@ public interface IStationTripMatcher Task FindStationTripsAsync(int userId, int stationId, CancellationToken cancellationToken = default); Task> FindStationsForTripAsync(int userId, int routeInstanceId, CancellationToken cancellationToken = default); Task> MatchStopsAsync(IEnumerable stops, CancellationToken cancellationToken = default); + + /// + /// Whether the route index is already built for the data as it stands, answered without + /// building it. Lets a caller offer to wait knowingly rather than stall mid-request. + /// + Task IsRouteIndexWarmAsync(CancellationToken cancellationToken = default); + + /// + /// Builds the route index unless it is already in hand. Same build the first match would + /// have triggered, only deliberate and watchable — see . + /// + Task WarmRouteIndexAsync(IProgress progress = null, CancellationToken cancellationToken = default); } /// @@ -394,7 +409,21 @@ private async Task GetRouteIndexAsync(CancellationToken cancellation // and it ran on every station in the backfill. The fingerprint only has to notice change, so // rows are as good a signal as rows-with-geometry. var routeCount = await dbContext.Routes.CountAsync(cancellationToken); - return await indexCache.GetRoutesAsync(routeCount, BuildRouteIndexAsync, cancellationToken); + return await indexCache.GetRoutesAsync( + routeCount, token => BuildRouteIndexAsync(routeCount, null, token), cancellationToken); + } + + public async Task IsRouteIndexWarmAsync(CancellationToken cancellationToken = default) + { + var routeCount = await dbContext.Routes.CountAsync(cancellationToken); + return indexCache.HasRoutes(routeCount); + } + + public async Task WarmRouteIndexAsync(IProgress progress = null, CancellationToken cancellationToken = default) + { + var routeCount = await dbContext.Routes.CountAsync(cancellationToken); + await indexCache.GetRoutesAsync( + routeCount, token => BuildRouteIndexAsync(routeCount, progress, token), cancellationToken); } /// The same, for stations — counted and cached apart from the routes. @@ -404,7 +433,11 @@ private async Task GetStationIndexAsync(CancellationToken cancella return await indexCache.GetStationsAsync(stationCount, BuildStationIndexAsync, cancellationToken); } - private async Task BuildRouteIndexAsync(CancellationToken cancellationToken) + /// + /// Rows expected, from the count the fingerprint was taken with. Only for reporting — the + /// fingerprint itself is the rows actually seen, counted below. + /// + private async Task BuildRouteIndexAsync(int total, IProgress progress, CancellationToken cancellationToken) { var segments = new STRtree(); var endpoints = new Dictionary(); @@ -426,6 +459,9 @@ private async Task BuildRouteIndexAsync(CancellationToken cancellati await foreach (var route in routes.WithCancellation(cancellationToken)) { routeRowCount++; + // Reported before the skip below, so a run of routes without geometry still moves the + // bar rather than looking like a stall. + progress?.Report(new MatcherWarmupProgress(routeRowCount, total)); if (route.LineString == null) { continue; @@ -449,6 +485,7 @@ private async Task BuildRouteIndexAsync(CancellationToken cancellati endpoints[route.RouteId] = (first.X, first.Y, last.X, last.Y); } segments.Build(); + progress?.Report(new MatcherWarmupProgress(routeRowCount, Math.Max(total, routeRowCount))); return new RouteIndex { diff --git a/OV_DB/Startup.cs b/OV_DB/Startup.cs index 5006f682..b64bf412 100644 --- a/OV_DB/Startup.cs +++ b/OV_DB/Startup.cs @@ -114,7 +114,8 @@ public void ConfigureServices(IServiceCollection services) var path = context.HttpContext.Request.Path; if (!string.IsNullOrEmpty(accessToken) && (path.StartsWithSegments("/traewellingHub") - || path.StartsWithSegments("/mapGenerationHub"))) + || path.StartsWithSegments("/mapGenerationHub") + || path.StartsWithSegments("/stationsHub"))) { context.Token = accessToken; } @@ -198,6 +199,11 @@ public void ConfigureServices(IServiceCollection services) services.AddSingleton(); services.AddHostedService(); + // Same arrangement for the route index: opening the backfill page queues the build + // rather than stalling the first station request on it. + services.AddSingleton(); + services.AddHostedService(); + // Register named HttpClients for different services to avoid socket exhaustion services.AddHttpClient("OSM", client => { @@ -278,6 +284,7 @@ public void Configure(IApplicationBuilder app, IWebHostEnvironment env) { r.MapHub("/mapGenerationHub"); r.MapHub("/traewellingHub"); + r.MapHub("/stationsHub"); r.MapControllers(); r.MapSwagger(); }); From 511d803d479d646ccbf0f1a5033d0725f704a80a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 19 Sep 2026 22:06:49 +0000 Subject: [PATCH 3/3] fix(traewelling): stop the import waiting out a slow calling-pattern fetch The save is committed before suggestions are computed, so the one upstream call that follows it was holding the response open for something the user had already got. Rate limited, that call sleeps its whole window - up to a minute of a screen that will not finish. It is now bounded twice over. A limiter that is already blocked is not worth trying: the call would sleep the window out before it left, and we know that up front. Otherwise the fetch gets five seconds, cancelled through the same token the limiter's delay and the HTTP call already honour. What that costs is the suggestions for that trip, and they do not come back - they are computed at import and never stored. So the result now says which happened: no stations because there were none to offer, or because the pattern could not be read. The page says so in a snackbar with no action button, which clears itself; there is nothing to act on, so there is nothing to dismiss. Also moved the inbox-row delete ahead of the fetch. With the request's own token now reaching that call, a client that goes away mid-fetch would have skipped the delete and left an imported trip sitting in the inbox looking unimported. The trip is saved either way, so the row stops being pending either way. GetTripStopoversAsync passes cancellation on rather than logging it as a failed fetch - giving up is a decision, not a fault. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012EGxqy7LB2FpCSns4JiNgZ --- OV_DB.Tests/StationSuggestionServiceTests.cs | 118 ++++++++++++++---- OV_DB/Controllers/RoutesController.cs | 23 ++-- OV_DB/Controllers/TraewellingController.cs | 7 +- .../route-instances.component.ts | 10 ++ .../src/app/models/traewelling.model.ts | 2 + .../route-instance-search-dialog.component.ts | 8 ++ OV_DB/OVDBFrontend/src/assets/i18n/en.json | 1 + OV_DB/OVDBFrontend/src/assets/i18n/nl.json | 1 + OV_DB/Services/StationSuggestionService.cs | 71 +++++++++-- OV_DB/Services/TrawellingService.cs | 6 + 10 files changed, 204 insertions(+), 43 deletions(-) diff --git a/OV_DB.Tests/StationSuggestionServiceTests.cs b/OV_DB.Tests/StationSuggestionServiceTests.cs index 7162d6db..35b6b5ed 100644 --- a/OV_DB.Tests/StationSuggestionServiceTests.cs +++ b/OV_DB.Tests/StationSuggestionServiceTests.cs @@ -62,6 +62,21 @@ private static Station StationRow(int id, string name) => private static User TestUser() => new() { Id = 7 }; + /// The ordinary case: Träwelling has budget left, so the call is worth making. + private static ITraewellingRateLimiter NotLimited() + { + var limiter = new Mock(); + limiter.SetupGet(l => l.IsLimited).Returns(false); + return limiter.Object; + } + + private static ITraewellingRateLimiter Limited() + { + var limiter = new Mock(); + limiter.SetupGet(l => l.IsLimited).Returns(true); + return limiter.Object; + } + /// A three-station trip, boarded at the first station and left at the last. private static string StatusPayload(int tripId = TripId, int originId = 1, int destinationId = 3) => $$""" { @@ -84,7 +99,7 @@ public async Task SuggestsTheStationsTheOperatorSaysTheTrainCalledAt() var service = new StationSuggestionService( context, MatcherFor(Candidate(1, "Karlsruhe Hbf", 12), Candidate(2, "Pforzheim Hbf", 30)).Object, - new Mock().Object); + new Mock().Object, NotLimited()); var suggestions = await service.FromStopsAsync(7, [Stop("Karlsruhe Hbf"), Stop("Pforzheim Hbf")]); @@ -103,10 +118,11 @@ public async Task ReadsTheTripOutOfThePayloadTheImportAlreadyHas() .ReturnsAsync([Stopover(1, "Origin"), Stopover(3, "Destination")]); var service = new StationSuggestionService( - context, MatcherFor(Candidate(1, "Origin")).Object, trawelling.Object); - var suggestions = await service.FromTrawellingStatusAsync(TestUser(), StatusPayload()); + context, MatcherFor(Candidate(1, "Origin")).Object, trawelling.Object, NotLimited()); + var result = await service.FromTrawellingStatusAsync(TestUser(), StatusPayload()); - Assert.Equal([1], suggestions.Select(s => s.StationId)); + Assert.Equal([1], result.Stations.Select(s => s.StationId)); + Assert.False(result.Unavailable); // One call, because the trip id was already in the stored payload. trawelling.Verify(t => t.GetTripStopoversAsync(It.IsAny(), TripId, It.IsAny()), Times.Once); } @@ -121,7 +137,7 @@ public async Task TheEndsOfTheJourneyAreFlaggedAndTheMiddleIsNot() var service = new StationSuggestionService( context, MatcherFor(Candidate(1, "A"), Candidate(2, "B"), Candidate(3, "C")).Object, - new Mock().Object); + new Mock().Object, NotLimited()); var suggestions = await service.FromStopsAsync(7, [Stop("A"), Stop("B"), Stop("C")]); @@ -144,7 +160,7 @@ public async Task AnEndAlreadyMarkedLeavesTheStationsBehindItUnflagged() var service = new StationSuggestionService( context, MatcherFor(Candidate(1, "A"), Candidate(2, "B"), Candidate(3, "C")).Object, - new Mock().Object); + new Mock().Object, NotLimited()); var suggestions = await service.FromStopsAsync(7, [Stop("A"), Stop("B"), Stop("C")]); @@ -161,7 +177,7 @@ public async Task ASingleStopIsNotAnEndpoint() await context.SaveChangesAsync(); var service = new StationSuggestionService( - context, MatcherFor(Candidate(1, "A")).Object, new Mock().Object); + context, MatcherFor(Candidate(1, "A")).Object, new Mock().Object, NotLimited()); var suggestions = await service.FromStopsAsync(7, [Stop("A")]); @@ -188,10 +204,10 @@ public async Task OnlyTheSectionRiddenIsSuggested() MatcherFor( Candidate(1, "Before"), Candidate(2, "Origin"), Candidate(3, "Middle"), Candidate(4, "Destination"), Candidate(5, "Beyond")).Object, - trawelling.Object); + trawelling.Object, NotLimited()); - var suggestions = await service.FromTrawellingStatusAsync( - TestUser(), StatusPayload(originId: 20, destinationId: 40)); + var suggestions = (await service.FromTrawellingStatusAsync( + TestUser(), StatusPayload(originId: 20, destinationId: 40))).Stations; // The train ran on past where the user got off, and had been running before they boarded. // Those stations were never reached. @@ -215,10 +231,10 @@ public async Task AStationCalledAtTwiceTakesTheArrivalAfterBoarding() var service = new StationSuggestionService( context, MatcherFor(Candidate(1, "Loop"), Candidate(2, "Origin"), Candidate(3, "Far")).Object, - trawelling.Object); + trawelling.Object, NotLimited()); - var suggestions = await service.FromTrawellingStatusAsync( - TestUser(), StatusPayload(originId: 20, destinationId: 10)); + var suggestions = (await service.FromTrawellingStatusAsync( + TestUser(), StatusPayload(originId: 20, destinationId: 10))).Stations; // Boarded at Origin and left at the second call at Loop, so Far is on the way and the first // call at Loop is not. @@ -237,11 +253,11 @@ public async Task TheWholePatternIsKeptWhenTheEndsAreNotInIt() .ReturnsAsync([Stopover(10, "A"), Stopover(20, "B")]); var service = new StationSuggestionService( - context, MatcherFor(Candidate(1, "A"), Candidate(2, "B")).Object, trawelling.Object); + context, MatcherFor(Candidate(1, "A"), Candidate(2, "B")).Object, trawelling.Object, NotLimited()); // Neither the origin nor the destination appears among the stopovers. - var suggestions = await service.FromTrawellingStatusAsync( - TestUser(), StatusPayload(originId: 999, destinationId: 998)); + var suggestions = (await service.FromTrawellingStatusAsync( + TestUser(), StatusPayload(originId: 999, destinationId: 998))).Stations; // Over-offering is recoverable - the user says no. Dropping stations they did visit is not. Assert.Equal([1, 2], suggestions.Select(s => s.StationId)); @@ -272,7 +288,7 @@ public async Task OnlyTheStopsWithCoordinatesReachTheMatcher() list.Where(s => s.Name == "Origin").Select(s => Candidate(1, "Origin")).ToList()); }); - var service = new StationSuggestionService(context, matcher.Object, trawelling.Object); + var service = new StationSuggestionService(context, matcher.Object, trawelling.Object, NotLimited()); await service.FromTrawellingStatusAsync(TestUser(), StatusPayload()); Assert.Equal(["Origin"], seen[0].Select(s => s.Name)); @@ -289,7 +305,7 @@ public async Task AStationAlreadyMarkedIsNotSuggestedAgain() var service = new StationSuggestionService( context, MatcherFor(Candidate(1, "Karlsruhe Hbf"), Candidate(2, "Pforzheim Hbf")).Object, - new Mock().Object); + new Mock().Object, NotLimited()); var suggestions = await service.FromStopsAsync(7, [Stop("Karlsruhe Hbf"), Stop("Pforzheim Hbf")]); @@ -312,7 +328,7 @@ public async Task AStationTheUserWavedAwayIsNotSuggestedAgain() var service = new StationSuggestionService( context, MatcherFor(Candidate(1, "Karlsruhe Hbf"), Candidate(2, "Pforzheim Hbf")).Object, - new Mock().Object); + new Mock().Object, NotLimited()); var suggestions = await service.FromStopsAsync(7, [Stop("Karlsruhe Hbf"), Stop("Pforzheim Hbf")]); @@ -333,7 +349,7 @@ public async Task HiddenAndSpecialStationsAreNeverSuggested() var service = new StationSuggestionService( context, MatcherFor(Candidate(1, "Depot"), Candidate(2, "Marker"), Candidate(3, "Pforzheim Hbf")).Object, - new Mock().Object); + new Mock().Object, NotLimited()); var suggestions = await service.FromStopsAsync( 7, [Stop("Depot"), Stop("Marker"), Stop("Pforzheim Hbf")]); @@ -352,9 +368,9 @@ public async Task WithoutATripThereIsNothingToAsk(string payload) // Check-ins with no trip behind them happen: manual entries, older payloads. Suggestions sit // on top of an import that has already succeeded, so this has to be quiet, not an error. var trawelling = new Mock(); - var service = new StationSuggestionService(NewContext(), MatcherFor().Object, trawelling.Object); + var service = new StationSuggestionService(NewContext(), MatcherFor().Object, trawelling.Object, NotLimited()); - Assert.Empty(await service.FromTrawellingStatusAsync(TestUser(), payload)); + Assert.Empty((await service.FromTrawellingStatusAsync(TestUser(), payload)).Stations); trawelling.Verify( t => t.GetTripStopoversAsync(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); @@ -369,8 +385,62 @@ public async Task ATripWithNoCallingPatternSuggestsNothing() trawelling.Setup(t => t.GetTripStopoversAsync(It.IsAny(), TripId, It.IsAny())) .ReturnsAsync([]); - var service = new StationSuggestionService(NewContext(), MatcherFor().Object, trawelling.Object); + var service = new StationSuggestionService(NewContext(), MatcherFor().Object, trawelling.Object, NotLimited()); + + Assert.Empty((await service.FromTrawellingStatusAsync(TestUser(), StatusPayload())).Stations); + } + + [Fact] + public async Task ARateLimitedTraewellingIsNotWaitedOnAtAll() + { + // The trip is already saved by the time this runs, and the limiter sleeps out its whole + // window. Waiting a minute to find out what we already know is the worst of both. + var trawelling = new Mock(); + var service = new StationSuggestionService( + NewContext(), MatcherFor().Object, trawelling.Object, Limited()); + + var result = await service.FromTrawellingStatusAsync(TestUser(), StatusPayload()); + + Assert.True(result.Unavailable); + Assert.Empty(result.Stations); + trawelling.Verify( + t => t.GetTripStopoversAsync(It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never); + } + + [Fact] + public async Task AFetchThatOutlastsItsWelcomeIsReportedRatherThanWaitedOut() + { + // Cancelled from inside, which is what the timeout does. The caller's own token is still + // live, so this is the service giving up rather than the request going away. + var trawelling = new Mock(); + trawelling.Setup(t => t.GetTripStopoversAsync(It.IsAny(), TripId, It.IsAny())) + .ThrowsAsync(new OperationCanceledException()); + + var service = new StationSuggestionService( + NewContext(), MatcherFor().Object, trawelling.Object, NotLimited()); + + var result = await service.FromTrawellingStatusAsync(TestUser(), StatusPayload()); + + Assert.True(result.Unavailable); + Assert.Empty(result.Stations); + } + + [Fact] + public async Task ACallerWhoGaveUpIsNotToldTheTripHasNoPattern() + { + // The request itself was aborted, so there is nobody to tell and nothing to report. That + // cancellation belongs to the caller and is passed back rather than swallowed as a result. + var trawelling = new Mock(); + trawelling.Setup(t => t.GetTripStopoversAsync(It.IsAny(), TripId, It.IsAny())) + .ThrowsAsync(new OperationCanceledException()); + + var service = new StationSuggestionService( + NewContext(), MatcherFor().Object, trawelling.Object, NotLimited()); + using var aborted = new CancellationTokenSource(); + await aborted.CancelAsync(); - Assert.Empty(await service.FromTrawellingStatusAsync(TestUser(), StatusPayload())); + await Assert.ThrowsAnyAsync( + () => service.FromTrawellingStatusAsync(TestUser(), StatusPayload(), aborted.Token)); } } diff --git a/OV_DB/Controllers/RoutesController.cs b/OV_DB/Controllers/RoutesController.cs index b0a6b91d..2ad8efd7 100644 --- a/OV_DB/Controllers/RoutesController.cs +++ b/OV_DB/Controllers/RoutesController.cs @@ -1169,7 +1169,7 @@ public async Task UpdateInstance([FromBody] RouteInstanceUpdate up } await _context.SaveChangesAsync(); - var suggestions = new List(); + var suggestions = StationSuggestionResult.Nothing; if (update.TraewellingStatusId.HasValue) { // This is the import moment, and the only place the calling pattern is worth @@ -1180,16 +1180,20 @@ public async Task UpdateInstance([FromBody] RouteInstanceUpdate up .Select(s => s.PayloadJson) .FirstOrDefaultAsync(); + // The status is imported now; its Träwelling inbox row is no longer pending. + // Before the suggestions rather than after: the trip is saved either way, and the + // fetch below can be abandoned — if the client has gone, skipping this would leave + // an imported trip sitting in the inbox pretending it still needs importing. + await _context.TrawellingInboxStatuses + .Where(s => s.UserId == userIdClaim && s.TrawellingStatusId == update.TraewellingStatusId.Value) + .ExecuteDeleteAsync(); + var user = await _context.Users.SingleOrDefaultAsync(u => u.Id == userIdClaim); if (payload != null && user != null) { - suggestions = await suggestionService.FromTrawellingStatusAsync(user, payload); + suggestions = await suggestionService.FromTrawellingStatusAsync( + user, payload, HttpContext.RequestAborted); } - - // The status is imported now; its Träwelling inbox row is no longer pending - await _context.TrawellingInboxStatuses - .Where(s => s.UserId == userIdClaim && s.TrawellingStatusId == update.TraewellingStatusId.Value) - .ExecuteDeleteAsync(); } // Stations this trip calls at that are not marked. Proposals only — the response carries @@ -1198,7 +1202,10 @@ await _context.TrawellingInboxStatuses return Ok(new { routeInstanceId = savedInstance?.RouteInstanceId, - stationSuggestions = suggestions + stationSuggestions = suggestions.Stations, + // So the page can say the pattern went unread, rather than silently offering + // nothing and leaving it looking like there was nothing to offer. + stationSuggestionsUnavailable = suggestions.Unavailable }); } diff --git a/OV_DB/Controllers/TraewellingController.cs b/OV_DB/Controllers/TraewellingController.cs index 32517bd8..4f018b6c 100644 --- a/OV_DB/Controllers/TraewellingController.cs +++ b/OV_DB/Controllers/TraewellingController.cs @@ -632,8 +632,8 @@ public async Task LinkToRouteInstance([FromBody] LinkToRouteInsta return BadRequest("Failed to link status to RouteInstance. Status may already be linked or RouteInstance may not exist."); var suggestions = payload == null - ? [] - : await suggestionService.FromTrawellingStatusAsync(user, payload); + ? StationSuggestionResult.Nothing + : await suggestionService.FromTrawellingStatusAsync(user, payload, HttpContext.RequestAborted); return Ok(new { @@ -647,7 +647,8 @@ public async Task LinkToRouteInstance([FromBody] LinkToRouteInsta trawellingStatusId = routeInstance.TrawellingStatusId }, routeInstanceId = routeInstance.RouteInstanceId, - stationSuggestions = suggestions + stationSuggestions = suggestions.Stations, + stationSuggestionsUnavailable = suggestions.Unavailable }); } catch (Exception ex) diff --git a/OV_DB/OVDBFrontend/src/app/admin/route-instances/route-instances.component.ts b/OV_DB/OVDBFrontend/src/app/admin/route-instances/route-instances.component.ts index d745491a..20bfc52c 100644 --- a/OV_DB/OVDBFrontend/src/app/admin/route-instances/route-instances.component.ts +++ b/OV_DB/OVDBFrontend/src/app/admin/route-instances/route-instances.component.ts @@ -316,8 +316,18 @@ export class RouteInstancesComponent implements OnInit { const body = response as { routeInstanceId?: number; stationSuggestions?: StationSuggestion[]; + stationSuggestionsUnavailable?: boolean; }; const suggestions = body?.stationSuggestions; + if (body?.stationSuggestionsUnavailable) { + // Said once and gone on its own: nothing was lost that the user can act on, and a notice + // they have to dismiss would cost more attention than the news is worth. + this.snackBar.open( + this.translateService.instant('ROUTEINSTANCE.SUGGESTIONS_UNAVAILABLE'), + undefined, + { duration: 6000 } + ); + } if (!suggestions?.length) { // No Träwelling calling pattern on this save, so this may instead be the first trip on a // freshly imported OSM route — which is the moment its stops finally have a date. diff --git a/OV_DB/OVDBFrontend/src/app/models/traewelling.model.ts b/OV_DB/OVDBFrontend/src/app/models/traewelling.model.ts index dc28b9d6..5b6b0556 100644 --- a/OV_DB/OVDBFrontend/src/app/models/traewelling.model.ts +++ b/OV_DB/OVDBFrontend/src/app/models/traewelling.model.ts @@ -148,6 +148,8 @@ export interface LinkToRouteInstanceResponse { /** Linking is an import, so it carries the same station suggestions a fresh import would. */ routeInstanceId?: number; stationSuggestions?: StationSuggestion[]; + /** The calling pattern could not be read, so no stations are being offered for this trip. */ + stationSuggestionsUnavailable?: boolean; } // Ignore functionality diff --git a/OV_DB/OVDBFrontend/src/app/traewelling/components/route-instance-search-dialog/route-instance-search-dialog.component.ts b/OV_DB/OVDBFrontend/src/app/traewelling/components/route-instance-search-dialog/route-instance-search-dialog.component.ts index ce199f58..8a806b77 100644 --- a/OV_DB/OVDBFrontend/src/app/traewelling/components/route-instance-search-dialog/route-instance-search-dialog.component.ts +++ b/OV_DB/OVDBFrontend/src/app/traewelling/components/route-instance-search-dialog/route-instance-search-dialog.component.ts @@ -198,6 +198,14 @@ export class RouteInstanceSearchDialogComponent implements OnInit { * pattern was fetched once, here, and is not stored. */ private offerStationSuggestions(response: LinkToRouteInstanceResponse): void { + if (response.stationSuggestionsUnavailable) { + // No action button: this is news, not a question, and it should clear itself. + this.snackBar.open( + this.translateService.instant('ROUTEINSTANCE.SUGGESTIONS_UNAVAILABLE'), + undefined, + { duration: 6000 } + ); + } const stations = response.stationSuggestions; if (!stations?.length) { return; diff --git a/OV_DB/OVDBFrontend/src/assets/i18n/en.json b/OV_DB/OVDBFrontend/src/assets/i18n/en.json index b8716387..f350d4fa 100644 --- a/OV_DB/OVDBFrontend/src/assets/i18n/en.json +++ b/OV_DB/OVDBFrontend/src/assets/i18n/en.json @@ -152,6 +152,7 @@ "ROUTES.ASSIGNREGIONS": "Assign regions", "STATS.COMPACT": "Compact", "ROUTEINSTANCE.SAVE_ERROR": "Could not save the trip. Please try again.", + "ROUTEINSTANCE.SUGGESTIONS_UNAVAILABLE": "The trip was saved, but this trip's calling pattern could not be read, so no stations are being suggested.", "TRAEWELLING.IGNORE_ERROR": "Could not ignore the trip. Please try again.", "IMPORTER.STEP1": "Search for a line and a type, add a network to limit results.", "IMPORTER.REFERENCE": "Name of the line", diff --git a/OV_DB/OVDBFrontend/src/assets/i18n/nl.json b/OV_DB/OVDBFrontend/src/assets/i18n/nl.json index c9ee71bd..198a9bce 100644 --- a/OV_DB/OVDBFrontend/src/assets/i18n/nl.json +++ b/OV_DB/OVDBFrontend/src/assets/i18n/nl.json @@ -150,6 +150,7 @@ "ROUTES.ASSIGNREGIONS": "Regio's toewijzen", "STATS.COMPACT": "Compact", "ROUTEINSTANCE.SAVE_ERROR": "Kon de rit niet opslaan. Probeer het opnieuw.", + "ROUTEINSTANCE.SUGGESTIONS_UNAVAILABLE": "De rit is opgeslagen, maar de stopplaatsen van deze rit konden niet worden opgehaald. Er worden daarom geen stations voorgesteld.", "TRAEWELLING.IGNORE_ERROR": "Kon de rit niet negeren. Probeer het opnieuw.", "IMPORTER.STEP1": "Zoek op een naam en een type. Voeg een netwerk toe om resultaten te beperken.", "IMPORTER.REFERENCE": "Naam van de lijn", diff --git a/OV_DB/Services/StationSuggestionService.cs b/OV_DB/Services/StationSuggestionService.cs index 0f25e45c..ea724fb4 100644 --- a/OV_DB/Services/StationSuggestionService.cs +++ b/OV_DB/Services/StationSuggestionService.cs @@ -3,6 +3,7 @@ using OV_DB.Models; using OVDB_database.Database; using OVDB_database.Models; +using System; using System.Collections.Generic; using System.Linq; using System.Threading; @@ -10,9 +11,27 @@ namespace OV_DB.Services; +/// +/// What an import's calling pattern turned into, and whether it could be read at all. +/// +/// +/// The two are genuinely different and the user deserves to be told which: no stations means +/// there was nothing left to offer, while Unavailable means the question was +/// never answered, and those suggestions are not coming back — they are computed at import and +/// never stored. +/// +public sealed record StationSuggestionResult(List Stations, bool Unavailable) +{ + /// Asked and answered; there was simply nothing to offer. + public static StationSuggestionResult Nothing => new([], false); + + /// Never asked, or asked and not answered in time. + public static StationSuggestionResult NotAvailable => new([], true); +} + public interface IStationSuggestionService { - Task> FromTrawellingStatusAsync(User user, string statusPayloadJson, CancellationToken cancellationToken = default); + Task FromTrawellingStatusAsync(User user, string statusPayloadJson, CancellationToken cancellationToken = default); Task> FromStopsAsync(int userId, IEnumerable stops, CancellationToken cancellationToken = default); } @@ -27,17 +46,26 @@ public interface IStationSuggestionService public class StationSuggestionService( OVDBDatabaseContext dbContext, IStationTripMatcher matcher, - ITrawellingService trawellingService) : IStationSuggestionService + ITrawellingService trawellingService, + ITraewellingRateLimiter rateLimiter) : IStationSuggestionService { + /// + /// How long the one upstream call is given. The trip is already saved by the time this runs, + /// so the only thing waiting longer buys is suggestions — and the user is sitting in front of + /// a screen that will not finish until this does. A second or two is a normal answer; anything + /// beyond this is Träwelling being slow, and the rate limiter alone can sleep a minute. + /// + private static readonly TimeSpan StopoverFetchTimeout = TimeSpan.FromSeconds(5); + /// /// Reads the trip id out of a stored check-in payload and asks Träwelling what that trip calls /// at. The payload is already on hand at import, so this costs one API call rather than two. /// - public async Task> FromTrawellingStatusAsync(User user, string statusPayloadJson, CancellationToken cancellationToken = default) + public async Task FromTrawellingStatusAsync(User user, string statusPayloadJson, CancellationToken cancellationToken = default) { if (string.IsNullOrWhiteSpace(statusPayloadJson)) { - return []; + return StationSuggestionResult.Nothing; } TrawellingStatus status; @@ -47,16 +75,43 @@ public async Task> FromTrawellingStatusAsync(User use } catch (JsonException) { - return []; + return StationSuggestionResult.Nothing; } var tripId = status?.Checkin?.Trip ?? 0; if (tripId == 0) { - return []; + return StationSuggestionResult.Nothing; + } + + // Known to be blocked, so there is nothing to find out by trying: the call would sleep out + // the window before it even left. Say so now rather than a minute from now. + if (rateLimiter.IsLimited) + { + return StationSuggestionResult.NotAvailable; + } + + List stopovers; + using var timeout = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + timeout.CancelAfter(StopoverFetchTimeout); + try + { + stopovers = await trawellingService.GetTripStopoversAsync(user, tripId, timeout.Token); + } + catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) + { + // Ours, not the caller's: the request is still live and wants an answer, just without + // the suggestions. + return StationSuggestionResult.NotAvailable; } - var stopovers = await trawellingService.GetTripStopoversAsync(user, tripId, cancellationToken); + if (stopovers.Count == 0) + { + // Either the trip publishes no calling pattern or the fetch failed — GetTripStopoversAsync + // reports both by answering with nothing. Told as one thing, because to the user they are: + // this trip's calling pattern could not be read, so no stations are being offered. + return StationSuggestionResult.NotAvailable; + } // Only the section actually ridden. The endpoint answers with the whole trip, and the // stations before boarding or beyond where the user got off were never reached — offering @@ -67,7 +122,7 @@ public async Task> FromTrawellingStatusAsync(User use .Where(s => s.Station?.Latitude != null && s.Station?.Longitude != null) .Select(s => new StopPoint(s.Station.Name, s.Station.Latitude.Value, s.Station.Longitude.Value)); - return await FromStopsAsync(user.Id, stops, cancellationToken); + return new StationSuggestionResult(await FromStopsAsync(user.Id, stops, cancellationToken), false); } /// diff --git a/OV_DB/Services/TrawellingService.cs b/OV_DB/Services/TrawellingService.cs index 35b134c9..6c471b4e 100644 --- a/OV_DB/Services/TrawellingService.cs +++ b/OV_DB/Services/TrawellingService.cs @@ -1880,6 +1880,12 @@ public async Task> GetTripStopoversAsync(User user, int var stopovers = data[tripId.ToString()] ?? data.Properties().FirstOrDefault()?.Value; return stopovers?.ToObject>() ?? []; } + catch (OperationCanceledException) + { + // The caller gave up waiting, which is a decision rather than a fault. Passed on + // so it can say the pattern went unread instead of being told there was none. + throw; + } catch (Exception ex) { // A missing calling pattern costs suggestions, not the import. Never fail the one