Merge pull request #476 from Shopify/fix-revalidation

Fix a cache corruption issue during revalidation
Shopify · Jan 31, 2024 · 08cd0d9 · 08cd0d9
2 parents 2dba040 + ad189d1
commit 08cd0d9
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 5 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,7 @@
 # Unreleased
 
+* Fix the cache corruption issue in the revalidation feature. See #474.
+
 # 1.18.2
 
 * Disable stale cache entries revalidation by default as it seems to cause cache corruption issues. See #471 and #474.

diff --git a/ext/bootsnap/bootsnap.c b/ext/bootsnap/bootsnap.c
@@ -124,7 +124,7 @@ static void bs_cache_path(const char * cachedir, const VALUE path, char (* cache
 static int bs_read_key(int fd, struct bs_cache_key * key);
 static enum cache_status cache_key_equal_fast_path(struct bs_cache_key * k1, struct bs_cache_key * k2);
 static int cache_key_equal_slow_path(struct bs_cache_key * current_key, struct bs_cache_key * cached_key, const VALUE input_data);
-static int update_cache_key(struct bs_cache_key *current_key, int cache_fd, const char ** errno_provenance);
+static int update_cache_key(struct bs_cache_key *current_key, struct bs_cache_key *old_key, int cache_fd, const char ** errno_provenance);
 
 static void bs_cache_key_digest(struct bs_cache_key * key, const VALUE input_data);
 static VALUE bs_fetch(char * path, VALUE path_v, char * cache_path, VALUE handler, VALUE args);
@@ -353,10 +353,11 @@ static int cache_key_equal_slow_path(struct bs_cache_key *current_key,
   return current_key->digest == cached_key->digest;
 }
 
-static int update_cache_key(struct bs_cache_key *current_key, int cache_fd, const char ** errno_provenance)
+static int update_cache_key(struct bs_cache_key *current_key, struct bs_cache_key *old_key, int cache_fd, const char ** errno_provenance)
 {
+  old_key->mtime = current_key->mtime;
   lseek(cache_fd, 0, SEEK_SET);
-  ssize_t nwrite = write(cache_fd, current_key, KEY_SIZE);
+  ssize_t nwrite = write(cache_fd, old_key, KEY_SIZE);
   if (nwrite < 0) {
       *errno_provenance = "update_cache_key:write";
       return -1;
@@ -825,7 +826,7 @@ bs_fetch(char * path, VALUE path_v, char * cache_path, VALUE handler, VALUE args
       valid_cache = cache_key_equal_slow_path(&current_key, &cached_key, input_data);
       if (valid_cache) {
         if (!readonly) {
-          if (update_cache_key(&current_key, cache_fd, &errno_provenance)) {
+          if (update_cache_key(&current_key, &cached_key, cache_fd, &errno_provenance)) {
               exception_message = path_v;
               goto fail_errno;
           }
@@ -993,7 +994,7 @@ bs_precompile(char * path, VALUE path_v, char * cache_path, VALUE handler)
       }
       valid_cache = cache_key_equal_slow_path(&current_key, &cached_key, input_data);
        if (valid_cache) {
-         if (update_cache_key(&current_key, cache_fd, &errno_provenance)) {
+         if (update_cache_key(&current_key, &cached_key, cache_fd, &errno_provenance)) {
              goto fail;
          }
       }

diff --git a/test/compile_cache_key_format_test.rb b/test/compile_cache_key_format_test.rb
@@ -20,6 +20,11 @@ class CompileCacheKeyFormatTest < Minitest::Test
     data_size: 32...40,
   }.freeze
 
+  def teardown
+    Bootsnap::CompileCache::Native.revalidation = false
+    super
+  end
+
   def test_key_version
     key = cache_key_for_file(FILE)
     exp = [5].pack("L")
@@ -79,6 +84,22 @@ def test_fetch
     assert_equal("NEATO #{target.upcase}", actual)
   end
 
+  def test_revalidation
+    Bootsnap::CompileCache::Native.revalidation = true
+    cache_dir = File.join(@tmp_dir, "compile_cache")
+
+    target = Help.set_file("a.rb", "foo = 1")
+
+    actual = Bootsnap::CompileCache::Native.fetch(cache_dir, target, TestHandler, nil)
+    assert_equal("NEATO #{target.upcase}", actual)
+
+    10.times do
+      FileUtils.touch(target, mtime: File.mtime(target) + 42)
+      actual = Bootsnap::CompileCache::Native.fetch(cache_dir, target, TestHandler, nil)
+      assert_equal("NEATO #{target.upcase}", actual)
+    end
+  end
+
   def test_unexistent_fetch
     assert_raises(Errno::ENOENT) do
       Bootsnap::CompileCache::Native.fetch(@tmp_dir, "123", Bootsnap::CompileCache::ISeq, nil)

diff --git a/test/compile_cache_test.rb b/test/compile_cache_test.rb
@@ -160,6 +160,24 @@ def test_dont_store_cache_after_a_stale_when_readonly
     load(path)
   end
 
+  def test_revalidation
+    Bootsnap::CompileCache::Native.revalidation = true
+
+    file_path = Help.set_file("a.rb", "a = a = 3", 100)
+    load(file_path)
+
+    calls = []
+    Bootsnap.instrumentation = ->(event, path) { calls << [event, path] }
+
+    5.times do
+      FileUtils.touch("a.rb", mtime: File.mtime("a.rb") + 42)
+      load(file_path)
+      load(file_path)
+    end
+
+    assert_equal [[:revalidated, "a.rb"], [:hit, "a.rb"]] * 5, calls
+  end
+
   def test_dont_revalidate_when_readonly
     Bootsnap::CompileCache::Native.revalidation = true