Deeper fixes #1: FK indexes, session-factory invalidation, HTTP pooling - #5
Merged
Conversation
Four FK columns lacked indexes — composite-PK tables had only the forward direction covered, and one cache column wasn't indexed at all: - gauge_source.source_id (composite PK indexes (gauge_id, source_id)) - reach_guidebook.guidebook_id (same composite-PK reason) - reach_class.reach_id (FK column, no covering index) - latest_gauge_observation.source_id (non-PK FK, no index) Tables are small today (gauge_source 224, reach_class 401, reach_guidebook 1093, latest_gauge_observation 461 rows) so latency is fine, but adding the indexes now removes the future scaling cliff and matches the ix_reach_state_state_id pattern already used for the same reason. Verified by applying migration 0013 to a copy of the prod DB: all four indexes present, foreign_key_check + integrity_check both ok. init-db on a fresh DB produces the same indexes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
get_engine(url=...) disposes the old engine and creates a new one, but the cached _session_factory was only rebuilt when get_session_factory() itself was called with a url. If a caller went through get_engine() directly, the factory stayed bound to the now-disposed engine — sessions created from it would either fail or, worse, write to a stale connection pool kept alive by GC. Fix: invalidate _session_factory inside get_engine() at the same time the old engine is disposed. Added a regression test that exercises this exact path (get_engine(url) then get_session_factory() with no url). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A typical ``levels fetch`` run hits ~50 URLs across a handful of hosts
(USGS, USBR, USACE, NWPS, NWRFC, wa.gov). Calling ``requests.get()``
opens a fresh TCP + TLS connection every time; reusing a Session keeps
a per-host connection pool (HTTPAdapter default pool_connections=10,
pool_maxsize=10) so the second call to a host reuses the existing
connection.
Added _get_session() (lazy singleton) and reset_session() (test
isolation). fetch() now routes through _get_session().get(...).
Tests: TestFetch's per-method ``@patch("...requests.get")`` decorators
collapsed into a single autouse fixture that hands every test a mocked
Session. New TestSessionPooling covers singleton-ness, User-Agent
priming, reset behaviour, and that two fetches go through the same
session.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
May 10, 2026
preg_replace_callback returns unmatched portions of the input verbatim. Without pre-escaping, any HTML metacharacters between matches (or in malformed input) would land on the rendered page unescaped. The fix is to htmlspecialchars() the expression first, then run the regex on the escaped string — \w+ subsegments are unaffected by escaping, so the links are still created correctly. Verified: a synthetic <script>alert(1)</script> injected before/after a "name::gauge::type" reference renders as <script> entities while the legitimate gauge link is still produced. Live calc-expression pages (reach 265 Calapooia) render byte-identically to before for normal input. Closes pre-launch must-fix #5. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
May 23, 2026
…able regen Addresses docs/REVIEW_gradient_profile.md findings: * #1 (HIGH): drop hardcoded macOS DB default from the four phase-2 scripts. They now default DEFAULT_DB to "" and exit with "error: pass --db /path/to/kayak.db or set KAYAK_DB in env" if neither is supplied. The macOS path was wrong on both Linux locations; failing loud beats operating on whatever DB happens to match the shape. emit_max_gradient_migration.py docstring updated to match. * #3 (LOW): walk_reach take-out-emit condition rewritten as `cum_m - last_emit_m > 1.0` (the natural reading) with a clearer comment. The old `next_emit_m - interval_m < cum_m - 1.0` form is mathematically equivalent — re-sampling all 407 reaches with the new code produces identical sample counts (359,349), confirming the reviewer's "0.5m short" case is already handled correctly by the 1m close-enough tolerance. Renaming for readability, not changing behavior. * #4 (LOW): drop EPSG:4267 (NAD27) from _GEOGRAPHIC_CRS. NAD27 has a 50-100m horizontal offset from WGS84 across the lower 48 — if a tile shows up tagged 4267, we want the pyproj transform path to correct for that, not silently sample the wrong cell. 3DEP / current OPR tiles are all 4269/4326 so this branch wasn't reached in practice, but the latent risk goes away. * #6 (LOW, a11y): add role="img" + aria-label="Gradient profile chart" to the generated SVG. The hover-replaces-title interaction was leaving keyboard-nav users with no announced label when the chart came into focus mid-interaction. Also locks migration 0046 to byte-stable regeneration: * emit_max_gradient_migration.py: removed the wall-clock `at {datetime.now}` line from the header. Provenance is anchored by the source-DB sha256[:16] which already lives in the comment above. Now a fresh regen against an unchanged DB is byte-identical to the committed file — a `diff` cleanly tells "methodology changed" or "local DB drifted" without false positives from a timestamp. * Migration 0045 docstring updated to match the current JSON shape (top-level `default_rmse_m` / `src_rmse_m` / `src_histogram` after the per-sample-RMSE refactor; `min_drop_ft_for_significance` is now per-window so doesn't live at the top level). * Migration 0046 regenerated against the post-walk_reach-readability cache. Verified byte-identical across two consecutive regens. Findings deferred (not blockers, flagged for follow-ups): * #2: 0046 size — 11 MiB per regen. Reviewer recommends splitting max_gradient (small) from gradient_profile (large) next time methodology changes. * #5: find_tile O(samples × tiles). Fine at ~150 tiles; spatial index when coverage grows. * #8: el._kayakMap cross-script handle. CustomEvent refactor would be more idiomatic. * #9 (NIT): field-order change in 456a37a was in the wrong commit. Retroactive split not worth it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
May 23, 2026
Adds a Responses section to docs/REVIEW_gradient_profile.md mapping each finding to the action taken in commit 1ea9a4b plus the verification results. Findings #1, #4, #6 fixed; #3 reworded as readability-only (the "bug" was already correctly handled by the 1m close-enough tolerance, verified by identical sample counts on re-resample); #2/#5/#8/#9 deferred with rationale; #7/#10 confirmed clean. Verification checks #1 and #2 both pass after fixing a stale 0045 docstring and dropping the wall-clock timestamp from the 0046 generator (now byte-stable across regens). Format: one entry per finding, so the next live-host review pass can read top-to-bottom and see exactly what changed vs. what was tabled. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
Jun 4, 2026
install-config.sh (MEDIUM): the secrets sanity check enforced only a non-empty TURNSTILE_SECRET via a bare grep, so a fresh host could pass the installer with the site key missing — and turnstile_enabled() requires BOTH JSON keys, recreating the captcha-silently-off failure this PR closes. The guard now requires both keys and parses with the SAME semantics as the install wrapper's merge (export prefix, quote strip, empty = disabled — the old grep also rejected export-style lines and accepted quoted-empty values). A no-root `--check-secrets [FILE]` mode makes the real script testable; 7 new tests include a guard↔wrapper parity case pinning that any file the guard accepts merges both keys. review_send_reply (minor #1): the reviewer-note merge happened in PHP from the request-start row, so two concurrent reply tabs (both passing the `pending` predicate — replies don't flip status) would last-writer-win and drop the first reply's note. The append now happens SQL-side inside the atomic UPDATE; reviewer_note_entry() extracted so the stamp format stays shared with merge_reviewer_note(), whose docstring now explains why terminal actions can keep the PHP-side merge. Regression test drives the stale-row shape and asserts both notes survive. validate-config (minor #2): add the SQLITE_ prefix and allowlist SQLITE_PATH (PHP db.php fallback + health-check.sh DB override; not a model field — python uses DATABASE_URL). Strict scan re-verified rc=0 against the live host's env set with the new prefix active. health-check.sh (minor #3): comment the bare fu.is_active under GROUP BY s.id — SQLite's bare-column extension, safe because is_active is functionally dependent on s.id; flags it against a future "fix" to MAX() or an unexamined port to a stricter engine. Findings #4 (wrapper merges all secrets.env keys — observation, no action; documented in secrets.env.example) and #5 (deploy-day transient — acknowledged in the scope notes) per the review need no change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mousebrains
added a commit
that referenced
this pull request
Jun 4, 2026
…crets merge, runbook fixes (#119) * fix: address 2026-06-03 gpt-5.5 project-review findings health-check.sh (HIGH): the script only checked the single global MAX(observed_at), so a dead feed — or a source that never produced data at all — was invisible while any other source kept writing, and the healthchecks.io heartbeat stayed green. Add a per-source liveness check: active gauge-linked fetch-backed sources fail when never-fed or silent > STALE_SOURCE_DAYS (default 14, env-overridable, numeric-guarded against silently disabling the SQL); OGC-fetched USGS sources (selected by agency + gauge link — `fu.is_active IS NOT 1` so an inactive-fetch_url USGS source isn't a scope hole) get the same silent->fail window once fed, but never-fed ones are exempt as speculative metadata additions awaiting upstream OGC coverage (operator decision 2026-06-03). Reconcile docs/slo.md SLO F to what is actually enforced — the documented per-source "cadence + 2 h" model never existed anywhere. 12 regression tests run the real script against an ORM-built SQLite DB (tests/test_scripts/test_health_check.py). Verified against the prod snapshot: 0 offenders at the 14-day window, so the new check is green on deploy day. config-refresh runbook (MED): docs/operations.md still told the operator to run the old `sudo levels emit-config --out ...` — the exact pat->root RCE the review-3 R1.5 wrapper eliminated. Rewrite to the real flow (`emit-config --dry-run` piped into the root-owned kayak-install-runtime-config) and document the one-time wrapper + sudoers install. Also fix deploy/SETUP.md's claim that the §7 sudoers grant invokes the pat-writable venv levels binary. local PHP quick start (MED): README, CLAUDE.md, and .env.example all ended with a bare `php -S`, which 500s every dynamic page since Config became fatal-on-missing (T3.3 Phase 4). Add the `levels emit-config --out` + KAYAK_CONFIG_PATH step everywhere and correct the stale "PHP gets SQLITE_PATH from nginx fastcgi_param" claims (database_path from the runtime-config JSON first, SQLITE_PATH env fallback). Smoke-tested locally: 200 with the config step, 500 without. review reply race (LOW): review_send_reply() updated reviewer_note with no status predicate, so a stale "reply, keep pending" tab could mutate an already-approved/rejected row and email the editor a misleading "still pending" message. Use the same atomic `WHERE id = ? AND status = 'pending'` + rowCount() pattern as the terminal actions, return bool, and surface "Already reviewed by another maintainer." in the handler. Race regression test added. validate-config (LOW): add METADATA_ and USGS_ to the scanned prefixes, declare hc_fetch_osmb/hc_status (referenced by their systemd units but never added as fields — either would have failed the first strict deploy), and allowlist KAYAK_DATA / KAYAK_VENV / USGS_API_KEY (env-read names that must not become model fields; USGS_API_KEY in particular would otherwise be emitted into the www-data-readable runtime-config.json). Flip scripts/deploy.sh to `validate-config --known-env --strict`; verified rc=0 against the live host's actual .env contents. stale docs (LOW): drop the long-dead `pages` table from db_sync.md and export_metadata.py's docstring, and correct the levels-test.wkcc.org/_internal/ claims — that host 301s wholesale to levels.wkcc.org since 2026-05-19. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: merge secrets.env into runtime-config JSON in the install wrapper gpt-5.5 take-2 review (2026-06-03), MEDIUM, confirmed FIRED in production: the live login page serves no Turnstile widget — captcha is silently off. Root cause is the collision of two individually-sound changes: review-3 R1.5 moved the runtime-config render to unprivileged pat (`levels emit-config --dry-run`), which cannot read /etc/kayak/secrets.env (0600 root:www-data), so TURNSTILE_SITE_KEY / TURNSTILE_SECRET vanish from the JSON; and T3.3 Phase 4 removed PHP's getenv fallback, so the FPM-pool env re-export no longer rescues it. turnstile_enabled() then false-paths to turnstile_verify() === true. config.py's secrets.env comment still described the pre-R1.5 root-rendered flow that made this work. Fix: the root-owned kayak-install-runtime-config wrapper now merges secrets.env into the piped JSON before installing — each KEY=VALUE lands as lowercase(KEY) unless the rendered JSON already carries a non-empty value (parity with config.py's load_dotenv override=False precedence). This keeps both prior security decisions intact: the R1.5 boundary (no pat-writable code runs as root; the merge code is inside the fixed root-owned wrapper) and Phase 4's JSON single-source-of-truth. `export KEY=VALUE` lines are accepted, matching python-dotenv and systemd EnvironmentFile parsing of the same file (adversarial-review finding: without that strip an export-style secrets.env would mint a bogus "export turnstile_secret" key and re-create the captcha-off bug). Test hooks KAYAK_INSTALL_DEST / KAYAK_INSTALL_SECRETS are honored only when euid != 0 — as root (the sudoers entry) the paths stay fixed, and sudo's env_reset (no SETENV tag) strips the vars anyway, so the hooks add no escalation surface. 10 new wrapper tests run the real script (tests/test_scripts/test_install_runtime_config.py); a new PHP test drives the full production shape end-to-end (pat-shaped JSON without turnstile keys + secrets.env -> wrapper -> Config -> turnstile_enabled() true). Prose swept to match: config.py secrets comment, turnstile.php docblock (claimed a getenv fallback Phase 4 removed), kayak-fpm-pool.conf (its env re-export is now an unread legacy channel), secrets.env.example (documents the merge flow + both keys), SETUP.md, operations.md, deploy.sh step 3.5. Also annotated the Phase 4.3 draft SLO targets in PLAN_production_discipline.md as superseded by docs/slo.md (take-2 residual note). Deploy note: the wrapper at /usr/local/sbin is a copied artifact — the fix is live only after `sudo install -m 0755 -o root -g root deploy/kayak-install-runtime-config.sh /usr/local/sbin/kayak-install-runtime-config` and a deploy (or the emit|install step) re-renders the JSON. Until then kayak-config-drift.service flags the stale copy (byte-exact cmp). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: archive the gpt-5.5 project-review reports (2026-06-03) Both rounds of the external review whose findings the two preceding commits address, archived per the docs/done/REVIEW_* convention. Round 1: healthcheck per-source freshness (HIGH) + runbook/quick-start staleness + review-reply race + validate-config gaps. Round 2 (take 2, reviewing commit 39e7b6c): the Turnstile secrets-merge hole in the runtime-config install pipeline, confirmed fired in production. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: index the archived gpt-5.5 reviews in docs/done/README.md test_docs_done_plans_are_indexed (review-4 R2.3 guard) requires every docs/done/REVIEW_*.md to have an index row — fc8c16d archived the two reports without one, which is exactly the drift the guard exists to catch. CI red on #119; lesson re-learned: docs-only commits still get the full test gate in this repo. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: address PR #119 review findings install-config.sh (MEDIUM): the secrets sanity check enforced only a non-empty TURNSTILE_SECRET via a bare grep, so a fresh host could pass the installer with the site key missing — and turnstile_enabled() requires BOTH JSON keys, recreating the captcha-silently-off failure this PR closes. The guard now requires both keys and parses with the SAME semantics as the install wrapper's merge (export prefix, quote strip, empty = disabled — the old grep also rejected export-style lines and accepted quoted-empty values). A no-root `--check-secrets [FILE]` mode makes the real script testable; 7 new tests include a guard↔wrapper parity case pinning that any file the guard accepts merges both keys. review_send_reply (minor #1): the reviewer-note merge happened in PHP from the request-start row, so two concurrent reply tabs (both passing the `pending` predicate — replies don't flip status) would last-writer-win and drop the first reply's note. The append now happens SQL-side inside the atomic UPDATE; reviewer_note_entry() extracted so the stamp format stays shared with merge_reviewer_note(), whose docstring now explains why terminal actions can keep the PHP-side merge. Regression test drives the stale-row shape and asserts both notes survive. validate-config (minor #2): add the SQLITE_ prefix and allowlist SQLITE_PATH (PHP db.php fallback + health-check.sh DB override; not a model field — python uses DATABASE_URL). Strict scan re-verified rc=0 against the live host's env set with the new prefix active. health-check.sh (minor #3): comment the bare fu.is_active under GROUP BY s.id — SQLite's bare-column extension, safe because is_active is functionally dependent on s.id; flags it against a future "fix" to MAX() or an unexamined port to a stricter engine. Findings #4 (wrapper merges all secrets.env keys — observation, no action; documented in secrets.env.example) and #5 (deploy-day transient — acknowledged in the scope notes) per the review need no change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #4 (auto-closed when its base branch
cleanup-and-fixeswas deleted on merge of #3).Summary
Second batch from the deep-review pass. Three small targeted fixes — each one a real correctness or scaling concern surfaced by the original review.
1. FK indexes (migration 0013)
Four FK columns weren't covered by an existing index:
gauge_source.source_id— composite PK only indexes(gauge_id, source_id), so reverse-direction lookups full-scanned.reach_guidebook.guidebook_id— same composite-PK reason.reach_class.reach_id— non-PK FK, no covering index.latest_gauge_observation.source_id— non-PK cache FK.Tables are small today (224 / 1093 / 401 / 461 rows), so latency is fine; the indexes remove the future scaling cliff and match the
ix_reach_state_state_idpattern already used for the same reason. Migration applied cleanly on a copy of the prod DB;foreign_key_check+integrity_checkbothok;init-dbon a fresh DB produces matching indexes.2. Session factory invalidation (db/engine.py)
get_engine(url=...)disposed the old engine and created a new one, but the cached_session_factorywas only rebuilt whenget_session_factory()itself was called with a url. A caller going throughget_engine()directly left the factory bound to the now-disposed engine — sessions created from it would either fail or write to a stale connection pool kept alive by GC.Fix: invalidate
_session_factoryat the same time the old engine is disposed. Regression test exercises the exact path.3. HTTP connection pooling (utils/http_client.py)
A typical
levels fetchrun hits ~50 URLs across a handful of hosts. Callingrequests.get()opened a fresh TCP+TLS connection per call. Hoisted a module-levelrequests.Session(lazy via_get_session()) so the second call to a host reuses the pool.reset_session()for test isolation.fetch()now routes through_get_session().get(...).Test fixtures simplified: per-method
@patch("...requests.get")collapsed into one autouse fixture. NewTestSessionPoolingcovers singleton-ness, User-Agent priming, reset behaviour, and shared-session usage.Test plan
make test— 579 passed, 1 deselected (was 574)ruff checkandruff format --checkcleanmypy src/cleancp ../DB/kayak.db /tmp/kayak-mig13.db; all four indexes present;foreign_key_check+integrity_checkbothokinit-dbon a fresh DB produces all four indexes🤖 Generated with Claude Code