mikebridge commented on PR #41824: URL: https://github.com/apache/superset/pull/41824#issuecomment-4897489018
## Multi-lens self-review (posting for transparency before anyone spends review time) We ran this PR through a six-lens parallel review panel (Python, committer, CD, clean-code, tidy-first, Amin-style scans) and verified every load-bearing claim against the pinned cachelib 0.13.0 source. **Verdicts: 0 blockers, unanimous approve-with-changes** — the invariant, skip-not-clobber choice, and deterministic test were endorsed by all six, but the panel found real hardening gaps. Findings below; a follow-up commit addressing them is queued. ### HIGH (all independently found by multiple lenses) 1. **Unconditional read-path locking** *(6/6 lenses)* — `try_serve_from_cache` invokes the locked prune on every read with a non-empty bucket, scanning up to 128 **full pickled payloads inside the lock** just to test existence. Readers serialize; writers can exhaust their retry budget behind reader prunes and skip — losing more cache benefit than the race being fixed. On `SupersetMetastoreCache` each lock attempt is a metadata-DB INSERT+commit. *Fix: gate the prune on actually-observed eviction (the candidate loop already sees `payload is None`), single non-blocking attempt on the read path, `cache.has()` (with fallback — `BaseCache.has` raises `NotImplementedError`) instead of `get()`.* 2. **Unfenced lock release** *(6/6)* — if a critical section outlives `_INDEX_LOCK_TIMEOUT=2s`, a successor legitimately acquires the lock; the original holder's unconditional `finally: cache.delete(lock_key)` then **releases the successor's lock**, and its late `cache.set` writes unlocked — so the docstring's "only ever written while holding the lock" overclaims. *Fix: per-acquisition token, compare-before-delete; the `has()` change collapses the hold time that makes expiry plausible.* 3. **Crash-orphaned lock** *(3 lenses; verified in cachelib source)* — `RedisCache.add` is `SETNX` followed by a **separate** `EXPIRE`. A process killed between the two leaves a TTL-less lock, and with skip-on-timeout that means **every index update for the bucket is silently skipped forever** (debug-level log only) until the view's `changed_on` rotates the key. *Fix: store a timestamp token as the lock value and break locks older than k×TTL on repeated failure; raise repeated-skip logging to warning.* ### MEDIUM - An exception escaping the prune's lock machinery now propagates to the outer `except → return None`, **discarding an already-computed cache hit** — housekeeping needs its own try/except. - Two mutation-testing gaps: the **timeout-skip invariant is untested** (mutating the skip into a fall-through passes the whole suite), and the **prune half of the race has no regression test** — both cheap with the existing `_RacingCache` harness. - Lock-skip/degraded events are unobservable in production; and the "backend without `add`" branch is unreachable (Flask-Caching always exposes it) — the *real* degradation is backends whose `add` isn't atomic (`BaseCache.add` returns `True` unconditionally; `SimpleCache.add` is check-then-act), which the docstring should state. ### LOW (condensed) Orphaned value blob when a store's index write is skipped (worth a comment; deleting is unsafe) · `cache: Any` → `flask_caching.Cache` or a small Protocol · test-harness inline imports / `Any`-typed events · a harness assertion can be swallowed by `store_result`'s defensive `except`, yielding a misleading failure message · patch `_INDEX_LOCK_ATTEMPTS` up inside the test for slow-CI headroom. --- **What the panel endorsed** (unchanged in the follow-up): skip-on-timeout as "the correct half of the invariant" (writing unlocked on timeout would reintroduce the clobber exactly under contention); the harness releasing the parked thread from inside a *failed* `add()` (proving the lock is engaged rather than inferring from timing); one `_update_index` helper for both writers; the prune re-reading under the lock. Mutation spot-checks confirmed the existing test kills the four primary regressions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
