[rockhunters08]

Why

Follow-up to #18. That PR replaced the recursive retry with a bounded loop and fixed the extractor OOMs, but I set MAX_REQUEST_ATTEMPTS = 10 arbitrarily, and ten is too low. It is causing recoverable broken pipes to become failed writes in production.

The bound was set for the wrong reason

I chose ten while thinking about memory. That was the wrong axis. The memory problem was frames, not attempts: recursion kept every attempt's row values and built request alive simultaneously, which is what reached ~3.1 GiB. A loop rebinds each attempt and frees the previous one, so retrying costs nothing.

Measured, 500 attempts at 1MB each:

attempts made:          500
request objects alive:  0
implied peak retained:  ~0 MB   (vs 1500 MB if they accumulated)

So the bound exists only to stop a permanently failing row spinning forever. It should be set for time, and ten left no room for recovery.

What ten actually costs

After #18 rolled to the flavour-model fleet, extractors stopped OOMing (0 restarts across 16 pods) but began failing HBase writes at several hundred per minute, rotating between pods, with the DLQ growing from a steady 15 to 27.

The failures are BrokenPipeError on the region socket, converted to RegionServerException at region/client.py:152, which _handle_exception recovers by reestablishing the client. That recovery works by retrying, and critically it does not sleep on this path: _dynamic_sleep was invoked 0 times in 10 minutes of live logs, against 156 RegionServerException and 78 BrokenPipeError. So attempts are cheap and fast, and the old code effectively got up to ~1000 of them before the recursion limit. Ten is not enough to ride out a reconnect, so a connection broken mid-write now fails the write instead of recovering.

100 keeps a real bound while restoring the headroom recovery needs.

Tests

Two added to tests/test_retry_bounds.py, both offline:

• test_retrying_does_not_accumulate_memory asserts that after exhausting the bound, with the cyclic collector disabled, zero per-attempt requests remain reachable. This is the property that makes the bound free to raise, so it should not be able to regress silently.
• test_bound_leaves_room_for_a_region_reconnect pins the bound at 50 or more, so nobody lowers it back into the failure range without deliberately editing the assertion.

All 6 pass. flake8 unchanged on client.py (2 E128, 1 F821, 2 W293, all pre-existing); test file clean.

Note

I cannot fully separate my bound's contribution from a genuine HBase broken-pipe event that began at 17:58 the same day. Both are plausibly involved in the observed failures. The bound is wrong on its own merits regardless, since it trades away recovery for a memory saving that does not exist.

[Copilot]

🟡 Changes recommended

Future-backed request retention undermines the memory-safety claim, and the new numeric literal breaks supported Python versions.

3 open findings

• Failed Futures retain request cycles on the real RPC path · New
• Numeric separator breaks advertised Python 2.7–3.5 compatibility · New
• Check live request retention during retries, not only after completion · New

What changed in this PR

Raises the bounded retry budget introduced in #18 to give transient HBase connection failures more recovery attempts.

Changes:

• Increases request attempts from 10 to 100.
• Adds memory-retention and minimum-bound tests.
• Updates comments explaining the retry budget.

File	Description
tests/​test_retry_bounds.py	Adds memory and retry-budget regression checks.
pybase/​client.py	Raises the attempt limit and documents its rationale.

🧠 Review effort: Balanced

---

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

[rockhunters08]

Confirmed and fixed at the source in 5a2a3cf, rather than by lowering the bound.

Reproduced it first with a real ThreadPoolExecutor: 20 of 20 requests stayed reachable with gc disabled, freed only by an explicit gc.collect(). The cycle is Future -> _exception -> traceback -> _send_request frame -> Future, which the except block's implicit del e cannot break because it is self-contained.

Fix is in _send_request: drop the local in a finally so the frame stops referencing the Future. Same measurement then gives 0 of 20. The _async branch still returns the Future untouched.

You were right that this undermined the memory-neutral claim; with the cycle broken it holds on the real path.

[Copilot]

🔵 Needs a closer look

The memory test bypasses production Future-backed request retention and does not establish the claimed memory safety.

3 open findings

• Numeric separator breaks advertised Python 2.7–3.5 compatibility
• Failed Futures retain request cycles on the real RPC path
• Check live request retention during retries, not only after completion

Previously missed (1)

In code that hasn't changed since last review

Test Future-backed failures to detect retained requests

tests/​test_retry_bounds.py:109

This direct raise bypasses the production sender's Future.result() path (pybase/region/client.py:131–134). A failed Future holds its exception, whose traceback references the sender frame containing both future and rq. That cycle can retain requests until cyclic garbage collection runs, even though the retry loop rebinds rq. With collection disabled, this fake therefore misses the retention mechanism relevant to the larger bound. Exercise the Future-backed failure path and address any retained requests before using this test as evidence that retries release each attempt's memory.

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

The new memory-release assertion races with worker cleanup and can fail nondeterministically.

2 open findings

• Failed Futures retain request cycles on the real RPC path
• Join executor before counting weak references · New

2 resolved since last review

• Numeric separator breaks advertised Python 2.7–3.5 compatibility
• Check live request retention during retries, not only after completion

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

A verified cache-key mismatch prevents the broken-socket reconnection that motivates the increased retry bound.

2 open findings

• Broken-socket recovery skips purge due to cache key type mismatch · New
• Failed Futures retain request cycles on the real RPC path

1 resolved since last review

• Join executor before counting weak references

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Delayed failures can evict healthy replacements, and reconnections retain retired clients and executor threads.

3 open findings

• Stale client failure can purge replacement cache entries · New
• Failed Futures retain request cycles on the real RPC path
• Retired clients leak executor workers through bucket references · New

1 resolved since last review

• Broken-socket recovery skips purge due to cache key type mismatch

🧠 Review effort: Balanced

[rockhunters08]

Both confirmed and fixed in accc5ca.

Client.close() only closed sockets; self.thread_pool was never shut down, so every retired client stranded its workers. It now shuts the pool down with wait=False so the handler is not blocked.

_let_all_through now pops its bucket entry. The key is (exception class name, region_client), so leaving it kept every retired client alive forever. Popping under _buckets_lock is safe for waiters: a thread blocked in _let_one_through already holds its own reference to the semaphore, and a later arrival simply gets a fresh one.

Tests are test_close_retires_the_executor and test_repeated_retirement_does_not_grow_the_bucket_map, both verified to fail against the previous commit.

[Copilot]

🟡 Changes recommended

Concurrent retirement can strand recovery handlers and cause submission failures to bypass retries.

4 open findings

• Prevent semaphore race when removing recovery buckets · New
• Failed Futures retain request cycles on the real RPC path
• Translate shutdown submission errors into retryable region failures · New
• Retired clients leak executor workers through bucket references

1 resolved since last review

• Stale client failure can purge replacement cache entries

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

Unresolved discovery and retirement races require coordinated fixes and human review of concurrent recovery behavior.

3 open findings

• Client retirement race causes KeyError or stale region cache · New
• Failed Futures retain request cycles on the real RPC path
• Retired clients leak executor workers through bucket references

2 resolved since last review

• Prevent semaphore race when removing recovery buckets
• Translate shutdown submission errors into retryable region failures

🧠 Review effort: Balanced

[rockhunters08]

Verified, and I am going to leave this one open rather than attempt it.

Confirmed all three parts: the check-then-get at lines 465 and 467 is unlocked so eviction between them raises KeyError; eviction after selection caches a region backed by a retired client; and _cache_lock is a plain non-reentrant Lock that _add_to_region_cache already takes, so wrapping discovery in it would deadlock exactly as you warn.

Stopping because the fix needs restructuring of lock usage across discovery and retirement, and I agree with your verdict that this wants human review of concurrent recovery behaviour. This is also the third consecutive round where a fix of mine exposed the next layer: working eviction surfaced the executor and bucket leaks, those surfaced the bucket claim race, and that surfaces this. The individual fixes are sound and tested, but the cascade is the signal.

Leaving the PR open and flagged for a human reviewer who knows this fork, with the race documented in the description. The production incident that started this is already resolved and stable on the merged work, so nothing is waiting on it.

[rockhunters08]

Needs a human reviewer before merge

Stopping the automated review loop here. Seven rounds; summary of where it landed.

Fixed and tested in this PR

Pre-existing bugs, all reproduced before fixing:

1. Region clients were never purged. Cached under the protobuf bytes spelling while Client decodes host and port, so the handler's lookup always missed. Production evidence: 496 BrokenPipeError in one hour and zero Purging cache log lines. _purge_client was worse than a miss, building str + bytes which is a TypeError on py3. This is the root cause of the sustained HBase write failures.
2. Failed Futures retained their request. Future -> _exception -> __traceback__ -> frame -> Future is a cycle the interpreter cannot refcount away. Measured 20 of 20 requests retained with gc off; 0 after releasing the local.
3. Retiring a client leaked threads and references. close() never shut down its executor, and the _buckets key holds the client itself.
4. Bucket claim was not atomic, so a caller could own a retired semaphore and release a different one.
5. submit() on a retired pool raised RuntimeError, which the retry handlers do not catch.

16 offline tests. Each was checked to fail against the code it covers; one that did not was relabelled rather than left looking like coverage it did not provide.

Open, and why I stopped

Discovery and retirement do not share a lock (see the thread on _create_new_region). Confirmed: the check-then-get at lines 465/467 can raise KeyError, and a region can be cached against a retired client. The naive fix deadlocks, because _cache_lock is a non-reentrant Lock that _add_to_region_cache already takes.

Items 3, 4 and 5 were each exposed by the fix before them: working eviction surfaced the leaks, the leak fix surfaced the claim race, and that surfaces this. The cascade is the reason for a human pass, not the individual findings.

Note on splitting

I had suggested splitting out the cache-key fix to ship sooner. That was wrong: normalising the keys is precisely what makes eviction start happening, so it cannot ship without the retirement work. The change is coupled and should land or wait as one piece.

Not urgent

The extractor OOM incident this came from is resolved and stable on already-merged work: 16 pods, 0 restarts, 0 OOMKills, visible queue 0. The remaining HBase reconnect failures are degraded-but-absorbed, so this can take the time it needs.

[rockhunters08]

Closing this unmerged. The work is written and tested, but the open discovery/retirement race needs someone who knows this fork, and I would rather not leave a half-reviewed concurrency change sitting open as if it were ready.

Tracked in Flipboard/ops#2542, assigned to @zacharyp, with the full context and the warning not to split the change. Issues are disabled on this repo, hence the other home.

The branch greg/bound-hbase-request-retries stays pushed, so reopening this PR is enough to pick it all back up. The review history here is the real artifact: seven rounds, six defects, each one reproduced before being fixed.

The production incident that started this is resolved and stable on the already merged PyBase#18: 16 extractors, 0 restarts, 0 OOMKills.

[rockhunters08]

Correction to my comment above: the branch is greg/raise-request-attempt-bound at d5f9ba3, which is this PR's head.

I wrote greg/bound-hbase-request-retries, which is PR#18's branch (already merged, sitting at 4d09036). Checking that one out would get the shipped fix and none of the five in this PR. Both branches still exist on origin.