Skip to content

Close playlist file handle and database connection after use - #9

Open
JavaGT wants to merge 1 commit into
mainfrom
audit/resource-hygiene
Open

JavaGT wants to merge 1 commit into
mainfrom
audit/resource-hygiene

Conversation

@JavaGT

@JavaGT JavaGT commented Sep 15, 2026

Copy link
Copy Markdown
Owner

60-second summary

Two leaked resources are now closed: the playlist M3U8 file handle and the download-database connection. Verify with git diff -w 478c3f2 e5c44fe (semantic change is +4/−4 lines; the raw diff is mostly re-indentation of main() moving under a new try). Risk is minimal; rollback is git revert e5c44fe.

What

_update_playlist_file reads the playlist file inside a with block, and the CLI closes the Database in a finally after all uses (success or error).

Why

The playlist read at gamdl/downloader/downloader.py:119-123 left the file handle to the garbage collector, and Database created at gamdl/cli/cli.py:130-135 was never closed — Database.close (gamdl/cli/database.py:44-45) had no callers. Evidence and discussion: #2.

The change

  • gamdl/downloader/downloader.py: with block around the playlist-file read (same data, handle now released deterministically).
  • gamdl/cli/cli.py: everything after Database creation wrapped in try/finally with if database: database.close(); the database can be None on early config errors, and the close sits after the last database.add / flat_filter use.
  • Intentional non-goal: the per-track yt-dlp process spawn (Perf: a fresh OS process is spawned per track for yt-dlp downloads (spawn + full yt-dlp import per song) #3) is untouched — that finding is "measure first" and stays issue-only.

Verification

  • python3 -m py_compile on both touched files: clean; both modules import in the project venv.
  • Synthetic probe on the real Database (temp sqlite, no network/credentials): add/get roundtrip, connection unusable after close() (sqlite3.ProgrammingError), double-close safe, flat_filter behavior, and the committed with-read shape — 5/5 PASS. Probe and output preserved at scratch-resource-hygiene/ in the workspace (the first probe run was not kept; it was re-run and saved — flagged by the test-honesty review, see below).
  • No Apple Music credentials in this environment, so the end-to-end CLI/download path was not exercised.

Reviews

  • Hostile review (DeepSeek v4.1 Flash): APPROVED — close ordering, None guard, and no double-close verified; one theoretical note (a raising close() could mask an in-flight exception) accepted as non-blocking.
  • Test-honesty review (DeepSeek v4 Flash): MINOR — original probe artifacts weren't preserved; resolved by the coordinator re-run above.
  • GPT-5.6 ("Luna") and Grok seats were unavailable this run (usage limit / HTTP 403, consistent with prior runs); DeepSeek seats covered both lenses per the route policy.

Attribution

This PR was produced with AI assistance under the direction of @JavaGT:

  • Exploration, evidence verification, implementation: GLM agents (ZCode)
  • Planning: GLM agents; plan audited by the consultants below
  • Hostile review: DeepSeek v4.1 Flash; test-honesty review: DeepSeek v4 Flash (via OpenRouter)
    Every change was cross-reviewed and the final diff verified against the described behavior. Happy to adjust or close any part of this — tell me what doesn't fit the project's direction.

Wrap the playlist file read in _update_playlist_file in a with block so
the handle is not left open, and close the Database in a finally in the
CLI so it is closed on both success and error paths.

The cli.py diff is mostly re-indentation of the code moved under the new
try block; review with git diff -w.
@JavaGT

JavaGT commented Sep 20, 2026

Copy link
Copy Markdown
Owner Author

[Account-2 upstream-readiness review — 2026-09-20 · read-only adversarial verification pass; nothing in this PR, repo, or workspace was modified]

Verdict: READY-WITH-NOTES for upstream filing (one stale number to fix first). Mechanics verified at head e5c44fe (2 files, one theme; re-indentation disclosed): with-block playlist read; None-guarded DB close() after last use (downloader.py:119–123; the close sits after the final database.add :295 and flat_filter use; finally :308–310); cli.py:130–135; close() had no other callers at base. Probe real and preserved (scratch-resource-hygiene/, 5/5, re-run disclosed in the docstring). Attribution matches the run log. Base == current upstream HEAD 478c3f2 (verified — no drift).

PRE-SHIP (body-only):

  1. Semantic count wrong (high): the body (and the staging log) say "+4/−4"; git diff -w 478c3f2..e5c44fe is +9/−5 (cli.py +4/−0, downloader.py +5/−5; net +4). State the real numbers.
  2. Rollback phrasing (low): "rollback is git revert e5c44fe" dangles after an upstream squash — phrase it as reverting the change, not the fork SHA.
  3. Wording (low): "database can be None on early config errors" — it is None when no database_path is configured; say that.

EVALUATE (high — filing mechanics): open upstream PRs glomatico#345 (hunk @ :148) and glomatico#335 (@ :285) touch cli.py inside this PR's re-indented region — either filing order needs a rebase; downloader.py has no upstream overlap; glomatico#341 sits outside the region.

Decision question for the owner: fix the count + two phrasings at pickup? (implement-it / evaluate — nothing pre-selected.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant