mikebridge commented on PR #41824: URL: https://github.com/apache/superset/pull/41824#issuecomment-4897693591
## Round-2 review (six-lens panel, run via Claude) Re-ran the same six-lens review panel over the cumulative diff after the hardening commit, with an adversarial brief: *verify the round-1 fixes hold and hunt for defects the new code introduced*. Full disclosure: this is an AI-generated review (Claude), with every load-bearing claim verified against source or by execution. **Round-1 findings: all resolved, 6/6 lenses — two verified empirically.** One lens re-ran the suite in an isolated worktree (73/73, ruff clean); another ran the four concurrency tests **30/30 consecutive passes** (0.42–0.80 s, no sleeps in the pass path) and confirmed **all four fail fast against the base branch** (0.73 s, no hangs) — genuinely pinning, CI-safe. **New findings — all in the round-2 hardening code itself** (each degrades to the pre-fix benign lost-entry, never wrong data; fixes queued as round 3): 1. **Token timestamp is minted before the retry loop** *(2 lenses)* — a slow acquisition yields a lock that is stale at birth and immediately breakable → two holders. The docstring says "acquisition timestamp"; the code implements generation time. Fix: mint per attempt. 2. **The stale-lock break is delete-then-add, unfenced** *(6/6 lenses; one built a threaded repro demonstrating the double-acquire — and showing the release fence correctly contained the cascade)* — two waiters can both observe the stale value and the loser's delayed delete removes the winner's fresh lock. Fix: re-get-compare before delete, scope the docstring, log breaks at info (herd storms). 3. **The prune holds the lock across O(entries) backend round-trips** *(5 lenses)* — up to 128 sequential existence checks can approach the 2 s TTL on slow links, contradicting the "critical section far under the TTL" justification. Fix: collect the dead value-keys already observed in the candidate loop and prune only those — zero backend calls inside the lock. 4. **4 s staleness threshold is a thin clock-skew tolerance** *(4 lenses)* — a waiter 4 s ahead breaks every live lock it sees. Fix: raise the factor; the guarded failure (frozen bucket) is the rarer event. 5. **Mutation-testing gaps** *(4 lenses)*: the release fence is currently unfalsifiable (a blind-delete mutation survives the suite); the breaker-not-fired property is claimed but unasserted; the lock TTL is unobserved; the clean-hit-lock-free property is unpinned. Fix: a batch of kill-tests, each one fake-tweak. 6. Assorted: `"token:nan"` parses as never-stale (an unbreakable wedge — `math.isfinite` guard); the break's `continue` consumes the only attempt on the prune path; two docstring/log-wording overclaims; the PR body predates the hardening commit and will be refreshed. **CI note**: the red pre-commit at HEAD is an infra flake (the "Enable brew and helm-docs" setup step died with "Broken pipe" before any hook ran) — will re-trigger. Verdicts: SHIP ×1, approve ×2, approve-with-nits ×2, polish-first ×1 · 0 blockers. -- 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]
