fix(p1am): anchor historian DB to package dir; ignore stray dcs_scada.db - #4068
fix(p1am): anchor historian DB to package dir; ignore stray dcs_scada.db#4068dieterolson wants to merge 5 commits into
Conversation
`database.DATABASE_URL` was a bare relative `sqlite:///dcs_scada.db`, so the SQLite file resolved against the process CWD rather than a fixed location. Two consequences: 1. The historian silently forked into a different DB per launch directory. A `uvicorn main:app` from `backend/`, a test run from the repo root, and a systemd unit with its own WorkingDirectory each got their own file, so tag history appeared to vanish depending on how the backend was started. 2. `pytest src/p1am_control_system/backend/tests/` from the repo root dropped an untracked `dcs_scada.db` at the repo root, which `git add -A` would stage. The .gitignore entries were path-anchored to the backend directory and did not cover it. Fixes: - `DB_FILE` now resolves to an absolute path anchored to the backend package directory via `Path(__file__).resolve().parent`, with a `P1AM_DB_PATH` env override for deployments keeping the historian on separate storage. This makes the location documented in BENCH_HANDOFF.md (`backend/dcs_scada.db`) authoritative. The URL uses `as_posix()` so a Windows drive path does not put backslashes into `sqlite:///...`, where they would read as escapes. - Docker is unaffected: the image does `WORKDIR /app` + `COPY . /app`, so the package directory *is* `/app` -- the same path the compose `dcs_db_data` volume mounts. The default resolves where it always did. - Also fixes `data_capture._db_size_bytes`, which called `os.path.getsize(DB_FILE + suffix)` and was therefore measuring whichever DB happened to sit in the CWD (or nothing), making the historian size cap and status display inaccurate. - .gitignore entries are now unanchored (no slash => git matches at any depth), covering both the backend directory and any other CWD. Verified: full p1am backend suite passes from the repo root (878 passed, 6 skipped), no stray DB at the root afterwards, ruff check + format clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ff5a7f631
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """ | ||
| override = os.environ.get("P1AM_DB_PATH", "").strip() | ||
| if override: | ||
| return str(Path(override).expanduser().resolve()) |
There was a problem hiding this comment.
When an operator supplies a relative override such as P1AM_DB_PATH=data/historian.db, Path.resolve() anchors it to the process CWD, so launching from different directories still creates separate historians—the exact failure this change is intended to prevent. Since the override is advertised without an absolute-path precondition, reject relative values or resolve them against a fixed base; the new test currently covers only an absolute tmp_path.
AGENTS.md reference: AGENTS.md:L129-L133
Useful? React with 👍 / 👎.
Performance Benchmark ResultsNo benchmark results available. |
…-2026-08-13 `main` moved under this branch when CONS-A1 landed, and the PR went `mergeable_state=dirty`. With `strict_required_status_checks_policy` true and `allow_update_branch` false, auto-merge cannot land a behind-or-conflicting branch — it would have sat silently forever. One conflict: SPEC.md. Both sides insert into the §12 change-log table directly under its header. Resolved by rebuilding deterministically rather than by hand: new main's SPEC.md, plus this batch's eleven §3 P1AM subsections inserted once each, plus its single §12 row. CONS-A1's three rows are preserved because they are already in new main. Verified: the diff against `origin/main` is +403/-0 — purely additive — with one `## 3. Goals & Non-Goals`, one `## 12. Change Log`, eleven `### 2026-07-31 P1AM` headings and exactly one added table row. `.gitignore` auto-merged. Note that its diff against main is deliberately **+9/-4, not zero**: CONS-A1's `.codex-worktrees/` block is present and intact (this branch never edited it), while the remaining delta is #4068's own change — three anchored `src/p1am_control_system/backend/dcs_scada.db*` patterns replaced by unanchored equivalents plus the comment explaining why. A zero diff here would mean #4068's fix had been dropped. The four `pdf_renamer` files, `scripts/test_assertion_allowlist.txt` and the two workflow files CONS-A1 touched come across cleanly and now match `origin/main` byte-for-byte. This branch had restored them to the previous main during the churn strip, so its side was unchanged from the merge base and git took theirs without a conflict. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Superseded by #4448 (the P1AM SCADA safety consolidation), which merged to Containment was verified before closing, with all three stages of Closing to reduce CI load rather than because the work was unwanted. |
Pull request was closed
1 participant
Problem
database.DATABASE_URLwas a bare relativesqlite:///dcs_scada.db, so the SQLite file resolved against the process CWD rather than a fixed location. Two consequences:uvicorn main:appfrombackend/, a test run from the repo root, and a systemd unit with its ownWorkingDirectoryeach got their own file — so tag history appeared to vanish depending on how the backend was started.pytest src/p1am_control_system/backend/tests/from the repo root dropped an untrackeddcs_scada.dbat the repo root, whichgit add -Awould happily stage. The.gitignoreentries were path-anchored to the backend directory and did not cover it. This nearly landed in SCADA: fix poll-loop data integrity, cadence and backpressure (#4004 #4008 #4009 #4023 #4024) #4064.Changes
backend/database.py—DB_FILEnow resolves to an absolute path anchored to the backend package directory viaPath(__file__).resolve().parent, with aP1AM_DB_PATHenv override for deployments keeping the historian on separate storage. This makes the location documented inBENCH_HANDOFF.md(backend/dcs_scada.db) authoritative.Two details worth reviewer attention:
as_posix(). On Windows a raw drive path would put backslashes intosqlite:///…, where they read as escapes rather than a drive-absolute path.WORKDIR /app+COPY . /app, so the backend package directory is/app— the same directory the composedcs_db_datavolume mounts. The default resolves exactly where it always did.This also incidentally fixes
data_capture._db_size_bytes, which calledos.path.getsize(DB_FILE + suffix)and was therefore measuring whichever DB happened to sit in the CWD — or nothing at all — making the historian size cap and the status display inaccurate..gitignore— entries are now unanchored. A pattern with no slash matches at any depth, so this covers the backend directory and the repo root (and any other CWD):Verification
test_database.pycovering package anchoring, the env override, and URL well-formedness.ruff check+ruff format --checkclean.git check-ignore -vconfirms all six paths (root + backend × 3 suffixes) resolve to the new rules.dcs_scada.dbat the root, andgit statusclean of it.Not included
src/plant_simulator/train.py:51has a--db-pathdefault of"dcs_scada.db", also CWD-relative. Left alone deliberately: it's a reader invoked explicitly by an operator, a relative default is normal CLI convention, and pointing it atdatabase.DB_FILEwould coupleplant_simulatortop1am_control_system— against the LOD rule inCLAUDE.mdabout not importing across package boundaries.🤖 Generated with Claude Code