fix(doctor): stop creating files and database during diagnostics - #3646
Conversation
github-actions
Bot
added
area/config
Preview DeploymentPreview deployments for this PR are available at:
The preview environment will start automatically on first visit. Subsequent pushes to this PR will update the Run locally with Dockerdocker pull ghcr.io/go-vikunja/vikunja:pr-3646
docker run -p 3456:3456 ghcr.io/go-vikunja/vikunja:pr-3646Last updated for commit ff88e09 |
tink-bot
left a comment
tink-bot
left a comment
There was a problem hiding this comment.
🤖 PR Swarm — automated review, round 1. Panel: bughunter · security · conventions · design · verifier. See inline comments.
|
🤖 PR Swarm — automated review. Panel: bughunter · security · conventions · design · verifier — 3 rounds, final @ Important This summary was rewritten after the PR was restructured. The Verdict: ✅ APPROVE — 17 fixed across 3 rounds, 5 deferred (all now have follow-ups)Scope of this PR after the splitDoctor no longer creates the storage it reports on. ✅ Fixed here (11)
🔴 CI failure introduced and fixed after the reviewThe review rounds missed this; CI caught it. Capturing a config value before init returns
❓ Deferred — all now have follow-ups
🔎 Runtime verificationEvery round compared against a binary built from the merge base, so pre-existing behavior was never misreported as a regression.
Round historyround 1 — 8 fixed, 2 nit-resolved, 5 deferred. Included two verified regressions in the round 2 — all 8 round-1 fixes held under re-verification; 6 more fixed, including a regression round 1 itself introduced. round 3 — audited round 2's fixes; found two regressions it introduced plus one gap, all fixed. |
tink-bot
left a comment
tink-bot
left a comment
There was a problem hiding this comment.
🤖 PR Swarm — automated review, round 2. Re-reviewed round 1's own fixes. All 8 held under re-verification; 6 new items found and fixed, including one regression round 1 introduced. See inline comments.
33a8939 to
887f9b1
Compare
tink-bot
left a comment
tink-bot
left a comment
There was a problem hiding this comment.
🤖 PR Swarm — automated review, round 3 (final). Audited round 2's fixes and found two regressions it introduced, plus one gap in the new flag. All fixed. Round cap reached.
tink-bot
added
pr-swarm/needs-input
887f9b1 to
0064730
Compare
|
🤖 PR Swarm — follow-up: the |
|
🤖 PR Swarm — all five deferred items now have follow-ups:
#3656 also closes a hazard found while building it: |
cff515a to
23954c7
Compare
ValidateFileStorage created the local base directory as a side effect of checking it. Move that into ensureLocalBasePath so callers that only want to inspect storage can do so without creating it. InitFileHandler now calls both, so server and restore behavior is unchanged. InitFileHandlerReadOnly sets up the storage backend and nothing else.
Running `vikunja doctor` outside the install directory created a `files` directory in the current working directory and then reported it as existing and writable, which reads as a passing check against storage doctor made itself. Use the read-only file handler init. A missing base directory is now reported as a failed check instead of being silently created.
ResolvedDatabasePath exists so callers can report the path a diagnostic would use. Resolving it must therefore create nothing, which took three things: getUserDataDir ended in os.MkdirAll, so merely reporting the path created the user data directory. resolveUserDataDir computes it; getUserDataDir creates it. resolveDatabasePath then chose its rootpath fallback from whether the injected getter errored, which only the creating variant could do — so the reporting and engine paths disagreed, and doctor failed a working install by naming a path the server never opened. Creation now lives inside the injected getter: resolution passes existingUserDataDir, the engine passes createdUserDataDir, and both fall back to rootpath on failure. Creation stays scoped to the user data directory. An operator-configured database.path never gets its parents created, and an unwritable data directory still degrades to rootpath rather than refusing to start. getUserDataDir, the resolve-and-create variant, has no caller left. Keeping one around invites reintroducing the bug where a reporting path creates the directory it reports on.
Connecting to SQLite creates the database file plus its WAL and SHM sidecars, so running `vikunja doctor` in the wrong directory left three files behind and reported "Connection: OK" against an empty database it had just created. Stat the resolved path first and report it as a check of its own. If the file is missing, skip connecting entirely.
database.path: memory made doctor call CreateDBEngine, which creates a real WAL database under os.MkdirTemp and leaks it on every invocation, then reported "Connection: OK" against it. Report the ephemeral database and verify nothing. Also drops checkSqliteFile's redundant bool return, which duplicated CheckResult.Passed on every path.
checkSqliteFile only checked the stat error, so a database.path pointing at a directory passed "Database file" and surfaced one line later at Connection. Mirrors the files-side check, with the polarity inverted.
pkg/doctor had no test for the database half — the part that created vikunja.db and its WAL sidecars. Covers the missing-file, ephemeral and path-is-a-directory cases, each asserting nothing lands on disk. Verified by mutation: removing the early return, the memory branch or the IsDir guard fails these tests.
Every relative config path resolves against it and it defaults to the working directory, so a reported path is untraceable without it. It sits with the config file it explains rather than in system info.
A relative --config rendered as-is next to absolute paths in the same report, which tells the reader nothing without knowing the cwd it came from.
Capturing a config value before init returns "", and Key.Set writes at viper's override level, which outranks defaults. Restoring a captured value therefore pinned database.path to "" for the rest of the process, and TestRestore resolved it to the bare rootpath and failed with "is a directory". Only visible when VIKUNJA_TESTS_USE_CONFIG=1, which CI sets for every database except sqlite-in-memory, which is why every test-api job failed while local runs passed. ResetForTests restores genuine fresh-process state instead.
TestInitFileHandler_S3Configuration and TestInitFileHandler_LocalFilesystem replaced the package-level mem storage and never put it back, so any test ordered after them failed. Pre-existing; surfaced by running with -shuffle=on.
23954c7 to
ff88e09
Compare
2 participants
vikunja doctorcreated afilesdirectory and a SQLite database in the working directory, then reported them as existing, writable, and connected — passing checks against storage it had just made itself.Reported at https://community.vikunja.io/t/vikunja-doctor-assumes-pwd-is-vikunja-files-basepath/4778
Root cause: doctor called
files.InitFileHandler(), which creates the base path, anddb.CreateDBEngine(), which creates the SQLite file plus WAL and SHM sidecars. Both are split into create vs verify, with doctor using verify-only paths. Server and restore behavior unchanged, verified against a binary built from the merge base.Also covers the same class of bug elsewhere in doctor:
database.path: memorycreated and leaked a temp WAL database per run,ResolvedDatabasePathcreated the user data directory, and a basepath pointing at a regular file passed four checks before failing.service.rootpathnow appears in the diagnostic output, since every relative config path resolves against it and it defaults to the working directory.The
--configflag that was originally part of this PR now lives in #3652.How to verify
mkdir /tmp/somewhere && cd /tmp/somewhereVIKUNJA_SERVICE_PUBLICURL=http://x.test/ vikunja doctor✗ Directory exists: ... no such file or directoryand✗ Database file: ... no such file or directory. The directory is still empty afterwards; check withls -A, which must print nothing.files/,vikunja.db,vikunja.db-shmandvikunja.db-wal.Then confirm the ephemeral database is no longer materialised by a diagnostic:
database: {type: sqlite, path: memory}and runvikunja doctor.✓ Database file: memory (ephemeral, nothing to verify)and no Connection row, and no new/tmp/vikunja-*directory appears.vikunja migratewith that same config. Expected: it still logsUsing ephemeral SQLite database at: /tmp/vikunja-.../vikunja.dband migrates successfully, because only doctor abstains.Finally, confirm the real server and restore paths still create storage:
filesdirectory from a test install, then startvikunja web.files/is recreated and the API serves/api/v1/info.vikunja restoreagainst a dump. Expected:files/is recreated there too.