diff --git a/c-sharp-tests/DummyPapiClient.cs b/c-sharp-tests/DummyPapiClient.cs index ba47d602cca..2a51e98298c 100644 --- a/c-sharp-tests/DummyPapiClient.cs +++ b/c-sharp-tests/DummyPapiClient.cs @@ -15,6 +15,11 @@ internal class DummyPapiClient : PapiClient private readonly ConcurrentQueue<(string eventType, object? eventParameters)> _sentEvents = new(); + private readonly Queue<( + string requestType, + IReadOnlyList? requestContents + )> _sentRequests = []; + // ConcurrentDictionary (not Dictionary): RegisterRequestHandlerAsync writes this on every // PDP registration, and per-project PDP creation registers PDPs concurrently (fire- // and-forget, not serialized behind a single creation lock - see @@ -25,6 +30,16 @@ private readonly ConcurrentDictionary< OpenRpcSingleMethodDocumentation? > _documentationByRequestType = new(); + /// + /// Test-only override for how a network:registerEvent request resolves. Defaults to + /// "main accepted" (returns true) — the normal happy path that services under test + /// rely on so they don't take their registration-failure branches. A test can swap in a + /// delegate that returns false (registry rejection) or throws (registry failure) to + /// exercise those best-effort branches. Scoped strictly to network:registerEvent so + /// no other test's expectations change. + /// + public Func RegisterEventResponse { get; set; } = () => true; + #region Overrides of PapiClient public override Task ConnectAsync() @@ -106,9 +121,37 @@ public int SentEventCount { if (_localMethods.TryGetValue(requestType, out _)) return base.SendRequestAsync(requestType, requestContents); + _sentRequests.Enqueue((requestType, requestContents)); + // Central-registry event registration returns an acceptance boolean; defer to the + // configurable RegisterEventResponse (defaults to "main accepted", mirroring + // ConnectAsync's "pretend we succeeded") so a test can drive the registration-failure + // paths. Kept strictly to this one request type so no other test's SendRequestAsync + // expectations change. + if (requestType == "network:registerEvent") + // T is bool here; there is no non-casting way to satisfy the generic return type. + return Task.FromResult((T)(object)RegisterEventResponse()); return Task.FromResult(default); } + /// + /// Test-only count of requests sent through that were NOT + /// handled by a locally-registered handler (i.e. would have gone over the wire to main). + /// + public int SentRequestCount + { + get { return _sentRequests.Count; } + } + + /// + /// Test-only dequeue of the oldest captured outgoing request (see + /// ). Lets tests assert wire calls like + /// network:registerEvent without a live PAPI connection. + /// + public (string requestType, IReadOnlyList? requestContents) NextSentRequest + { + get { return _sentRequests.Dequeue(); } + } + /// /// Test-only accessor that reports whether a handler is registered in /// _localMethods for the given wire name. Exposes the protected @@ -119,6 +162,20 @@ public int SentEventCount public bool IsHandlerRegistered(string requestType) => _localMethods.ContainsKey(requestType); + /// + /// Test-only helper that invokes a locally-registered request handler directly (bypassing the + /// websocket the base client would use) and returns its result. Lets a test assert what a + /// registered command handler returns without a live PAPI connection. + /// + public object? InvokeRequestHandler(string requestType, params object?[] args) + { + if (!_localMethods.TryGetValue(requestType, out var handler)) + throw new InvalidOperationException( + $"No handler registered for request type \"{requestType}\"" + ); + return handler.DynamicInvoke(args); + } + #endregion } } diff --git a/c-sharp-tests/Projects/SendReceive/SendReceiveBlockNotifierServiceTests.cs b/c-sharp-tests/Projects/SendReceive/SendReceiveBlockNotifierServiceTests.cs new file mode 100644 index 00000000000..b18ef6db6c9 --- /dev/null +++ b/c-sharp-tests/Projects/SendReceive/SendReceiveBlockNotifierServiceTests.cs @@ -0,0 +1,266 @@ +using System.Diagnostics.CodeAnalysis; +using System.Text.Json; +using Paranext.DataProvider.JsonUtils; +using Paranext.DataProvider.Projects.SendReceive; + +namespace TestParanextDataProvider.Projects.SendReceive +{ + /// + /// Unit tests for : it forwards + /// transitions to the PAPI as the + /// paratextBibleSendReceive.onSyncWriteLockChanged event and answers the + /// command:paratextBibleSendReceive.getAutoSyncBlocking pull command with the current + /// snapshot. Uses (the base fixture's Client), which captures + /// sent events and locally-registered handlers, so no live PAPI connection is needed. Every test + /// resets the static gate before and after so a subscription can never leak between tests. + /// + [TestFixture] + [ExcludeFromCodeCoverage] + internal class SendReceiveBlockNotifierServiceTests : PapiTestBase + { + private const string BlockStateChangedEvent = + "paratextBibleSendReceive.onSyncWriteLockChanged"; + private const string GetAutoSyncBlockingCommand = + "command:paratextBibleSendReceive.getAutoSyncBlocking"; + + private SendReceiveBlockNotifierService _service = null!; + + [SetUp] + public override async Task TestSetupAsync() + { + await base.TestSetupAsync(); + SendReceiveWriteLock.ResetForTests(); + _service = new SendReceiveBlockNotifierService(Client); + await _service.InitializeAsync(); + } + + [TearDown] + public void ResetGate() => SendReceiveWriteLock.ResetForTests(); + + [Test] + public void InitializeAsync_RegistersTheGetAutoSyncBlockingCommand() + { + Assert.That( + Client.IsHandlerRegistered(GetAutoSyncBlockingCommand), + Is.True, + "InitializeAsync must register the getAutoSyncBlocking command handler" + ); + } + + [Test] + public void InitializeAsync_RegistersTheEventWithTheCentralRegistry() + { + Assert.That( + Client.SentRequestCount, + Is.EqualTo(1), + "InitializeAsync must send exactly one wire request: the event registration" + ); + var (requestType, requestContents) = Client.NextSentRequest; + Assert.Multiple(() => + { + Assert.That(requestType, Is.EqualTo("network:registerEvent")); + Assert.That( + requestContents, + Is.EqualTo(new object?[] { BlockStateChangedEvent }), + "the registration must name the onSyncWriteLockChanged event" + ); + }); + } + + [Test] + public async Task InitializeAsync_RegistryRejectsEventRegistration_LogsAndStillEmits() + { + // Best-effort contract: if main's central registry rejects the event registration + // (returns false), InitializeAsync must warn and continue — the service still registers + // its command handler and still pushes gate transitions. Build a fresh gate + client so + // only the service under test here is subscribed (the base SetUp already initialized on + // the happy path). + SendReceiveWriteLock.ResetForTests(); + using var client = new DummyPapiClient { RegisterEventResponse = () => false }; + var service = new SendReceiveBlockNotifierService(client); + + using var consoleError = new StringWriter(); + var originalError = Console.Error; + Console.SetError(consoleError); + try + { + await service.InitializeAsync(); + } + finally + { + Console.SetError(originalError); + } + + Assert.Multiple(() => + { + Assert.That( + consoleError.ToString(), + Does.Contain(BlockStateChangedEvent), + "a rejected registration must be logged, naming the event" + ); + Assert.That( + client.IsHandlerRegistered(GetAutoSyncBlockingCommand), + Is.True, + "a rejected registration must not stop the command handler from registering" + ); + }); + + // The service still forwards a gate transition despite the rejected registration. + SendReceiveWriteLock.SetSyncing(["projectA"]); + Assert.That( + client.SentEventCount, + Is.EqualTo(1), + "the service must still push the block-state event after a rejected registration" + ); + var (eventType, _) = client.NextSentEvent; + Assert.That(eventType, Is.EqualTo(BlockStateChangedEvent)); + } + + [Test] + public async Task InitializeAsync_EventRegistrationThrows_IsCaughtAndStillEmits() + { + // Best-effort contract: if the registration request itself fails (throws), InitializeAsync + // must catch it, log it, and continue — a registry hiccup must never break backend + // startup. Fresh gate + client, as above. + SendReceiveWriteLock.ResetForTests(); + using var client = new DummyPapiClient + { + RegisterEventResponse = () => + throw new InvalidOperationException("registry offline"), + }; + var service = new SendReceiveBlockNotifierService(client); + + using var consoleError = new StringWriter(); + var originalError = Console.Error; + Console.SetError(consoleError); + try + { + // Must not throw: the registration failure is caught internally. + await service.InitializeAsync(); + } + finally + { + Console.SetError(originalError); + } + + Assert.Multiple(() => + { + var log = consoleError.ToString(); + Assert.That( + log, + Does.Contain(BlockStateChangedEvent), + "a thrown registration must be caught and logged, naming the event" + ); + Assert.That( + log, + Does.Contain("registry offline"), + "the caught exception detail must be logged" + ); + Assert.That( + client.IsHandlerRegistered(GetAutoSyncBlockingCommand), + Is.True, + "a thrown registration must not stop the command handler from registering" + ); + }); + + // The service still forwards a gate transition despite the thrown registration. + SendReceiveWriteLock.SetSyncing(["projectA"]); + Assert.That( + client.SentEventCount, + Is.EqualTo(1), + "the service must still push the block-state event after a thrown registration" + ); + var (eventType, _) = client.NextSentEvent; + Assert.That(eventType, Is.EqualTo(BlockStateChangedEvent)); + } + + [Test] + public void GateArm_PushesOnSyncWriteLockChangedEventWithBlockingSnapshot() + { + Assert.That(Client.SentEventCount, Is.Zero, "no events before any transition"); + + SendReceiveWriteLock.SetSyncing(["projectA", "projectB"]); + + Assert.That(Client.SentEventCount, Is.EqualTo(1), "an arm must push exactly one event"); + var (eventType, payload) = Client.NextSentEvent; + Assert.That(eventType, Is.EqualTo(BlockStateChangedEvent)); + Assert.That(payload, Is.InstanceOf()); + var state = (SendReceiveBlockState)payload!; + Assert.Multiple(() => + { + Assert.That(state.IsBlocking, Is.True); + Assert.That(state.ProjectIds, Is.EquivalentTo(new[] { "projectA", "projectB" })); + }); + } + + [Test] + public void GateClear_PushesOnSyncWriteLockChangedEventWithNotBlockingSnapshot() + { + SendReceiveWriteLock.SetSyncing(["projectA"]); + Assert.That(Client.SentEventCount, Is.EqualTo(1), "the arm event"); + _ = Client.NextSentEvent; // discard the arm event + + SendReceiveWriteLock.Clear(); + + Assert.That(Client.SentEventCount, Is.EqualTo(1), "the disarm pushes one more event"); + var (eventType, payload) = Client.NextSentEvent; + Assert.That(eventType, Is.EqualTo(BlockStateChangedEvent)); + var state = (SendReceiveBlockState)payload!; + Assert.Multiple(() => + { + Assert.That(state.IsBlocking, Is.False); + Assert.That(state.ProjectIds, Is.Empty); + }); + } + + [Test] + public void GetAutoSyncBlockingCommand_ReturnsTheCurrentSnapshot() + { + // Not blocking before any sync. + var before = Client.InvokeRequestHandler(GetAutoSyncBlockingCommand); + Assert.That(before, Is.InstanceOf()); + Assert.That(((SendReceiveBlockState)before!).IsBlocking, Is.False); + + SendReceiveWriteLock.SetSyncing(["projectA"]); + + // Blocking, with the armed ids, after arming. + var after = Client.InvokeRequestHandler(GetAutoSyncBlockingCommand); + var state = (SendReceiveBlockState)after!; + Assert.Multiple(() => + { + Assert.That(state.IsBlocking, Is.True); + Assert.That(state.ProjectIds, Is.EquivalentTo(new[] { "projectA" })); + }); + } + + [Test] + public void SendReceiveBlockState_SerializesToTheCamelCaseWireShape() + { + // The renderer consumes exactly { isBlocking, projectIds }. Serialize with the same + // options the PAPI JSON-RPC formatter uses (PropertyNamingPolicy = CamelCase) to pin that + // contract at the C# boundary. + var options = SerializationOptions.CreateSerializationOptions(); + + var json = JsonSerializer.Serialize( + new SendReceiveBlockState(true, new[] { "projectA" }), + options + ); + + Assert.Multiple(() => + { + Assert.That(json, Does.Contain("\"isBlocking\":true")); + Assert.That(json, Does.Contain("\"projectIds\":[\"projectA\"]")); + Assert.That( + json, + Does.Not.Contain("IsBlocking"), + "keys must be camelCase on the wire" + ); + Assert.That( + json, + Does.Not.Contain("ProjectIds"), + "keys must be camelCase on the wire" + ); + }); + } + } +} diff --git a/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs b/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs index 235c6e8e1be..c84f84f1a7a 100644 --- a/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs +++ b/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs @@ -160,28 +160,98 @@ public void EnterWrite_Armed_ThrowsWithSentinel() } [Test] - public void EnterWrite_WhileAnotherProjectSyncs_RejectsAllProjects_GlobalGate() + public void EnterWrite_WhileAnotherProjectSyncs_AllowsTheUnsyncedProject_PerProjectGate() { - // The write gate is GLOBAL (a single process-wide gate): while a sync is armed, writes - // to EVERY project fail fast, not just the syncing one. This is the intended coarse - // exclusion (a sync is globally exclusive). IsBlocked stays per-project pure data, so it - // is deliberately narrower than the gate. + // The write gate is PER-PROJECT: while projectA is syncing, a write to projectB (which + // is NOT in the armed set) must proceed normally — editing an unrelated project stays + // possible while another syncs (PT9 parity). IsBlocked agrees: projectB is not in the + // armed set. SendReceiveWriteLock.SetSyncing(["projectA"]); + Assert.That( + SendReceiveWriteLock.IsBlocked("projectB"), + Is.False, + "projectB is not in the armed set — IsBlocked must be false" + ); + + IDisposable scope = null!; + Assert.DoesNotThrow( + () => scope = SendReceiveWriteLock.EnterWrite("projectB"), + "a write to the unsynced project must be allowed while another project syncs" + ); + Assert.That( + SendReceiveWriteLock.InFlightWriteCount, + Is.EqualTo(1), + "the allowed write must count as in-flight" + ); + + scope.Dispose(); + Assert.That(SendReceiveWriteLock.InFlightWriteCount, Is.Zero, "dispose releases it"); + } + + [Test] + public void EnterWrite_WhileProjectSyncs_RejectsThatProject_PerProjectGate() + { + // The other half of the per-project gate: a write to a project that IS in the armed set + // is rejected with the sentinel, exactly as before. + SendReceiveWriteLock.SetSyncing(["projectA", "projectB"]); + Assert.Multiple(() => { - Assert.That( - SendReceiveWriteLock.IsBlocked("projectB"), - Is.False, - "IsBlocked is per-project pure data — projectB is not in the armed set" + var exA = Assert.Throws( + () => SendReceiveWriteLock.EnterWrite("projectA") ); - var ex = Assert.Throws( + Assert.That(exA!.Message, Does.EndWith(SendReceiveWriteLock.EditBlockedSentinel)); + var exB = Assert.Throws( () => SendReceiveWriteLock.EnterWrite("projectB") ); - Assert.That(ex!.Message, Does.EndWith(SendReceiveWriteLock.EditBlockedSentinel)); + Assert.That(exB!.Message, Does.EndWith(SendReceiveWriteLock.EditBlockedSentinel)); }); } + [Test] + public void EnterWrite_AllowedUnsyncedWrite_ParticipatesInALaterDrain() + { + // An unsynced-project write that EnterWrite allowed while armed is a genuine in-flight + // write: the (global) drain of a SUBSEQUENT sync must wait for it. Arm projectA, open an + // allowed projectB write, then start a projectB sync with a shortened DrainTimeout and + // prove the drain actually waited it out (degraded), rather than skipping the drain — its + // count is what the drain waits on. + var previousTimeout = SendReceiveWriteLock.DrainTimeout; + SendReceiveWriteLock.DrainTimeout = TimeSpan.FromMilliseconds(300); + IDisposable? projectBWrite = null; + try + { + SendReceiveWriteLock.SetSyncing(["projectA"]); + projectBWrite = SendReceiveWriteLock.EnterWrite("projectB"); // allowed (out of set) + Assert.That(SendReceiveWriteLock.InFlightWriteCount, Is.EqualTo(1)); + + var stopwatch = Stopwatch.StartNew(); + SendReceiveWriteLock.SetSyncing(["projectB"]); // takeover; global drain waits on the scope + stopwatch.Stop(); + + Assert.Multiple(() => + { + Assert.That( + stopwatch.Elapsed, + Is.GreaterThanOrEqualTo(TimeSpan.FromMilliseconds(250)), + "the global drain must wait for the allowed unsynced write, not skip it" + ); + Assert.That( + SendReceiveWriteLock.InFlightWriteCount, + Is.EqualTo(1), + "the still-open allowed write is what the drain waited on" + ); + }); + } + finally + { + projectBWrite?.Dispose(); + SendReceiveWriteLock.DrainTimeout = previousTimeout; + SendReceiveWriteLock.Clear(); + } + } + // ---- Forgiving lifecycle contract (no thread affinity, no recursion policy) ---- [Test] @@ -200,13 +270,15 @@ public void EnterWrite_NestedOnSameThread_BothScopesSucceed() } [Test] - public void EnterWrite_NestedWhileSyncArmed_InnerThrowsSentinel() + public void EnterWrite_NestedWhileSameProjectSyncArmed_InnerThrowsSentinel() { // Pins the nesting hazard the docs warn about: the gate tracks no ownership, so once a - // sync arms — here on the degraded path, since the outer scope on this thread can - // never drain — a nested EnterWrite inside an open outer scope is rejected - // MID-mutation. This is why delegating methods must call an un-gated core inside one - // scope (see SetBookUsfmInScope) instead of nesting gated entry points. + // sync of the SAME project arms — here on the degraded path, since the outer scope on + // this thread can never drain — a nested EnterWrite for that project inside an open + // outer scope is rejected MID-mutation. A method mutates one project, so its outer and + // inner scopes share that project and the per-project filter does not save it. This is + // why delegating methods must call an un-gated core inside one scope (see + // SetBookUsfmInScope) instead of nesting gated entry points. var previousTimeout = SendReceiveWriteLock.DrainTimeout; SendReceiveWriteLock.DrainTimeout = TimeSpan.FromMilliseconds(50); try @@ -215,7 +287,7 @@ public void EnterWrite_NestedWhileSyncArmed_InnerThrowsSentinel() SendReceiveWriteLock.SetSyncing(["projectA"]); // degrades: outer cannot drain var ex = Assert.Throws( - () => SendReceiveWriteLock.EnterWrite("projectB") + () => SendReceiveWriteLock.EnterWrite("projectA") ); Assert.That(ex!.Message, Does.EndWith(SendReceiveWriteLock.EditBlockedSentinel)); } @@ -311,6 +383,43 @@ public void SetSyncing_NullOrEmptyIdsInBatch_AreIgnored() SendReceiveWriteLock.Clear(); } + [Test] + public void SetSyncing_AllInvalidBatch_ArmsButBlocksNoProject() + { + // Pins the ACTUAL per-project behavior of an all-invalid batch: it still arms (and the + // drain still runs, and the token is still valid — none of that machinery cares whether + // the blocked set is empty), but the per-project filter (armed && set.Contains(id)) can + // never match an empty set, so this arm rejects NOTHING for ANY project. + long token = 0; + Assert.DoesNotThrow( + () => token = SendReceiveWriteLock.SetSyncing([null!, ""]), + "an all-invalid batch must not crash the arming thread" + ); + + Assert.Multiple(() => + { + Assert.That( + SendReceiveWriteLock.IsArmed, + Is.True, + "the gate still arms even though the batch had no valid ids" + ); + Assert.That( + SendReceiveWriteLock.ArmedProjectIds, + Is.Empty, + "no valid id means the armed set is empty" + ); + + IDisposable scope = null!; + Assert.DoesNotThrow( + () => scope = SendReceiveWriteLock.EnterWrite("anyProject"), + "an armed gate with an EMPTY blocked set must reject NO project's writes" + ); + scope.Dispose(); + }); + + SendReceiveWriteLock.Clear(token); + } + // ---- SetSyncing drains in-flight writes ---- [Test] @@ -713,6 +822,180 @@ public void SetSyncing_ThenClear_RoundTrips() }); } + // ---- BlockStateChanged event + GetBlockState snapshot ---- + + [Test] + public void SetSyncing_Arm_RaisesBlockStateChangedOnceWithArmedIds() + { + var events = new List(); + SendReceiveWriteLock.BlockStateChanged += events.Add; + + SendReceiveWriteLock.SetSyncing(["projectA", "projectB"]); + + Assert.That(events, Has.Count.EqualTo(1), "a clean arm must raise exactly once"); + Assert.Multiple(() => + { + Assert.That(events[0].IsBlocking, Is.True); + Assert.That( + events[0].ProjectIds, + Is.EquivalentTo(new[] { "projectA", "projectB" }), + "the event must carry the exact armed ids" + ); + }); + } + + [Test] + public void SetSyncing_Rearm_RaisesBlockStateChangedWithTheNewIds() + { + SendReceiveWriteLock.SetSyncing(["projectA"]); + var events = new List(); + SendReceiveWriteLock.BlockStateChanged += events.Add; // subscribe after the first arm + + SendReceiveWriteLock.SetSyncing(["projectB"]); // takeover + + Assert.That(events, Has.Count.EqualTo(1), "a takeover re-arm must raise"); + Assert.Multiple(() => + { + Assert.That(events[0].IsBlocking, Is.True); + Assert.That(events[0].ProjectIds, Is.EquivalentTo(new[] { "projectB" })); + }); + } + + [Test] + public void ClearWithToken_Disarm_RaisesBlockStateChangedEmpty() + { + var token = SendReceiveWriteLock.SetSyncing(["projectA"]); + var events = new List(); + SendReceiveWriteLock.BlockStateChanged += events.Add; // subscribe after arm + + SendReceiveWriteLock.Clear(token); + + Assert.That(events, Has.Count.EqualTo(1), "a real disarm must raise exactly once"); + Assert.Multiple(() => + { + Assert.That(events[0].IsBlocking, Is.False); + Assert.That(events[0].ProjectIds, Is.Empty, "a disarm event carries an empty set"); + }); + } + + [Test] + public void ClearWithToken_StaleToken_DoesNotRaise() + { + var tokenA = SendReceiveWriteLock.SetSyncing(["projectA"]); + SendReceiveWriteLock.SetSyncing(["projectB"]); // takeover -> tokenA is now stale + var events = new List(); + SendReceiveWriteLock.BlockStateChanged += events.Add; + + SendReceiveWriteLock.Clear(tokenA); // stale no-op — no transition + + Assert.That( + events, + Is.Empty, + "a stale-token Clear is a no-op and must not raise a spurious event" + ); + } + + [Test] + public void Clear_Disarm_RaisesBlockStateChangedEmpty() + { + SendReceiveWriteLock.SetSyncing(["projectA"]); + var events = new List(); + SendReceiveWriteLock.BlockStateChanged += events.Add; // subscribe after arm + + SendReceiveWriteLock.Clear(); + + Assert.That(events, Has.Count.EqualTo(1)); + Assert.Multiple(() => + { + Assert.That(events[0].IsBlocking, Is.False); + Assert.That(events[0].ProjectIds, Is.Empty); + }); + } + + [Test] + public void Clear_WhenNothingArmed_DoesNotRaise() + { + // A Clear() that disarms nothing is a no-op — it must not fire a spurious "changed" + // signal (consistent with the stale-token Clear(long) no-op). Only real transitions + // raise. + var events = new List(); + SendReceiveWriteLock.BlockStateChanged += events.Add; + + SendReceiveWriteLock.Clear(); + + Assert.That(events, Is.Empty, "a Clear() that disarms nothing must not raise"); + } + + [Test] + public void BlockStateChanged_ThrowingSubscriber_DoesNotBreakArmOrClear() + { + // A subscriber that throws must never corrupt the gate or abort an arm/clear — the gate + // swallows the fault (it has no logger; the notifier service owns real error handling). + SendReceiveWriteLock.BlockStateChanged += _ => + throw new InvalidOperationException("subscriber boom"); + + long token = 0; + Assert.DoesNotThrow( + () => token = SendReceiveWriteLock.SetSyncing(["projectA"]), + "arm must succeed despite a throwing subscriber" + ); + Assert.That(SendReceiveWriteLock.IsArmed, Is.True, "the arm transition still happened"); + + Assert.DoesNotThrow( + () => SendReceiveWriteLock.Clear(token), + "clear must succeed despite a throwing subscriber" + ); + Assert.That( + SendReceiveWriteLock.IsArmed, + Is.False, + "the disarm transition still happened" + ); + } + + [Test] + public void GetBlockState_WhenNotArmed_ReportsNotBlockingAndEmpty() + { + var state = SendReceiveWriteLock.GetBlockState(); + + Assert.Multiple(() => + { + Assert.That(state.IsBlocking, Is.False); + Assert.That(state.ProjectIds, Is.Empty); + }); + } + + [Test] + public void GetBlockState_WhenArmed_ReportsBlockingAndTheArmedIds() + { + SendReceiveWriteLock.SetSyncing(["projectA", "projectB"]); + + var state = SendReceiveWriteLock.GetBlockState(); + + Assert.Multiple(() => + { + Assert.That(state.IsBlocking, Is.True); + Assert.That(state.ProjectIds, Is.EquivalentTo(new[] { "projectA", "projectB" })); + }); + } + + [Test] + public void ResetForTests_ClearsBlockStateChangedSubscribers() + { + // The event is static; ResetForTests must drop subscribers so a subscription can never + // leak into a later test (and be invoked by ITS transitions). + var events = new List(); + SendReceiveWriteLock.BlockStateChanged += events.Add; + + SendReceiveWriteLock.ResetForTests(); + + SendReceiveWriteLock.SetSyncing(["projectA"]); + Assert.That( + events, + Is.Empty, + "ResetForTests must unsubscribe BlockStateChanged handlers" + ); + } + // ---- Concurrency stress: the core safety invariant ---- /// diff --git a/c-sharp/Program.cs b/c-sharp/Program.cs index c4a6492d68b..1dd2503f897 100644 --- a/c-sharp/Program.cs +++ b/c-sharp/Program.cs @@ -134,6 +134,10 @@ public static async Task Main() "Background service registration" ); + // Bridge the S/R write gate to the PAPI (event + getAutoSyncBlocking). Initialized in the + // critical barrier below so it is serving before extension activation. + var sendReceiveBlockNotifierService = new SendReceiveBlockNotifierService(papi); + StartupTiming.Mark("init-barrier-start"); // Critical path: everything the renderer needs to list projects and open an editor. await Task.WhenAll( @@ -142,7 +146,8 @@ await Task.WhenAll( versificationConversionService.InitializeAsync(), paratextRegistrationService.InitializeAsync(), paratextSendReceiveService.InitializeAsync(), - dblResources.RegisterDataProviderAsync() + dblResources.RegisterDataProviderAsync(), + sendReceiveBlockNotifierService.InitializeAsync() ); StartupTiming.Mark("init-barrier-end"); diff --git a/c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs b/c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs new file mode 100644 index 00000000000..d99e66041af --- /dev/null +++ b/c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs @@ -0,0 +1,137 @@ +namespace Paranext.DataProvider.Projects.SendReceive; + +/// +/// Bridges the process-wide gate to the PAPI so the renderer has a +/// backend-authoritative view of whether an automatic Send/Receive is currently blocking edits, and +/// for which projects. The renderer never has to infer the block state from indirect cues — it reads +/// it here. +/// +/// Two surfaces, both carrying the same wire shape { isBlocking, projectIds } (a +/// serialized via the shared camelCase PAPI JSON options): +/// +/// +/// An paratextBibleSendReceive.onSyncWriteLockChanged event, pushed whenever the gate's block +/// state changes (this service subscribes to ). +/// +/// +/// A command:paratextBibleSendReceive.getAutoSyncBlocking request handler returning the +/// current snapshot, so a renderer can seed its state on demand (e.g. when a WebView mounts) without +/// waiting for the next transition. +/// +/// +/// +/// +/// Inert in open-source Platform.Bible. Nothing arms in +/// public core, so never fires and the command +/// always returns a not-blocking snapshot. The service is truthful either way — it just has nothing +/// to report until the Paratext 10 Studio patch brackets a sync with the gate (PT-4210). +/// +/// +/// Central-registry registration. Unlike the other C#-origin events (the PDP +/// <id>-pdp-data:onDidUpdate family, SharedStore's shared-store:change), +/// which still announce unregistered and draw main's once-per-name deprecation warning pending a +/// platform-wide migration, this event IS formally registered with main's central event registry: +/// sends a network:registerEvent request — the same +/// main-process JSON-RPC method TypeScript's createNetworkEventEmitterAsync registration +/// path calls — making this connection the event's single registered source. +/// has no dedicated wrapper for that method, so the request goes out +/// generically via , mirroring how +/// calls network:registerMethod. +/// We subscribe to the gate BEFORE issuing that registration request, not after, so a transition +/// arriving during the registration round-trip cannot be dropped unsubscribed; the accepted cost is +/// that such an early transition announces unregistered (main logs its once-per-name deprecation +/// warning) until the registration completes. Registration is best-effort either way: on rejection +/// or failure we log and still emit, because announcing unregistered remains functional (main just +/// logs the deprecation warning) and a registry hiccup must never break backend startup. +/// +/// +internal class SendReceiveBlockNotifierService(PapiClient papiClient) +{ + /// + /// Wire name of the block-state-changed event. camelCase to match the PAPI event-name convention; + /// the renderer subscribes to this exact string. + /// + private const string BlockStateChangedEvent = "paratextBibleSendReceive.onSyncWriteLockChanged"; + + /// + /// Wire name of the "is an automatic Send/Receive currently blocking edits?" pull command. + /// + private const string GetAutoSyncBlockingCommand = + "command:paratextBibleSendReceive.getAutoSyncBlocking"; + + /// + /// Wire name of the main-process JSON-RPC method that registers a network event with the + /// central event registry (the TypeScript REGISTER_EVENT constant). + /// + private const string RegisterEventMethod = "network:registerEvent"; + + private PapiClient PapiClient { get; } = papiClient; + + public async Task InitializeAsync() + { + // Subscribe to the gate FIRST, before the registration round-trip below, so no transition + // can slip through unsubscribed while the network:registerEvent request is in flight. A + // transition that beats registration just announces unregistered (main logs its once-per-name + // deprecation warning), which is the right trade for the single block-state signal — a missed + // transition would be worse than an early warning. The subscription lives for the process + // lifetime (this service is a startup singleton, like the other PAPI services), so there is + // no unsubscribe — mirrors SharedStore's process-lifetime change-event handler. + SendReceiveWriteLock.BlockStateChanged += OnBlockStateChanged; + + // Register the event with main's central event registry (best-effort — on rejection or + // failure, log and continue; emitting unregistered still works, and startup must never break + // over this). We accept that a transition arriving during this await may announce before the + // registration completes; see the subscribe-first rationale above. + try + { + bool accepted = await PapiClient.SendRequestAsync( + RegisterEventMethod, + [BlockStateChangedEvent] + ); + if (!accepted) + Console.Error.WriteLine( + $"Central registry rejected network event '{BlockStateChangedEvent}' (already " + + "registered by another process?); announcements will warn as unregistered" + ); + } + catch (Exception ex) + { + Console.Error.WriteLine( + $"Failed to register network event '{BlockStateChangedEvent}' with the central " + + $"registry; announcements will warn as unregistered. {ex}" + ); + } + + // Register the pull command so a renderer can read the current snapshot on demand instead of + // waiting for the next transition. GetBlockState returns the snapshot the handler serializes + // straight back to the caller. + await PapiClient.RegisterRequestHandlerAsync( + GetAutoSyncBlockingCommand, + SendReceiveWriteLock.GetBlockState + ); + } + + /// + /// Forwards a gate transition to the renderer as a PAPI event. Fire-and-forget: the gate raises + /// inline on the arming/clearing thread and + /// must never be delayed or thrown into, so we never await the send. + /// + /// is declared async, so the compiler wraps its + /// ENTIRE body — including anything before its first await — in a state machine: no + /// exception from it can ever throw synchronously into this method. A bare try/catch + /// around the discarded call (the shape SharedStore.Set also uses for its own + /// fire-and-forget SendEventAsync) is therefore DEAD here: every failure lands on the + /// discarded , unobserved and unlogged, and the catch never runs. We + /// deliberately diverge from that precedent — which has the same dead spot — and instead + /// capture the task and attach a fault-only continuation via + /// (the same helper other fire-and-forget call sites in this codebase use to observe a task's + /// fault without awaiting it), so a failed send is actually logged. This keeps the same + /// fire-and-forget semantics — nothing here awaits or re-throws into the gate's event raise. + /// + /// + private void OnBlockStateChanged(SendReceiveBlockState state) => + ThreadingUtils.RunTask( + PapiClient.SendEventAsync(BlockStateChangedEvent, state), + $"send {BlockStateChangedEvent} event" + ); +} diff --git a/c-sharp/Projects/SendReceive/SendReceiveBlockState.cs b/c-sharp/Projects/SendReceive/SendReceiveBlockState.cs new file mode 100644 index 00000000000..0c38519f23e --- /dev/null +++ b/c-sharp/Projects/SendReceive/SendReceiveBlockState.cs @@ -0,0 +1,23 @@ +namespace Paranext.DataProvider.Projects.SendReceive; + +/// +/// An immutable snapshot of 's block state, carried by +/// and returned by +/// . +/// +/// Whether an automatic Send/Receive is currently armed (rejecting writes +/// to the listed projects). false whenever the gate is idle — always so in public core. +/// The project ids whose writes are being rejected while +/// is true; empty when not blocking. Serialized to the PAPI as +/// a JSON array under the camelCase key projectIds (see below). +/// +/// Serializes to the exact wire shape the renderer consumes — { isBlocking, projectIds } — +/// via the shared PAPI JSON options (PropertyNamingPolicy = CamelCase, configured on the +/// JSON-RPC formatter in SerializationOptions), so no per-property attributes are needed. The +/// notifier (SendReceiveBlockNotifierService) sends this record straight over the wire as both +/// the onSyncWriteLockChanged event payload and the getAutoSyncBlocking command return. +/// +public readonly record struct SendReceiveBlockState( + bool IsBlocking, + IReadOnlyCollection ProjectIds +); diff --git a/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs b/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs index 9fdd1e1953f..ac8c61e642f 100644 --- a/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs +++ b/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs @@ -14,11 +14,12 @@ namespace Paranext.DataProvider.Projects.SendReceive; /// /// /// -/// An armed sync rejects new writes (fail-fast). While a sync is armed, every -/// throws immediately (message ending in -/// ) rather than queueing — a user's keystroke must never hang -/// behind a background sync. The editor catches the sentinel, shows an "editing paused during -/// Send/Receive" notice, and reverts the un-saved change. +/// An armed sync rejects new writes to the syncing projects (fail-fast). While a set of +/// projects is syncing, every for one of THOSE projects throws immediately +/// (message ending in ) rather than queueing — a user's keystroke +/// must never hang behind a background sync. The editor catches the sentinel, shows an "editing +/// paused during Send/Receive" notice, and reverts the un-saved change. A write to a project that +/// is NOT being synced is unaffected and proceeds (see the per-project remarks below). /// /// /// @@ -37,17 +38,31 @@ namespace Paranext.DataProvider.Projects.SendReceive; /// scopes. Every transition — enter a write, exit a write, arm, disarm — is a single interlocked /// read-modify-write on that one word, so all transitions are totally ordered and each one sees the /// exact state it replaces. The safety invariant falls out directly: a write scope can only open by -/// atomically observing "not armed" while incrementing the count, and arming atomically sets the -/// flag, so after the arming operation NO new write scope can open, and the in-flight count the -/// drain then waits on can only fall. Because a write's mutation happens entirely between its enter -/// and exit operations (both full fences), a drained count also means every finished write's -/// effects are visible to the sync. The word also carries an arm generation — the token -/// returns — so can check-and-disarm in the -/// same single CAS (see the overlap remarks below). No mutual exclusion depends on any other -/// field: _blockedProjectIds is pure data (it only feeds queries, -/// and can disagree with the gate while racing arm/clear calls overlap — potentially for that -/// whole bracket, not just momentarily; rejection never consults it), and there is deliberately -/// no "is the lock held" bookkeeping to fall out of step with reality. +/// atomically incrementing the count on the very same word that arming sets the flag on, so every +/// scope-open is ordered strictly before or strictly after the arm. A scope that opened BEFORE the +/// arm is one the drain then waits for; a scope that opens AFTER the arm is rejected for any +/// project in the synced set (the per-project filter — see below), so no write to a synced project +/// can open once that project is armed. Because a write's mutation happens entirely between its +/// enter and exit operations (both full fences), a drained count also means every finished write's +/// effects are visible to the sync. (One consequence of the per-project filter: the drain is still +/// GLOBAL — it waits for the count to reach zero across ALL projects — but writes to NON-synced +/// projects may keep opening while armed, so a busy unrelated project can push the drain toward +/// its timeout and into the degraded path. That is an accepted cost of letting unrelated edits +/// continue; it never threatens safety, since only synced-project writes are what a synced +/// project's replacement could race, and those are barred.) The word also carries an arm +/// generation — the token returns — so can +/// check-and-disarm in the same single CAS (see the overlap remarks below). The core +/// mutual-exclusion invariant — no write executes inside its scope during a synced project's +/// file-replacement window — depends ONLY on the atomic word: the count++/arm ordering proves it, +/// and there is deliberately no "is the lock held" bookkeeping to fall out of step with reality. +/// _blockedProjectIds is pure data that +/// layers a per-project FILTER on top of that invariant: it selects WHICH projects an armed gate +/// rejects (both and consult it), but it never +/// weakens the invariant — narrowing rejection can only let MORE writes through, and the +/// count++/arm ordering still bars any write to a synced project from racing that project's file +/// replacement. The set is published data-then-flag (see ), so a reader +/// that observes the armed bit always observes the matching set; the only skew is the two benign +/// flag/set windows in the overlap remarks below, both of which resolve to "allow". /// /// /// Forgiving lifecycle contract (deliberate). The arm→clear bracket is activated by code @@ -71,12 +86,14 @@ namespace Paranext.DataProvider.Projects.SendReceive; /// /// /// Nested calls do not crash or deadlock (there is no recursion policy) — -/// while nothing is armed, an inner scope simply counts as one more in-flight write. They are NOT -/// rejection-safe, though: the gate tracks no ownership, so if a sync arms while the outer scope -/// is open (the normal drain window), the inner call throws the sentinel MID-mutation and tears -/// the outer write. Keep one scope per mutation — a method that delegates to another write must -/// call an un-gated core inside its single outer scope (see SetBookUsfmInScope), never a -/// second gated entry point. +/// while the inner project is not armed, an inner scope simply counts as one more in-flight write. +/// They are NOT rejection-safe, though: the gate tracks no ownership, so if a sync of the SAME +/// project arms while the outer scope is open (the normal drain window), the inner call for that +/// project throws the sentinel MID-mutation and tears the outer write. (A method mutates one +/// project, so its outer and inner scopes share that project — the per-project filter does not save +/// it.) Keep one scope per mutation — a method that delegates to another write must call an un-gated +/// core inside its single outer scope (see SetBookUsfmInScope), never a second gated entry +/// point. /// /// /// @@ -105,19 +122,46 @@ namespace Paranext.DataProvider.Projects.SendReceive; /// instead, leaving the retry/defer decision to the scheduler. /// /// -/// One global sync slot; overlap semantics. The gate is global: while ANY sync is armed, ALL -/// project writes are rejected (automatic syncs are globally exclusive by design), which is why -/// rejection does not consult the per-project set. A repeat while armed -/// takes over the slot: it replaces the armed project-id set and returns a NEW token, invalidating -/// every earlier one (call it once per sync batch with the full set, and Clear with the latest -/// token). A parameterless always disarms; disarms -/// only the bracket that owns the slot, so a stale bracket's late Clear is a logged no-op instead -/// of silently disarming a newer sync. Overlapping arm→clear brackets are still not meaningful — -/// the scheduler must serialize sync runs — but no interleaving of calls can corrupt the state -/// word; the worst outcome of an overlap is now an early disarm via a force- -/// (or a stale pure-data set, possibly for that whole bracket — see above). -/// answers per-project queries from the pure-data set and is deliberately -/// narrower than the global gate. +/// Per-project rejection; one sync slot; overlap semantics. Rejection is PER-PROJECT: while a +/// set of projects is syncing, only writes to THOSE projects are rejected — a write to any other +/// project proceeds normally. This is a DELIBERATE product requirement (PT9 parity — a sync locks +/// only the project it is syncing, so a user can keep editing project B while project A syncs). +/// Both and consult _blockedProjectIds, and +/// the safety invariant holds (narrowing rejection can only allow more writes; the +/// count++/arm ordering still bars any write to a synced project). There is still ONE arm slot (one +/// armed flag + generation): a repeat while armed takes over the slot — it +/// replaces the armed project-id set and returns a NEW token, invalidating every earlier one (call +/// it once per sync batch with the full set, and Clear with the latest token). A parameterless +/// always disarms; disarms only the bracket that +/// owns the slot, so a stale bracket's late Clear is a logged no-op instead of silently disarming a +/// newer sync. Overlapping arm→clear brackets are still not meaningful — the scheduler must +/// serialize sync runs — but no interleaving of calls can corrupt the state word; the worst outcome +/// of an overlap is an early disarm via a force- (or a stale pure-data set, +/// possibly for that whole bracket — see the benign windows below). +/// +/// +/// Two benign flag/set-skew windows (why the per-project read is always safe). The armed +/// flag and _blockedProjectIds are two separate words, so a disarm updates them in some +/// order and there is a brief window where they disagree. Both windows resolve to "allow the write", +/// never to a wrong rejection: +/// +/// +/// disarms flag-then-data (the CAS clears the flag; then the set is +/// dropped to empty). In between: disarmed but the set is still non-empty. Harmless — rejection +/// short-circuits on the flag (armed && set.Contains), so a disarmed gate rejects +/// nothing regardless of the stale set. +/// +/// +/// clears data-then-flag (the set is emptied; then the flag is +/// cleared). In between: still armed but the set is already empty, so writes to every project are +/// briefly allowed while technically armed. Acceptable — is the crash-recovery +/// hammer whose whole job is to stop rejecting; allowing writes a beat early is exactly its intent. +/// +/// +/// (Symmetrically, the arm side publishes data-then-flag — the set is fully populated before the +/// flag is set — so an armed reader never sees a stale/empty set for the arm it observes.) +/// answers per-project queries from the pure-data set alone (it ignores the +/// flag), so during these windows it can momentarily disagree with the gate. /// /// /// Distinct from the Send/Receive server-side repository lock. This class is an @@ -144,6 +188,42 @@ internal static class SendReceiveWriteLock // generic permissions message) and revert the un-saved change. public const string EditBlockedSentinel = "(SR_EDIT_BLOCKED)"; + /// + /// Raised whenever the block state changes: after a successful arm (with the armed ids), and + /// after any disarm (with an empty set). A stale-token no-op and an + /// already-disarmed do NOT raise (no transition occurred). This is a + /// backend-authoritative signal a subscriber can forward to the renderer so the UI never has to + /// infer the block state from indirect cues. + /// + /// Subscribers MUST NOT throw and MUST return quickly: handlers run inline on the arming/clearing + /// thread, and a slow handler delays that transition. This class is a pure synchronization + /// primitive with no logger, so it swallows any exception a handler leaks (it must never let a + /// subscriber fault corrupt the gate or abort an arm/clear) — real error handling and logging + /// belong to the subscriber (see ). Inert in public + /// core: nothing arms the gate there, so this never fires. + /// + /// + /// Honest about ordering (deliberately no sequence stamp). A raise happens AFTER its + /// transition's atomic CAS on the state word, not as part of it, and carries no sequence + /// number. Under the serialized-scheduler contract (one bracket at a time — see the class + /// remarks), that is moot: a single thread's CAS-then-raise pairs are trivially delivered in + /// the order they happened. The contract has exactly two documented escapes: + /// may run on ANY thread as crash recovery, and a takeover + /// can race a stale bracket's . Off-contract + /// like that, two transitions on + /// different threads can have their CAS succeed in one order but their raises run in the other + /// (the winning thread can be preempted between its CAS and its raise while the other thread's + /// CAS-then-raise both complete first) — a subscriber can then briefly observe events out of + /// order and hold a stale snapshot. This is self-correcting: can + /// always re-seed on demand, and the very next transition raises the true state again. Given + /// that self-correction and that the reordering is only reachable outside the serialized + /// contract this class otherwise guarantees, adding a sequence stamp is deliberately YAGNI — it + /// would only ever matter for these two off-contract races, for a staleness window no worse + /// than the benign flag/set-skew windows already documented above. + /// + /// + public static event Action? BlockStateChanged; + // The single atomic word all mutual exclusion rests on (see the class remarks): bits 0–31 // count in-flight write scopes, bit 32 is the "a sync is armed" flag, and bits 33–62 hold the // 30-bit arm generation — the token SetSyncing returns and Clear(token) checks, riding in the @@ -176,6 +256,12 @@ internal static class SendReceiveWriteLock // never mutated. Case-insensitive because project ID casing varies across call sites. private static volatile IImmutableSet _blockedProjectIds = EmptyProjectIds; + // The single not-blocking snapshot, shared across every disarm/idle site (Clear(), Clear(long), + // and GetBlockState's not-blocking branch) so the empty shape is defined exactly once. Safe to + // share as a static readonly value: SendReceiveBlockState is an immutable record struct and its + // empty id collection is never mutated. + private static readonly SendReceiveBlockState NotBlocking = new(false, []); + // How long SetSyncing waits for in-flight writes to drain before giving up. BOUNDED on purpose: // a sync start must never be able to deadlock behind a write scope that (through a bug, a stuck // ParatextData call, or an editor that forgot to dispose) never completes. On timeout we log @@ -208,10 +294,54 @@ internal static class SendReceiveWriteLock /// internal static void ResetForTests(long generation = 0) { + // Drop every BlockStateChanged subscriber too — the event is static, so a subscription a + // test wires up would otherwise leak into later tests (and be invoked by their transitions). + BlockStateChanged = null; _blockedProjectIds = EmptyProjectIds; Volatile.Write(ref _state, (generation << GenerationShift) & GenerationMask); } + /// + /// A snapshot of the current block state: whether the gate is armed, and (when armed) the set of + /// project ids it is rejecting writes for. When not armed, + /// is false and the id set is empty. Always the not-blocking snapshot in public core. + /// + /// Best-effort, not atomic: the armed flag and the pure-data set are two independent reads, so a + /// concurrent arm/clear can land between them (the two benign flag/set windows in the class + /// remarks). Any skew is momentary and self-correcting — every arm and clear also fires + /// , so a subscriber that seeds from this and then follows the + /// event stream converges on the true state. + /// + /// + public static SendReceiveBlockState GetBlockState() + { + bool isBlocking = (Volatile.Read(ref _state) & ArmedFlag) != 0; + // When disarmed, report the shared not-blocking snapshot regardless of the pure-data field + // (Clear() empties the set data-then-flag, so it can briefly still hold ids while already + // disarmed). When blocking, hand out _blockedProjectIds directly — it is already an immutable + // set (safe to share by reference), and its type satisfies SendReceiveBlockState.ProjectIds, + // so there is no need to copy it. This also matches the runtime shape the arm-side raise + // publishes (it passes the ImmutableHashSet straight in). + return isBlocking ? new SendReceiveBlockState(true, _blockedProjectIds) : NotBlocking; + } + + // Invokes BlockStateChanged, isolating the gate from any subscriber fault. There is no logger in + // this pure class by design: subscribers must not throw, and the notifier service owns real + // error handling and logging (see the BlockStateChanged remarks). Swallowing here is the + // last-resort backstop that keeps a misbehaving subscriber from corrupting the gate or aborting + // an arm/clear — it must never rethrow. + private static void RaiseBlockStateChanged(SendReceiveBlockState state) + { + try + { + BlockStateChanged?.Invoke(state); + } + catch + { + // Deliberately swallowed — see above. No logger in this class. + } + } + private static InvalidOperationException EditBlocked(string projectId) => new( $"Cannot write to project '{projectId}' while an automatic Send/Receive is in " @@ -221,15 +351,19 @@ private static InvalidOperationException EditBlocked(string projectId) => /// /// Marks the given projects as being synced, then waits (bounded by ) /// for any already-in-flight writes to drain before returning. From the moment this arms the - /// gate, new calls for ANY project fail fast; after it returns, either - /// no write scopes remain open (normal path), or the drain timed out and it has logged a - /// warning and proceeded anyway (degraded path — new writes are still rejected either way; or, - /// with , it rolled this arm back and threw instead), or - /// a concurrent / newer ended this arm mid-drain + /// gate, new calls for a project IN THIS BATCH fail fast (the + /// per-project filter — see the class remarks); a write to a project NOT in the batch is + /// unaffected. After it returns, either no write scopes remain open (normal path — the drain + /// itself is still GLOBAL, so it waits on in-flight writes to every project, not just this + /// batch's), or the drain timed out and it has logged a warning and proceeded anyway (degraded + /// path — new writes to this batch's projects are still rejected either way; or, with + /// , it rolled this arm back and threw instead), or a + /// concurrent / newer ended this arm mid-drain /// (logged — the gate is then no longer rejecting on behalf of THIS sync). Replaces any /// previously set project list; call once per sync batch with the full set of projects. Null or - /// empty ids in the batch are ignored (an all-invalid batch still arms the global gate, with a - /// logged warning). + /// empty ids in the batch are ignored — an all-invalid batch still arms (and the drain still + /// runs) but then rejects NOTHING, since the per-project filter it arms with is empty (see the + /// logged warning below). /// /// May be called from any thread; the bracket-ending Clear may later run on a different thread /// (an await between them is fine). End the bracket with and @@ -264,14 +398,18 @@ public static long SetSyncing(IEnumerable projectIds, bool throwOnDrainT .ToImmutableHashSet(StringComparer.OrdinalIgnoreCase); // An all-invalid batch is almost certainly a caller bug (e.g. a failed project lookup). - // Still arm — fail-safe: an armed gate with an empty set rejects writes exactly like any - // other arm — but say so, because IsBlocked will report false for every project while - // every write is rejected, which is otherwise baffling to diagnose. + // Still arm, and the drain still runs and returns a valid token — none of that machinery + // cares whether the set is empty. But under the PER-PROJECT filter (EnterWrite/IsBlocked + // both check armed && set.Contains(id)), an empty set can never match, so this arm rejects + // NO writes for ANY project: it is a fully armed, fully toothless bracket for its whole + // lifetime. Say so loudly, because IsBlocked will ALSO report false for every project, so + // nothing here looks broken from the outside — otherwise this is baffling to diagnose. if (armedProjectIds.Count == 0) Console.Error.WriteLine( "[SendReceiveWriteLock] Warning: SetSyncing was called with no valid project ids; " - + "the global gate is armed anyway (all writes rejected), but IsBlocked will " - + "report false for every project." + + "the gate is armed and the drain still ran, but the per-project blocked set " + + "is empty, so this arm blocks NO writes for any project (IsBlocked will also " + + "report false for every project)." ); // Publish the pure data first, then arm. Arming is a single CAS on the state word that @@ -292,6 +430,11 @@ public static long SetSyncing(IEnumerable projectIds, bool throwOnDrainT break; } + // The block state just changed (armed, with this batch's ids) — announce it. Raised here, + // right after the arm, so the signal fires on every arm including the degraded path; on the + // throwOnDrainTimeout path the subsequent rollback Clear(token) fires its own disarm signal. + RaiseBlockStateChanged(new SendReceiveBlockState(true, armedProjectIds)); + // Drain: wait (bounded) until no write scopes remain open — or until this arm is ended by // a concurrent Clear (without that second condition the loop's premise would be gone: once // disarmed, writes enter again and the count may never settle, so the wait would just burn @@ -362,7 +505,12 @@ public static long SetSyncing(IEnumerable projectIds, bool throwOnDrainT public static void Clear() { _blockedProjectIds = EmptyProjectIds; - Interlocked.And(ref _state, ~ArmedFlag); + long previous = Interlocked.And(ref _state, ~ArmedFlag); + // Announce the disarm only if this call actually cleared an armed flag — a Clear() when + // nothing was armed is a no-op and must not fire a spurious "changed" signal (consistent + // with the stale-token Clear(long) no-op below). + if ((previous & ArmedFlag) != 0) + RaiseBlockStateChanged(NotBlocking); } /// @@ -391,6 +539,9 @@ public static void Clear(long token) { // The disarm won atomically against the token check; now drop the pure-data set. _blockedProjectIds = EmptyProjectIds; + // A real disarm happened (this bracket owned the slot) — announce it. The + // stale-token and idempotent-no-op paths above return without raising. + RaiseBlockStateChanged(NotBlocking); return; } } @@ -400,8 +551,10 @@ public static void Clear(long token) /// Whether writes to are currently blocked by an in-progress /// automatic Send/Receive. Always false in public core (see the class remarks). Kept for /// read-only consumers (e.g. status queries); write paths must use so - /// their mutation is what the sync's drain waits for. Note this pure-data answer is per-project, - /// whereas is a global gate (any armed sync rejects all writes). + /// their mutation is what the sync's drain waits for. This answer is per-project and agrees + /// with 's rejection decision (both consult the same blocked set), apart + /// from the two benign flag/set-skew windows noted in the class remarks — this pure-data query + /// ignores the armed flag, so during those windows it can momentarily disagree with the gate. /// public static bool IsBlocked(string? projectId) { @@ -413,32 +566,47 @@ public static bool IsBlocked(string? projectId) /// method that mutates the project, so the scope brackets the whole mutation: /// using var _ = SendReceiveWriteLock.EnterWrite(projectId); /// Throws immediately (message ending in ) if a Send/Receive is - /// armed — it NEVER queues or blocks the caller. Otherwise the open scope counts as an in-flight - /// write until disposed, and a sync starting via waits for it to close - /// before replacing files. A no-op gate in public core (nothing arms the gate there). + /// armed AND is one of the projects being synced — it NEVER queues + /// or blocks the caller. A write to a project NOT in the syncing set proceeds (its on-disk files + /// are not what this sync replaces). Otherwise the open scope counts as an in-flight write until + /// disposed, and a sync starting via waits for it to close before + /// replacing files. A no-op gate in public core (nothing arms the gate there). /// /// The scope is forgiving: it may be disposed on a different thread (holding it across an /// await is safe, though scopes should stay tight — a long-held scope delays a sync's /// drain toward its timeout), and double-dispose is a no-op. Nesting does not crash, but it is - /// NOT safe around a live sync — if a sync arms while the outer scope is open, the inner call - /// throws mid-mutation (see the class remarks); keep one scope per mutation. + /// NOT safe around a live sync of the SAME project — if a sync of this project arms while the + /// outer scope is open, the inner call for that project throws mid-mutation (see the class + /// remarks); keep one scope per mutation. /// - /// Because arming is global, this is a global gate: while ANY project is syncing, ALL project - /// writes are rejected (syncs are globally exclusive by design). + /// This is a PER-PROJECT gate: while a set of projects is syncing, only writes to THOSE projects + /// are rejected; writes to any other project flow normally (PT9 parity — a sync locks only the + /// project it is syncing, so editing project B stays possible while project A syncs). /// public static IDisposable EnterWrite(string projectId) { ArgumentNullException.ThrowIfNull(projectId); - // Atomically "observe not-armed AND count myself in-flight" in one read-modify-write on the - // state word. This is the load-bearing step: arming is also a single operation on the same - // word, so this either happens entirely before the arm (the sync's drain then waits for the - // scope to close) or entirely after it (rejected here). There is no interleaving in which a - // write slips past an armed sync. + // Atomically "observe (not-armed OR not-my-project) AND count myself in-flight" in one + // read-modify-write on the state word. This is the load-bearing step: arming is also a + // single operation on the same word, so the count++ either happens entirely before the arm + // (the sync's drain then waits for the scope to close) or entirely after it. When it lands + // after the arm, we reject ONLY if this project is in the blocked set. + // + // Why reading the set here is safe (no torn view): SetSyncing publishes _blockedProjectIds + // (a volatile field) BEFORE the CAS that sets the armed flag (data-then-flag). All writes + // to the state word are totally ordered, and Volatile.Read gives acquire ordering, so any + // thread that observes the armed bit is guaranteed to also observe the fully-populated set + // that was published ahead of it (or a NEWER set from a takeover arm — equally valid). We + // therefore never see "armed" paired with a stale/empty set belonging to an earlier arm. + // The set is re-read on every loop iteration together with the freshly re-read state, so a + // failed CAS that re-reads a newer state also re-reads the set consistent with it. (Two + // benign windows where flag and set momentarily disagree are documented in the class + // remarks — both resolve to "allow", never to a wrong rejection.) while (true) { long state = Volatile.Read(ref _state); - if ((state & ArmedFlag) != 0) + if ((state & ArmedFlag) != 0 && _blockedProjectIds.Contains(projectId)) throw EditBlocked(projectId); if (Interlocked.CompareExchange(ref _state, state + 1, state) == state) return new WriteScope(); diff --git a/extensions/src/platform-scripture-editor/src/main.ts b/extensions/src/platform-scripture-editor/src/main.ts index baa6128294f..215634eddb5 100644 --- a/extensions/src/platform-scripture-editor/src/main.ts +++ b/extensions/src/platform-scripture-editor/src/main.ts @@ -309,14 +309,50 @@ async function open( if (needsOverlay) { // Create emitters lazily on first use, after full activation, to avoid network init races. + // Uses the async factory (the sync createNetworkEventEmitter is deprecated) so the events can + // carry `x-experimental` in the generated OpenRPC document — these are recently-added switch- + // pairing events whose contract isn't settled (see their @experimental TSDoc in the d.ts). The + // payload type is inferred from the NetworkEvents augmentation, so no explicit generic here. + // Awaited before emit: the enclosing handler is async, so this only adds first-use latency and + // the will-start event still fires before the switch below. if (!projectSwitchWillStartEmitter) - projectSwitchWillStartEmitter = papi.network.createNetworkEventEmitter<{ - switchId: string; - }>(PROJECT_SWITCH_WILL_START_EVENT); + projectSwitchWillStartEmitter = await papi.network.createNetworkEventEmitterAsync( + PROJECT_SWITCH_WILL_START_EVENT, + { + notification: { + 'x-experimental': true, + summary: + 'Emitted just before a scripture editor web view is opened or replaced with a new project.', + params: [ + { + name: 'switchId', + required: true, + summary: 'Token pairing this switch with its matching onDidSwitchProject event.', + schema: { type: 'string' }, + }, + ], + }, + }, + ); if (!projectSwitchDidFinishEmitter) - projectSwitchDidFinishEmitter = papi.network.createNetworkEventEmitter<{ - switchId: string; - }>(PROJECT_SWITCH_DID_FINISH_EVENT); + projectSwitchDidFinishEmitter = await papi.network.createNetworkEventEmitterAsync( + PROJECT_SWITCH_DID_FINISH_EVENT, + { + notification: { + 'x-experimental': true, + summary: 'Emitted after the scripture editor web view open/replace call resolves.', + params: [ + { + name: 'switchId', + required: true, + summary: + 'The switchId of the onWillSwitchProject event that started this switch.', + schema: { type: 'string' }, + }, + ], + }, + }, + ); projectSwitchWillStartEmitter.emit({ switchId }); const outgoing = allScriptureEditors.find((e) => e.id === dispatch.targetTabId); diff --git a/extensions/src/platform-scripture-editor/src/types/platform-scripture-editor.d.ts b/extensions/src/platform-scripture-editor/src/types/platform-scripture-editor.d.ts index e1456490035..387327b97f2 100644 --- a/extensions/src/platform-scripture-editor/src/types/platform-scripture-editor.d.ts +++ b/extensions/src/platform-scripture-editor/src/types/platform-scripture-editor.d.ts @@ -697,6 +697,9 @@ declare module 'papi-shared-types' { * `switchId` uniquely identifies this switch and pairs it with the matching * `platformScriptureEditor.onDidSwitchProject` event. Subscribe with * `papi.network.getNetworkEvent('platformScriptureEditor.onWillSwitchProject')`. + * + * @experimental Recently-added switch-pairing plumbing for the workspace-updating overlay; the + * payload shape and event name are not yet a settled contract and may change. */ 'platformScriptureEditor.onWillSwitchProject': { switchId: string }; /** @@ -704,6 +707,9 @@ declare module 'papi-shared-types' { * `switchId` of the `platformScriptureEditor.onWillSwitchProject` event that started the * switch. Subscribe with * `papi.network.getNetworkEvent('platformScriptureEditor.onDidSwitchProject')`. + * + * @experimental Recently-added switch-pairing plumbing for the workspace-updating overlay; the + * payload shape and event name are not yet a settled contract and may change. */ 'platformScriptureEditor.onDidSwitchProject': { switchId: string }; } diff --git a/lib/papi-dts/papi.d.ts b/lib/papi-dts/papi.d.ts index af07a7582af..c6930b25470 100644 --- a/lib/papi-dts/papi.d.ts +++ b/lib/papi-dts/papi.d.ts @@ -8495,14 +8495,11 @@ declare module 'shared/data/platform.data' { * (which has its own progress and Cancel). A sync of a large repo can run for minutes, so this is * deliberately long. * - * Two different consumers share this value and must never diverge: - * - * - The main process (`shutdown-tasks.ts`) bounds how long app shutdown waits on its final sync. - * - The renderer (`auto-sync-blocking-store.ts`) bounds each edit-block safety leash — how long a - * single blocker may block editing if its clearing event never arrives. - * - * If they diverged, a shutdown sync could outlive the renderer's opinion of it (or vice versa); - * tune this value knowing it retimes both. + * Consumed by the main process (`shutdown-tasks.ts`), which uses it to bound how long app shutdown + * waits on its final sync. It also conceptually matches the C# write gate's stall watchdog, which + * bounds the same "one automatic Send/Receive" window. The renderer does not time blocking locally + * — it reads the backend write gate's snapshot (`auto-sync-blocking-store.ts`), so blocking clears + * when the backend says so rather than on a renderer-side timer. * * @experimental */ diff --git a/src/renderer/index.tsx b/src/renderer/index.tsx index c5add733a5e..5dc92d616e3 100644 --- a/src/renderer/index.tsx +++ b/src/renderer/index.tsx @@ -118,10 +118,12 @@ async function runPromisesAndThrowIfRejected(...promises: Promise[]) { initializeWindowService(), ); - // Drives the auto-sync edit-block banner on Scripture editors during a scheduled Send/Receive. - // Needs the network service (already up above) for the blocking event and the web view service - // (already up, from the block above) to read/update editor definitions. Both are synchronous and - // return unsubscribers we intentionally never call — they run for the renderer's lifetime. + // Drives the auto-sync edit-block banner on Scripture editors during a Send/Receive. Needs the + // network service (already up above) for the blocking event and the web view service (already + // up, from the block above) to read/update editor definitions. Both return unsubscribers we + // intentionally never call — they run for the renderer's lifetime. The blocking service also + // launches a fire-and-forget consult of the backend's current blocking snapshot, so a renderer + // reload during an in-flight sync seeds the store instead of assuming unblocked. initAutoSyncBlockingService(); initAutoSyncEditBlockDriver(); diff --git a/src/renderer/services/auto-sync-blocking-integration.test.ts b/src/renderer/services/auto-sync-blocking-integration.test.ts deleted file mode 100644 index 4931b0c5b88..00000000000 --- a/src/renderer/services/auto-sync-blocking-integration.test.ts +++ /dev/null @@ -1,85 +0,0 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { getNetworkEvent } from '@shared/services/network.service'; -import { - getAutoSyncBlocking, - resetAutoSyncBlocking, -} from '@renderer/services/auto-sync-blocking-store'; -import { initAutoSyncBlockingService } from './auto-sync-blocking-service'; - -vi.mock('@shared/services/network.service', () => ({ - getNetworkEvent: vi.fn(), - requestNoRetry: vi.fn(), -})); - -vi.mock('@shared/services/logger.service', () => ({ - logger: { debug: vi.fn(), warn: vi.fn() }, -})); - -const TEN_MINUTES_MS = 10 * 60 * 1000; - -/** - * Integration tests: real auto-sync-blocking store driven through the service by simulated blocking - * events, under fake timers. The blocking events are anonymous booleans, so the service pairs - * clears to raises oldest-first — but a raise whose own leash already fired must absorb its late - * clear as a no-op instead of releasing a newer, still-live blocker (the #2571 review's finding 1 - * trace, in this store's 10-minute shape). - */ -describe('auto-sync blocking service + store integration', () => { - let blockingHandler: ((event: { isBlocking: boolean }) => void) | undefined; - let cleanup: (() => void) | undefined; - - beforeEach(() => { - vi.useFakeTimers(); - resetAutoSyncBlocking(); - blockingHandler = undefined; - - vi.mocked(getNetworkEvent).mockImplementation( - // getNetworkEvent has a complex generic signature; cast is required for the mock - // eslint-disable-next-line no-type-assertion/no-type-assertion, @typescript-eslint/no-explicit-any - ((eventName: string) => { - if (eventName === 'paratextBibleSendReceive.onAutoSyncBlockingChanged') - return (cb: (event: { isBlocking: boolean }) => void) => { - blockingHandler = cb; - return vi.fn(); - }; - return () => vi.fn(); - // Same cast as above: closing the type assertion needed for the complex generic signature - // eslint-disable-next-line no-type-assertion/no-type-assertion, @typescript-eslint/no-explicit-any - }) as any, - ); - - cleanup = initAutoSyncBlockingService(); - }); - - afterEach(() => { - cleanup?.(); - resetAutoSyncBlocking(); - vi.useRealTimers(); - }); - - function emitBlocking(isBlocking: boolean): void { - if (!blockingHandler) throw new Error('blocking handler not subscribed'); - blockingHandler({ isBlocking }); - } - - it('keeps a live blocker blocked when an older raise clears late, after its own leash fired (review finding 1 trace)', () => { - // A raises at t=0; B raises just before A's 10-minute leash fires. - emitBlocking(true); - vi.advanceTimersByTime(TEN_MINUTES_MS - 10_000); - emitBlocking(true); - - // t=10min: A's leash fires. A is released; B is still blocking. - vi.advanceTimersByTime(10_000); - expect(getAutoSyncBlocking()).toBe(true); - - // A's real clear arrives late. It must pair with A (already released by its own leash — a - // no-op), never with B: B's sync is still running, so blocking must stay on. - vi.advanceTimersByTime(30_000); - emitBlocking(false); - expect(getAutoSyncBlocking()).toBe(true); - - // B's own leash fires (B never cleared) — now everything is released. - vi.advanceTimersByTime(TEN_MINUTES_MS); - expect(getAutoSyncBlocking()).toBe(false); - }); -}); diff --git a/src/renderer/services/auto-sync-blocking-service.test.ts b/src/renderer/services/auto-sync-blocking-service.test.ts index 0eb1afd3109..a1cadd66df9 100644 --- a/src/renderer/services/auto-sync-blocking-service.test.ts +++ b/src/renderer/services/auto-sync-blocking-service.test.ts @@ -1,22 +1,42 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { getNetworkEvent } from '@shared/services/network.service'; -import { raiseAutoSyncBlock } from '@renderer/services/auto-sync-blocking-store'; +import { getNetworkEvent, requestNoRetry } from '@shared/services/network.service'; +import { setBlockedProjects } from '@renderer/services/auto-sync-blocking-store'; +import { logger } from '@shared/services/logger.service'; import { initAutoSyncBlockingService } from './auto-sync-blocking-service'; vi.mock('@shared/services/network.service', () => ({ getNetworkEvent: vi.fn(), + requestNoRetry: vi.fn(), })); vi.mock('@renderer/services/auto-sync-blocking-store', () => ({ - raiseAutoSyncBlock: vi.fn(), + setBlockedProjects: vi.fn(), })); +vi.mock('@shared/services/logger.service', () => ({ + logger: { debug: vi.fn(), warn: vi.fn() }, +})); + +const SYNC_WRITE_LOCK_CHANGED_EVENT = 'paratextBibleSendReceive.onSyncWriteLockChanged'; +const LEGACY_BLOCKING_CHANGED_EVENT = 'paratextBibleSendReceive.onAutoSyncBlockingChanged'; + +/** Flushes the service's fire-and-forget seeding chain (await + then-callbacks). */ +async function flushSeeding(): Promise { + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); +} + describe('initAutoSyncBlockingService', () => { - let capturedHandler: ((event: { isBlocking: boolean }) => void) | undefined; + /** Every event name getNetworkEvent was subscribed for. */ + let subscribedEventNames: string[]; + /** The handler the service registered for the write-lock-changed event; receives raw payloads. */ + let capturedHandler: ((event: unknown) => void) | undefined; let unsub: ReturnType; beforeEach(() => { vi.clearAllMocks(); + subscribedEventNames = []; capturedHandler = undefined; unsub = vi.fn(); @@ -24,8 +44,9 @@ describe('initAutoSyncBlockingService', () => { // getNetworkEvent has a complex generic signature; cast is required for the mock // eslint-disable-next-line no-type-assertion/no-type-assertion, @typescript-eslint/no-explicit-any ((eventName: string) => { - if (eventName === 'paratextBibleSendReceive.onAutoSyncBlockingChanged') - return (cb: (event: { isBlocking: boolean }) => void) => { + subscribedEventNames.push(eventName); + if (eventName === SYNC_WRITE_LOCK_CHANGED_EVENT) + return (cb: (event: unknown) => void) => { capturedHandler = cb; return unsub; }; @@ -34,63 +55,138 @@ describe('initAutoSyncBlockingService', () => { // eslint-disable-next-line no-type-assertion/no-type-assertion, @typescript-eslint/no-explicit-any }) as any, ); + + // Default: no core serves the initial-state command (an older core / absent extension). + vi.mocked(requestNoRetry).mockRejectedValue(new Error('command not registered')); }); - function emit(isBlocking: boolean): void { - if (!capturedHandler) throw new Error('blocking handler not subscribed'); - capturedHandler({ isBlocking }); - } + describe('event source', () => { + it('subscribes to the backend write-lock-changed event', () => { + initAutoSyncBlockingService(); + expect(subscribedEventNames).toContain(SYNC_WRITE_LOCK_CHANGED_EVENT); + }); - it('raises a block when the event fires with isBlocking true', () => { - vi.mocked(raiseAutoSyncBlock).mockReturnValue(vi.fn()); - initAutoSyncBlockingService(); - emit(true); - expect(vi.mocked(raiseAutoSyncBlock)).toHaveBeenCalledTimes(1); - }); + it('does NOT subscribe to the legacy onAutoSyncBlockingChanged event', () => { + initAutoSyncBlockingService(); + expect(subscribedEventNames).not.toContain(LEGACY_BLOCKING_CHANGED_EVENT); + }); - it('pairs each false event with the oldest raise that has not consumed a clear', () => { - const clearA = vi.fn(); - const clearB = vi.fn(); - vi.mocked(raiseAutoSyncBlock).mockReturnValueOnce(clearA).mockReturnValueOnce(clearB); - initAutoSyncBlockingService(); - emit(true); // raise A - emit(true); // raise B - emit(false); // A's clear - expect(clearA).toHaveBeenCalledTimes(1); - expect(clearB).not.toHaveBeenCalled(); - emit(false); // B's clear - expect(clearB).toHaveBeenCalledTimes(1); + it('returns a cleanup function that unsubscribes the event', () => { + const cleanup = initAutoSyncBlockingService(); + cleanup(); + expect(unsub).toHaveBeenCalledTimes(1); + }); }); - it('keeps a leash-released raise queued as a tombstone so its late clear cannot consume a newer raise', () => { - // Raise A's block gets released by the store's safety leash before its clear event arrives. - // The service must still hand A's late clear to A's (now no-op) token — never to B's. - const clearA = vi.fn(); - const clearB = vi.fn(); - vi.mocked(raiseAutoSyncBlock).mockReturnValueOnce(clearA).mockReturnValueOnce(clearB); - initAutoSyncBlockingService(); - emit(true); // raise A (store leash-releases it later; the service cannot know) - emit(true); // raise B - emit(false); // A's late clear — consumes A's idempotent token - emit(false); // B's clear — pairs with B - expect(clearA).toHaveBeenCalledTimes(1); - expect(clearB).toHaveBeenCalledTimes(1); - }); + describe('event consumption', () => { + it('blocks the reported projects when a blocking snapshot arrives', () => { + initAutoSyncBlockingService(); + if (!capturedHandler) throw new Error('capturedHandler not set'); + capturedHandler({ isBlocking: true, projectIds: ['p1', 'p2'] }); + expect(vi.mocked(setBlockedProjects)).toHaveBeenCalledWith(['p1', 'p2']); + }); + + it('blocks nothing when a not-blocking snapshot arrives', () => { + initAutoSyncBlockingService(); + if (!capturedHandler) throw new Error('capturedHandler not set'); + capturedHandler({ isBlocking: false, projectIds: [] }); + expect(vi.mocked(setBlockedProjects)).toHaveBeenCalledWith([]); + }); - it('ignores a false event with no outstanding raise', () => { - const clear = vi.fn(); - vi.mocked(raiseAutoSyncBlock).mockReturnValue(clear); - initAutoSyncBlockingService(); - emit(false); // emitted before this subscription existed — nothing to release - expect(clear).not.toHaveBeenCalled(); - emit(true); - emit(false); // pairs with the raise above, unaffected by the stray clear - expect(clear).toHaveBeenCalledTimes(1); + it('fails safe to block-none and warns once on a malformed snapshot', () => { + initAutoSyncBlockingService(); + if (!capturedHandler) throw new Error('capturedHandler not set'); + capturedHandler({ isBlocking: 'yes', projectIds: 'nope' }); // wrong types + capturedHandler(undefined); // missing entirely + expect(vi.mocked(setBlockedProjects)).toHaveBeenCalledTimes(2); + expect(vi.mocked(setBlockedProjects)).toHaveBeenNthCalledWith(1, []); + expect(vi.mocked(setBlockedProjects)).toHaveBeenNthCalledWith(2, []); + // Warn once per service lifetime, not once per malformed event. + expect(vi.mocked(logger.warn)).toHaveBeenCalledTimes(1); + }); + + it('fails safe to block-none when projectIds contains a non-string', () => { + initAutoSyncBlockingService(); + if (!capturedHandler) throw new Error('capturedHandler not set'); + // capturedHandler takes an unknown payload, so a mixed array needs no cast. + capturedHandler({ isBlocking: true, projectIds: ['p1', 2] }); + expect(vi.mocked(setBlockedProjects)).toHaveBeenCalledWith([]); + expect(vi.mocked(logger.warn)).toHaveBeenCalledTimes(1); + }); }); - it('returns a cleanup function that unsubscribes the event', () => { - const cleanup = initAutoSyncBlockingService(); - cleanup(); - expect(unsub).toHaveBeenCalledTimes(1); + describe('seeding from the initial-state query', () => { + it('queries the current blocking state on init', async () => { + initAutoSyncBlockingService(); + await flushSeeding(); + expect(vi.mocked(requestNoRetry)).toHaveBeenCalledWith( + 'command:paratextBibleSendReceive.getAutoSyncBlocking', + ); + }); + + it('seeds the blocked projects when the query reports an in-flight sync (reload mid-sync)', async () => { + vi.mocked(requestNoRetry).mockResolvedValue({ isBlocking: true, projectIds: ['p1'] }); + initAutoSyncBlockingService(); + await flushSeeding(); + expect(vi.mocked(setBlockedProjects)).toHaveBeenCalledTimes(1); + expect(vi.mocked(setBlockedProjects)).toHaveBeenCalledWith(['p1']); + }); + + it('does not seed when the query reports no sync in flight', async () => { + vi.mocked(requestNoRetry).mockResolvedValue({ isBlocking: false, projectIds: [] }); + initAutoSyncBlockingService(); + await flushSeeding(); + expect(vi.mocked(setBlockedProjects)).not.toHaveBeenCalled(); + }); + + it('does not seed when the query result is malformed', async () => { + vi.mocked(requestNoRetry).mockResolvedValue('yes'); + initAutoSyncBlockingService(); + await flushSeeding(); + expect(vi.mocked(setBlockedProjects)).not.toHaveBeenCalled(); + }); + + it('swallows a failed query and keeps the assume-unblocked default', async () => { + vi.mocked(requestNoRetry).mockRejectedValue(new Error('extension absent')); + initAutoSyncBlockingService(); + await flushSeeding(); + expect(vi.mocked(setBlockedProjects)).not.toHaveBeenCalled(); + }); + + it('lets a live event win over the snapshot — no seeding after an event arrived', async () => { + let resolveQuery: ((snapshot: unknown) => void) | undefined; + vi.mocked(requestNoRetry).mockImplementation( + async () => + new Promise((resolve) => { + resolveQuery = resolve; + }), + ); + initAutoSyncBlockingService(); + if (!capturedHandler) throw new Error('capturedHandler not set'); + // The sync the snapshot would report ends while the query is in flight. + capturedHandler({ isBlocking: false, projectIds: [] }); + if (!resolveQuery) throw new Error('resolveQuery not set'); + resolveQuery({ isBlocking: true, projectIds: ['p1'] }); // stale snapshot arrives late + await flushSeeding(); + // Only the event's call — the stale snapshot did not add a phantom block. + expect(vi.mocked(setBlockedProjects)).toHaveBeenCalledTimes(1); + expect(vi.mocked(setBlockedProjects)).toHaveBeenCalledWith([]); + }); + + it('does not seed after cleanup', async () => { + let resolveQuery: ((snapshot: unknown) => void) | undefined; + vi.mocked(requestNoRetry).mockImplementation( + async () => + new Promise((resolve) => { + resolveQuery = resolve; + }), + ); + const cleanup = initAutoSyncBlockingService(); + cleanup(); + if (!resolveQuery) throw new Error('resolveQuery not set'); + resolveQuery({ isBlocking: true, projectIds: ['p1'] }); + await flushSeeding(); + expect(vi.mocked(setBlockedProjects)).not.toHaveBeenCalled(); + }); }); }); diff --git a/src/renderer/services/auto-sync-blocking-service.ts b/src/renderer/services/auto-sync-blocking-service.ts index 946392e0a1e..03b743662e8 100644 --- a/src/renderer/services/auto-sync-blocking-service.ts +++ b/src/renderer/services/auto-sync-blocking-service.ts @@ -1,43 +1,112 @@ -import { getNetworkEvent } from '@shared/services/network.service'; -import { raiseAutoSyncBlock } from './auto-sync-blocking-store'; +import { CATEGORY_COMMAND } from '@shared/data/rpc.model'; +import { logger } from '@shared/services/logger.service'; +import { getNetworkEvent, requestNoRetry } from '@shared/services/network.service'; +import { serializeRequestType } from '@shared/utils/util'; +import { getErrorMessage, isString } from 'platform-bible-utils'; +import { setBlockedProjects } from './auto-sync-blocking-store'; -// String value must match the event emitted by the Send/Receive extension. The extension emits it -// only around scheduled (unattended) syncs — the surface that blocks the workspace. Manual syncs -// (driven from the Send/Receive dialog, which has its own progress and Cancel) never raise it. -// `true` raises a block, `false` clears one. -const AUTO_SYNC_BLOCKING_CHANGED_EVENT = 'paratextBibleSendReceive.onAutoSyncBlockingChanged'; +/** + * Backend-authoritative snapshot of which projects an automatic Send/Receive is blocking edits on. + * Carried identically by both the change event and the init-consult command (see the C# + * `SendReceiveBlockState`). `isBlocking` false always pairs with an empty `projectIds`. + */ +type SyncWriteLockSnapshot = { isBlocking: boolean; projectIds: string[] }; + +// Backend-authoritative network event: the C# write gate emits a full snapshot on every arm/disarm, +// for ALL sync types (manual + scheduled + session). This is the single signal source for blocking +// — the store never combines it with any other opinion. Only fires in Paratext 10 Studio builds +// where the patch arms the gate; plain Platform.Bible never emits it. +const SYNC_WRITE_LOCK_CHANGED_EVENT = 'paratextBibleSendReceive.onSyncWriteLockChanged'; + +/** + * Command served by the C# backend (the emitter of {@link SYNC_WRITE_LOCK_CHANGED_EVENT}) returning + * the current {@link SyncWriteLockSnapshot}, so this service can seed the store on init instead of + * assuming unblocked. Requested untyped (the same pattern as `paratextBibleSendReceive.cancelSync` + * in shutdown-tasks) because the copied `paratext-bible-send-receive.d.ts` command contract does + * not declare it. Served on Paratext 10 Studio (returns not-blocking on plain Platform.Bible); + * older cores lack it entirely and the request rejects, leaving the assume-unblocked default. + */ +const GET_AUTO_SYNC_BLOCKING_COMMAND = 'paratextBibleSendReceive.getAutoSyncBlocking'; /** - * Subscribes to the auto-sync blocking network event and drives the auto-sync-blocking store. Call + * Subscribes to the backend write-gate change event and drives the auto-sync-blocking store. Call * once at app startup. Returns a cleanup function. * - * The event payload is an anonymous boolean, so this service pairs each `false` with the oldest - * raise that has not yet consumed a clear. A raise whose own safety leash already fired absorbs its - * late clear as a no-op (the store's release tokens are idempotent), so a late clear can never - * release a newer, still-live blocker. What anonymous pairing cannot fix: a raise whose clear never - * arrives keeps consuming later raises' clears, shifting the unmatched block onto ever-newer raises - * — each still bounded by its own leash, but blocking can persist while syncs keep coming. The real - * fix is identity on the wire; PT-4214 replaces this event with a backend-authoritative per-project - * snapshot, which removes the pairing problem entirely. - * - * Known limitation (deliberate): this service only learns about blocking from live events, so a - * renderer reload during an in-flight scheduled sync comes up unblocked while the backend is still - * syncing (the raise event was emitted before this subscription existed). Seeding the initial state - * requires a backend query that does not exist yet; PT-4214 adds a C#-served - * `paratextBibleSendReceive.getAutoSyncBlocking` snapshot command and an init consult of it. + * On init it also queries the backend's current snapshot (best effort — see + * {@link GET_AUTO_SYNC_BLOCKING_COMMAND}) and seeds the store from it, so a renderer reload during + * an in-flight sync does not come up unblocked while the backend is still syncing (the change event + * was emitted before this subscription existed). Any live event wins over the (possibly stale) + * snapshot. Malformed payloads and a rejected consult both fail safe to assume-unblocked, so a + * broken or absent signal can never leave editors stuck read-only. */ export function initAutoSyncBlockingService(): () => void { - // Clear functions for raises, oldest first, in raise order. A leash-released raise stays here as - // a tombstone until its (late) clear consumes it, keeping later clears paired with later raises. - const pendingClears: (() => void)[] = []; - - const unsubscribe = getNetworkEvent<{ isBlocking: boolean }>(AUTO_SYNC_BLOCKING_CHANGED_EVENT)(({ - isBlocking, - }) => { - if (isBlocking) pendingClears.push(raiseAutoSyncBlock()); - // A clear with no outstanding raise (emitted before this subscription existed) is ignored. - else pendingClears.shift()?.(); + let hasReceivedEvent = false; + let isDisposed = false; + let hasWarnedMalformed = false; + + /** + * Extracts the project ids to block from an untrusted snapshot payload. A well-formed + * not-blocking snapshot yields `[]`; a malformed/missing-field payload also yields `[]` + * (fail-safe assume-unblocked) and warns once per service lifetime, consistent with the + * assume-unblocked init philosophy — a broken signal must never leave the workspace blocked. + */ + const readBlockedProjectIds = (payload: unknown): string[] => { + if ( + typeof payload === 'object' && + payload && + 'isBlocking' in payload && + 'projectIds' in payload + ) { + const { isBlocking, projectIds } = payload; + if (typeof isBlocking === 'boolean' && Array.isArray(projectIds)) { + const stringIds = projectIds.filter(isString); + if (stringIds.length === projectIds.length) return isBlocking ? stringIds : []; + } + } + if (!hasWarnedMalformed) { + hasWarnedMalformed = true; + logger.warn( + `auto-sync blocking service received a malformed ${SYNC_WRITE_LOCK_CHANGED_EVENT} snapshot; assuming not blocking`, + ); + } + return []; + }; + + const unsubscribe = getNetworkEvent(SYNC_WRITE_LOCK_CHANGED_EVENT)(( + event, + ) => { + hasReceivedEvent = true; + setBlockedProjects(readBlockedProjectIds(event)); }); - return unsubscribe; + // LIMITATION: this init consult is the only backend re-seed — there is no re-query on websocket + // reconnect, C# data-provider restart, or editor mount. So if a disarm is ever lost (a faulted + // fire-and-forget SendEventAsync, an off-contract raise reorder, or a provider that restarts + // disarmed without re-emitting), the visible blocked set stays stale until the next real + // transition or a full renderer reload. Closing that gap needs an editor-mount re-query, tracked + // on PT-4214; until that lands, this one-shot seed is the only recovery. + (async () => { + try { + const snapshot = await requestNoRetry<[], unknown>( + serializeRequestType(CATEGORY_COMMAND, GET_AUTO_SYNC_BLOCKING_COMMAND), + ); + // Only seed if nothing live has spoken: an event that arrived while the request was in flight + // (either direction) supersedes this snapshot and must win, so we never clobber it here. + if (!hasReceivedEvent && !isDisposed) { + const blockedProjectIds = readBlockedProjectIds(snapshot); + if (blockedProjectIds.length > 0) setBlockedProjects(blockedProjectIds); + } + } catch (e) { + // Command not served (older core) or the extension is absent — keep the assume-unblocked + // default. Debug, not warn: this is the expected path on plain Platform.Bible and older cores. + logger.debug( + `auto-sync blocking service could not read the initial blocking state: ${getErrorMessage(e)}`, + ); + } + })(); + + return () => { + isDisposed = true; + unsubscribe(); + }; } diff --git a/src/renderer/services/auto-sync-blocking-store.test.ts b/src/renderer/services/auto-sync-blocking-store.test.ts index 6334fc92de9..202991b6baf 100644 --- a/src/renderer/services/auto-sync-blocking-store.test.ts +++ b/src/renderer/services/auto-sync-blocking-store.test.ts @@ -1,18 +1,17 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { - raiseAutoSyncBlock, - getAutoSyncBlocking, + setBlockedProjects, + getBlockedProjectIds, + isProjectBlocked, subscribeToAutoSyncBlocking, resetAutoSyncBlocking, } from './auto-sync-blocking-store'; /** Must match SHOW_GRACE_MS in auto-sync-blocking-store.ts */ const SHOW_GRACE_MS = 200; -/** Must match SAFETY_TIMEOUT_MS in auto-sync-blocking-store.ts */ -const SAFETY_TIMEOUT_MS = 10 * 60 * 1000; describe('auto-sync-blocking-store', () => { - // The store's show-grace and safety timers mean nearly every test needs timer control + // The store's show-grace timer means nearly every test needs timer control. beforeEach(() => { vi.useFakeTimers(); resetAutoSyncBlocking(); @@ -22,83 +21,163 @@ describe('auto-sync-blocking-store', () => { vi.useRealTimers(); }); - describe('getAutoSyncBlocking', () => { - it('returns false initially', () => { - expect(getAutoSyncBlocking()).toBe(false); + describe('initial state', () => { + it('reports nothing blocked', () => { + expect(getBlockedProjectIds().size).toBe(0); + expect(isProjectBlocked('P1')).toBe(false); + }); + + it('treats an undefined project id as never blocked', () => { + expect(isProjectBlocked(undefined)).toBe(false); }); }); describe('show grace', () => { it('is not visible immediately when blocking starts', () => { - raiseAutoSyncBlock(); - expect(getAutoSyncBlocking()).toBe(false); + setBlockedProjects(['P1']); + expect(getBlockedProjectIds().size).toBe(0); + expect(isProjectBlocked('P1')).toBe(false); }); it('becomes visible once the 200 ms grace elapses', () => { - raiseAutoSyncBlock(); + setBlockedProjects(['P1']); vi.advanceTimersByTime(SHOW_GRACE_MS); - expect(getAutoSyncBlocking()).toBe(true); + expect(isProjectBlocked('P1')).toBe(true); + expect([...getBlockedProjectIds()]).toEqual(['P1']); }); it('never becomes visible when blocking clears within the grace (sync finished fast)', () => { const listener = vi.fn(); subscribeToAutoSyncBlocking(listener); - const clear = raiseAutoSyncBlock(); + setBlockedProjects(['P1']); vi.advanceTimersByTime(150); // still inside the grace window - clear(); + setBlockedProjects([]); vi.advanceTimersByTime(SHOW_GRACE_MS); // well past when the grace would have fired - expect(getAutoSyncBlocking()).toBe(false); + expect(isProjectBlocked('P1')).toBe(false); expect(listener).not.toHaveBeenCalled(); // nothing ever showed, so nothing ever notified }); it('does not notify listeners during the grace period', () => { const listener = vi.fn(); subscribeToAutoSyncBlocking(listener); - raiseAutoSyncBlock(); + setBlockedProjects(['P1']); vi.advanceTimersByTime(SHOW_GRACE_MS - 1); expect(listener).not.toHaveBeenCalled(); vi.advanceTimersByTime(1); expect(listener).toHaveBeenCalledTimes(1); }); + + it('shows whatever is blocked when the grace fires, even if the set grew during the grace', () => { + setBlockedProjects(['P1']); + vi.advanceTimersByTime(100); // inside the grace + setBlockedProjects(['P1', 'P2']); // a second project joins the in-flight batch + expect(getBlockedProjectIds().size).toBe(0); // still inside the grace + vi.advanceTimersByTime(SHOW_GRACE_MS); + expect([...getBlockedProjectIds()].sort()).toEqual(['P1', 'P2']); + }); + + it('does not re-arm a fresh grace when a project joins an already-visible batch', () => { + setBlockedProjects(['P1']); + vi.advanceTimersByTime(SHOW_GRACE_MS); + expect(isProjectBlocked('P1')).toBe(true); + setBlockedProjects(['P1', 'P2']); // already visible → reflected immediately, no new grace + expect(isProjectBlocked('P2')).toBe(true); + }); }); - describe('overlapping blockers', () => { - it('stays visible when a second raise arrives before the first clears', () => { - const clearA = raiseAutoSyncBlock(); + describe('case canonicalization (ingestion)', () => { + it('canonicalizes ingested project ids to upper case', () => { + // The backend gate matches ids OrdinalIgnoreCase and the canonical project id is upper; the + // store canonicalizes at ingestion so a mixed/lower-case snapshot still matches a canonical + // (upper) query — no casing skew can leave a project silently unblocked in the UI. + setBlockedProjects(['proj_a', 'Proj_B']); vi.advanceTimersByTime(SHOW_GRACE_MS); - const clearB = raiseAutoSyncBlock(); - clearA(); - expect(getAutoSyncBlocking()).toBe(true); // blocker B still in flight - clearB(); - expect(getAutoSyncBlocking()).toBe(false); + expect([...getBlockedProjectIds()].sort()).toEqual(['PROJ_A', 'PROJ_B']); + expect(isProjectBlocked('PROJ_A')).toBe(true); + expect(isProjectBlocked('PROJ_B')).toBe(true); }); - it('clear is idempotent — clearing twice cannot release another blocker', () => { - const clearA = raiseAutoSyncBlock(); + it('isProjectBlocked matches a non-canonical (mixed/lower-case) query', () => { + // The reader canonicalizes its own argument too, so a caller passing a non-canonical id can't + // miss a real block (mirrors the driver's isEditorBlocked). + setBlockedProjects(['P1']); vi.advanceTimersByTime(SHOW_GRACE_MS); - raiseAutoSyncBlock(); // blocker B, still in flight - clearA(); - clearA(); // duplicate — must be a no-op, not release B - expect(getAutoSyncBlocking()).toBe(true); + expect(isProjectBlocked('p1')).toBe(true); + expect(isProjectBlocked('P1')).toBe(true); }); + }); - it('does not notify listeners when visibility is unchanged (nested raises)', () => { - raiseAutoSyncBlock(); + describe('snapshot replace semantics', () => { + it('replaces the blocked set wholesale', () => { + setBlockedProjects(['P1', 'P2']); + vi.advanceTimersByTime(SHOW_GRACE_MS); + expect([...getBlockedProjectIds()].sort()).toEqual(['P1', 'P2']); + + setBlockedProjects(['P2', 'P3']); // wholesale replace, not a merge + expect(isProjectBlocked('P1')).toBe(false); + expect(isProjectBlocked('P2')).toBe(true); + expect(isProjectBlocked('P3')).toBe(true); + }); + + it('clears blocking when replaced with an empty set', () => { + setBlockedProjects(['P1']); + vi.advanceTimersByTime(SHOW_GRACE_MS); + expect(isProjectBlocked('P1')).toBe(true); + setBlockedProjects([]); + expect(getBlockedProjectIds().size).toBe(0); + }); + + it('notifies once when the visible set content changes', () => { + setBlockedProjects(['P1']); vi.advanceTimersByTime(SHOW_GRACE_MS); const listener = vi.fn(); subscribeToAutoSyncBlocking(listener); - raiseAutoSyncBlock(); // second raise — set grows 1→2, visibility still true + setBlockedProjects(['P1', 'P2']); // content changed → one notify + expect(listener).toHaveBeenCalledTimes(1); + }); + + it('does not notify when the replacement set has identical content', () => { + setBlockedProjects(['P1', 'P2']); + vi.advanceTimersByTime(SHOW_GRACE_MS); + const listener = vi.fn(); + subscribeToAutoSyncBlocking(listener); + setBlockedProjects(['P2', 'P1']); // same content, different order/array → no change expect(listener).not.toHaveBeenCalled(); }); }); + describe('no timer-driven expiry', () => { + it('leaves no pending timer once blocking is visible', () => { + setBlockedProjects(['P1']); + expect(vi.getTimerCount()).toBe(1); // the grace timer + vi.advanceTimersByTime(SHOW_GRACE_MS); + expect(vi.getTimerCount()).toBe(0); // grace fired; NO safety leash left running + }); + + it('never auto-clears a long-running block (no safety timeout)', () => { + setBlockedProjects(['P1']); + vi.advanceTimersByTime(SHOW_GRACE_MS); + expect(isProjectBlocked('P1')).toBe(true); + // Well beyond any plausible sync duration — the block persists until the backend clears it. + vi.advanceTimersByTime(60 * 60 * 1000); + expect(isProjectBlocked('P1')).toBe(true); + expect(vi.getTimerCount()).toBe(0); + }); + + it('leaves no pending timer after blocking clears inside the grace', () => { + setBlockedProjects(['P1']); + setBlockedProjects([]); // cleared inside the grace + expect(vi.getTimerCount()).toBe(0); // grace timer cancelled, nothing else armed + }); + }); + describe('subscribeToAutoSyncBlocking', () => { it('notifies listeners when visibility flips to false', () => { - const clear = raiseAutoSyncBlock(); + setBlockedProjects(['P1']); vi.advanceTimersByTime(SHOW_GRACE_MS); const listener = vi.fn(); subscribeToAutoSyncBlocking(listener); - clear(); + setBlockedProjects([]); expect(listener).toHaveBeenCalledTimes(1); }); @@ -106,7 +185,7 @@ describe('auto-sync-blocking-store', () => { const listener = vi.fn(); const unsubscribe = subscribeToAutoSyncBlocking(listener); unsubscribe(); - raiseAutoSyncBlock(); + setBlockedProjects(['P1']); vi.advanceTimersByTime(SHOW_GRACE_MS); expect(listener).not.toHaveBeenCalled(); }); @@ -116,103 +195,22 @@ describe('auto-sync-blocking-store', () => { const listener2 = vi.fn(); subscribeToAutoSyncBlocking(listener1); subscribeToAutoSyncBlocking(listener2); - raiseAutoSyncBlock(); + setBlockedProjects(['P1']); vi.advanceTimersByTime(SHOW_GRACE_MS); expect(listener1).toHaveBeenCalledTimes(1); expect(listener2).toHaveBeenCalledTimes(1); }); }); - describe('safety leash', () => { - it('auto-clears after 10 minutes if the blocker never clears', () => { - const listener = vi.fn(); - subscribeToAutoSyncBlocking(listener); - raiseAutoSyncBlock(); - vi.advanceTimersByTime(SHOW_GRACE_MS); - expect(getAutoSyncBlocking()).toBe(true); - vi.advanceTimersByTime(SAFETY_TIMEOUT_MS); - expect(getAutoSyncBlocking()).toBe(false); - expect(listener).toHaveBeenCalledTimes(2); // shown, then auto-cleared - }); - - it('arms a leash per raise — a later blocker outlives an earlier raise expiring', () => { - raiseAutoSyncBlock(); // blocker A raises — A's leash armed - vi.advanceTimersByTime(SAFETY_TIMEOUT_MS / 2); - raiseAutoSyncBlock(); // blocker B raises — B gets its own 10 min leash - vi.advanceTimersByTime(SAFETY_TIMEOUT_MS / 2); // 10 min from A (its leash fires), 5 min from B - expect(getAutoSyncBlocking()).toBe(true); // B's own leash is still alive - vi.advanceTimersByTime(SAFETY_TIMEOUT_MS / 2); // 10 min from B — B's leash fires - expect(getAutoSyncBlocking()).toBe(false); - }); - - it('releases every expired block — a later single raise shows again', () => { - raiseAutoSyncBlock(); - raiseAutoSyncBlock(); // two blockers in flight - vi.advanceTimersByTime(SAFETY_TIMEOUT_MS); // both leashes expire - expect(getAutoSyncBlocking()).toBe(false); - const clear = raiseAutoSyncBlock(); // a fresh raise blocks again - vi.advanceTimersByTime(SHOW_GRACE_MS); - expect(getAutoSyncBlocking()).toBe(true); - clear(); // its own clear hides again - expect(getAutoSyncBlocking()).toBe(false); - }); - - it('releases only its own block when a stale leash expires — the newer blocker keeps blocking until it clears', () => { - raiseAutoSyncBlock(); // blocker A raises at t=0 and is abandoned - vi.advanceTimersByTime(SAFETY_TIMEOUT_MS / 2); - const clearB = raiseAutoSyncBlock(); // blocker B raises at t=5min - vi.advanceTimersByTime(SAFETY_TIMEOUT_MS / 2); // t=10min — A's leash expires - expect(getAutoSyncBlocking()).toBe(true); // B still in flight — stays blocked - clearB(); - expect(getAutoSyncBlocking()).toBe(false); - }); - - it('a late clear after the leash already fired is a no-op — it cannot release another blocker (review finding 1)', () => { - const clearA = raiseAutoSyncBlock(); // blocker A raises at t=0 - vi.advanceTimersByTime(SAFETY_TIMEOUT_MS - 10_000); - raiseAutoSyncBlock(); // blocker B raises just before A's leash fires - vi.advanceTimersByTime(10_000); // t=10min — A's leash fires, releasing A - expect(getAutoSyncBlocking()).toBe(true); // B still in flight - clearA(); // A's real clear arrives late — must be a no-op, never release B - expect(getAutoSyncBlocking()).toBe(true); - vi.advanceTimersByTime(SAFETY_TIMEOUT_MS); // B's own leash fires - expect(getAutoSyncBlocking()).toBe(false); - }); - - it('an abandoned blocker is released by its own leash despite other blockers clearing (review finding 2)', () => { - raiseAutoSyncBlock(); // blocker X raises at t=0 and is abandoned - // Ordinary syncs keep raising and clearing by their own tokens — they must never cancel X's - // leash, so X is released at t=10min and blocking does not persist indefinitely. - for (let i = 0; i < 24; i += 1) { - const clear = raiseAutoSyncBlock(); - vi.advanceTimersByTime(30_000); - clear(); - vi.advanceTimersByTime(4.5 * 60 * 1000); - } - // Two hours in, between syncs: X's own leash fired at t=10min, so nothing is blocking. - expect(getAutoSyncBlocking()).toBe(false); - }); - - it('cancels the safety leash when blocking clears normally', () => { - const listener = vi.fn(); - subscribeToAutoSyncBlocking(listener); - const clear = raiseAutoSyncBlock(); - vi.advanceTimersByTime(SHOW_GRACE_MS); - clear(); // normal completion - vi.advanceTimersByTime(SAFETY_TIMEOUT_MS); - expect(getAutoSyncBlocking()).toBe(false); - expect(listener).toHaveBeenCalledTimes(2); // shown, then cleared — no extra firing - }); - }); - describe('resetAutoSyncBlocking', () => { - it('clears state and pending timers', () => { + it('clears state and the pending grace timer', () => { const listener = vi.fn(); subscribeToAutoSyncBlocking(listener); - raiseAutoSyncBlock(); + setBlockedProjects(['P1']); resetAutoSyncBlocking(); - vi.advanceTimersByTime(SAFETY_TIMEOUT_MS); // neither grace nor safety should fire - expect(getAutoSyncBlocking()).toBe(false); + vi.advanceTimersByTime(SHOW_GRACE_MS); // the grace should not fire + expect(getBlockedProjectIds().size).toBe(0); + expect(vi.getTimerCount()).toBe(0); expect(listener).not.toHaveBeenCalled(); }); }); diff --git a/src/renderer/services/auto-sync-blocking-store.ts b/src/renderer/services/auto-sync-blocking-store.ts index 19e5efa6727..f179938de9b 100644 --- a/src/renderer/services/auto-sync-blocking-store.ts +++ b/src/renderer/services/auto-sync-blocking-store.ts @@ -1,22 +1,23 @@ /** - * Store tracking whether an automatic (scheduled) Send/Receive is currently blocking the workspace. + * Store tracking which projects an automatic (scheduled or session) Send/Receive is currently + * blocking edits on. * - * Tracks one entry per in-flight blocker so overlapping blockers don't prematurely hide the - * overlay: the visible state (isBlocking) only flips to false once every in-flight blocker has been - * released. + * Backend-authoritative snapshot model: the backend write gate is the single source of truth. It + * emits a full snapshot of the blocked project ids on every arm/disarm, and the producer applies it + * wholesale via {@link setBlockedProjects} — an empty set means nothing is blocked. There is + * deliberately no local ref-counting and no timer-driven opinion about blocking (a second, + * timer-driven opinion is exactly the kind of drift that lets the UI disagree with the backend): + * resilience against a lost event is re-query of the backend authority (the service's init + * consult), not a local safety leash. * - * Each block is identified by the token {@link raiseAutoSyncBlock} returns, and is released exactly - * once — by its own clear (calling the token) or by its own one-shot 10-minute safety leash if it - * never clears (e.g. the extension deactivates mid-sync and the clearing event is never emitted). - * Identity pairing means a clear can never release a different block and a stale leash can never - * wipe newer in-flight blockers. - * - * Visibility has a 200 ms show-grace matching PT9's automatic-sync surface: on the first raise a - * grace timer is armed, and listeners only ever see `true` if blocking is still in flight when it - * fires. A sync that finishes within the grace never shows anything. + * Visibility keeps a 200 ms show-grace matching PT9's automatic-sync surface: on the first project + * becoming blocked (empty → non-empty) a grace timer is armed, and consumers only ever see a + * non-empty visible set if blocking is still in flight when it fires. A sync that finishes within + * the grace never shows anything. Set changes while already visible (e.g. a project joins or leaves + * an in-flight batch) are reflected immediately, with no fresh grace. */ -import { AUTO_SYNC_MAX_DURATION_MS } from '@shared/data/platform.data'; +import { deepEqual } from 'platform-bible-utils'; /** * How long blocking must persist before it becomes visible; a sync finishing inside this window @@ -24,27 +25,15 @@ import { AUTO_SYNC_MAX_DURATION_MS } from '@shared/data/platform.data'; */ const SHOW_GRACE_MS = 200; -/** - * Heuristic upper bound; if a blocker never clears (e.g. extension deactivates mid-sync), - * auto-clear after this long. Each blocker leash bounds a single automatic Send/Receive — exactly - * what {@link AUTO_SYNC_MAX_DURATION_MS} is. - */ -const SAFETY_TIMEOUT_MS = AUTO_SYNC_MAX_DURATION_MS; - -/** One in-flight blocker: its own safety leash. */ -type InFlightBlock = { - leash: ReturnType; -}; +const EMPTY_SET: ReadonlySet = new Set(); +/** The latest backend snapshot: which projects are blocked right now (raw, pre-grace). */ +let rawBlockedProjectIds: ReadonlySet = EMPTY_SET; /** - * Every in-flight (not yet released) blocker. The raw blocking state is exactly "this set is - * non-empty", so there is no separate counter to keep in lockstep. (Same pattern as - * workspace-updating-store; extracting a shared abstraction is deliberately deferred — PT-4214 - * Stage U.) + * The grace-debounced set consumers see. Empty until the show-grace elapses on the first block; + * every public accessor reads this, so all consumers observe the same debounced view. */ -const inFlightBlocks = new Set(); - -let isBlockingVisible = false; +let visibleBlockedProjectIds: ReadonlySet = EMPTY_SET; let graceTimer: ReturnType | undefined; const listeners = new Set<() => void>(); @@ -53,49 +42,76 @@ function notifyListeners(): void { listeners.forEach((listener) => listener()); } -/** Flips the derived visibility, notifying listeners only when it actually changes. */ -function setBlockingVisible(value: boolean): void { - if (isBlockingVisible === value) return; - isBlockingVisible = value; +/** Publishes a new visible set, notifying listeners only when its contents actually change. */ +function setVisible(next: ReadonlySet): void { + if (deepEqual(visibleBlockedProjectIds, next)) return; + visibleBlockedProjectIds = next; notifyListeners(); } -/** Releases one block. A no-op if it was already released (identity — released exactly once). */ -function releaseBlock(block: InFlightBlock): void { - if (!inFlightBlocks.has(block)) return; - inFlightBlocks.delete(block); - clearTimeout(block.leash); - if (inFlightBlocks.size === 0) { - // Cancel a pending grace timer — blocking cleared inside the grace, so nothing ever shows. +/** + * Replaces the set of projects an automatic Send/Receive is blocking edits on. This is the sole + * producer API: the backend emits a full snapshot on every gate arm/disarm and the service forwards + * it here verbatim (an empty array clears blocking entirely). + */ +export function setBlockedProjects(projectIds: ReadonlyArray): void { + // Canonicalize to upper once at ingestion: the backend gate matches ids OrdinalIgnoreCase and the + // canonical project id is upper (ProjectMetadata.Id = id.ToUpperInvariant()), but every consumer + // here (setVisible's equality, isProjectBlocked's Set.has, the driver's isEditorBlocked) is + // case-sensitive. Upper-casing at the single ingestion point keeps the whole store canonical. + const next: ReadonlySet = new Set(projectIds.map((id) => id.toUpperCase())); + + if (next.size === 0) { + // Fully cleared: cancel a pending grace (cleared inside the window → nothing ever showed) and + // drop the visible set immediately. clearTimeout(graceTimer); graceTimer = undefined; - setBlockingVisible(false); + setVisible(EMPTY_SET); + return; } -} -/** - * Registers one in-flight blocker and returns its clear function. Call the returned function when - * the blocker's sync finishes; it is idempotent and releases only this block. If the block never - * clears, its own safety leash releases it after {@link SAFETY_TIMEOUT_MS} — a stale leash can never - * release a newer in-flight blocker. - */ -export function raiseAutoSyncBlock(): () => void { - const block: InFlightBlock = { - leash: setTimeout(() => releaseBlock(block), SAFETY_TIMEOUT_MS), - }; - inFlightBlocks.add(block); - // On the first raise, arm the show grace; visibility only turns on if blocking survives it. - if (inFlightBlocks.size === 1) { + if (visibleBlockedProjectIds.size > 0) { + // Already past the grace and visible: reflect set changes immediately (a project joined or left + // an in-flight batch) with no fresh grace. + setVisible(next); + return; + } + + // Empty → non-empty. Record the latest raw snapshot for the grace-timer callback to read, then arm + // the show-grace if one is not already pending; visibility only turns on if blocking survives the + // grace. This assignment belongs here (below the cleared and already-visible branches) rather than + // at the top of the function: only the grace callback reads rawBlockedProjectIds, so at the top it + // would be a dead store on those earlier branches. Keeping it above the pending-grace check means a + // second empty → non-empty snapshot arriving while a grace is still pending still refreshes what + // that pending timer will read. + rawBlockedProjectIds = next; + if (graceTimer === undefined) { graceTimer = setTimeout(() => { graceTimer = undefined; - if (inFlightBlocks.size > 0) setBlockingVisible(true); + // Re-read the raw set on fire: if the sync cleared during the grace it is empty and nothing + // shows; otherwise publish whatever is blocked now (it may have grown since the grace armed). + if (rawBlockedProjectIds.size > 0) setVisible(rawBlockedProjectIds); }, SHOW_GRACE_MS); } - return () => releaseBlock(block); } -export function getAutoSyncBlocking(): boolean { - return isBlockingVisible; +/** The (visible, grace-debounced) set of projects currently blocked by an automatic Send/Receive. */ +export function getBlockedProjectIds(): ReadonlySet { + return visibleBlockedProjectIds; +} + +/** + * True when a specific project is (visibly) blocked. An undefined project id is never blocked. + * + * Currently unused within core; the project-settings UI (a follow-up) consumes it to disable + * per-project actions while that project's automatic sync is blocking edits. + */ +export function isProjectBlocked(projectId: string | undefined): boolean { + if (projectId === undefined) return false; + // Upper-case to match the store's canonical form (setBlockedProjects canonicalizes to upper at + // ingestion), so a caller passing a non-canonical id can't miss a real block — mirrors the + // driver's isEditorBlocked. + return visibleBlockedProjectIds.has(projectId.toUpperCase()); } /** Subscribe to state changes. Returns an unsubscribe function. */ @@ -112,9 +128,8 @@ export function subscribeToAutoSyncBlocking(listener: () => void): () => void { * WARNING: Test-only. @internal */ export function resetAutoSyncBlocking(): void { - inFlightBlocks.forEach((block) => clearTimeout(block.leash)); - inFlightBlocks.clear(); - isBlockingVisible = false; + rawBlockedProjectIds = EMPTY_SET; + visibleBlockedProjectIds = EMPTY_SET; clearTimeout(graceTimer); graceTimer = undefined; listeners.clear(); diff --git a/src/renderer/services/auto-sync-edit-block-driver.test.ts b/src/renderer/services/auto-sync-edit-block-driver.test.ts index 1c7574b54ae..8e83a94f517 100644 --- a/src/renderer/services/auto-sync-edit-block-driver.test.ts +++ b/src/renderer/services/auto-sync-edit-block-driver.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import type { SavedWebViewDefinition } from '@shared/models/web-view.model'; import { - getAutoSyncBlocking, + getBlockedProjectIds, subscribeToAutoSyncBlocking, } from '@renderer/services/auto-sync-blocking-store'; import { @@ -13,7 +13,7 @@ import { import { initAutoSyncEditBlockDriver } from './auto-sync-edit-block-driver'; vi.mock('@renderer/services/auto-sync-blocking-store', () => ({ - getAutoSyncBlocking: vi.fn(), + getBlockedProjectIds: vi.fn(), subscribeToAutoSyncBlocking: vi.fn(), })); @@ -32,29 +32,35 @@ const EDITOR_TYPE = 'platformScriptureEditor.react'; /** * Tracks every definition object handed out by `makeDefinition`, keyed by id, so the - * `updateWebViewDefinitionSync` mock below can (a) know a written id's `webViewType` when - * synthesizing the object it hands to a live `onDidUpdateWebView` handler, and (b) mutate the + * `updateWebViewDefinitionSync` mock below can (a) know a written id's `webViewType`/`projectId` + * when synthesizing the object it hands to a live `onDidUpdateWebView` handler, and (b) mutate the * definition's `.state` in place so a later `getAllOpenWebViewDefinitionsSync` read (the mock * returns the same object reference every call) reflects the write, matching how the real store - * persists updates. + * persists. */ const definitionsById = new Map(); function makeDefinition( id: string, webViewType: string, + projectId?: string, state?: Record, ): SavedWebViewDefinition { - // The tests only exercise id/webViewType/state; the full SavedWebViewDefinition union has many - // more required members that are irrelevant here, so build a minimal object and assert the type. + // The tests only exercise id/webViewType/projectId/state; the full SavedWebViewDefinition union has + // many more required members that are irrelevant here, so build a minimal object and assert the + // type. // eslint-disable-next-line no-type-assertion/no-type-assertion - const definition = { id, webViewType, state } as SavedWebViewDefinition; + const definition = { id, webViewType, projectId, state } as SavedWebViewDefinition; definitionsById.set(id, definition); return definition; } -function editor(id: string, state?: Record): SavedWebViewDefinition { - return makeDefinition(id, EDITOR_TYPE, state); +function editor( + id: string, + projectId?: string, + state?: Record, +): SavedWebViewDefinition { + return makeDefinition(id, EDITOR_TYPE, projectId, state); } function nonEditor(id: string): SavedWebViewDefinition { @@ -72,13 +78,24 @@ describe('initAutoSyncEditBlockDriver', () => { let updateHandler: ((event: { webView: SavedWebViewDefinition }) => void) | undefined; let updateUnsub: ReturnType; + /** + * Points `getBlockedProjectIds` at a fresh set of the given project ids. Ids are upper-cased to + * model the real store, which canonicalizes to upper at ingestion (`setBlockedProjects`); the + * driver's `isEditorBlocked` upper-cases each editor's `projectId` before matching against this + * set, so the two sides meet in canonical case. + */ + function setBlockedProjects(...projectIds: string[]): void { + vi.mocked(getBlockedProjectIds).mockReturnValue( + new Set(projectIds.map((id) => id.toUpperCase())), + ); + } + /** * Shared body for the mocked `updateWebViewDefinitionSync`: mutates the tracked definition's - * `.state` in place (so a later `getAllOpenWebViewDefinitionsSync` read reflects the write, - * matching how the real store persists updates) and SYNCHRONOUSLY invokes any live - * `onDidUpdateWebView` handler with the updated web view — the buffered emitter and this - * subscription resolve to the same underlying event-emitter instance in production, so a local - * write dispatches inline, not on a later tick. + * `.state` in place (so a later `getAllOpenWebViewDefinitionsSync` read reflects the write) and + * SYNCHRONOUSLY invokes any live `onDidUpdateWebView` handler with the updated web view — the + * buffered emitter and this subscription resolve to the same underlying event-emitter instance in + * production, so a local write dispatches inline, not on a later tick. */ function dispatchUpdate( id: Parameters[0], @@ -87,10 +104,11 @@ describe('initAutoSyncEditBlockDriver', () => { const definition = definitionsById.get(id); if (definition && 'state' in updateInfo) definition.state = updateInfo.state; const webViewType = definition?.webViewType ?? EDITOR_TYPE; + const projectId = definition?.projectId; const state = 'state' in updateInfo ? updateInfo.state : undefined; - // The tests only exercise id/webViewType/state; see the same cast in `makeDefinition` above. + // The tests only exercise id/webViewType/projectId/state; see the same cast in `makeDefinition`. // eslint-disable-next-line no-type-assertion/no-type-assertion - const updatedWebView = { id, webViewType, state } as SavedWebViewDefinition; + const updatedWebView = { id, webViewType, projectId, state } as SavedWebViewDefinition; updateHandler?.({ webView: updatedWebView }); } @@ -136,9 +154,10 @@ describe('initAutoSyncEditBlockDriver', () => { // eslint-disable-next-line no-type-assertion/no-type-assertion, @typescript-eslint/no-explicit-any }) as any, ); - vi.mocked(getAutoSyncBlocking).mockReturnValue(false); + // Default: nothing blocked. + vi.mocked(getBlockedProjectIds).mockReturnValue(new Set()); vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([]); - // This is what lets the regression test below reproduce the live re-flag bug: a still-subscribed + // This is what lets the ordering regression test reproduce the live re-flag bug: a still-live // handler observes the driver's own unflag write before the driver gets a chance to unsubscribe. vi.mocked(updateWebViewDefinitionSync).mockImplementation((id, updateInfo) => { dispatchUpdate(id, updateInfo); @@ -146,41 +165,65 @@ describe('initAutoSyncEditBlockDriver', () => { }); }); - it('does not flag any editor on init when not blocking', () => { - vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([editor('e1'), nonEditor('n1')]); + it('does not flag any editor on init when nothing is blocked', () => { + vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([ + editor('e1', 'projA'), + nonEditor('n1'), + ]); initAutoSyncEditBlockDriver(); expect(updateWebViewDefinitionSync).not.toHaveBeenCalled(); }); - it('sets isSyncBlocked true only on Scripture editors when blocking starts', () => { + it( + 'flags only the editor whose project is syncing, leaving other projects editable ' + + '(one project syncing must not block edits in another)', + () => { + vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([ + editor('e1', 'projA', { viewType: 'formatted' }), // the syncing project + editor('e2', 'projB'), // a different, NOT-syncing project + nonEditor('n1'), + ]); + initAutoSyncEditBlockDriver(); + + setBlockedProjects('projA'); + if (!storeListener) throw new Error('store listener not registered'); + storeListener(); + + expect(updateWebViewDefinitionSync).toHaveBeenCalledWith('e1', { + state: { viewType: 'formatted', isSyncBlocked: true }, + }); + expect(updateWebViewDefinitionSync).not.toHaveBeenCalledWith('e2', expect.anything()); + expect(updateWebViewDefinitionSync).not.toHaveBeenCalledWith('n1', expect.anything()); + expect(updateWebViewDefinitionSync).toHaveBeenCalledTimes(1); + }, + ); + + it('never flags a Scripture editor that has no projectId', () => { vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([ - editor('e1', { viewType: 'formatted' }), - nonEditor('n1'), - editor('e2'), + editor('no-project'), // projectId undefined + editor('e1', 'projA'), ]); initAutoSyncEditBlockDriver(); - vi.mocked(getAutoSyncBlocking).mockReturnValue(true); + setBlockedProjects('projA'); if (!storeListener) throw new Error('store listener not registered'); storeListener(); expect(updateWebViewDefinitionSync).toHaveBeenCalledWith('e1', { - state: { viewType: 'formatted', isSyncBlocked: true }, - }); - expect(updateWebViewDefinitionSync).toHaveBeenCalledWith('e2', { state: { isSyncBlocked: true }, }); - expect(updateWebViewDefinitionSync).not.toHaveBeenCalledWith('n1', expect.anything()); - expect(updateWebViewDefinitionSync).toHaveBeenCalledTimes(2); + expect(updateWebViewDefinitionSync).not.toHaveBeenCalledWith('no-project', expect.anything()); + expect(updateWebViewDefinitionSync).toHaveBeenCalledTimes(1); }); - it('clears isSyncBlocked when blocking ends', () => { + it('clears isSyncBlocked on all editors when blocking ends', () => { vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([ - editor('e1', { isSyncBlocked: true }), + editor('e1', 'projA', { isSyncBlocked: true }), ]); - initAutoSyncEditBlockDriver(); + setBlockedProjects('projA'); + initAutoSyncEditBlockDriver(); // starts already blocking projA — e1 already flagged, no write - vi.mocked(getAutoSyncBlocking).mockReturnValue(false); + setBlockedProjects(); if (!storeListener) throw new Error('store listener not registered'); storeListener(); @@ -189,12 +232,94 @@ describe('initAutoSyncEditBlockDriver', () => { }); }); + it('unflags only the no-longer-syncing project on a partial set shrink {A,B} → {A}', () => { + const writes: { id: string; blocked: boolean }[] = []; + vi.mocked(updateWebViewDefinitionSync).mockImplementation((id, updateInfo) => { + if (updateInfo.state) writes.push({ id, blocked: Boolean(updateInfo.state.isSyncBlocked) }); + dispatchUpdate(id, updateInfo); + return true; + }); + vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([ + editor('a1', 'projA'), + editor('b1', 'projB'), + ]); + initAutoSyncEditBlockDriver(); + if (!storeListener) throw new Error('store listener not registered'); + + // Both projects syncing → both flagged. + setBlockedProjects('projA', 'projB'); + storeListener(); + expect(writes).toEqual([ + { id: 'a1', blocked: true }, + { id: 'b1', blocked: true }, + ]); + + // projB finishes; projA keeps syncing. b1 must unflag; a1 must stay flagged (no write). + writes.length = 0; + setBlockedProjects('projA'); + storeListener(); + expect(writes).toEqual([{ id: 'b1', blocked: false }]); + + // And projA's editor, if rebuilt now, is still re-flagged; projB's is not. + if (!updateHandler) throw new Error('update handler not registered'); + writes.length = 0; + updateHandler({ webView: editor('a1', 'projA', { isSyncBlocked: false }) }); + updateHandler({ webView: editor('b1', 'projB', { isSyncBlocked: false }) }); + expect(writes).toEqual([{ id: 'a1', blocked: true }]); + }); + + it( + 'swaps blocking from one project to another on a pure swap transition {A} → {B}, and does not ' + + "let a stale update from A's editor bounce it back to blocked once B is the (only) live " + + 'handler (regression: a swapped-out project must not bounce back)', + () => { + const writes: { id: string; blocked: boolean }[] = []; + vi.mocked(updateWebViewDefinitionSync).mockImplementation((id, updateInfo) => { + if (updateInfo.state) writes.push({ id, blocked: Boolean(updateInfo.state.isSyncBlocked) }); + dispatchUpdate(id, updateInfo); + return true; + }); + vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([ + editor('a1', 'projA'), + editor('b1', 'projB'), + ]); + initAutoSyncEditBlockDriver(); + if (!storeListener) throw new Error('store listener not registered'); + + // projA syncing → a1 flagged. + setBlockedProjects('projA'); + storeListener(); + expect(writes).toEqual([{ id: 'a1', blocked: true }]); + + // Pure swap: projA finishes and projB starts syncing in the same notification. a1 must unflag + // and b1 must flag, with no bounce-back write for a1 in between — the ordering fix (teardown + // before apply) keeps the stale {A}-scoped handler from observing its own unflag write. + writes.length = 0; + setBlockedProjects('projB'); + storeListener(); + expect(writes).toEqual([ + { id: 'a1', blocked: false }, + { id: 'b1', blocked: true }, + ]); + + // The re-subscribed handler now closes over {B}. A's editor firing a stale onDidUpdateWebView + // (e.g. a reload already in flight when the swap happened) must not re-flag it: A is no longer + // in the blocked set, so isEditorBlocked's early return keeps the handler from acting on it. + if (!updateHandler) throw new Error('update handler not registered'); + writes.length = 0; + updateHandler({ webView: editor('a1', 'projA', { isSyncBlocked: false }) }); + expect(writes).toEqual([]); + + // Sanity: B's editor, if rebuilt mid-block, IS still re-flagged by the new handler. + updateHandler({ webView: editor('b1', 'projB', { isSyncBlocked: false }) }); + expect(writes).toEqual([{ id: 'b1', blocked: true }]); + }, + ); + it( 'does not let a still-live re-flag handler bounce an editor back to blocked when unblocking ' + '(regression: the driver must unsubscribe onDidUpdateWebView before applying the unblock)', () => { - // Every write to e1's isSyncBlocked flag, in order, so we can assert the unflag sticks instead - // of immediately being reverted by the (still-subscribed) re-flag handler. const e1Writes: boolean[] = []; vi.mocked(updateWebViewDefinitionSync).mockImplementation((id, updateInfo) => { if (id === 'e1' && updateInfo.state) e1Writes.push(Boolean(updateInfo.state.isSyncBlocked)); @@ -202,12 +327,12 @@ describe('initAutoSyncEditBlockDriver', () => { return true; }); - vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([editor('e1')]); + vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([editor('e1', 'projA')]); initAutoSyncEditBlockDriver(); if (!storeListener) throw new Error('store listener not registered'); - // Start a scheduled sync: e1 gets flagged blocked, and the re-flag subscription goes live. - vi.mocked(getAutoSyncBlocking).mockReturnValue(true); + // Start a scheduled sync of projA: e1 gets flagged, and the re-flag subscription goes live. + setBlockedProjects('projA'); storeListener(); expect(onDidUpdateWebView).toHaveBeenCalledTimes(1); @@ -215,7 +340,7 @@ describe('initAutoSyncEditBlockDriver', () => { // writing `isSyncBlocked: false`, or the still-live handler observes its own unflag write, its // "came back unblocked" guard passes, and it re-flags e1 straight back to `true` — the live // E2E bug (every scripture editor permanently read-only after a scheduled sync finishes). - vi.mocked(getAutoSyncBlocking).mockReturnValue(false); + setBlockedProjects(); storeListener(); expect(e1Writes).toEqual([true, false]); @@ -225,70 +350,79 @@ describe('initAutoSyncEditBlockDriver', () => { ); it('does not re-write an editor already in the desired state', () => { - // Editor already flagged and a sync already in flight at init: nothing to change. + // Editor already flagged and its project already syncing at init: nothing to change. vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([ - editor('e1', { isSyncBlocked: true }), + editor('e1', 'projA', { isSyncBlocked: true }), ]); - vi.mocked(getAutoSyncBlocking).mockReturnValue(true); + setBlockedProjects('projA'); initAutoSyncEditBlockDriver(); expect(updateWebViewDefinitionSync).not.toHaveBeenCalled(); }); - it('flags editors opened mid-block and stops after blocking ends', () => { + it('flags editors opened mid-block only for a blocked project, and stops after blocking ends', () => { initAutoSyncEditBlockDriver(); if (!storeListener) throw new Error('store listener not registered'); - // Start blocking → subscribes to onDidOpenWebView - vi.mocked(getAutoSyncBlocking).mockReturnValue(true); + // Start blocking projA → subscribes to onDidOpenWebView. + setBlockedProjects('projA'); storeListener(); expect(onDidOpenWebView).toHaveBeenCalledTimes(1); - - // An editor opened mid-block gets flagged if (!openHandler) throw new Error('open handler not registered'); - openHandler({ webView: editor('opened-mid-block') }); - expect(updateWebViewDefinitionSync).toHaveBeenCalledWith('opened-mid-block', { + + // An editor of the blocked project opened mid-block gets flagged. + openHandler({ webView: editor('opened-blocked', 'projA') }); + expect(updateWebViewDefinitionSync).toHaveBeenCalledWith('opened-blocked', { state: { isSyncBlocked: true }, }); - // A non-editor opened mid-block is ignored + // An editor of a DIFFERENT project opened mid-block is not flagged. vi.mocked(updateWebViewDefinitionSync).mockClear(); + openHandler({ webView: editor('opened-other', 'projB') }); + expect(updateWebViewDefinitionSync).not.toHaveBeenCalled(); + + // A non-editor opened mid-block is ignored. openHandler({ webView: nonEditor('other') }); expect(updateWebViewDefinitionSync).not.toHaveBeenCalled(); - // Blocking ends → unsubscribes from onDidOpenWebView - vi.mocked(getAutoSyncBlocking).mockReturnValue(false); + // Blocking ends → unsubscribes from onDidOpenWebView. + setBlockedProjects(); storeListener(); expect(openUnsub).toHaveBeenCalledTimes(1); }); - it('only subscribes to onDidOpenWebView once across overlapping blocking notifications', () => { + it('does not re-subscribe onDidOpenWebView across overlapping identical notifications', () => { initAutoSyncEditBlockDriver(); if (!storeListener) throw new Error('store listener not registered'); - vi.mocked(getAutoSyncBlocking).mockReturnValue(true); - storeListener(); + setBlockedProjects('projA'); storeListener(); + storeListener(); // same blocked set → no-op expect(onDidOpenWebView).toHaveBeenCalledTimes(1); }); - it('re-flags a Scripture editor rebuilt (updated) mid-block that came back unblocked', () => { + it('re-flags a Scripture editor of a blocked project rebuilt (updated) mid-block that came back unblocked', () => { initAutoSyncEditBlockDriver(); if (!storeListener) throw new Error('store listener not registered'); - // Start blocking → subscribes to onDidUpdateWebView - vi.mocked(getAutoSyncBlocking).mockReturnValue(true); + setBlockedProjects('projA'); storeListener(); expect(onDidUpdateWebView).toHaveBeenCalledTimes(1); - - // A rebuilt editor comes back with isSyncBlocked forced to false → re-flagged to true if (!updateHandler) throw new Error('update handler not registered'); - updateHandler({ webView: editor('rebuilt', { viewType: 'formatted', isSyncBlocked: false }) }); + + // A rebuilt editor of projA comes back with isSyncBlocked forced to false → re-flagged to true. + updateHandler({ + webView: editor('rebuilt', 'projA', { viewType: 'formatted', isSyncBlocked: false }), + }); expect(updateWebViewDefinitionSync).toHaveBeenCalledWith('rebuilt', { state: { viewType: 'formatted', isSyncBlocked: true }, }); - // A non-editor update mid-block is ignored + // A rebuilt editor of a NON-blocked project is not re-flagged. vi.mocked(updateWebViewDefinitionSync).mockClear(); + updateHandler({ webView: editor('rebuilt-other', 'projB', { isSyncBlocked: false }) }); + expect(updateWebViewDefinitionSync).not.toHaveBeenCalled(); + + // A non-editor update mid-block is ignored. updateHandler({ webView: nonEditor('other') }); expect(updateWebViewDefinitionSync).not.toHaveBeenCalled(); }); @@ -296,18 +430,18 @@ describe('initAutoSyncEditBlockDriver', () => { it('does not re-flag an already-blocked editor update (the driver does not loop on its own update)', () => { initAutoSyncEditBlockDriver(); if (!storeListener) throw new Error('store listener not registered'); - vi.mocked(getAutoSyncBlocking).mockReturnValue(true); + setBlockedProjects('projA'); storeListener(); if (!updateHandler) throw new Error('update handler not registered'); // Model the real service: the driver's own re-flag write re-emits onDidUpdateWebView with the // now-blocked definition. If the handler acted on that it would recurse forever. vi.mocked(updateWebViewDefinitionSync).mockImplementation((id) => { - updateHandler?.({ webView: editor(id, { isSyncBlocked: true }) }); + updateHandler?.({ webView: editor(id, 'projA', { isSyncBlocked: true }) }); return true; }); - updateHandler({ webView: editor('rebuilt', { isSyncBlocked: false }) }); + updateHandler({ webView: editor('rebuilt', 'projA', { isSyncBlocked: false }) }); // Exactly one write: the original re-flag. The re-emitted (already-blocked) update is a no-op. expect(updateWebViewDefinitionSync).toHaveBeenCalledTimes(1); @@ -316,10 +450,10 @@ describe('initAutoSyncEditBlockDriver', () => { }); }); - it('only subscribes to onDidUpdateWebView once across overlapping blocking notifications', () => { + it('does not re-subscribe onDidUpdateWebView across overlapping identical notifications', () => { initAutoSyncEditBlockDriver(); if (!storeListener) throw new Error('store listener not registered'); - vi.mocked(getAutoSyncBlocking).mockReturnValue(true); + setBlockedProjects('projA'); storeListener(); storeListener(); expect(onDidUpdateWebView).toHaveBeenCalledTimes(1); @@ -329,19 +463,36 @@ describe('initAutoSyncEditBlockDriver', () => { initAutoSyncEditBlockDriver(); if (!storeListener) throw new Error('store listener not registered'); - vi.mocked(getAutoSyncBlocking).mockReturnValue(true); + setBlockedProjects('projA'); storeListener(); expect(onDidUpdateWebView).toHaveBeenCalledTimes(1); - vi.mocked(getAutoSyncBlocking).mockReturnValue(false); + setBlockedProjects(); storeListener(); expect(updateUnsub).toHaveBeenCalledTimes(1); }); + it('re-subscribes the handlers over the new set on a set change (so partial transitions track it)', () => { + initAutoSyncEditBlockDriver(); + if (!storeListener) throw new Error('store listener not registered'); + + setBlockedProjects('projA'); + storeListener(); + setBlockedProjects('projA', 'projB'); // real content change → tear down + re-arm + storeListener(); + + // One subscribe per distinct blocking snapshot. + expect(onDidOpenWebView).toHaveBeenCalledTimes(2); + expect(onDidUpdateWebView).toHaveBeenCalledTimes(2); + // The stale handler was torn down before the re-arm. + expect(openUnsub).toHaveBeenCalledTimes(1); + expect(updateUnsub).toHaveBeenCalledTimes(1); + }); + it('cleanup unsubscribes from the store and the open/update web-view subscriptions', () => { const cleanup = initAutoSyncEditBlockDriver(); if (!storeListener) throw new Error('store listener not registered'); - vi.mocked(getAutoSyncBlocking).mockReturnValue(true); + setBlockedProjects('projA'); storeListener(); cleanup(); @@ -354,7 +505,29 @@ describe('initAutoSyncEditBlockDriver', () => { vi.mocked(getAllOpenWebViewDefinitionsSync).mockImplementation(() => { throw new Error('dock layout not registered'); }); + setBlockedProjects('projA'); // force enumeration (which throws) on init expect(() => initAutoSyncEditBlockDriver()).not.toThrow(); expect(updateWebViewDefinitionSync).not.toHaveBeenCalled(); }); + + it('does not record a set it failed to apply, so an identical later notification retries', () => { + // Enumeration throws on the first apply, so nothing is flagged and the applied-set snapshot must + // NOT advance to the blocked set — otherwise the top-of-syncState equality guard would + // short-circuit the identical next notification and leave the editor unflagged for the block. + vi.mocked(getAllOpenWebViewDefinitionsSync).mockImplementationOnce(() => { + throw new Error('dock layout not registered'); + }); + setBlockedProjects('projA'); + initAutoSyncEditBlockDriver(); + if (!storeListener) throw new Error('store listener not registered'); + expect(updateWebViewDefinitionSync).not.toHaveBeenCalled(); + + // Enumeration now succeeds and the SAME blocked set is re-notified. Because the failed apply + // left the snapshot unadvanced, the guard does not short-circuit and the editor is flagged. + vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([editor('e1', 'projA')]); + storeListener(); + expect(updateWebViewDefinitionSync).toHaveBeenCalledWith('e1', { + state: { isSyncBlocked: true }, + }); + }); }); diff --git a/src/renderer/services/auto-sync-edit-block-driver.ts b/src/renderer/services/auto-sync-edit-block-driver.ts index 8d6b46c8e3c..b6ace1a0acb 100644 --- a/src/renderer/services/auto-sync-edit-block-driver.ts +++ b/src/renderer/services/auto-sync-edit-block-driver.ts @@ -1,22 +1,25 @@ /** - * Headless driver that translates the auto-sync-blocking store's visible state into a per-editor - * `isSyncBlocked` flag on every open Scripture editor web view. + * Headless driver that translates the auto-sync-blocking store's visible blocked-project set into a + * per-editor `isSyncBlocked` flag on the open Scripture editor web views whose project is syncing. * * This replaces the earlier full-workspace blocking overlay: instead of covering the whole - * workspace and trapping focus, an automatic (scheduled) Send/Receive now blocks only _editing_. - * The Scripture editor web view reads `isSyncBlocked` from its own web view state (via - * `useWebViewState`), folds it into its read-only computation, and shows a slim non-covering banner - * — the rest of the UI (menus, dialogs, navigation) stays fully usable. + * workspace and trapping focus, an automatic (scheduled or session) Send/Receive now blocks only + * _editing_, and only of the projects actually being synced. The Scripture editor web view reads + * `isSyncBlocked` from its own web view state (via `useWebViewState`), folds it into its read-only + * computation, and shows a slim non-covering banner — the rest of the UI (menus, dialogs, + * navigation) and every editor of a project that is NOT syncing stays fully usable. * - * The store (see auto-sync-blocking-store.ts) already provides the 200 ms show-grace, the - * ref-counted overlapping-blocker handling, and the safety timeout, so this driver just mirrors the - * store's derived visibility onto the editors and needs none of that logic itself. + * An editor is flagged iff its `projectId` is in the store's blocked set; editors with no + * `projectId` are never flagged. The driver reacts to SET CHANGES (backend snapshots), not a + * boolean raise/clear: on each store notification it re-applies the current blocked set to every + * open editor. The store (see auto-sync-blocking-store.ts) provides the 200 ms show-grace, so this + * driver just mirrors the store's derived visible set onto the editors. * - * While blocking is active it also flags editors opened mid-block (via `onDidOpenWebView`), so an - * editor a user opens during a sync is blocked too, and re-flags editors that are rebuilt mid-block - * (via `onDidUpdateWebView`) — the Scripture editor factory forces `isSyncBlocked: false` on every - * rebuild (e.g. `reloadWebView`, an interface-mode switch, or `loadLayout`), so without this a - * rebuild during a sustained block would come back editable with the banner gone. + * While a set is blocking it also flags editors opened mid-block (via `onDidOpenWebView`) and + * re-flags editors rebuilt mid-block (via `onDidUpdateWebView`) whose project is in the blocked set + * — the Scripture editor factory forces `isSyncBlocked: false` on every rebuild (e.g. + * `reloadWebView`, an interface-mode switch, or `loadLayout`), so without this a rebuild during a + * sustained block would come back editable with the banner gone. */ import { logger } from '@shared/services/logger.service'; @@ -24,9 +27,9 @@ import { SavedWebViewDefinition, SCRIPTURE_EDITOR_WEBVIEW_TYPE, } from '@shared/models/web-view.model'; -import { getErrorMessage } from 'platform-bible-utils'; +import { deepEqual, getErrorMessage } from 'platform-bible-utils'; import { - getAutoSyncBlocking, + getBlockedProjectIds, subscribeToAutoSyncBlocking, } from '@renderer/services/auto-sync-blocking-store'; import { @@ -39,6 +42,19 @@ import { /** Web view state key the Scripture editor reads to know it is edit-blocked by an automatic sync. */ const IS_SYNC_BLOCKED_STATE_KEY = 'isSyncBlocked'; +/** True when a Scripture editor definition's project is in the blocked set. No project → never. */ +function isEditorBlocked( + definition: SavedWebViewDefinition, + blockedProjectIds: ReadonlySet, +): boolean { + // Upper-case to match the store's canonical form (setBlockedProjects canonicalizes to upper at + // ingestion; canonical project ids are upper — ProjectMetadata.Id = id.ToUpperInvariant()), so a + // casing skew between the editor's projectId and the blocked set can never miss a real block. + return ( + definition.projectId !== undefined && blockedProjectIds.has(definition.projectId.toUpperCase()) + ); +} + /** * Sets `isSyncBlocked` on a single Scripture editor's saved definition, but only when it differs * from the current value — `updateWebViewDefinitionSync` always emits an update event when `state` @@ -61,8 +77,17 @@ function setEditorSyncBlocked(definition: SavedWebViewDefinition, isBlocked: boo } } -/** Applies `isBlocked` to every currently open Scripture editor web view. */ -function applyToAllEditors(isBlocked: boolean): void { +/** + * Applies the blocked set to every currently open Scripture editor: each editor whose project is in + * the set is flagged, every other editor is unflagged. The per-editor equality guard turns this + * into a diff — unchanged editors get no write (and thus emit no update event). + * + * Returns `true` when the enumeration succeeded and the set was applied, `false` when + * `getAllOpenWebViewDefinitionsSync` threw and nothing was applied. The caller uses this to avoid + * recording a set it did not actually apply (see `syncState`): an open-but-unenumerable editor must + * not be treated as flagged, or the equality guard would short-circuit and leave it unflagged. + */ +function applyBlockedSetToAllEditors(blockedProjectIds: ReadonlySet): boolean { let definitions: SavedWebViewDefinition[]; try { definitions = getAllOpenWebViewDefinitionsSync(); @@ -72,96 +97,109 @@ function applyToAllEditors(isBlocked: boolean): void { logger.debug( `auto-sync edit-block driver could not enumerate web views: ${getErrorMessage(e)}`, ); - return; + return false; } definitions.forEach((definition) => { if (definition.webViewType === SCRIPTURE_EDITOR_WEBVIEW_TYPE) - setEditorSyncBlocked(definition, isBlocked); + setEditorSyncBlocked(definition, isEditorBlocked(definition, blockedProjectIds)); }); + return true; } /** - * Starts the driver: mirrors the auto-sync-blocking store's visible state onto every open Scripture - * editor's `isSyncBlocked` state, and — while blocking — onto editors opened or rebuilt mid-block. - * Call once at app startup. Returns a cleanup function that stops the driver (it does NOT clear any - * flags it set; the store clearing to `false` is what unblocks the editors). + * Starts the driver: mirrors the auto-sync-blocking store's visible blocked-project set onto every + * open Scripture editor's `isSyncBlocked` state, and — while a set is blocking — onto editors + * opened or rebuilt mid-block whose project is in the set. Call once at app startup. Returns a + * cleanup function that stops the driver (it does NOT clear any flags it set; the store clearing to + * empty is what unblocks the editors). */ export function initAutoSyncEditBlockDriver(): () => void { let unsubscribeOpen: (() => void) | undefined; let unsubscribeUpdate: (() => void) | undefined; + /** The blocked set we last applied to the editors; lets us skip a no-op notification. */ + let appliedBlockedProjectIds: ReadonlySet = new Set(); + + const teardownHandlers = () => { + if (unsubscribeOpen) { + unsubscribeOpen(); + unsubscribeOpen = undefined; + } + if (unsubscribeUpdate) { + unsubscribeUpdate(); + unsubscribeUpdate = undefined; + } + }; + + // Subscribe the mid-block handlers, each closing over the CURRENT blocked set. Because syncState + // tears the handlers down and rebuilds them on every real transition, the closures always see the + // latest set — which is what makes partial transitions ({A,B} → {A}) correct. + const setupHandlers = (blockedProjectIds: ReadonlySet) => { + const openUnsub = onDidOpenWebView(({ webView }) => { + if ( + webView.webViewType === SCRIPTURE_EDITOR_WEBVIEW_TYPE && + isEditorBlocked(webView, blockedProjectIds) + ) + setEditorSyncBlocked(webView, true); + }); + // Wrapped because a network-event unsubscriber returns a boolean; keep our own type void. + unsubscribeOpen = () => { + openUnsub(); + }; + + const updateUnsub = onDidUpdateWebView(({ webView }) => { + if (webView.webViewType !== SCRIPTURE_EDITOR_WEBVIEW_TYPE) return; + // Only re-flag editors whose project is blocked; a rebuilt editor of a non-synced project must + // stay editable. + if (!isEditorBlocked(webView, blockedProjectIds)) return; + // Guard against self-triggering: our own re-flag below calls updateWebViewDefinitionSync, which + // fires another onDidUpdateWebView. Only act when the definition came back unblocked; once we + // set it back to true the next event is a no-op, so this cannot loop. + if (webView.state?.[IS_SYNC_BLOCKED_STATE_KEY]) return; + setEditorSyncBlocked(webView, true); + }); + unsubscribeUpdate = () => { + updateUnsub(); + }; + }; const syncState = () => { - const isBlocking = getAutoSyncBlocking(); + const next = getBlockedProjectIds(); + // No content change → nothing to do. This keeps overlapping identical notifications from + // needlessly rewriting editors or re-subscribing the handlers. + if (deepEqual(appliedBlockedProjectIds, next)) return; - if (!isBlocking) { - // Unsubscribe BEFORE applying the unblock below. `setEditorSyncBlocked`'s unflag write goes - // through `updateWebViewDefinitionSync`, which fires `onDidUpdateWebView` SYNCHRONOUSLY — the - // web-view service host's buffered emitter and this subscription resolve to the same - // underlying PapiNetworkEventEmitter instance, so a local emit dispatches inline, not on a - // later tick. If the re-flag handler below were still subscribed when `applyToAllEditors( - // false)` runs, it would observe the just-written `isSyncBlocked: false`, pass its "came back - // unblocked" guard, and set the editor straight back to `true` — permanently blocking every - // open Scripture editor, with no recovery (the store's safety timer's value-unchanged - // early-return means it never renotifies, and the banner's Cancel becomes inert). Found live - // in E2E, 2026-07-16. - if (unsubscribeOpen) { - unsubscribeOpen(); - unsubscribeOpen = undefined; - } - if (unsubscribeUpdate) { - unsubscribeUpdate(); - unsubscribeUpdate = undefined; - } - } + // ORDERING IS LOAD-BEARING: tear the mid-block handlers down BEFORE applying the diff below. + // Applying the diff issues unflag writes (isSyncBlocked: false) for projects no longer in the + // blocked set, and `updateWebViewDefinitionSync` fires `onDidUpdateWebView` SYNCHRONOUSLY (the + // buffered emitter and this subscription resolve to the same underlying PapiNetworkEventEmitter, + // so a local emit dispatches inline, not on a later tick). A still-live re-flag handler would + // observe its own unflag write, pass its "came back unblocked" guard, and set the editor straight + // back to `true` — permanently blocking it. Tearing down first also drops handlers that closed + // over the STALE set, so the rebuild below re-subscribes over the new one. This is correct for + // PARTIAL transitions too: on {A,B} → {A}, B's editors unflag with no live handler to bounce + // them, A's editors keep their flag (the equality guard skips them), and the rebuilt {A} handlers + // still protect A. + teardownHandlers(); - applyToAllEditors(isBlocking); + // Only advance the applied-set snapshot when the apply actually succeeded. If enumeration threw + // (open-but-unenumerable editors), applyBlockedSetToAllEditors returns false having applied + // nothing; recording `next` anyway would let the equality guard above short-circuit the next + // notification and leave those editors unflagged for the rest of this block. Leaving the snapshot + // unchanged lets the next real transition (or the open/update handlers) re-apply. + if (applyBlockedSetToAllEditors(next)) + // Snapshot a copy so a later store reassignment can never alias what we think we applied. + appliedBlockedProjectIds = new Set(next); - if (isBlocking) { - // Block editors opened while a sync is in flight. Subscribe once; the store can notify - // multiple times during one blocking episode (overlapping blockers) without re-subscribing. - if (!unsubscribeOpen) { - const unsubscribe = onDidOpenWebView(({ webView }) => { - if (webView.webViewType === SCRIPTURE_EDITOR_WEBVIEW_TYPE) - setEditorSyncBlocked(webView, true); - }); - // Wrapped because a network-event unsubscriber returns a boolean; keep our own type void. - unsubscribeOpen = () => { - unsubscribe(); - }; - } - // Re-flag editors rebuilt mid-block. The Scripture editor factory forces `isSyncBlocked: - // false` on every in-place rebuild (reloadWebView / interface-mode switch / loadLayout), which - // emits `onDidUpdateWebView` (not `onDidOpenWebView`), so without this the editor comes back - // editable with no banner for the rest of the sync. - if (!unsubscribeUpdate) { - const unsubscribe = onDidUpdateWebView(({ webView }) => { - if (webView.webViewType !== SCRIPTURE_EDITOR_WEBVIEW_TYPE) return; - // Guard against self-triggering: our own re-flag below calls updateWebViewDefinitionSync, - // which fires another onDidUpdateWebView. Only act when the definition came back unblocked; - // once we set it back to true the next event is a no-op, so this cannot loop. - if (webView.state?.[IS_SYNC_BLOCKED_STATE_KEY]) return; - setEditorSyncBlocked(webView, true); - }); - unsubscribeUpdate = () => { - unsubscribe(); - }; - } - } + // Re-arm the mid-block handlers over the new set, unless nothing is blocked anymore. + if (next.size > 0) setupHandlers(next); }; - // Reflect the current state immediately (in case blocking is already active on init), then track. + // Reflect the current state immediately (in case a set is already blocking on init), then track. syncState(); const unsubscribeStore = subscribeToAutoSyncBlocking(syncState); return () => { unsubscribeStore(); - if (unsubscribeOpen) { - unsubscribeOpen(); - unsubscribeOpen = undefined; - } - if (unsubscribeUpdate) { - unsubscribeUpdate(); - unsubscribeUpdate = undefined; - } + teardownHandlers(); }; } diff --git a/src/renderer/services/workspace-updating-store.ts b/src/renderer/services/workspace-updating-store.ts index ed1bd7755ca..b2a9041b808 100644 --- a/src/renderer/services/workspace-updating-store.ts +++ b/src/renderer/services/workspace-updating-store.ts @@ -25,10 +25,11 @@ type InFlightSwitch = { }; /** - * Every in-flight (not yet released) switch. `getWorkspaceUpdating()` is exactly "this set is - * non-empty", so there is no separate counter to keep in lockstep. (Same pattern as - * auto-sync-blocking-store; extracting a shared abstraction is deliberately deferred — PT-4214 - * Stage U.) + * Every in-flight (not yet released) switch. This store is a ref-count model: each concurrent + * switch adds one identity-keyed entry and removes it on release, and `getWorkspaceUpdating()` is + * exactly "this set is non-empty", so there is no separate counter to keep in lockstep. Each entry + * carries its own safety leash (see {@link SWITCH_SAFETY_TIMEOUT_MS}) that releases it if its finish + * never arrives. */ const inFlightSwitches = new Set(); diff --git a/src/shared/data/platform.data.ts b/src/shared/data/platform.data.ts index 52d7e9d743e..16a320e6c36 100644 --- a/src/shared/data/platform.data.ts +++ b/src/shared/data/platform.data.ts @@ -54,14 +54,11 @@ export const MAX_ZOOM_FACTOR = 3.0; * (which has its own progress and Cancel). A sync of a large repo can run for minutes, so this is * deliberately long. * - * Two different consumers share this value and must never diverge: - * - * - The main process (`shutdown-tasks.ts`) bounds how long app shutdown waits on its final sync. - * - The renderer (`auto-sync-blocking-store.ts`) bounds each edit-block safety leash — how long a - * single blocker may block editing if its clearing event never arrives. - * - * If they diverged, a shutdown sync could outlive the renderer's opinion of it (or vice versa); - * tune this value knowing it retimes both. + * Consumed by the main process (`shutdown-tasks.ts`), which uses it to bound how long app shutdown + * waits on its final sync. It also conceptually matches the C# write gate's stall watchdog, which + * bounds the same "one automatic Send/Receive" window. The renderer does not time blocking locally + * — it reads the backend write gate's snapshot (`auto-sync-blocking-store.ts`), so blocking clears + * when the backend says so rather than on a renderer-side timer. * * @experimental */