Fix cache read VC replacement after a lost write lock

[bneradt]

A transaction that revalidates a stale cached object and cannot take the
cache write lock is sent back through a second cache lookup while it
still holds the cache read connection its first lookup opened. The read
that completes for that second lookup replaces the connection the
transaction is using: debug builds abort on the read connection
assertion in HttpCacheSM::state_cache_open_read(), and release builds
close that connection out from under the stale object saved as the retry
fallback, leaving the fallback pointing into freed memory. The re-lookup
runs for every cache_open_write_fail_action rather than only for the two
that configure a read retry, so fail action 2, which is documented to
serve the stale object instead of retrying anything, aborts a debug
build several times a day under production traffic.

This patch limits the re-lookup to the fail actions that configure a
read retry. A transaction that loses the write lock with a cached object
and no retry configured now hands that object straight to the freshness
handling that serves stale content, with no second lookup. The retry
actions do want that lookup, so this also makes replacing the read
connection explicit and drops the saved stale object along with the
connection that owns it, since neither can outlive the other. This adds
an autest covering both configurations that does not depend on
contention between transactions: denying the write lock through
max_open_write_retries makes the failure synchronous, and each
configuration aborts an unpatched debug build on the production
assertion.

The re-lookup arrived with the fail action 6 work in #12852, which
applied it to every non-default fail action; that commit's own test
notes the stale path is timing sensitive and does not exercise it. The
resulting aborts resemble the ones #13487 fixed, because both land in
HttpCacheSM while a cache write retry dispatches events, but they are a
distinct failure. #13487 stopped HttpSM from canceling its own captive
action, which aborts on the cancellation assertion in
HttpCacheSM.cc:138; this is the read connection assertion ten lines
later, reached with that action perfectly valid. Both fixes are needed,
and neither subsumes the other.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This pull request fixes a cache state-machine correctness issue that can occur when a transaction revalidates a stale cached object, loses the cache write lock, and then incorrectly re-enters a second cache lookup while still holding the original cache read VC—leading to debug assertions and potential stale-fallback lifetime problems.

Changes:

• Limit the “re-lookup after lost write lock” behavior to only the open-write-fail actions that explicitly configure a read retry (actions 5 and 6).
• Make cache read VC replacement explicit for the READ_RETRY path and ensure stale retry fallback state does not outlive the VC that owns it.
• Add gold tests (ATSReplay) that deterministically exercise both “serve stale directly” (action 2) and “retry read then serve stale” (action 6) configurations.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/gold_tests/cache/replay/cache-write-lock-stale-serve.replay.yaml	New replay validating action 2 serves stale without issuing a read retry / second lookup.
tests/gold_tests/cache/replay/cache-write-lock-stale-retry.replay.yaml	New replay validating action 6 triggers a read retry and remains stable when the retry completes.
tests/gold_tests/cache/cache-write-lock-stale-revalidate.test.py	New AuTest entry point running both replays.
tests/gold_tests/cache/cache-write-lock-fail-write.conf	New header_rewrite rules file to deterministically deny the write lock (via max_open_write_retries=0).
src/proxy/http/HttpTransact.cc	Avoids second cache lookup for non-read-retry actions when a stale object is already present; preserves READ_RETRY behavior.
src/proxy/http/HttpCacheSM.cc	Allows cache read VC replacement in the READ_RETRY write-lock-loss path and clears stale retry fallback when the owning VC is replaced/closed.
include/proxy/http/HttpConfig.h	Adds a shared helper is_read_retry_write_fail_action() to centralize which fail actions configure a read retry.

[cmcfarlen]

I traced this through and the diagnosis holds up. Recording the path since it spans three files and is not obvious from any one of them.

HttpSM::state_cache_open_write() handles CACHE_EVENT_OPEN_WRITE_FAILED by bailing out to FAIL only for DEFAULT, for ERROR_ON_MISS_OR_REVALIDATE, or when there is no object_read. Everything else takes the INTENTIONAL FALL THROUGH into case CACHE_EVENT_OPEN_READ, and with object_read != nullptr that tail unconditionally ends in:

t_state.cache_info.write_lock_state = HttpTransact::CacheWriteLock_t::READ_RETRY;

So fail actions 2 and 3 arrive at the CacheWriteLock_t::READ_RETRY branch of handle_cache_write_lock() despite configuring no retry, and the stale sub-branch there saves stale_fallback and issues TRANSACT_RETURN(CACHE_LOOKUP) while the first lookup's read VC is still held. The CACHE_EVENT_OPEN_READ for that second lookup then trips ink_assert((cache_read_vc == nullptr) || ...), and in release builds close_read() frees the VC that owns the object stale_fallback points at. Fail action 2 is documented as "serve stale ... otherwise go to origin" with no retry in it at all, so it should never have been on that path.

Scoping the branch with is_read_retry_write_fail_action() is the right fix rather than widening the assertion alone, and routing 2/3 into HandleCacheOpenReadHitFreshness() matches what those actions are documented to do.

The deferred-hook question resolves in your favor, but only by a non-local invariant. My first concern was that the new early return calls HandleCacheOpenReadHitFreshness(s) with no cache_lookup_complete_deferred handling, unlike the fresh path just below it, which fires API_CACHE_LOOKUP_COMPLETE when deferred. That turns out to be safe: cache_lookup_complete_deferred is only ever set for actions 5 and 6 (HttpSM.cc:2738-2742), so it is guaranteed false on a path guarded by !is_read_retry_write_fail_action(...). The two predicates are exact complements.

Two things follow from that, and they are my only real asks:

1. Assert it. The guarantee lives in HttpSM.cc while the new guard lives in HttpTransact.cc; nothing connects them. An ink_assert(!s->cache_lookup_complete_deferred) in the new branch would pin the invariant where a future change to either side would notice.
2. The two predicates read different variables. HttpSM decides deferral from t_state.txn_conf->cache_open_write_fail_action, and the existing is_read_retry_write_fail_action() call sites in HttpCacheSM.cc also use txn_conf, but the new call in HttpTransact.cc uses the state copy s->cache_open_write_fail_action. They are equal wherever this matters, and the one place they diverge is the redirect case, which forces DEFAULT and write_lock_state = FAIL and so never reaches here. Still, "same predicate, different source of truth" is a trap; a comment or picking one consistently would help.

On clearing stale_fallback before close_read() — I convinced myself this does not cost action 6 its fallback, and it may be worth saying so in the description since it reads like a behavior loss. The clear only happens when a replacement read VC actually arrives, and in that case object_read is re-derived from the new VC and freshness re-evaluated, so the saved fallback is genuinely redundant. The case where action 6 needs the fallback is the second lookup missing, which yields CACHE_EVENT_OPEN_READ_FAILED, never reaches state_cache_open_read(), and so leaves stale_fallback intact for HandleCacheOpenReadMiss(). Trading a use-after-free for nothing at all is the good outcome, but it took a while to be sure of.

Hoisting is_read_retry_action out of the anonymous namespace into HttpConfig.h as is_read_retry_write_fail_action is fine now that two translation units need it, and the rename is clearer.

This wants backporting to 10.2.x, and I would treat it as RC-blocking. #12852 is already on 10.2.x as 49ab050e0e, and the vulnerable block is present there, so the regression ships in 10.2.0 as it stands — with the same "aborts a debug build several times a day under production traffic" exposure you describe. The good news is that it should pick cleanly: the new autest uses only Test.ATSReplayTest, and the autest: keys in both replay files (process_config, copy_to_config_dir, records_config, remap_config, log_validation) are all supported by 10.2.x's ats_replay.test.ext.

Agreed too that this is distinct from #13487 and neither subsumes the other: that one stopped HttpSM from canceling its own captive action via pending_action = adjust_thread(...), which fails the cancellation assertion; this one reaches the read-VC assertion with the action perfectly valid.

Nice touch making the test synchronous via max_open_write_retries instead of relying on contention between transactions — that is exactly the weakness in #13487's autest, which needs a delay: 3s / delay: 200ms overlap to provoke the race.

[cmcfarlen]

Cherry-picked to the 10.2.x branch as 601871f for the 10.2.0 release.