diff --git a/CLAUDE.md b/CLAUDE.md index 21f3154ea29..2e7cb38f33b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -161,6 +161,40 @@ npm run typecheck - Don't add features, refactor code, or make "improvements" beyond what was asked. - Avoid indecipherable [initialisms and abbreviations](.context/standards/Code-Style-Guide.md#initialisms-and-abbreviations). +## Send/Receive Write Gate + +Any new C# code path that **mutates project data** (`ScrText` writes — `PutText`, +`Settings.Save`/`SetSetting`/`RemoveSetting`, `FileManager` operations, comment/note mutations, +extension data) MUST wrap the mutation in `using var _ = SendReceiveWriteLock.EnterWrite(projectId);` +as the first statement of its entry-point method (see +`c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs`). The gate works in both directions: an +armed automatic Send/Receive rejects the write fail-fast (the `(SR_EDIT_BLOCKED)` sentinel), +while a starting sync waits, bounded, for open write scopes to drain before it replaces files on +disk. + +The gate has **no thread affinity** (its state is a single atomic word — an armed flag, an +in-flight write count, and an arm generation — not an OS lock): a scope may be disposed on a +different thread, holding one across an `await` is safe, and `SetSyncing`/`Clear` may run on any +threads. `SetSyncing` returns a token; end the bracket with `Clear(token)` (a stale token is a +logged no-op, so a late Clear can never disarm a newer sync) and keep parameterless `Clear()` for +crash recovery — it force-disarms unconditionally and is idempotent. Nested `EnterWrite` calls do +not crash, but they are NOT safe: if a sync arms while the outer scope is open, the inner call +throws the sentinel mid-mutation — keep one scope per mutation (delegate to an un-gated core +inside a single scope, as `SetBookUsfmInScope` does). Keep scopes **tight** — the mutation and +nothing else — because every open scope delays a starting sync's bounded drain toward its timeout. + +This is an **in-process** gate, distinct from the S/R server-side repository lock +(`lockrepo`/`unlockrepo` between clients) — do not conflate the two. `SendReceiveWriteLockCoverageTests` +(`c-sharp-tests/Projects/SendReceive/`) scans the source tree (excluding `bin`/`obj`) for direct +project-write call patterns (a general `.Save(` heuristic, `PutText`, comment `SaveUser`/`SaveEdits`, +and `File`/`FileManager` deletes) and fails on any hit that isn't covered per site by ONE of: gate +evidence (an `EnterWrite`/`EnterSyncWriteScope` call above it in the same method); an inline +`// SR-write-gate: exempt — ` marker on/above the write (for writes reached only through an +already-gated caller — the un-gated `SetBookUsfmInScope` core and the ManageBooks orchestrators, +each citing its gated caller + `TODO(PT-4210)`); or a whole-file entry on the test's exempt list, +which is reserved for **not-project-data** files only. Per-site (not whole-file), so a NEW ungated +write added to an already-gated file is still caught. + ## Never Commit Secrets This is an open-source repository. Never introduce secrets into the codebase: diff --git a/c-sharp-tests/Projects/SendReceive/SendReceiveWriteLockCoverageTests.cs b/c-sharp-tests/Projects/SendReceive/SendReceiveWriteLockCoverageTests.cs new file mode 100644 index 00000000000..fba3d77eea7 --- /dev/null +++ b/c-sharp-tests/Projects/SendReceive/SendReceiveWriteLockCoverageTests.cs @@ -0,0 +1,338 @@ +using System.Diagnostics.CodeAnalysis; +using System.Reflection; +using System.Text.RegularExpressions; + +namespace TestParanextDataProvider.Projects.SendReceive +{ + /// + /// Enforcement test for the AGENTS.md/CLAUDE.md "Send/Receive Write Gate" rule: any C# code path + /// that mutates project data must open a + /// scope + /// at its entry point (see that class for the full write-gate design). + /// + /// + /// This is a coarse, regex-based safety net, not a call-graph analysis: it greps the c-sharp + /// source tree for direct project-write call patterns () and, for + /// every hit, requires ONE of the following to hold — otherwise the hit is a violation: + /// + /// + /// + /// Gated in its enclosing method. An EnterWrite/EnterSyncWriteScope call + /// () appears above the write within the same method body. + /// Because the gate is always the method's FIRST statement, it sits between the write and the + /// enclosing member's boundary; walks upward from + /// the write and returns true on a gate call, false once it reaches that boundary. This is what + /// covers the ~20 gated write sites (ParatextProjectDataProvider, ManageBooksService, + /// InventoryDataProvider, CheckRunner) — and, unlike the old whole-file allowlist, it re-checks + /// EVERY site, so a NEW ungated write added to one of those files still fails (PT-4159 review, + /// finding 14). + /// + /// + /// + /// + /// Marked as consciously ungated-but-safe. An inline + /// // SR-write-gate: exempt — <reason> marker () + /// on the write's own line or the line directly above it. Used for writes reached ONLY through an + /// already-gated caller — the un-gated SetBookUsfmInScope core, and the ManageBooks + /// orchestrators / RawDirectoryProjectStreamManager whose writes run inside a gated + /// ManageBooksService / SetExtensionData scope (each marker cites its gated caller + /// and TODO(PT-4210) for the closer look). Per-line (not whole-file), so a new ungated write in + /// one of those files is still caught. + /// + /// + /// + /// + /// Whole-file exempt (not-project-data only). The file is on + /// . Reserved for files whose matched writes do not + /// touch the shared ScrText/Settings project data the S/R merge replaces (per-user + /// settings, resource caches). Gated files and gated-caller helpers are deliberately NOT here — + /// they are checked per write site via (1)/(2) above. + /// + /// + /// + /// + /// Commented out / documentation. The hit is on a comment line + /// () — e.g. a doc comment mentioning inventory.Save() — so it + /// is not a live write. + /// + /// + /// + /// + /// Heuristic bounds (honest disclosure — this is a regex scanner, not a compiler). + /// + /// + /// Enclosing-method detection assumes the code is formatted as CSharpier leaves it and the + /// files use file-scoped namespaces (class members indented one level = 4 spaces, method bodies + /// deeper). The upward walk stops at the first non-blank line indented + /// <= spaces (the enclosing member's signature/brace). It + /// only needs to be correct for files that actually CONTAIN a gate call — a file with none can + /// never spuriously "find" one — which today is exactly the four gated services above, all + /// file-scoped. A block-scoped-namespace file that both gates one method and leaves another + /// ungated could mis-scope; there are none, and gate sites are concentrated here by design. + /// + /// + /// Comment handling only skips hits whose line STARTS with //, ///, or + /// *. A write pattern hidden in a trailing inline comment or a /* … */ block on a + /// code line would still be flagged; there are none in the tree today, and a spurious flag is a + /// loud failure (safe), not a silent miss. + /// + /// + /// Write patterns are a deliberately broad net (see ); false + /// positives are expected and handled by the marker/exempt mechanisms, not by narrowing the + /// regexes to dodge a hit. + /// + /// + /// + /// + /// When this test fails on a new write site: gate it with + /// using var _ = SendReceiveWriteLock.EnterWrite(projectId); as the first statement of its + /// entry-point method; or — if it is genuinely reached only through an already-gated caller — add + /// a // SR-write-gate: exempt — <reason> marker on/above the write; or, only for + /// not-project-data writes, add the file to (with a + /// reason). Do not silently widen the write-pattern regexes to dodge a hit. + /// + [TestFixture] + [ExcludeFromCodeCoverage] + internal class SendReceiveWriteLockCoverageTests + { + /// + /// Direct project-write call patterns that must be gated. Matched against raw file text (not + /// per-line), so a call wrapped across multiple lines by a formatter is still caught. These + /// aim to cover the write kinds the AGENTS.md/CLAUDE.md rule promises — ScrText writes + /// (PutText, Save), settings writes, comment/note persistence, and project-file + /// deletes. The .Save( heuristic is deliberately broad (it catches + /// ScrText.Save(), Settings.Save(), ErrorMessageDenials.Save(), + /// ScriptureInventoryBase.Save(), and any other persistence .Save(); non-project + /// .Save( sites are consciously exempted below with a reason. It is a coarse net, not + /// a semantic analysis — false positives are expected and handled by the exempt mechanisms. + /// + private static readonly (string Label, Regex Pattern)[] s_writePatterns = + [ + // Broad persistence heuristic: subsumes Settings.Save and catches ScrText.Save, + // denials.Save, inventory.Save, XDocument.Save, etc. + ("Save()", new Regex(@"\.Save\(", RegexOptions.Compiled)), + ("ScrText.PutText", new Regex(@"\.PutText\(", RegexOptions.Compiled)), + ("Settings.SetSetting", new Regex(@"\.Settings\.SetSetting\(", RegexOptions.Compiled)), + ( + "Settings.RemoveSetting", + new Regex(@"\.Settings\.RemoveSetting\(", RegexOptions.Compiled) + ), + // Comment/note persistence (mutations are staged in memory then written by these). + ("CommentManager.SaveUser", new Regex(@"\.SaveUser\(", RegexOptions.Compiled)), + ( + "CommentEditHelper.SaveEdits", + new Regex(@"CommentEditHelper\.SaveEdits\(", RegexOptions.Compiled) + ), + // Project-file deletes: ScrText.FileManager.Delete plus raw File.Delete of project files. + ("FileManager.Delete", new Regex(@"\.FileManager\.Delete\(", RegexOptions.Compiled)), + ("File.Delete", new Regex(@"\bFile\.Delete\(", RegexOptions.Compiled)), + ]; + + /// + /// Gate-scope evidence: an EnterWrite( or EnterSyncWriteScope( call opening the + /// enclosing method's write scope. A write site with this above it (within its method body) + /// is gated. See . + /// + private static readonly Regex s_gateEvidencePattern = + new(@"EnterWrite\(|EnterSyncWriteScope\(", RegexOptions.Compiled); + + /// + /// The per-line opt-out marker for a write that is consciously ungated-but-safe (reached only + /// through an already-gated caller). Placed on the write's own line or the line directly + /// above it, e.g. // SR-write-gate: exempt — reached only via gated CreateBooksAsync. + /// + private static readonly Regex s_exemptMarkerPattern = + new(@"//\s*SR-write-gate:\s*exempt", RegexOptions.Compiled); + + /// + /// Leading-space indentation (in spaces) at or below which a non-blank line is treated as the + /// enclosing member's boundary (its signature/brace) rather than a statement in the method + /// body. Class members are one indent level (4 spaces) in these file-scoped-namespace files; + /// method-body statements — including the gate, always the first statement — are deeper. + /// + private const int MemberIndentThreshold = 4; + + /// + /// Files exempt from the scan as WHOLE files, as paths relative to the c-sharp/ source + /// root (forward-slash, case-sensitive). Reserved for the not-project-data category + /// ONLY: files whose matched writes do not touch the shared ScrText/Settings + /// project data the S/R merge replaces (per-user settings, resource caches), so the write gate + /// does not apply. Gated services and their gated-caller helpers are deliberately NOT listed + /// here — each of their write sites is verified individually (gate evidence in the enclosing + /// method, or an inline // SR-write-gate: exempt marker), so a new ungated write added + /// to one of them is still caught (PT-4159 review, finding 14). + /// + private static readonly string[] s_fileLevelExemptRelativePaths = + [ + // not-project-data: per-user UserSettings-{userId}.xml (XDocument.Save), not shared merged data. + "Projects/UserProjectSettings.cs", + // not-project-data: enhanced-resource cache; File.Delete removes a superseded V1 companion file. + "EnhancedResources/MarblePackageDiscoverer.cs", + ]; + + /// + /// Resolves the absolute path to the c-sharp/ source directory by walking upward from + /// the executing test assembly until a directory containing both c-sharp/ and + /// c-sharp-tests/ is found. Robust to the assembly living under varying + /// bin/<Configuration>/<TFM> nesting and to path-separator differences + /// across OSes (uses throughout, no hardcoded separators). + /// + private static string ResolveSourceDir() + { + string? walk = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location); + for (var i = 0; i < 10 && walk != null; i++) + { + var candidate = Path.Combine(walk, "c-sharp"); + if ( + Directory.Exists(candidate) + && Directory.Exists(Path.Combine(walk, "c-sharp-tests")) + ) + return Path.GetFullPath(candidate); + walk = Path.GetDirectoryName(walk); + } + + Assert.Fail( + "Could not locate the c-sharp/ source directory by walking up from the test " + + $"assembly at '{Assembly.GetExecutingAssembly().Location}'." + ); + return string.Empty; // unreachable; Assert.Fail throws + } + + [Test] + public void AllProjectWriteCallSites_AreGatedOrAllowlisted() + { + var sourceDir = ResolveSourceDir(); + var fileLevelExempt = new HashSet( + s_fileLevelExemptRelativePaths, + StringComparer.Ordinal + ); + + // Deterministic ordering: sort files by their (OS-independent) relative path so failure + // output — and the order files are scanned in — never depends on filesystem enumeration + // order. Skip build output (bin/obj): those hold generated source (*.g.cs, AssemblyInfo) + // that is neither ours to gate nor allowlistable by a stable relative path (PT-4159 + // review, finding 15). + var csFiles = Directory + .EnumerateFiles(sourceDir, "*.cs", SearchOption.AllDirectories) + .Select(path => (Full: path, Relative: NormalizeRelativePath(sourceDir, path))) + .Where(file => !IsInGeneratedBuildDir(file.Relative)) + .OrderBy(file => file.Relative, StringComparer.Ordinal); + + var violations = new List(); + + foreach (var (fullPath, relativePath) in csFiles) + { + if (fileLevelExempt.Contains(relativePath)) + continue; + + var text = File.ReadAllText(fullPath); + var lines = text.Split('\n'); + foreach (var (label, pattern) in s_writePatterns) + { + foreach (Match match in pattern.Matches(text)) + { + var lineNumber = CountLines(text, match.Index); // 1-based + var codeLine = lines[lineNumber - 1]; + + // Not a live write: a comment (commented-out code or a doc reference). + if (IsCommentLine(codeLine)) + continue; + // Consciously ungated-but-safe: reached only through a gated caller. + if (HasExemptMarkerNear(lines, lineNumber)) + continue; + // Gated at the enclosing method's entry point. + if (HasGateEvidenceInEnclosingMethod(lines, lineNumber)) + continue; + + violations.Add($"{relativePath}:{lineNumber} — {label}"); + } + } + } + + Assert.That( + violations, + Is.Empty, + "Found direct project-write call site(s) that are neither gated by " + + "SendReceiveWriteLock.EnterWrite in their enclosing method nor consciously " + + "exempted:\n" + + string.Join('\n', violations) + + "\n\nFix each by ONE of: (a) gate it with " + + "`using var _ = SendReceiveWriteLock.EnterWrite(projectId);` as the first " + + "statement of its entry-point method; (b) if it is reached only through an " + + "already-gated caller, add a `// SR-write-gate: exempt — ` marker on " + + "or directly above the write; or (c) only for not-project-data writes, add the " + + $"file to {nameof(s_fileLevelExemptRelativePaths)} (with a reason). See the " + + "AGENTS.md/CLAUDE.md \"Send/Receive Write Gate\" rule." + ); + } + + /// True if the relative path lies under a bin or obj build-output + /// directory at any depth. + private static bool IsInGeneratedBuildDir(string relativePath) => + relativePath.Split('/').Any(segment => segment is "bin" or "obj"); + + /// True if the line is (or opens/continues) a comment — trimmed start is + /// //, ///, /*, or a * block-comment continuation. + private static bool IsCommentLine(string line) + { + var trimmed = line.TrimStart(); + return trimmed.StartsWith("//", StringComparison.Ordinal) + || trimmed.StartsWith("/*", StringComparison.Ordinal) + || trimmed.StartsWith("*", StringComparison.Ordinal); + } + + /// True if a // SR-write-gate: exempt marker is on the write's own line + /// (, 1-based) or the line directly above it. + private static bool HasExemptMarkerNear(string[] lines, int writeLine) + { + if (s_exemptMarkerPattern.IsMatch(lines[writeLine - 1])) + return true; + return writeLine - 2 >= 0 && s_exemptMarkerPattern.IsMatch(lines[writeLine - 2]); + } + + /// + /// True if a gate call () opens the write's enclosing + /// method above (1-based). Walks upward from the line above the + /// write; because the gate is always the method's first statement it sits between the write + /// and the enclosing member's boundary, so we return true on the first gate call and false + /// once we reach that boundary (the first non-blank line indented + /// <= ). See the heuristic bounds in the class remarks. + /// + private static bool HasGateEvidenceInEnclosingMethod(string[] lines, int writeLine) + { + for (var i = writeLine - 2; i >= 0; i--) + { + var line = lines[i]; + if (string.IsNullOrWhiteSpace(line)) + continue; + if (s_gateEvidencePattern.IsMatch(line)) + return true; + if (LeadingSpaces(line) <= MemberIndentThreshold) + return false; // reached the enclosing member's boundary without a gate + } + return false; + } + + private static int LeadingSpaces(string line) + { + var count = 0; + while (count < line.Length && line[count] == ' ') + count++; + return count; + } + + private static string NormalizeRelativePath(string root, string fullPath) => + Path.GetRelativePath(root, fullPath).Replace(Path.DirectorySeparatorChar, '/'); + + private static int CountLines(string text, int upToIndex) + { + var line = 1; + for (var i = 0; i < upToIndex && i < text.Length; i++) + { + if (text[i] == '\n') + line++; + } + return line; + } + } +} diff --git a/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs b/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs new file mode 100644 index 00000000000..235c6e8e1be --- /dev/null +++ b/c-sharp-tests/Projects/SendReceiveWriteLockTests.cs @@ -0,0 +1,1064 @@ +using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; +using System.Reflection; +using Paranext.DataProvider.Checks; +using Paranext.DataProvider.JsonUtils; +using Paranext.DataProvider.ManageBooks; +using Paranext.DataProvider.Projects; +using Paranext.DataProvider.Projects.SendReceive; +using Paratext.Data; +using Paratext.Data.ProjectComments; +using SIL.Scripture; + +namespace TestParanextDataProvider.Projects +{ + /// + /// Unit tests for the process-wide write-gate. Every test + /// fully resets the (static) gate before and after — including the in-flight count, which a + /// production Clear deliberately leaves alone — so a test that leaks a scope cannot + /// poison later tests with full-DrainTimeout drains. The fixture runs single-threaded (no + /// NUnit parallelism), so the shared static gate is never touched concurrently across tests. + /// + [ExcludeFromCodeCoverage] + internal class SendReceiveWriteLockTests + { + [SetUp] + public void SetUp() => SendReceiveWriteLock.ResetForTests(); + + [TearDown] + public void TearDown() => SendReceiveWriteLock.ResetForTests(); + + // ---- IsBlocked (pure per-project data) ---- + + [Test] + public void IsBlocked_NothingSyncing_ReturnsFalse() + { + Assert.That(SendReceiveWriteLock.IsBlocked("projectA"), Is.False); + } + + [Test] + public void IsBlocked_ProjectSyncing_ReturnsTrueForThatProjectOnly() + { + SendReceiveWriteLock.SetSyncing(["projectA", "projectB"]); + + Assert.Multiple(() => + { + Assert.That(SendReceiveWriteLock.IsBlocked("projectA"), Is.True); + Assert.That(SendReceiveWriteLock.IsBlocked("projectB"), Is.True); + Assert.That(SendReceiveWriteLock.IsBlocked("projectC"), Is.False); + }); + } + + [Test] + public void IsBlocked_ProjectIdCasingDiffers_StillBlocked() + { + SendReceiveWriteLock.SetSyncing(["AbCdEf123"]); + + Assert.Multiple(() => + { + Assert.That(SendReceiveWriteLock.IsBlocked("abcdef123"), Is.True); + Assert.That(SendReceiveWriteLock.IsBlocked("ABCDEF123"), Is.True); + }); + } + + [Test] + public void IsBlocked_NullOrEmptyProjectId_ReturnsFalse() + { + SendReceiveWriteLock.SetSyncing(["projectA"]); + + Assert.Multiple(() => + { + Assert.That(SendReceiveWriteLock.IsBlocked(null), Is.False); + Assert.That(SendReceiveWriteLock.IsBlocked(string.Empty), Is.False); + }); + } + + [Test] + public void SetSyncing_CalledAgain_ReplacesPreviousSet() + { + // Re-arming while already armed just swaps the armed set (one global sync slot). + SendReceiveWriteLock.SetSyncing(["projectA"]); + SendReceiveWriteLock.SetSyncing(["projectB"]); + + Assert.Multiple(() => + { + Assert.That(SendReceiveWriteLock.IsBlocked("projectA"), Is.False); + Assert.That(SendReceiveWriteLock.IsBlocked("projectB"), Is.True); + }); + } + + [Test] + public void Clear_AfterSetSyncing_UnblocksEverything() + { + SendReceiveWriteLock.SetSyncing(["projectA", "projectB"]); + SendReceiveWriteLock.Clear(); + + Assert.Multiple(() => + { + Assert.That(SendReceiveWriteLock.IsBlocked("projectA"), Is.False); + Assert.That(SendReceiveWriteLock.IsBlocked("projectB"), Is.False); + }); + } + + // ---- EnterWrite scope (the in-flight write side) ---- + + [Test] + public void EnterWrite_NotArmed_ReturnsScope_AndReleasesOnDispose() + { + var scope = SendReceiveWriteLock.EnterWrite("projectA"); + Assert.That(scope, Is.Not.Null); + Assert.That(SendReceiveWriteLock.InFlightWriteCount, Is.EqualTo(1)); + + scope.Dispose(); + + // The count — the exact value a sync's drain waits on — proves the release. + // (Re-entering would prove nothing: EnterWrite only consults the armed flag.) + Assert.That( + SendReceiveWriteLock.InFlightWriteCount, + Is.Zero, + "dispose must release the in-flight write" + ); + } + + [Test] + public void EnterWrite_DoubleDispose_IsSafe() + { + // Hold a SECOND scope open so a broken double-dispose (decrementing twice) shows up in + // the count instead of being absorbed by the underflow guard at count zero. + var scope = SendReceiveWriteLock.EnterWrite("projectA"); + using var otherScope = SendReceiveWriteLock.EnterWrite("projectB"); + Assert.That(SendReceiveWriteLock.InFlightWriteCount, Is.EqualTo(2)); + + scope.Dispose(); + + // Second dispose must be a no-op (never decrement the in-flight count twice). + Assert.DoesNotThrow(scope.Dispose); + Assert.That( + SendReceiveWriteLock.InFlightWriteCount, + Is.EqualTo(1), + "the second dispose must not release the OTHER scope's in-flight count" + ); + } + + [Test] + public void EnterWrite_NullProjectId_Throws() + { + Assert.Throws(() => SendReceiveWriteLock.EnterWrite(null!)); + } + + [Test] + public void EnterWrite_Armed_ThrowsWithSentinel() + { + // A sync is armed (the gate has no thread affinity, so arming on the test thread is + // exactly what production writes would observe from any thread). + SendReceiveWriteLock.SetSyncing(["projectA"]); + + var ex = Assert.Throws( + () => SendReceiveWriteLock.EnterWrite("projectA") + ); + Assert.That(ex!.Message, Does.EndWith(SendReceiveWriteLock.EditBlockedSentinel)); + } + + [Test] + public void EnterWrite_WhileAnotherProjectSyncs_RejectsAllProjects_GlobalGate() + { + // 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. + SendReceiveWriteLock.SetSyncing(["projectA"]); + + Assert.Multiple(() => + { + Assert.That( + SendReceiveWriteLock.IsBlocked("projectB"), + Is.False, + "IsBlocked is per-project pure data — projectB is not in the armed set" + ); + var ex = Assert.Throws( + () => SendReceiveWriteLock.EnterWrite("projectB") + ); + Assert.That(ex!.Message, Does.EndWith(SendReceiveWriteLock.EditBlockedSentinel)); + }); + } + + // ---- Forgiving lifecycle contract (no thread affinity, no recursion policy) ---- + + [Test] + public void EnterWrite_NestedOnSameThread_BothScopesSucceed() + { + // While nothing is armed, a nested EnterWrite must not crash or deadlock (there is no + // recursion policy) — the inner scope just counts as one more in-flight write. See + // EnterWrite_NestedWhileSyncArmed_InnerThrowsSentinel for why nesting is still NOT a + // safe pattern around a live sync. + using var outer = SendReceiveWriteLock.EnterWrite("projectA"); + + Assert.DoesNotThrow(() => + { + using var inner = SendReceiveWriteLock.EnterWrite("projectB"); + }); + } + + [Test] + public void EnterWrite_NestedWhileSyncArmed_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. + var previousTimeout = SendReceiveWriteLock.DrainTimeout; + SendReceiveWriteLock.DrainTimeout = TimeSpan.FromMilliseconds(50); + try + { + using var outer = SendReceiveWriteLock.EnterWrite("projectA"); + SendReceiveWriteLock.SetSyncing(["projectA"]); // degrades: outer cannot drain + + var ex = Assert.Throws( + () => SendReceiveWriteLock.EnterWrite("projectB") + ); + Assert.That(ex!.Message, Does.EndWith(SendReceiveWriteLock.EditBlockedSentinel)); + } + finally + { + SendReceiveWriteLock.DrainTimeout = previousTimeout; + } + } + + [Test] + public void Clear_FromDifferentThreadThanSetSyncing_DisarmsAndUnblocks() + { + // Arm on a worker thread that then goes away (as an async sync worker's pool thread + // might). Clear from ANY other thread must fully disarm — recovery must never depend on + // the arming thread still existing. + var worker = new Thread(() => SendReceiveWriteLock.SetSyncing(["projectA"])) + { + IsBackground = true, + Name = "ArmingWorker", + }; + worker.Start(); + Assert.That( + worker.Join(TimeSpan.FromSeconds(10)), + Is.True, + "worker should finish arming" + ); + + // Assert the premise before acting on it: the worker's arm must be observable from + // this thread, or the disarm assertions below would pass vacuously. + Assert.That(SendReceiveWriteLock.IsArmed, Is.True, "the worker's arm must be visible"); + Assert.That(SendReceiveWriteLock.IsBlocked("projectA"), Is.True); + + Assert.DoesNotThrow(SendReceiveWriteLock.Clear); + + Assert.That(SendReceiveWriteLock.IsBlocked("projectA"), Is.False); + Assert.DoesNotThrow(() => + { + using var _ = SendReceiveWriteLock.EnterWrite("projectA"); + }); + } + + [Test] + public void WriteScope_DisposedOnDifferentThread_ReleasesTheInFlightWrite() + { + // A write scope that crosses an await can resume — and dispose — on a different pool + // thread. The release must work from any thread, and a subsequent sync must then see + // zero in-flight writes (drain immediately rather than wait for the DrainTimeout). + var scope = SendReceiveWriteLock.EnterWrite("projectA"); + + var disposeTask = Task.Run(scope.Dispose); + Assert.That( + disposeTask.Wait(TimeSpan.FromSeconds(10)), + Is.True, + "disposing on another thread must complete promptly" + ); + Assert.That( + SendReceiveWriteLock.InFlightWriteCount, + Is.Zero, + "the cross-thread dispose must have released the in-flight write" + ); + + var stopwatch = Stopwatch.StartNew(); + SendReceiveWriteLock.SetSyncing(["projectA"]); + stopwatch.Stop(); + Assert.That( + stopwatch.Elapsed, + Is.LessThan(TimeSpan.FromSeconds(2)), + "the released write must not count as in-flight during the sync's drain" + ); + + SendReceiveWriteLock.Clear(); + } + + [Test] + public void SetSyncing_NullOrEmptyIdsInBatch_AreIgnored() + { + // A defective batch must not crash the arming thread or leave a torn arm state; the + // valid ids must still be armed. + Assert.DoesNotThrow(() => SendReceiveWriteLock.SetSyncing(["projectA", null!, ""])); + + Assert.Multiple(() => + { + // Assert on the stored set itself: IsBlocked("") could never return true (its own + // null/empty guard short-circuits), so it cannot falsify the ignore-filter. + Assert.That( + SendReceiveWriteLock.ArmedProjectIds, + Is.EquivalentTo(new[] { "projectA" }), + "only the valid id may enter the armed set" + ); + Assert.That(SendReceiveWriteLock.IsBlocked("projectA"), Is.True); + }); + + SendReceiveWriteLock.Clear(); + } + + // ---- SetSyncing drains in-flight writes ---- + + [Test] + public void SetSyncing_WaitsForInFlightWriteOnAnotherThread_ToDrain() + { + var opened = new ManualResetEventSlim(false); + var release = new ManualResetEventSlim(false); + const int holdMs = 500; + + // An in-flight write held on another thread. SetSyncing (on the test thread) must WAIT + // for the scope to dispose before returning. + var writer = Task.Run(() => + { + using var scope = SendReceiveWriteLock.EnterWrite("projectA"); + opened.Set(); + release.Wait(TimeSpan.FromSeconds(15)); + }); + + Assert.That( + opened.Wait(TimeSpan.FromSeconds(5)), + Is.True, + "the in-flight write should have opened its scope" + ); + + // Release the in-flight write ~holdMs from now, then time how long SetSyncing blocks. + var releaser = Task.Run(() => + { + Thread.Sleep(holdMs); + release.Set(); + }); + + var stopwatch = Stopwatch.StartNew(); + SendReceiveWriteLock.SetSyncing(["projectA"]); // blocks until the other thread disposes + stopwatch.Stop(); + + Task.WaitAll([writer, releaser], TimeSpan.FromSeconds(20)); + + Assert.Multiple(() => + { + Assert.That( + stopwatch.ElapsedMilliseconds, + Is.GreaterThanOrEqualTo(holdMs - 200), + "SetSyncing should have blocked until the in-flight write on the other thread drained" + ); + Assert.That( + SendReceiveWriteLock.InFlightWriteCount, + Is.Zero, + "after draining, no write scopes remain open" + ); + Assert.That( + SendReceiveWriteLock.IsArmed, + Is.True, + "after draining, the sync is armed" + ); + }); + + SendReceiveWriteLock.Clear(); + } + + [Test] + public void SetSyncing_DrainTimesOut_ProceedsDegraded_AndNewWritesStayRejected() + { + var previousTimeout = SendReceiveWriteLock.DrainTimeout; + SendReceiveWriteLock.DrainTimeout = TimeSpan.FromMilliseconds(300); + var opened = new ManualResetEventSlim(false); + var release = new ManualResetEventSlim(false); + Task? stuck = null; + try + { + // An in-flight write on another thread that will NOT drain within the (shortened) + // timeout. + stuck = Task.Run(() => + { + using var scope = SendReceiveWriteLock.EnterWrite("projectA"); + opened.Set(); + release.Wait(TimeSpan.FromSeconds(15)); + }); + Assert.That(opened.Wait(TimeSpan.FromSeconds(5)), Is.True); + + var stopwatch = Stopwatch.StartNew(); + Assert.DoesNotThrow( + () => SendReceiveWriteLock.SetSyncing(["projectA"]), + "a bounded drain must proceed (log + continue) on timeout, not hang or throw" + ); + stopwatch.Stop(); + + Assert.Multiple(() => + { + Assert.That( + stopwatch.Elapsed, + Is.GreaterThanOrEqualTo(TimeSpan.FromMilliseconds(250)), + "the degraded path must actually wait out the DrainTimeout, not skip the drain" + ); + Assert.That( + stopwatch.Elapsed, + Is.LessThan(TimeSpan.FromSeconds(5)), + "SetSyncing must not hang past its bounded DrainTimeout" + ); + Assert.That( + SendReceiveWriteLock.InFlightWriteCount, + Is.EqualTo(1), + "the stuck write is still in flight on the degraded path" + ); + + // New writes stay rejected while armed, even though the drain timed out. + var ex = Assert.Throws( + () => SendReceiveWriteLock.EnterWrite("projectA") + ); + Assert.That( + ex!.Message, + Does.EndWith(SendReceiveWriteLock.EditBlockedSentinel) + ); + }); + } + finally + { + release.Set(); + stuck?.Wait(TimeSpan.FromSeconds(10)); + SendReceiveWriteLock.DrainTimeout = previousTimeout; + SendReceiveWriteLock.Clear(); + } + } + + [Test] + public void SetSyncing_DrainTimesOut_WithThrowOption_RollsBackArmAndThrows() + { + var previousTimeout = SendReceiveWriteLock.DrainTimeout; + SendReceiveWriteLock.DrainTimeout = TimeSpan.FromMilliseconds(300); + var opened = new ManualResetEventSlim(false); + var release = new ManualResetEventSlim(false); + Task? stuck = null; + try + { + stuck = Task.Run(() => + { + using var scope = SendReceiveWriteLock.EnterWrite("projectA"); + opened.Set(); + release.Wait(TimeSpan.FromSeconds(15)); + }); + Assert.That(opened.Wait(TimeSpan.FromSeconds(5)), Is.True); + + // Opting into throw-on-timeout aborts the sync start instead of degrading... + Assert.Throws( + () => SendReceiveWriteLock.SetSyncing(["projectA"], throwOnDrainTimeout: true) + ); + + // ...and the throw means "arm rolled back, no cleanup owed": nothing stays armed, + // writes flow again, and the stuck write's own count is untouched. + Assert.Multiple(() => + { + Assert.That(SendReceiveWriteLock.IsArmed, Is.False, "the arm must roll back"); + Assert.That(SendReceiveWriteLock.IsBlocked("projectA"), Is.False); + Assert.That(SendReceiveWriteLock.InFlightWriteCount, Is.EqualTo(1)); + Assert.DoesNotThrow(() => + { + using var _ = SendReceiveWriteLock.EnterWrite("projectA"); + }); + }); + } + finally + { + release.Set(); + stuck?.Wait(TimeSpan.FromSeconds(10)); + SendReceiveWriteLock.DrainTimeout = previousTimeout; + } + } + + [Test] + public void SetSyncing_CleanDrain_WithThrowOption_ArmsNormally() + { + // The throw option changes ONLY the drain-timeout outcome: with nothing in flight the + // call must arm and return a usable token exactly like the default path. + long token = 0; + Assert.DoesNotThrow( + () => + token = SendReceiveWriteLock.SetSyncing(["projectA"], throwOnDrainTimeout: true) + ); + + Assert.Multiple(() => + { + Assert.That(SendReceiveWriteLock.IsArmed, Is.True); + Assert.That(SendReceiveWriteLock.IsBlocked("projectA"), Is.True); + }); + + SendReceiveWriteLock.Clear(token); + Assert.That(SendReceiveWriteLock.IsArmed, Is.False); + } + + [Test] + public void Clear_WhileAWriteIsStillStuck_RecoversNewWritesImmediately() + { + // Recovery must not depend on the stuck write ever completing: Clear disarms the gate, + // new writes flow again, and the straggler's eventual dispose is a harmless decrement. + var previousTimeout = SendReceiveWriteLock.DrainTimeout; + SendReceiveWriteLock.DrainTimeout = TimeSpan.FromMilliseconds(300); + var opened = new ManualResetEventSlim(false); + var release = new ManualResetEventSlim(false); + Task? stuck = null; + try + { + stuck = Task.Run(() => + { + using var scope = SendReceiveWriteLock.EnterWrite("projectA"); + opened.Set(); + release.Wait(TimeSpan.FromSeconds(15)); + }); + Assert.That(opened.Wait(TimeSpan.FromSeconds(5)), Is.True); + + SendReceiveWriteLock.SetSyncing(["projectA"]); // degraded (stuck write never drains) + SendReceiveWriteLock.Clear(); + + Assert.DoesNotThrow(() => + { + using var _ = SendReceiveWriteLock.EnterWrite("projectA"); + }); + + // The third clause of the recovery contract: the straggler's eventual dispose must + // land as its own harmless decrement (not vanish, not eat another scope's count). + release.Set(); + Assert.That( + stuck.Wait(TimeSpan.FromSeconds(10)), + Is.True, + "the straggler should finish once released" + ); + Assert.That( + SendReceiveWriteLock.InFlightWriteCount, + Is.Zero, + "the straggler's dispose after Clear must release its own in-flight count" + ); + } + finally + { + release.Set(); + stuck?.Wait(TimeSpan.FromSeconds(10)); + SendReceiveWriteLock.DrainTimeout = previousTimeout; + SendReceiveWriteLock.Clear(); + } + } + + // ---- Clear(token): stale-bracket protection ---- + + [Test] + public void ClearWithToken_CurrentBracket_Disarms() + { + var token = SendReceiveWriteLock.SetSyncing(["projectA"]); + + SendReceiveWriteLock.Clear(token); + + Assert.Multiple(() => + { + Assert.That(SendReceiveWriteLock.IsArmed, Is.False); + Assert.That(SendReceiveWriteLock.IsBlocked("projectA"), Is.False); + }); + } + + [Test] + public void ClearWithToken_StaleAfterNewerSetSyncing_DoesNotDisarmTheNewerSync() + { + // Sync A ends late: its Clear(tokenA) must be a logged no-op once sync B owns the + // slot, instead of silently disarming the gate in the middle of B's run. + var tokenA = SendReceiveWriteLock.SetSyncing(["projectA"]); + var tokenB = SendReceiveWriteLock.SetSyncing(["projectB"]); + + SendReceiveWriteLock.Clear(tokenA); + + Assert.Multiple(() => + { + Assert.That(SendReceiveWriteLock.IsArmed, Is.True, "sync B must stay armed"); + Assert.That(SendReceiveWriteLock.IsBlocked("projectB"), Is.True); + }); + + SendReceiveWriteLock.Clear(tokenB); + Assert.That(SendReceiveWriteLock.IsArmed, Is.False); + } + + [Test] + public void ClearWithToken_WhenAlreadyCleared_IsSafeNoOp() + { + var token = SendReceiveWriteLock.SetSyncing(["projectA"]); + SendReceiveWriteLock.Clear(token); + + // A duplicate bracket-end (e.g. a finally after an explicit Clear) must stay a no-op. + Assert.DoesNotThrow(() => SendReceiveWriteLock.Clear(token)); + Assert.That(SendReceiveWriteLock.IsArmed, Is.False); + } + + [Test] + public void SetSyncing_GenerationWrap_SkipsTokenZero() + { + // Token 0 is reserved as a natural "no arm" default for callers (e.g. + // `long token = 0; ... if (token != 0) Clear(token);`), so the arm at the wrap + // boundary must skip it — and its bracket must still round-trip. + SendReceiveWriteLock.ResetForTests(generation: SendReceiveWriteLock.MaxGeneration); + + var wrappedToken = SendReceiveWriteLock.SetSyncing(["projectA"]); + + Assert.Multiple(() => + { + Assert.That(wrappedToken, Is.Not.Zero, "token 0 must never be issued"); + Assert.That(SendReceiveWriteLock.IsArmed, Is.True); + }); + + SendReceiveWriteLock.Clear(wrappedToken); + Assert.That( + SendReceiveWriteLock.IsArmed, + Is.False, + "the wrapped token must still end its own bracket" + ); + } + + [Test] + public void SetSyncing_ClearedMidDrain_ReturnsPromptlyInsteadOfBurningTheTimeout() + { + // A Clear that lands while SetSyncing is still draining removes the drain's premise + // (the gate is disarmed, so writes may enter again and the count may never settle). + // The drain must notice the disarm and return promptly instead of spinning out its + // full DrainTimeout — here the DEFAULT 10s timeout, so a prompt return is clearly + // distinguishable from a burned timeout. + var opened = new ManualResetEventSlim(false); + var release = new ManualResetEventSlim(false); + Task? stuck = null; + Task? sync = null; + try + { + // A stuck write pins the count so the drain cannot finish on its own. + stuck = Task.Run(() => + { + using var scope = SendReceiveWriteLock.EnterWrite("projectA"); + opened.Set(); + release.Wait(TimeSpan.FromSeconds(15)); + }); + Assert.That(opened.Wait(TimeSpan.FromSeconds(5)), Is.True); + + sync = Task.Run(() => SendReceiveWriteLock.SetSyncing(["projectA"])); + Assert.That( + SpinWait.SpinUntil(() => SendReceiveWriteLock.IsArmed, TimeSpan.FromSeconds(5)), + Is.True, + "SetSyncing should arm before draining" + ); + + SendReceiveWriteLock.Clear(); + + Assert.That( + sync.Wait(TimeSpan.FromSeconds(3)), + Is.True, + "a Clear during the drain must end the wait promptly, not burn the full DrainTimeout" + ); + Assert.That(SendReceiveWriteLock.IsArmed, Is.False); + } + finally + { + release.Set(); + stuck?.Wait(TimeSpan.FromSeconds(10)); + sync?.Wait(TimeSpan.FromSeconds(15)); + } + } + + // ---- Clear / round-trip ---- + + [Test] + public void Clear_WhenNeverArmed_IsSafeNoOp() + { + // No SetSyncing → nothing armed. Clear must not throw (it only disarms). + Assert.DoesNotThrow(SendReceiveWriteLock.Clear); + Assert.That(SendReceiveWriteLock.IsArmed, Is.False); + + // And a subsequent write still works. + Assert.DoesNotThrow(() => + { + using var _ = SendReceiveWriteLock.EnterWrite("projectA"); + }); + } + + [Test] + public void SetSyncing_ThenClear_RoundTrips() + { + // No in-flight writes → SetSyncing returns at once, armed. + SendReceiveWriteLock.SetSyncing(["projectA"]); + Assert.Multiple(() => + { + Assert.That( + SendReceiveWriteLock.IsArmed, + Is.True, + "SetSyncing should arm the gate when there is nothing to drain" + ); + Assert.That(SendReceiveWriteLock.IsBlocked("projectA"), Is.True); + }); + + SendReceiveWriteLock.Clear(); + Assert.Multiple(() => + { + Assert.That(SendReceiveWriteLock.IsArmed, Is.False, "Clear should disarm the gate"); + Assert.That(SendReceiveWriteLock.IsBlocked("projectA"), Is.False); + }); + + // The gate is reusable after a clean round-trip. + Assert.DoesNotThrow(() => + { + using var _ = SendReceiveWriteLock.EnterWrite("projectA"); + }); + } + + // ---- Concurrency stress: the core safety invariant ---- + + /// + /// Hammers from several threads while a "sync" + /// repeatedly arms (draining in-flight writes), simulates a file-replacement window, and + /// clears. Asserts the invariant the whole mechanism exists to guarantee: a project write + /// NEVER executes inside its scope during the sync's file-replacement window. A broken gate — + /// one that let EnterWrite open a scope while armed, or that let SetSyncing + /// return with in-flight writes still open (a broken drain) — would let a write slip in and + /// violate this. + /// + [Test] + public void ConcurrentWrites_NeverExecuteDuringSyncFileReplacement() + { + const string project = "raceProject"; + const int iterations = 300; + const int writerCount = 4; + + var stop = false; + var syncReplacing = 0; // 1 while the "sync" is in its file-replacement window + var violations = 0; // a write ran while syncReplacing == 1 + var writesObserved = 0; // sanity: writers actually got through + var rejectionsDuringReplacement = 0; // sanity: writers CONTENDED during the windows + + var writers = Enumerable + .Range(0, writerCount) + .Select(_ => + Task.Run(() => + { + while (!Volatile.Read(ref stop)) + { + try + { + using var scope = SendReceiveWriteLock.EnterWrite(project); + Interlocked.Increment(ref writesObserved); + if (Volatile.Read(ref syncReplacing) == 1) + Interlocked.Increment(ref violations); + } + catch (InvalidOperationException) + { + // Fail-fast rejection while a sync is armed — expected; keep + // trying. Rejections inside the replacement window prove writers + // were actively contending exactly when it matters (see the + // vacuity assert below). + if (Volatile.Read(ref syncReplacing) == 1) + Interlocked.Increment(ref rejectionsDuringReplacement); + } + } + }) + ) + .ToArray(); + + for (var i = 0; i < iterations; i++) + { + // Yielding sleeps (NOT Thread.SpinWait, which monopolizes the core on a 1-2 vCPU + // CI runner so writers never get scheduled inside the windows that matter). + Thread.Sleep(1); // unarmed window so writers make progress + SendReceiveWriteLock.SetSyncing([project]); // arm + drain in-flight writes + Volatile.Write(ref syncReplacing, 1); + Thread.Sleep(1); // "file replacement" window + Volatile.Write(ref syncReplacing, 0); + SendReceiveWriteLock.Clear(); + } + + Volatile.Write(ref stop, true); + Assert.That( + Task.WaitAll(writers, TimeSpan.FromSeconds(30)), + Is.True, + "writer tasks must finish before the counters are inspected" + ); + + Assert.Multiple(() => + { + Assert.That( + violations, + Is.Zero, + "a project write executed while the sync was replacing files" + ); + Assert.That( + writesObserved, + Is.GreaterThan(0), + "writers never succeeded; the test would be vacuous" + ); + Assert.That( + rejectionsDuringReplacement, + Is.GreaterThan(0), + "no writer ever attempted a write during a replacement window; violations == 0 " + + "would be vacuous" + ); + }); + } + } + + /// + /// Verifies the write-gate is actually wired into every gated project-write entry point: each + /// method opens a scope as its first statement, so + /// it is rejected (with the sentinel) while the project is registered as syncing. One round-trip + /// test additionally proves a write succeeds again after the sync ends. + /// + /// Covered here: all 11 write methods, the 5 mutating + /// wire methods, and the two CheckRunner denial writers + /// (DenyCheckResult/AllowCheckResult, invoked via reflection since they are private + /// and CheckRunner is sealed). The two InventoryDataProvider setters are gated + /// identically but are private (only reachable through the PAPI wire dispatch), so they have no + /// direct wiring test; the gate call itself is the same one-line EnterWrite scope covered + /// by . + /// + /// Each test arms the sync with directly on + /// the test thread — the gate has no thread affinity, so a gated write observes the armed state + /// identically from any thread. + /// + [ExcludeFromCodeCoverage] + internal class SendReceiveWriteLockGateTests : PapiTestBase + { + private const string PdpName = "sendReceiveWriteLockGateTestProject"; + + private ScrText _scrText = null!; + private ProjectDetails _projectDetails = null!; + private DummyParatextProjectDataProvider _provider = null!; + private ManageBooksService _manageBooksService = null!; + + private string ProjectId => _projectDetails.Metadata.Id; + + [SetUp] + public override async Task TestSetupAsync() + { + await base.TestSetupAsync(); + SendReceiveWriteLock.ResetForTests(); + + _scrText = CreateDummyProject(); + _projectDetails = CreateProjectDetails(_scrText); + ParatextProjects.FakeAddProject(_projectDetails, _scrText); + + // Seed a book so the round-trip write has something valid to modify once the gate is + // cleared. The entry-gate tests don't need it (they throw before touching the project). + _scrText.PutText( + 1, + 0, + false, + @"\id GEN \ip intro \c 2 \p \v 1 verse one \c 3 \p \v 1 bla", + null + ); + + _provider = new DummyParatextProjectDataProvider( + PdpName, + Client, + _projectDetails, + ParatextProjects + ); + + var pdpFactory = new ParatextProjectDataProviderFactory(Client, ParatextProjects); + await pdpFactory.InitializeAsync(); + _manageBooksService = new ManageBooksService(Client, ParatextProjects, pdpFactory); + } + + [TearDown] + public void TearDown() + { + SendReceiveWriteLock.ResetForTests(); + _scrText?.Dispose(); + } + + /// + /// Arms this fixture's project as syncing, invokes , and asserts it + /// is rejected with the sentinel-suffixed exception (before any mutation can happen). The + /// TearDown gate reset disarms afterward. + /// + private void AssertWriteBlocked(TestDelegate write) + { + SendReceiveWriteLock.SetSyncing([ProjectId]); + + var ex = Assert.Throws(write); + Assert.That(ex!.Message, Does.EndWith(SendReceiveWriteLock.EditBlockedSentinel)); + } + + /// Minimal comment for the comment-mutation gate tests (never dereferenced — + /// the gate throws first). + private PlatformCommentWrapper CreateMinimalCommentWrapper() => + new(new Comment(_scrText.User)); + + [Test] + public void SetChapterUsfm_ProjectSyncing_ThrowsWithSentinel_ThenSucceedsAfterClear() + { + var verseRef = new VerseRef(1, 2, 0); + + SendReceiveWriteLock.SetSyncing([ProjectId]); + var ex = Assert.Throws( + () => _provider.SetChapterUsfm(verseRef, @"\c 2 \p \v 2 New chapter text.") + ); + Assert.That(ex!.Message, Does.EndWith(SendReceiveWriteLock.EditBlockedSentinel)); + + // Sync ended (Clear disarms the gate). + SendReceiveWriteLock.Clear(); + Assert.That( + _provider.SetChapterUsfm(verseRef, @"\c 2 \p \v 2 New chapter text."), + Is.True, + "Write should succeed once the project is no longer syncing" + ); + } + + [Test] + public void SetBookUsfm_ProjectSyncing_ThrowsWithSentinel() => + AssertWriteBlocked( + () => _provider.SetBookUsfm(new VerseRef(1, 1, 0), @"\id GEN \c 1 \p \v 1 text") + ); + + [Test] + public void SetBookUsx_ProjectSyncing_ThrowsWithSentinel() => + AssertWriteBlocked(() => _provider.SetBookUsx(new VerseRef(1, 1, 0), "")); + + [Test] + public void SetChapterUsx_ProjectSyncing_ThrowsWithSentinel() => + AssertWriteBlocked(() => _provider.SetChapterUsx(new VerseRef(1, 2, 0), "")); + + [Test] + public void SetProjectSetting_ProjectSyncing_ThrowsWithSentinel() => + AssertWriteBlocked(() => _provider.SetProjectSetting("platform.fullName", "New Name")); + + [Test] + public void SetExtensionData_ProjectSyncing_ThrowsWithSentinel() => + AssertWriteBlocked( + () => + _provider.SetExtensionData( + new ProjectDataScope + { + ProjectID = ProjectId, + ExtensionName = "myExtension", + DataQualifier = "myFile.txt", + }, + "data" + ) + ); + + [Test] + public void CreateComment_ProjectSyncing_ThrowsWithSentinel() => + AssertWriteBlocked(() => _provider.CreateComment(CreateMinimalCommentWrapper())); + + [Test] + public void AddCommentToThread_ProjectSyncing_ThrowsWithSentinel() => + AssertWriteBlocked(() => _provider.AddCommentToThread(CreateMinimalCommentWrapper())); + + [Test] + public void UpdateComment_ProjectSyncing_ThrowsWithSentinel() => + AssertWriteBlocked(() => _provider.UpdateComment("someCommentId", "

updated

")); + + [Test] + public void DeleteComment_ProjectSyncing_ThrowsWithSentinel() => + AssertWriteBlocked(() => _provider.DeleteComment("someCommentId")); + + [Test] + public void ResolveConflict_ProjectSyncing_ThrowsWithSentinel() => + AssertWriteBlocked(() => _provider.ResolveConflict("someThreadId", "accept")); + + // ManageBooksService wire methods (the gate is the first statement, so none of these need the + // request contents to be otherwise valid). The methods are non-async Task methods, so the + // gate's throw propagates synchronously at call time. + + [Test] + public void DeleteBooks_ProjectSyncing_ThrowsWithSentinel() => + AssertWriteBlocked( + () => _manageBooksService.DeleteBooksAsync(new DeleteBooksRequest(ProjectId, [1])) + ); + + [Test] + public void CreateBooks_ProjectSyncing_ThrowsWithSentinel() => + AssertWriteBlocked( + () => + _manageBooksService.CreateBooksAsync( + new CreateBooksRequest(ProjectId, [1], CreationMethod.Empty, null) + ) + ); + + [Test] + public void CopyBooks_DestinationProjectSyncing_ThrowsWithSentinel() => + AssertWriteBlocked( + () => + _manageBooksService.CopyBooksAsync( + new CopyBooksRequest("someOtherProjectId", ProjectId, [1]) + ) + ); + + [Test] + public void CopyCustomVersification_DestinationProjectSyncing_ThrowsWithSentinel() => + AssertWriteBlocked( + () => + _manageBooksService.CopyCustomVersificationAsync( + "someOtherProjectId", + ProjectId + ) + ); + + [Test] + public void ImportBooks_ProjectSyncing_ThrowsWithSentinel() => + AssertWriteBlocked( + () => + _manageBooksService.ImportBooksAsync(new ImportBooksInput(ProjectId, [], false)) + ); + + // CheckRunner denial writers (DenyCheckResult/AllowCheckResult persist ErrorMessageDenials + // into the project folder via denials.Save()). They are private and CheckRunner is sealed, so + // they can neither be called nor overridden directly; invoke them through reflection. The gate + // is their first statement, so it throws before the check cache or project is touched — the + // arguments only need to satisfy the signature. MethodInfo.Invoke wraps the throw in a + // TargetInvocationException, so assert on its InnerException. + + private void AssertCheckRunnerDenialBlocked(string methodName) + { + var checkRunner = new CheckRunner( + Client, + new InventoryDataProvider(Client, ParatextProjects) + ); + var method = + typeof(CheckRunner).GetMethod( + methodName, + BindingFlags.NonPublic | BindingFlags.Instance + ) ?? throw new MissingMethodException(nameof(CheckRunner), methodName); + + SendReceiveWriteLock.SetSyncing([ProjectId]); + + var tie = Assert.Throws( + () => + method.Invoke( + checkRunner, + [ + "checkId", + "checkResultType", + ProjectId, + new VerseRef(1, 1, 0), + "itemText", + null, + ] + ) + ); + Assert.That(tie!.InnerException, Is.InstanceOf()); + Assert.That( + tie.InnerException!.Message, + Does.EndWith(SendReceiveWriteLock.EditBlockedSentinel) + ); + } + + [Test] + public void DenyCheckResult_ProjectSyncing_ThrowsWithSentinel() => + AssertCheckRunnerDenialBlocked("DenyCheckResult"); + + [Test] + public void AllowCheckResult_ProjectSyncing_ThrowsWithSentinel() => + AssertCheckRunnerDenialBlocked("AllowCheckResult"); + } +} diff --git a/c-sharp/Checks/CheckRunner.cs b/c-sharp/Checks/CheckRunner.cs index 98d1849ddba..12d64365811 100644 --- a/c-sharp/Checks/CheckRunner.cs +++ b/c-sharp/Checks/CheckRunner.cs @@ -2,6 +2,7 @@ using System.Text.Json; using Paranext.DataProvider.NetworkObjects; using Paranext.DataProvider.Projects; +using Paranext.DataProvider.Projects.SendReceive; using Paratext.Checks; using Paratext.Data.Checking; using PtxUtils; @@ -159,6 +160,12 @@ private bool DenyCheckResult( string? _checkResultUniqueId ) { + // Bracket the whole mutation in a write scope — denials.Save() below persists the + // error-message denials into the project folder. Rejects (fail-fast) while an automatic + // Send/Receive is syncing this project, and keeps the sync's drain waiting until this + // mutation completes. Inert in public core — see SendReceiveWriteLock. + using var _ = SendReceiveWriteLock.EnterWrite(projectId); + var check = _checkCache.GetCheck(checkId, projectId); var denials = GetOrCreateDenials(projectId); denials.AddDenial(new Enum(checkResultType), vRef, null, itemText); @@ -176,6 +183,12 @@ private bool AllowCheckResult( string? _checkResultUniqueId ) { + // Bracket the whole mutation in a write scope — denials.Save() below persists the + // error-message denials into the project folder. Rejects (fail-fast) while an automatic + // Send/Receive is syncing this project, and keeps the sync's drain waiting until this + // mutation completes. Inert in public core — see SendReceiveWriteLock. + using var _ = SendReceiveWriteLock.EnterWrite(projectId); + var check = _checkCache.GetCheck(checkId, projectId); var denials = GetOrCreateDenials(projectId); denials.RemoveDenial(new Enum(checkResultType), vRef, null, itemText); diff --git a/c-sharp/Checks/InventoryDataProvider.cs b/c-sharp/Checks/InventoryDataProvider.cs index 2efed5e26ec..321b1c2a903 100644 --- a/c-sharp/Checks/InventoryDataProvider.cs +++ b/c-sharp/Checks/InventoryDataProvider.cs @@ -4,6 +4,7 @@ using Paranext.DataProvider.JsonUtils; using Paranext.DataProvider.NetworkObjects; using Paranext.DataProvider.Projects; +using Paranext.DataProvider.Projects.SendReceive; using Paratext.Checks; using PtxUtils; @@ -135,6 +136,14 @@ private List GetInventoryItemStatus(InventoryItemStatusSele private bool SetInventoryItemStatus(InventoryItemStatusSelector selector, JsonElement status) { + // Bracket the whole mutation in a write scope — inventory.Save() below persists + // valid/invalid items into the project's checking settings. Rejects (fail-fast) while an + // automatic Send/Receive is syncing this project, and keeps the sync's drain waiting until + // this mutation completes. Inert in public core — see SendReceiveWriteLock. Opened as the + // first statement per the write-gate rule; the argument guards below only validate (no + // mutation) and EnterWrite already rejects a null project id. + using var _ = SendReceiveWriteLock.EnterWrite(selector.ProjectId); + ArgumentException.ThrowIfNullOrEmpty(selector.ProjectId); ArgumentException.ThrowIfNullOrEmpty(selector.InventoryId); @@ -306,6 +315,14 @@ private List GetInventoryOptionValues(InventoryOptionsSele private bool SetInventoryOptionValues(InventoryOptionsSelector selector, JsonElement values) { + // Bracket the whole mutation in a write scope — this method writes the project's settings + // (SetSetting/RemoveSetting/Save below). Rejects (fail-fast) while an automatic Send/Receive + // is syncing this project, and keeps the sync's drain waiting until this mutation completes. + // Inert in public core — see SendReceiveWriteLock. Opened as the first statement per the + // write-gate rule; the argument guards below only validate (no mutation) and EnterWrite + // already rejects a null project id. + using var _ = SendReceiveWriteLock.EnterWrite(selector.ProjectId); + ArgumentException.ThrowIfNullOrEmpty(selector.ProjectId); ArgumentException.ThrowIfNullOrEmpty(selector.InventoryId); diff --git a/c-sharp/ManageBooks/CopyBooksOrchestrator.cs b/c-sharp/ManageBooks/CopyBooksOrchestrator.cs index 2a600f58b01..682b4940fb5 100644 --- a/c-sharp/ManageBooks/CopyBooksOrchestrator.cs +++ b/c-sharp/ManageBooks/CopyBooksOrchestrator.cs @@ -830,6 +830,7 @@ List errors string sourceUsfm = fromScrText.GetText(bookNum); if (replaceEntireBook) { + // SR-write-gate: exempt — reached only via the gated ManageBooksService.CopyBooksAsync/CopyCustomVersificationAsync (TODO(PT-4210): assess). toScrText.PutText(bookNum, 0, false, sourceUsfm, null); return true; } @@ -888,6 +889,7 @@ List errors bool destBookExists = toScrText.Settings.BooksPresentSet.IsSelected(bookNum); if (!destBookExists) { + // SR-write-gate: exempt — reached only via the gated ManageBooksService.CopyBooksAsync/CopyCustomVersificationAsync (TODO(PT-4210): assess). toScrText.PutText(bookNum, 0, false, sourceUsfm, null); return true; } @@ -940,6 +942,7 @@ List errors } try { + // SR-write-gate: exempt — reached only via the gated ManageBooksService.CopyBooksAsync/CopyCustomVersificationAsync (TODO(PT-4210): assess). toScrText.PutText(bookNum, i + 1, false, chapterText, null); } catch (Exception ex) diff --git a/c-sharp/ManageBooks/DeleteBooksOrchestrator.cs b/c-sharp/ManageBooks/DeleteBooksOrchestrator.cs index 72e8b02a619..01022d3d621 100644 --- a/c-sharp/ManageBooks/DeleteBooksOrchestrator.cs +++ b/c-sharp/ManageBooks/DeleteBooksOrchestrator.cs @@ -75,6 +75,7 @@ public static void DeleteBooks(ScrText scrText, BookSet selectedBooks) string bookFileName = scrText.Settings.BookFileName(bookNum, true); if (scrText.FileManager.Exists(bookFileName)) + // SR-write-gate: exempt — reached only via the gated ManageBooksService.DeleteBooksAsync (TODO(PT-4210): assess). scrText.FileManager.Delete(bookFileName); success = true; @@ -84,6 +85,7 @@ public static void DeleteBooks(ScrText scrText, BookSet selectedBooks) if (success) { scrText.Settings.BooksPresentSet = availableBooks; + // SR-write-gate: exempt — reached only via the gated ManageBooksService.DeleteBooksAsync (TODO(PT-4210): assess). scrText.Save(); } } diff --git a/c-sharp/ManageBooks/ImportBooksOrchestrator.cs b/c-sharp/ManageBooks/ImportBooksOrchestrator.cs index a8a816d0ef6..3e97e6d6ac9 100644 --- a/c-sharp/ManageBooks/ImportBooksOrchestrator.cs +++ b/c-sharp/ManageBooks/ImportBooksOrchestrator.cs @@ -816,6 +816,7 @@ List errors // per-chapter writes correctly without a special test seam. if (replaceEntireBook) { + // SR-write-gate: exempt — reached only via the gated ManageBooksService.ImportBooksAsync/CreateBooksAsync (TODO(PT-4210): assess). scrText.PutText(bookNum, 0, false, bookText, null); return true; } @@ -895,6 +896,7 @@ List errors // PutText(0) call so BooksPresentSet picks it up via the existing fast path. if (!destBookExists) { + // SR-write-gate: exempt — reached only via the gated ManageBooksService.ImportBooksAsync/CreateBooksAsync (TODO(PT-4210): assess). scrText.PutText(bookNum, 0, false, bookText, null); return true; } @@ -956,6 +958,7 @@ List errors } try { + // SR-write-gate: exempt — reached only via the gated ManageBooksService.ImportBooksAsync/CreateBooksAsync (TODO(PT-4210): assess). scrText.PutText(bookNum, i + 1, false, chapterText, null); } catch (Exception ex) diff --git a/c-sharp/ManageBooks/ManageBooksService.cs b/c-sharp/ManageBooks/ManageBooksService.cs index 6ff06b813a7..91a6f339237 100644 --- a/c-sharp/ManageBooks/ManageBooksService.cs +++ b/c-sharp/ManageBooks/ManageBooksService.cs @@ -2,6 +2,7 @@ using Paranext.DataProvider.NetworkObjects.Documentation; using Paranext.DataProvider.ParatextUtils; using Paranext.DataProvider.Projects; +using Paranext.DataProvider.Projects.SendReceive; using Paranext.DataProvider.Services; using Paratext.Data; using PtxUtils; @@ -354,6 +355,11 @@ private static Dictionary< /// Result with success flag, deleted count, warnings, errors. public Task DeleteBooksAsync(DeleteBooksRequest request) { + // Bracket the whole deletion in a write scope: rejects (fail-fast) while an automatic + // Send/Receive is syncing this project, and keeps the sync's drain waiting until this + // mutation completes. Inert in public core — see SendReceiveWriteLock. + using var _ = SendReceiveWriteLock.EnterWrite(request.ProjectId); + EnsureBookNumbersNonEmpty(request.BookNumbers); ScrText scrText = GetProjectOrThrowNotFound(request.ProjectId); @@ -640,6 +646,11 @@ public Task IsProjectSharedAsync(string projectId) /// public Task CreateBooksAsync(CreateBooksRequest request) { + // Bracket the whole creation in a write scope: rejects (fail-fast) while an automatic + // Send/Receive is syncing this project, and keeps the sync's drain waiting until this + // mutation completes. Inert in public core — see SendReceiveWriteLock. + using var _ = SendReceiveWriteLock.EnterWrite(request.ProjectId); + EnsureBookNumbersNonEmpty(request.BookNumbers); ScrText scrText = GetProjectOrThrowNotFound(request.ProjectId); @@ -991,6 +1002,12 @@ public Task GetToProjectFilterAsync(ProjectFilterInput input) /// public Task CopyBooksAsync(CopyBooksRequest request) { + // Bracket the whole copy in a write scope on the DESTINATION project (the only one written; + // the source is read-only): rejects (fail-fast) while it is syncing, and keeps the sync's + // drain waiting until this mutation completes. Inert in public core — see + // SendReceiveWriteLock. + using var _ = SendReceiveWriteLock.EnterWrite(request.ToProjectId); + EnsureBookNumbersNonEmpty(request.BookNumbers); EnsureDifferentProjects(request.FromProjectId, request.ToProjectId); @@ -1056,6 +1073,12 @@ public Task CopyBooksAsync(CopyBooksRequest request) /// public Task CopyCustomVersificationAsync(string sourceProjectId, string destProjectId) { + // Bracket the whole copy in a write scope on the DESTINATION project (its custom.vrs and + // versification table are mutated): rejects (fail-fast) while it is syncing, and keeps the + // sync's drain waiting until this mutation completes. Inert in public core — see + // SendReceiveWriteLock. + using var _ = SendReceiveWriteLock.EnterWrite(destProjectId); + ScrText fromScrText = ResolveProjectOrThrow( sourceProjectId, PlatformErrorCodes.NotFound, @@ -1222,6 +1245,11 @@ public Task CheckOverlappingFilesAsync(OverlapCheckEntry[] ent /// public Task ImportBooksAsync(ImportBooksInput request) { + // Bracket the whole import in a write scope: rejects (fail-fast) while an automatic + // Send/Receive is syncing this project, and keeps the sync's drain waiting until this + // mutation completes. Inert in public core — see SendReceiveWriteLock. + using var _ = SendReceiveWriteLock.EnterWrite(request.ProjectId); + // Guard 1: project must resolve (NOT_FOUND per Theme 7). ScrText scrText = GetProjectOrThrowNotFound(request.ProjectId); diff --git a/c-sharp/ManageBooks/ScriptureTemplateService.cs b/c-sharp/ManageBooks/ScriptureTemplateService.cs index 640b3233c98..7f8fbf71289 100644 --- a/c-sharp/ManageBooks/ScriptureTemplateService.cs +++ b/c-sharp/ManageBooks/ScriptureTemplateService.cs @@ -145,6 +145,7 @@ public static bool CreateOneBook( else result = CreateIdLineOnly(scrText, initialLines, bookNum, textLock); + // SR-write-gate: exempt — reached only via the gated ManageBooksService.CreateBooksAsync (TODO(PT-4210): assess). scrText.Save(); } catch (IOException) @@ -272,6 +273,7 @@ WriteLock textLock return false; string template = idLine + ExtractTemplate(text, modelScrText, bookNum); + // SR-write-gate: exempt — reached only via the gated ManageBooksService.CreateBooksAsync (TODO(PT-4210): assess). scrText.PutText(bookNum, 0, false, template, textLock); return true; @@ -389,6 +391,7 @@ private static bool CreateCV(ScrText scrText, string idLine, int bookNum, WriteL if (cvText == null) return false; + // SR-write-gate: exempt — reached only via the gated ManageBooksService.CreateBooksAsync (TODO(PT-4210): assess). scrText.PutText(bookNum, 0, false, idLine + cvText, textLock); return true; } @@ -451,6 +454,7 @@ private static bool CreateIdLineOnly( WriteLock textLock ) { + // SR-write-gate: exempt — reached only via the gated ManageBooksService.CreateBooksAsync (TODO(PT-4210): assess). scrText.PutText(bookNum, 0, false, idLine + "\r\n", textLock); return true; } diff --git a/c-sharp/Projects/ParatextProjectDataProvider.cs b/c-sharp/Projects/ParatextProjectDataProvider.cs index a624d25b282..7e9e9384bfd 100644 --- a/c-sharp/Projects/ParatextProjectDataProvider.cs +++ b/c-sharp/Projects/ParatextProjectDataProvider.cs @@ -6,6 +6,7 @@ using System.Xml.XPath; using Paranext.DataProvider.JsonUtils; using Paranext.DataProvider.NetworkObjects.Documentation; +using Paranext.DataProvider.Projects.SendReceive; using Paranext.DataProvider.Services; using Paratext.Data; using Paratext.Data.ProjectComments; @@ -343,6 +344,7 @@ protected override Task StartDataProviderAsync() public override bool SetExtensionData(ProjectDataScope scope, string data) { + using var _ = EnterSyncWriteScope(); if (string.IsNullOrEmpty(scope.ExtensionName)) throw new InvalidDataException("Must provide an extension name"); if (string.IsNullOrEmpty(scope.DataQualifier)) @@ -482,6 +484,7 @@ public List GetCommentThreads(CommentThreadSelecto public bool DeleteComment(string commentId) { + using var _ = EnterSyncWriteScope(); lock (_commentMutationLock) { // Find the comment by ID and its parent thread @@ -523,6 +526,7 @@ private static string ReplaceNewlinesWithSpaces(string text) /// in this project. public string CreateComment(PlatformCommentWrapper comment) { + using var _ = EnterSyncWriteScope(); VerifyUserCanCreateComments(); // Never let the "content could not be displayed" placeholder become a note's real content. @@ -655,6 +659,7 @@ public string CreateComment(PlatformCommentWrapper comment) /// If the thread ID is missing or doesn't exist public string AddCommentToThread(PlatformCommentWrapper comment) { + using var _ = EnterSyncWriteScope(); lock (_commentMutationLock) { if (string.IsNullOrEmpty(comment.Thread)) @@ -767,6 +772,9 @@ public string AddCommentToThread(PlatformCommentWrapper comment) /// Not a verseText conflict, the thread is already resolved, the user lacks permission, the resolve was canceled, or resolution is 'reject' or 'merge' and the verse text has changed since the conflict was recorded (stale). public void ResolveConflict(string threadId, string resolution) { + // Named (not `_`) because this method later uses `out _` discards, which would bind to a + // using variable named `_` and fail to compile. + using var syncWriteScope = EnterSyncWriteScope(); if (resolution != "accept" && resolution != "reject" && resolution != "merge") throw new InvalidDataException( $"Invalid resolution '{resolution}' for ResolveConflict; expected 'accept', 'reject', or 'merge'." @@ -1077,6 +1085,7 @@ private static void CopyCommentProperties(PlatformCommentWrapper source, Comment public bool UpdateComment(string commentId, string updatedContentHtml) { + using var _ = EnterSyncWriteScope(); lock (_commentMutationLock) { if (string.IsNullOrEmpty(commentId)) @@ -1719,6 +1728,7 @@ out string? settingValue public bool SetProjectSetting(string settingName, object? value) { + using var _ = EnterSyncWriteScope(); var scrText = LocalParatextProjects.GetParatextProject(ProjectDetails.Metadata.Id); if (scrText.IsResourceProject) throw new Exception("Cannot change settings on resources"); @@ -2400,6 +2410,20 @@ public string GetVerseUsfm(VerseRef verseRef) } public bool SetBookUsfm(VerseRef verseRef, string data) + { + using var _ = EnterSyncWriteScope(); + return SetBookUsfmInScope(verseRef, data); + } + + /// + /// The body of WITHOUT opening its own sync-write scope. Callers MUST + /// already hold one (via ). This exists so a gated method that + /// delegates to the book-USFM write () can reuse it inside a single + /// outer scope. (Nesting a second is NOT a safe + /// alternative: if a sync armed while the outer scope was open, the nested call would throw + /// mid-mutation and tear the write. One scope per mutation is required, not stylistic.) + /// + private bool SetBookUsfmInScope(VerseRef verseRef, string data) { verseRef.ChapterNum = 0; var scrText = LocalParatextProjects.GetParatextProject(ProjectDetails.Metadata.Id); @@ -2413,6 +2437,7 @@ public bool SetBookUsfm(VerseRef verseRef, string data) BookSet localBooksPresentSet = scrText.Settings.LocalBooksPresentSet; isNewBook = !localBooksPresentSet.IsSelected(verseRef.BookNum); // Set with chapter 0 sets the whole book + // SR-write-gate: exempt — un-gated core; the whole mutation runs inside SetBookUsfm/SetBookUsx's write scope (nesting a 2nd gate is unsafe; see SendReceiveWriteLock). scrText.PutText(verseRef.BookNum, 0, false, data, writeLock); } ); @@ -2429,6 +2454,7 @@ public bool SetBookUsfm(VerseRef verseRef, string data) public bool SetChapterUsfm(VerseRef verseRef, string data) { + using var _ = EnterSyncWriteScope(); try { var scrText = LocalParatextProjects.GetParatextProject(ProjectDetails.Metadata.Id); @@ -2561,15 +2587,21 @@ public string GetVerseUsx(VerseRef verseRef) public bool SetBookUsx(VerseRef verseRef, string data) { - // Don't need to take a write lock in this function because SetBookUsfm will do it + // Open ONE sync-write scope for the whole convert-then-write mutation. Rejecting here also + // skips the USX→USFM conversion when sync-blocked. We then call the un-gated + // SetBookUsfmInScope (NOT the public SetBookUsfm) so the whole mutation sits under a single + // scope — nesting is unsafe under an arm race; see SetBookUsfmInScope. Inert in public core. + using var _ = EnterSyncWriteScope(); + // The ParatextData project write lock (RunWithinLock) is taken inside SetBookUsfmInScope — + // unrelated to the S/R gate scope above, despite the similar name. var scrText = LocalParatextProjects.GetParatextProject(ProjectDetails.Metadata.Id); string usfm = ConvertUsxToUsfm(scrText, verseRef, data); - SetBookUsfm(verseRef, usfm); - return true; + return SetBookUsfmInScope(verseRef, usfm); } public bool SetChapterUsx(VerseRef verseRef, string data) { + using var _ = EnterSyncWriteScope(); string? failedMessage = null; bool didChange = true; try @@ -2831,5 +2863,20 @@ private static void RunWithinLock(WriteScope writeScope, Action actio } } + /// + /// Opens a write scope that brackets a project mutation while an automatic Send/Receive is + /// syncing this project, so an editor change can't race the sync's on-disk file replacement. If + /// the project is sync-blocked the scope throws immediately (fail-fast); otherwise it counts the + /// write as in-flight until disposed, so a sync starting mid-write drains it first. Inert in + /// public core: nothing calls there, so this never + /// throws (see that class). Use as the FIRST statement of the project write methods (Scripture, + /// settings, extension data, and comment mutations): + /// using var _ = EnterSyncWriteScope(); so the scope covers the whole mutation. + /// + private IDisposable EnterSyncWriteScope() + { + return SendReceiveWriteLock.EnterWrite(ProjectDetails.Metadata.Id); + } + #endregion } diff --git a/c-sharp/Projects/RawDirectoryProjectStreamManager.cs b/c-sharp/Projects/RawDirectoryProjectStreamManager.cs index 420a6277818..b374e39fa94 100644 --- a/c-sharp/Projects/RawDirectoryProjectStreamManager.cs +++ b/c-sharp/Projects/RawDirectoryProjectStreamManager.cs @@ -94,6 +94,7 @@ public bool DeleteDataStream(string streamName) // TODO: This doesn't seem to be try { + // SR-write-gate: exempt — reached only via the gated ParatextProjectDataProvider.SetExtensionData; DeleteDataStream currently unused (TODO(PT-4210): assess). File.Delete(fileName); return File.Exists(fileName); } diff --git a/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs b/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs new file mode 100644 index 00000000000..9fdd1e1953f --- /dev/null +++ b/c-sharp/Projects/SendReceive/SendReceiveWriteLock.cs @@ -0,0 +1,487 @@ +using System.Collections.Immutable; +using System.Diagnostics; + +namespace Paranext.DataProvider.Projects.SendReceive; + +/// +/// Process-wide coordination between ordinary project writes and an automatic (scheduled) +/// Send/Receive, so an editor change can never race the sync's on-disk file replacement (Mercurial +/// merge) and corrupt the project. +/// +/// +/// +/// The coordination works in both directions: +/// +/// +/// +/// 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. +/// +/// +/// +/// +/// A starting sync WAITS (bounded) for in-flight writes to drain. +/// arms the gate, then waits up to for already-open write scopes to +/// close before returning. The wait is bounded so a single stuck write can never deadlock a sync +/// start — on timeout it logs a warning and proceeds anyway (see the degraded path below). +/// +/// +/// +/// +/// +/// How it works — one atomic word. All gate state that synchronization depends on lives in a +/// single (_state): an "armed" flag bit plus the count of in-flight write +/// scopes. Every transition — enter a write, exit a write, arm, disarm — is a single interlocked +/// read-modify-write on that one word, so all transitions are totally ordered and each one sees the +/// exact state it replaces. The safety invariant falls out directly: a write scope can only open by +/// atomically observing "not armed" while incrementing the count, and arming atomically sets the +/// flag, so after the arming operation NO new write scope can open, and the in-flight count the +/// drain then waits on can only fall. Because a write's mutation happens entirely between its enter +/// and exit operations (both full fences), a drained count also means every finished write's +/// effects are visible to the sync. The word also carries an arm generation — the token +/// returns — so can check-and-disarm in the +/// same single CAS (see the overlap remarks below). No mutual exclusion depends on any other +/// field: _blockedProjectIds is pure data (it only feeds queries, +/// and can disagree with the gate while racing arm/clear calls overlap — potentially for that +/// whole bracket, not just momentarily; rejection never consults it), and there is deliberately +/// no "is the lock held" bookkeeping to fall out of step with reality. +/// +/// +/// Forgiving lifecycle contract (deliberate). The arm→clear bracket is activated by code +/// that lives outside this repository (see activation below) and whose threading model this class +/// cannot see or test, so the gate assumes as little as possible: +/// +/// +/// +/// and / may run on ANY +/// thread, including different threads — an await between them (which resumes on a +/// different pool thread) is fine. +/// +/// +/// +/// +/// A write scope may be disposed on a different thread than the one that opened it, so a gated +/// method that comes to hold its scope across an await stays correct. (Still, hold scopes +/// tightly: a long-held scope delays a sync's drain toward its timeout.) +/// +/// +/// +/// +/// 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. +/// +/// +/// +/// +/// is idempotent and safe to call when nothing is armed, so it can serve as +/// crash recovery: if a sync worker dies without clearing, ANY thread (e.g. a watchdog or the next +/// sync) can call it to recover. Bracket code should prefer with the +/// token its returned — a newer arm turns that into a logged no-op, so a +/// late or duplicate Clear cannot disarm a sync it does not own. Double-disposing a scope and +/// over-releasing are guarded no-ops. No SetSyncing/Clear/Dispose sequence, on any combination of +/// threads, can leave writes permanently rejected. (A write scope that is never disposed is the +/// one durable failure: its count is deliberately NOT reset by Clear, so every later sync waits +/// out the full and proceeds degraded.) +/// +/// +/// +/// +/// +/// Degraded (drain-timed-out) path. If in-flight writes do not drain within +/// , logs a warning and returns anyway — a sync +/// start must never hang forever behind a stuck write. The armed flag keeps rejecting NEW writes +/// exactly as on the normal path; only the already-stuck write(s) may still overlap the sync. This +/// is an accepted residual risk, traded against deadlocking every future sync. A caller that must +/// not run concurrently with even a stuck write can opt out per call: +/// with throwOnDrainTimeout: true rolls the arm back and throws +/// 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. +/// +/// +/// Distinct from the Send/Receive server-side repository lock. This class is an +/// in-process gate between local editing and a local sync run. It is not the S/R +/// server's repository lock (the lockrepo/unlockrepo REST calls the sync makes +/// against the Send/Receive server to exclude other clients from pushing concurrently). +/// Different mechanism, different scope (this process vs. all clients of a shared repo), different +/// failure modes. Do not conflate the two. +/// +/// +/// Inert in open-source Platform.Bible. Nothing in public core ever calls +/// , so the gate is never armed, always returns +/// false, every scope succeeds, and no write is ever rejected — +/// public behavior is unchanged by this class. The Paratext 10 Studio closed-source patch brackets +/// each automatic sync with / (Jira PT-4210), +/// which is what activates the gate. This class is the public seam; the activation lives in the +/// patch. +/// +/// +internal static class SendReceiveWriteLock +{ + // The exact suffix on every rejection message. The Paratext 10 Studio Scripture editor matches + // this sentinel to show an "editing paused during Send/Receive" notification (rather than the + // generic permissions message) and revert the un-saved change. + public const string EditBlockedSentinel = "(SR_EDIT_BLOCKED)"; + + // 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 + // same word so the check-and-disarm stays one CAS. Token 0 is never issued (it is skipped on + // wrap) so callers can safely treat default(long) as "no arm"; at 30 bits a stale token could + // only false-match after ~10^9 intervening arms. Mutate ONLY via Interlocked operations; read + // via Volatile.Read — it gives acquire ordering, and the BCL implements the long overload with + // an interlocked read on 32-bit runtimes, so (unlike a plain long read, which can tear there) + // it is also atomic. The shipping targets are 64-bit; that is defense for ports. + private const long ArmedFlag = 1L << 32; + private const long InFlightCountMask = 0xFFFF_FFFFL; + private const int GenerationShift = 33; + private const long GenerationMask = 0x3FFF_FFFFL << GenerationShift; + private static long _state; + + private static long CountOf(long state) => state & InFlightCountMask; + + private static long GenerationOf(long state) => (state & GenerationMask) >> GenerationShift; + + // Cached empty set: the non-default comparer defeats ImmutableHashSet's Empty singleton, so a + // computed property would allocate on every access. MUST stay declared ABOVE _blockedProjectIds + // (static field initializers run in textual order, and _blockedProjectIds reads this one — the + // other way around it would silently initialize to null). + private static readonly IImmutableSet EmptyProjectIds = ImmutableHashSet.Create( + StringComparer.OrdinalIgnoreCase + ); + + // Pure data (no synchronization role — the atomic word does all exclusion): drives IsBlocked + // per-project queries. Published (volatile) before the armed flag is set and replaced wholesale, + // never mutated. Case-insensitive because project ID casing varies across call sites. + private static volatile IImmutableSet _blockedProjectIds = EmptyProjectIds; + + // 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 + // and proceed (degraded path), accepting a small residual race rather than hanging the sync + // forever. Internal setter exists ONLY so tests can shorten it; production always uses the + // default. + internal static TimeSpan DrainTimeout { get; set; } = TimeSpan.FromSeconds(10); + + /// Whether a sync is currently armed. For tests only. + internal static bool IsArmed => (Volatile.Read(ref _state) & ArmedFlag) != 0; + + /// The number of write scopes currently open. For tests only. + internal static long InFlightWriteCount => CountOf(Volatile.Read(ref _state)); + + /// The armed project-id set exactly as stored. For tests only. + internal static IImmutableSet ArmedProjectIds => _blockedProjectIds; + + /// The largest value the arm generation can hold before wrapping. For tests only + /// (lets a test park the generation at the wrap boundary via + /// ). + internal static long MaxGeneration => GenerationMask >> GenerationShift; + + /// + /// Resets ALL gate state, INCLUDING the in-flight count that deliberately + /// leaves alone. For tests only: contains the blast radius of a test that leaked a write scope + /// (otherwise every later in the run would burn its full + /// ). Optionally seeds the arm generation (e.g. to + /// , to exercise the wrap). Never call in production — a straggler + /// scope disposed after this reset would decrement a newer scope's count. + /// + internal static void ResetForTests(long generation = 0) + { + _blockedProjectIds = EmptyProjectIds; + Volatile.Write(ref _state, (generation << GenerationShift) & GenerationMask); + } + + private static InvalidOperationException EditBlocked(string projectId) => + new( + $"Cannot write to project '{projectId}' while an automatic Send/Receive is in " + + $"progress. {EditBlockedSentinel}" + ); + + /// + /// 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 + /// (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). + /// + /// 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 + /// the returned token — a stale token is a logged no-op, so a late Clear can never disarm a + /// newer sync's arm. Do not call while holding an open scope this + /// call would wait for — the drain cannot finish and will time out into the degraded path. + /// Overlapping sync brackets are still not meaningful (see the class remarks); the scheduler + /// must serialize sync runs. + /// + /// + /// The full set of project ids in this sync batch (null or empty + /// entries are ignored). + /// When true, a drain timeout aborts the sync start + /// instead of degrading: this arm is rolled back (nothing stays armed on this bracket's + /// behalf, so writes flow again and the caller has NO cleanup obligation) and a + /// is thrown — the sync must not proceed; the caller decides + /// whether to retry, defer, or notify. When false (the default), the degraded path + /// applies (see the class remarks). This affects ONLY the drain-timeout outcome — an arm + /// ended or replaced mid-drain still returns normally with a logged warning. + /// The arm token identifying this bracket; pass it to . + /// The drain timed out and + /// was true; the arm has been rolled back. + /// + public static long SetSyncing(IEnumerable projectIds, bool throwOnDrainTimeout = false) + { + ArgumentNullException.ThrowIfNull(projectIds); + + // Build the set BEFORE touching any state, so an exception while enumerating (or a + // defective batch) can never leave a torn arm. + var armedProjectIds = projectIds + .Where(projectId => !string.IsNullOrEmpty(projectId)) + .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. + 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." + ); + + // Publish the pure data first, then arm. Arming is a single CAS on the state word that + // sets the flag AND advances the generation (the returned token): every write that enters + // afterward must observe the flag (all transitions on the word are totally ordered) and is + // rejected, and every earlier token goes stale in the same atomic step. Token 0 is skipped + // on wrap — it stays reserved as a natural "no arm" default for callers. + _blockedProjectIds = armedProjectIds; + long token; + while (true) + { + long state = Volatile.Read(ref _state); + token = (GenerationOf(state) + 1) & (GenerationMask >> GenerationShift); + if (token == 0) + token = 1; + long armedState = CountOf(state) | ArmedFlag | (token << GenerationShift); + if (Interlocked.CompareExchange(ref _state, armedState, state) == state) + break; + } + + // 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 + // the whole timeout for nothing). Poll with SpinWait.SpinOnce(), which escalates + // spin → yield → Sleep(1): after the first ~20 iterations the loop wakes ~once per ms and + // uses negligible CPU. (Deliberately NOT SpinWait.SpinUntil — it never escalates to + // Sleep(1), so it would busy-burn a core for the whole drain.) + var spinner = new SpinWait(); + var stopwatch = Stopwatch.StartNew(); + while (true) + { + long current = Volatile.Read(ref _state); + if (CountOf(current) == 0 || (current & ArmedFlag) == 0) + break; + if (stopwatch.Elapsed >= DrainTimeout) + break; + spinner.SpinOnce(); + } + + // Triage on a fresh read (the authority — the state may have settled between the last poll + // and here). + long observed = Volatile.Read(ref _state); + if ((observed & ArmedFlag) == 0 || GenerationOf(observed) != token) + { + Console.Error.WriteLine( + "[SendReceiveWriteLock] Warning: this arm was ended (Clear) or replaced (a newer " + + "SetSyncing) while its drain was still waiting; proceeding, but the gate is " + + "no longer rejecting writes on behalf of THIS sync." + ); + return token; + } + long stillInFlight = CountOf(observed); + if (stillInFlight != 0) + { + string drainFailure = + $"{stillInFlight} in-flight project write(s) did not drain within " + + $"{DrainTimeout.TotalSeconds:0.#}s"; + if (throwOnDrainTimeout) + { + // Roll this arm back BEFORE throwing (own-token disarm — a safe no-op if something + // else already took or cleared the slot in the meantime), so a throw always means + // "nothing armed on this bracket's behalf, writes flow again, no cleanup owed". + Clear(token); + throw new TimeoutException( + $"{drainFailure}; the Send/Receive arm was rolled back and the sync must " + + "not proceed." + ); + } + + // Degraded: proceed rather than deadlock the sync. The armed flag keeps rejecting + // new writes; only the already-stuck write(s) we couldn't drain may still overlap. + Console.Error.WriteLine( + $"[SendReceiveWriteLock] Warning: {drainFailure}; proceeding with Send/Receive " + + "anyway (new writes stay rejected while the sync is armed)." + ); + } + return token; + } + + /// + /// Ends the sync UNCONDITIONALLY: disarms the gate (whichever bracket armed it) and clears the + /// armed project set, so writes are accepted again. May be called from ANY thread, is + /// idempotent, and is safe to call when no sync is active at all — this is the force/crash + /// recovery path (e.g. a watchdog, or the next sync finding a dead worker's arm still set). + /// Normal bracket code should prefer , which cannot disarm a sync it + /// does not own. + /// + public static void Clear() + { + _blockedProjectIds = EmptyProjectIds; + Interlocked.And(ref _state, ~ArmedFlag); + } + + /// + /// Ends the sync bracket identified by (returned by + /// ): disarms only if that bracket still owns the sync slot. A stale + /// token — a newer has taken the slot — is a logged no-op, so a late + /// or duplicate Clear can never disarm a newer sync's arm. When nothing is armed at all this + /// is a silent no-op (idempotent). Runs on any thread. + /// + public static void Clear(long token) + { + while (true) + { + long state = Volatile.Read(ref _state); + if ((state & ArmedFlag) == 0) + return; // Idempotent: nothing armed (this bracket was already cleared). + if (GenerationOf(state) != token) + { + Console.Error.WriteLine( + "[SendReceiveWriteLock] Warning: Clear(token) ignored a stale token — a newer " + + "SetSyncing owns the sync slot (late Clear from an overlapping bracket?)." + ); + return; + } + if (Interlocked.CompareExchange(ref _state, state & ~ArmedFlag, state) == state) + { + // The disarm won atomically against the token check; now drop the pure-data set. + _blockedProjectIds = EmptyProjectIds; + return; + } + } + } + + /// + /// 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). + /// + public static bool IsBlocked(string? projectId) + { + return !string.IsNullOrEmpty(projectId) && _blockedProjectIds.Contains(projectId); + } + + /// + /// Opens a write scope for . Use as the FIRST statement of any + /// 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). + /// + /// 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. + /// + /// 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). + /// + 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. + while (true) + { + long state = Volatile.Read(ref _state); + if ((state & ArmedFlag) != 0) + throw EditBlocked(projectId); + if (Interlocked.CompareExchange(ref _state, state + 1, state) == state) + return new WriteScope(); + } + } + + /// + /// Closes a write scope: atomically decrements the in-flight count. Guarded against underflow + /// (a release with no matching open scope is logged and ignored) so no caller bug can corrupt + /// the armed flag stored in the same word. + /// + private static void ExitWrite() + { + while (true) + { + long state = Volatile.Read(ref _state); + if (CountOf(state) == 0) + { + Console.Error.WriteLine( + "[SendReceiveWriteLock] Error: a write scope was released with no write " + + "in flight (unbalanced Dispose); ignoring." + ); + return; + } + if (Interlocked.CompareExchange(ref _state, state - 1, state) == state) + return; + } + } + + /// + /// The disposable returned by . Releases its in-flight count exactly + /// once, from whatever thread disposes it (an interlocked guard makes cross-thread double- + /// dispose safe). + /// + private sealed class WriteScope : IDisposable + { + private int _disposed; + + public void Dispose() + { + if (Interlocked.Exchange(ref _disposed, 1) != 0) + return; + ExitWrite(); + } + } +} diff --git a/extensions/src/platform-scripture-editor/contributions/localizedStrings.json b/extensions/src/platform-scripture-editor/contributions/localizedStrings.json index e8809b0af26..006ce56a89c 100644 --- a/extensions/src/platform-scripture-editor/contributions/localizedStrings.json +++ b/extensions/src/platform-scripture-editor/contributions/localizedStrings.json @@ -79,6 +79,7 @@ "%webView_platformScriptureEditor_error_noTextSelected%": "Please select a range of text before inserting a comment.", "%webView_platformScriptureEditor_error_selectionContainsMarkers%": "Invalid selection. Selected text must be a simple word or phrase with no markers.", "%webView_platformScriptureEditor_error_structureProtected%": "Structure is locked. Paragraph and verse markers cannot be changed.", + "%webView_platformScriptureEditor_error_syncEditBlocked%": "Editing is paused while an automatic Send/Receive is in progress. Your change was not saved.", "%webView_platformScriptureEditor_info%": "Info", "%webView_platformScriptureEditor_insert%": "Insert", "%webView_platformScriptureEditor_insertCommentAtSelection%": "Insert comment", @@ -105,6 +106,7 @@ "%webView_platformScriptureEditor_structureProtection_unlockStructure%": "Unlock structure", "%webView_platformScriptureEditor_structureProtection_unlockStructureForProject%": "Unlock structure for project", "%webView_platformScriptureEditor_switchScriptureView%": "Switch Scripture view", + "%webView_platformScriptureEditor_syncEditBlocked_banner%": "Editing paused — Send/Receive in progress", "%webView_platformScriptureEditor_textColor%": "Text Color", "%webView_platformScriptureEditor_thickBorders%": "Thick Borders", "%webView_platformScriptureEditor_title_editable_indicator%": "(Editable)", @@ -220,6 +222,7 @@ "%webView_platformScriptureEditor_error_noTextSelected%": "Por favor, seleccione un rango de texto antes de insertar un comentario.", "%webView_platformScriptureEditor_error_selectionContainsMarkers%": "Selección inválida. El texto seleccionado debe ser una palabra o frase simple sin marcadores.", "%webView_platformScriptureEditor_error_structureProtected%": "La estructura está bloqueada. Los marcadores de párrafo y versículo no se pueden cambiar.", + "%webView_platformScriptureEditor_error_syncEditBlocked%": "La edición está pausada mientras se realiza un Enviar/Recibir automático. Su cambio no se guardó.", "%webView_platformScriptureEditor_info%": "Información", "%webView_platformScriptureEditor_insert%": "Insertar", "%webView_platformScriptureEditor_insertCommentAtSelection%": "Insertar comentario", @@ -246,6 +249,7 @@ "%webView_platformScriptureEditor_structureProtection_unlockStructure%": "Desbloquear estructura", "%webView_platformScriptureEditor_structureProtection_unlockStructureForProject%": "Desbloquear estructura para el proyecto", "%webView_platformScriptureEditor_switchScriptureView%": "Cambiar vista de escrituras", + "%webView_platformScriptureEditor_syncEditBlocked_banner%": "Edición pausada — Enviar/Recibir en curso", "%webView_platformScriptureEditor_textColor%": "Color del texto", "%webView_platformScriptureEditor_thickBorders%": "Bordes gruesos", "%webView_platformScriptureEditor_title_editable_indicator%": "(Editable)", diff --git a/extensions/src/platform-scripture-editor/src/main.ts b/extensions/src/platform-scripture-editor/src/main.ts index 08dc4cf0897..aa87e57f6fe 100644 --- a/extensions/src/platform-scripture-editor/src/main.ts +++ b/extensions/src/platform-scripture-editor/src/main.ts @@ -510,6 +510,10 @@ class ScriptureEditorWebViewFactory extends WebViewFactory item[1].description) @@ -160,6 +165,7 @@ const EDITOR_LOCALIZED_STRINGS: LocalizeKey[] = [ '%webView_platformScriptureEditor_error_bookNotFoundResource%', '%webView_platformScriptureEditor_emptyState_noProject%', '%webView_platformScriptureEditor_error_permissions_format%', + '%webView_platformScriptureEditor_error_syncEditBlocked%', '%webView_platformScriptureEditor_error_noTextSelected%', '%webView_platformScriptureEditor_error_selectionContainsMarkers%', '%webView_platformScriptureEditor_paragraphSelection_protectedTooltip%', @@ -240,6 +246,10 @@ const bookNotFoundRegex = /Book number \d+ not found in project/; // This regex is connected directly to the exception message within PermissionsException.cs const PERMISSIONS_EXCEPTION_REGEX = /Permissions exception for projectId/; +// Sentinel appended by the backend write-gate (SendReceiveWriteLock in paranext-core's c-sharp) +// when a project write is rejected because an automatic Send/Receive is syncing that project. +const SYNC_EDIT_BLOCKED_REGEX = /\(SR_EDIT_BLOCKED\)/; + /** * Corrects editor USJ version from 3.1 to 3.0. Returns a shallow clone of the object passed in. * @@ -337,6 +347,12 @@ globalThis.webViewComponent = function PlatformScriptureEditor({ const currentSelectionRef = useRef(undefined); const [isReadOnly] = useWebViewState('isReadOnly', true); + // Set by the core auto-sync edit-block driver while an automatic (scheduled) Send/Receive is + // syncing this project: editing is frozen (folded into isReadOnlyEffective below) and a slim + // banner is shown, but the rest of the UI stays usable. Always defaults false; the web-view + // factory forces it back to false when rebuilding saved state so a crash mid-sync can't persist + // it (see main.ts). + const [isSyncBlocked] = useWebViewState('isSyncBlocked', false); const [decorations, setDecorations] = useWebViewState( 'decorations', defaultEditorDecorations, @@ -445,24 +461,27 @@ globalThis.webViewComponent = function PlatformScriptureEditor({ const nodeOptions = useMemo( () => ({ - noteCallerOnClick: isReadOnly - ? undefined - : (event, noteNodeKey, isCollapsed, _getCaller, _setCaller, getNoteOps) => { - if (!isCollapsed || editingNoteKey.current) return; - - const noteOp = getNoteOps()?.at(0); - if (!noteOp || !isInsertEmbedOpOfType('note', noteOp)) return; - - const targetRect = event.currentTarget.getBoundingClientRect(); - setNotePopoverAnchorX(targetRect.left); - setNotePopoverAnchorY(targetRect.top); - setNotePopoverAnchorHeight(targetRect.height); - editingNoteKey.current = noteNodeKey; - editingNoteOps.current = [noteOp]; - setShowFootnoteEditor(true); - }, + // Also disabled while sync-blocked: opening a note caller can create/edit a note, which is a + // project write that must be frozen during an automatic Send/Receive. + noteCallerOnClick: + isReadOnly || isSyncBlocked + ? undefined + : (event, noteNodeKey, isCollapsed, _getCaller, _setCaller, getNoteOps) => { + if (!isCollapsed || editingNoteKey.current) return; + + const noteOp = getNoteOps()?.at(0); + if (!noteOp || !isInsertEmbedOpOfType('note', noteOp)) return; + + const targetRect = event.currentTarget.getBoundingClientRect(); + setNotePopoverAnchorX(targetRect.left); + setNotePopoverAnchorY(targetRect.top); + setNotePopoverAnchorHeight(targetRect.height); + editingNoteKey.current = noteNodeKey; + editingNoteOps.current = [noteOp]; + setShowFootnoteEditor(true); + }, }), - [isReadOnly, editingNoteKey], + [isReadOnly, isSyncBlocked, editingNoteKey], ); /** @@ -473,8 +492,10 @@ globalThis.webViewComponent = function PlatformScriptureEditor({ const isReadOnlyEffective = useMemo( () => isReadOnly || + // An automatic Send/Receive is syncing this project — freeze editing until it finishes. + isSyncBlocked || (viewType === 'markers' && localStorage.getItem('dev-editableMarkersView') !== 'true'), - [isReadOnly, viewType], + [isReadOnly, isSyncBlocked, viewType], ); // Effective structure-protection state for this project/user, used to gate keyboard edits to @@ -571,6 +592,24 @@ globalThis.webViewComponent = function PlatformScriptureEditor({ [], ); + /** + * Show the standard "editing paused during Send/Receive" warning notification (the + * `(SR_EDIT_BLOCKED)` gate rejection surfaced to the user). Extracted so the severity/message + * cannot drift across the several call sites that report it, and self-catching so fire-and-forget + * callers (the ones that cannot `await`) never surface an unhandled promise rejection from the + * notification service. + */ + const notifySyncEditBlocked = useCallback(async () => { + try { + await papi.notifications.send({ + severity: 'warning', + message: localizedStrings['%webView_platformScriptureEditor_error_syncEditBlocked%'], + }); + } catch (e) { + logger.warn(`Failed to send the sync-edit-blocked notification: ${getErrorMessage(e)}`); + } + }, [localizedStrings]); + const paragraphSwitcherMenuItems = useMemo( () => generateParagraphMenuListItems( @@ -587,7 +626,16 @@ globalThis.webViewComponent = function PlatformScriptureEditor({ const insertCommentAtCurrentSelection = useCallback(() => { const selection = currentSelectionRef.current; + // Comment creation is gated by canUserCreateComments (not read-only), so it must be blocked + // separately during an automatic Send/Receive. Guarding here covers both the hotkey and the + // context-menu item's onSelect. if (!selection?.start || !canUserCreateComments) return; + // The context-menu item is visibly disabled while sync-blocked, but the hotkey reaches here + // directly — show the same "editing paused" notice instead of silently no-op'ing. + if (isSyncBlocked) { + notifySyncEditBlocked(); + return; + } // Store the selection as annotation range to show it as the pending annotation const annotationRange: AnnotationRange = { @@ -712,7 +760,7 @@ globalThis.webViewComponent = function PlatformScriptureEditor({ } setShowCommentEditor(true); - }, [scrRef, canUserCreateComments]); + }, [scrRef, canUserCreateComments, isSyncBlocked, notifySyncEditBlocked]); const options = useMemo( () => ({ @@ -728,7 +776,8 @@ globalThis.webViewComponent = function PlatformScriptureEditor({ { title: localizedStrings['%webView_platformScriptureEditor_insertCommentAtSelection%'], onSelect: insertCommentAtCurrentSelection, - isDisabled: !canUserCreateComments, + // Disabled while sync-blocked too, so the menu reflects the frozen state. + isDisabled: !canUserCreateComments || isSyncBlocked, }, ], }), @@ -736,6 +785,7 @@ globalThis.webViewComponent = function PlatformScriptureEditor({ isReadOnlyEffective, isStructureProtected, canUserCreateComments, + isSyncBlocked, textDirectionEffective, nodeOptions, viewOptions, @@ -1310,34 +1360,41 @@ globalThis.webViewComponent = function PlatformScriptureEditor({ logger.error(`Error saving USJ to PDP: ${errorMessage}`); currentlyWritingUsjToPdp.current = false; - if (!PERMISSIONS_EXCEPTION_REGEX.test(errorMessage)) return; + // Two recoverable backend rejections revert the editor to the last PDP state and notify; + // only the message differs. A sync-edit-block is expected/transient (editing paused during + // an automatic Send/Receive), so it is a warning; a permissions failure is an error. + const isSyncEditBlocked = SYNC_EDIT_BLOCKED_REGEX.test(errorMessage); + const isPermissionsError = PERMISSIONS_EXCEPTION_REGEX.test(errorMessage); + if (!isSyncEditBlocked && !isPermissionsError) return; - // The error is due to a permissions issue, so make a notification to inform the user and - // reset the text to how it was before try { if (usjFromPdp && editorRef.current) { usjSentToPdp.current = usjFromPdp; setEditorUsj.current(usjFromPdp); } - await papi.notifications.send({ - severity: 'error', - message: formatReplacementString( - localizedStrings['%webView_platformScriptureEditor_error_permissions_format%'], - { projectName }, - ), - }); + if (isSyncEditBlocked) { + await notifySyncEditBlocked(); + } else { + await papi.notifications.send({ + severity: 'error', + message: formatReplacementString( + localizedStrings['%webView_platformScriptureEditor_error_permissions_format%'], + { projectName }, + ), + }); + } } catch (innerError) { logger.error( - `Error handling permissions exception when saving USJ to PDP: ${getErrorMessage( - innerError, - )}`, + `Error handling ${ + isSyncEditBlocked ? 'sync-edit-block' : 'permissions' + } exception when saving USJ to PDP: ${getErrorMessage(innerError)}`, ); } } } return saveUsjToPdpIfUpdatedInternal; - }, [usjFromPdp, projectName, localizedStrings, projectId]); + }, [usjFromPdp, projectName, localizedStrings, projectId, notifySyncEditBlocked]); /** * Close the footnote editor, optionally deleting the note from the main editor first. Pass @@ -1553,6 +1610,13 @@ globalThis.webViewComponent = function PlatformScriptureEditor({ logger.warn('Cannot create comment: no projectId'); return; } + // A comment popover opened before the block began is not closed by it, so Save must be + // guarded here too. Early-return keeps the popover open (the user's text isn't lost — they + // can save once the sync finishes) and warns like the scripture-edit path. + if (isSyncBlocked) { + await notifySyncEditBlocked(); + return; + } const capturedSelection = pendingCommentAnnotationRange.current; @@ -1606,12 +1670,29 @@ globalThis.webViewComponent = function PlatformScriptureEditor({ if (assignedUser !== undefined) setLastAssignedUser(assignedUser); setShowCommentEditor(false); } catch (error) { - logger.error(`Error creating comment: ${getErrorMessage(error)}`); + const errorMessage = getErrorMessage(error); + logger.error(`Error creating comment: ${errorMessage}`); + // A sync started in the window between the isSyncBlocked guard above and the backend + // write, and the backend gate rejected the comment. Warn like the scripture-edit path and + // discard the pending state (close the popover, clear the pending highlight) — the + // rejected comment cannot be saved as-is. + if (SYNC_EDIT_BLOCKED_REGEX.test(errorMessage)) { + await notifySyncEditBlocked(); + onCommentEditorCancel(); + } } finally { isSubmittingComment.current = false; } }, - [projectId, scrRef, createCommentAnnotationClickHandler, webViewId], + [ + projectId, + scrRef, + createCommentAnnotationClickHandler, + webViewId, + isSyncBlocked, + notifySyncEditBlocked, + onCommentEditorCancel, + ], ); // Clear annotation info when the editor clears annotations internally @@ -1877,6 +1958,9 @@ globalThis.webViewComponent = function PlatformScriptureEditor({ } /> + {/* Slim, non-covering banner while an automatic Send/Receive freezes editing. Shown only when + sync-blocked and not genuinely read-only (a real viewer shouldn't say "editing paused"). */} + {isSyncBlocked && !isReadOnly && } {/* Mount the editor in a reverse portal so it doesn't unmount and lose its internal state */} {renderEditor()} diff --git a/extensions/src/platform-scripture-editor/src/sync-blocked-banner.component.tsx b/extensions/src/platform-scripture-editor/src/sync-blocked-banner.component.tsx new file mode 100644 index 00000000000..12656b9653e --- /dev/null +++ b/extensions/src/platform-scripture-editor/src/sync-blocked-banner.component.tsx @@ -0,0 +1,96 @@ +import papi, { logger } from '@papi/frontend'; +import { Button, Progress, useEvent } from 'platform-bible-react'; +import { getErrorMessage, LanguageStrings, LocalizeKey } from 'platform-bible-utils'; +import { useCallback, useState } from 'react'; + +/** Banner text plus the reused general Cancel label. */ +export const SYNC_BLOCKED_BANNER_STRING_KEYS: LocalizeKey[] = [ + '%webView_platformScriptureEditor_syncEditBlocked_banner%', + '%general_cancel%', +]; + +const BANNER_TEXT_KEY: LocalizeKey = '%webView_platformScriptureEditor_syncEditBlocked_banner%'; +const CANCEL_KEY: LocalizeKey = '%general_cancel%'; + +/** + * Payload of the Send/Receive extension's sync progress event (the same public seam the old + * full-workspace overlay used). + */ +type SyncProgressEvent = { + /** + * For determinate progress, the bare current item (e.g. a project name); for indeterminate + * progress, a complete localized message shown verbatim. + */ + progressText: string; + /** Progress fraction 0–1 for determinate progress; null/undefined means indeterminate. */ + progressValue?: number | null; +}; + +type Props = { + /** Localized strings from the editor (must include the keys in SYNC_BLOCKED_BANNER_STRING_KEYS). */ + localizedStrings: LanguageStrings; +}; + +/** + * Slim, non-covering banner shown across the top of the Scripture editor pane while an automatic + * (scheduled) Send/Receive is syncing this project. It states that editing is paused, shows live + * sync progress, and offers a single-shot Cancel that requests the Send/Receive extension to cancel + * the running sync (the bottom-right progress toast has its own Cancel too). + * + * This mounts only while blocking (the editor renders it conditionally), so its progress + * subscription and single-shot Cancel state reset cleanly for each blocking episode — no explicit + * teardown/re-arm-on-clear is needed, unlike the old always-mounted overlay. + */ +export function SyncBlockedBanner({ localizedStrings }: Props) { + const [progress, setProgress] = useState<{ text: string; value?: number } | undefined>(undefined); + const [isCancelEnabled, setIsCancelEnabled] = useState(true); + + // Live sync progress. Typed getNetworkEvent so the handler is strongly typed. Normalize the + // payload's null (indeterminate) to undefined at this boundary (repo style forbids null). + useEvent( + papi.network.getNetworkEvent('paratextBibleSendReceive.onSyncProgress'), + useCallback((event: SyncProgressEvent) => { + setProgress({ text: event.progressText, value: event.progressValue ?? undefined }); + }, []), + ); + + const handleCancel = useCallback(() => { + // Single-shot: one cancel request per blocking episode. Disable immediately; re-enable only if + // the request is rejected (the sync is still running, so the user can retry). + setIsCancelEnabled(false); + papi.commands.sendCommand('paratextBibleSendReceive.cancelSync').catch((e) => { + logger.warn(`Sync-blocked banner failed to cancel sync: ${getErrorMessage(e)}`); + setIsCancelEnabled(true); + }); + }, []); + + return ( +
+
+ {localizedStrings[BANNER_TEXT_KEY]} + {progress?.text && ( + {progress.text} + )} + {/* platform-bible-react's Progress has no indeterminate visual, so the bar only appears for + determinate progress; indeterminate progress is shown as text only. */} + {progress?.value !== undefined && ( + + )} + +
+
+ ); +} + +export default SyncBlockedBanner; diff --git a/extensions/src/platform-scripture/contributions/localizedStrings.json b/extensions/src/platform-scripture/contributions/localizedStrings.json index 534cbbd2f03..14683a6f40b 100644 --- a/extensions/src/platform-scripture/contributions/localizedStrings.json +++ b/extensions/src/platform-scripture/contributions/localizedStrings.json @@ -500,6 +500,7 @@ "%webView_inventory_unknown%": "Unknown items", "%webView_inventory_unknown_marker%": "Unknown Marker", "%webView_markersInventory_title%": "Markers Inventory: {projectName}", + "%webView_platformScripture_error_syncEditBlocked%": "Editing is paused while an automatic Send/Receive is in progress. Your change was not saved.", "%webView_platformScripture_showCheckResults%": "Show Check Results...", "%webView_platformScripture_tools%": "Tools", "%webView_punctuationInventory_title%": "Punctuation Inventory: {projectName}", @@ -755,6 +756,7 @@ "%webView_inventory_unknown%": "Elementos desconocidos", "%webView_inventory_unknown_marker%": "Marcador desconocido", "%webView_markersInventory_title%": "Inventario de marcadores: {projectName}", + "%webView_platformScripture_error_syncEditBlocked%": "La edición está pausada mientras se realiza un Enviar/Recibir automático. Su cambio no se guardó.", "%webView_platformScripture_showCheckResults%": "Mostrar resultados de verificación...", "%webView_platformScripture_tools%": "Herramientas", "%webView_repeatedWordsInventory_title%": "Inventario de palabras repetidas: {projectName}", diff --git a/extensions/src/platform-scripture/src/checks-side-panel.web-view.tsx b/extensions/src/platform-scripture/src/checks-side-panel.web-view.tsx index 2b54939d1f0..55a8498b315 100644 --- a/extensions/src/platform-scripture/src/checks-side-panel.web-view.tsx +++ b/extensions/src/platform-scripture/src/checks-side-panel.web-view.tsx @@ -34,6 +34,7 @@ import { CHECKS_SIDE_PANEL_STRING_KEYS, } from './checks/checks-side-panel/checks-side-panel.component'; import { useOpenProjectTabs } from './hooks/use-open-project-tabs'; +import { SYNC_EDIT_BLOCKED_MESSAGE_KEY, isSyncEditBlockedError } from './sync-edit-blocked.util'; /** * Gets the short and full names of a project from its ID. Kept in the webview (not the shared, @@ -614,14 +615,26 @@ global.webViewComponent = function ChecksSidePanelWebView({ async (result: CheckRunResult) => { if (!result || !result.checkId || !projectId || !checkAggregator) return false; - const denyResultSuccess = await checkAggregator.denyCheckResult( - result.checkId, - result.checkResultType, - projectId, - result.verseRef, - result.itemText, - result.checkResultUniqueId, - ); + let denyResultSuccess: boolean; + try { + denyResultSuccess = await checkAggregator.denyCheckResult( + result.checkId, + result.checkResultType, + projectId, + result.verseRef, + result.itemText, + result.checkResultUniqueId, + ); + } catch (error) { + // The deny/allow buttons are fire-and-forget, so without this catch a write-gate rejection + // during an automatic Send/Receive becomes an unhandled promise rejection with no UI. Show + // the shared "editing paused" warning; re-throw anything else so real errors still surface. + if (isSyncEditBlockedError(error)) { + papi.notifications.send({ message: SYNC_EDIT_BLOCKED_MESSAGE_KEY, severity: 'warning' }); + return false; + } + throw error; + } if (!isMountedRef.current) return false; if (denyResultSuccess) setDeniedStatusForResult(result, true); else logger.debug(`Could not deny check result: ${JSON.stringify(result)}`); @@ -634,14 +647,26 @@ global.webViewComponent = function ChecksSidePanelWebView({ async (result: CheckRunResult) => { if (!result || !result.checkId || !projectId || !checkAggregator) return false; - const allowResultStatus = await checkAggregator.allowCheckResult( - result.checkId, - result.checkResultType, - projectId, - result.verseRef, - result.itemText, - result.checkResultUniqueId, - ); + let allowResultStatus: boolean; + try { + allowResultStatus = await checkAggregator.allowCheckResult( + result.checkId, + result.checkResultType, + projectId, + result.verseRef, + result.itemText, + result.checkResultUniqueId, + ); + } catch (error) { + // The deny/allow buttons are fire-and-forget, so without this catch a write-gate rejection + // during an automatic Send/Receive becomes an unhandled promise rejection with no UI. Show + // the shared "editing paused" warning; re-throw anything else so real errors still surface. + if (isSyncEditBlockedError(error)) { + papi.notifications.send({ message: SYNC_EDIT_BLOCKED_MESSAGE_KEY, severity: 'warning' }); + return false; + } + throw error; + } if (!isMountedRef.current) return false; if (allowResultStatus) setDeniedStatusForResult(result, false); else logger.debug(`Could not allow check result: ${JSON.stringify(result)}`); diff --git a/extensions/src/platform-scripture/src/inventory.web-view.tsx b/extensions/src/platform-scripture/src/inventory.web-view.tsx index 19e28f53466..3967144b300 100644 --- a/extensions/src/platform-scripture/src/inventory.web-view.tsx +++ b/extensions/src/platform-scripture/src/inventory.web-view.tsx @@ -1,5 +1,5 @@ import { WebViewProps } from '@papi/core'; -import { logger } from '@papi/frontend'; +import papi, { logger } from '@papi/frontend'; import { useLocalizedStrings, useProjectData, useProjectSetting } from '@papi/frontend/react'; import { Canon, SerializedVerseRef } from '@sillsdev/scripture'; import { INVENTORY_STRING_KEYS, Scope } from 'platform-bible-react'; @@ -23,6 +23,7 @@ import { REPEATED_WORDS_INVENTORY_STRING_KEYS, } from './checks/inventories/repeated-words-inventory.component'; import { useInventory } from './hooks/use-inventory'; +import { SYNC_EDIT_BLOCKED_MESSAGE_KEY, isSyncEditBlockedError } from './sync-edit-blocked.util'; const VALID_ITEMS_DEFAULT = ''; const INVALID_ITEMS_DEFAULT = ''; @@ -314,6 +315,12 @@ global.webViewComponent = function InventoryWebView({ try { await setValidItems?.(items.join(' ')); } catch (error) { + // The write-gate rejects this settings write while an automatic Send/Receive is syncing. + // Show the shared "editing paused" warning instead of only logging (which is invisible). + if (isSyncEditBlockedError(error)) { + papi.notifications.send({ message: SYNC_EDIT_BLOCKED_MESSAGE_KEY, severity: 'warning' }); + return; + } logger.error(`Error updating approved items: ${getErrorMessage(error)}`); } }, @@ -326,6 +333,12 @@ global.webViewComponent = function InventoryWebView({ try { await setInvalidItems?.(items.join(' ')); } catch (error) { + // The write-gate rejects this settings write while an automatic Send/Receive is syncing. + // Show the shared "editing paused" warning instead of only logging (which is invisible). + if (isSyncEditBlockedError(error)) { + papi.notifications.send({ message: SYNC_EDIT_BLOCKED_MESSAGE_KEY, severity: 'warning' }); + return; + } logger.error(`Error updating unapproved items: ${getErrorMessage(error)}`); } }, diff --git a/extensions/src/platform-scripture/src/manage-books.web-view.tsx b/extensions/src/platform-scripture/src/manage-books.web-view.tsx index 6b8bdfd86b8..c110278a23f 100644 --- a/extensions/src/platform-scripture/src/manage-books.web-view.tsx +++ b/extensions/src/platform-scripture/src/manage-books.web-view.tsx @@ -49,6 +49,7 @@ import { GreekEstherTemplatePicker, GreekEstherTemplatePickerLocalizedStrings, } from './greek-esther-template-picker.component'; +import { SYNC_EDIT_BLOCKED_MESSAGE_KEY, isSyncEditBlockedError } from './sync-edit-blocked.util'; const NETWORK_OBJECT_ID = 'platformScripture.manageBooks'; const BOOKS_PRESENT_DEFAULT = '0'.repeat(123); @@ -865,11 +866,20 @@ global.webViewComponent = function ManageBooksWebView({ const onMutationResult = useCallback((result: MutationResult) => { const entries: AlertEntry[] = [...result.errors, ...result.warnings]; entries.forEach((entry) => { - const message = entry.caption ? `${entry.caption}: ${entry.text}` : entry.text; try { // notificationService is exposed on @papi/frontend; per - // ui-spec-manage-books.md:118 toasts are the canonical surface. - papi.notifications.send({ message, severity: alertLevelToSeverity(entry.level) }); + // ui-spec-manage-books.md:118 toasts are the canonical surface. A write-gate rejection + // reaches here as an error entry whose text carries the `(SR_EDIT_BLOCKED)` sentinel (the + // dialog turns a thrown backend error into a MutationResult error) — show the shared + // "editing paused" warning instead of leaking the raw technical message at error severity. + papi.notifications.send( + isSyncEditBlockedError(entry.text) + ? { message: SYNC_EDIT_BLOCKED_MESSAGE_KEY, severity: 'warning' } + : { + message: entry.caption ? `${entry.caption}: ${entry.text}` : entry.text, + severity: alertLevelToSeverity(entry.level), + }, + ); } catch (e) { logger.warn( `manage-books: notifications.send failed for AlertEntry: ${e instanceof Error ? e.message : String(e)}`, diff --git a/extensions/src/platform-scripture/src/sync-edit-blocked.util.ts b/extensions/src/platform-scripture/src/sync-edit-blocked.util.ts new file mode 100644 index 00000000000..f2f98ba1393 --- /dev/null +++ b/extensions/src/platform-scripture/src/sync-edit-blocked.util.ts @@ -0,0 +1,34 @@ +import { getErrorMessage } from 'platform-bible-utils'; + +/** + * Sentinel that paranext-core's C# write-gate (`SendReceiveWriteLock`) appends to the message of + * any project write it rejects because an automatic Send/Receive is syncing that project. Backend + * mutations across ManageBooks (create/delete/copy/import), the Inventory setters + * (`SetInventoryItemStatus`/`SetInventoryOptionValues`), and the Checks allow/deny path + * (`CheckRunner.AllowCheckResult`/`DenyCheckResult`) all reject with a message ending in this + * sentinel. + * + * Mirrors the scripture editor's own `SYNC_EDIT_BLOCKED_REGEX`; kept as a separate copy so the + * editor's self-contained handling stays untouched. + */ +export const SYNC_EDIT_BLOCKED_REGEX = /\(SR_EDIT_BLOCKED\)/; + +/** + * Localization key for the shared "editing is paused during Send/Receive" warning shown when one of + * this extension's mutations is rejected by the write-gate. Contributed in this extension's + * `contributions/localizedStrings.json`. + */ +export const SYNC_EDIT_BLOCKED_MESSAGE_KEY = '%webView_platformScripture_error_syncEditBlocked%'; + +/** + * Whether `error` was thrown by the write-gate because an automatic Send/Receive is in progress + * (its message carries {@link SYNC_EDIT_BLOCKED_REGEX}). Lets each mutating webview show the shared + * "editing paused" notification instead of surfacing a raw technical error or leaking an unhandled + * promise rejection. + * + * @param error Error, thrown value, or message string to inspect. + * @returns `true` if the error is a Send/Receive write-gate rejection, `false` otherwise. + */ +export function isSyncEditBlockedError(error: unknown): boolean { + return SYNC_EDIT_BLOCKED_REGEX.test(getErrorMessage(error)); +} diff --git a/lib/papi-dts/papi.d.ts b/lib/papi-dts/papi.d.ts index 08eb39222d9..92e9a81bc08 100644 --- a/lib/papi-dts/papi.d.ts +++ b/lib/papi-dts/papi.d.ts @@ -8234,6 +8234,13 @@ declare module 'shared/data/platform.data' { export const DEFAULT_ZOOM_FACTOR = 1; export const MIN_ZOOM_FACTOR = 0.5; export const MAX_ZOOM_FACTOR = 3; + /** + * Upper bound (10 minutes) for how long an automatic Send/Receive is allowed to run before we stop + * waiting on it. Used as the shutdown-sync timeout in the main process and as the auto-sync + * edit-block safety leash in the renderer. A scheduled sync of a large repo can run for minutes, so + * this is deliberately long. + */ + export const SHUTDOWN_SYNC_TIME_OUT_MS: number; } declare module 'shared/log-error.model' { /** Error that force logs the error message before throwing. Useful for debugging in some situations. */ diff --git a/src/main/shutdown-tasks.ts b/src/main/shutdown-tasks.ts index a8451513445..c429286f157 100644 --- a/src/main/shutdown-tasks.ts +++ b/src/main/shutdown-tasks.ts @@ -1,3 +1,4 @@ +import { SHUTDOWN_SYNC_TIME_OUT_MS } from '@shared/data/platform.data'; import { CATEGORY_COMMAND } from '@shared/data/rpc.model'; import { logger } from '@shared/services/logger.service'; import { networkObjectService } from '@shared/services/network-object.service'; @@ -11,8 +12,6 @@ import { serializeRequestType } from '@shared/utils/util'; import { SCRIPTURE_EDITOR_WEBVIEW_TYPE } from '@shared/models/web-view.model'; import { AsyncVariable } from 'platform-bible-utils'; -const SHUTDOWN_SYNC_TIME_OUT_MS = 10 * 60 * 1000; // 10 minutes - /** * Runs cleanup tasks (e.g., syncing projects) when the user closes the main window. * diff --git a/src/renderer/index.tsx b/src/renderer/index.tsx index aacb6f4f19b..1af37d78e96 100644 --- a/src/renderer/index.tsx +++ b/src/renderer/index.tsx @@ -4,6 +4,8 @@ import '@renderer/global-this-web-view.model'; import '@renderer/global-this.model'; import { App } from '@renderer/app.component'; +import { initAutoSyncBlockingService } from '@renderer/services/auto-sync-blocking-service'; +import { initAutoSyncEditBlockDriver } from '@renderer/services/auto-sync-edit-block-driver'; import { startDialogService } from '@renderer/services/dialog.service-host'; import { startNotificationService } from '@renderer/services/notification.service-host'; import { startOverlayService } from '@renderer/services/overlays/overlay.service-host'; @@ -108,6 +110,13 @@ 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. + initAutoSyncBlockingService(); + initAutoSyncEditBlockDriver(); + // Subscribe to updates to the current theme await localThemeService.subscribeCurrentTheme(undefined, (newTheme) => { if (isPlatformError(newTheme)) { diff --git a/src/renderer/services/auto-sync-blocking-service.test.ts b/src/renderer/services/auto-sync-blocking-service.test.ts new file mode 100644 index 00000000000..672ec0832a9 --- /dev/null +++ b/src/renderer/services/auto-sync-blocking-service.test.ts @@ -0,0 +1,60 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { getNetworkEvent } from '@shared/services/network.service'; +import { setAutoSyncBlocking } from '@renderer/services/auto-sync-blocking-store'; +import { initAutoSyncBlockingService } from './auto-sync-blocking-service'; + +vi.mock('@shared/services/network.service', () => ({ + getNetworkEvent: vi.fn(), +})); + +vi.mock('@renderer/services/auto-sync-blocking-store', () => ({ + setAutoSyncBlocking: vi.fn(), +})); + +describe('initAutoSyncBlockingService', () => { + let capturedHandler: ((event: { isBlocking: boolean }) => void) | undefined; + let unsub: ReturnType; + + beforeEach(() => { + vi.clearAllMocks(); + capturedHandler = undefined; + unsub = vi.fn(); + + 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) => { + capturedHandler = cb; + return unsub; + }; + 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, + ); + }); + + it('calls setAutoSyncBlocking(true) when the event fires with isBlocking true', () => { + initAutoSyncBlockingService(); + expect(capturedHandler).toBeDefined(); + if (!capturedHandler) throw new Error('capturedHandler not set'); + capturedHandler({ isBlocking: true }); + expect(vi.mocked(setAutoSyncBlocking)).toHaveBeenCalledWith(true); + }); + + it('calls setAutoSyncBlocking(false) when the event fires with isBlocking false', () => { + initAutoSyncBlockingService(); + expect(capturedHandler).toBeDefined(); + if (!capturedHandler) throw new Error('capturedHandler not set'); + capturedHandler({ isBlocking: false }); + expect(vi.mocked(setAutoSyncBlocking)).toHaveBeenCalledWith(false); + }); + + it('returns a cleanup function that unsubscribes the event', () => { + const cleanup = initAutoSyncBlockingService(); + cleanup(); + expect(unsub).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/renderer/services/auto-sync-blocking-service.ts b/src/renderer/services/auto-sync-blocking-service.ts new file mode 100644 index 00000000000..1a4a1d6139a --- /dev/null +++ b/src/renderer/services/auto-sync-blocking-service.ts @@ -0,0 +1,24 @@ +import { getNetworkEvent } from '@shared/services/network.service'; +import { setAutoSyncBlocking } 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; the store ref-counts so nested raises don't clear +// early. +const AUTO_SYNC_BLOCKING_CHANGED_EVENT = 'paratextBibleSendReceive.onAutoSyncBlockingChanged'; + +/** + * Subscribes to the auto-sync blocking network event and drives the auto-sync-blocking store. Call + * once at app startup. Returns a cleanup function. + * + * Known limitation (deliberate): the protocol is event-only, so a renderer reload during an + * in-flight scheduled sync misses the earlier raise and leaves the rest of that sync unblocked. + * Upgrade path when PT-4163 lands is a queryable state on the emitter side (a command to read the + * current blocking state, or a periodic re-emit) that this service consults on init. + */ +export function initAutoSyncBlockingService(): () => void { + return getNetworkEvent<{ isBlocking: boolean }>(AUTO_SYNC_BLOCKING_CHANGED_EVENT)( + ({ isBlocking }) => setAutoSyncBlocking(isBlocking), + ); +} diff --git a/src/renderer/services/auto-sync-blocking-store.test.ts b/src/renderer/services/auto-sync-blocking-store.test.ts new file mode 100644 index 00000000000..c694971ba7a --- /dev/null +++ b/src/renderer/services/auto-sync-blocking-store.test.ts @@ -0,0 +1,190 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { + setAutoSyncBlocking, + getAutoSyncBlocking, + 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 + beforeEach(() => { + vi.useFakeTimers(); + resetAutoSyncBlocking(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + describe('getAutoSyncBlocking', () => { + it('returns false initially', () => { + expect(getAutoSyncBlocking()).toBe(false); + }); + }); + + describe('show grace', () => { + it('is not visible immediately when blocking starts', () => { + setAutoSyncBlocking(true); + expect(getAutoSyncBlocking()).toBe(false); + }); + + it('becomes visible once the 200 ms grace elapses', () => { + setAutoSyncBlocking(true); + vi.advanceTimersByTime(SHOW_GRACE_MS); + expect(getAutoSyncBlocking()).toBe(true); + }); + + it('never becomes visible when blocking clears within the grace (sync finished fast)', () => { + const listener = vi.fn(); + subscribeToAutoSyncBlocking(listener); + setAutoSyncBlocking(true); + vi.advanceTimersByTime(150); // still inside the grace window + setAutoSyncBlocking(false); + 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 + }); + + it('does not notify listeners during the grace period', () => { + const listener = vi.fn(); + subscribeToAutoSyncBlocking(listener); + setAutoSyncBlocking(true); + vi.advanceTimersByTime(SHOW_GRACE_MS - 1); + expect(listener).not.toHaveBeenCalled(); + vi.advanceTimersByTime(1); + expect(listener).toHaveBeenCalledTimes(1); + }); + }); + + describe('ref counting', () => { + it('stays visible when a second raise arrives before the first clears', () => { + setAutoSyncBlocking(true); // blocker A raises + vi.advanceTimersByTime(SHOW_GRACE_MS); + setAutoSyncBlocking(true); // blocker B raises + setAutoSyncBlocking(false); // blocker A clears + expect(getAutoSyncBlocking()).toBe(true); // blocker B still in flight + setAutoSyncBlocking(false); // blocker B clears + expect(getAutoSyncBlocking()).toBe(false); + }); + + it('clamps at zero — extra false calls do not underflow', () => { + setAutoSyncBlocking(false); // extra call while already at zero — should be ignored + setAutoSyncBlocking(true); + vi.advanceTimersByTime(SHOW_GRACE_MS); + expect(getAutoSyncBlocking()).toBe(true); // count went 0→1, not -1→0 + setAutoSyncBlocking(false); + expect(getAutoSyncBlocking()).toBe(false); + }); + + it('does not notify listeners when visibility is unchanged (nested raises)', () => { + setAutoSyncBlocking(true); + vi.advanceTimersByTime(SHOW_GRACE_MS); + const listener = vi.fn(); + subscribeToAutoSyncBlocking(listener); + setAutoSyncBlocking(true); // second raise — count 1→2, visibility still true + expect(listener).not.toHaveBeenCalled(); + }); + + it('does not notify listeners when value is unchanged (already false)', () => { + const listener = vi.fn(); + subscribeToAutoSyncBlocking(listener); + setAutoSyncBlocking(false); // already at zero — no change + expect(listener).not.toHaveBeenCalled(); + }); + }); + + describe('subscribeToAutoSyncBlocking', () => { + it('notifies listeners when visibility flips to false', () => { + setAutoSyncBlocking(true); + vi.advanceTimersByTime(SHOW_GRACE_MS); + const listener = vi.fn(); + subscribeToAutoSyncBlocking(listener); + setAutoSyncBlocking(false); + expect(listener).toHaveBeenCalledTimes(1); + }); + + it('returns an unsubscriber that stops notifications', () => { + const listener = vi.fn(); + const unsubscribe = subscribeToAutoSyncBlocking(listener); + unsubscribe(); + setAutoSyncBlocking(true); + vi.advanceTimersByTime(SHOW_GRACE_MS); + expect(listener).not.toHaveBeenCalled(); + }); + + it('notifies multiple listeners', () => { + const listener1 = vi.fn(); + const listener2 = vi.fn(); + subscribeToAutoSyncBlocking(listener1); + subscribeToAutoSyncBlocking(listener2); + setAutoSyncBlocking(true); + vi.advanceTimersByTime(SHOW_GRACE_MS); + expect(listener1).toHaveBeenCalledTimes(1); + expect(listener2).toHaveBeenCalledTimes(1); + }); + }); + + describe('safety timer', () => { + it('auto-clears after 10 minutes if the blocker never clears', () => { + const listener = vi.fn(); + subscribeToAutoSyncBlocking(listener); + setAutoSyncBlocking(true); + 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('re-arms the safety timer on every raise', () => { + setAutoSyncBlocking(true); // blocker A raises — safety armed + vi.advanceTimersByTime(SAFETY_TIMEOUT_MS / 2); + setAutoSyncBlocking(true); // blocker B raises — safety re-armed to 10 min from now + vi.advanceTimersByTime(SAFETY_TIMEOUT_MS / 2); // 10 min from A, but only 5 min from B + expect(getAutoSyncBlocking()).toBe(true); // B's raise re-armed the timer — still alive + vi.advanceTimersByTime(SAFETY_TIMEOUT_MS / 2); // 10 min from B — safety fires + expect(getAutoSyncBlocking()).toBe(false); + }); + + it('zeroes the count when it fires — a later single raise shows again', () => { + setAutoSyncBlocking(true); + setAutoSyncBlocking(true); // count is 2 + vi.advanceTimersByTime(SAFETY_TIMEOUT_MS); + expect(getAutoSyncBlocking()).toBe(false); + setAutoSyncBlocking(true); // count 0→1, not 2→3 + vi.advanceTimersByTime(SHOW_GRACE_MS); + expect(getAutoSyncBlocking()).toBe(true); + setAutoSyncBlocking(false); // a single clear hides again + expect(getAutoSyncBlocking()).toBe(false); + }); + + it('cancels the safety timer when blocking clears normally', () => { + const listener = vi.fn(); + subscribeToAutoSyncBlocking(listener); + setAutoSyncBlocking(true); + vi.advanceTimersByTime(SHOW_GRACE_MS); + setAutoSyncBlocking(false); // 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', () => { + const listener = vi.fn(); + subscribeToAutoSyncBlocking(listener); + setAutoSyncBlocking(true); + resetAutoSyncBlocking(); + vi.advanceTimersByTime(SAFETY_TIMEOUT_MS); // neither grace nor safety should fire + expect(getAutoSyncBlocking()).toBe(false); + expect(listener).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/src/renderer/services/auto-sync-blocking-store.ts b/src/renderer/services/auto-sync-blocking-store.ts new file mode 100644 index 00000000000..377309ef978 --- /dev/null +++ b/src/renderer/services/auto-sync-blocking-store.ts @@ -0,0 +1,106 @@ +/** + * Store tracking whether an automatic (scheduled) Send/Receive is currently blocking the workspace. + * + * Uses a reference counter so overlapping blockers don't prematurely hide the overlay: the visible + * state (isBlocking) only flips to false once every in-flight blocker has cleared. + * + * 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. + * + * A 10-minute safety timer re-arms on every raise and auto-clears the state if a blocker never + * clears (e.g. the extension deactivates mid-sync and the clearing event is never emitted). + */ + +import { SHUTDOWN_SYNC_TIME_OUT_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. Tracks SHUTDOWN_SYNC_TIME_OUT_MS (the shutdown-sync timeout) because + * both bound the same operation — a single automatic Send/Receive — so they should never diverge; a + * scheduled sync of a large repo can run minutes, so the leash is deliberately long. + */ +const SAFETY_TIMEOUT_MS = SHUTDOWN_SYNC_TIME_OUT_MS; + +let blockCount = 0; +let isBlockingVisible = false; +let graceTimer: ReturnType | undefined; +let safetyTimer: ReturnType | undefined; + +const listeners = new Set<() => void>(); + +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; + notifyListeners(); +} + +export function setAutoSyncBlocking(value: boolean): void { + if (value) { + blockCount += 1; + // Re-arm the safety timer on each raise, giving 10 min from the latest start. + clearTimeout(safetyTimer); + safetyTimer = setTimeout(() => { + blockCount = 0; + safetyTimer = undefined; + clearTimeout(graceTimer); + graceTimer = undefined; + setBlockingVisible(false); + }, SAFETY_TIMEOUT_MS); + // On 0→1, arm the show grace; visibility only turns on if blocking survives the grace. + if (blockCount === 1) { + graceTimer = setTimeout(() => { + graceTimer = undefined; + if (blockCount > 0) setBlockingVisible(true); + }, SHOW_GRACE_MS); + } + } else { + blockCount = Math.max(0, blockCount - 1); + if (blockCount === 0) { + // Cancel a pending grace timer — blocking cleared inside the grace, so nothing ever shows. + clearTimeout(graceTimer); + graceTimer = undefined; + clearTimeout(safetyTimer); + safetyTimer = undefined; + setBlockingVisible(false); + } + } +} + +export function getAutoSyncBlocking(): boolean { + return isBlockingVisible; +} + +/** Subscribe to state changes. Returns an unsubscribe function. */ +export function subscribeToAutoSyncBlocking(listener: () => void): () => void { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; +} + +/** + * Resets the store to its initial state. + * + * WARNING: Test-only. @internal + */ +export function resetAutoSyncBlocking(): void { + blockCount = 0; + isBlockingVisible = false; + clearTimeout(graceTimer); + graceTimer = undefined; + clearTimeout(safetyTimer); + safetyTimer = undefined; + listeners.clear(); +} diff --git a/src/renderer/services/auto-sync-edit-block-driver.test.ts b/src/renderer/services/auto-sync-edit-block-driver.test.ts new file mode 100644 index 00000000000..1c7574b54ae --- /dev/null +++ b/src/renderer/services/auto-sync-edit-block-driver.test.ts @@ -0,0 +1,360 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { SavedWebViewDefinition } from '@shared/models/web-view.model'; +import { + getAutoSyncBlocking, + subscribeToAutoSyncBlocking, +} from '@renderer/services/auto-sync-blocking-store'; +import { + getAllOpenWebViewDefinitionsSync, + onDidOpenWebView, + onDidUpdateWebView, + updateWebViewDefinitionSync, +} from '@renderer/services/web-view.service-host'; +import { initAutoSyncEditBlockDriver } from './auto-sync-edit-block-driver'; + +vi.mock('@renderer/services/auto-sync-blocking-store', () => ({ + getAutoSyncBlocking: vi.fn(), + subscribeToAutoSyncBlocking: vi.fn(), +})); + +vi.mock('@renderer/services/web-view.service-host', () => ({ + getAllOpenWebViewDefinitionsSync: vi.fn(), + onDidOpenWebView: vi.fn(), + onDidUpdateWebView: vi.fn(), + updateWebViewDefinitionSync: vi.fn(), +})); + +vi.mock('@shared/services/logger.service', () => ({ + logger: { warn: vi.fn(), debug: vi.fn() }, +})); + +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 + * 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. + */ +const definitionsById = new Map(); + +function makeDefinition( + id: string, + webViewType: 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. + // eslint-disable-next-line no-type-assertion/no-type-assertion + const definition = { id, webViewType, state } as SavedWebViewDefinition; + definitionsById.set(id, definition); + return definition; +} + +function editor(id: string, state?: Record): SavedWebViewDefinition { + return makeDefinition(id, EDITOR_TYPE, state); +} + +function nonEditor(id: string): SavedWebViewDefinition { + return makeDefinition(id, 'some.other.webView'); +} + +describe('initAutoSyncEditBlockDriver', () => { + /** The store listener the driver registers, captured so tests can drive state changes. */ + let storeListener: (() => void) | undefined; + let storeUnsub: ReturnType; + /** The onDidOpenWebView callback the driver registers while blocking. */ + let openHandler: ((event: { webView: SavedWebViewDefinition }) => void) | undefined; + let openUnsub: ReturnType; + /** The onDidUpdateWebView callback the driver registers while blocking. */ + let updateHandler: ((event: { webView: SavedWebViewDefinition }) => void) | undefined; + let updateUnsub: ReturnType; + + /** + * 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. + */ + function dispatchUpdate( + id: Parameters[0], + updateInfo: Parameters[1], + ): void { + const definition = definitionsById.get(id); + if (definition && 'state' in updateInfo) definition.state = updateInfo.state; + const webViewType = definition?.webViewType ?? EDITOR_TYPE; + const state = 'state' in updateInfo ? updateInfo.state : undefined; + // The tests only exercise id/webViewType/state; see the same cast in `makeDefinition` above. + // eslint-disable-next-line no-type-assertion/no-type-assertion + const updatedWebView = { id, webViewType, state } as SavedWebViewDefinition; + updateHandler?.({ webView: updatedWebView }); + } + + beforeEach(() => { + vi.clearAllMocks(); + definitionsById.clear(); + storeListener = undefined; + openHandler = undefined; + updateHandler = undefined; + storeUnsub = vi.fn(); + // Unsubscribing clears the captured handler, modeling real subscription teardown: once + // unsubscribed, the emitter no longer calls the handler. + openUnsub = vi.fn(() => { + openHandler = undefined; + return true; + }); + updateUnsub = vi.fn(() => { + updateHandler = undefined; + return true; + }); + + vi.mocked(subscribeToAutoSyncBlocking).mockImplementation((listener) => { + storeListener = listener; + return storeUnsub; + }); + vi.mocked(onDidOpenWebView).mockImplementation( + // The network-event type is a complex generic; capture the handler for the test. + // eslint-disable-next-line no-type-assertion/no-type-assertion, @typescript-eslint/no-explicit-any + ((handler: (event: { webView: SavedWebViewDefinition }) => void) => { + openHandler = handler; + return openUnsub; + // 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, + ); + vi.mocked(onDidUpdateWebView).mockImplementation( + // The network-event type is a complex generic; capture the handler for the test. + // eslint-disable-next-line no-type-assertion/no-type-assertion, @typescript-eslint/no-explicit-any + ((handler: (event: { webView: SavedWebViewDefinition }) => void) => { + updateHandler = handler; + return updateUnsub; + // 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, + ); + vi.mocked(getAutoSyncBlocking).mockReturnValue(false); + vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([]); + // This is what lets the regression test below reproduce the live re-flag bug: a still-subscribed + // 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); + return true; + }); + }); + + it('does not flag any editor on init when not blocking', () => { + vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([editor('e1'), nonEditor('n1')]); + initAutoSyncEditBlockDriver(); + expect(updateWebViewDefinitionSync).not.toHaveBeenCalled(); + }); + + it('sets isSyncBlocked true only on Scripture editors when blocking starts', () => { + vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([ + editor('e1', { viewType: 'formatted' }), + nonEditor('n1'), + editor('e2'), + ]); + initAutoSyncEditBlockDriver(); + + vi.mocked(getAutoSyncBlocking).mockReturnValue(true); + 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); + }); + + it('clears isSyncBlocked when blocking ends', () => { + vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([ + editor('e1', { isSyncBlocked: true }), + ]); + initAutoSyncEditBlockDriver(); + + vi.mocked(getAutoSyncBlocking).mockReturnValue(false); + if (!storeListener) throw new Error('store listener not registered'); + storeListener(); + + expect(updateWebViewDefinitionSync).toHaveBeenCalledWith('e1', { + state: { isSyncBlocked: false }, + }); + }); + + 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)); + dispatchUpdate(id, updateInfo); + return true; + }); + + vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([editor('e1')]); + 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); + storeListener(); + expect(onDidUpdateWebView).toHaveBeenCalledTimes(1); + + // The sync finishes: blocking clears. The driver must tear down the update subscription BEFORE + // 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); + storeListener(); + + expect(e1Writes).toEqual([true, false]); + expect(e1Writes.at(-1)).toBe(false); + expect(updateUnsub).toHaveBeenCalledTimes(1); + }, + ); + + 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. + vi.mocked(getAllOpenWebViewDefinitionsSync).mockReturnValue([ + editor('e1', { isSyncBlocked: true }), + ]); + vi.mocked(getAutoSyncBlocking).mockReturnValue(true); + initAutoSyncEditBlockDriver(); + + expect(updateWebViewDefinitionSync).not.toHaveBeenCalled(); + }); + + it('flags editors opened mid-block 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); + 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', { + state: { isSyncBlocked: true }, + }); + + // A non-editor opened mid-block is ignored + vi.mocked(updateWebViewDefinitionSync).mockClear(); + openHandler({ webView: nonEditor('other') }); + expect(updateWebViewDefinitionSync).not.toHaveBeenCalled(); + + // Blocking ends → unsubscribes from onDidOpenWebView + vi.mocked(getAutoSyncBlocking).mockReturnValue(false); + storeListener(); + expect(openUnsub).toHaveBeenCalledTimes(1); + }); + + it('only subscribes to onDidOpenWebView once across overlapping blocking notifications', () => { + initAutoSyncEditBlockDriver(); + if (!storeListener) throw new Error('store listener not registered'); + vi.mocked(getAutoSyncBlocking).mockReturnValue(true); + storeListener(); + storeListener(); + expect(onDidOpenWebView).toHaveBeenCalledTimes(1); + }); + + it('re-flags a Scripture editor 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); + 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 }) }); + expect(updateWebViewDefinitionSync).toHaveBeenCalledWith('rebuilt', { + state: { viewType: 'formatted', isSyncBlocked: true }, + }); + + // A non-editor update mid-block is ignored + vi.mocked(updateWebViewDefinitionSync).mockClear(); + updateHandler({ webView: nonEditor('other') }); + expect(updateWebViewDefinitionSync).not.toHaveBeenCalled(); + }); + + 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); + 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 }) }); + return true; + }); + + updateHandler({ webView: editor('rebuilt', { isSyncBlocked: false }) }); + + // Exactly one write: the original re-flag. The re-emitted (already-blocked) update is a no-op. + expect(updateWebViewDefinitionSync).toHaveBeenCalledTimes(1); + expect(updateWebViewDefinitionSync).toHaveBeenCalledWith('rebuilt', { + state: { isSyncBlocked: true }, + }); + }); + + it('only subscribes to onDidUpdateWebView once across overlapping blocking notifications', () => { + initAutoSyncEditBlockDriver(); + if (!storeListener) throw new Error('store listener not registered'); + vi.mocked(getAutoSyncBlocking).mockReturnValue(true); + storeListener(); + storeListener(); + expect(onDidUpdateWebView).toHaveBeenCalledTimes(1); + }); + + it('unsubscribes from onDidUpdateWebView when blocking ends', () => { + initAutoSyncEditBlockDriver(); + if (!storeListener) throw new Error('store listener not registered'); + + vi.mocked(getAutoSyncBlocking).mockReturnValue(true); + storeListener(); + expect(onDidUpdateWebView).toHaveBeenCalledTimes(1); + + vi.mocked(getAutoSyncBlocking).mockReturnValue(false); + storeListener(); + 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); + storeListener(); + + cleanup(); + expect(storeUnsub).toHaveBeenCalledTimes(1); + expect(openUnsub).toHaveBeenCalledTimes(1); + expect(updateUnsub).toHaveBeenCalledTimes(1); + }); + + it('does not throw if the dock layout is not ready when enumerating web views', () => { + vi.mocked(getAllOpenWebViewDefinitionsSync).mockImplementation(() => { + throw new Error('dock layout not registered'); + }); + 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 new file mode 100644 index 00000000000..8d6b46c8e3c --- /dev/null +++ b/src/renderer/services/auto-sync-edit-block-driver.ts @@ -0,0 +1,167 @@ +/** + * 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. + * + * 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. + * + * 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. + * + * 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. + */ + +import { logger } from '@shared/services/logger.service'; +import { + SavedWebViewDefinition, + SCRIPTURE_EDITOR_WEBVIEW_TYPE, +} from '@shared/models/web-view.model'; +import { getErrorMessage } from 'platform-bible-utils'; +import { + getAutoSyncBlocking, + subscribeToAutoSyncBlocking, +} from '@renderer/services/auto-sync-blocking-store'; +import { + getAllOpenWebViewDefinitionsSync, + onDidOpenWebView, + onDidUpdateWebView, + updateWebViewDefinitionSync, +} from '@renderer/services/web-view.service-host'; + +/** 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'; + +/** + * 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` + * is present (it is compared by reference), so the equality guard keeps a no-op from rippling + * through every editor's update subscribers. + */ +function setEditorSyncBlocked(definition: SavedWebViewDefinition, isBlocked: boolean): void { + const currentState = definition.state ?? {}; + // Normalize a missing flag to false so init and unblock never write false onto an editor that is + // already (implicitly) unblocked. + if (Boolean(currentState[IS_SYNC_BLOCKED_STATE_KEY]) === isBlocked) return; + try { + updateWebViewDefinitionSync(definition.id, { + state: { ...currentState, [IS_SYNC_BLOCKED_STATE_KEY]: isBlocked }, + }); + } catch (e) { + logger.warn( + `auto-sync edit-block driver failed to update editor ${definition.id}: ${getErrorMessage(e)}`, + ); + } +} + +/** Applies `isBlocked` to every currently open Scripture editor web view. */ +function applyToAllEditors(isBlocked: boolean): void { + let definitions: SavedWebViewDefinition[]; + try { + definitions = getAllOpenWebViewDefinitionsSync(); + } catch (e) { + // The dock layout may not be registered yet at startup; nothing is blocked then, so this is a + // benign no-op. Logged at debug so a normal startup does not warn. + logger.debug( + `auto-sync edit-block driver could not enumerate web views: ${getErrorMessage(e)}`, + ); + return; + } + definitions.forEach((definition) => { + if (definition.webViewType === SCRIPTURE_EDITOR_WEBVIEW_TYPE) + setEditorSyncBlocked(definition, isBlocked); + }); +} + +/** + * 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). + */ +export function initAutoSyncEditBlockDriver(): () => void { + let unsubscribeOpen: (() => void) | undefined; + let unsubscribeUpdate: (() => void) | undefined; + + 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; + } + } + + 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(); + }; + } + } + }; + + // Reflect the current state immediately (in case blocking is already active on init), then track. + syncState(); + const unsubscribeStore = subscribeToAutoSyncBlocking(syncState); + + return () => { + unsubscribeStore(); + if (unsubscribeOpen) { + unsubscribeOpen(); + unsubscribeOpen = undefined; + } + if (unsubscribeUpdate) { + unsubscribeUpdate(); + unsubscribeUpdate = undefined; + } + }; +} diff --git a/src/shared/data/platform.data.ts b/src/shared/data/platform.data.ts index 8c7d33a1bdd..ed0cc6ef915 100644 --- a/src/shared/data/platform.data.ts +++ b/src/shared/data/platform.data.ts @@ -19,3 +19,11 @@ export const DEFAULT_THEME_TYPE = 'light'; export const DEFAULT_ZOOM_FACTOR = 1.0; export const MIN_ZOOM_FACTOR = 0.5; export const MAX_ZOOM_FACTOR = 3.0; + +/** + * Upper bound (10 minutes) for how long an automatic Send/Receive is allowed to run before we stop + * waiting on it. Used as the shutdown-sync timeout in the main process and as the auto-sync + * edit-block safety leash in the renderer. A scheduled sync of a large repo can run for minutes, so + * this is deliberately long. + */ +export const SHUTDOWN_SYNC_TIME_OUT_MS = 10 * 60 * 1000;