Conversation
scripts/check-windows-footguns.py --all runs in CI lint and flags the bare calls; no behaviour change.
teknium1
left a comment
There was a problem hiding this comment.
Verdict: premise confirmed, fix is correct on Linux, one review-fix pushed. Recommend merge after Teknium's call; macOS half of the class still open (non-blocking).
Context. _secure_state_db_files (from #109509's umask hardening) runs os.open/fchmod/close on the live state.db, -wal and -shm every time a writer connection opens, including _open_writer_conn after sqlite3.connect. Per SQLite's howtocorrupt §2.2 the close() cancels every POSIX record lock this process holds on those inodes, so the next short-lived opener/closer in another process thinks it is the last holder and unlinks the WAL generation. This is the producer behind #109687 (and the "5 live SessionDB handles" precursor in #100896 makes sense now: each extra in-process handle re-runs the helper on a live generation).
Live repro (Linux 7.0, Python 3.11, SQLite from the repo venv), tmp HERMES_HOME, real SessionDB:
| step | origin/main @ b6b53c6 |
this PR @ 64ade66 |
|---|---|---|
one SessionDB + external sqlite3.connect/close |
sidecars survive | sidecars survive |
second in-process SessionDB, then external reader close |
-wal/-shm gone; process holds 3 (deleted) fds; next write → DeletedWalGenerationError |
sidecars survive, 0 deleted fds, write OK, all three files 0600 |
So the bug fires deterministically on main with two in-process handles (the gateway's normal state), and this branch closes it. tests/hermes_state (342) + the new file pass; ruff clean.
Code notes
O_PATH+os.chmod("/proc/self/fd/N")is the right shape on Linux: nolchmod,fchmodon anO_PATHfd isEBADF, andos.chmod(path, follow_symlinks=False)is unsupported there (os.chmod in os.supports_follow_symlinksis False). Verified.O_EXCLcreate-then-O_PATHfor the missing main file keeps the "private from the first byte" property; the created fd is closed before SQLite has any lock, so that close is safe.- Symlink handling:
O_PATH|O_NOFOLLOWon a symlink yields an fd to the link itself, hence theS_ISLNK→ELOOPbranch. Good; the test proves the target's mode/content are untouched. - Pushed one commit to your branch (
bdcec6b6):encoding="utf-8"on the tworead_text/write_textcalls in the new test. CI's lint job runsscripts/check-windows-footguns.py --alland would have flagged them; no behaviour change.
Remaining gap, not blocking this PR: the non-Linux branch still does the raw O_RDONLY open/fchmod/close on live files, and POSIX lock cancellation applies on macOS too (#104451 is a macOS report with this signature; #109641 says the holder scan is Linux-only, so macOS has neither the guard nor the fix). macOS has lchmod, so os.lstat + S_ISREG check + os.chmod(path, 0o600, follow_symlinks=False) would close the class there without any descriptor. Happy to take that as a follow-up if you'd rather keep this one Linux-scoped.
Siblings seen in the sweep: #109725 (read-only sessions stats) and #109737 (NO_CKPT_ON_CLOSE / pin writer on 3.11) attack the closer side; a bare sqlite3.connect().close() reproduces the orphan on main, so neither replaces this fix. Not merging; that decision is Teknium's.
teknium1
left a comment
There was a problem hiding this comment.
Maintainer bot review, live-verified on Linux (Py 3.11.15): #109728 repro after: 3 → 0; stricter probe (parent holds BEGIN IMMEDIATE, helper runs, child tries to write) → child gets database is locked in both WAL and journal_mode=delete. On main both fail. Tests pass locally.
Two notes, neither blocking the Linux fix:
- Non-Linux still drops locks.
path_onlyis Linux-only; the fallback branch keeps theO_RDONLYopen/fchmod/close, so macOS (see #109752/#109759 reports) has the same bug after this lands. A plainlstat+ symlink refusal +os.chmod(path, 0o600)for existing files works on every POSIX platform without O_PATH/procfs, andO_CREAT|O_EXCLfor first creation keeps the private-from-first-byte property. That would make one platform-neutral helper instead of two code paths. - Trim to the two invariant tests you have (lock retention + symlink refusal) — good as-is.
Merge decision is Teknium's; not approving/merging from here.
Summary
Fixes #109728. Regression follow-up to the permission hardening merged in #109509.
On Linux,
_secure_state_db_filesopens and closes ordinary descriptors for the database and its WAL/SHM files. Closing such a descriptor releases the process's POSIX locks on that inode, including locks held by SQLite through other descriptors. Opening a secondSessionDBcan therefore silently remove the first connection's locks. Another SQLite process may then unlink its sidecars, leaving live connections on deleted generations. The deleted-generation guard correctly refuses further access, but the result is a session outage.This behavior is documented in SQLite's corruption guidance. The regression reproduces with a throwaway database; no manual WAL deletion, update script, or session cleanup is required.
Changes
O_PATH | O_NOFOLLOW | O_CLOEXECand apply owner-only permissions through/proc/self/fd/<fd>. Closing anO_PATHdescriptor does not cancel SQLite's POSIX locks.0600before SQLite opens it. If another opener creates it first, use the existing-file path.The shared helper also covers callers in async delegation. No WAL guard is disabled, no journal-mode default changes, no sidecars are manually removed, and no permanent descriptor cache is introduced.
Validation
Tested on Linux with Python 3.11, against main
b6b53c69a6ed49cb099cf1bfe76b5e6edd718e5a, usingscripts/run_tests.shin an isolated test environment.Red on unmodified main:
Result: 1 failed, 3 passed. The live-generation test finds three deleted sidecar holders where it expects none.
With this fix:
Result: 328 passed, 1 failed, 5 skipped. All four new regression cases pass.
The one failure is
TestFTS5Search::test_search_projection_skips_context_enrichment_queries(context_query_count()is 0 rather than 1). I independently reran that test on the unmodified base and obtained the same failure. It was also noted in #109509; this PR does not change the unrelated FTS behavior.ruff check hermes_state.py tests/test_state_permission_locks.pyandgit diff --checkpass.The same fix was applied temporarily to the affected Linux installation. Gateway/dashboard operation resumed without a database restore or journal-mode change, and the user confirmed that existing sessions open and a new prompt receives a response. This contribution does not change that installation's official upstream remote or update workflow.