Skip to content

Deeper fixes #1: FK indexes, session-factory invalidation, HTTP pooling - #4

Closed
mousebrains wants to merge 3 commits into
cleanup-and-fixesfrom
deeper-fixes-1
Closed

Deeper fixes #1: FK indexes, session-factory invalidation, HTTP pooling#4
mousebrains wants to merge 3 commits into
cleanup-and-fixesfrom
deeper-fixes-1

Conversation

@mousebrains

Copy link
Copy Markdown
Owner

Stacked on top of #3 (cleanup-and-fixes). Set the base back to main once #3 merges.

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_id pattern already used for the same reason. Migration applied cleanly on a copy of the prod DB; foreign_key_check + integrity_check both ok; init-db on 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_factory was only rebuilt when get_session_factory() itself was called with a url. A caller going through get_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_factory at 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 fetch run hits ~50 URLs across a handful of hosts. Calling requests.get() opened a fresh TCP+TLS connection per call. Hoisted a module-level requests.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. New TestSessionPooling covers singleton-ness, User-Agent priming, reset behaviour, and shared-session usage.

Test plan

  • make test — 579 passed, 1 deselected (was 574)
  • ruff check and ruff format --check clean
  • mypy src/ clean
  • Migration 0013 applies on cp ../DB/kayak.db /tmp/kayak-mig13.db; all four indexes present; foreign_key_check + integrity_check both ok
  • init-db on a fresh DB produces all four indexes
  • CI on this PR (3.13 + 3.14 + gitleaks + lint + typecheck)

🤖 Generated with Claude Code

mousebrains and others added 3 commits May 2, 2026 20:56
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
mousebrains deleted the branch cleanup-and-fixes May 3, 2026 04:01
@mousebrains mousebrains closed this May 3, 2026
mousebrains added a commit that referenced this pull request May 10, 2026
- gauge.php / source.php text search: LIMIT 200
- reach.php text search (with or without state filter): LIMIT 200
- reach.php state-only listing: LIMIT 1000 (legitimate "all reaches in
  Oregon" page; large states can exceed 200)
- plot.php: $days clamped to [1, 366]; explicit start/end window clamped
  to 366 days; both observation queries gain LIMIT 100000

Defends against a single q=% (or 1900-2030 date) request pinning a
PHP-FPM worker for seconds. Smoke-tested: gauge/source/reach wildcard
searches and plot.php with extreme range all return <0.5s.

Closes pre-launch must-fix #4.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mousebrains added a commit that referenced this pull request May 12, 2026
Three findings from the Tier 2 + Tier 3 review of `e778053..a6d5242`;
all test-file edits, no production code touched.

Finding #1 — Lost CSP regression guard. Phase 1.4's lone drill test
(`d3e7dce` tests/php/ReachIntegrationTest.php) had:

  $this->assertStringNotContainsString(
      '<script>', $resp['body'],
      'inline <script> would clash with prod CSP'
  );

That assertion was dropped when ReachIntegrationTest expanded from 1
to 6 tests in Phase 2.1 and never made it into DescriptionIntegration
Test in Phase 3.1. The CSP-header check (`assertArrayNotHasKey('
content-security-policy', $resp['headers'])`) was preserved on the
detail tests, but the body-side guard against re-introducing bare
inline `<script>` was lost. Production runs strict CSP — bare inline
scripts would break, but the test suite would stay green.

Restore as `assertNoBareInlineScript($body)` in IntegrationTestCase
and call from every HTML-rendering test in both Reach (5 calls) and
Description (4 calls — 400 cases don't render HTML). Catches bare
`<script>inline</script>` but allows `<script src=...>` (the attribute
value pushes the closing `>` past the substring match — works for
both Leaflet and the page-specific maps).

Finding #2 — `ReachIntegrationTest.php:119` comment was wrong about
which columns the LIKE pattern matches. The comment claimed "'A test
reach' description for both — the OR-of-LIKEs picks both", but
_search_reaches_query LIKEs display_name OR name OR river — NOT
description. The test still passes because both reaches have 'Test'
in their name/display_name; only the comment misleads. Updated to
spell out the actual matching columns per row.

Finding #4 — `DescriptionIntegrationTest.php:143` comment said
'Sandy' pins "// River field", but description.php's $fields list
doesn't include a 'River' label. 'Sandy' actually matches via the
display_name "Sandy Test Reach" in <title>, <h2>, and the meta
description. The same test class already had a longer comment on
the no-gauge case (line 159-163) explaining this exact mechanism for
"Nowhere" — the gauged-reach test was inconsistent with that
explanation. Reworded the comment, kept the assertion (still valid).

Finding #3 (dead `$html_fields` entry 'AW ID' at
php/includes/reach_detail.php:350) is a pre-existing carryover from
the inline reach.php code, not introduced by Tiers 2/3. Flagged for
a separate cleanup pass.

Test/lint state:
  - All 12 integration tests pass with the new guard active
    (full PHPUnit: 67/67, +9 assertions)
  - PHPStan + php-cs-fixer green

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>
mousebrains added a commit that referenced this pull request Jun 14, 2026
…lure (errtrace)

Adversarial review of #192 surfaced a chain of rollback gaps. Fixing finding #1
(arm DOCROOT_BUILT before the build) exposed a deeper, pre-existing PR #190 bug:
the rollback never ran for the activation mutation steps at all.

- **errtrace (the big one).** The deployer ran `set -euo pipefail` without `-E`.
  Every DB/build mutation runs through `run_app()` (a function), and without
  errtrace a failure INSIDE a function aborts under `set -e` WITHOUT triggering
  the `ERR` trap. So a failed `run_app … migrate/sync/import/build` exited with
  the DB already mutated, consumers stopped, maintenance still on — and NO
  rollback (only the EXIT scratch-cleanup). `set -Eeuo pipefail` makes those
  failures hit `trap rollback ERR`. The trap is armed for the activation phase
  only (staging has no ERR trap, so `-E` is inert there); atomic_relink's `mv`
  probes sit in `if` conditions, which stay exempt. Surfaced writing finding #1's
  test — the docroot-rebuild fix is moot if rollback doesn't run.

- **#1: DOCROOT_BUILT armed before the build, not after.** `build` writes the
  live docroot in place (stage → per-file rename → orphan sweep), so a failure
  mid-write leaves it mixed yet exits non-zero. The flag must mean "build
  started" (docroot may have changed) so rollback rebuilds; a rebuild when it was
  in fact untouched is harmless/idempotent.

- **#3: normalize an absolute `current` symlink target.** The rebuild used
  `$ROOT/$PREV_TARGET`; a manual-recovery absolute `current` made that
  `/opt/kayak//opt/...`. Compute `PREV_DIR` with an absolute/relative case split.

- **nit #4: keep the rollback rebuild's stderr** (to `$SCRATCH/docroot-rebuild.err`,
  retained) instead of discarding it — the recovery path is where the reason
  matters most.

- **#2 (ACL): commit the shared-docroot web ACL** to deploy/SETUP.md's cutover
  section — the recursive + default `u:www-data:rX` on `/var/cache/kayak/docroot`
  the deployer doesn't grant; without the `-d` default ACL each freshly built
  page 403s. The nginx-root / FPM-open_basedir / unit verification gate stays a
  4C-renderer deliverable (it owns those templates).

Tests: two new slow tests — build mutates the docroot then exits 1 → rollback
rebuilds (proves the flag-before-build + errtrace fix); and an absolute `current`
target → rebuild reads the normalized dir. Full slow suite 5 passed, non-slow
1725 passed, ruff/format/shellcheck clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mousebrains added a commit that referenced this pull request Jun 15, 2026
…add tests

- #1 (the conscious trade-off): document the KNOWN LIMITATION inline — masking the
  whole line means config-drift no longer verifies the *content* of `root` /
  `open_basedir` between deploys (a manual tamper of just those lines wouldn't be
  flagged). Note that the deployer's serving-path gate still verifies them at
  deploy time, and record the verify-against-`render-serving` upgrade as the
  follow-up that closes the continuous-monitoring gap.
- #2: comment the FPM-pool-path duplication (RENDER_NORMALIZED + MANIFEST) so a
  PHP-version bump updates both, not just one.
- #4: add tests/test_scripts/test_config_drift.py — sources the script in a new
  lib mode (KAYAK_DRIFT_LIB=1, early-return before the /etc manifest walk) and
  drives normalize_rendered directly, locking the surgical mask: the docroot
  `root`/`open_basedir` are exempt, but the ACME `root /var/www/certbot;` stays
  byte-checked (a tampered ACME root still differs). Guards the certbot exclusion
  against a future sed edit.

#3 (the favicon/security.txt aliases re-introducing drift) is mooted by PR #199,
which makes those aliases docroot-relative (try_files) rather than hardcoding the
cache path — so they stay generic and need no mask.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant