SF-1992 Fail sync if push failed (#3778)

- When pushing a bundle to the PT SendReceive server, we can
incorrectly think we gave commits to the server and that they were
retained. This leads to additional problems later when pushing again,
because the server doesn't have a commit that we are expecting it to
have.
- This patch checks if the PT SendReceive server has the commit we
intended to push, and fails the sync if not.
- The outgoing bundle size is also logged for help in investigating
problems.
- This patch does not fix the situation where pushing a commit to the
SR server doesn't work. But it should prevent SF from (1) incorrectly
communicating to the user that the sync succeeded, and (2) getting
into a state where the SF project is stuck and can not sync without
manual intervention. The SF project is left in a state where the sync
can at least be re-tried.

Author: marksvc

AGENTS.md

@@ -82,6 +82,7 @@ This repository contains three interconnected applications:
 - Do put comments into the code if the intent is not clear from the code.
 - All classes and interfaces should have a comment to briefly explain why it is there and what its purpose is in the overall system, even if it seems obvious.
 - Please do not fail to add a comment to any classes or interfaces that are created. All classes and interfaces should have a comment.
+- Use good argument and variable names that explain themselves without needing a comment. Well named arguments or variables are better than unclearly named arguments or variables with a comment.
 
 # TypeScript language
 

src/SIL.XForge.Scripture/Services/HgWrapper.cs

@@ -6,7 +6,7 @@
 
 namespace SIL.XForge.Scripture.Services;
 
-/// <summary> A wrapper for the <see cref="Hg" /> class for calling Mercurial. </summary>
+/// <summary>A wrapper for the <see cref="Hg" /> class for calling Mercurial.</summary>
 public class HgWrapper : IHgWrapper
 {
     public static string RunCommand(string repository, string cmd)
@@ -16,14 +16,17 @@ public static string RunCommand(string repository, string cmd)
         return Hg.Default.RunCommand(repository, cmd).StdOut;
     }
 
-    public static byte[] Bundle(string repository, params string[] heads)
+    /// <summary>
+    /// Returns a Mercurial bundle containing changesets above the given base revisions.
+    /// </summary>
+    public byte[] Bundle(string repository, params string[] baseRevisions)
     {
         if (Hg.Default == null)
             throw new InvalidOperationException("Hg default has not been set.");
-        return Hg.Default.Bundle(repository, heads);
+        return Hg.Default.Bundle(repository, baseRevisions);
     }
 
-    public static string[] Pull(string repository, byte[] bundle)
+    public string[] Pull(string repository, byte[] bundle)
     {
         if (Hg.Default == null)
             throw new InvalidOperationException("Hg default has not been set.");
@@ -72,12 +75,12 @@ public string GetLastPublicRevision(string repository)
     /// <summary>
     /// Returns the currently checked out revision of an hg repository.
     /// </summary>
-    public string GetRepoRevision(string repositoryPath)
+    public string GetRepoRevision(string repository)
     {
-        string rev = RunCommand(repositoryPath, "log --limit 1 --rev . --template {node}");
+        string rev = RunCommand(repository, "log --limit 1 --rev . --template {node}");
         if (string.IsNullOrWhiteSpace(rev))
         {
-            throw new InvalidDataException($"Unable to determine repo revision for hg repo at {repositoryPath}");
+            throw new InvalidDataException($"Unable to determine repo revision for hg repo at {repository}");
         }
         return rev;
     }
@@ -98,9 +101,9 @@ public void RestoreRepository(string destination, string backupFile)
     }
 
     /// <summary>
-    /// Mark all changesets available on the PT server public.
+    /// Mark all changesets as public.
     /// </summary>
-    /// <param name="repository">The repository.</param>
+    /// <param name="repository">The repository path.</param>
     public void MarkSharedChangeSetsPublic(string repository) => RunCommand(repository, "phase -p -r 'tip'");
 
     /// <summary>
@@ -136,5 +139,5 @@ public void SetDefault(Hg hgDefault)
     /// `git checkout --force --detach COMMITTISH`
     /// Changes to tracked files will be discarded. Untracked files are left in place without being cleaned up.
     /// </summary>
-    public void Update(string repositoryPath, string rev) => Hg.Default.Update(repositoryPath, rev);
+    public void Update(string repository, string rev) => Hg.Default.Update(repository, rev);
 }

src/SIL.XForge.Scripture/Services/IHgWrapper.cs

@@ -2,6 +2,7 @@
 
 namespace SIL.XForge.Scripture.Services;
 
+/// <summary>Mercurial operations</summary>
 public interface IHgWrapper
 {
     void SetDefault(Hg hgDefault);
@@ -11,8 +12,10 @@ public interface IHgWrapper
     void BackupRepository(string repository, string backupFile);
     void RestoreRepository(string destination, string backupFile);
     string GetLastPublicRevision(string repository);
-    string GetRepoRevision(string repositoryPath);
+    string GetRepoRevision(string repository);
     void MarkSharedChangeSetsPublic(string repository);
+    string[] Pull(string repository, byte[] bundle);
+    byte[] Bundle(string repository, params string[] baseRevisions);
     string RecentLogGraph(string repositoryPath);
     string[] GetDraftRevisions(string repositoryPath);
 }

src/SIL.XForge.Scripture/Services/JwtInternetSharedRepositorySource.cs

@@ -13,12 +13,15 @@
 
 namespace SIL.XForge.Scripture.Services;
 
-/// <summary> An internet shared repository source that networks using JWT authenticated REST clients. </summary>
+/// <summary>
+/// An internet shared repository source that networks using JWT authenticated REST clients.
+/// </summary>
 public class JwtInternetSharedRepositorySource : InternetSharedRepositorySource, IInternetSharedRepositorySource
 {
     private readonly JwtRestClient _registryClient;
     private readonly IHgWrapper _hgWrapper;
     private readonly ILogger _logger;
+    private readonly int _maxJsonLogChars = 200;
 
     public JwtInternetSharedRepositorySource(
         string accessToken,
@@ -62,9 +65,9 @@ public bool CanUserAuthenticateToPTArchives()
     /// Uses the a REST client to pull from the Paratext send/receive server. This overrides the base implementation
     /// to avoid needing the current user's Paratext registration code to get the base revision.
     /// </summary>
-    public override string[] Pull(string repository, SharedRepository pullRepo)
+    public override string[] Pull(string repositoryPath, SharedRepository pullRepo)
     {
-        string baseRev = _hgWrapper.GetLastPublicRevision(repository);
+        string baseRev = _hgWrapper.GetLastPublicRevision(repositoryPath);
 
         // Get bundle
         string guid = Guid.NewGuid().ToString();
@@ -92,24 +95,33 @@ public override string[] Pull(string repository, SharedRepository pullRepo)
             return [];
 
         // Use bundle
-        string[] changeSets = HgWrapper.Pull(repository, bundle);
+        string[] changeSets = _hgWrapper.Pull(repositoryPath, bundle);
 
-        _hgWrapper.MarkSharedChangeSetsPublic(repository);
+        _hgWrapper.MarkSharedChangeSetsPublic(repositoryPath);
         return changeSets;
     }
 
     /// <summary>
     /// Uses the a REST client to push to the Paratext send/receive server. This overrides the base implementation
     /// to avoid needing the current user's Paratext registration code to get the base revision.
     /// </summary>
-    public override void Push(string repository, SharedRepository pushRepo)
+    public override void Push(string repositoryPath, SharedRepository pushRepo)
     {
-        string baseRev = _hgWrapper.GetLastPublicRevision(repository);
+        string baseRev = _hgWrapper.GetLastPublicRevision(repositoryPath);
 
         // Create bundle
-        byte[] bundle = HgWrapper.Bundle(repository, baseRev);
+        byte[] bundle = _hgWrapper.Bundle(repositoryPath, baseRev);
         if (bundle.Length == 0)
+        {
+            _logger.LogInformation($"Not pushing a 0 Byte bundle for project PT ID {pushRepo.SendReceiveId.Id}.");
             return;
+        }
+
+        string localTip = _hgWrapper.GetRepoRevision(repositoryPath);
+        _logger.LogInformation(
+            $"Pushing bundle of {bundle.Length} Bytes to S/R server for project PT ID "
+                + $"{pushRepo.SendReceiveId.Id}. Base revision {baseRev ?? "(null)"}. Local tip {localTip}."
+        );
 
         // Send bundle
         string guid = Guid.NewGuid().ToString();
@@ -128,7 +140,78 @@ public override void Push(string repository, SharedRepository pushRepo)
             "no"
         );
 
-        _hgWrapper.MarkSharedChangeSetsPublic(repository);
+        (bool isRevOnServer, int serverRevCount, string? serverLastRev) = CheckIfRevisionIsOnServer(pushRepo, localTip);
+        if (!isRevOnServer)
+        {
+            throw new InvalidOperationException(
+                $"Push verification failed for project PT ID {pushRepo.SendReceiveId.Id}. "
+                    + $"Expected revision {localTip} was not found in the server's revision history. "
+                    + $"Server has {serverRevCount} revisions. Last server revision: {serverLastRev ?? "(null)"}."
+            );
+        }
+
+        _hgWrapper.MarkSharedChangeSetsPublic(repositoryPath);
+    }
+
+    /// <summary>
+    /// Returns whether the expected revision is present on the Paratext Send/Receive server.
+    /// </summary>
+    internal (bool isRevOnServer, int serverRevCount, string? serverLastRev) CheckIfRevisionIsOnServer(
+        SharedRepository serverRepo,
+        string expectedRevision
+    )
+    {
+        string projRevHistResponse = GetClient()
+            .Get("projrevhist", "proj", serverRepo.ScrTextName, "projid", serverRepo.SendReceiveId.Id, "all", "1");
+
+        JObject jsonResult = JObject.Parse(projRevHistResponse);
+        if (jsonResult["project"]?["revision_history"]?["revisions"] is not JArray revisions)
+        {
+            string truncatedResult = FormatAndTruncate(jsonResult, _maxJsonLogChars);
+            _logger.LogWarning(
+                $"Getting projrevhist unexpectedly received null revisions for PT project ID {serverRepo.SendReceiveId.Id}. The JSON result is: {truncatedResult}"
+            );
+            return (false, 0, null);
+        }
+
+        bool isRevOnServer = revisions.Any(r =>
+            string.Equals(r["id"]?.ToString(), expectedRevision, StringComparison.Ordinal)
+        );
+        int serverRevCount = revisions.Count;
+        string? serverLastRev = GetFirstElementId(revisions);
+
+        return (isRevOnServer, serverRevCount, serverLastRev);
+    }
+
+    /// <summary>
+    /// Returns the first element's "id" value, if possible.
+    /// </summary>
+    private static string? GetFirstElementId(JArray revisions)
+    {
+        if (revisions.Count > 0)
+            return revisions[0]["id"]?.ToString();
+        return null;
+    }
+
+    /// <summary>
+    /// Formats JSON and truncates if too long.
+    /// </summary>
+    private static string FormatAndTruncate(JObject jsonResult, int maxChars)
+    {
+        string prettyJson = jsonResult.ToString(Newtonsoft.Json.Formatting.Indented);
+        return TruncateLogString(prettyJson, maxChars);
+    }
+
+    /// <summary>
+    /// Truncates a string to the configured character count if needed and appends truncation details.
+    /// </summary>
+    private static string TruncateLogString(string value, int maxChars)
+    {
+        if (value.Length <= maxChars)
+            return value;
+
+        int truncatedChars = value.Length - maxChars;
+        return value[..maxChars] + Environment.NewLine + $"... (truncated {truncatedChars} more characters)";
     }
 
     /// <summary>

src/SIL.XForge.Scripture/Services/LazyScrTextCollection.cs

@@ -32,7 +32,8 @@ public void Initialize(string projectsPath)
     }
 
     /// <summary>
-    /// Get a ScrText for a given user from the data for a paratext project with the target project ID and type.
+    /// Get a ScrText for a given user from the data for a paratext project with the target project ID and type. Not to
+    /// be confused with ParatextData ScrTextCollection.FindById, which this is not an override of.
     /// </summary>
     /// <param name="ptUsername"> The username of the user retrieving the ScrText. </param>
     /// <param name="projectId"> The ID of the target project. </param>