reftable/stack: gracefully handle failed auto-compaction due to locks

Whenever we commit a new table to the reftable stack we will end up
invoking auto-compaction of the stack to keep the total number of tables
at bay. This auto-compaction may fail though in case at least one of the
tables which we are about to compact is locked. This is indicated by the
compaction function returning `REFTABLE_LOCK_ERROR`. We do not handle
this case though, and thus bubble that return value up the calling
chain, which will ultimately cause a failure.

Fix this bug by ignoring `REFTABLE_LOCK_ERROR`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/reftable/stack.c b/reftable/stack.c
index 79856b6565e447b1729842e466127edff1a8566c..dde50b61d696befd29bb16452e0e2588f0823761 100644 (file)
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -680,8 +680,19 @@ int reftable_addition_commit(struct reftable_addition *add)
        if (err)
                goto done;
 
-       if (!add->stack->disable_auto_compact)
+       if (!add->stack->disable_auto_compact) {
+               /*
+                * Auto-compact the stack to keep the number of tables in
+                * control. It is possible that a concurrent writer is already
+                * trying to compact parts of the stack, which would lead to a
+                * `REFTABLE_LOCK_ERROR` because parts of the stack are locked
+                * already. This is a benign error though, so we ignore it.
+                */
                err = reftable_stack_auto_compact(add->stack);
+               if (err < 0 && err != REFTABLE_LOCK_ERROR)
+                       goto done;
+               err = 0;
+       }
 
 done:
        reftable_addition_close(add);

diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index b0c7041a4fa6b1565a87bd7859a97260c484f365..887f458e5c3b5adba67ea363958740766487d241 100644 (file)
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -343,6 +343,49 @@ static void test_reftable_stack_transaction_api_performs_auto_compaction(void)
        clear_dir(dir);
 }
 
+static void test_reftable_stack_auto_compaction_fails_gracefully(void)
+{
+       struct reftable_ref_record ref = {
+               .refname = "refs/heads/master",
+               .update_index = 1,
+               .value_type = REFTABLE_REF_VAL1,
+               .value.val1 = {0x01},
+       };
+       struct reftable_write_options cfg = {0};
+       struct reftable_stack *st;
+       struct strbuf table_path = STRBUF_INIT;
+       char *dir = get_tmp_dir(__LINE__);
+       int err;
+
+       err = reftable_new_stack(&st, dir, cfg);
+       EXPECT_ERR(err);
+
+       err = reftable_stack_add(st, write_test_ref, &ref);
+       EXPECT_ERR(err);
+       EXPECT(st->merged->stack_len == 1);
+       EXPECT(st->stats.attempts == 0);
+       EXPECT(st->stats.failures == 0);
+
+       /*
+        * Lock the newly written table such that it cannot be compacted.
+        * Adding a new table to the stack should not be impacted by this, even
+        * though auto-compaction will now fail.
+        */
+       strbuf_addf(&table_path, "%s/%s.lock", dir, st->readers[0]->name);
+       write_file_buf(table_path.buf, "", 0);
+
+       ref.update_index = 2;
+       err = reftable_stack_add(st, write_test_ref, &ref);
+       EXPECT_ERR(err);
+       EXPECT(st->merged->stack_len == 2);
+       EXPECT(st->stats.attempts == 1);
+       EXPECT(st->stats.failures == 1);
+
+       reftable_stack_destroy(st);
+       strbuf_release(&table_path);
+       clear_dir(dir);
+}
+
 static void test_reftable_stack_validate_refname(void)
 {
        struct reftable_write_options cfg = { 0 };
@@ -1089,6 +1132,7 @@ int stack_test_main(int argc, const char *argv[])
        RUN_TEST(test_reftable_stack_tombstone);
        RUN_TEST(test_reftable_stack_transaction_api);
        RUN_TEST(test_reftable_stack_transaction_api_performs_auto_compaction);
+       RUN_TEST(test_reftable_stack_auto_compaction_fails_gracefully);
        RUN_TEST(test_reftable_stack_update_index_check);
        RUN_TEST(test_reftable_stack_uptodate);
        RUN_TEST(test_reftable_stack_validate_refname);

diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index 686781192eb7c2d820e28393b6d0352db3f4822e..5f2f9baa9b705bde72fdc334a39bee6ecebcc87e 100755 (executable)
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -340,6 +340,26 @@ test_expect_success 'ref transaction: empty transaction in empty repo' '
        EOF
 '
 
+test_expect_success 'ref transaction: fails gracefully when auto compaction fails' '
+       test_when_finished "rm -rf repo" &&
+       git init repo &&
+       (
+               cd repo &&
+
+               test_commit A &&
+               for i in $(test_seq 10)
+               do
+                       git branch branch-$i &&
+                       for table in .git/reftable/*.ref
+                       do
+                               touch "$table.lock" || exit 1
+                       done ||
+                       exit 1
+               done &&
+               test_line_count = 13 .git/reftable/tables.list
+       )
+'
+
 test_expect_success 'pack-refs: compacts tables' '
        test_when_finished "rm -rf repo" &&
        git init repo &&