cmcfarlen commented on PR #13515: URL: https://github.com/apache/trafficserver/pull/13515#issuecomment-5220043589
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: ```cpp 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
