SF-1992 Fail sync if push failed by marksvc · Pull Request #3778 · sillsdev/web-xforge

marksvc · 2026-04-02T16:41:31Z

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.

This change is

devin-ai-integration

Devin Review found 3 potential issues.

View 2 additional findings in Devin Review.

devin-ai-integration · 2026-04-02T16:46:46Z

+        (bool isRevOnServer, int serverRevCount, string serverLastRev) = CheckPushRevisionOnServer(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}. "
+            );
+        }
+
+        _hgWrapper.MarkSharedChangeSetsPublic(repositoryPath);


🚩 Push verification failure prevents MarkSharedChangeSetsPublic from running

In the new Push method at src/SIL.XForge.Scripture/Services/JwtInternetSharedRepositorySource.cs:143-153, if CheckIfRevisionIsOnServer returns false (or throws an unexpected exception like a network error or JSON parse failure), the InvalidOperationException is thrown and MarkSharedChangeSetsPublic at line 153 is never called. This is a behavioral change from the old code where MarkSharedChangeSetsPublic was always called after a successful PostStreaming. If the push actually succeeded on the server but verification produces a false negative (e.g. eventual consistency delay, unexpected JSON structure, transient network issue), the local changesets remain in 'draft' phase, causing the next sync to re-bundle and re-push the same data. This appears to be the intended design (fail loudly so the issue can be investigated), but the risk of false negatives from CheckIfRevisionIsOnServer should be considered — especially since the projrevhist endpoint is queried immediately after the push completes.

Was this helpful? React with 👍 or 👎 to provide feedback.

codecov · 2026-04-02T16:50:01Z

Codecov Report

❌ Patch coverage is 47.45763% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.27%. Comparing base (a1eaeca) to head (9f04e10).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...ture/Services/JwtInternetSharedRepositorySource.cs	50.90%	23 Missing and 4 partials ⚠️
src/SIL.XForge.Scripture/Services/HgWrapper.cs	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3778      +/-   ##
==========================================
- Coverage   81.30%   81.27%   -0.03%     
==========================================
  Files         622      622              
  Lines       39273    39322      +49     
  Branches     6407     6415       +8     
==========================================
+ Hits        31929    31958      +29     
- Misses       6350     6366      +16     
- Partials      994      998       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

devin-ai-integration

Devin Review found 1 new potential issue.

View 2 additional findings in Devin Review.

devin-ai-integration · 2026-04-02T23:08:18Z

+    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);
+        JArray? revisions = jsonResult["project"]?["revision_history"]?["revisions"] as JArray;
+
+        bool isRevOnServer =
+            revisions?.Any(r => string.Equals(r["id"]?.ToString(), expectedRevision, StringComparison.Ordinal))
+            ?? false;
+        int serverRevCount = revisions?.Count ?? 0;
+        string? serverLastRev = GetFirstElementId(revisions);
+
+        return (isRevOnServer, serverRevCount, serverLastRev);
+    }


🚩 Verification assumes specific JSON structure from projrevhist API

The CheckIfRevisionIsOnServer method at line 156 assumes the projrevhist API returns JSON with the structure {"project":{"revision_history":{"revisions":[{"id":"..."},...]}}}. If the actual API response differs (e.g., different field names or nesting), the as JArray cast at line 165 would return null, causing isRevOnServer to be false and the push to be incorrectly reported as failed. Since this is calling an external Paratext server API, the assumed structure should be validated against actual API documentation or integration tests. The unit tests only verify the parsing logic with the assumed structure.

Was this helpful? React with 👍 or 👎 to provide feedback.

I wonder if it would be useful to log a warning if revisions is null, as if the API changes (or fails), as it might help to diagnose the issue. Maybe even logging the JSON to the console in this case?

Good thinking. How's this? Unfortunately these aren't included in SyncMetrics. (And I think couldn't without more elaborate enhancements.)

Hmm; it occurs to me that if the json output does include the revisions, but they are in a different format than we expect, logging the JSON might include thousands of revision IDs :). I adjusted the logging so it should truncate the output.

pmachapman

@pmachapman reviewed 6 files and all commit messages, made 4 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on marksvc).

src/SIL.XForge.Scripture/Services/HgWrapper.cs line 29 at r2 (raw file):

    }

    public string[] Pull(string repositoryPath, byte[] bundle)

I would argue we should keep the old parameter names, as these are what is used by Paratext. I think when we just exposing a PT method, we should keep method and parameter naming as close as possible to PT?

I'm not marking this as blocking, as if you feel strongly about renaming these, that's OK - I just thought I'd raise this.

Code quote:

public string[] Pull(string repositoryPath, byte[] bundle)

src/SIL.XForge.Scripture/Services/HgWrapper.cs line 89 at r2 (raw file):

    /// Mark all changesets as public.
    /// </summary>
    public void MarkSharedChangeSetsPublic(string repositoryPath) => RunCommand(repositoryPath, "phase -p -r 'tip'");

Can please we keep the codedoc param comment?

/// <param name="repositoryPath">The path to the repository.</param>

Visual Studio's intellisense uses these.

Code quote:

public void MarkSharedChangeSetsPublic(string repositoryPath)

src/SIL.XForge.Scripture/Services/IHgWrapper.cs line 18 at r2 (raw file):

    void MarkSharedChangeSetsPublic(string repositoryPath);
    string[] Pull(string repositoryPath, byte[] bundle);
    byte[] Bundle(string repositoryPath, params string[] baseRevisions);

I think for consistency these should all be either string repository or string repositoryPath. I would argue for string repository to keep consistency with Paratext (and so your PR doesn't change existing methods it doesn't need to)

Code quote:

    void Init(string repository);
    void Update(string repository);
    void Update(string repositoryPath, string rev);
    void BackupRepository(string repository, string backupFile);
    void RestoreRepository(string destination, string backupFile);
    string GetLastPublicRevision(string repository);
    string GetRepoRevision(string repositoryPath);
    void MarkSharedChangeSetsPublic(string repositoryPath);
    string[] Pull(string repositoryPath, byte[] bundle);
    byte[] Bundle(string repositoryPath, params string[] baseRevisions);

pmachapman · 2026-04-06T22:34:09Z

+    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);
+        JArray? revisions = jsonResult["project"]?["revision_history"]?["revisions"] as JArray;
+
+        bool isRevOnServer =
+            revisions?.Any(r => string.Equals(r["id"]?.ToString(), expectedRevision, StringComparison.Ordinal))
+            ?? false;
+        int serverRevCount = revisions?.Count ?? 0;
+        string? serverLastRev = GetFirstElementId(revisions);
+
+        return (isRevOnServer, serverRevCount, serverLastRev);
+    }


I wonder if it would be useful to log a warning if revisions is null, as if the API changes (or fails), as it might help to diagnose the issue. Maybe even logging the JSON to the console in this case?

marksvc

@marksvc made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on pmachapman).

src/SIL.XForge.Scripture/Services/HgWrapper.cs line 89 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Can please we keep the codedoc param comment?
/// <param name="repositoryPath">The path to the repository.</param>
Visual Studio's intellisense uses these.

Oh; yes. Please bring to my attention other places where I'm pruning too much as well.

src/SIL.XForge.Scripture/Services/IHgWrapper.cs line 18 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I think for consistency these should all be either string repository or string repositoryPath. I would argue for string repository to keep consistency with Paratext (and so your PR doesn't change existing methods it doesn't need to)

Done.

marksvc · 2026-04-07T17:45:12Z

+    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);
+        JArray? revisions = jsonResult["project"]?["revision_history"]?["revisions"] as JArray;
+
+        bool isRevOnServer =
+            revisions?.Any(r => string.Equals(r["id"]?.ToString(), expectedRevision, StringComparison.Ordinal))
+            ?? false;
+        int serverRevCount = revisions?.Count ?? 0;
+        string? serverLastRev = GetFirstElementId(revisions);
+
+        return (isRevOnServer, serverRevCount, serverLastRev);
+    }


Good thinking. How's this? Unfortunately these aren't included in SyncMetrics. (And I think couldn't without more elaborate enhancements.)

Hmm; it occurs to me that if the json output does include the revisions, but they are in a different format than we expect, logging the JSON might include thousands of revision IDs :). I adjusted the logging so it should truncate the output.

pmachapman

@pmachapman reviewed 4 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on marksvc).

- 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.

marksvc marked this pull request as draft April 2, 2026 16:41

devin-ai-integration bot reviewed Apr 2, 2026

View reviewed changes

marksvc force-pushed the task/verify-push branch from 758623f to e1401a2 Compare April 2, 2026 23:00

devin-ai-integration bot reviewed Apr 2, 2026

View reviewed changes

marksvc added will require testing PR should not be merged until testers confirm testing is complete e2e Run e2e tests for this pull request labels Apr 3, 2026

marksvc marked this pull request as ready for review April 3, 2026 00:30

marksvc had a problem deploying to e2e_tests April 3, 2026 00:32 — with GitHub Actions Failure

marksvc temporarily deployed to e2e_tests April 3, 2026 00:32 — with GitHub Actions Inactive

marksvc had a problem deploying to e2e_tests April 3, 2026 00:32 — with GitHub Actions Failure

marksvc temporarily deployed to e2e_tests April 3, 2026 00:32 — with GitHub Actions Inactive

marksvc temporarily deployed to e2e_tests April 6, 2026 19:11 — with GitHub Actions Inactive

pmachapman requested changes Apr 6, 2026

View reviewed changes

pmachapman self-assigned this Apr 6, 2026

marksvc force-pushed the task/verify-push branch from e1401a2 to 20ecdf0 Compare April 7, 2026 17:45

marksvc commented Apr 7, 2026

View reviewed changes

marksvc temporarily deployed to e2e_tests April 7, 2026 17:47 — with GitHub Actions Inactive

pmachapman approved these changes Apr 7, 2026

View reviewed changes

pmachapman added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Apr 7, 2026

pmachapman temporarily deployed to e2e_tests April 7, 2026 22:35 — with GitHub Actions Inactive

marksvc added 2 commits April 8, 2026 10:13

log warning on null revisions response

9f04e10

marksvc force-pushed the task/verify-push branch from 20ecdf0 to 9f04e10 Compare April 8, 2026 16:32

marksvc added testing complete Testing of PR is complete and should no longer hold up merging of the PR and removed ready to test labels Apr 8, 2026

marksvc added the testing complete Testing of PR is complete and should no longer hold up merging of the PR label Apr 8, 2026

marksvc temporarily deployed to e2e_tests April 8, 2026 16:35 — with GitHub Actions Inactive

marksvc enabled auto-merge (squash) April 8, 2026 16:35

marksvc merged commit b691ada into master Apr 8, 2026
33 of 34 checks passed

marksvc deleted the task/verify-push branch April 8, 2026 16:39

Uh oh!

Conversation

marksvc commented Apr 2, 2026 • edited by irahopkinson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

devin-ai-integration bot Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmachapman Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

marksvc Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

pmachapman left a comment

Choose a reason for hiding this comment

Uh oh!

pmachapman Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

marksvc left a comment

Choose a reason for hiding this comment

Uh oh!

marksvc Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

pmachapman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

marksvc commented Apr 2, 2026 •

edited by irahopkinson

Loading

devin-ai-integration bot Apr 2, 2026 •

edited

Loading

codecov bot commented Apr 2, 2026 •

edited

Loading

devin-ai-integration bot Apr 2, 2026 •

edited

Loading