Say when media bytes go missing, and show it on /admin/health (#4926) - #4944
jonfroehlich wants to merge 10 commits into
Conversation
A media row whose file is gone answers every request with a bare 404, indistinguishable from an id that never existed. That is why #4925 — a story photo deleted by a deploy — went six days without anyone noticing, and it was found by a human opening the story, not by us. Request-time: /cropImage and /backupImage now say so. Both endpoints are only ever reached through a signed URL, and PanoDataService only signs one for a file it just saw on disk, so a miss means the bytes vanished inside the signature's ~75-minute life. Panos are the error tier (the store holds the only copies of GSV imagery Google expired), crops the warning tier (re-cuttable). Share previews get nothing: a missing one is the normal cold-cache case and rebuilds itself. The dedup behind those lines moves into LostMediaLog, shared with StoryController. Its unbounded set was fine for a handful of story rows and would not have been for pano ids — a dead mount reports every pano at once — so the tracking set is now bounded. Dashboard: a Media storage panel on /admin/health, covering every city on the stage rather than only the instance being viewed. It shows where each persistent directory resolves and what the boot check makes of it, then counts story_media rows with no file (destroyed content) and files with no row (a retraction whose file delete didn't land, against #4054's hard-delete contract). One directory listing per city serves any row count, and the schema and id reads are single UNION ALL queries — a per-city fan-out is the ~50-connection flood this dashboard exists to catch. Two rules keep it from crying wolf, since an ignored monitor leaves us where #4925 found us: an unreadable base directory reports the scan unavailable rather than every row lost, and a dev checkout is not scolded for the relative defaults landing where they are meant to. Filesystem work runs on a new blocking-io dispatcher, so a stat against a dead mount can only park a thread nothing else uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found QAing the panel against a dev container where SIDEWALK_CITY_ID and DATABASE_USER disagree — the state CLAUDE.md warns about, and one the panel handled badly. StoryService builds its write path from city-id, so the photo landed under seattle-wa/ while its row sat in sidewalk_teaneck; the scan, deriving the directory from the schema, looked under teaneck-nj/ and called a file that was right there lost. The whole point of the panel is that people believe it when it says data is gone, so it has to look where the writer actually writes. So this instance's own schema, read from current_schema() rather than inferred from config, takes its directory from city-id. That claim is exclusive: without it, the schema the config maps to that same city id listed the same directory and reported every one of those files as an orphan instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deep code reviewAll 17 files read. The design is sound — sourcing the panel from Six findings. 1. An unreadable directory is reported as total data loss — the exact cry-wolf failure the design rules out
The base-dir guard doesn't save us, because it uses
— is not implemented. A base directory the process can't read sails past Fix: 2. A hung mount permanently kills the
|
Two of the six defeated the panel in the conditions it exists for. An unreadable directory read as total data loss. File.list answers null both for a directory that isn't there and for one the process may not read, and the scan collapsed them into "every row missing" — while the base-dir guard used isDirectory, which is true for an unreadable directory. So a permissions change on the media base would have put every story photo on the stage on the panel as destroyed, which is the one thing a monitor like this must never do. Listing now reports Absent and Unreadable apart, an unreadable city directory reports unscanned instead of lost, and the base-dir guard checks canRead. A hung mount permanently killed the blocking-io pool. The five-second timeout abandons the future but cannot cancel the thread under it, and the dashboard polls every ~20s, so four stuck scans parked all four threads for good — the panel stayed dead even after the mount came back. One scan at a time now, gated on the underlying scan rather than on the timeout, so a stuck mount costs one thread and releases it when it unsticks. The rest: the "Missing media files" KPI reported a count of unsafe directories when the scan was unavailable, so it now only ever shows a missing-file count and lets a bad directory color the tile without supplying its number; an unscanned city said "no city configured for schema X" even when a city was configured and this instance had simply claimed its directory, so the reason travels from the backend that knows which case applies; and the arming rule lives once, on the boot check, rather than being recomputed by the panel. Tests: LostMediaLogSpec covers the dedup, the kind/id key, the error-vs-warn tiering and eviction by asserting the log lines themselves, since both ways that class fails are silent. MediaIntegritySpec covers the unreadable branches, and the directory status rules move behind a pure seam so the permission branches are reachable from a suite that runs as root. Both are in ci.yml's testOnly list, or they run nowhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All six fixed in bbab1c3. 1 — unreadable directory read as loss. 2 — hung mount killed the pool. A single in-flight gate, cleared on the underlying scan rather than on the timeout, so a stuck mount parks one thread and releases it when it unsticks instead of stacking a fresh scan every poll until all four are gone. When the gate is shut the panel says so rather than rendering a stale all-clear. 3 — KPI. It now only ever shows a missing-file count or 4 — unscanned reason. 5 — tests. New 6 — arming rule. Now Verified locally: 🤖 Generated with Claude Code (claude-opus-5[1m]) |
Its directory rows rendered 193-208px tall against ~32px for every
other table on the page. Each one carried the boot check's full
wipe-zone sentence under the status badge, in the narrowest column —
and that sentence names the config key and the resolved path, both of
which are already their own columns, so four rows repeated the same
explanation four times to say nothing the row didn't already say. The
badge states the status; the fix ("point its environment variable at
storage outside the application") is said once in the note below.
Row-specific details that aren't the generic one — unresolved, not
readable, not writable — still show, and they fit on a line.
The "Holds" column read "content" or "rebuildable", which doesn't
answer anything: every directory holds content. It now asks the
question that matters, "If lost", and answers "gone for good" or
"rebuildable". Same fix for "Ids" over the story table, which shows a
sentence rather than ids whenever a city couldn't be scanned; it's
"Notes" now.
The intro said in 425 characters what it says here in 290.
Left the directories table the only one on the page wide enough to
scroll sideways, which clipped Status — the column being read. Long
paths now wrap, scoped to the path cell so the config keys and
variable names beside them stay whole.
Measured in headless Chromium at 1440px against the running app: rows
34-46px (the connections table is 35px), section 1281px -> 641px, and
horizontal overflow 0 across all four health tables.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three things this PR added could be deleted with CI still green, and each one is silent when it breaks -- which is the failure mode the whole PR exists to end. Nothing asserted that ImageController reports a loss at all. The 404 a signed crop or pano URL answers when its bytes are gone is indistinguishable from an id that never existed, so the log line is the only signal, and the old cases checked only the status code. ImageControllerSpec now pins the line, its tier, and the path it names -- and pins silence on a crop that is present, using a second label id so dedup can't supply that silence. HealthPage.js had no tests. Its rules decide whether an operator believes the panel: an unscanned city must not render as zero losses, and the KPI must never call an unknown healthy. healthMediaPanel.test.js drives the real load path through jsdom for 19 cases, including the escaping of server-supplied reasons. The field names joining the two are unpinned in both directions: rename a case class field and the writer emits a different key, every value the page reads goes undefined, and a monitor reporting nothing looks exactly like a monitor reporting nothing wrong. HealthMediaPayloadSpec asserts the key sets the page consumes, including that absent Options stay absent. Two seams make the rest reachable. The base-directory guard moves to MediaIntegrity.scanRefusal, so the isDirectory-is-true-for-unreadable trap is covered by the same spec as its per-directory twin rather than only from a booted app. The in-flight guard becomes SingleFlightGate, whose contract -- the gate opens on the work, never on a caller giving up -- is the part a deadline alone doesn't give and the part a spec can drive with promises. HealthServiceSpec's media cases would have passed against a checkout with no media directory, which is CI. It now owns its directory, seeds a file with no row in it, and requires that file to be attributed to this instance's own schema: the assertion fails if the scan resolves the directory from the schema instead of city-id, which is the bug live QA caught. ImageControllerSpec ran nowhere in CI; it and the two new pure specs join the gating list. 92 Scala tests across 8 suites and 837 jsdom tests green; scalafmt and eslint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
serveStoryMedia skips reporting a media row younger than a minute, because StoryService commits the row before the file move lands and a healthy upload looks exactly like a destroyed one for that moment. Nothing covered the rule, and it is not free to get wrong in either direction: reporting is once per media id, so a false alarm spends the single line that id will ever get, and a window too generous is a stretch in which real loss passes unannounced. The predicate moves to StoryController.withinUploadWindow, next to ListingMax and following StoryServiceImpl.secondsUntilFree's precedent, so the two boundaries can be pinned against a fixed clock rather than a live one. StoryUploadWindowSpec joins the CI gating list. 129 tests across 11 suites green, including StoryControllerSpec and StoryServiceSpec. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two columns were asking the reader to do the interpreting. "If lost" gave no clue what the loss was of, and answered in a warn-toned badge: every irreplaceable directory carried an amber "gone for good" that sat right beside the live Status badge and read as a second alarm on a row where nothing was wrong. It is now "Recoverable?", answered Yes or No first in muted text, so it can't be mistaken for a condition. "Status" on a dev checkout read "inside the build tree (dev)" -- a location, leaving the reader to work out whether that was a problem. It now leads with the verdict: "ok for dev (inside the build tree)". Resolves-to moves up beside the variable that sets it, so the row reads as identity (key, var, path) then judgment (recoverable, status). Three jsdom cases pin the new shape, including that recoverability is never badged. 51 Scala tests and 22 jsdom tests green; verified live on :9000. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A crop is a screenshot of the pano canvas taken in the labeler's browser as the label was placed, and nothing in this app regenerates one. The Street View Static still used as a fallback elsewhere is a different, smaller image of a pano the provider must still serve — and roughly half the labels on prod sit on panos already marked expired. So the guardrails move the crop directory to the fatal tier alongside the pano and story-media directories: a stage that would place it inside the tree `sbt clean stage` deletes refuses to boot, and a signed crop URL whose file has vanished logs at ERROR. Cached share previews are now the only entry that still rebuilds on demand. Every deployed stage already points SIDEWALK_IMAGES_DIR outside the build tree, and CI's e2e-smoke job exports it, so no stage newly fails the check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5ee439a to
7ff1cdf
Compare
jonfroehlich
changed the title
jonfroehlich
changed the title
|
Retargeted from Dropped the 🤖 Generated with Claude Code (claude-opus-5[1m]) |
Three conflicts, all "both sides added a sibling panel" rather than disagreements — #4928's nightly-jobs panel landed beside this branch's media-storage panel, so both are kept: - HealthService: both imports, both DbHealthData fields, both injected dependencies, both futures in getDbHealth, both Writes. - HealthPage.js: both render calls and both panel sections. develop moved esc/num/nil/setHtml off HealthPage onto AdminShell, so the media panel's calls follow them; healthMediaPanel.test.js now eval's AdminShell ahead of HealthPage the way healthNightlyJobs.test.js does. - ci.yml: the gating testOnly allowlist, union of both sides. HealthMediaPayloadSpec builds a whole DbHealthData/HealthThresholds, so it takes develop's two new threshold values and nightlyJobs. Test / compile clean under -Xfatal-warnings, scalafmt clean, 70 pure specs and 994 jsdom tests green, eslint/htmlhint 0 errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Closes #4926.
Stacked on #4927 (base is
4925-story-media-deploy-wipe, notdevelop) — it needsservice.MediaDirsandPersistentMediaDirCheck.persistentDirsfrom that PR. Review only the two commits above the base; retarget thisPR onto
developbefore #4927 merges, or GitHub will close it when the base branch is deleted.Items (1), (4) and (5) of the issue already shipped inside #4927 (
PersistentMediaDirCheck, its spec, and the"Directories that must survive a deploy" docs). What's left is item (2), extended past story media, and item (3).
Why
A media row whose file is gone answers every request with a bare
404, indistinguishable from an id that neverexisted. That is why #4925 — a story photo deleted by a deploy — went six days without anyone noticing, and why it
was found by a human opening the story rather than by us.
What's here
Request-time (
LostMediaLog)./cropImageand/backupImagenow say when the bytes are gone. Both are onlyever reached through a signed URL, and
PanoDataServiceonly signs one for a file it just saw on disk, so a missmeans the bytes vanished inside the signature's ~75-minute life — no DB read needed to know it is real loss. Panos
and crops are both the ERROR tier — nothing can re-fetch imagery the provider no longer serves, and nothing
regenerates a crop, which is a canvas screenshot the labeler's browser took as the label was placed. Share previews
get nothing — a miss there is the normal cold-cache case and rebuilds itself.
StoryController's dedup set moves into the sharedLostMediaLog. The oldConcurrentHashMap.newKeySetwas finefor a handful of story ids and would not have been for pano ids — a dead mount reports every pano in a city at
once — so the tracking set is now a bounded LRU.
Dashboard. A Media storage panel on
/admin/health:persistentDirs/unsafeDirsso the page and the check can't disagree.story_mediascan reporting missing (rows with no file → destroyed content, Story photos are permanently deleted by every prod deploy (media dir resolves inside target/) #4925) andorphans (files with no row → a retraction whose file delete didn't land, against Sharing users' lived experiences #4054's hard-delete
contract). Counts and sample ids, because a count comparison calls a city with one lost photo and one orphaned
file clean.
It covers every city on the stage, not just the instance being viewed. One
dir.list()per city serves any rowcount; schema discovery, counts and ids are three
UNION ALLqueries — a per-city fan-out is exactly the~50-connection flood this dashboard exists to catch (#4559).
Two rules keep it from crying wolf, since an ignored monitor leaves us where #4925 found us: an unreadable base
directory reports the scan unavailable rather than every row lost, and a dev checkout is not scolded for the
relative defaults landing where they are meant to (
enforced: falseoutsideMode.Prod). Filesystem work runs ona new
blocking-iodispatcher, so a stat against a dead mount can only park a thread nothing else uses.The current-city fix (second commit). Found QAing against a dev container where
SIDEWALK_CITY_ID=seattle-waand
DATABASE_USER=sidewalk_teaneckdisagree — the state CLAUDE.md warns about.StoryServicebuilds its writepath from
city-id, so the photo landed underseattle-wa/while its row sat insidewalk_teaneck, and the firstcut — deriving the directory from the schema — called a file that was right there lost. So this instance's own
schema, read from
current_schema()rather than inferred from config, takes its directory fromcity-id, and thatclaim is exclusive (otherwise the schema config maps to that same city id lists the same directory and reports
every file in it as an orphan). Generalizable: a monitor over a write path has to resolve paths exactly the way
the writer does — the same principle as
MediaDirsitself.Testing
test/service/MediaIntegritySpec.scala(added to ci.yml'stestOnlyallowlist, or it runs nowhere).-Xfatal-warnings; scalafmt, ESLint and HTMLHint clean.defaults; deleting
story_331.jpgproduced exactly oneERROR s.LostMediaLog - story_media 331 has no file on disk at …across three requests (dedup working), with abare 404 each time.
Not yet verified: the rendered panel HTML and the
missingcount end-to-end — port :9000 was in use by anotherworktree during QA. Recipe is in the issue thread.
🤖 Generated with Claude Code (claude-opus-5[1m])