From efa7c1ec97ca168bb824ccb7f41372f195623923 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Fri, 15 May 2026 18:40:14 +0200 Subject: [PATCH] LibHTTP: Publish disk cache entries by rename Write cache entries to a temporary file and rename them over the final path only after the entry has been fully flushed. This keeps previously mapped cache bodies tied to their original inode instead of exposing them to truncation when the same cache key is replaced. Clean up only the temporary file for incomplete writers so a failed replacement does not remove the still-valid existing cache entry. Add coverage that replaces an entry while an old body mapping remains live and verifies that it still exposes the original bytes. --- Libraries/LibHTTP/Cache/CacheEntry.cpp | 33 +++++++++++++++++++++----- Libraries/LibHTTP/Cache/CacheEntry.h | 2 ++ Tests/LibHTTP/TestDiskCache.cpp | 27 +++++++++++++++++++++ 3 files changed, 56 insertions(+), 6 deletions(-) diff --git a/Libraries/LibHTTP/Cache/CacheEntry.cpp b/Libraries/LibHTTP/Cache/CacheEntry.cpp index 279e788252..b62f9a86e7 100644 --- a/Libraries/LibHTTP/Cache/CacheEntry.cpp +++ b/Libraries/LibHTTP/Cache/CacheEntry.cpp @@ -119,6 +119,7 @@ CacheEntryWriter::CacheEntryWriter(DiskCache& disk_cache, CacheIndex& index, u64 ErrorOr CacheEntryWriter::write_status_and_reason(u32 status_code, Optional reason_phrase, HeaderList const& request_headers, HeaderList const& response_headers) { if (m_marked_for_deletion) { + remove_incomplete_temporary_file(); close_and_destroy_cache_entry(); return Error::from_string_literal("Cache entry has been deleted"); } @@ -137,6 +138,7 @@ ErrorOr CacheEntryWriter::write_status_and_reason(u32 status_code, Optiona m_vary_key = create_vary_key(request_headers, response_headers); m_path = path_for_cache_entry(m_disk_cache.cache_directory(), m_cache_key, m_vary_key); + m_temporary_path = LexicalPath::join(m_disk_cache.cache_directory().string(), ByteString::formatted("{}.tmp", m_path->basename())); auto freshness_lifetime = calculate_freshness_lifetime(status_code, response_headers, m_current_time_offset_for_testing); auto current_age = calculate_age(response_headers, m_request_time, m_response_time, m_current_time_offset_for_testing); @@ -146,7 +148,8 @@ ErrorOr CacheEntryWriter::write_status_and_reason(u32 status_code, Optiona if (cache_lifetime_status(request_headers, response_headers, freshness_lifetime, current_age) == CacheLifetimeStatus::Expired) return Error::from_string_literal("Response has already expired"); - auto unbuffered_file = TRY(Core::File::open(m_path->string(), Core::File::OpenMode::Write)); + (void)FileSystem::remove(m_temporary_path->string(), FileSystem::RecursionMode::Disallowed); + auto unbuffered_file = TRY(Core::File::open(m_temporary_path->string(), Core::File::OpenMode::Write | Core::File::OpenMode::MustBeNew)); m_file = TRY(Core::OutputBufferedFile::create(move(unbuffered_file))); TRY(m_file->write_value(m_cache_header)); @@ -161,7 +164,7 @@ ErrorOr CacheEntryWriter::write_status_and_reason(u32 status_code, Optiona if (result.is_error()) { dbgln_if(HTTP_DISK_CACHE_DEBUG, "\033[36m[disk]\033[0m \033[31;1mUnable to write status/reason to cache entry for\033[0m {}: {}", m_url, result.error()); - remove(); + remove_incomplete_temporary_file(); close_and_destroy_cache_entry(); return result.release_error(); @@ -173,6 +176,7 @@ ErrorOr CacheEntryWriter::write_status_and_reason(u32 status_code, Optiona ErrorOr CacheEntryWriter::write_data(ReadonlyBytes data) { if (m_marked_for_deletion) { + remove_incomplete_temporary_file(); close_and_destroy_cache_entry(); return Error::from_string_literal("Cache entry has been deleted"); } @@ -180,7 +184,7 @@ ErrorOr CacheEntryWriter::write_data(ReadonlyBytes data) if (auto result = m_file->write_until_depleted(data); result.is_error()) { dbgln_if(HTTP_DISK_CACHE_DEBUG, "\033[36m[disk]\033[0m \033[31;1mUnable to write data to cache entry for\033[0m {}: {}", m_url, result.error()); - remove(); + remove_incomplete_temporary_file(); close_and_destroy_cache_entry(); return result.release_error(); @@ -206,19 +210,29 @@ ErrorOr CacheEntryWriter::flush_impl(NonnullRefPtr request_hea { ScopeGuard guard { [&]() { close_and_destroy_cache_entry(); } }; - if (m_marked_for_deletion) + if (m_marked_for_deletion) { + remove_incomplete_temporary_file(); return Error::from_string_literal("Cache entry has been deleted"); + } + VERIFY(m_path.has_value()); + VERIFY(m_temporary_path.has_value()); + + ArmedScopeGuard remove_temporary_file = [&]() { + remove_incomplete_temporary_file(); + }; m_cache_footer.header_hash = m_cache_header.hash(); if (auto result = m_file->write_value(m_cache_footer); result.is_error()) { dbgln_if(HTTP_DISK_CACHE_DEBUG, "\033[36m[disk]\033[0m \033[31;1mUnable to flush cache entry for\033[0m {}: {}", m_url, result.error()); - remove(); return result.release_error(); } TRY(m_file->flush_buffer()); + m_file.clear(); + TRY(Core::System::rename(m_temporary_path->string(), m_path->string())); + remove_temporary_file.disarm(); int body_fd = -1; ArmedScopeGuard close_body_fd = [&] { @@ -254,10 +268,17 @@ ErrorOr CacheEntryWriter::flush_impl(NonnullRefPtr request_hea void CacheEntryWriter::remove_incomplete_entry() { - remove(); + remove_incomplete_temporary_file(); close_and_destroy_cache_entry(); } +void CacheEntryWriter::remove_incomplete_temporary_file() +{ + if (!m_temporary_path.has_value()) + return; + (void)FileSystem::remove(m_temporary_path->string(), FileSystem::RecursionMode::Disallowed); +} + ErrorOr> CacheEntryReader::create(DiskCache& disk_cache, CacheIndex& index, u64 cache_key, u64 vary_key, NonnullRefPtr response_headers, u64 data_size) { auto path = path_for_cache_entry(disk_cache.cache_directory(), cache_key, vary_key); diff --git a/Libraries/LibHTTP/Cache/CacheEntry.h b/Libraries/LibHTTP/Cache/CacheEntry.h index 9f46119b8e..229b233363 100644 --- a/Libraries/LibHTTP/Cache/CacheEntry.h +++ b/Libraries/LibHTTP/Cache/CacheEntry.h @@ -107,8 +107,10 @@ private: CacheEntryWriter(DiskCache&, CacheIndex&, u64 cache_key, String url, CacheHeader, UnixDateTime request_time, AK::Duration current_time_offset_for_testing); ErrorOr flush_impl(NonnullRefPtr request_headers, NonnullRefPtr response_headers, CacheEntryBodyFile*); + void remove_incomplete_temporary_file(); OwnPtr m_file; + Optional m_temporary_path; u64 m_data_offset { 0 }; UnixDateTime m_request_time; diff --git a/Tests/LibHTTP/TestDiskCache.cpp b/Tests/LibHTTP/TestDiskCache.cpp index 0f3a1daa6d..ccb9200df6 100644 --- a/Tests/LibHTTP/TestDiskCache.cpp +++ b/Tests/LibHTTP/TestDiskCache.cpp @@ -135,6 +135,33 @@ TEST_CASE(flush_returns_mappable_body_file) EXPECT_EQ(body.bytes(), "console.log('hello');"sv.bytes()); } +TEST_CASE(replacing_cache_entry_keeps_existing_body_mapping_stable) +{ + auto disk_cache = MUST(HTTP::DiskCache::create(HTTP::DiskCache::Mode::Testing)); + TestCacheRequest request; + + auto url = parse_url("https://example.com/script.js"sv); + auto request_headers = create_cacheable_request_headers(); + auto response_headers = create_cacheable_response_headers(); + + auto& writer = create_cache_entry(disk_cache, request, url, *request_headers); + TRY_OR_FAIL(writer.write_status_and_reason(200, "OK"_string, *request_headers, *response_headers)); + TRY_OR_FAIL(writer.write_data("console.log('old');"sv.bytes())); + + auto old_body_file = TRY_OR_FAIL(writer.flush_and_take_body_file(request_headers, response_headers)); + auto old_body = TRY_OR_FAIL(Core::ImmutableBytes::map_from_fd_range_and_close(old_body_file.fd, "old cache body"sv, old_body_file.offset, old_body_file.size)); + EXPECT_EQ(old_body.bytes(), "console.log('old');"sv.bytes()); + + auto replacement_request_headers = create_cacheable_request_headers(); + auto replacement_response_headers = create_cacheable_response_headers(); + auto& replacement_writer = create_cache_entry(disk_cache, request, url, *replacement_request_headers); + TRY_OR_FAIL(replacement_writer.write_status_and_reason(200, "OK"_string, *replacement_request_headers, *replacement_response_headers)); + TRY_OR_FAIL(replacement_writer.write_data("console.log('new');"sv.bytes())); + TRY_OR_FAIL(replacement_writer.flush(replacement_request_headers, replacement_response_headers)); + + EXPECT_EQ(old_body.bytes(), "console.log('old');"sv.bytes()); +} + TEST_CASE(associated_data_round_trips_with_explicit_vary_key) { auto disk_cache = MUST(HTTP::DiskCache::create(HTTP::DiskCache::Mode::Testing));