Skip to content

gh-135386: Fix "unable to open database file" errors on readonly DB #135566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Aug 22, 2025

Conversation

GeneralK1ng
Copy link
Contributor

@GeneralK1ng GeneralK1ng commented Jun 16, 2025

What

This PR adds the SQLite URI parameter immutable=1 when opening a database in read-only mode in Lib/dbm/sqlite3.py. This explicitly informs SQLite that the database is read-only and avoids attempts to create or write to auxiliary WAL/SHM files.

Why

Without this flag, opening a read-only SQLite database may fail with errors such as "unable to open database file" when the WAL or SHM files cannot be created due to filesystem permissions or other restrictions. This is especially common when the database file is read-only and the environment prevents creation of additional files.

Adding immutable=1 allows SQLite to operate correctly without needing to create WAL/SHM files, thus preventing these errors.

Where

The change is made in the _Database.__init__ constructor in Lib/dbm/sqlite3.py. The URI used to open the database connection is appended with &immutable=1 only if the flag is "ro" (read-only).

Related issue

#135386


This is my first contribution to CPython. I appreciate the opportunity and look forward to feedback from the community. Thank you!

@python-cla-bot
Copy link

python-cla-bot bot commented Jun 16, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Jun 16, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test case. You can refer to the devguide for how to do that.

@GeneralK1ng GeneralK1ng force-pushed the fix-issue-135386 branch 2 times, most recently from 707f4a3 to 779faf2 Compare June 17, 2025 04:53
@ZeroIntensity
Copy link
Member

No need to force-push, we squash at the end.

@GeneralK1ng
Copy link
Contributor Author

No need to force-push, we squash at the end.

Got it, thanks for the reminder!

@ZeroIntensity
Copy link
Member

FYI, tests are failing on Windows.

@GeneralK1ng
Copy link
Contributor Author

FYI, tests are failing on Windows.

Thanks for the suggestion. I've skipped the test on Windows using @unittest.skipIf(sys.platform.startswith("win"), ...) because of platform differences in file permissions and locking behavior that caused the test to fail in CI. Should be good now!

Copy link
Member

@tomasr8 tomasr8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

I've skipped the test on Windows

I think we should be testing this on all platforms if possible. Maybe we can figure out why the test is failing on Windows and adapt it?

GeneralK1ng and others added 2 commits June 23, 2025 21:38
Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
@GeneralK1ng
Copy link
Contributor Author

I think we should be testing this on all platforms if possible. Maybe we can figure out why the test is failing on Windows and adapt it?

Sure, that makes sense — I'd be happy to look into making the test work on Windows as well.
For now, I'm developing on macOS and don't have a Windows environment set up yet.
Once I'm back home and can test on my Windows machine, I'd be glad to revisit this.

Also, thanks for the suggestions — I've adopted the URI change and simplified the flag check as suggested. : )

@GeneralK1ng
Copy link
Contributor Author

I think we should be testing this on all platforms if possible. Maybe we can figure out why the test is failing on Windows and adapt it?

I've updated the test so it no longer skips on Windows — instead, it now explicitly closes the database before setting read-only permissions, and restores the original permissions after the test.
This should work more reliably across platforms, including Windows.
Let me know if you'd suggest any further adjustments!

@tomasr8 tomasr8 self-requested a review June 23, 2025 17:45
Comment on lines 94 to 115
def test_readonly_open_without_wal_shm(self):
wal_path = self.filename + "-wal"
shm_path = self.filename + "-shm"

for suffix in wal_path, shm_path:
os_helper.unlink(suffix)

try:
self.db.close()
except Exception:
pass

os.chmod(self.filename, stat.S_IREAD)

db = dbm_sqlite3.open(self.filename, "r")
try:
self.assertEqual(db[b"key1"], b"value1")
self.assertEqual(db[b"key2"], b"value2")
finally:
db.close()

os.chmod(self.filename, stat.S_IWRITE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC correctly, this change makes it so that the WAL/SHM files are no longer created in read-only mode? In that case, we could simplify the test to simply open the database in read-only mode and check that the auxiliary files are not written. I'm thinking something like this:

class Immutable(unittest.TestCase):
    def setUp(self):
        self.filename = os_helper.TESTFN
        self.db = dbm_sqlite3.open(self.filename, "r")

    def tearDown(self):
        self.db.close()

    def test_readonly_open_without_wal_shm(self):
        wal_path = self.filename + "-wal"
        shm_path = self.filename + "-shm"

        self.assertFalse(os.path.exists(wal_path))
        self.assertFalse(os.path.exists(shm_path))

Or does this also affect the database file itself? i.e. does passing immutable=1 allow opening read-only files whereas otherwise it would be impossible?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I’ve replaced the original test with your suggested version in a new Immutable test class. It’s much cleaner and focused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or does this also affect the database file itself? i.e. does passing immutable=1 allow opening read-only files whereas otherwise it would be impossible?

Yes, exactly — without immutable=1, even in mode=ro, SQLite may still attempt operations that require write access (like writing lock files or checking for journal/WAL state), and will fail if the database file is read-only or in a read-only directory.

Passing immutable=1 tells SQLite to treat the database as entirely static and read-only, which avoids those operations and allows the file to be opened successfully even with 0444 permissions or from a read-only mount.

@toslunar
Copy link

In order to use immutable=1, we must assert the DB will not be overwritten (by other jobs). Maybe dbm.open needs a new flag beyond 'r' to indicate the immutability.

https://www.sqlite.org/uri.html#recognized_query_parameters says

immutable=1
The immutable query parameter is a boolean that signals to SQLite that the underlying database file is held on read-only media and cannot be modified, even by another process with elevated privileges. SQLite always opens immutable database files read-only and it skips all file locking and change detection on immutable database files. If this query parameter (or the SQLITE_IOCAP_IMMUTABLE bit in xDeviceCharacteristics) asserts that a database file is immutable and that file changes anyhow, then SQLite might return incorrect query results and/or SQLITE_CORRUPT errors.

@serhiy-storchaka
Copy link
Member

I solved the puzzle. The PR changed journaling from "wal" to "OFF". It was difficult to notice because the code was indented. This is not desirable change.

@GeneralK1ng
Copy link
Contributor Author

I solved the puzzle. The PR changed journaling from "wal" to "OFF". It was difficult to notice because the code was indented. This is not desirable change.

Thanks for digging into this and clarifying the journaling issue. I appreciate the fixes and polishing! Please let me know if there’s anything I can help adjust or test further.

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Serhiy's amendments, this is good to go. Thank you so much for fixing this, @GeneralK1ng!

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@GeneralK1ng
Copy link
Contributor Author

Thanks everyone for the reviews and help polishing this! Really appreciate the guidance.

@serhiy-storchaka serhiy-storchaka merged commit c0ae92b into python:main Aug 22, 2025
44 checks passed
@serhiy-storchaka serhiy-storchaka added the needs backport to 3.14 bugs and security fixes label Aug 22, 2025
@miss-islington-app
Copy link

Thanks @GeneralK1ng for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 22, 2025
…y DB (pythonGH-135566)

Add immutable=1 flag for read-only SQLite access to avoid WAL/SHM errors on readonly DB.
(cherry picked from commit c0ae92b)

Co-authored-by: General_K1ng <generak1ng0@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Aug 22, 2025

GH-138056 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Aug 22, 2025
@serhiy-storchaka serhiy-storchaka added the needs backport to 3.13 bugs and security fixes label Aug 22, 2025
@miss-islington-app
Copy link

Thanks @GeneralK1ng for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 22, 2025
…y DB (pythonGH-135566)

Add immutable=1 flag for read-only SQLite access to avoid WAL/SHM errors on readonly DB.
(cherry picked from commit c0ae92b)

Co-authored-by: General_K1ng <generak1ng0@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Aug 22, 2025

GH-138057 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Aug 22, 2025
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Debian root 3.x (tier-1) has failed when building commit c0ae92b.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/345/builds/12027) and take a look at the build logs.
  4. Check if the failure is related to this commit (c0ae92b) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/345/builds/12027

Failed tests:

  • test_dbm_sqlite3

Failed subtests:

  • test_readonly_file_write - test.test_dbm_sqlite3.ReadOnlyFilesystem.test_readonly_file_write

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/test_dbm_sqlite3.py", line 115, in test_readonly_file_write
    with self.assertRaises(dbm_sqlite3.error):
         ~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^
AssertionError: error not raised

serhiy-storchaka added a commit that referenced this pull request Aug 22, 2025
…ly DB (GH-135566) (GH-138057)

Add immutable=1 flag for read-only SQLite access to avoid WAL/SHM errors on readonly DB.
(cherry picked from commit c0ae92b)

Co-authored-by: General_K1ng <generak1ng0@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit to miss-islington/cpython that referenced this pull request Aug 22, 2025
…y DB (pythonGH-135566)

Add immutable=1 flag for read-only SQLite access to avoid WAL/SHM errors on readonly DB.
(cherry picked from commit c0ae92b)

Co-authored-by: General_K1ng <generak1ng0@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@serhiy-storchaka
Copy link
Member

I forgot to test this under the root user. #138058 skips tests for that case.

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Debian root 3.13 (tier-1) has failed when building commit 8f22802.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1441/builds/1394) and take a look at the build logs.
  4. Check if the failure is related to this commit (8f22802) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1441/builds/1394

Failed tests:

  • test_dbm_sqlite3

Failed subtests:

  • test_readonly_file_write - test.test_dbm_sqlite3.ReadOnlyFilesystem.test_readonly_file_write

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/root/buildarea/3.13.angelico-debian-amd64/build/Lib/test/test_dbm_sqlite3.py", line 116, in test_readonly_file_write
    with self.assertRaises(dbm_sqlite3.error):
         ~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^
AssertionError: error not raised

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

Successfully merging this pull request may close these issues.

8 participants