Log pano imagery transitions instead of reading a snapshot (#4947)

[jonfroehlich]

Closes #4947. Supersedes and closes #4935.

The problem

/admin/street-status charted "Panoramas whose imagery went away" off pano_data.expired_at, which is current state, not history: it is cleared the moment the imagery comes back — by a provider re-check that finds it (the non-expired branch of PanoDataTable.updateExpiredStatus) and by a labeler viewing the pano again (PanoDataTable.upsert).

So a pano that expired in March and returned in May didn't move buckets, it vanished from March. Past weeks shrank between two loads of the same page, and the recovery — the thing that separates permanent imagery loss from a provider blip, and so decides whether a city needs a re-drive — was applied silently and retroactively. #4932 shipped the chart with a paragraph apologizing for exactly this.

The change

Evolution 364 adds pano_imagery_change, the same shape street_edge_status_change took in 358: keep the current-state column because it is cheap to read and join, and add an append-only log as the record of what happened. Rows carry the state moved into and whether a provider check or a labeler's view established it.

• Edge-triggered, which is what keeps the table free. A nightly re-check of an already-expired pano and a labeler loading a pano that was never expired both record nothing. Logging checks rather than transitions would cost a row per pano per night — 266k on the largest pano_data — where real crossings run to a few thousand a year.
• All three writers stay single statements. Each needs the pre-update value of expired and can't read it back afterwards (UPDATE ... RETURNING hands back the new row), so each gets a CTE that sees the statement's snapshot — the pattern db/scripts/helpers.sh already uses for street_edge_status_change. Every method's DBIO[Int] row-count contract is unchanged.
• Backfilled from the dates expired_at still holds, so the chart keeps the history it has today rather than starting empty. Panos that expired before 358 added the column have no date to seed from; those stay in the panos_expired_undated footnote the page already renders.
• The chart now draws both directions as a grouped bar pair with a legend, and the caveat paragraph is gone.

A write-surface audit over app/, db/, scripts/, and the evolutions confirmed those three sites are the only writers of pano_data.expired/expired_at, so the log cannot miss a transition.

Testing

• PanoImageryLogSpec (new, DB-backed): both halves of the contract — every real crossing recorded exactly once, and nothing recorded for the confirmations that dominate the traffic. Getting the second half wrong is a row per pano per night rather than a wrong number, so the edge semantics are pinned against a real Postgres. Added to the CI gating-test list.
• EnumTypeParitySpec: pano_imagery_change_source against PanoImageryChangeSource. The writers cast a Scala-supplied string to this type inside raw SQL, so drift fails the pano upsert every labeler's viewer performs, not just a read.
• StreetLifecycleServiceSpec: the payload has to carry both counts and an ISO week_start. The client indexes these by name and draws zeros on a miss, so a rename would read as a quiet week rather than a failure.
• streetStatusTrend.test.js: the two series render separately, and a loss stays in its week when the imagery later comes back.

All green locally: Test/compile (warning-clean under -Xfatal-warnings), scalafmtAll, all four frontend linters, lint-evolutions, 35/35 across the four affected specs, 23/23 JS.

Evolution 364 was also dry-run inside a rolled-back transaction against a real schema (backfill count, table/type/index/FK shape, edge-triggering, and the rollup query), then applied and QA'd on a booted app: /adminapi/streetStatusTrend serves pano_imagery_changes with both counts, and a seeded loss stays in its own week while its recovery lands in a later one.

🤖 Generated with Claude Code — Opus 5 (1M context), claude-opus-5[1m]

---

Renumbered 363 → 364 after the v11.9.0 release landed its version-bump row on develop as 363.sql. The rename carried its four comment references, and develop was merged in (one conflict, the CI gating-test list, resolved as the union of both additions).

[jonfroehlich]

Deep review

Verdict: sound design, correctly implemented, safe to merge. The core mechanism does what the PR claims — verified empirically against Postgres rather than by reading. Three findings, none blocking; only the first would change code, and its obvious fix is wrong in a way worth recording.

1. The edge CTE can drop a recovery under concurrency — but FOR UPDATE is not the fix

app/models/pano/PanoDataTable.scala:223, :236, :342

All three CTEs read pano_data on the statement snapshot, taken before the statement blocks on the row lock. If the nightly sweep's updateExpiredStatus(expired = true) commits while a labeler's upsert is in flight, the sweep logs a TRUE row, the upsert's edge (which saw NOT expired) logs nothing, and ON CONFLICT DO UPDATE sets expired = false. The log keeps a "went away" with no matching "came back" for a pano that is fine — and the new page copy specifically teaches admins to read that shape as imagery that is really gone.

Reproduced with two concurrent sessions against a real schema:

result -> pano.expired=false   log=true      # dangling loss, no recovery

The textbook remedy is FOR UPDATE on the edge SELECT. It makes things worse. With no concurrency at all, on an already-expired pano:

as written  -> pano.expired=false   log=false     # correct recovery logged
FOR UPDATE  -> pano.expired=false   log=(empty)   # logging silently disabled

The CTE's FOR UPDATE collides with the same statement's own ON CONFLICT DO UPDATE on that row, and edge comes back empty. It doesn't fix the race and it breaks the primary path. PanoImageryLogSpec would catch it, but it's worth not going down that road.

Real severity: low. The window is one autocommit statement (sub-millisecond), needing a labeler to submit the exact pano the sweep is re-checking at that instant. Note COUNT(DISTINCT pano_id) does not absorb this one — it's a missing event, not a duplicate.

Resolution: documented on updateExpiredStatus rather than fixed. The airtight fix is an AFTER UPDATE row trigger on pano_data (sees true OLD/NEW after locking, and drops the "every writer must remember to log" invariant this PR maintains by hand), but that's a new pattern here — the DB has zero non-internal triggers — and RETURNING OLD.* needs PG 18, while we're on 16.1. Not worth it for this failure rate.

2. Recoveries-without-losses will be the dominant early shape, and nothing says so

app/models/pano/PanoDataTable.scala:150 and the view copy.

The ScalaDoc says the undated panos are rows pano_imagery_change "has no event for and never will" — not so. A pre-358 expired pano that regains imagery gets a FALSE row from either writer.

Bigger than a comment nit. On a dev schema the split is stark:

backfilled rows  = 0
expired_undated  = 3,635

Every currently-expired pano there predates 358, so the backfill seeds nothing and all 3,635 are recovery-only candidates. If prod looks similar, "Imagery came back" will exceed "Imagery went away" with no loss bar to explain it — on a page that just deleted its caveat paragraph. The new prose warns about losses without recoveries but is silent on the reverse, which is the case admins hit first.

3. Cache-TTL comment names the wrong tables

app/service/StreetLifecycleService.scala:114 — "most of them unindexed, over street_edge_issue and pano_data" omits street_edge_status_change and pano_imagery_change, and the imagery query moved onto an indexed column (changed_at). Comment-only.

Verified sound

Empirically, in rolled-back transactions against real Postgres:

• Edge-triggering is correct — four statements produced exactly two log rows; a confirming re-check logged nothing; a brand-new pano logged nothing and raised no FK violation.
• DBIO[Int] contracts hold — UPDATE 1 / INSERT 0 1 still come from the outer statement, so no caller's row-count check changes. (Unreferenced data-modifying CTEs do execute, as the design assumes.)
• Evolution 363 applies and reverts cleanly; no number collision (develop is at 362); OWNER TO on the table and correctly absent on the enum type.
• The backfill is safe — CHECK (expired OR expired_at IS NULL) already guarantees expired_at IS NOT NULL implies expired, so the WHERE needs no AND expired.
• Write-surface audit confirmed independently — PanoDataTable is the only writer of expired/expired_at across app/, db/, scripts/, and no runtime path deletes pano_data rows, so ON DELETE CASCADE is inert in practice.
• Wire format — the snake_case JsonConfiguration is in scope for the new writer, so pano_imagery_changes / expired_count / returned_count / week_start match what the client indexes.
• Frontend — both CSS tokens exist; MiniLineChart already does grouped bars + legend for >1 series; per-series mini-bar-hit rects give each bar a named, keyboard-reachable tooltip, which mitigates the orange/green pairing for colorblind readers.
• Empty repository trait and unused fromString both match established convention (60 and 10+ instances) — not findings.
• No docs out of sync; 358 set the precedent for this area.

🤖 Generated with Claude Code — Opus 5 (1M context), claude-opus-5[1m]