fix(branch): stop retry storms on ACL/SQL failures and no-op metadata edits

.github/workflows/quick.yml

@@ -23,6 +23,9 @@ jobs:
       - name: Block retired managed-library code and contracts
         run: python3 tests/test_v2_no_managed_library.py
 
+      - name: Run cheap C-source ACL and Branch retry contracts
+        run: python3 -m unittest discover -s tests -p 'test_cf_*.py'
+
       - name: Validate only changed source
         shell: bash
         run: |

common/branch-mgr.c

@@ -518,18 +518,20 @@ test_and_update_branch (SeafBranchManager *mgr,
     default:
         g_return_val_if_reached (-1);
     }
-    if (seaf_db_trans_foreach_selected_row (trans, sql,
-                                            get_commit_id, commit_id,
-                                            2, "string", branch->name,
-                                            "string", branch->repo_id) < 0) {
+    int head_rows = seaf_db_trans_foreach_selected_row (trans, sql,
+                                                          get_commit_id, commit_id,
+                                                          2, "string", branch->name,
+                                                          "string", branch->repo_id);
+    /* A missing/corrupt Branch row is not a confirmed HEAD race. */
+    if (head_rows != 1) {
         seaf_db_rollback (trans);
         seaf_db_trans_close (trans);
-        return -1;
+        return SEAF_BRANCH_UPDATE_ERROR;
     }
     if (strcmp (old_commit_id, commit_id) != 0) {
         seaf_db_rollback (trans);
         seaf_db_trans_close (trans);
-        return -1;
+        return SEAF_BRANCH_UPDATE_HEAD_CONFLICT;
     }
 
     /* Branch is locked before consulting the current authority. ACL writers
@@ -564,7 +566,7 @@ test_and_update_branch (SeafBranchManager *mgr,
     if (acl_result < 0) {
         seaf_db_rollback (trans);
         seaf_db_trans_close (trans);
-        return -2;
+        return SEAF_BRANCH_UPDATE_ACL_DENIED;
     }
 
     sql = "UPDATE Branch SET commit_id = ? "

common/branch-mgr.h

@@ -52,6 +52,11 @@ seaf_branch_manager_update_branch (SeafBranchManager *mgr,
                                    SeafBranch *branch);
 
 #ifdef SEAFILE_SERVER
+/* Only a changed HEAD is retryable. Policy or SQL failure must stop
+ * immediately; the caller cannot recover it by creating more commits. */
+#define SEAF_BRANCH_UPDATE_ERROR         (-1)
+#define SEAF_BRANCH_UPDATE_ACL_DENIED    (-2)
+#define SEAF_BRANCH_UPDATE_HEAD_CONFLICT (-3)
 /**
  * Atomically test whether the current head commit id on @branch
  * is the same as @old_commit_id and update branch in db.

server/repo-mgr.c

@@ -4294,6 +4294,7 @@ seaf_repo_manager_edit_repo (const char *repo_id,
     SeafRepo *repo = NULL;
     SeafCommit *commit = NULL, *parent = NULL;
     int ret = 0;
+    int retry_cnt = 0;
 
     if (!name && !description) {
         g_set_error (error, SEAFILE_DOMAIN, SEAF_ERR_BAD_ARGS,
@@ -4312,10 +4313,9 @@ seaf_repo_manager_edit_repo (const char *repo_id,
         g_set_error (error, SEAFILE_DOMAIN, SEAF_ERR_BAD_ARGS, "No such library");
         return -1;
     }
-    if (!name)
-        name = repo->name;
-    if (!description)
-        description = repo->desc;
+    /* Do not retain a previous head's metadata across a HEAD-conflict retry. */
+    const char *effective_name = name ? name : repo->name;
+    const char *effective_description = description ? description : repo->desc;
 
     /*
      * We only change repo_name or repo_desc, so just copy the head commit
@@ -4330,42 +4330,54 @@ seaf_repo_manager_edit_repo (const char *repo_id,
         ret = -1;
         goto out;
     }
-    if (!user) {
-        user = parent->creator_name;
-    }
+    /* Idempotent metadata writes must never create a commit or take ACL locks. */
+    if (g_strcmp0 (parent->repo_name, effective_name) == 0 &&
+        g_strcmp0 (parent->repo_desc, effective_description) == 0)
+        goto out;
+    const char *effective_user = user ? user : parent->creator_name;
 
     commit = seaf_commit_new (NULL,
                               repo->id,
                               parent->root_id,
-                              user,
+                              effective_user,
                               EMPTY_SHA1,
                               "Changed library name or description",
                               0);
     commit->parent_id = g_strdup(parent->commit_id);
     seaf_repo_to_commit (repo, commit);
 
     g_free (commit->repo_name);
-    commit->repo_name = g_strdup(name);
+    commit->repo_name = g_strdup(effective_name);
     g_free (commit->repo_desc);
-    commit->repo_desc = g_strdup(description);
+    commit->repo_desc = g_strdup(effective_description);
 
     if (seaf_commit_manager_add_commit (seaf->commit_mgr, commit) < 0) {
         ret = -1;
         goto out;
     }
 
     seaf_branch_set_commit (repo->head, commit->commit_id);
-    if (seaf_branch_manager_test_and_update_branch (seaf->branch_mgr,
-                                                    repo->head,
-                                                    parent->commit_id,
-                                                    FALSE, NULL, NULL, NULL) < 0) {
-        seaf_repo_unref (repo);
-        seaf_commit_unref (commit);
-        seaf_commit_unref (parent);
-        repo = NULL;
-        commit = NULL;
-        parent = NULL;
-        goto retry;
+    int published = seaf_branch_manager_test_and_update_branch (seaf->branch_mgr,
+                                                                 repo->head,
+                                                                 parent->commit_id,
+                                                                 FALSE, NULL, NULL, NULL);
+    if (published < 0) {
+        if (published == SEAF_BRANCH_UPDATE_HEAD_CONFLICT && retry_cnt < 3) {
+            ++retry_cnt;
+            seaf_repo_unref (repo);
+            seaf_commit_unref (commit);
+            seaf_commit_unref (parent);
+            repo = NULL;
+            commit = parent = NULL;
+            g_usleep (g_random_int_range (10, 51) * 1000);
+            goto retry;
+        }
+        g_set_error (error, SEAFILE_DOMAIN,
+                     published == SEAF_BRANCH_UPDATE_HEAD_CONFLICT ?
+                     SEAF_ERR_CONCURRENT_UPLOAD : SEAF_ERR_GENERAL,
+                     "Library metadata update rejected or unavailable");
+        ret = -1;
+        goto out;
     }
 
     seaf_repo_manager_update_repo_info (seaf->repo_mgr, repo_id, repo->head->commit_id);

server/repo-op.c

@@ -635,7 +635,7 @@ gen_new_commit_guarded (const char *repo_id,
             repo->store_id, &gc_conflict);
     if (published < 0)
     {
-        if (published == -2) {
+        if (published == SEAF_BRANCH_UPDATE_ACL_DENIED) {
             g_set_error (error, SEAFILE_DOMAIN, SEAF_ERR_GENERAL,
                          "CloudFile authority unavailable or fenced");
             ret = -1;
@@ -648,6 +648,12 @@ gen_new_commit_guarded (const char *repo_id,
             ret = -1;
             goto out;
         }
+        if (published != SEAF_BRANCH_UPDATE_HEAD_CONFLICT) {
+            g_set_error (error, SEAFILE_DOMAIN, SEAF_ERR_GENERAL,
+                         "Branch update failed (not a HEAD conflict)");
+            ret = -1;
+            goto out;
+        }
         if (!handle_concurrent_update) {
             g_set_error (error, SEAFILE_DOMAIN, SEAF_ERR_CONCURRENT_UPLOAD, "Concurrent upload");
             ret = -1;
@@ -4930,8 +4936,9 @@ put_file_with_condition (SeafRepoManager *mgr,
                     repo->store_id, &gc_conflict);
             if (published < 0) {
                 g_set_error (error, SEAFILE_DOMAIN,
-                             published == -2 ? SEAF_ERR_GENERAL :
-                             (gc_conflict ? SEAF_ERR_GC_CONFLICT : SEAF_ERR_CONCURRENT_UPLOAD),
+                             gc_conflict ? SEAF_ERR_GC_CONFLICT :
+                             (published == SEAF_BRANCH_UPDATE_HEAD_CONFLICT ?
+                              SEAF_ERR_CONCURRENT_UPLOAD : SEAF_ERR_GENERAL),
                              "Conditional no-op could not be committed");
                 ret = -1;
                 goto out;
@@ -6849,10 +6856,11 @@ seaf_repo_manager_revert_on_server (SeafRepoManager *mgr,
                                     const char *user_name,
                                     GError **error)
 {
-    SeafRepo *repo;
+    SeafRepo *repo = NULL;
     SeafCommit *commit = NULL, *new_commit = NULL;
     char desc[512];
     int ret = 0;
+    int retry_cnt = 0;
     gboolean cf_prepared = FALSE;
 
     /* Outside the retry label: rewinding the whole library is one operation no
@@ -6869,7 +6877,8 @@ seaf_repo_manager_revert_on_server (SeafRepoManager *mgr,
     if (!repo) {
         g_set_error (error, SEAFILE_DOMAIN, SEAF_ERR_GENERAL,
                      "No such repo");
-        return -1;
+        ret = -1;
+        goto out;
     }
 
     commit = seaf_commit_manager_get_commit (seaf->commit_mgr,
@@ -6903,17 +6912,27 @@ seaf_repo_manager_revert_on_server (SeafRepoManager *mgr,
     }
 
     seaf_branch_set_commit (repo->head, new_commit->commit_id);
-    if (seaf_branch_manager_test_and_update_branch (seaf->branch_mgr,
-                                                    repo->head,
-                                                    new_commit->parent_id,
-                                                    FALSE, NULL, NULL, NULL) < 0)
-    {
-        seaf_repo_unref (repo);
-        seaf_commit_unref (commit);
-        seaf_commit_unref (new_commit);
-        repo = NULL;
-        commit = new_commit = NULL;
-        goto retry;
+    int published = seaf_branch_manager_test_and_update_branch (seaf->branch_mgr,
+                                                                 repo->head,
+                                                                 new_commit->parent_id,
+                                                                 FALSE, NULL, NULL, NULL);
+    if (published < 0) {
+        if (published == SEAF_BRANCH_UPDATE_HEAD_CONFLICT && retry_cnt < 3) {
+            ++retry_cnt;
+            seaf_repo_unref (repo);
+            seaf_commit_unref (commit);
+            seaf_commit_unref (new_commit);
+            repo = NULL;
+            commit = new_commit = NULL;
+            g_usleep (g_random_int_range (10, 51) * 1000);
+            goto retry;
+        }
+        g_set_error (error, SEAFILE_DOMAIN,
+                     published == SEAF_BRANCH_UPDATE_HEAD_CONFLICT ?
+                     SEAF_ERR_CONCURRENT_UPLOAD : SEAF_ERR_GENERAL,
+                     "Revert branch update rejected or unavailable");
+        ret = -1;
+        goto out;
     }
 
     CF_FILEOP_COMMITTED (CF_OP_REVERT_REPO,

tests/test_cf_branch_retry_contract.py

@@ -0,0 +1,60 @@
+"""Cheap regression guard for C Branch CAS return codes and retry boundaries.
+
+This is a source-level contract test. The native ACL integration harness still
+requires an isolated MariaDB/Hub/C-server environment before merge.
+"""
+from pathlib import Path
+import unittest
+
+ROOT = Path(__file__).resolve().parents[1]
+
+
+def function(path, marker, until):
+    source = (ROOT / path).read_text(encoding="utf-8")
+    assert marker in source and until in source, path
+    return source.split(marker, 1)[1].split(until, 1)[0]
+
+
+class BranchRetryContract(unittest.TestCase):
+    def test_only_head_mismatch_is_classified_retryable(self):
+        hdr = (ROOT / "common/branch-mgr.h").read_text()
+        branch = function("common/branch-mgr.c",
+                          "test_and_update_branch (SeafBranchManager *mgr,",
+                          "\nint\nseaf_branch_manager_test_and_update_branch (")
+        self.assertIn("#define SEAF_BRANCH_UPDATE_HEAD_CONFLICT (-3)", hdr)
+        self.assertIn("if (head_rows != 1)", branch)
+        self.assertIn("return SEAF_BRANCH_UPDATE_ERROR;", branch)
+        self.assertIn("return SEAF_BRANCH_UPDATE_HEAD_CONFLICT;", branch)
+        self.assertIn("return SEAF_BRANCH_UPDATE_ACL_DENIED;", branch)
+        self.assertIn("return -1;", branch)  # SQL/transaction failure stays distinct
+
+    def test_metadata_noop_and_bounded_retry(self):
+        method = function("server/repo-mgr.c", "seaf_repo_manager_edit_repo (",
+                          "\ngboolean\nget_total_file_number_cb")
+        self.assertIn("g_strcmp0 (parent->repo_name, effective_name)", method)
+        self.assertIn("g_strcmp0 (parent->repo_desc, effective_description)", method)
+        self.assertLess(method.index("g_strcmp0 (parent->repo_name"),
+                        method.index("seaf_commit_new ("))
+        self.assertIn("published == SEAF_BRANCH_UPDATE_HEAD_CONFLICT && retry_cnt < 3", method)
+        self.assertIn("g_set_error (error", method)
+        self.assertIn("ret = -1;\n        goto out;", method)
+        self.assertNotIn("if (!user) {\n        user = parent->creator_name;", method)
+
+    def test_file_commit_retries_only_head_conflicts(self):
+        method = function("server/repo-op.c", "gen_new_commit_guarded (",
+                          "\nstatic int\ngen_new_commit (")
+        self.assertIn("if (published != SEAF_BRANCH_UPDATE_HEAD_CONFLICT)", method)
+        self.assertIn("if (published == SEAF_BRANCH_UPDATE_ACL_DENIED)", method)
+        self.assertIn("MAX_RETRY_COUNT 10", method)
+
+    def test_revert_retries_are_bounded_and_always_abort_on_error(self):
+        method = function("server/repo-op.c",
+                          "seaf_repo_manager_revert_on_server (",
+                          "\nstatic void\nadd_deleted_entry")
+        self.assertIn("published == SEAF_BRANCH_UPDATE_HEAD_CONFLICT && retry_cnt < 3", method)
+        self.assertIn("CF_FILEOP_ABORTED (CF_OP_REVERT_REPO", method)
+        self.assertIn("ret = -1;\n        goto out;", method)
+
+
+if __name__ == "__main__":
+    unittest.main()