Conversation
ps / tasklist / taskkill and the git shard-split invocation ran without a timeout, so a wedged subprocess blocked the calling thread indefinitely. Broaden the handlers from CalledProcessError to SubprocessError so the newly reachable TimeoutExpired is caught rather than escaping.
Destroying an unhealthy session frees a tracked slot, so _can_grow() flipped straight back to True and checkout() spawned again. A factory that always produced unhealthy instances (crash-on-launch browser, dead health check) looped forever, churning real browser processes and ignoring the timeout. Cap the replacement attempts at size * 3 so the failure is deterministic rather than a spin that only stops at the deadline.
quit() aborted its loop on the first driver that raised, leaving the remaining browsers running as orphans and the tracking list uncleared. Attempt every driver and report the failures afterwards. The close paths left current_webdriver pointing at a closed session, and neither path cleared the shared webdriver_wrapper singleton, so the next caller inherited a dead driver and a stale ActionChains. Close before dropping the reference too, so a failed close stays tracked for quit().
The handler did a single recv(8192), so any request larger than one segment was silently truncated, and a client that connected without sending pinned its thread forever (one thread per connection under ThreadingMixIn). Add a receive timeout and read until the request is actually complete. New clients can opt into WRLEN <n>\n<body> framing, which is auto-detected, so existing clients keep working unchanged and replies mirror the dialect the request arrived in. Framing also fixes authentication when the token line straddles a packet boundary, which the legacy heuristic cannot resolve. Legacy requests fall back to an incremental bracket-balance scanner that distinguishes a truncated body from a malformed one, keeping the prompt error reply for bad JSON. Requests are capped at 8 MiB. Export send_command / encode_frame / read_frame so the framed format is usable from client code.
items_by_page_key only guarantees Hashable, which does not imply orderable. Mixed key types raised a bare TypeError out of assert_sorted_by instead of the module exception callers handle.
The 0.12s wall-clock budget was flaky on loaded machines, where thread start-up alone can exceed it. Use a barrier instead: three tasks can only clear it when they are genuinely in flight at the same time.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 121 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Bandit B106 flagged the intentionally wrong token used to assert that authentication rejects it, matching the suppression the surrounding fixtures already carry.
All six were fixture data or protocol constants rather than credentials: stubbed OAuth/FCM tokens, the "Bearer" token-type constant (the mock server issues its real token from secrets.token_hex), and "pass_rate", which matched only on the "pass" substring and holds a float ratio. Bandit parses space-separated test IDs after nosec, not comma-separated, and the marker has to sit on the flagged line itself. Both tripped up the existing suppression on the autofill fixture. Bandit now reports clean across the package and the test suite at every severity level, not just the -ll threshold CI had been using.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Project-wide scan for crash / runtime defects, and the fixes. Static analysis
(ruff, mypy, bandit) was already clean, so these were found by hunting hazard
patterns: unbounded external calls, loops without exit conditions, and cleanup
paths that abort partway.
Hangs
process_supervisor,diff_shard—ps/tasklist/taskkilland thegit shard-split ran with no
timeout, so a wedged subprocess blocked thecalling thread forever. A git process waiting on an index lock or a credential
prompt would stall the whole shard split. Handlers widened to
SubprocessErrorso the now-reachableTimeoutExpiredis caught.socket_server—recv()had no socket timeout.ThreadingMixInspawns athread per connection, so a few clients that connect and never send could
exhaust the server.
Infinite loop
BrowserPool.checkout()— destroying an unhealthy session frees a trackedslot, so
_can_grow()flipped back to True and the loop spawned again. Afactory that always yields unhealthy instances (crash-on-launch browser, dead
health check) looped forever, churning real browser processes and never
checking the timeout. Measured ~6400 spawn/destroy cycles in 0.2s before the
fix. Retries are now capped deterministically at
size * 3.Resource leaks
WebdriverManager.quit()— aborted the loop on the first driver thatraised, so every remaining browser leaked as an orphan process and the
tracking list was never cleared. Every driver now gets a quit attempt.
WebdriverManagerclose paths — leftcurrent_webdriverpointing at aclosed session, and never cleared the shared
webdriver_wrappersingleton, sothe next caller inherited a dead driver and a stale
ActionChains. Close nowhappens before the reference is dropped, so a failed close stays tracked for
quit()to reclaim.Socket protocol
The handler did one
recv(8192), so any request over one segment was silentlytruncated. It now reads until the request is complete.
Clients can opt into
WRLEN <n>\n<body>framing. It is auto-detected, soexisting clients are unaffected, and replies mirror the dialect the request
arrived in. Framing also fixes authentication when the token line straddles a
packet boundary — something the legacy format cannot resolve, since a
token-less first segment must be rejected promptly and is indistinguishable
from a partial one.
Legacy requests use an incremental bracket-balance scanner that tells a
truncated body from a malformed one, preserving the prompt error reply for bad
JSON. Requests are capped at 8 MiB.
send_command/encode_frame/read_frameare exported so the framed format is usable from client code.Smaller fixes
assert_sorted_byleaked a bareTypeErrorfor non-orderable keys(
Hashabledoes not imply orderable) instead of the module exception.on loaded machines (observed 0.234s). Replaced with a barrier, which is
timing-independent.
Verification
41 regression tests added covering every fix above.
🤖 Generated with Claude Code