fix(ext/cache): cache.put overwrites previous call (#18649)

cli/tests/integration/cache_tests.rs

@@ -127,3 +127,57 @@ fn cache_matching_package_json_dep_should_not_install_all() {
     "Initialize @types/node@18.8.2\n",
   ));
 }
+
+// Regression test for https://github.com/denoland/deno/issues/17299
+#[test]
+fn cache_put_overwrite() {
+  let test_context = TestContextBuilder::new().use_temp_cwd().build();
+  let temp_dir = test_context.temp_dir();
+
+  let part_one = r#"
+  const req = new Request('http://localhost/abc');
+  const res1 = new Response('res1');
+  const res2 = new Response('res2');
+
+  const cache = await caches.open('test');
+
+  await cache.put(req, res1);
+  await cache.put(req, res2);
+
+  const res = await cache.match(req).then((res) => res?.text());
+  console.log(res);
+    "#;
+
+  let part_two = r#"
+  const req = new Request("http://localhost/abc");
+  const res1 = new Response("res1");
+  const res2 = new Response("res2");
+
+  const cache = await caches.open("test");
+
+  // Swap the order of put() calls.
+  await cache.put(req, res2);
+  await cache.put(req, res1);
+
+  const res = await cache.match(req).then((res) => res?.text());
+  console.log(res);
+      "#;
+
+  temp_dir.write("cache_put.js", part_one);
+
+  let run_command =
+    test_context.new_command().args_vec(["run", "cache_put.js"]);
+
+  let output = run_command.run();
+  output.assert_matches_text("res2\n");
+  output.assert_exit_code(0);
+
+  // The wait will surface the bug as we check last written time
+  // when we overwrite a response.
+  std::thread::sleep(std::time::Duration::from_secs(1));
+
+  temp_dir.write("cache_put.js", part_two);
+  let output = run_command.run();
+  output.assert_matches_text("res1\n");
+  output.assert_exit_code(0);
+}

cli/tests/unit/cache_api_test.ts

@@ -173,3 +173,20 @@ Deno.test(async function cachePutFailedBody() {
   // if it fails to read the body, the cache should be empty
   assertEquals(response, undefined);
 });
+
+Deno.test(async function cachePutOverwrite() {
+  const cacheName = "cache-v1";
+  const cache = await caches.open(cacheName);
+
+  const request = new Request("https://example.com/overwrite");
+  const res1 = new Response("res1");
+  const res2 = new Response("res2");
+
+  await cache.put(request, res1);
+  const res = await cache.match(request);
+  assertEquals(await res?.text(), "res1");
+
+  await cache.put(request, res2);
+  const res_ = await cache.match(request);
+  assertEquals(await res_?.text(), "res2");
+});

ext/cache/sqlite.rs

@@ -285,29 +285,11 @@ impl Cache for SqliteBackedCache {
 async fn insert_cache_asset(
   db: Arc<Mutex<rusqlite::Connection>>,
   put: CachePutRequest,
-  body_key_start_time: Option<(String, u64)>,
+  response_body_key: Option<String>,
 ) -> Result<Option<String>, deno_core::anyhow::Error> {
   tokio::task::spawn_blocking(move || {
     let maybe_response_body = {
       let db = db.lock();
-      let mut response_body_key = None;
-      if let Some((body_key, start_time)) = body_key_start_time {
-        response_body_key = Some(body_key);
-          let last_inserted_at = db.query_row("
-          SELECT last_inserted_at FROM request_response_list
-          WHERE cache_id = ?1 AND request_url = ?2",
-          (put.cache_id, &put.request_url), |row| {
-            let last_inserted_at: i64 = row.get(0)?;
-            Ok(last_inserted_at)
-          }).optional()?;
-          if let Some(last_inserted) = last_inserted_at {
-            // Some other worker has already inserted this resource into the cache.
-            // Note: okay to unwrap() as it is always present when response_body_key is present.
-            if start_time > (last_inserted as u64) {
-              return Ok(None);
-            }
-          }
-      }
       db.query_row(
         "INSERT OR REPLACE INTO request_response_list
              (cache_id, request_url, request_headers, response_headers,
@@ -368,13 +350,23 @@ impl CachePutResource {
     let mut file = resource.borrow_mut().await;
     file.flush().await?;
     file.sync_all().await?;
-    insert_cache_asset(
+    let maybe_body_key = insert_cache_asset(
       self.db.clone(),
       self.put_request.clone(),
-      Some((self.response_body_key.clone(), self.start_time)),
+      Some(self.response_body_key.clone()),
     )
     .await?;
-    Ok(())
+    match maybe_body_key {
+      Some(key) => {
+        assert_eq!(key, self.response_body_key);
+        Ok(())
+      }
+      // This should never happen because we will always have
+      // body key associated with CachePutResource
+      None => Err(deno_core::anyhow::anyhow!(
+        "unexpected: response body key is None"
+      )),
+    }
   }
 }