Record imagery expiries, street status changes, and job runs (#4928) - #4932
Conversation
Every imagery and street-status surface was a snapshot of the current state, so "what newly expired this week" and "which streets were closed this month" had no answer, and a nightly job that silently stopped firing looked exactly like one that found nothing to do. Evolution 358 adds the three records that were missing: - `pano_data.expired_at`, stamped only on the false -> true edge, so it dates the disappearance rather than the last look (which `last_checked` already bumps on every sweep). - `street_edge_status_change`, the only trace the `db/scripts` status writers leave. `street_edge.status` has no application write path, so the scripts log old status, new status and which script ran, guarded so a re-run over the same CSV records nothing. - `background_job_run`, one row per run of every scheduled actor, opened before the work and closed with its outcome and counts. `JobRunService.record` brackets each job and is strictly subordinate to it: a bookkeeping failure is logged and swallowed, and the job's own failure propagates unchanged. `ScheduledJobs` now holds the nightly schedule in one place, which each actor reads instead of a literal, and which doubles as the roster the Health panel checks so a job that has never run still gets a row. The imagery jobs return structured counts rather than a log line, which is what makes the #4384 freshness pipeline checkable: the poll's rotation coverage and the sync's flag counts are now persisted (#4908). Two admin surfaces read it back: a "Recently changed" section on /admin/street-status charting status transitions, missing-imagery reports and pano expiries by week, plus the queue of still-open streets several labelers independently reported (#4922) for the offline checker to confirm; and a "Nightly jobs" panel on /admin/health flagging anything overdue, failed, or never run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
audit_task.outdated_imagery said a completed audit predates its street's newer imagery, but not since when -- so flag-to-re-audit latency and "newly flagged this week" were unanswerable, and retroactively so. The sync's set-pass now stamps outdated_imagery_at (it only touches unflagged rows, so the stamp marks the false-to-true edge and survives re-runs) and the clear-pass nulls it, mirroring pano_data.expired_at. Also enrolls the three pre-existing outdated-imagery specs in the ci.yml testOnly allowlist, which they had been missing from. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
) Deep review turned up three ways the Health panel could show green while knowing nothing, all in the same direction: the panel exists to catch a scheduler that stopped, and each of these let it miss one. Overdue now keys on the last successful *scheduled* run rather than on the latest run of any kind. A sweep an admin triggers from /adminapi is recorded under the actor's own job name, so it was clearing the alarm for the job the scheduler had stopped firing -- exactly what JobRunTrigger's own contract says must not happen. The same read also called every job overdue for as long as it was running, since an in-flight run is not a successful one; keying on the last success leaves the previous night's run standing through the next one. The panel now also says when a last run was hand-triggered, which it never showed. An empty jobs list rendered as "No scheduled jobs are configured" behind a green check. The roster is a compile-time constant, so empty can only mean the background_job_run read failed and was recovered -- the blind-panel-reads-clear failure the roster-driven design was meant to rule out. It renders as a problem now. The expiry chart is a snapshot, not a series: expired_at is cleared when imagery returns or a user views the pano, so past weeks shrink between visits. Said so on the page and in the DAO rather than implying a transition log. Also: cache the trend payload for ten minutes (six unindexed scans backed every load and every window change, uncached); queue a window change made mid-fetch instead of dropping it; source the default window from the server; stop double-escaping an error that already goes through textContent; count the status-change summary as changes rather than streets, which is what the per-bucket SQL actually returns; and note that corroboration counts accounts, not people, since every anonymous sign-up mints its own user. Adds lastScheduledSuccessPerJob coverage for the manual, in-flight, and never-succeeded cases, and makes the undated-expiry spec assert the exclusion its name promises. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Enrolling OutdatedImageryFlagSyncSpec in CI turned two of its refreshFromPanoData cases red -- the first time they had ever run outside a dev DB. Both cases place a pano on a street's midpoint and expect the refresh to give that street an imagery row. The refresh assigns each pano to its *nearest* street via DISTINCT ON, so that only holds if exactly one street is nearest, and a street taken from whatever the connected database happens to hold cannot promise it. CI's seed clones the tutorial street's geometry onto its one non-tutorial street, so the pano sits at distance zero from both; the tie breaks arbitrarily, and when it falls to the tutorial street the row is dropped by the tutorial filter and the street under test gets nothing. Hence row.isDefined failing outright in one case, and only the updated_at assertion failing in the other -- every earlier assertion there passes precisely because the row went untouched. Each case now inserts its own street, off the coast of Africa where no city has one, and takes the id from MAX rather than the sequence, which a dump-restored dev DB leaves behind the table's own ids. Verified against the dev schema in a rolled-back transaction: the insert succeeds and no other street falls within the refresh's 0.001-degree prefilter of the new midpoint, so there is no tie left to break. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Prod data says the loophole it warns about has never occurred: across 26 weeks of Seattle reports, no street reaches the threshold on two accounts sharing an ip_address, and the single street that qualifies at all does so with four independent reporters. A caveat for a case with no instances is noise in front of a reader looking at a one-row queue. The caveat stays in the ScalaDoc, where it is aimed at whoever considers automating this predicate -- which is when accounts-not-people would start to matter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The alarm exists to catch a scheduler that has stopped, so the load-bearing rule is what counts as evidence it is still running: only a scheduled run that succeeded. A run someone triggered by hand from /adminapi shows the code works, not that anything still fires it, and a run merely in flight has proved nothing yet -- and both land under the same job_name as the nightly one, so this rule is all that separates them. That rule lived only in an expression inside a private method. Anyone simplifying it back to "the latest run, whatever it was" would reintroduce both bugs silently, since the panel looks healthier afterwards, not worse. Five cases, driven through getDbHealth against seeded history, including a control that pins overdue can be false at all -- without it the suite would pass against an implementation that never cleared. Confirmed the suite discriminates by reverting the fix in a throwaway copy: the hand-triggered and in-flight cases fail (false was not equal to true, and its converse), the other three pass on both implementations. Enrolled in the ci.yml allowlist, since a spec absent from it is dead weight while CI stays green -- the trap this PR set out to close. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
19ba268 to
678170f
Compare
…y-and-status-transitions # Conflicts: # .github/workflows/ci.yml
Deep code reviewReviewed at Overall this is careful, well-documented work. The SQL in particular avoids the traps it could have fallen into: The findings below cluster around one theme, which is worth stating plainly because it's the PR's own thesis: several paths let the Health panel report health it doesn't actually have. Blocking before merge1. Evolution Whoever merges second gets an add/add conflict on that exact path, and the natural-looking resolution for what appears to be a duplicate file (take one side) silently drops the other PR's entire schema change. The failure mode is nasty because CI stays green either way — it seeds from the evolutions that survive, so the surviving file applies cleanly — while at runtime Correctness — the panel can report health it doesn't have2. A poll that structurally cannot run is recorded as a success. If a city's Maps key expires or is rotated out, every night records Note: 3. A hand-triggered run masks a failed nightly one. So: nightly sweep fails at 00:15, admin clicks "Check imagery" at 09:00 and it succeeds. 4. The "Failures (7d)" column has two independent defects. Both live in
Correctness — the trend charts5. The server and the client disagree about what offset Server side — .toLocalDate.atStartOfDay(OffsetDateTime.now.getOffset)That stamps today's offset onto a Monday up to 156 weeks in the past. Today is PDT ( Client side — The shared symptom is what makes these worth fixing: 6. This isn't hypothetical — in production 7. This repo grows Postgres enums with Observability gaps8. Five of six hand-trigger endpoints bypass the wrapper. Only 9. A throwing details builder is silently discarded. Schema / model mirror10. The Slick FK omits the cascade the schema declares. Quality, reuse and conventionsGrouped, since none of these change behaviour:
Non-findings worth recordingSo these don't get re-litigated later:
🤖 Generated with Claude Code (claude-opus-5[1m]) |
Review findings on this PR clustered on one theme: several paths let the nightly-jobs panel show green over a pipeline that had in fact stopped. Fixing them: A poll that structurally cannot run no longer counts as a success. A GSV or Mapillary city whose API key is missing now fails the run instead of recording `succeeded` with `streets_polled: 0`, so a rotated-out key surfaces rather than quietly ending the #4384 re-audit signal behind an ok badge. A provider with no age query to make (Infra3d) still reports success -- that is a settled configuration, not a fault, and alarming on it forever would be noise. A hand-triggered run can no longer stand in for the scheduled one. `overdue` was already scoped to scheduled runs, but the badge, error, duration and details all came from the latest run of any trigger -- so an admin clicking "run it now" the morning after a failed nightly run replaced that failure with a green ok. The DAO now reads the latest run per (job, trigger); the panel's columns describe the schedule and report the last manual run beside them rather than in place of them. The "Failures (7d)" column had two independent defects. Abandoned runs -- still open long past any plausible duration -- landed in the denominator and never the numerator, so a job the JVM is killed inside every night read 0/7. And the count was not trigger-scoped, so five failed debugging clicks read as five failed nights. Both fixed, and both pinned by tests. The trend charts silently dropped rows. The window start stamped *today's* UTC offset onto a Monday up to 156 weeks back, so a window crossing a daylight-saving change began at 23:00 the preceding Sunday and pulled in rows Postgres then bucketed under a week_start the client never generates. The client's own week grid compared UTC midnight against `Date.now()`, which drops the current week for the first hours of local Monday on any host east of UTC. Both fixed, and the status-change headline now totals the rendered series rather than the raw rows, so a future bucket mismatch shows up instead of hiding. Also: `topReportRegions` and `corroboratedOpenStreets` now exclude deleted regions (prod `sidewalk_seattle` has 12, several carrying PanoNotAvailable reports, eligible to rank into a table framed as "most likely to lose streets next"); the Slick FK mirrors the schema's ON DELETE CASCADE that remove_streets.sql relies on; a details builder that throws is logged rather than silently indistinguishable from a job with nothing to report; and the five remaining hand-trigger endpoints -- user stats, funnel stats, street priority, OSM way refresh, clustering -- record a Manual run like checkImagery already did. Quality: shared formatting helpers (escape, number, duration, relative time, table markup) move to AdminShell.js, which loads on every dashboard page, replacing HealthPage's near-duplicate pair of duration formatters, the fourth relative-time format this PR introduced, the three pre-existing copies of `#relativeTime`, and StreetStatusTrend's line-for-line re-copy of the table helper. An unknown job status now renders warn and sorts most-urgent rather than green and least-urgent. The hand-built 45-line Writes becomes a scoped SnakeCase + `Json.writes`; the trend window options, the corroboration threshold and the status palette come from the backend instead of being mirrored in JS; `PanoNotAvailable` comes from its enum; the duplicated imagery-check details builder moves onto `ImageryCheckResult`; `ImageryFreshnessSync` takes its time from the job it runs inside; the six trend reads and three job reads are bound before their for-comprehensions so they run concurrently; and the schedule times left in three doc comments now point at ScheduledJobs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review findings addressedAll ten numbered findings and the quality list are handled in Correctness2 — a poll that structurally cannot run. Split into the two cases, which turn out not to be the same thing. A missing credential on a provider that does support age polling ( 3 — a hand-triggered run masking a failed nightly one. 4 — the Failures (7d) column, both defects. One thing worth flagging: my first cut expressed the staleness flag as a Slick 5 — the 6 — deleted regions. 7 —
On the blast radius: the Health panel already degrades — Observability8 — the five bypassing endpoints. All wrapped as 9 — the silent details builder. Schema10 — the missing cascade. Quality list
Not changed1 — the So this is a merge-order decision rather than a code change, and you're right that it's worth making deliberately instead of discovering it in the conflict. Whoever merges second renumbers to 359 in the same sitting — the loser's file, not a "take one side" conflict resolution. I've checked the other open PRs: #4924 is the only other claimant (#4927 and #4944 top out at 357). The rest of the "non-findings" section I agree with and haven't touched — including the never-run-reads-overdue grace period, which I left as-is. 🤖 Generated with Claude Code (claude-opus-5[1m]) |
A coverage audit before merge turned up five units this PR added or changed with nothing asserting them at all, plus two client-side fixes from the review round that only existed as code. The largest gap was `StreetEdgeIssueTable`: the weekly report series, the by-region ranking and the "Awaiting confirmation" queue had no test of any kind, which meant the deleted-region fix from the last commit was unpinned and so was the rule the whole queue rests on -- corroboration is distinct *accounts*, not repeat visits by one. NoImageryReportsSpec seeds its own regions, streets, users and reports inside a rolled-back transaction and covers all three reads. Confirmed it discriminates: dropping `region.deleted = FALSE` turns exactly the two deleted-region cases red and leaves the other eight green. Seeding needs explicit ids rather than the sequences. The dev DB is restored from a dump that inserts ids without advancing its sequences, so `nextval` returns ids that already exist -- street_edge is at 2172 with its sequence on 4. ScheduledJobsSpec pins the roster, which nothing checked. A job missing from `All` is simply absent from the Health panel, and absent is indistinguishable from healthy, so the invariant is found by reflection rather than by a second hand-written list: adding a `val Foo: ScheduledJob` and forgetting `All` now fails here. It also pins that every name a hand-trigger endpoint records under is on the roster, since a Manual run recorded under an unlisted name is written and still invisible. EnumTypeParitySpec checks the three Scala enums against the Postgres types they back. That pairing was held together by a `NOTE:` comment asking the next person to change both sides; drift surfaces as a NoSuchElementException mid-read, on whichever page happens to read that row first. ImageryPollOutcomeSpec separates a poll with nothing to do from one that could not run -- the distinction the last commit introduced and nothing asserted. It rebuilds the service against a config with the provider's credential removed and requires the Future to fail. Also covers ImageryCheckResult's totals and its recorded shape, which two callers share. NightlyJobStatusSpec now asserts the serialized field names. The panel's JS renders from its own fixtures, so a rename here would surface as blank cells against a live database and nowhere else. On the frontend, HealthPage and StreetStatusTrend had no tests at all, which left the client half of the timezone fix and the badge-tone fix unpinned. Three jsdom suites cover the shared AdminShell formatters, the nightly-jobs panel (unknown status tones and sorts as a reason to look; a hand-triggered run sits beside the schedule's record; an empty panel reads as a failed read) and the trend section (the week grid, the headline totalling only what it can draw, the threshold read from the payload, escaping). Reverting the four fixes turns exactly the four corresponding cases red. Two of the grid's edges only exist away from UTC, and `process.env.TZ` set inside a test is ignored -- V8 caches the zone per context and jest's jsdom context is already built by then, so the case passes for the wrong reason. test/js/support/timeZoneJsdomEnvironment.js sets it before that context exists, letting a file declare its timezone; streetStatusTrendWeekGrid.test.js pins itself to Sydney and asserts the environment really is east of UTC before relying on it. `#weekStarts` now reads the clock via `Date.now()` rather than `new Date()`, so "now" enters the method in one place and the grid's edges are reachable. All five new specs are enrolled in the ci.yml allowlist, since a spec absent from it is dead weight while CI stays green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage audit before mergeWent through every unit this PR adds or changes and checked what actually asserts it. Five had nothing at all, and two of the fixes from the last round existed only as code. All closed in What had no coverage
The
Every fix is mutation-checkedA test that passes with and without the fix is decoration, so I reverted each fix in a throwaway copy and confirmed the intended cases — and only those — go red:
Two things worth flaggingA latent bug surfaced. The timezone case in the review's finding #5 could not be tested the obvious way: Seeding needs explicit ids. The dev DB is restored from a dump that inserts ids without advancing its sequences — Known gaps I did not closeStating these rather than implying the surface is complete:
All five new specs are enrolled in the Local: 96 backend tests across the 14 specs touching this PR, 969 jsdom tests (up from 918). CI: 972 jsdom tests, all 8 jobs green. 🤖 Generated with Claude Code (claude-opus-5[1m]) |
The stack was branched before #4932's second review pass, which replaced `last_triggered_by` with `last_manual_run_at`/`last_manual_status` and moved the dashboard's formatters onto AdminShell. The pipeline panel read the field that no longer exists and carried its own copy of the job-row formatters — including the `unknown status -> good` default that pass fixed on the Health panel. The three job-row formatters now live on AdminShell, so both panels reach the same verdict about the same `nightly_jobs` entry, and the Imagery page's escaping, number formatting, and table markup come from there too rather than from three private re-implementations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Develop claimed 358-361 while this PR was in flight (#4932's transition logs took 358). The repair evolution moves to 362 unchanged; conflicts were both-sides method additions in StreetEdgeIssueTable and a comment in ExploreNoImageryRateLimitSpec, resolved by keeping both/develop's. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Closes #4928.
Every imagery/street-status surface we have is a snapshot:
pano_data.expiredsays what is expired now,street_edge.statussays what is retired now, and a nightly job that silently stopped looks exactly like anightly job that found nothing to do. This PR records the transitions, so "what newly expired this week",
"which streets were retired this month, by what", and "did last night's sweep run" become answerable — and gives
them a first read side in
/admin.Schema (evolution 358)
pano_data.expired_at— when the pano's imagery went away, as distinct fromlast_checked("when did welast look", bumped by every sweep). Stamped by the sweep only on the false→true edge; cleared when a re-check or
a view un-expires the pano.
CHECK (expired OR expired_at IS NULL).audit_task.outdated_imagery_at— same pattern for the With new coverage maps, an important question is: what areas need to be re-audited because new streetscape imagery has landed #4384 re-audit flag (decided with Jon on thisissue's thread): the sync's set-pass stamps it, the clear-pass nulls it, and because the set-pass only touches
unflagged rows the stamp survives re-runs — flag→re-audit latency and "newly flagged since X" read straight off
the column.
street_edge_status_change— old/new status +sourceenum per transition.street_edge.statushas no appwrite path at all (only the hand-run
db/scriptswriters), so this table is the only trace those script runsleave. A
CHECK (old_status <> new_status)keeps re-runs of the same CSV from faking a spike.background_job_run— one row per nightly-actor run (started/finished, status, summary), written by a newJobRunService.recordwrapper around all nine scheduled actors.ScheduledJobs.scalanow declares the nightlyroster once — each actor reads its schedule from it, and the Health panel uses it so a job that has never run
still shows.
Read side
/admin/street-status: "Recently changed" trend (via/adminapi/streetStatusTrend) + an "Awaitingconfirmation" queue of still-open streets with ≥2 distinct no-imagery reporters
(
StreetLifecycleService.MinCorroboratingReporters) — the evidence queue for No-imagery reports should not mark streets audited; resolve real gaps via verification #4922's offline checker; decidingto flip them stays No-imagery reports should not mark streets audited; resolve real gaps via verification #4922's call.
/admin/health: "Nightly jobs" panel overbackground_job_run+ theScheduledJobsroster.checkForImagery→ImageryCheckResultandpollImageryAges→PollResult, so the With new coverage maps, an important question is: what areas need to be re-audited because new streetscape imagery has landed #4384 pipeline's countspersist instead of dying in a log line — this unblocks Admin: imagery-freshness panel — street priority visualization + poll-health stats #4908's poll-health section.
Review callouts
makes Play silently skip the file, so neither can pre-emptively take 359). Whichever merges second renumbers —
same protocol as the 344→348→356 renumbers on Re-route labelers to streets with new imagery; completion = up-to-date (#4384) #4649.
hide-streets-without-imagery.sh,reveal-or-hide-neighborhoods.sh,remove_streets.sql) now insertstreet_edge_status_changerows; verified idempotent by runninghide-streets-without-imagery.shtwice (second run inserts 0 rows).(
OutdatedImageryFlagSyncSpec,OutdatedImageryRoutingSpec,UpToDateCoverageSpec) that were missing from thetestOnlyallowlist — the CI-allowlist dead-weight trap from Stop transient imagery failures from marking streets audited #4923, again.Verified
Test/compilewarning-clean; scalafmt clean; evolutions lint clean.the new
outdated_imagery_atedge-only stamping case (sentinel-based, so a re-stamp can't pass).Filed while building this: #4929 (nothing reopens a
no_imagerystreet that regains imagery) and #4930 (admintype renders at ~62.5% of intended size).
🤖 Generated with Claude Code (claude-fable-5)