Bound cache directory bucket walks in all builds

[masaori335]

The loop protection in probe/overwrite/remove/dir_clean_bucket sat behind LOOP_CHECK_MODE, which is commented out and not settable from CMake, so production builds walked bucket chains unbounded. A cycle in dir_next spins an event thread while it holds the stripe mutex.

I have never seen this infinite loop in production. This is defensive change and cleanup of the LOOP_CHECK_MODE.

Directory::max_bucket_depth() is the bound, (DIR_DEPTH - 1) * buckets + 1: init_segment() frees rows 1..DIR_DEPTH-1 of every bucket onto the free list and never row 0, so one chain holds at most every free entry of the segment plus its own head. Exceeding it proves a cycle, so no exact check is needed at the trigger. bucket_length() capped at a hardcoded 100 instead, below the longest legitimate chain, so a chain over that would make check_segment() report corruption and Stripe::_shm_directory_is_valid() reject a healthy shared memory attach.

Readers report and move on, writers repair. probe() throttles its report because it does not repair, so the loop lives until a writer clears it and every lookup that hashes to the bucket lands on the same trigger. insert()'s tail walk is the only chain walk it makes, so that is where a writer meets a cycle, and stopping at the cap linked the new entry into the cycle.

Stripe::dir_check() had no cap at all and wrote chain_tag[] once per step, so a cycle ran off the end of a 65536-entry stack array. It now stops at the bound, and at the first revisited entry rather than printing a cycle line per step. check_bucket_not_contains(), an unbounded walk with no caller since the 2009 import, is deleted.

[Copilot]

Pull request overview

This PR makes cache directory bucket-chain walks bounded in all builds to prevent event-thread hangs (and potential stack overruns) when directory corruption introduces cycles in dir_next chains.

Changes:

• Introduces Directory::max_bucket_depth() as a derived, segment-aware upper bound and uses it to cap/trigger handling for bucket chain walks.
• Removes the dead LOOP_CHECK_MODE gating and replaces the prior hardcoded bucket-length cap with the derived bound.
• Updates Stripe::dir_check() and cache dir unit tests to validate bounded traversal and repair behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/iocore/cache/CacheDir.cc	Replaces unbounded/compile-time-gated bucket walks with max_bucket_depth()-bounded logic and adjusts loop handling across probe/insert/overwrite/remove/clean paths.
src/iocore/cache/P_CacheDir.h	Adds Directory::max_bucket_depth() API and documentation explaining the derivation from directory layout invariants.
src/iocore/cache/Stripe.cc	Bounds Stripe::dir_check() chain walking to prevent runaway traversal/array overrun when chains loop.
src/iocore/cache/unit_tests/test_CacheDir.cc	Reworks corruption tests to reliably exercise bounded reader behavior and writer repair paths in all builds.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bneradt]

Reviewed the bounded walks, segment reset paths, and updated unit tests. One persistence issue noted inline. All reported PR checks pass; local tests were not run because this worktree has no configured build.

[Copilot]

🔵 Needs a closer look

Resolve gauge accounting drift and add coverage for the diagnostic walk safety guard.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

src/iocore/cache/CacheDir.cc:223

• init_segment() now runs from the new production repair paths and clears every occupied entry in the segment, but it only rebuilds the free list; it never decrements cache_rsb.direntries_used or the volume gauge for the entries discarded. After a loop repair, subsequent inserts increment those gauges again, so cache statistics overcount live entries (and the later exact entries_used() subtraction cannot fully undo the drift). Account for the entries removed when resetting a segment, or resynchronize the gauges as part of the repair.
  src/iocore/cache/P_CacheDir.h:313
• max_bucket_depth() bounds valid chains, but it does not prove that an overlong chain contains a cycle: a corrupted link can point at another bucket's row 0 and produce an acyclic path longer than this bound. The implementation deliberately has a fallback for that case, so this public comment is misleading; describe the condition as an overlong/corrupt chain rather than asserting that an entry was revisited.

src/iocore/cache/unit_tests/test_CacheDir.cc:194

• This test verifies the new bound through Directory::check(), but never exercises Stripe::dir_check() or its chain_tag stack guard added in the PR. A regression in that diagnostic walk could still write past the array even while all assertions here pass; invoke dir_check() with the loop installed (or add an equivalent focused test) to cover this safety-critical path.

      // Detection: a cycle makes the chain longer than its segment can hold.
      CHECK(!stripe->directory.check());
      CHECK(stripe->directory.bucket_length(dir_bucket(b1, stripe->directory.get_segment(s1)), s1) == -1);

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[bneradt]

Follow-up review of b2763ee: the earlier persistence finding is addressed. init_segment() now marks the directory dirty, and the new tests check the remove and must-overwrite early-return paths as well as the no-repair case.

One remaining P2 issue is confirmed inline: repair leaves the used-entry gauges inflated (also mentioned in the automated review summary). I found no additional correctness findings in the traversal caps. The maximum valid chain is 49,153 entries at the maximum segment size, below the 65,536-element diagnostic array.

All 15 reported CI checks pass. This was a source review; I did not run the cache unit tests locally because this worktree has no configured ATS build.

[Copilot]

🔵 Needs a closer look

Cleanup can abandon an acyclic chain, and the corruption helper can fail to create a loop deterministically.

Review details

Suppressed comments (2)

src/iocore/cache/CacheDir.cc:338

• When the cap is exceeded by an acyclic cross-bucket chain, bucket_loop_fix() returns 0, but this unconditional return abandons dir_clean_bucket() without cleaning the remainder of the chain. clean_segment() then leaves stale/invalid entries unreclaimed; the previous guarded implementation only returned when a cycle was actually repaired and continued for the acyclic case. Continue the walk when the exact loop check returns 0, and return only after a repair.

    if (++loop_count > stripe->directory.max_bucket_depth()) {
      stripe->directory.bucket_loop_fix(b, s, stripe);
      return;

src/iocore/cache/unit_tests/test_CacheDir.cc:186

• This helper can leave the chain uncorrupted: dir_corrupt_bucket() chooses l from [0, bucket_length), and when l == 0 it selects the bucket's row 0. dir_to_offset() for that head is the null sentinel 0, so setting dir_next() to it does not create a loop. With a five-entry chain this happens about 20% of the time, making the new CHECK(!stripe->directory.check()) fail nondeterministically. Make the corruption helper choose a non-head entry (or build the self-loop deterministically).

      dir_corrupt_bucket(dir_bucket(b1, stripe->directory.get_segment(s1)), s1, stripe);

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

[bneradt]

Follow-up review of 8c29ff0: the earlier persistence and used-entry gauge findings are addressed. Runtime segment resets count physical occupied rows, debit both gauges, and mark the directory dirty; the tests now check these effects and exercise dir_check(). All 14 reported CI checks pass. One remaining test reliability issue is noted inline. The automated concern about a null head offset applies specifically to bucket 0, not every bucket; abandoning an already-corrupt acyclic chain in cleanup is not an additional blocker for this bounded-walk change. Source review only; no local ATS build or test run was performed.

[bneradt]

[P2] Make the corruption helper reliably create a cycle

When the randomly selected b1 is 0 and dir_corrupt_bucket() chooses l == 0, e is the segment base. dir_to_offset(e, seg) is then 0, the null-link sentinel, so dir_set_next(e, 0) truncates the chain instead of creating a cycle. The new CHECK(!directory.check()), bucket_length == -1, and repair/dirty assertions can therefore fail intermittently. Other bucket heads have nonzero offsets, so this is specifically the bucket-0 case (not a 20% failure rate across all buckets). Please select a non-head entry for the self-loop, or deterministically loop the second entry; the helper always inserts enough entries for that.