Code cleanup: duplication, dead code, and bad-practice backlog

[Rushaway]

Summary

Follow-up to #126. The bugs and resource leaks found in that audit are all merged; this issue tracks the remaining findings — duplication, dead code, and bad practices. None of these are correctness bugs on their own, but they make the codebase harder to maintain and, in a couple of cases, hide real inconsistencies (e.g. differing retry behavior between two code paths that look interchangeable).

Sub-issues

• AudioManager.Stop() duplicates its stop-message logic across two branches #148 — AudioManager.Stop() duplicates its stop-message logic across two branches — merged in refactor(audiomanager): factor the duplicated stop-clip logic into one helper #161
• AudioPlayerFactory.NewPlayer() is dead branching around a single-member enum #149 — AudioPlayerFactory.NewPlayer() is dead branching around a single-member enum — merged in refactor(audioplayerfactory): fail loudly on an unsupported player type #160
• FFmpegAudioPlayer has two duplicate network-fetch implementations with inconsistent retry behavior #150 — FFmpegAudioPlayer has two duplicate network-fetch implementations with inconsistent retry behavior — merged in refactor(ffmpeg): share the ffmpeg stdin write and cleanup between fetch paths #170 (partial scope; retry parity split to CF-bypass (curl_cffi) streaming has no retry, unlike the aiohttp path #169)
• TorchlightHandler awaits independent async setup calls sequentially instead of concurrently #151 — TorchlightHandler awaits independent async setup calls sequentially instead of concurrently — closed as invalid, see below
• Fetch-with-timeout boilerplate is hand-rolled 7+ times across Commands.py/URLInfo.py/FlareSolverr.py #152 — Fetch-with-timeout boilerplate is hand-rolled 7+ times across Commands.py/URLInfo.py/FlareSolverr.py — merged in refactor(commands): replace hand-rolled fetch-with-timeout blocks with helpers #166
• Config/AccessManager/Sourcemod/TriggerManager duplicate path-resolution and JSON-loading instead of composing Config #153 — Config/AccessManager/Sourcemod/TriggerManager duplicate path-resolution and JSON-loading instead of composing Config — merged in refactor(config): share config path resolution and JSON loading via ConfigFile #165 (error-handling consistency split to Config loading error handling is inconsistent: two files tolerate malformed JSON, two crash #164)
• ~25 classes duplicate self.logger = logging.getLogger(...) instead of a shared helper #154 — ~25 classes duplicate self.logger = logging.getLogger(...) instead of a shared helper — closed as invalid, see below
• AsyncClient/SourceRCONClient/SourceRCONServer each hand-roll a different reconnect/retry policy #155 — AsyncClient/SourceRCONClient/SourceRCONServer each hand-roll a different reconnect/retry policy — closed as invalid, see below
• FFmpegAudioPlayer hardcodes MyInstants-specific headers inside the generic streaming path #156 — FFmpegAudioPlayer hardcodes MyInstants-specific headers inside the generic streaming path — merged in refactor(ffmpeg): resolve per-domain request headers from a registry #168
• FFmpegAudioPlayer hardcodes the ffmpeg binary path instead of reading it from Config #157 — FFmpegAudioPlayer hardcodes the ffmpeg binary path instead of reading it from Config — merged in feat(ffmpeg): make the ffmpeg binary path configurable #162
• CommandHandler uses a string class-name comparison instead of a BaseCommand flag for menu-echo behavior #158 — CommandHandler uses a string class-name comparison instead of a BaseCommand flag for menu-echo behavior — merged in refactor(commandhandler): replace the menu-echo class-name check with a flag #163
• FlareSolverr's bypass URL is module-global mutable state rather than configured #159 — FlareSolverr's bypass URL is module-global mutable state rather than configured — merged in refactor(flaresolverr): replace module-global state with a configured client #167

All 9 real findings are merged to master. VERSION is now 1.8.22.

Closed as invalid (3 of 12)

Checked each premise against the actual code before implementing rather than shipping the change:

• TorchlightHandler awaits independent async setup calls sequentially instead of concurrently #151 — measured asyncio.gather against the real AsyncClient.Send: it holds send_lock across the whole round-trip with one shared recv_future, so concurrent sends serialize anyway. gather was 0.98x (marginally slower), not faster.
• ~25 classes duplicate self.logger = logging.getLogger(...) instead of a shared helper #154 — only 2 of ~20 classes carrying the duplicated logger line call super().__init__() at all, so a mixin would need adding cooperative super-calls project-wide — more churn than the line it saves.
• AsyncClient/SourceRCONClient/SourceRCONServer each hand-roll a different reconnect/retry policy #155 — only one of the three cited loops is actually a retry loop; the other two are an ordinary accept loop and an ordinary read loop with no retry policy to unify.

Follow-ups filed for scope split out of a merged PR

• Config loading error handling is inconsistent: two files tolerate malformed JSON, two crash #164 — config loading error handling is inconsistent (2 files tolerate malformed JSON, 2 raise) — split from Config/AccessManager/Sourcemod/TriggerManager duplicate path-resolution and JSON-loading instead of composing Config #153, needs an explicit fail-fast-vs-tolerate decision since it changes operator-visible startup behavior
• CF-bypass (curl_cffi) streaming has no retry, unlike the aiohttp path #169 — the Cloudflare-bypass (curl_cffi) fetch path has no retry unlike the aiohttp path — split from FFmpegAudioPlayer has two duplicate network-fetch implementations with inconsistent retry behavior #150, needs Range-resume support and a live voice-server/Cloudflare-source test setup to verify safely

Process

Lower risk than #126's items, but larger/more opinionated refactors — no fixed merge order needed since these are mostly independent of each other. Pick them up individually as time allows, each as its own PR.

🤖 Generated with Claude Code

[Rushaway]

Status: 9 PRs open, 3 issues closed as invalid

Merge order (each PR bumps VERSION sequentially from the current 1.8.13)

#	PR	Issue	VERSION	Scope
1	#160	#149	1.8.14	AudioPlayerFactory.py
2	#161	#148	1.8.15	AudioManager.py
3	#162	#157	1.8.16	FFmpegAudioPlayer.py, config.json
4	#163	#158	1.8.17	Commands.py, CommandHandler.py
5	#165	#153	1.8.18	Config.py, AccessManager.py, Sourcemod.py, TriggerManager.py
6	#166	#152	1.8.19	Commands.py, URLInfo.py, Utils.py
7	#167	#159	1.8.20	FlareSolverr.py, Torchlight.py, TorchlightHandler.py, FFmpegAudioPlayer.py
8	#168	#156	1.8.21	FFmpegAudioPlayer.py
9	#170	#150	1.8.22	FFmpegAudioPlayer.py

Expected conflicts

Unlike the #126 batch, these overlap heavily. Merging in the order above minimizes it, but expect rebases regardless:

• FFmpegAudioPlayer.py is touched by four PRs (feat(ffmpeg): make the ffmpeg binary path configurable #162, refactor(flaresolverr): replace module-global state with a configured client #167, refactor(ffmpeg): resolve per-domain request headers from a registry #168, refactor(ffmpeg): share the ffmpeg stdin write and cleanup between fetch paths #170). Ordering them 3 → 7 → 8 → 9 keeps each one's changes in a different region, but each after the first will still need VERSION resolved, and refactor(ffmpeg): share the ffmpeg stdin write and cleanup between fetch paths #170 touches the same block refactor(ffmpeg): resolve per-domain request headers from a registry #168 does.
• Commands.py is touched by two (refactor(commandhandler): replace the menu-echo class-name check with a flag #163, refactor(commands): replace hand-rolled fetch-with-timeout blocks with helpers #166) in different functions.
• Every PR bumps VERSION, so out-of-order merges conflict there each time (trivial — keep the higher number).

Closed as invalid

Three issues from this backlog didn't survive verification. I checked each premise before implementing rather than shipping the change:

• TorchlightHandler awaits independent async setup calls sequentially instead of concurrently #151 (asyncio.gather for setup calls) — AsyncClient.Send holds send_lock across the whole round-trip with a single shared recv_future, so concurrent sends serialize. Measured: 0.98x — gather is marginally slower, not 2x faster.
• AsyncClient/SourceRCONClient/SourceRCONServer each hand-roll a different reconnect/retry policy #155 (three hand-rolled retry loops) — only one is a retry loop. The other two are an ordinary accept loop and an ordinary recv loop; the finding pattern-matched on while True:.
• ~25 classes duplicate self.logger = logging.getLogger(...) instead of a shared helper #154 (LoggerMixin) — only 2 of ~20 classes call super().__init__(), so a mixin would mean adding cooperative super-calls everywhere: more churn than the line it removes. Noted the genuinely useful alternative (hierarchical torchlight.* logger names) in the issue.

Follow-ups filed

• Config loading error handling is inconsistent: two files tolerate malformed JSON, two crash #164 — config loading error handling is inconsistent (split from Config/AccessManager/Sourcemod/TriggerManager duplicate path-resolution and JSON-loading instead of composing Config #153, needs a fail-fast vs tolerate decision)
• CF-bypass (curl_cffi) streaming has no retry, unlike the aiohttp path #169 — CF-bypass path has no retry unlike the aiohttp path (split from FFmpegAudioPlayer has two duplicate network-fetch implementations with inconsistent retry behavior #150, needs Range resume + a live test setup)

🤖 Generated with Claude Code

[Rushaway]

All 9 real findings merged to `master` (PRs #160–#168, #170), in the recommended order, VERSION now `1.8.22`. 3 issues (#151, #154, #155) closed as invalid after checking their premise against the actual code — details above.

One of the merged PRs (#166) briefly broke CI lint (an undefined-name Utils reference plus a formatting diff) — fixed same-day with a follow-up commit, verified with the exact ruff check/ruff format --diff commands CI runs, and confirmed clean across the other 8 branches too.

Two scoped-down PRs left explicit follow-ups: #164 (config error-handling consistency) and #169 (CF-bypass retry parity, needs a live test setup).

Closing this tracking issue as done.

🤖 Generated with Claude Code