diff --git a/AGENTS.md b/AGENTS.md index 3ce7914..d8b881f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -142,6 +142,15 @@ say so explicitly in the change description. runs six times over once six schools exist. `OpenSchool` only subscribes to clock frames. - The menu polls `GET /api/schools` once a second and patches its cards in place. Rebuilding the list on every refresh would drop focus and swallow clicks. +- **An abandoned `WaitToReadAsync` stays queued on its channel.** `GameClient.RunSendLoopAsync` + keeps its two wait tasks across iterations for that reason; recreating both on every idle pass + leaked the loser of `Task.WhenAny` once per clock frame. +- **A map snapshot is not bounded by `MaxMessageSize`.** That constant limits what the server + reads. Outbound snapshots are sized with `ProtocolCodec.MapSnapshotSize`, because a school the + player enlarged in the create editor passes 8 KiB at roughly sixty furnished rooms. +- **A worker that dies must tell the supervisor** (`GameCommand.WorkerFailed`). It owns the table, + so a school that removed itself would break invariant 3 — and a school that removes nothing + leaves a card in the menu whose clock never moves again. - The client outbox drops the oldest frame under pressure. That is correct for clock frames and wrong for anything that must arrive exactly once — such a message would need its own path. - `erasableSyntaxOnly` is off in `tsconfig.app.json` on purpose: constructor parameter properties diff --git a/README.md b/README.md index 2368866..46f9c5b 100644 --- a/README.md +++ b/README.md @@ -118,6 +118,7 @@ Simulation tunables live under the `Simulation` section of | `SavesDirectory` | `saves` | per-school JSON files | | `ModsDirectory` | `mods` | pack folders; `core` is required | | `SaveIntervalSeconds` | 30 | rare clock snapshot; not every tick | +| `MinSaveIntervalMilliseconds` | 1000 | shortest gap between saves caused by pause or speed | ## What is deliberately missing diff --git a/docs/architecture.md b/docs/architecture.md index 1383926..57553f9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -97,8 +97,11 @@ cannot land between a mouse-down and a click. ## Saves Each school is a JSON file under `Simulation:SavesDirectory` (`saves/{id}.json` plus `index.json` -for the next id). The worker writes on create, pause, speed change, shutdown, and on a rare clock -snapshot (`SaveIntervalSeconds`, 30 by default) — never on every tick. The file also stores the +for the next id). The worker writes on create, on shutdown, and on a rare clock snapshot +(`SaveIntervalSeconds`, 30 by default) — never on every tick. Pause and speed changes are written +too, but coalesced to at most one write per `MinSaveIntervalMilliseconds`: a client can send those +as fast as the socket allows, and each one is a file write on the school's own thread. Shutdown +always flushes, so a pause is never lost. The file also stores the mod pack ids and the map layout; the catalog is loaded again from `mods/` on start. A missing mod folder or a map that no longer validates leaves the file in place and that school unstarted. diff --git a/docs/protocol.md b/docs/protocol.md index 9cc0668..4d0fcce 100644 --- a/docs/protocol.md +++ b/docs/protocol.md @@ -248,7 +248,9 @@ Each node: ## Guarantees and limits -- Frames larger than 8 KiB are refused with close status `1009 MessageTooBig`. +- **Inbound** frames larger than 8 KiB are refused with close status `1009 MessageTooBig`. That + limit is about what the server reads; it does not bound what the server sends. A `MapSnapshot` + of a large school legitimately exceeds it, and the server sizes that frame from the message. - A malformed frame closes the connection with `1007 InvalidPayloadData`. - Unknown message ids are ignored rather than fatal, so new ids can be added without breaking older clients within the same protocol version. diff --git a/src/HSchool.Client/src/net/connection.ts b/src/HSchool.Client/src/net/connection.ts index cfe2145..9753cae 100644 --- a/src/HSchool.Client/src/net/connection.ts +++ b/src/HSchool.Client/src/net/connection.ts @@ -94,13 +94,13 @@ export class GameConnection { this.connect(); } - /** Starts watching a school; its calendar starts running server-side. */ + /** Starts watching a school. Its calendar was already running — opening only subscribes. */ openSchool(schoolId: number): void { this.openSchoolId = schoolId; this.send(encodeOpenSchool(schoolId)); } - /** Back to the menu; the school stops ticking. */ + /** Back to the menu. Clock frames stop; the school keeps running until the player pauses it. */ closeSchool(): void { if (this.openSchoolId === null) { return; diff --git a/src/HSchool.Protocol/ProtocolCodec.cs b/src/HSchool.Protocol/ProtocolCodec.cs index 46e909d..a9b3971 100644 --- a/src/HSchool.Protocol/ProtocolCodec.cs +++ b/src/HSchool.Protocol/ProtocolCodec.cs @@ -1,3 +1,5 @@ +using System.Text; + namespace HSchool.Protocol; /// @@ -99,6 +101,38 @@ public static class ProtocolCodec return writer.Position; } + /// + /// Bytes needs for this message. + /// + /// A school's map has no fixed size — the create editor lets a player add rooms — so a + /// snapshot can outgrow . That limit guards + /// what the server *reads*; callers size an outbound snapshot from the message itself. + /// + public static int MapSnapshotSize(ServerMapSnapshotMessage message) + { + var size = sizeof(byte) + sizeof(int) + sizeof(ushort); + + foreach (var node in message.Nodes) + { + size += sizeof(byte); + size += StringSize(node.Id) + StringSize(node.ParentId) + StringSize(node.Name); + + size += sizeof(byte); + foreach (var item in node.Items) + { + size += StringSize(item); + } + + size += sizeof(byte); + foreach (var position in node.Positions) + { + size += StringSize(position); + } + } + + return size; + } + public static int WriteMapSnapshot(Span destination, ServerMapSnapshotMessage message) { if (message.Nodes.Count > ushort.MaxValue) @@ -249,6 +283,9 @@ public static class ProtocolCodec return new ServerMapSnapshotMessage(schoolId, nodes); } + /// Matches : a u16 length plus UTF-8. + private static int StringSize(string value) => sizeof(ushort) + Encoding.UTF8.GetByteCount(value); + private static void Expect(ref PacketReader reader, MessageType expected) { var actual = reader.ReadMessageType(); diff --git a/src/HSchool.Server/Game/GameCommand.cs b/src/HSchool.Server/Game/GameCommand.cs index 39bd383..4481608 100644 --- a/src/HSchool.Server/Game/GameCommand.cs +++ b/src/HSchool.Server/Game/GameCommand.cs @@ -35,4 +35,10 @@ internal abstract record GameCommand /// Stops every worker, re-reads the save directory, starts workers from those files. internal sealed record ReloadSaves(TaskCompletionSource Result) : GameCommand; + + /// + /// A worker thread ended on an exception. The supervisor owns the table, so the worker asks + /// it to drop the school instead of removing itself. + /// + internal sealed record WorkerFailed(int SchoolId) : GameCommand; } diff --git a/src/HSchool.Server/Game/GameLoopService.cs b/src/HSchool.Server/Game/GameLoopService.cs index 3579d0a..c5130a1 100644 --- a/src/HSchool.Server/Game/GameLoopService.cs +++ b/src/HSchool.Server/Game/GameLoopService.cs @@ -144,9 +144,39 @@ internal sealed class GameLoopService( case GameCommand.ReloadSaves reload: await HandleReloadAsync(reload).ConfigureAwait(false); break; + + case GameCommand.WorkerFailed failed: + HandleWorkerFailed(failed.SchoolId); + break; } } + /// + /// A school's thread died. Drop it from the table so the menu stops drawing a card whose clock + /// never moves again, and tell anybody watching it to go back to the menu. The save file stays + /// where it is — a restart or reload-schools is the way back. + /// + private void HandleWorkerFailed(int schoolId) + { + if (!_workers.ContainsKey(schoolId)) + { + return; + } + + Untrack(schoolId); + + foreach (var client in clients.All) + { + if (client.OpenSchoolId == schoolId) + { + client.OpenSchoolId = null; + SendSchoolGone(client, schoolId); + } + } + + logger.LogError("School {SchoolId} stopped after a worker failure; its save file is unchanged.", schoolId); + } + private async Task HandleCreateAsync(GameCommand.CreateSchool command) { try @@ -373,6 +403,11 @@ internal sealed class GameLoopService( await worker.StopAsync(persist: false).ConfigureAwait(false); } } + + if (_workers.Count > 0) + { + logger.LogInformation("Restored {Count} school(s) from disk.", _workers.Count); + } } private async Task StopAllWorkersAsync(bool persist) @@ -386,11 +421,6 @@ internal sealed class GameLoopService( { await Task.WhenAll(stopping).ConfigureAwait(false); } - - if (_workers.Count > 0) - { - logger.LogInformation("Restored {Count} school(s) from disk.", _workers.Count); - } } private SchoolWorker SpawnWorker( @@ -416,6 +446,7 @@ internal sealed class GameLoopService( metrics, store, mods, + onFailed: schoolId => commands.Enqueue(new GameCommand.WorkerFailed(schoolId)), loggerFactory.CreateLogger($"HSchool.Server.Game.SchoolWorker.{id}")); private void Track(SchoolWorker worker) diff --git a/src/HSchool.Server/Game/SchoolStore.cs b/src/HSchool.Server/Game/SchoolStore.cs index 1d5c7f6..40844d7 100644 --- a/src/HSchool.Server/Game/SchoolStore.cs +++ b/src/HSchool.Server/Game/SchoolStore.cs @@ -92,6 +92,11 @@ internal sealed class SchoolStore { var saves = new List(); + // The id inside the file decides which school this is and which file it is saved back to, + // so two files claiming the same id would give the menu two cards over one worker and then + // overwrite each other on the next save. + var claimed = new Dictionary(); + foreach (var path in Directory.EnumerateFiles(DirectoryPath, "*.json")) { if (string.Equals(Path.GetFileName(path), IndexFileName, StringComparison.OrdinalIgnoreCase)) @@ -109,6 +114,15 @@ internal sealed class SchoolStore continue; } + if (save.Id <= 0) + { + _logger.LogWarning( + "Save {Path} has id {Id}; ids start at 1. Leaving the file in place.", + path, + save.Id); + continue; + } + if (!GameClock.IsValidStartDate(save.GameTime)) { _logger.LogWarning( @@ -117,6 +131,17 @@ internal sealed class SchoolStore continue; } + // Claimed last, so a file rejected above does not reserve an id a good file needs. + if (!claimed.TryAdd(save.Id, path)) + { + _logger.LogWarning( + "Save {Path} claims id {Id}, already taken by {Owner}; leaving the file in place.", + path, + save.Id, + claimed[save.Id]); + continue; + } + saves.Add(new SchoolSave { Format = save.Format, diff --git a/src/HSchool.Server/Game/SchoolWorker.cs b/src/HSchool.Server/Game/SchoolWorker.cs index b3650f9..6664573 100644 --- a/src/HSchool.Server/Game/SchoolWorker.cs +++ b/src/HSchool.Server/Game/SchoolWorker.cs @@ -29,6 +29,7 @@ internal sealed class SchoolWorker private readonly bool _isNew; private readonly IReadOnlyList? _modIds; private readonly MapLayout? _savedMap; + private readonly Action _onFailed; private readonly int _id; private readonly string _name; @@ -40,6 +41,8 @@ internal sealed class SchoolWorker private School? _school; private Task? _run; private bool _persistOnStop = true; + private bool _settingsDirty; + private long _lastSettingsSave; public SchoolWorker( int id, @@ -55,6 +58,7 @@ internal sealed class SchoolWorker GameMetrics metrics, SchoolStore store, ModContent mods, + Action onFailed, ILogger logger) { _id = id; @@ -70,6 +74,7 @@ internal sealed class SchoolWorker _metrics = metrics; _store = store; _mods = mods; + _onFailed = onFailed; _logger = logger; _snapshot = new SchoolState(id, name, time, running, (byte)speedIndex); } @@ -127,11 +132,35 @@ internal sealed class SchoolWorker { _logger.LogWarning(ex, "School {SchoolId} was not started; the save file is unchanged.", _id); _started.TrySetException(ex); + ReportFailure(); } catch (Exception ex) { _logger.LogError(ex, "School {SchoolId} worker died.", _id); _started.TrySetException(ex); + ReportFailure(); + } + } + + /// + /// Tells the supervisor this school is gone. Without it a dead worker stayed in the table and + /// the menu kept drawing its card with a frozen clock, as if the school were alive. + /// + private void ReportFailure() + { + if (_stopping.IsCancellationRequested) + { + // Already being torn down on purpose; the supervisor knows. + return; + } + + try + { + _onFailed(_id); + } + catch (Exception ex) + { + _logger.LogError(ex, "Could not report the failure of school {SchoolId}.", _id); } } @@ -219,6 +248,8 @@ internal sealed class SchoolWorker BroadcastClock(); } + FlushSettings(); + if (Stopwatch.GetElapsedTime(lastSave) >= _options.SaveInterval) { Persist(); @@ -272,32 +303,45 @@ internal sealed class SchoolWorker while (_mailbox.Reader.TryRead(out var command)) { - switch (command) + // Every command here was triggered by a browser. One of them failing — an oversized + // snapshot, a client that vanished mid-send — must cost that command, not the school. + try { - case WorkerCommand.Open open: - open.Client.OpenSchoolId = _id; - SendMapSnapshot(open.Client, school); - BroadcastClockTo(open.Client, school); - break; + switch (command) + { + case WorkerCommand.Open open: + open.Client.OpenSchoolId = _id; + SendMapSnapshot(open.Client, school); + BroadcastClockTo(open.Client, school); + break; - case WorkerCommand.Close close: - var leaving = _clients.Find(close.PlayerId); - if (leaving?.OpenSchoolId == _id) - { - leaving.OpenSchoolId = null; - } + case WorkerCommand.Close close: + var leaving = _clients.Find(close.PlayerId); + if (leaving?.OpenSchoolId == _id) + { + leaving.OpenSchoolId = null; + } - break; + break; - case WorkerCommand.SetRunning setRunning: - school.Clock.IsRunning = setRunning.Running; - dirty = true; - break; + case WorkerCommand.SetRunning setRunning: + school.Clock.IsRunning = setRunning.Running; + dirty = true; + break; - case WorkerCommand.SetSpeed setSpeed: - school.Clock.SpeedIndex = setSpeed.SpeedIndex; - dirty = true; - break; + case WorkerCommand.SetSpeed setSpeed: + school.Clock.SpeedIndex = setSpeed.SpeedIndex; + dirty = true; + break; + } + } + catch (Exception ex) + { + _logger.LogError( + ex, + "Command {Command} failed for school {SchoolId}; the school keeps running.", + command.GetType().Name, + _id); } } @@ -305,10 +349,29 @@ internal sealed class SchoolWorker { PublishSnapshot(); BroadcastClock(); - Persist(); + + // Not written here: a client can send SetSpeed as fast as the socket allows, and each + // one used to be a synchronous file write on this thread. FlushSettings coalesces them. + _settingsDirty = true; } } + /// + /// Writes pause/speed changes, at most once per . + /// A single click still lands within that window; a burst collapses into one write. + /// + private void FlushSettings() + { + if (!_settingsDirty || Stopwatch.GetElapsedTime(_lastSettingsSave) < _options.MinSaveInterval) + { + return; + } + + Persist(); + _settingsDirty = false; + _lastSettingsSave = Stopwatch.GetTimestamp(); + } + private void PublishSnapshot() { var school = _school; @@ -335,17 +398,25 @@ internal sealed class SchoolWorker return; } - _store.Save(new SchoolSave + // A full disk or a locked file must not end the school; the next save will try again. + try { - Format = SchoolStore.CurrentFormat, - Id = school.Id, - Name = school.Name, - GameTime = school.Clock.Time, - Running = school.Clock.IsRunning, - SpeedIndex = school.Clock.SpeedIndex, - ModIds = school.Catalog?.PackIds, - Map = school.Map, - }); + _store.Save(new SchoolSave + { + Format = SchoolStore.CurrentFormat, + Id = school.Id, + Name = school.Name, + GameTime = school.Clock.Time, + Running = school.Clock.IsRunning, + SpeedIndex = school.Clock.SpeedIndex, + ModIds = school.Catalog?.PackIds, + Map = school.Map, + }); + } + catch (Exception ex) + { + _logger.LogError(ex, "Could not save school {SchoolId}; it keeps running unsaved.", _id); + } } private void BroadcastClock() @@ -387,8 +458,11 @@ internal sealed class SchoolWorker node.Positions); } - var frame = new byte[ProtocolConstants.MaxMessageSize]; - var length = ProtocolCodec.WriteMapSnapshot(frame, new ServerMapSnapshotMessage(school.Id, nodes)); + // Sized from the message, not from the inbound frame limit: a map the player enlarged in + // the create editor outgrows 8 KiB somewhere past sixty furnished rooms. + var message = new ServerMapSnapshotMessage(school.Id, nodes); + var frame = new byte[ProtocolCodec.MapSnapshotSize(message)]; + var length = ProtocolCodec.WriteMapSnapshot(frame, message); client.TrySendReliable(frame.AsMemory(0, length)); } diff --git a/src/HSchool.Server/Net/GameClient.cs b/src/HSchool.Server/Net/GameClient.cs index a55da36..a15b3bf 100644 --- a/src/HSchool.Server/Net/GameClient.cs +++ b/src/HSchool.Server/Net/GameClient.cs @@ -80,6 +80,13 @@ internal sealed class GameClient(uint playerId, WebSocket socket) var reliable = _reliable.Reader; var outbox = _outbox.Reader; + // The two waits live across iterations on purpose. Creating a fresh pair every idle pass + // left the loser of Task.WhenAny queued on its channel forever — and because clock frames + // arrive twenty times a second, the reliable channel collected one dead waiter per tick, + // each holding a cancellation registration, for as long as the player stayed in a school. + Task? waitReliable = null; + Task? waitOutbox = null; + while (Socket.State == WebSocketState.Open && !cancellationToken.IsCancellationRequested) { if (reliable.TryRead(out var reliableFrame)) @@ -102,44 +109,44 @@ internal sealed class GameClient(uint playerId, WebSocket socket) continue; } - var waitReliable = reliable.WaitToReadAsync(cancellationToken).AsTask(); - var waitOutbox = outbox.WaitToReadAsync(cancellationToken).AsTask(); + // A completed channel is empty for good; waiting on it again would spin. + if (!reliable.Completion.IsCompleted) + { + waitReliable ??= reliable.WaitToReadAsync(cancellationToken).AsTask(); + } + + if (!outbox.Completion.IsCompleted) + { + waitOutbox ??= outbox.WaitToReadAsync(cancellationToken).AsTask(); + } + + if (waitReliable is null && waitOutbox is null) + { + return; + } + Task finished; try { - finished = await Task.WhenAny(waitReliable, waitOutbox).ConfigureAwait(false); + finished = waitReliable is null ? waitOutbox! + : waitOutbox is null ? waitReliable + : await Task.WhenAny(waitReliable, waitOutbox).ConfigureAwait(false); + + await finished.ConfigureAwait(false); } catch (OperationCanceledException) { return; } - bool hasData; - try + // Only the wait that finished is dropped; the other one stays queued on its channel. + if (ReferenceEquals(finished, waitReliable)) { - hasData = await finished.ConfigureAwait(false); + waitReliable = null; } - catch (OperationCanceledException) + else { - return; - } - - if (hasData) - { - continue; - } - - var other = ReferenceEquals(finished, waitReliable) ? waitOutbox : waitReliable; - try - { - if (!await other.ConfigureAwait(false)) - { - return; - } - } - catch (OperationCanceledException) - { - return; + waitOutbox = null; } } } diff --git a/src/HSchool.Server/Net/GameSocketHandler.cs b/src/HSchool.Server/Net/GameSocketHandler.cs index 3913a07..c933a4e 100644 --- a/src/HSchool.Server/Net/GameSocketHandler.cs +++ b/src/HSchool.Server/Net/GameSocketHandler.cs @@ -95,6 +95,10 @@ internal sealed class GameSocketHandler( commands.Enqueue(new GameCommand.CloseSchool(client.PlayerId, watchedSchoolId)); } + // Cancel first, then wait. The other order hung here whenever the send loop sat inside + // SendAsync on a half-open connection: nothing was left to cancel it. + await connectionCts.CancelAsync().ConfigureAwait(false); + if (sendLoop is not null) { try @@ -106,8 +110,6 @@ internal sealed class GameSocketHandler( // Expected while tearing the connection down. } } - - await connectionCts.CancelAsync().ConfigureAwait(false); } } diff --git a/src/HSchool.Simulation/SimulationOptions.cs b/src/HSchool.Simulation/SimulationOptions.cs index 9de3394..afb1b48 100644 --- a/src/HSchool.Simulation/SimulationOptions.cs +++ b/src/HSchool.Simulation/SimulationOptions.cs @@ -38,15 +38,23 @@ public sealed class SimulationOptions public string ModsDirectory { get; set; } = "mods"; /// - /// How often a running school writes its clock to disk. Create, pause, speed and shutdown - /// write immediately; the tick itself never does. + /// How often a running school writes its clock to disk. Create and shutdown write + /// immediately; the tick itself never does. /// public int SaveIntervalSeconds { get; set; } = 30; + /// + /// Shortest gap between two saves caused by pause or speed. A client can send those as fast + /// as the socket allows, and each one used to be a file write on the school's own thread. + /// + public int MinSaveIntervalMilliseconds { get; set; } = 1000; + /// Length of one fixed step. public double FixedDeltaTime => 1d / TickRate; public TimeSpan TickInterval => TimeSpan.FromSeconds(1d / TickRate); public TimeSpan SaveInterval => TimeSpan.FromSeconds(SaveIntervalSeconds); + + public TimeSpan MinSaveInterval => TimeSpan.FromMilliseconds(MinSaveIntervalMilliseconds); } diff --git a/tests/HSchool.Protocol.Tests/ProtocolCodecTests.cs b/tests/HSchool.Protocol.Tests/ProtocolCodecTests.cs index 1170962..f435b1f 100644 --- a/tests/HSchool.Protocol.Tests/ProtocolCodecTests.cs +++ b/tests/HSchool.Protocol.Tests/ProtocolCodecTests.cs @@ -158,6 +158,52 @@ public class ProtocolCodecTests Assert.Equal(["Директор"], read.Nodes[1].Positions); } + [Fact] + public void MapSnapshotSize_IsExactlyWhatTheWriterProduces() + { + var message = new ServerMapSnapshotMessage(7, [ + new MapSnapshotNode(0, "yard", "", "Двор", [], []), + new MapSnapshotNode(2, "floor-1", "main", "Этаж 1", [], []), + new MapSnapshotNode(3, "office", "floor-1", "Кабинет директора", ["Стол", "Стул"], ["Директор"]), + ]); + + var size = ProtocolCodec.MapSnapshotSize(message); + var buffer = new byte[size]; + + Assert.Equal(size, ProtocolCodec.WriteMapSnapshot(buffer, message)); + } + + [Fact] + public void MapSnapshot_OfALargeSchoolSurvivesTheEightKilobyteLimit() + { + // A player who keeps clicking "Add room" in the create editor gets past 8 KiB somewhere + // around sixty furnished rooms. Sizing the buffer from the message is what keeps that + // school openable instead of killing its worker thread on the first snapshot. + var nodes = new List { new(0, "yard", "", "Двор", [], []) }; + for (var i = 1; i <= 200; i++) + { + nodes.Add(new MapSnapshotNode( + 3, + $"principals-office-{i}", + "floor-1", + "Кабинет директора", + ["Кресло директора", "Стол", "Стул"], + ["Директор"])); + } + + var message = new ServerMapSnapshotMessage(1, nodes); + var buffer = new byte[ProtocolCodec.MapSnapshotSize(message)]; + + Assert.True(buffer.Length > ProtocolConstants.MaxMessageSize); + + var length = ProtocolCodec.WriteMapSnapshot(buffer, message); + var read = ProtocolCodec.ReadMapSnapshot(buffer.AsSpan(0, length)); + + Assert.Equal(nodes.Count, read.Nodes.Count); + Assert.Equal("principals-office-200", read.Nodes[^1].Id); + Assert.Equal(["Кресло директора", "Стол", "Стул"], read.Nodes[^1].Items); + } + [Fact] public void Numbers_AreLittleEndian() {