From a651f81ce152bfba6e4a65545a0d2f95258ec256 Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Thu, 16 Jul 2026 22:57:35 +0200 Subject: [PATCH 01/14] feat(send-receive): make the write gate reject per-project + emit a block-state change event (PT-4214 Stage U) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverse SendReceiveWriteLock's rejection from a process-wide global gate to a per-project one: while a set of projects is syncing, EnterWrite now rejects only writes to THOSE projects — a write to any other project proceeds. 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 EnterWrite and IsBlocked now consult _blockedProjectIds; the core mutual-exclusion invariant is unaffected (narrowing rejection can only allow more writes, and the count++/arm ordering still bars any write to a synced project from racing that project's file replacement). The read of the blocked set is safe against tearing because SetSyncing publishes it data-then-flag, so a reader that observes the armed bit always observes the matching set. Add a backend-authoritative change-notification surface on the gate: - public event Action? BlockStateChanged, raised after a successful arm (with the armed ids) and after any real disarm (empty); no-op Clear()/stale-token Clear(long) do not raise. Subscriber faults are swallowed (this pure class has no logger; the notifier owns real error handling). - public GetBlockState() returning a best-effort snapshot from the armed flag + set. - readonly record struct SendReceiveBlockState(IsBlocking, ProjectIds) — serializes to the exact { isBlocking, projectIds } wire shape via the shared camelCase PAPI JSON options. SetSyncing/Clear/Clear(long)/EnterWrite signatures are unchanged (the in-flight studio patch PR #164 calls them). Drain semantics stay GLOBAL. The class doc is rewritten to describe the per-project reversal and the two benign flag/set-skew windows, both of which resolve to "allow". ResetForTests now also clears BlockStateChanged subscribers so tests can't leak subscriptions. Tests: replace the global-gate test with per-project tests (unsynced project allowed with a working scope that participates in a later drain; synced projects rejected); fix the nesting-hazard test to use the same project (per-project filter no longer rejects a different one); add event tests (arm/re-arm/disarm raise, stale and no-op Clear do not, throwing subscriber can't break arm/clear) and GetBlockState snapshot tests. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA --- .../Projects/SendReceiveWriteLockTests.cs | 280 ++++++++++++++++-- .../SendReceive/SendReceiveWriteLock.cs | 256 ++++++++++++---- 2 files changed, 469 insertions(+), 67 deletions(-) diff --git a/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs b/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs index 235c6e8e1be..ee523374624 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). This is a deliberate reversal of the old + // global gate. 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)); } @@ -713,6 +785,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/Projects/SendReceive/SendReceiveWriteLock.cs b/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs index 9fdd1e1953f..0fe233c2dc4 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,30 @@ 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 +/// atomically incrementing the count on the same word arming sets the flag on, so every scope 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). 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. +/// 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 now 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 +85,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 +121,47 @@ 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). It +/// is a reversal of this gate's original "one global sync slot rejects ALL writes" behavior; both +/// and now consult _blockedProjectIds, and +/// the safety invariant is unaffected (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,23 @@ 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. + /// + /// + 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 @@ -208,10 +269,53 @@ 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 an empty set regardless of the pure-data field (Clear() empties the + // set data-then-flag, so it can briefly still hold ids while already disarmed). + return new SendReceiveBlockState( + isBlocking, + isBlocking ? _blockedProjectIds.ToArray() : [] + ); + } + + // 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 " @@ -292,6 +396,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 +471,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(new SendReceiveBlockState(false, [])); } /// @@ -391,6 +505,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(new SendReceiveBlockState(false, [])); return; } } @@ -400,8 +517,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 now 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 +532,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(); @@ -485,3 +619,25 @@ public void Dispose() } } } + +/// +/// 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 +); From d8e12717576321b6879432a551097a3a9c4cbd97 Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Thu, 16 Jul 2026 22:57:59 +0200 Subject: [PATCH 02/14] feat(send-receive): add SendReceiveBlockNotifierService bridging the write gate to the PAPI (PT-4214 Stage U) Introduce a startup service that forwards SendReceiveWriteLock's block-state transitions to the renderer so the UI has a backend-authoritative view of whether an automatic Send/Receive is blocking edits, and for which projects: - subscribes to SendReceiveWriteLock.BlockStateChanged and fire-and-forgets a paratextBibleSendReceive.onSyncWriteLockChanged PAPI event (try/catch + log, mirroring SharedStore.Set's event-send handling). - registers command:paratextBibleSendReceive.getAutoSyncBlocking returning the current GetBlockState() snapshot so a renderer can seed on demand. Both carry the same { isBlocking, projectIds } wire shape. Wire the service into Program.cs's startup Task.WhenAll alongside the other PAPI services. Inert in open-source Platform.Bible: nothing arms the gate there, so the event never fires and the command always returns not-blocking. Because PapiClient has no network:registerEvent counterpart, the event is an unregistered announcement following the existing SharedStore STORE_CHANGE_EVENT precedent (TODO PT-4214 follow-up when C# gains registerEvent). Tests: SendReceiveBlockNotifierServiceTests verifies the command is registered, a gate arm/clear pushes the event with the right name + snapshot payload, the command returns the current snapshot, and SendReceiveBlockState serializes to the camelCase wire shape. Add a DummyPapiClient.InvokeRequestHandler test helper to invoke a locally-registered handler without a live PAPI connection. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA --- c-sharp-tests/DummyPapiClient.cs | 14 ++ .../SendReceiveBlockNotifierServiceTests.cs | 139 ++++++++++++++++++ c-sharp/Program.cs | 7 +- .../SendReceiveBlockNotifierService.cs | 88 +++++++++++ 4 files changed, 247 insertions(+), 1 deletion(-) create mode 100644 c-sharp-tests/Projects/SendReceive/SendReceiveBlockNotifierServiceTests.cs create mode 100644 c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs diff --git a/c-sharp-tests/DummyPapiClient.cs b/c-sharp-tests/DummyPapiClient.cs index ba47d602cca..cd9357e623f 100644 --- a/c-sharp-tests/DummyPapiClient.cs +++ b/c-sharp-tests/DummyPapiClient.cs @@ -119,6 +119,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..bf10eb267df --- /dev/null +++ b/c-sharp-tests/Projects/SendReceive/SendReceiveBlockNotifierServiceTests.cs @@ -0,0 +1,139 @@ +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 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/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..98b64aa9d71 --- /dev/null +++ b/c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs @@ -0,0 +1,88 @@ +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). +/// +/// +/// Why the event is an "unregistered announcement". exposes +/// network:registerMethod (via ) but has +/// no network:registerEvent counterpart, so C# cannot formally register an event type. We +/// therefore emit this event with without a prior +/// registration, following the existing precedent of SharedStore's shared-store:change +/// event. The main process logs at most a deprecation warning for such an event today; when C# gains +/// a registerEvent API this should register the event properly (TODO PT-4214 follow-up). +/// +/// +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"; + + private PapiClient PapiClient { get; } = papiClient; + + public async Task InitializeAsync() + { + // Forward every gate transition to the renderer. 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 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 do not await the send and we swallow + log any + /// failure (mirrors SharedStore.Set's event-send error handling). + /// + private void OnBlockStateChanged(SendReceiveBlockState state) + { + try + { + _ = PapiClient.SendEventAsync(BlockStateChangedEvent, state); + } + catch (Exception ex) + { + Console.WriteLine($"Error sending {BlockStateChangedEvent} event: {ex}"); + } + } +} From 0f7c516b6065e745b20b6da4945c37920047f3c4 Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Thu, 16 Jul 2026 23:15:55 +0200 Subject: [PATCH 03/14] refactor(renderer): make auto-sync-blocking store a per-project snapshot (PT-4214 Stage U) Replace the ref-counted boolean model with a backend-authoritative SNAPSHOT model: the store now holds the set of project ids an automatic Send/Receive is blocking edits on, replaced wholesale by the producer via the new setBlockedProjects(). Empty set = not blocking. Add per-project accessors getBlockedProjectIds() and isProjectBlocked() (undefined project id -> false) alongside the preserved getAutoSyncBlocking() (any project blocked) and subscribeToAutoSyncBlocking(). The 200 ms show-grace debounce for the derived visible flag is kept verbatim (PT9-parity UX), and listeners now notify on any visible-set content change (so a project joining or leaving an in-flight batch is observable), not just a boolean flip. Delete the per-blocker safety-leash timer array and all AUTO_SYNC_MAX_DURATION_MS usage in the store. This is the ratified PT-4214 5.2 decision: the renderer's SAFETY_TIMEOUT_MS timer is deleted, not retained -- a second, timer-driven opinion about blocking is precisely the drift findings 7/8/16 indict. Renderer resilience against a lost event is now re-query of the backend authority (the service's init consult), not a local timer. These leashes were added by Stage T on this very base branch (7a103c79a7c); superseding them here is deliberate. AUTO_SYNC_MAX_DURATION_MS is kept in platform.data.ts (shutdown-tasks.ts still uses it); its doc comment is updated to record that the renderer-store consumer is gone per Stage U and the constant's remaining rightful homes are the shutdown-sync bound and, conceptually, the C# stall watchdog. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA --- .../services/auto-sync-blocking-store.test.ts | 213 ++++++++---------- .../services/auto-sync-blocking-store.ts | 139 ++++++------ src/shared/data/platform.data.ts | 13 +- 3 files changed, 176 insertions(+), 189 deletions(-) diff --git a/src/renderer/services/auto-sync-blocking-store.test.ts b/src/renderer/services/auto-sync-blocking-store.test.ts index 6334fc92de9..6464a8252ee 100644 --- a/src/renderer/services/auto-sync-blocking-store.test.ts +++ b/src/renderer/services/auto-sync-blocking-store.test.ts @@ -1,18 +1,18 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { - raiseAutoSyncBlock, + setBlockedProjects, getAutoSyncBlocking, + 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,30 +22,40 @@ describe('auto-sync-blocking-store', () => { vi.useRealTimers(); }); - describe('getAutoSyncBlocking', () => { - it('returns false initially', () => { + describe('initial state', () => { + it('reports nothing blocked', () => { expect(getAutoSyncBlocking()).toBe(false); + 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(); + setBlockedProjects(['p1']); expect(getAutoSyncBlocking()).toBe(false); + 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(listener).not.toHaveBeenCalled(); // nothing ever showed, so nothing ever notified @@ -54,51 +64,103 @@ describe('auto-sync-blocking-store', () => { 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(getAutoSyncBlocking()).toBe(false); // 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('snapshot replace semantics', () => { + it('replaces the blocked set wholesale', () => { + setBlockedProjects(['p1', 'p2']); 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(['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('clear is idempotent — clearing twice cannot release another blocker', () => { - const clearA = raiseAutoSyncBlock(); + it('clears blocking when replaced with an empty set', () => { + 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); + setBlockedProjects([]); + expect(getAutoSyncBlocking()).toBe(false); + expect(getBlockedProjectIds().size).toBe(0); }); - it('does not notify listeners when visibility is unchanged (nested raises)', () => { - raiseAutoSyncBlock(); + 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 (safety leash deleted, PT-4214 Stage U)', () => { + 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(getAutoSyncBlocking()).toBe(true); + // Far beyond the old 10-minute leash — the block persists until the backend clears it. + vi.advanceTimersByTime(60 * 60 * 1000); + expect(getAutoSyncBlocking()).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 +168,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 +178,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 + vi.advanceTimersByTime(SHOW_GRACE_MS); // the grace should not fire expect(getAutoSyncBlocking()).toBe(false); + 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..695aef09c83 100644 --- a/src/renderer/services/auto-sync-blocking-store.ts +++ b/src/renderer/services/auto-sync-blocking-store.ts @@ -1,50 +1,38 @@ /** - * 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 (PT-4214 Stage U): 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 no local ref-counting and no timer-driven opinion about blocking: resilience + * against a lost event is re-query of the backend authority (the service's init consult), not a + * local safety leash. The renderer's old per-blocker `SAFETY_TIMEOUT_MS` leash is deleted, not + * retained — a second, timer-driven opinion about blocking is precisely the drift the design's + * findings 7/8/16 indict. * - * 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'; - /** * How long blocking must persist before it becomes visible; a sync finishing inside this window * shows nothing (PT9 parity). */ 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 +41,71 @@ 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; +/** Content equality for two id sets (the store always replaces sets wholesale, never mutates). */ +function areSetsEqual(a: ReadonlySet, b: ReadonlySet): boolean { + if (a === b) return true; + if (a.size !== b.size) return false; + return [...a].every((id) => b.has(id)); +} + +/** Publishes a new visible set, notifying listeners only when its contents actually change. */ +function setVisible(next: ReadonlySet): void { + if (areSetsEqual(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 { + const next: ReadonlySet = new Set(projectIds); + rawBlockedProjectIds = next; + + 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. Arm the show-grace if one is not already pending; visibility only turns on + // if blocking survives the grace. + 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); } +/** True when any project is (visibly) blocked. Preserved for existing boolean consumers. */ export function getAutoSyncBlocking(): boolean { - return isBlockingVisible; + return visibleBlockedProjectIds.size > 0; +} + +/** 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. */ +export function isProjectBlocked(projectId: string | undefined): boolean { + if (projectId === undefined) return false; + return visibleBlockedProjectIds.has(projectId); } /** Subscribe to state changes. Returns an unsubscribe function. */ @@ -112,9 +122,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/shared/data/platform.data.ts b/src/shared/data/platform.data.ts index 52d7e9d743e..076badf11ab 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. + * As of PT-4214 Stage U the renderer auto-sync-blocking store no longer consumes this: the backend + * write gate is the single authority for blocking, so the store's own `SAFETY_TIMEOUT_MS` leash was + * deleted (renderer-side resilience is re-query of the authority, not a local timer). The constant + * now survives in two rightful homes — the shutdown-sync bound in main (`shutdown-tasks.ts`) and, + * conceptually, the C# stall watchdog — both of which bound "one automatic Send/Receive". * * @experimental */ From 32b081ad08551eebf8e623e1f720e2a2c3df71fe Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Thu, 16 Jul 2026 23:16:11 +0200 Subject: [PATCH 04/14] feat(renderer): drive auto-sync blocking from the backend write-gate event (PT-4214 Stage U) Subscribe the auto-sync-blocking service to the C# backend's paratextBibleSendReceive.onSyncWriteLockChanged event -- a full { isBlocking, projectIds } snapshot emitted on every gate arm/disarm for ALL sync types (manual + scheduled + session) -- and forward it to the store's new setBlockedProjects(isBlocking ? projectIds : []). Remove the subscription to paratextBibleSendReceive.onAutoSyncBlockingChanged entirely: the backend gate is now the single signal source (PT-4214 finding 16); a second renderer-side signal is exactly the drift that finding indicts. A short comment names the replaced event and why. Parse the snapshot defensively: a malformed or missing-field payload is treated as block-none and warns once per service lifetime (fail-safe assume-unblocked, consistent with the existing init-seeding philosophy -- a broken signal must never leave editors stuck read-only). Keep the init consult of paratextBibleSendReceive.getAutoSyncBlocking (requestNoRetry; it is C#-served now) but parse the NEW snapshot shape, keep the hasReceivedEvent live-event-wins race guard, and keep the fail-safe assume-unblocked on rejection (plain Platform.Bible serves the command but returns not-blocking; older cores lack it and the request rejects). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA --- .../auto-sync-blocking-service.test.ts | 206 +++++++++++++----- .../services/auto-sync-blocking-service.ts | 134 +++++++++--- 2 files changed, 252 insertions(+), 88 deletions(-) diff --git a/src/renderer/services/auto-sync-blocking-service.test.ts b/src/renderer/services/auto-sync-blocking-service.test.ts index 0eb1afd3109..5b4756effa7 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 OLD_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 replaced onAutoSyncBlockingChanged event', () => { + initAutoSyncBlockingService(); + expect(subscribedEventNames).not.toContain(OLD_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..117a03c33ca 100644 --- a/src/renderer/services/auto-sync-blocking-service.ts +++ b/src/renderer/services/auto-sync-blocking-service.ts @@ -1,43 +1,111 @@ -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 } 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). It is the SINGLE signal source for blocking +// (PT-4214 finding 16). 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'; + +// REPLACED (PT-4214 Stage U): the extension-emitted, renderer-side boolean raise/clear event +// `paratextBibleSendReceive.onAutoSyncBlockingChanged`. It is no longer subscribed — a second signal +// source alongside the backend gate is exactly the drift finding 16 indicts, so the backend gate is +// now the single authority for blocking. /** - * Subscribes to the auto-sync blocking network event and drives the auto-sync-blocking store. Call + * 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 now 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 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((id): id is string => typeof id === 'string'); + 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; + (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(); + }; } From 19bd137b0c70c1b002de4c64abbc8d910cd69f4b Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Thu, 16 Jul 2026 23:16:28 +0200 Subject: [PATCH 05/14] feat(renderer): edit-block only the syncing projects' editors (PT-4214 Stage U) Make the auto-sync edit-block driver per-project. A Scripture editor web view is flagged isSyncBlocked iff its definition.projectId is in the store's blocked set; editors with no projectId, or whose project is not syncing, are never flagged. This is the fix for E2E defect 3: syncing project A must not block edits in an open editor of project B. The driver now reacts to SET CHANGES (backend snapshots) rather than a boolean raise/clear. On each store notification it diffs the new blocked set against the last-applied one (no-op when unchanged) and re-applies desired state to every open editor; the per-editor write-equality guard turns the re-apply into a minimal diff (flag newly-blocked projects, unflag no-longer-blocked ones). Preserve the two hard-won behaviors, generalized to per-project: - mid-block subscriptions: onDidOpenWebView flags a newly-opened editor whose project is blocked; onDidUpdateWebView re-flags a rebuilt editor of a blocked project (the editor factory forces isSyncBlocked:false on rebuild). - THE ORDERING FIX: on any transition, tear the re-flag/open handlers down BEFORE applying the diff, because updateWebViewDefinitionSync fires onDidUpdateWebView synchronously and a live re-flag handler would observe its own unflag write and re-flag permanently. Chosen shape: unsubscribe handlers, apply the full diff, then resubscribe over the new set if it is non-empty. This is correct for PARTIAL transitions too ({A,B} -> {A}): B's editors unflag with no live handler to bounce them, A's editors keep their flag, and the rebuilt {A} handlers still protect A on a later rebuild. Adapt the regression tests to per-project semantics (projectId-carrying editors, getBlockedProjectIds set mock): defect-3 two-project isolation, partial shrink {A,B}->{A}, own-unflag-must-not-re-flag incl. partial transitions, rebuilt re-flag only for the blocked project, no-projectId editor never flagged. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA --- .../auto-sync-edit-block-driver.test.ts | 241 ++++++++++++------ .../services/auto-sync-edit-block-driver.ts | 201 ++++++++------- 2 files changed, 284 insertions(+), 158 deletions(-) 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..dfabd755ab2 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,17 @@ describe('initAutoSyncEditBlockDriver', () => { let updateHandler: ((event: { webView: SavedWebViewDefinition }) => void) | undefined; let updateUnsub: ReturnType; + /** Points `getBlockedProjectIds` at a fresh set of the given project ids. */ + function setBlockedProjects(...projectIds: string[]): void { + vi.mocked(getBlockedProjectIds).mockReturnValue(new Set(projectIds)); + } + /** * 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 +97,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 +147,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 +158,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 ' + + '(regression: E2E defect 3 — 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 +225,46 @@ 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( '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 +272,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 +285,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 +295,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 +375,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 +395,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 +408,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,6 +450,7 @@ 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(); }); diff --git a/src/renderer/services/auto-sync-edit-block-driver.ts b/src/renderer/services/auto-sync-edit-block-driver.ts index 8d6b46c8e3c..8953455b78f 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'; @@ -26,7 +29,7 @@ import { } from '@shared/models/web-view.model'; import { getErrorMessage } from 'platform-bible-utils'; import { - getAutoSyncBlocking, + getBlockedProjectIds, subscribeToAutoSyncBlocking, } from '@renderer/services/auto-sync-blocking-store'; import { @@ -39,6 +42,21 @@ 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'; +/** Content equality for two id sets (both come from the store, which replaces sets wholesale). */ +function areSetsEqual(a: ReadonlySet, b: ReadonlySet): boolean { + if (a === b) return true; + if (a.size !== b.size) return false; + return [...a].every((id) => b.has(id)); +} + +/** True when a Scripture editor definition's project is in the blocked set. No project → never. */ +function isEditorBlocked( + definition: SavedWebViewDefinition, + blockedProjectIds: ReadonlySet, +): boolean { + return definition.projectId !== undefined && blockedProjectIds.has(definition.projectId); +} + /** * 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 +79,12 @@ 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). + */ +function applyBlockedSetToAllEditors(blockedProjectIds: ReadonlySet): void { let definitions: SavedWebViewDefinition[]; try { definitions = getAllOpenWebViewDefinitionsSync(); @@ -76,92 +98,99 @@ function applyToAllEditors(isBlocked: boolean): void { } definitions.forEach((definition) => { if (definition.webViewType === SCRIPTURE_EDITOR_WEBVIEW_TYPE) - setEditorSyncBlocked(definition, isBlocked); + setEditorSyncBlocked(definition, isEditorBlocked(definition, blockedProjectIds)); }); } /** - * 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 syncState = () => { - const isBlocking = getAutoSyncBlocking(); - - 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; - } + const teardownHandlers = () => { + if (unsubscribeOpen) { + unsubscribeOpen(); + unsubscribeOpen = undefined; } - - applyToAllEditors(isBlocking); - - 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(); - }; - } + if (unsubscribeUpdate) { + unsubscribeUpdate(); + unsubscribeUpdate = undefined; } }; - // Reflect the current state immediately (in case blocking is already active on init), then track. + // 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 next = getBlockedProjectIds(); + // No content change → nothing to do. This keeps overlapping identical notifications from + // needlessly rewriting editors or re-subscribing the handlers. + if (areSetsEqual(next, appliedBlockedProjectIds)) return; + + // THE ORDERING FIX. Tear the mid-block handlers down BEFORE applying the diff below. Applying the + // diff issues unflag writes (isSyncBlocked: false) for no-longer-blocked projects, 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. Found live in E2E, 2026-07-16. + teardownHandlers(); + + applyBlockedSetToAllEditors(next); + // Snapshot a copy so a later store reassignment can never alias what we think we applied. + appliedBlockedProjectIds = new Set(next); + + // 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 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(); }; } From 08c370fcfb72bed92117c60e38aa4188011ec1f8 Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Thu, 16 Jul 2026 23:30:01 +0200 Subject: [PATCH 06/14] =?UTF-8?q?fix(send-receive):=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20truthful=20armed-empty-set=20warning,=20per-project?= =?UTF-8?q?=20docs,=20observed=20notifier=20faults=20(PT-4214)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of the per-project write-gate found 5 low-severity doc/behavior-description issues: - SetSyncing's all-invalid-batch comment/warning claimed an armed empty set "rejects writes exactly like any other arm". Under the per-project EnterWrite check (armed && set.Contains(id)) that's false: an empty set matches nothing, so such an arm blocks NOTHING. Rewrote the comment/warning to say so, and pinned it with a new NUnit test (SetSyncing_AllInvalidBatch_ArmsButBlocksNoProject). - SetSyncing's XML doc still described the old global-gate behavior ("EnterWrite calls for ANY project fail fast", "arms the global gate"); reworded to per-project language consistent with the class remarks/EnterWrite/IsBlocked docs. - Added an honest paragraph to the BlockStateChanged doc: raises happen outside the atomic transition and carry no sequence stamp, so the two documented off-contract races (any-thread crash-recovery Clear(), or a takeover racing a stale Clear(token)) can deliver events out of order; self-corrects at the next transition. No sequence stamp added (YAGNI under the serialized-scheduler contract). - SendReceiveBlockNotifierService's try/catch around the discarded `_ = PapiClient.SendEventAsync(...)` task was dead: SendEventAsync is async, so no exception from it can ever throw synchronously, only land unobserved on the discarded Task. Switched to ThreadingUtils.RunTask (the existing repo pattern for observing a fire-and-forget task's fault) so a failed send is actually logged, and documented the deliberate divergence from SharedStore.Set's same-shaped (and equally dead) precedent. - Repaired a garbled sentence in the class doc's "one atomic word" paragraph. Also checked the getNetworkEvent deprecation flagged in auto-sync-blocking-service.ts:80 — the base branch (7a103c79a7c) already used the same deprecated explicit-type-parameter overload for the same reason (the event isn't declared in the public NetworkEvents map), so left it unchanged to match surrounding code. dotnet test c-sharp-tests --filter FullyQualifiedName~SendReceive: 66 passed, 0 failed. csharpier --check clean on touched files. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA --- .../Projects/SendReceiveWriteLockTests.cs | 39 +++++++++ .../SendReceiveBlockNotifierService.cs | 32 ++++--- .../SendReceive/SendReceiveWriteLock.cs | 86 ++++++++++++------- 3 files changed, 115 insertions(+), 42 deletions(-) diff --git a/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs b/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs index ee523374624..008be7a6846 100644 --- a/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs +++ b/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs @@ -383,6 +383,45 @@ 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. This is the + // opposite of the old global-gate behavior, where an armed-with-empty-set gate rejected + // every write. + 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] diff --git a/c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs b/c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs index 98b64aa9d71..c9ad185c726 100644 --- a/c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs +++ b/c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs @@ -71,18 +71,24 @@ await PapiClient.RegisterRequestHandlerAsync( /// /// 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 do not await the send and we swallow + log any - /// failure (mirrors SharedStore.Set's event-send error handling). + /// 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) - { - try - { - _ = PapiClient.SendEventAsync(BlockStateChangedEvent, state); - } - catch (Exception ex) - { - Console.WriteLine($"Error sending {BlockStateChangedEvent} event: {ex}"); - } - } + private void OnBlockStateChanged(SendReceiveBlockState state) => + ThreadingUtils.RunTask( + PapiClient.SendEventAsync(BlockStateChangedEvent, state), + $"send {BlockStateChangedEvent} event" + ); } diff --git a/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs b/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs index 0fe233c2dc4..8fc256f948b 100644 --- a/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs +++ b/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs @@ -38,23 +38,24 @@ 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 incrementing the count on the same word arming sets the flag on, so every scope 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 +/// 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 now consult it), but it never /// weakens the invariant — narrowing rejection can only let MORE writes through, and the @@ -202,6 +203,25 @@ internal static class SendReceiveWriteLock /// 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; @@ -325,15 +345,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 @@ -368,14 +392,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 From d8542b4c4bcd10bf77a83f03f592ab3c341a04e2 Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Thu, 16 Jul 2026 23:36:08 +0200 Subject: [PATCH 07/14] refactor(renderer): drop dead boolean blocking getter, pin swap-transition driver test (PT-4214) getAutoSyncBlocking() had no consumers outside its own test (the papi command of the same name is served independently by the C# backend); isProjectBlocked is what the store's real consumers use, plus the upcoming project-settings UI. Also pins the {A}->{B} pure-swap transition with a driver test, confirming a stale onDidUpdateWebView from the swapped-out project's editor cannot bounce it back to blocked. --- .../services/auto-sync-blocking-store.test.ts | 17 +++---- .../services/auto-sync-blocking-store.ts | 12 ++--- .../auto-sync-edit-block-driver.test.ts | 48 +++++++++++++++++++ 3 files changed, 60 insertions(+), 17 deletions(-) diff --git a/src/renderer/services/auto-sync-blocking-store.test.ts b/src/renderer/services/auto-sync-blocking-store.test.ts index 6464a8252ee..26fbfe6eb7f 100644 --- a/src/renderer/services/auto-sync-blocking-store.test.ts +++ b/src/renderer/services/auto-sync-blocking-store.test.ts @@ -1,7 +1,6 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { setBlockedProjects, - getAutoSyncBlocking, getBlockedProjectIds, isProjectBlocked, subscribeToAutoSyncBlocking, @@ -24,7 +23,6 @@ describe('auto-sync-blocking-store', () => { describe('initial state', () => { it('reports nothing blocked', () => { - expect(getAutoSyncBlocking()).toBe(false); expect(getBlockedProjectIds().size).toBe(0); expect(isProjectBlocked('p1')).toBe(false); }); @@ -37,7 +35,6 @@ describe('auto-sync-blocking-store', () => { describe('show grace', () => { it('is not visible immediately when blocking starts', () => { setBlockedProjects(['p1']); - expect(getAutoSyncBlocking()).toBe(false); expect(getBlockedProjectIds().size).toBe(0); expect(isProjectBlocked('p1')).toBe(false); }); @@ -45,7 +42,6 @@ describe('auto-sync-blocking-store', () => { it('becomes visible once the 200 ms grace elapses', () => { setBlockedProjects(['p1']); vi.advanceTimersByTime(SHOW_GRACE_MS); - expect(getAutoSyncBlocking()).toBe(true); expect(isProjectBlocked('p1')).toBe(true); expect([...getBlockedProjectIds()]).toEqual(['p1']); }); @@ -57,7 +53,7 @@ describe('auto-sync-blocking-store', () => { vi.advanceTimersByTime(150); // still inside the grace window 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 }); @@ -75,7 +71,7 @@ describe('auto-sync-blocking-store', () => { setBlockedProjects(['p1']); vi.advanceTimersByTime(100); // inside the grace setBlockedProjects(['p1', 'p2']); // a second project joins the in-flight batch - expect(getAutoSyncBlocking()).toBe(false); // still inside the grace + expect(getBlockedProjectIds().size).toBe(0); // still inside the grace vi.advanceTimersByTime(SHOW_GRACE_MS); expect([...getBlockedProjectIds()].sort()).toEqual(['p1', 'p2']); }); @@ -104,9 +100,8 @@ describe('auto-sync-blocking-store', () => { it('clears blocking when replaced with an empty set', () => { setBlockedProjects(['p1']); vi.advanceTimersByTime(SHOW_GRACE_MS); - expect(getAutoSyncBlocking()).toBe(true); + expect(isProjectBlocked('p1')).toBe(true); setBlockedProjects([]); - expect(getAutoSyncBlocking()).toBe(false); expect(getBlockedProjectIds().size).toBe(0); }); @@ -140,10 +135,10 @@ describe('auto-sync-blocking-store', () => { it('never auto-clears a long-running block (no safety timeout)', () => { setBlockedProjects(['p1']); vi.advanceTimersByTime(SHOW_GRACE_MS); - expect(getAutoSyncBlocking()).toBe(true); + expect(isProjectBlocked('p1')).toBe(true); // Far beyond the old 10-minute leash — the block persists until the backend clears it. vi.advanceTimersByTime(60 * 60 * 1000); - expect(getAutoSyncBlocking()).toBe(true); + expect(isProjectBlocked('p1')).toBe(true); expect(vi.getTimerCount()).toBe(0); }); @@ -192,7 +187,7 @@ describe('auto-sync-blocking-store', () => { setBlockedProjects(['p1']); resetAutoSyncBlocking(); vi.advanceTimersByTime(SHOW_GRACE_MS); // the grace should not fire - expect(getAutoSyncBlocking()).toBe(false); + 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 695aef09c83..fb83c9fa040 100644 --- a/src/renderer/services/auto-sync-blocking-store.ts +++ b/src/renderer/services/auto-sync-blocking-store.ts @@ -92,17 +92,17 @@ export function setBlockedProjects(projectIds: ReadonlyArray): void { } } -/** True when any project is (visibly) blocked. Preserved for existing boolean consumers. */ -export function getAutoSyncBlocking(): boolean { - return visibleBlockedProjectIds.size > 0; -} - /** 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. */ +/** + * True when a specific project is (visibly) blocked. An undefined project id is never blocked. + * + * No consumer in this diff — the project-settings UI (a stacked follow-up PR) is about to consume + * 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; return visibleBlockedProjectIds.has(projectId); 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 dfabd755ab2..18de8bcb5df 100644 --- a/src/renderer/services/auto-sync-edit-block-driver.test.ts +++ b/src/renderer/services/auto-sync-edit-block-driver.test.ts @@ -261,6 +261,54 @@ describe('initAutoSyncEditBlockDriver', () => { 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)', From 2ca99a3259a64b21e45ded8b02e5008094e207c7 Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Fri, 17 Jul 2026 11:52:22 +0200 Subject: [PATCH 08/14] feat(c-sharp): register onSyncWriteLockChanged with the central event registry (PT-4214) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The block-state event was announced unregistered, drawing main's boot-time deprecation warning like every C#-origin event. No new API is needed, though: network:registerEvent is an ordinary main-process JSON-RPC method served to every websocket client, and PapiClient's generic SendRequestAsync can call it generically — the exact way RegisterRequestHandlerAsync already calls network:registerMethod. InitializeAsync now registers the event before subscribing to the gate, making the C# connection the event's single registered source, so announcements pass the registry check cleanly. Best-effort: rejection or failure is logged and emitting continues, because announcing unregistered still works (main warns once) and backend startup must never break over a registry hiccup. DummyPapiClient now captures outgoing wire requests (and accepts network:registerEvent, mirroring ConnectAsync's "pretend we succeeded") so a test pins the registration. Other C#-origin events (-pdp-data:onDidUpdate, shared-store:change) stay on the legacy announce path pending the platform-wide migration. Co-Authored-By: Claude Fable 5 --- c-sharp-tests/DummyPapiClient.cs | 32 ++++++++++++ .../SendReceiveBlockNotifierServiceTests.cs | 20 ++++++++ .../SendReceiveBlockNotifierService.cs | 49 ++++++++++++++++--- 3 files changed, 94 insertions(+), 7 deletions(-) diff --git a/c-sharp-tests/DummyPapiClient.cs b/c-sharp-tests/DummyPapiClient.cs index cd9357e623f..a76d58fab09 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 @@ -106,9 +111,36 @@ 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; pretend main + // accepted (mirrors ConnectAsync's "pretend we succeeded") so services under test don't + // take their 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)true); 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 diff --git a/c-sharp-tests/Projects/SendReceive/SendReceiveBlockNotifierServiceTests.cs b/c-sharp-tests/Projects/SendReceive/SendReceiveBlockNotifierServiceTests.cs index bf10eb267df..f6736150498 100644 --- a/c-sharp-tests/Projects/SendReceive/SendReceiveBlockNotifierServiceTests.cs +++ b/c-sharp-tests/Projects/SendReceive/SendReceiveBlockNotifierServiceTests.cs @@ -47,6 +47,26 @@ public void InitializeAsync_RegistersTheGetAutoSyncBlockingCommand() ); } + [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 void GateArm_PushesOnSyncWriteLockChangedEventWithBlockingSnapshot() { diff --git a/c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs b/c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs index c9ad185c726..69243872f4a 100644 --- a/c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs +++ b/c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs @@ -27,13 +27,19 @@ namespace Paranext.DataProvider.Projects.SendReceive; /// to report until the Paratext 10 Studio patch brackets a sync with the gate (PT-4210). /// /// -/// Why the event is an "unregistered announcement". exposes -/// network:registerMethod (via ) but has -/// no network:registerEvent counterpart, so C# cannot formally register an event type. We -/// therefore emit this event with without a prior -/// registration, following the existing precedent of SharedStore's shared-store:change -/// event. The main process logs at most a deprecation warning for such an event today; when C# gains -/// a registerEvent API this should register the event properly (TODO PT-4214 follow-up). +/// 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 — before subscribing to the gate, 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. +/// Registration is best-effort: 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) @@ -50,10 +56,39 @@ internal class SendReceiveBlockNotifierService(PapiClient papiClient) 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() { + // Register the event with main's central event registry FIRST, so no announcement can + // precede the registration (see the class doc: best-effort — on rejection or failure, log + // and continue; emitting unregistered still works, and startup must never break over this). + 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}" + ); + } + // Forward every gate transition to the renderer. 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. From 952a090247741fbd6d29694187fd0689e93b1d47 Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Fri, 17 Jul 2026 12:51:21 +0200 Subject: [PATCH 09/14] fix(renderer): reconcile #2571's review fixes with the snapshot model Rebase reconciliation on top of #2571's identity-pairing fix round: drop the anonymous-boolean integration test whose event seam (onAutoSyncBlockingChanged + raise/clear pairing) this branch deletes - the snapshot store makes the pairing problem it pinned unrepresentable - and update the index.tsx startup comment, which #2571 made accurate again by cutting the inert seeding: this branch reintroduces the (now C#-served) init consult, so the services are no longer both synchronous (review finding 13 on #2571). Co-Authored-By: Claude Fable 5 --- lib/papi-dts/papi.d.ts | 13 ++- src/renderer/index.tsx | 10 ++- .../auto-sync-blocking-integration.test.ts | 85 ------------------- 3 files changed, 11 insertions(+), 97 deletions(-) delete mode 100644 src/renderer/services/auto-sync-blocking-integration.test.ts diff --git a/lib/papi-dts/papi.d.ts b/lib/papi-dts/papi.d.ts index af07a7582af..2e9575ad21f 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. + * As of PT-4214 Stage U the renderer auto-sync-blocking store no longer consumes this: the backend + * write gate is the single authority for blocking, so the store's own `SAFETY_TIMEOUT_MS` leash was + * deleted (renderer-side resilience is re-query of the authority, not a local timer). The constant + * now survives in two rightful homes — the shutdown-sync bound in main (`shutdown-tasks.ts`) and, + * conceptually, the C# stall watchdog — both of which bound "one automatic Send/Receive". * * @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); - }); -}); From 61fcd63b3c14e9853786af6a944a68a22d2d7617 Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Fri, 17 Jul 2026 15:42:38 +0200 Subject: [PATCH 10/14] test(send-receive): cover notifier registration-failure branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SendReceiveBlockNotifierService's InitializeAsync registers the onSyncWriteLockChanged event with main's central registry best-effort: on rejection (accepted == false) it warns and continues, and on a thrown registration it catches, logs, and continues — in both cases still registering the getAutoSyncBlocking command and still emitting gate transitions. DummyPapiClient hard-coded network:registerEvent -> true, so neither branch was exercised; the contract was asserted only by inspection. Make the dummy's register response configurable via a RegisterEventResponse Func (default unchanged: returns true), and add two tests pinning the best-effort contract: (a) registry rejects (false) -> warning logged, command still registered, gate transition still emitted; (b) registration throws -> caught and logged (with the exception detail), command still registered, transition still emitted. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe --- c-sharp-tests/DummyPapiClient.cs | 21 +++- .../SendReceiveBlockNotifierServiceTests.cs | 107 ++++++++++++++++++ 2 files changed, 123 insertions(+), 5 deletions(-) diff --git a/c-sharp-tests/DummyPapiClient.cs b/c-sharp-tests/DummyPapiClient.cs index a76d58fab09..2a51e98298c 100644 --- a/c-sharp-tests/DummyPapiClient.cs +++ b/c-sharp-tests/DummyPapiClient.cs @@ -30,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() @@ -112,13 +122,14 @@ 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; pretend main - // accepted (mirrors ConnectAsync's "pretend we succeeded") so services under test don't - // take their registration-failure paths. Kept strictly to this one request type so no - // other test's SendRequestAsync expectations change. + // 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)true); + return Task.FromResult((T)(object)RegisterEventResponse()); return Task.FromResult(default); } diff --git a/c-sharp-tests/Projects/SendReceive/SendReceiveBlockNotifierServiceTests.cs b/c-sharp-tests/Projects/SendReceive/SendReceiveBlockNotifierServiceTests.cs index f6736150498..b18ef6db6c9 100644 --- a/c-sharp-tests/Projects/SendReceive/SendReceiveBlockNotifierServiceTests.cs +++ b/c-sharp-tests/Projects/SendReceive/SendReceiveBlockNotifierServiceTests.cs @@ -67,6 +67,113 @@ public void InitializeAsync_RegistersTheEventWithTheCentralRegistry() }); } + [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() { From 6fc2197d1fbc0145ca5e2ae12b63db30436e2cd3 Mon Sep 17 00:00:00 2001 From: timothy-mccormack Date: Wed, 22 Jul 2026 01:13:35 +0800 Subject: [PATCH 11/14] fix(auto-sync): address lyonsil's #2574 review round (PT-4214 Stage U) - Canonicalize project ids to upper at ingestion + in both readers (isEditorBlocked, isProjectBlocked) so a casing skew can't leave a project silently unblocked - applyBlockedSetToAllEditors reports success; syncState only advances the applied snapshot on a successful apply (open-but-unenumerable editors no longer recorded as flagged) - Subscribe to the gate before the event-registration round-trip so no transition is dropped - Move SendReceiveBlockState to its own file (PNX004); add a shared static NotBlocking snapshot; drop the redundant ToArray copy in GetBlockState - Reuse deepEqual/isString from platform-bible-utils; scope the dead rawBlockedProjectIds writes to the arming branch - Add a pointer comment noting the init consult is the only backend re-seed (lost-disarm recovery tracked on PT-4214) - Fix the now-stale workspace-updating-store cross-reference comment Co-Authored-By: Claude Opus 4.8 (1M context) --- .../SendReceiveBlockNotifierService.cs | 36 ++++--- .../SendReceive/SendReceiveBlockState.cs | 23 +++++ .../SendReceive/SendReceiveWriteLock.cs | 45 +++------ .../services/auto-sync-blocking-service.ts | 11 ++- .../services/auto-sync-blocking-store.test.ts | 94 ++++++++++++------- .../services/auto-sync-blocking-store.ts | 32 ++++--- .../auto-sync-edit-block-driver.test.ts | 32 ++++++- .../services/auto-sync-edit-block-driver.ts | 39 +++++--- .../services/workspace-updating-store.ts | 9 +- 9 files changed, 205 insertions(+), 116 deletions(-) create mode 100644 c-sharp/Projects/SendReceive/SendReceiveBlockState.cs diff --git a/c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs b/c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs index 69243872f4a..d99e66041af 100644 --- a/c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs +++ b/c-sharp/Projects/SendReceive/SendReceiveBlockNotifierService.cs @@ -33,13 +33,16 @@ namespace Paranext.DataProvider.Projects.SendReceive; /// 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 — before subscribing to the gate, 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 +/// 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. -/// Registration is best-effort: 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. +/// 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) @@ -66,9 +69,19 @@ internal class SendReceiveBlockNotifierService(PapiClient papiClient) public async Task InitializeAsync() { - // Register the event with main's central event registry FIRST, so no announcement can - // precede the registration (see the class doc: best-effort — on rejection or failure, log - // and continue; emitting unregistered still works, and startup must never break over this). + // 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( @@ -89,11 +102,6 @@ public async Task InitializeAsync() ); } - // Forward every gate transition to the renderer. 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 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. 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 8fc256f948b..17cab3af083 100644 --- a/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs +++ b/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs @@ -257,6 +257,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 @@ -311,12 +317,13 @@ internal static void ResetForTests(long generation = 0) public static SendReceiveBlockState GetBlockState() { bool isBlocking = (Volatile.Read(ref _state) & ArmedFlag) != 0; - // When disarmed, report an empty set regardless of the pure-data field (Clear() empties the - // set data-then-flag, so it can briefly still hold ids while already disarmed). - return new SendReceiveBlockState( - isBlocking, - isBlocking ? _blockedProjectIds.ToArray() : [] - ); + // 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 @@ -504,7 +511,7 @@ public static void Clear() // 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(new SendReceiveBlockState(false, [])); + RaiseBlockStateChanged(NotBlocking); } /// @@ -535,7 +542,7 @@ public static void Clear(long token) _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(new SendReceiveBlockState(false, [])); + RaiseBlockStateChanged(NotBlocking); return; } } @@ -647,25 +654,3 @@ public void Dispose() } } } - -/// -/// 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/src/renderer/services/auto-sync-blocking-service.ts b/src/renderer/services/auto-sync-blocking-service.ts index 117a03c33ca..354616abfc2 100644 --- a/src/renderer/services/auto-sync-blocking-service.ts +++ b/src/renderer/services/auto-sync-blocking-service.ts @@ -2,7 +2,7 @@ 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 } from 'platform-bible-utils'; +import { getErrorMessage, isString } from 'platform-bible-utils'; import { setBlockedProjects } from './auto-sync-blocking-store'; /** @@ -64,7 +64,7 @@ export function initAutoSyncBlockingService(): () => void { ) { const { isBlocking, projectIds } = payload; if (typeof isBlocking === 'boolean' && Array.isArray(projectIds)) { - const stringIds = projectIds.filter((id): id is string => typeof id === 'string'); + const stringIds = projectIds.filter(isString); if (stringIds.length === projectIds.length) return isBlocking ? stringIds : []; } } @@ -84,6 +84,13 @@ export function initAutoSyncBlockingService(): () => void { setBlockedProjects(readBlockedProjectIds(event)); }); + // NOTE: this init consult is the ONLY backend re-seed today — 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 depends on the PT-4214 editor-mount + // re-query, which is out-of-scope Stage-U work tracked on PT-4214; this one-shot seed is all the + // recovery there is until it lands. (async () => { try { const snapshot = await requestNoRetry<[], unknown>( diff --git a/src/renderer/services/auto-sync-blocking-store.test.ts b/src/renderer/services/auto-sync-blocking-store.test.ts index 26fbfe6eb7f..acad2d1e2cc 100644 --- a/src/renderer/services/auto-sync-blocking-store.test.ts +++ b/src/renderer/services/auto-sync-blocking-store.test.ts @@ -24,7 +24,7 @@ describe('auto-sync-blocking-store', () => { describe('initial state', () => { it('reports nothing blocked', () => { expect(getBlockedProjectIds().size).toBe(0); - expect(isProjectBlocked('p1')).toBe(false); + expect(isProjectBlocked('P1')).toBe(false); }); it('treats an undefined project id as never blocked', () => { @@ -34,33 +34,33 @@ describe('auto-sync-blocking-store', () => { describe('show grace', () => { it('is not visible immediately when blocking starts', () => { - setBlockedProjects(['p1']); + setBlockedProjects(['P1']); expect(getBlockedProjectIds().size).toBe(0); - expect(isProjectBlocked('p1')).toBe(false); + expect(isProjectBlocked('P1')).toBe(false); }); it('becomes visible once the 200 ms grace elapses', () => { - setBlockedProjects(['p1']); + setBlockedProjects(['P1']); vi.advanceTimersByTime(SHOW_GRACE_MS); - expect(isProjectBlocked('p1')).toBe(true); - expect([...getBlockedProjectIds()]).toEqual(['p1']); + 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); - setBlockedProjects(['p1']); + setBlockedProjects(['P1']); vi.advanceTimersByTime(150); // still inside the grace window setBlockedProjects([]); vi.advanceTimersByTime(SHOW_GRACE_MS); // well past when the grace would have fired - expect(isProjectBlocked('p1')).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); - setBlockedProjects(['p1']); + setBlockedProjects(['P1']); vi.advanceTimersByTime(SHOW_GRACE_MS - 1); expect(listener).not.toHaveBeenCalled(); vi.advanceTimersByTime(1); @@ -68,82 +68,104 @@ describe('auto-sync-blocking-store', () => { }); it('shows whatever is blocked when the grace fires, even if the set grew during the grace', () => { - setBlockedProjects(['p1']); + setBlockedProjects(['P1']); vi.advanceTimersByTime(100); // inside the grace - setBlockedProjects(['p1', 'p2']); // a second project joins the in-flight batch + 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']); + expect([...getBlockedProjectIds()].sort()).toEqual(['P1', 'P2']); }); it('does not re-arm a fresh grace when a project joins an already-visible batch', () => { - setBlockedProjects(['p1']); + 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('case canonicalization (ingestion, PT-4214 Stage U)', () => { + 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); + expect([...getBlockedProjectIds()].sort()).toEqual(['PROJ_A', 'PROJ_B']); + expect(isProjectBlocked('PROJ_A')).toBe(true); + expect(isProjectBlocked('PROJ_B')).toBe(true); + }); + + 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); expect(isProjectBlocked('p1')).toBe(true); - setBlockedProjects(['p1', 'p2']); // already visible → reflected immediately, no new grace - expect(isProjectBlocked('p2')).toBe(true); + expect(isProjectBlocked('P1')).toBe(true); }); }); describe('snapshot replace semantics', () => { it('replaces the blocked set wholesale', () => { - setBlockedProjects(['p1', 'p2']); + setBlockedProjects(['P1', 'P2']); vi.advanceTimersByTime(SHOW_GRACE_MS); - expect([...getBlockedProjectIds()].sort()).toEqual(['p1', 'p2']); + 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); + 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']); + setBlockedProjects(['P1']); vi.advanceTimersByTime(SHOW_GRACE_MS); - expect(isProjectBlocked('p1')).toBe(true); + expect(isProjectBlocked('P1')).toBe(true); setBlockedProjects([]); expect(getBlockedProjectIds().size).toBe(0); }); it('notifies once when the visible set content changes', () => { - setBlockedProjects(['p1']); + setBlockedProjects(['P1']); vi.advanceTimersByTime(SHOW_GRACE_MS); const listener = vi.fn(); subscribeToAutoSyncBlocking(listener); - setBlockedProjects(['p1', 'p2']); // content changed → one notify + 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']); + setBlockedProjects(['P1', 'P2']); vi.advanceTimersByTime(SHOW_GRACE_MS); const listener = vi.fn(); subscribeToAutoSyncBlocking(listener); - setBlockedProjects(['p2', 'p1']); // same content, different order/array → no change + setBlockedProjects(['P2', 'P1']); // same content, different order/array → no change expect(listener).not.toHaveBeenCalled(); }); }); describe('no timer-driven expiry (safety leash deleted, PT-4214 Stage U)', () => { it('leaves no pending timer once blocking is visible', () => { - setBlockedProjects(['p1']); + 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']); + setBlockedProjects(['P1']); vi.advanceTimersByTime(SHOW_GRACE_MS); - expect(isProjectBlocked('p1')).toBe(true); + expect(isProjectBlocked('P1')).toBe(true); // Far beyond the old 10-minute leash — the block persists until the backend clears it. vi.advanceTimersByTime(60 * 60 * 1000); - expect(isProjectBlocked('p1')).toBe(true); + expect(isProjectBlocked('P1')).toBe(true); expect(vi.getTimerCount()).toBe(0); }); it('leaves no pending timer after blocking clears inside the grace', () => { - setBlockedProjects(['p1']); + setBlockedProjects(['P1']); setBlockedProjects([]); // cleared inside the grace expect(vi.getTimerCount()).toBe(0); // grace timer cancelled, nothing else armed }); @@ -151,7 +173,7 @@ describe('auto-sync-blocking-store', () => { describe('subscribeToAutoSyncBlocking', () => { it('notifies listeners when visibility flips to false', () => { - setBlockedProjects(['p1']); + setBlockedProjects(['P1']); vi.advanceTimersByTime(SHOW_GRACE_MS); const listener = vi.fn(); subscribeToAutoSyncBlocking(listener); @@ -163,7 +185,7 @@ describe('auto-sync-blocking-store', () => { const listener = vi.fn(); const unsubscribe = subscribeToAutoSyncBlocking(listener); unsubscribe(); - setBlockedProjects(['p1']); + setBlockedProjects(['P1']); vi.advanceTimersByTime(SHOW_GRACE_MS); expect(listener).not.toHaveBeenCalled(); }); @@ -173,7 +195,7 @@ describe('auto-sync-blocking-store', () => { const listener2 = vi.fn(); subscribeToAutoSyncBlocking(listener1); subscribeToAutoSyncBlocking(listener2); - setBlockedProjects(['p1']); + setBlockedProjects(['P1']); vi.advanceTimersByTime(SHOW_GRACE_MS); expect(listener1).toHaveBeenCalledTimes(1); expect(listener2).toHaveBeenCalledTimes(1); @@ -184,7 +206,7 @@ describe('auto-sync-blocking-store', () => { it('clears state and the pending grace timer', () => { const listener = vi.fn(); subscribeToAutoSyncBlocking(listener); - setBlockedProjects(['p1']); + setBlockedProjects(['P1']); resetAutoSyncBlocking(); vi.advanceTimersByTime(SHOW_GRACE_MS); // the grace should not fire expect(getBlockedProjectIds().size).toBe(0); diff --git a/src/renderer/services/auto-sync-blocking-store.ts b/src/renderer/services/auto-sync-blocking-store.ts index fb83c9fa040..738bdf8a5cb 100644 --- a/src/renderer/services/auto-sync-blocking-store.ts +++ b/src/renderer/services/auto-sync-blocking-store.ts @@ -18,6 +18,8 @@ * an in-flight batch) are reflected immediately, with no fresh grace. */ +import { deepEqual } from 'platform-bible-utils'; + /** * How long blocking must persist before it becomes visible; a sync finishing inside this window * shows nothing (PT9 parity). @@ -41,16 +43,9 @@ function notifyListeners(): void { listeners.forEach((listener) => listener()); } -/** Content equality for two id sets (the store always replaces sets wholesale, never mutates). */ -function areSetsEqual(a: ReadonlySet, b: ReadonlySet): boolean { - if (a === b) return true; - if (a.size !== b.size) return false; - return [...a].every((id) => b.has(id)); -} - /** Publishes a new visible set, notifying listeners only when its contents actually change. */ function setVisible(next: ReadonlySet): void { - if (areSetsEqual(visibleBlockedProjectIds, next)) return; + if (deepEqual(visibleBlockedProjectIds, next)) return; visibleBlockedProjectIds = next; notifyListeners(); } @@ -61,8 +56,11 @@ function setVisible(next: ReadonlySet): void { * it here verbatim (an empty array clears blocking entirely). */ export function setBlockedProjects(projectIds: ReadonlyArray): void { - const next: ReadonlySet = new Set(projectIds); - rawBlockedProjectIds = next; + // 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 @@ -80,8 +78,13 @@ export function setBlockedProjects(projectIds: ReadonlyArray): void { return; } - // Empty → non-empty. Arm the show-grace if one is not already pending; visibility only turns on - // if blocking survives the grace. + // 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. The assignment lives here (above the pending-grace check) — not at the top of the function + // — because only the grace callback ever reads rawBlockedProjectIds, so it was a dead store on the + // cleared and already-visible branches. Keeping it above the 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; @@ -105,7 +108,10 @@ export function getBlockedProjectIds(): ReadonlySet { */ export function isProjectBlocked(projectId: string | undefined): boolean { if (projectId === undefined) return false; - return visibleBlockedProjectIds.has(projectId); + // 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. */ 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 18de8bcb5df..0f144dacecb 100644 --- a/src/renderer/services/auto-sync-edit-block-driver.test.ts +++ b/src/renderer/services/auto-sync-edit-block-driver.test.ts @@ -78,9 +78,16 @@ describe('initAutoSyncEditBlockDriver', () => { let updateHandler: ((event: { webView: SavedWebViewDefinition }) => void) | undefined; let updateUnsub: ReturnType; - /** Points `getBlockedProjectIds` at a fresh set of the given project ids. */ + /** + * 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)); + vi.mocked(getBlockedProjectIds).mockReturnValue( + new Set(projectIds.map((id) => id.toUpperCase())), + ); } /** @@ -502,4 +509,25 @@ describe('initAutoSyncEditBlockDriver', () => { 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 8953455b78f..a2af50abcaa 100644 --- a/src/renderer/services/auto-sync-edit-block-driver.ts +++ b/src/renderer/services/auto-sync-edit-block-driver.ts @@ -27,7 +27,7 @@ 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 { getBlockedProjectIds, subscribeToAutoSyncBlocking, @@ -42,19 +42,17 @@ 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'; -/** Content equality for two id sets (both come from the store, which replaces sets wholesale). */ -function areSetsEqual(a: ReadonlySet, b: ReadonlySet): boolean { - if (a === b) return true; - if (a.size !== b.size) return false; - return [...a].every((id) => b.has(id)); -} - /** True when a Scripture editor definition's project is in the blocked set. No project → never. */ function isEditorBlocked( definition: SavedWebViewDefinition, blockedProjectIds: ReadonlySet, ): boolean { - return definition.projectId !== undefined && blockedProjectIds.has(definition.projectId); + // 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()) + ); } /** @@ -83,8 +81,13 @@ function setEditorSyncBlocked(definition: SavedWebViewDefinition, isBlocked: boo * 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): void { +function applyBlockedSetToAllEditors(blockedProjectIds: ReadonlySet): boolean { let definitions: SavedWebViewDefinition[]; try { definitions = getAllOpenWebViewDefinitionsSync(); @@ -94,12 +97,13 @@ function applyBlockedSetToAllEditors(blockedProjectIds: ReadonlySet): vo 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, isEditorBlocked(definition, blockedProjectIds)); }); + return true; } /** @@ -162,7 +166,7 @@ export function initAutoSyncEditBlockDriver(): () => void { const next = getBlockedProjectIds(); // No content change → nothing to do. This keeps overlapping identical notifications from // needlessly rewriting editors or re-subscribing the handlers. - if (areSetsEqual(next, appliedBlockedProjectIds)) return; + if (deepEqual(appliedBlockedProjectIds, next)) return; // THE ORDERING FIX. Tear the mid-block handlers down BEFORE applying the diff below. Applying the // diff issues unflag writes (isSyncBlocked: false) for no-longer-blocked projects, and @@ -177,9 +181,14 @@ export function initAutoSyncEditBlockDriver(): () => void { // protect A. Found live in E2E, 2026-07-16. teardownHandlers(); - applyBlockedSetToAllEditors(next); - // Snapshot a copy so a later store reassignment can never alias what we think we applied. - appliedBlockedProjectIds = new Set(next); + // 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); // Re-arm the mid-block handlers over the new set, unless nothing is blocked anymore. if (next.size > 0) setupHandlers(next); 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(); From 2747fe7f567b5355df75d99262f8ee5bf80b3172 Mon Sep 17 00:00:00 2001 From: timothy-mccormack Date: Wed, 22 Jul 2026 02:40:49 +0800 Subject: [PATCH 12/14] docs(platform-scripture-editor): mark onWillSwitchProject/onDidSwitchProject @experimental Per lyonsil's #2574 follow-up: the two new switch-pairing network events (added in #2571) are recently-added PAPI surface whose contract isn't settled. Add per-member @experimental TSDoc (extension .d.ts isn't run through TypeDoc, so each member needs its own tag). Matches the posture of AUTO_SYNC_MAX_DURATION_MS, which #2571 marked. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/types/platform-scripture-editor.d.ts | 6 ++++++ 1 file changed, 6 insertions(+) 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 }; } From b533d95eea82946c2e3acaa7b37672c1215701d9 Mon Sep 17 00:00:00 2001 From: timothy-mccormack Date: Wed, 22 Jul 2026 02:55:52 +0800 Subject: [PATCH 13/14] refactor(platform-scripture-editor): register switch-project events via async emitter with x-experimental Part 2 of lyonsil's #2574 @experimental follow-up: switch the onWillSwitchProject / onDidSwitchProject emitters from the deprecated sync createNetworkEventEmitter to createNetworkEventEmitterAsync, passing notification docs with 'x-experimental': true so the events are marked experimental in the generated OpenRPC document (not just the .d.ts TSDoc). The enclosing open() handler is already async, so awaiting emitter creation only adds first-use latency and the will-start event still fires before the switch. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/platform-scripture-editor/src/main.ts | 48 ++++++++++++++++--- 1 file changed, 42 insertions(+), 6 deletions(-) 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); From cfa3390327a8b35425acbbb70f48e5184061ed9e Mon Sep 17 00:00:00 2001 From: timothy-mccormack Date: Wed, 22 Jul 2026 19:51:29 +0800 Subject: [PATCH 14/14] docs: reformulate backward-facing PT-4214 comments as forward-facing (lyonsil #2574 review) Sweep the PR's comments per lyonsil's change request: drop change-history / 'PT-4214 Stage U' / 'review finding' / E2E-date justifications and restate each as present-tense documentation of what the code does. Keep genuine forward pointers to open follow-ups (editor-mount re-query, gate arming). papi.d.ts regenerated. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Projects/SendReceiveWriteLockTests.cs | 8 ++---- .../SendReceive/SendReceiveWriteLock.cs | 11 ++++---- lib/papi-dts/papi.d.ts | 10 +++---- .../auto-sync-blocking-service.test.ts | 6 ++-- .../services/auto-sync-blocking-service.ts | 20 +++++-------- .../services/auto-sync-blocking-store.test.ts | 6 ++-- .../services/auto-sync-blocking-store.ts | 28 +++++++++---------- .../auto-sync-edit-block-driver.test.ts | 2 +- .../services/auto-sync-edit-block-driver.ts | 22 +++++++-------- src/shared/data/platform.data.ts | 10 +++---- 10 files changed, 57 insertions(+), 66 deletions(-) diff --git a/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs b/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs index 008be7a6846..c84f84f1a7a 100644 --- a/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs +++ b/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs @@ -164,8 +164,8 @@ public void EnterWrite_WhileAnotherProjectSyncs_AllowsTheUnsyncedProject_PerProj { // 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). This is a deliberate reversal of the old - // global gate. IsBlocked agrees: projectB is not in the armed set. + // possible while another syncs (PT9 parity). IsBlocked agrees: projectB is not in the + // armed set. SendReceiveWriteLock.SetSyncing(["projectA"]); Assert.That( @@ -389,9 +389,7 @@ 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. This is the - // opposite of the old global-gate behavior, where an armed-with-empty-set gate rejected - // every write. + // never match an empty set, so this arm rejects NOTHING for ANY project. long token = 0; Assert.DoesNotThrow( () => token = SendReceiveWriteLock.SetSyncing([null!, ""]), diff --git a/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs b/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs index 17cab3af083..ac8c61e642f 100644 --- a/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs +++ b/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs @@ -57,7 +57,7 @@ namespace Paranext.DataProvider.Projects.SendReceive; /// 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 now consult it), but it never +/// 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 @@ -125,10 +125,9 @@ namespace Paranext.DataProvider.Projects.SendReceive; /// 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). It -/// is a reversal of this gate's original "one global sync slot rejects ALL writes" behavior; both -/// and now consult _blockedProjectIds, and -/// the safety invariant is unaffected (narrowing rejection can only allow more writes; the +/// 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 @@ -552,7 +551,7 @@ 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. This answer is per-project and now agrees + /// 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. diff --git a/lib/papi-dts/papi.d.ts b/lib/papi-dts/papi.d.ts index 2e9575ad21f..c6930b25470 100644 --- a/lib/papi-dts/papi.d.ts +++ b/lib/papi-dts/papi.d.ts @@ -8495,11 +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. * - * As of PT-4214 Stage U the renderer auto-sync-blocking store no longer consumes this: the backend - * write gate is the single authority for blocking, so the store's own `SAFETY_TIMEOUT_MS` leash was - * deleted (renderer-side resilience is re-query of the authority, not a local timer). The constant - * now survives in two rightful homes — the shutdown-sync bound in main (`shutdown-tasks.ts`) and, - * conceptually, the C# stall watchdog — both of which bound "one automatic Send/Receive". + * 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/services/auto-sync-blocking-service.test.ts b/src/renderer/services/auto-sync-blocking-service.test.ts index 5b4756effa7..a1cadd66df9 100644 --- a/src/renderer/services/auto-sync-blocking-service.test.ts +++ b/src/renderer/services/auto-sync-blocking-service.test.ts @@ -18,7 +18,7 @@ vi.mock('@shared/services/logger.service', () => ({ })); const SYNC_WRITE_LOCK_CHANGED_EVENT = 'paratextBibleSendReceive.onSyncWriteLockChanged'; -const OLD_BLOCKING_CHANGED_EVENT = 'paratextBibleSendReceive.onAutoSyncBlockingChanged'; +const LEGACY_BLOCKING_CHANGED_EVENT = 'paratextBibleSendReceive.onAutoSyncBlockingChanged'; /** Flushes the service's fire-and-forget seeding chain (await + then-callbacks). */ async function flushSeeding(): Promise { @@ -66,9 +66,9 @@ describe('initAutoSyncBlockingService', () => { expect(subscribedEventNames).toContain(SYNC_WRITE_LOCK_CHANGED_EVENT); }); - it('does NOT subscribe to the replaced onAutoSyncBlockingChanged event', () => { + it('does NOT subscribe to the legacy onAutoSyncBlockingChanged event', () => { initAutoSyncBlockingService(); - expect(subscribedEventNames).not.toContain(OLD_BLOCKING_CHANGED_EVENT); + expect(subscribedEventNames).not.toContain(LEGACY_BLOCKING_CHANGED_EVENT); }); it('returns a cleanup function that unsubscribes the event', () => { diff --git a/src/renderer/services/auto-sync-blocking-service.ts b/src/renderer/services/auto-sync-blocking-service.ts index 354616abfc2..03b743662e8 100644 --- a/src/renderer/services/auto-sync-blocking-service.ts +++ b/src/renderer/services/auto-sync-blocking-service.ts @@ -13,22 +13,17 @@ import { setBlockedProjects } from './auto-sync-blocking-store'; 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). It is the SINGLE signal source for blocking -// (PT-4214 finding 16). Only fires in Paratext 10 Studio builds where the patch arms the gate; plain -// Platform.Bible never emits it. +// 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'; -// REPLACED (PT-4214 Stage U): the extension-emitted, renderer-side boolean raise/clear event -// `paratextBibleSendReceive.onAutoSyncBlockingChanged`. It is no longer subscribed — a second signal -// source alongside the backend gate is exactly the drift finding 16 indicts, so the backend gate is -// now the single authority for blocking. - /** * 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 now on Paratext 10 Studio (returns not-blocking on plain Platform.Bible); + * 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'; @@ -84,13 +79,12 @@ export function initAutoSyncBlockingService(): () => void { setBlockedProjects(readBlockedProjectIds(event)); }); - // NOTE: this init consult is the ONLY backend re-seed today — there is no re-query on websocket + // 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 depends on the PT-4214 editor-mount - // re-query, which is out-of-scope Stage-U work tracked on PT-4214; this one-shot seed is all the - // recovery there is until it lands. + // 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>( diff --git a/src/renderer/services/auto-sync-blocking-store.test.ts b/src/renderer/services/auto-sync-blocking-store.test.ts index acad2d1e2cc..202991b6baf 100644 --- a/src/renderer/services/auto-sync-blocking-store.test.ts +++ b/src/renderer/services/auto-sync-blocking-store.test.ts @@ -85,7 +85,7 @@ describe('auto-sync-blocking-store', () => { }); }); - describe('case canonicalization (ingestion, PT-4214 Stage U)', () => { + 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 @@ -146,7 +146,7 @@ describe('auto-sync-blocking-store', () => { }); }); - describe('no timer-driven expiry (safety leash deleted, PT-4214 Stage U)', () => { + describe('no timer-driven expiry', () => { it('leaves no pending timer once blocking is visible', () => { setBlockedProjects(['P1']); expect(vi.getTimerCount()).toBe(1); // the grace timer @@ -158,7 +158,7 @@ describe('auto-sync-blocking-store', () => { setBlockedProjects(['P1']); vi.advanceTimersByTime(SHOW_GRACE_MS); expect(isProjectBlocked('P1')).toBe(true); - // Far beyond the old 10-minute leash — the block persists until the backend clears it. + // 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); diff --git a/src/renderer/services/auto-sync-blocking-store.ts b/src/renderer/services/auto-sync-blocking-store.ts index 738bdf8a5cb..f179938de9b 100644 --- a/src/renderer/services/auto-sync-blocking-store.ts +++ b/src/renderer/services/auto-sync-blocking-store.ts @@ -2,14 +2,13 @@ * Store tracking which projects an automatic (scheduled or session) Send/Receive is currently * blocking edits on. * - * BACKEND-AUTHORITATIVE SNAPSHOT model (PT-4214 Stage U): 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 no local ref-counting and no timer-driven opinion about blocking: resilience - * against a lost event is re-query of the backend authority (the service's init consult), not a - * local safety leash. The renderer's old per-blocker `SAFETY_TIMEOUT_MS` leash is deleted, not - * retained — a second, timer-driven opinion about blocking is precisely the drift the design's - * findings 7/8/16 indict. + * 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. * * 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 @@ -80,10 +79,11 @@ export function setBlockedProjects(projectIds: ReadonlyArray): void { // 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. The assignment lives here (above the pending-grace check) — not at the top of the function - // — because only the grace callback ever reads rawBlockedProjectIds, so it was a dead store on the - // cleared and already-visible branches. Keeping it above the check means a second empty → non-empty - // snapshot arriving while a grace is still pending still refreshes what that pending timer will read. + // 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(() => { @@ -103,8 +103,8 @@ export function getBlockedProjectIds(): ReadonlySet { /** * True when a specific project is (visibly) blocked. An undefined project id is never blocked. * - * No consumer in this diff — the project-settings UI (a stacked follow-up PR) is about to consume - * it to disable per-project actions while that project's automatic sync is blocking edits. + * 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; 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 0f144dacecb..8e83a94f517 100644 --- a/src/renderer/services/auto-sync-edit-block-driver.test.ts +++ b/src/renderer/services/auto-sync-edit-block-driver.test.ts @@ -176,7 +176,7 @@ describe('initAutoSyncEditBlockDriver', () => { it( 'flags only the editor whose project is syncing, leaving other projects editable ' + - '(regression: E2E defect 3 — one project syncing must not block edits in another)', + '(one project syncing must not block edits in another)', () => { vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([ editor('e1', 'projA', { viewType: 'formatted' }), // the syncing project diff --git a/src/renderer/services/auto-sync-edit-block-driver.ts b/src/renderer/services/auto-sync-edit-block-driver.ts index a2af50abcaa..b6ace1a0acb 100644 --- a/src/renderer/services/auto-sync-edit-block-driver.ts +++ b/src/renderer/services/auto-sync-edit-block-driver.ts @@ -168,17 +168,17 @@ export function initAutoSyncEditBlockDriver(): () => void { // needlessly rewriting editors or re-subscribing the handlers. if (deepEqual(appliedBlockedProjectIds, next)) return; - // THE ORDERING FIX. Tear the mid-block handlers down BEFORE applying the diff below. Applying the - // diff issues unflag writes (isSyncBlocked: false) for no-longer-blocked projects, 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. Found live in E2E, 2026-07-16. + // 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(); // Only advance the applied-set snapshot when the apply actually succeeded. If enumeration threw diff --git a/src/shared/data/platform.data.ts b/src/shared/data/platform.data.ts index 076badf11ab..16a320e6c36 100644 --- a/src/shared/data/platform.data.ts +++ b/src/shared/data/platform.data.ts @@ -54,11 +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. * - * As of PT-4214 Stage U the renderer auto-sync-blocking store no longer consumes this: the backend - * write gate is the single authority for blocking, so the store's own `SAFETY_TIMEOUT_MS` leash was - * deleted (renderer-side resilience is re-query of the authority, not a local timer). The constant - * now survives in two rightful homes — the shutdown-sync bound in main (`shutdown-tasks.ts`) and, - * conceptually, the C# stall watchdog — both of which bound "one automatic Send/Receive". + * 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 */