Stabilize SQLite entrypoint parking test - #826
Conversation
- Keep the test wrappers ahead of inherited virtual environments - Stub timeout and recovery-server transitions deterministically Fixes dannyvfilms#825
ryan-winkler
left a comment
ryan-winkler
left a comment
There was a problem hiding this comment.
Review outcome
One small test-isolation gap remains. The proposed repair is otherwise focused, easy to review, and correctly leaves production startup behavior unchanged.
Required follow-up
Please apply the same VIRTUAL_ENV isolation to test_entrypoint_term_during_integrity_check_stops_before_migrations.
That adjacent test creates the same python and timeout wrappers and starts the same entrypoint. It still inherits the caller's virtual environment. When that environment is active, entrypoint.sh can prepend its bin directory and bypass the test wrappers. The test can then miss checker waiting and exercise the real recovery path instead of the intended TERM path.
This is the same root cause as #825. I expanded #825 instead of creating a second issue.
Security and failure-path review
- Reviewed the complete one-file diff and the entrypoint process flow that it controls.
- No production source, API, database schema, dependency, secret, or user-data path changes.
- The generated wrappers stay inside a private temporary directory and execute fixed local commands.
- No exploitable security issue was found.
- The remaining risk is false test confidence when the caller environment changes command resolution.
QA evidence
- Lint: passed.
- CodeQL: passed.
- Docker Image: passed.
- Application test job: still running when this review was submitted.
- The PR records a failing targeted test before the repair, a passing targeted test after it, five repeat passes, Ruff success, and
git diff --checksuccess.
After the follow-up change, please repeat both entrypoint subprocess tests with an active virtual environment. This tests the real boundary that caused #825.
Scope and contract check
- Documentation and Markdown changes: not needed; this is a test-only repair.
- OpenAPI and contract artifacts: not applicable.
- UI screenshots, interaction review, accessibility review, and cognitive-load review: not applicable because no user-facing surface changes.
- No new helper is needed for two tests. If a third entrypoint subprocess test is added later, move the repeated wrapper and environment setup into one test helper so all tests use the same boundary.
Related work
- Fix scope: #825
- Failure source and related application suite: #792
- Upstream comparison:
FuzzyGrim/Yamtrackhas no equivalent SQLite recovery-server or parking flow, so this repair is specific to Floppy.
Post-mortem
Trigger: the test inherited a caller-controlled environment that could reorder command lookup.
Why it escaped: the first parking test received an explicit environment fix, but the adjacent TERM test uses the same subprocess pattern and kept the old environment behavior.
Prevention: define the command-resolution boundary explicitly in every entrypoint subprocess test, and keep each wrapper limited to the command that the test owns.
The PR body was not changed.
| @@ -959,6 +963,7 @@ def test_entrypoint_parks_once_instead_of_polling_exited_checker(self): | |||
| "FLOPPY_DB_PATH": db_path, | |||
There was a problem hiding this comment.
Good fix for this test. Please apply the same isolation to the adjacent test_entrypoint_term_during_integrity_check_stops_before_migrations environment. It creates the same wrapper-first PATH, but it still inherits VIRTUAL_ENV, so an active caller environment can bypass both wrappers.
Minimal follow-up in that test's env mapping:
"PATH": f"{bin_path}:{os.environ['PATH']}",
"PYTHONPATH": str(ENTRYPOINT.parent / "src"),
+ "VIRTUAL_ENV": "",This is the same root cause as #825, not a separate issue. Please run both entrypoint subprocess tests repeatedly with an active virtual environment after this change.
|
This is the correct fix. Verified under the condition that actually breaks it. I reached the same root cause independently while chasing this failure on #830, then found #825 had it first. Adding the executable evidence, since as far as I can tell nobody had yet run either candidate fix with port 8000 free: The mechanism, for the record: Directly demonstrated, same command both ways: That is why it passes locally, where we all run Floppy on 8000, and fails in CI. Two suggestions:
|
|
Verified this fix works, and found the sibling gap #825 predicted is still open. Evidence for both. This fix is correctThe mechanism, confirmed directly: A time bound cannot fix a wait for something that never happens, which is why The sibling is still environment-dependent
The failure is the real integrity checker running instead of the stub: the log Why it is conditional rather than always broken. elif [ -n "$VIRTUAL_ENV" ] && [ -d "$VIRTUAL_ENV/bin" ] && [ "$VIRTUAL_ENV" != "$PWD/.venv" ]; thenso a venv at Latent in CI today because GitHub runners set no Suggest adding Also worth taking from #834 before closing it: it attaches the entrypoint log to |
|
Two more things a cross-model review surfaced on this, both arguments for merging it rather than against. The test only ever covered the crash fallback, and this makes that honest. It also removes a port-binding hazard from the suite. As things stand, in CI Both point the same way. Happy to open a follow-up issue for the untested |
Applies the review note on dannyvfilms#826. The parking test got this isolation; its neighbour did not, and it has the same dependency. entrypoint.sh keeps a virtual environment first on PATH unless it is the one at $PWD/.venv (the guard from dannyvfilms#762). A test that stubs commands through a wrapper-first PATH therefore loses its wrappers whenever VIRTUAL_ENV points anywhere else: the real python runs, the integrity stub never fires, and the assertion reports the real checker's output instead of "checker waiting". That is why this passes under `uv run --no-sync` from the repo root, where VIRTUAL_ENV equals $PWD/.venv and is ignored, and fails from a git worktree pointed at another checkout's venv. Measured in a fresh network namespace with port 8000 free, VIRTUAL_ENV set outside the working directory: before: FAILED 3/3 after: OK 3/3 Full config suite in the same conditions: 121 tests, all pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed the review note as Verified before pushing, all in a fresh network namespace with loopback up so port 8000 is genuinely free, and with
Two notes for whoever merges: The earlier The |
Brings in the entrypoint parking test fix (#826), which was the only failure on this branch's CI.
Found while verifying the rebase in a namespace where the test user is root. Root bypasses file permission bits, so opening a 0444 database for append succeeds, the writability check correctly finds nothing to report, and the test fails on an assertion that cannot hold there. CI runs as an unprivileged user, so this was green either way. Skipping it explicitly states the precondition instead of leaving a test that passes or fails on who runs it, which is the same fragility #825 and #826 just removed from the entrypoint tests next door. Asserts as a normal user, skips as root. Verified both ways. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Brings in the entrypoint parking test fix (#826), the only failure on this branch's CI.
Picks up latest, including the entrypoint parking test fix (#826).
- Sleep for the interval the entrypoint asked for, not a shorter fixed one. - Name which fault left no parking child. - Assert the storage check ran once, which is what "parks once" means. #826 made this test deterministic by stubbing the recovery page, so it no longer depends on whether port 8000 is free. Three smaller problems are left. The parking sleep stub runs "sleep 30" while the entrypoint asks for 86400. The stub ends, the parking loop starts another one, and the pid file is overwritten while the test reads it. The stub now passes the interval through. The other "sleep 30" in this file stands in for a hanging checker and is left alone, because its arguments are python's, not an interval. The assertion reported only "False is not true", which does not say whether the pid file never appeared or appeared naming a dead process. Those need opposite fixes, and the distinction is what identified the port 8000 cause. Verified with port 8000 taken and with it free, in a network namespace: 43 tests, 0.39s, green in both. Refs #593 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* 🔒 Remove credentials from the authority part of a URL safe_url() removed the query string and the fragment but kept the netloc, so a URL that carries "user:password@" passed through with the password intact. Private podcast feeds authenticate that way, and Redis and database connection strings do too, which means the credential could reach the log file that Settings > Advanced offers users for download. Rebuild the authority from its host and port instead of copying it. The port is absent from most URLs, and urlsplit reports None for it then, so an unconditional f-string would write the text "None" into the host name. Parse the structure rather than match a pattern. A URL has a defined shape, and reading that shape cannot miss a spelling the way a keyword list can. This complements the redact_secrets() rule that already covers URL credentials in free log text; this one keeps the reconstructed URL readable instead of leaving "[REDACTED]:[REDACTED]@" in it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * 🔍 Add a read-only SQLite inspection with a bounded scan check_database_integrity() reaches a verdict and then acts on it: it writes an incident report beside the database, creates a verified backup, can delete orphaned rows, and ends the process with sys.exit. A diagnostic cannot call it. Add inspect_database(), which reports the same verdict from the same two primitives and does none of the rest. The two callers share the primitives; neither calls the other. That distinction is load-bearing. _check_foreign_keys inspects inside BEGIN IMMEDIATE because the quarantine delete that follows must operate on the rows the inspection found, and taking that write lock in a diagnostic would block a running Floppy. Startup keeps its transaction unchanged. Bound the scan with a progress handler. sqlite3.connect(timeout=...) sets busy_timeout, which is how long to wait for a lock, and does nothing to limit how long a statement runs, so PRAGMA quick_check on a large file would run past any value given there. Lock contention and scan duration are different failures and get different bounds: five seconds for the lock, and a caller-supplied deadline for the scan. Measured on a 44.8 MB database: a full scan finishes in 0.06s, and a 0.05s deadline ends it at 0.05s instead of letting it run on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ✨ Add floppy_preflight, a startup diagnostic for operators An operator whose container will not start has no way to find out why. The startup log says migrations and services did not run, and stops there. This command answers the question directly: paths, settings, database, migrations and Redis, each reported with what broke, why, and what to do next. The checks live in app/preflight.py and the command renders them, the same split redis_tuning and tune_redis already use, so the decisions return data instead of printing it. Design notes worth knowing: Statuses are ok, warn, fail and skipped, and only fail changes the exit code. Without a warn tier the checks would have to swallow a real hazard such as a Redis server with no memory ceiling, or block a boot over it. Every fix says where to run it. A chown pasted into a container shell fails, and an environment variable edited there is gone on the next restart. The wording follows the environment, because advice about compose files, PUID and PGID is wrong for a source install or a packaged application. Every detail is redacted before it is printed. Redis client errors quote the connection string, password included, so the failure path is exactly where a credential would otherwise escape. The Redis check groups the five configured URLs by server, so the common single-server stack is pinged once and a failure names the roles that server carries. Brokers this command has no client for, such as RabbitMQ, are left to their own services rather than reported as broken Redis. The migration check does not run against a database that does not exist yet. Connecting would create the file and reading the state would create a table inside it, and looking at an installation must not build part of it. Django's system checks are run explicitly rather than left on, because their output would corrupt the JSON report, and silencing them would let a settings error pass as a clean bill of health while the container dies. Refs #597. Reuses the integrity decision from #593, the paths from #595 and the Redis endpoints from #596. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * 🩹 Point a failed startup at the diagnostic, and check mounts first Two changes to the startup script, both for the operator who is reading it because something went wrong. Name the diagnostic in each failure message. A command nobody can find does not exist, and the moment they need it is the moment these lines are on their screen. The two forms are not interchangeable: docker exec needs a running container, so it works while startup is parked and fails with "Container is restarting" once a path exits and a restart policy takes over. The parked paths print the exec form and the exit path prints the one-off form. Check the mounts before anything imports Django. A read-only or wrongly owned mount is a common reason a container will not start, and it is the one failure floppy_preflight cannot report: Django opens its log file while it loads settings, so the process dies with a traceback before any command runs. The shell can still name the directory at that point. The data directory is fatal, because nothing works without it. The log directory is a warning, which is how the ownership step further down already treats it, and the warning is what tells the operator which directory caused the traceback that follows. Both create the directory when it is absent, matching what Django does with them a moment later, so a first start on a fresh volume is not treated as a fault. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * 📝 Document the startup check, and ask for it in bug reports README gains a "Startup diagnostics" section next to SQLite startup recovery, where an operator is already looking when a start fails. It gives both container commands and says which to use for which symptom, the source form with its SECRET prerequisite, the four result kinds, the options, the JSON version policy, and a systemd ExecStartPre example. The bug report template asked every reporter to download logs from Settings > Advanced. That needs a running, logged-in Floppy, so the reports that most need diagnostics could not carry any. Add the path for an installation that will not start. AGENTS.md lists the command as the way to tell a broken environment from a broken change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * 🐛 Keep the writability check from failing a start, and stop two tests depending on the machine Four CI failures, all from this branch. The startup writability check exited when a directory could not be created or written. That broke two existing tests which drive entrypoint.sh with paths that deliberately do not exist, because they cover path validation rather than path usability, and /var/lib/floppy cannot be created without root. The check should never have been a gate. The gates already exist: the ownership step exits when it cannot chown the data directory, and Django raises when it cannot create the generated secret. What was missing is a readable line for the log directory, because Django opens its log file while it loads settings, so the process dies with a logging traceback before any Floppy message appears. Report that and continue. A pre-check that can stop a start which would otherwise have worked is worse than no pre-check. Two tests in this branch called check_migrations() without controlling FLOPPY_DB_PATH, so they inherited whatever database the machine had. They passed on a developer machine, where one exists, and failed in CI, where the new short-circuit for a first start returned "skipped" instead of reaching the mocked planner. Both now create a real database first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * 🐛 Recognise Podman as a container runtime Found by building the image and running it. Podman does not create /.dockerenv; it writes /run/.containerenv. Checking only the Docker file meant a containerised Floppy under Podman reported itself as a bare install and gave advice written for one, telling the operator to check REDIS_URL rather than the service in their stack. Podman is common in the self-hosted setups Floppy runs in, so check both markers. Verified in a real image: before this change the failure advice inside a Podman container read "check that Redis is running and that REDIS_URL points at it", and now it reads "check that the Redis service is running and reachable". The same single-marker check exists in settings.py, where it decides whether to generate a secret key. That behaves differently under Podman and is left alone here: changing when Floppy generates its own secret is a separate decision. README now states what each runtime does when exec cannot reach a restarting container, measured rather than assumed: Docker reports "Container is restarting", and Podman kills the attempt with exit code 137. Twelve exec attempts against a looping container failed twelve times, and the one-off form succeeded every time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * 🧪 Cover the PostgreSQL branches, which had no tests at all Comparing the QA scenarios against the committed suite showed check_database's PostgreSQL path was never exercised, including the three branches that read the SQLSTATE to tell a refused password from a refused connection. Both read as "could not connect" and they send an operator to different places, one to their credentials and one to the network, so the distinction is the point of that code and it needed a test. Also asserts no database password reaches the report, matching the Redis equivalent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * 🐛 Six fixes from the final QA pass A last review round against the finished code and a running container. Never migrate on top of a broken foundation. When paths, settings or the database failed, "skipped" migrations means unknown rather than pending, and --auto-migrate ran migrate against an unreadable database, which can only make an incident worse. Redis failing does not block it, and neither does the migration check itself. Do not call a mount mistake corruption. A directory at the database path was reported twice: once correctly by the paths check, and again by the database check as "the database file cannot be read", advising a backup restore. That sends an operator to recover data that was never lost. The database check now defers to the paths check for this. Refresh the database result after migrating. On a first start the database check says "no database file yet". Once migrate had created it, the report still printed that sentence. Check the database file itself, not only its directory. Every other check reads, so a database file that was readable but not writable passed all of them and the container then died on its first write. The file is now probed by opening it for append, which adds and truncates nothing. Redact the crash report. An unexpected exception went into the report verbatim, so a settings or connection error carrying a password would have printed it. Carry the migration output. The failure advised reading an error that had been captured to keep the report parseable, and then discarded. The last twenty lines now travel with the result. Rejected one finding with evidence: redis-py decodes CONFIG replies regardless of decode_responses, so config_get returns str keys and the memory-ceiling warning fires correctly. Confirmed against a real unbounded Redis. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * 🧪 Skip the read-only database assertion when running as root Found while verifying the rebase in a namespace where the test user is root. Root bypasses file permission bits, so opening a 0444 database for append succeeds, the writability check correctly finds nothing to report, and the test fails on an assertion that cannot hold there. CI runs as an unprivileged user, so this was green either way. Skipping it explicitly states the precondition instead of leaving a test that passes or fails on who runs it, which is the same fragility #825 and #826 just removed from the entrypoint tests next door. Asserts as a normal user, skips as root. Verified both ways. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2 participants
Summary
Problem
test_entrypoint_parks_once_instead_of_polling_exited_checkerinheritsVIRTUAL_ENVfrom the test runner. The entrypoint correctly prepends that environment toPATH, which bypasses the test's command wrappers. The test then starts the real recovery server and only reaches its asserted parkingsleepwhen that server exits, making the result depend on platform, port state, and test order.This was the only failure after 3,981 tests in PR #792's application suite and reproduced as a targeted failure locally on the unchanged
latestentrypoint/test code.Solution
VIRTUAL_ENVin this test subprocess so its explicit wrapper directory stays first.timeoutwrapper.Production entrypoint behavior is unchanged.
AI Assistance
Generated with OpenAI Codex (
gpt-5.6-sol, Codex 5.6 SOL).Validation
parking_child_before_term == False.uv run --no-sync ruff check src/config/tests/test_sqlite_integrity.py— passed.git diff --check— passed./proc/self/fd; those platform failures are unrelated to this test-only patch. The hosted Linux application suite is the authoritative module/full-suite gate.Contract Handoff
Human Review
Gstack QA
/gstack-qa.Migration Sync Gate (Required for
upstream->latestsync PRs)Not applicable; this is not an upstream sync PR and contains no migrations.
Notes