feat(state): single-blob HTTP cache entry + drop cache_mutex (atomicity Phase 2) - #494
Merged
Merged
Conversation
…cache_mutex (Phase 2) Phase 2 / atomicity of docs/roadmap/STATE_LIFETIME_AND_ATOMICITY.md, building on the Phase 1 owning accessors (#492). Before: an entry was 7 metadata child value-nodes (status/etag/last_modified/ content_type/effective_url/fetched_at/freshness_ttl) plus the body on the entry node's data(). Reading/writing that spread across N node locks needed a process-wide cache_mutex to avoid a torn (half-written) entry AND to stop a concurrent sweep freeing a node under a bare pointer. After: the whole entry is ONE opaque blob on the entry node's data() (version byte + 8 length-prefixed fields). A read or write is now a SINGLE atomic node op under that node's own _mutex — a reader sees the whole old blob or the whole new blob, never a mix. read_entry uses findDescendantShared() (Phase 1) to PIN the node, so a concurrent sweepExpired() can only unlink it, never free it under the reader. Writers of one key are already serialized by the single-flight flight_mutex, and the write is atomic, so there is nothing left for a global lock to protect: cache_mutex is REMOVED. Reads no longer take any process-wide lock (lock-free apart from the node's own brief _mutex). Net: lock-free, torn-read-free cache reads; the cache's only write coordination is the pre-existing per-key single-flight. Entry child nodes are no longer individually addressable (they were internal-only; no consumer read them — verified). Test file unchanged (its checks are entry presence/absence + expireAt, both preserved). All 12 HttpCache tests pass, incl. ConcurrentDistinctKeysAreServedSafely now without cache_mutex; AriadneStateUri/StateLifetime/AriadneNetIntrinsics green (38/38).
…-vs-evict UAF) Adversarial review of the cache_mutex removal found a HIGH UAF I introduced: with cache_mutex gone, write_entry still used entries(key) (operator(), a bare unpinned state&) and then mutated it via data()/expireAt, which lock the node and walk its _parent. A concurrent evict/sweepExpired (a same-key store invalidation — an independent URI handler no longer serialized against the fetch write) could free the node between entries(key) returning and the data()/expireAt calls → use-after-free (or a silently-lost write). read_entry and evict were already hardened with owning accessors; write_entry was the one mutation site left on a bare ref. Fix: write_entry now uses entries.sharedChild(key) (owning create-or-get), so the entry node is PINNED across data()/expireAt — a concurrent evict can only unlink it, never free it under the write. The entry's ancestors are the fixed, never-expiring sys.net.http_cache.entries path, so the entry pin alone suffices (no chain needed). Also refreshed the stale header comment to describe the single-blob storage. 12/12 HttpCache tests pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 2 / atomicity of
docs/roadmap/STATE_LIFETIME_AND_ATOMICITY.md, building on the Phase 1 owning accessors (#492).Before
An HTTP cache entry was 7 metadata child value-nodes (
status/etag/last_modified/content_type/effective_url/fetched_at/freshness_ttl) plus the body on the entry node'sdata(). Reading or writing that spread across many node locks, so it needed a process-widecache_mutexfor two jobs: (1) atomicity — a reader must not see a half-written entry; (2) lifetime — a concurrentsweepExpired()must not free a node under a bare pointer.After
The whole entry is one opaque blob on the entry node's
data()(version byte + 8 length-prefixed fields). So:_mutex; a reader sees the whole old blob or the whole new blob, never a torn mix.read_entryusesfindDescendantShared()(Phase 1) to pin the node, so a concurrentsweepExpired()can only unlink it, never free it under the reader.flight_mutex), and the write is atomic.With atomicity + lifetime + single-flight covered, there's nothing left for a global lock to protect —
cache_mutexis removed. Cache reads take no process-wide lock (lock-free apart from the node's own brief_mutex); the only write coordination is the pre-existing single-flight.Notes / scope
state://…?children— they were internal-only and no consumer read them (verified by grep; only the test located the entry node, which still exists).expireAt, both preserved. All 12HttpCacheTestpass, includingConcurrentDistinctKeysAreServedSafelynow withoutcache_mutex;AriadneStateUri/StateLifetime/AriadneNetIntrinsicsgreen (38/38 locally).uint64lengths) with a version byte; a malformed/old-version blob decodes as a cache miss (safe re-fetch).Not in scope: PR3 cache hardening (LRU budget, Vary/private, Expires) — still tracked separately.