From cb0d249e812ea10f8901216e638dcf09d9cecb0a Mon Sep 17 00:00:00 2001 From: jonfroehlich Date: Wed, 19 Aug 2026 16:13:46 -0700 Subject: [PATCH 1/8] Say when media bytes go missing, and show it on /admin/health (#4926) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .github/workflows/ci.yml | 5 +- app/controllers/ImageController.scala | 15 +- app/controllers/StoryController.scala | 17 +- app/executors/CustomExecutionContexts.scala | 14 ++ app/models/utils/HealthTable.scala | 57 +++++ app/modules/ExecutorsModule.scala | 1 + app/service/HealthService.scala | 228 ++++++++++++++++++-- app/service/LostMediaLog.scala | 56 +++++ app/service/MediaIntegrity.scala | 117 ++++++++++ app/service/PanoDataService.scala | 6 +- app/views/admin/dashboard/health.scala.html | 9 + conf/application.conf | 13 ++ docs/deployment-and-stages.md | 4 +- public/js/admin-dashboard/HealthPage.js | 110 ++++++++++ test/controllers/ImageControllerSpec.scala | 33 ++- test/service/HealthServiceSpec.scala | 33 +++ test/service/MediaIntegritySpec.scala | 171 +++++++++++++++ 17 files changed, 858 insertions(+), 31 deletions(-) create mode 100644 app/service/LostMediaLog.scala create mode 100644 app/service/MediaIntegrity.scala create mode 100644 test/service/MediaIntegritySpec.scala diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4cd9502b16..9f3b449df9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -304,8 +304,11 @@ jobs: # - PersistentMediaDirCheckSpec (#4925): user-uploaded media must resolve outside the application directory, # which a deploy rebuilds and deletes. Pure logic, no DB — and the failure it guards against is invisible to # every other test, since the broken config behaves correctly right up until the next release. + # - MediaIntegritySpec (#4926): the media-storage panel's calls on what counts as data loss. Pure logic, no DB. + # Its false-alarm cases matter as much as its real ones — a monitor that cries loss over an absent directory + # gets ignored, and an ignored monitor leaves us exactly where #4925 found us. - name: Run gating/auth tests (health dashboard + route auth posture + geodesic distances) - run: sbt 'set Test / parallelExecution := false' 'testOnly controllers.HealthDashboardSpec service.HealthServiceSpec controllers.RouteAuthPostureSpec models.street.GeodesicDistanceSpec service.ExploreTutorialRouteSpec controllers.MobileDetectionSpec service.PanoDataServiceSpec models.utils.ConfigTableVoidedArchiveSpec controllers.api.StatsApiSpec controllers.ExploreSubmissionSpec controllers.ValidateSubmissionSpec modules.PersistentMediaDirCheckSpec' + run: sbt 'set Test / parallelExecution := false' 'testOnly controllers.HealthDashboardSpec service.HealthServiceSpec controllers.RouteAuthPostureSpec models.street.GeodesicDistanceSpec service.ExploreTutorialRouteSpec controllers.MobileDetectionSpec service.PanoDataServiceSpec models.utils.ConfigTableVoidedArchiveSpec controllers.api.StatsApiSpec controllers.ExploreSubmissionSpec controllers.ValidateSubmissionSpec modules.PersistentMediaDirCheckSpec service.MediaIntegritySpec' env: DATABASE_URL: jdbc:postgresql://localhost:5432/sidewalk DATABASE_USER: sidewalk diff --git a/app/controllers/ImageController.scala b/app/controllers/ImageController.scala index af7e0e43db..cde507c988 100644 --- a/app/controllers/ImageController.scala +++ b/app/controllers/ImageController.scala @@ -8,7 +8,7 @@ import models.label.LabelTypeEnum import play.api.libs.json._ import play.api.mvc.{AnyContent, Request, RequestHeader} import play.api.{Configuration, Logger} -import service.ImageSigningService +import service.{ImageSigningService, LostMediaLog} import java.awt.Image import java.awt.image.BufferedImage @@ -25,6 +25,7 @@ class ImageController @Inject() ( signingService: ImageSigningService, shareImageCache: service.ShareImageCache, config: Configuration, + lostMediaLog: LostMediaLog, cpuEc: CpuIntensiveExecutionContext )(implicit ec: ExecutionContext) extends CustomBaseController(cc) { @@ -137,6 +138,15 @@ class ImageController @Inject() ( val contentType = if (file.getName.toLowerCase.endsWith(".png")) "image/png" else "image/jpeg" Future.successful(Ok.sendFile(file, inline = true).as(contentType)) case None => + // Reaching here means the file was on disk when this URL was signed (backupImageUrl and + // getBackupImageMetadata both check first) and is gone within the signature's ~75-minute life. For an + // expired pano this store holds the only copy left anywhere, so say so (#4926) — the 404 stays bare. + lostMediaLog.reportMissing( + "pano", + panoId, + s"${panoDataService.backupImageDir(panoId).getAbsolutePath}/$panoId.{jpg,jpeg,png}", + irreplaceable = true + ) Future.successful(NotFound(s"Pano image not found: $panoId")) } } @@ -188,6 +198,9 @@ class ImageController @Inject() ( if (file.exists()) { Future.successful(Ok.sendFile(file, inline = true).as("image/png")) } else { + // Same signed-URL reasoning as serveBackupImage above: cropUrl only signs a crop it just saw on disk, so a + // miss here is a file that vanished. A crop can be re-cut from pano imagery, so this is the warning tier. + lostMediaLog.reportMissing("crop", s"$labelType/$labelId", file.getAbsolutePath, irreplaceable = false) Future.successful(NotFound("Crop image not found")) } } diff --git a/app/controllers/StoryController.scala b/app/controllers/StoryController.scala index 4a0bcdec62..5abdf9af18 100644 --- a/app/controllers/StoryController.scala +++ b/app/controllers/StoryController.scala @@ -9,11 +9,10 @@ import models.story.{Story, StoryMedia, StoryPhotoUpload, StoryRejection} import play.api.libs.json.{JsBoolean, Json} import play.api.{Configuration, Logger} import play.silhouette.api.Silhouette -import service.{ConfigService, ImageSigningService, RateLimiter, StoryService} +import service.{ConfigService, ImageSigningService, LostMediaLog, RateLimiter, StoryService} import java.io.File import java.time.{Duration, OffsetDateTime} -import java.util.concurrent.ConcurrentHashMap import javax.inject.{Inject, Singleton} import scala.concurrent.{ExecutionContext, Future} import scala.util.Try @@ -33,6 +32,7 @@ class StoryController @Inject() ( storyService: StoryService, signingService: ImageSigningService, rateLimiter: RateLimiter, + lostMediaLog: LostMediaLog, implicit val ec: ExecutionContext ) extends CustomBaseController(cc) { private val logger = Logger(this.getClass) @@ -228,27 +228,22 @@ class StoryController @Inject() ( } } - // serveStoryMedia's data-loss tripwire (#4925): one entry per media id already reported, so a busy page - // re-requesting one lost file reads as one loss in the log, not hundreds. - private val lostMediaLogged = ConcurrentHashMap.newKeySet[Int]() - // The upload flow commits the media row before the file move lands (StoryService's place-before-commit windows), so // a row this young with no bytes is almost certainly mid-upload, not loss. The window is sub-second; a minute is // generous slack. private val lostMediaGrace = Duration.ofMinutes(1) /** - * Logs a media row whose bytes are missing from disk. This error is a post-#4925 tripwire someone is expected to - * investigate, so it is kept high-signal: skipped inside the upload grace window (a false alarm has real cost) and - * logged once per media id per instance. + * Reports a media row whose bytes are missing from disk (#4925). Kept high-signal by skipping the upload grace + * window, since a false alarm on this tripwire has real cost; `LostMediaLog` handles the rest. * * @param media The media row whose file is missing. * @param file Where the bytes should have been. */ private def logLostMedia(media: StoryMedia, file: File): Unit = { val inUploadWindow = media.createdAt.isAfter(OffsetDateTime.now.minus(lostMediaGrace)) - if (!inUploadWindow && lostMediaLogged.add(media.storyMediaId)) { - logger.error(s"story_media ${media.storyMediaId} has no file on disk at ${file.getAbsolutePath}") + if (!inUploadWindow) { + lostMediaLog.reportMissing("story_media", media.storyMediaId.toString, file.getAbsolutePath, irreplaceable = true) } } diff --git a/app/executors/CustomExecutionContexts.scala b/app/executors/CustomExecutionContexts.scala index c4a2bd6336..fd858d7d74 100644 --- a/app/executors/CustomExecutionContexts.scala +++ b/app/executors/CustomExecutionContexts.scala @@ -27,3 +27,17 @@ object CpuIntensiveExecutionContext { extends CustomExecutionContext(system, "cpu-intensive") with CpuIntensiveExecutionContext } + +/** + * Execution context for blocking filesystem operations, isolated so that a call that never returns — a stat or a + * directory listing against an unreachable network mount — can only exhaust this pool and not the ones serving + * requests or running streams. + */ +trait BlockingIoExecutionContext extends ExecutionContext + +object BlockingIoExecutionContext { + @Singleton + class PekkoBased @Inject() (system: ActorSystem) + extends CustomExecutionContext(system, "blocking-io") + with BlockingIoExecutionContext +} diff --git a/app/models/utils/HealthTable.scala b/app/models/utils/HealthTable.scala index a9b443c052..c96be8d0d7 100644 --- a/app/models/utils/HealthTable.scala +++ b/app/models/utils/HealthTable.scala @@ -54,6 +54,12 @@ class HealthTable @Inject() (protected val dbConfigProvider: DatabaseConfigProvi implicit private val grDbEnvInfo: GetResult[DbEnvInfo] = GetResult(r => DbEnvInfo(r.nextString(), r.nextString(), r.nextBoolean())) + implicit private val grSchemaRowCount: GetResult[SchemaRowCount] = + GetResult(r => SchemaRowCount(r.nextString(), r.nextInt())) + + implicit private val grSchemaMediaId: GetResult[SchemaMediaId] = + GetResult(r => SchemaMediaId(r.nextString(), r.nextInt())) + /** * Caps a health read so it can never hold a pool connection for long. A monitoring query must not add load — least * of all when the database is already stressed, which is exactly when this dashboard gets opened. `statement_timeout` @@ -275,4 +281,55 @@ class HealthTable @Inject() (protected val dbConfigProvider: DatabaseConfigProvi LEFT JOIN pano_data pd ON pd.pano_id = labeled.pano_id """.as[PanoBackupStats].head } + + /** + * Schemas holding a readable `story_media` table, for the cross-city media-integrity scan (#4926). + * + * Same shape as [[getEvolutionSchemas]], including its trap: the table-privilege check takes the `pg_class` oid + * rather than a name, because the planner may evaluate the predicates in any order and the name-based form raises + * an error (instead of returning false) on a relation that the other filters would have excluded. + */ + def getStoryMediaSchemas: DBIO[Seq[String]] = bounded { + sql"""SELECT nspname FROM pg_catalog.pg_class + JOIN pg_catalog.pg_namespace ON pg_namespace.oid = pg_class.relnamespace + WHERE relname = 'story_media' AND relkind = 'r' + AND has_schema_privilege(nspname, 'USAGE') + AND has_table_privilege(pg_class.oid, 'SELECT') + ORDER BY nspname""".as[String] + } + + /** + * How many `story_media` rows each schema holds, in ONE query — the same `UNION ALL` construction and the same + * reason as [[getStuckEvolutionsForSchemas]]: a per-schema fan-out would want ~50 pool connections per poll on + * prod. Read before the ids so the scan can decline an implausibly large result rather than pulling it into heap. + * + * Every `schema` MUST already be validated as a bare identifier by the caller; the names are spliced literally. + * + * @param schemas Validated schema names to count. Must be non-empty. + * @return One row per schema, including the schemas that hold no media at all. + */ + def getStoryMediaCounts(schemas: Seq[String]): DBIO[Seq[SchemaRowCount]] = { + require(schemas.nonEmpty, "getStoryMediaCounts requires at least one schema (empty input builds invalid SQL)") + val union = schemas + .map(schema => s"""SELECT text '$schema' AS schema, count(*)::int AS media_rows FROM "$schema".story_media""") + .mkString("\nUNION ALL\n") + bounded(sql"""#$union""".as[SchemaRowCount]) + } + + /** + * Every `story_media` id in the given schemas, in ONE `UNION ALL` query. The ids themselves are needed — a count + * comparison would call a city with one lost photo and one orphaned file clean. + * + * Every `schema` MUST already be validated as a bare identifier by the caller; the names are spliced literally. + * + * @param schemas Validated schema names known to hold at least one row. Must be non-empty. + * @return One row per media id, tagged with its schema. + */ + def getStoryMediaIds(schemas: Seq[String]): DBIO[Seq[SchemaMediaId]] = { + require(schemas.nonEmpty, "getStoryMediaIds requires at least one schema (empty input builds invalid SQL)") + val union = schemas + .map(schema => s"""SELECT text '$schema' AS schema, story_media_id FROM "$schema".story_media""") + .mkString("\nUNION ALL\n") + bounded(sql"""#$union""".as[SchemaMediaId]) + } } diff --git a/app/modules/ExecutorsModule.scala b/app/modules/ExecutorsModule.scala index 632962820b..586caa9e27 100644 --- a/app/modules/ExecutorsModule.scala +++ b/app/modules/ExecutorsModule.scala @@ -9,5 +9,6 @@ import executors._ class ExecutorsModule extends AbstractModule { override def configure(): Unit = { bind(classOf[CpuIntensiveExecutionContext]).to(classOf[CpuIntensiveExecutionContext.PekkoBased]).asEagerSingleton() + bind(classOf[BlockingIoExecutionContext]).to(classOf[BlockingIoExecutionContext.PekkoBased]).asEagerSingleton() } } diff --git a/app/service/HealthService.scala b/app/service/HealthService.scala index 650d3351ab..f01f75ac4b 100644 --- a/app/service/HealthService.scala +++ b/app/service/HealthService.scala @@ -1,16 +1,20 @@ package service import com.google.inject.ImplementedBy +import executors.BlockingIoExecutionContext import models.utils.HealthTable +import org.apache.pekko.actor.ActorSystem +import org.apache.pekko.pattern.after import play.api.cache.AsyncCacheApi import play.api.db.slick.{DatabaseConfigProvider, HasDatabaseConfigProvider} import play.api.libs.json._ -import play.api.{Configuration, Logger} +import play.api.{Configuration, Environment, Logger} import models.utils.MyPostgresProfile +import java.io.File import java.time.OffsetDateTime import javax.inject._ -import scala.concurrent.duration.Duration +import scala.concurrent.duration.{Duration, DurationInt, FiniteDuration} import scala.concurrent.{ExecutionContext, Future} /** A session that currently blocks one or more other sessions from acquiring a lock. */ @@ -84,6 +88,87 @@ case class PanoBackupStats( /** The connecting role's environment: database, role, and whether it can read every session's statement text. */ case class DbEnvInfo(database: String, role: String, canSeeAllQueries: Boolean) +/** How many `story_media` rows one schema holds. Internal to the media scan, not part of the dashboard payload. */ +case class SchemaRowCount(schema: String, rows: Int) + +/** One `story_media` id, tagged with its schema. Internal to the media scan, not part of the dashboard payload. */ +case class SchemaMediaId(schema: String, storyMediaId: Int) + +/** + * One persistent media directory as this instance resolves it. + * + * `label` and `severity` are computed server-side so the page holds no copy of the rules that decide when a + * directory is a problem — the same reason [[HealthThresholds]] travels in the payload. + * + * @param key Config key naming the directory. + * @param envVar Environment variable a deployment sets it with. + * @param irreplaceable Whether its contents are content rather than a rebuildable cache. + * @param path Where it resolves on this instance. + * @param status Machine-readable state: `ok`, `absent`, `not_writable`, `unsafe`, `unresolved`. + * @param label Display text for that state. + * @param severity Badge tone: `good`, `ok`, `warn`, `bad`. + * @param detail Longer explanation when there is one to give. + */ +case class MediaDirStatus( + key: String, + envVar: String, + irreplaceable: Boolean, + path: String, + status: String, + label: String, + severity: String, + detail: Option[String] +) + +/** + * One city's `story_media` rows measured against the files in its media directory. + * + * @param cityId City the schema belongs to; None when this instance's config doesn't name it. + * @param schema Database schema the rows came from. + * @param rows `story_media` rows in that schema. + * @param missing Rows whose bytes are gone — data loss (#4925). + * @param orphans Files with no row — a retraction whose file delete didn't land (#4054). + * @param missingIds Sample of the missing ids, to start looking from. + * @param orphanIds Sample of the orphaned ids. + * @param scanned False when the directory couldn't be located, so the counts mean nothing. + */ +case class CityStoryMedia( + cityId: Option[String], + schema: String, + rows: Int, + missing: Int, + orphans: Int, + missingIds: Seq[Int], + orphanIds: Seq[Int], + scanned: Boolean +) + +/** + * The story-media integrity scan across every city schema visible from this instance. + * + * @param baseDir The resolved base directory the per-city subdirectories live under. + * @param cities One row per schema holding a `story_media` table, in schema order. + * @param missing Total rows with no file, across every scanned city. + * @param orphans Total files with no row, across every scanned city. + */ +case class StoryMediaIntegrity(baseDir: String, cities: Seq[CityStoryMedia], missing: Int, orphans: Int) + +/** + * The media-storage panel: where this instance keeps persistent media, and whether any of it has gone missing. + * + * @param directories One status per directory the boot check guards. + * @param enforced Whether `PersistentMediaDirCheck` arms in this run mode; false in dev, where the relative + * defaults landing in the checkout is the intended behavior rather than a fault. + * @param storyMedia The integrity scan, or None when it couldn't run. + * @param unavailable Why the scan couldn't run, when it didn't. + */ +case class MediaStorageHealth( + directories: Seq[MediaDirStatus], + enforced: Boolean, + storyMedia: Option[StoryMediaIntegrity], + unavailable: Option[String] +) + /** * Server-owned thresholds the dashboard uses to color each panel, echoed in the payload so the frontend never * hard-codes them (CLAUDE.md: domain values come from the backend). Seconds unless noted. @@ -117,6 +202,7 @@ case class DbHealthData( tableBloat: Seq[TableBloat], connections: Seq[ConnCount], panoBackups: Option[PanoBackupStats], + mediaStorage: Option[MediaStorageHealth], thresholds: HealthThresholds ) @@ -139,8 +225,11 @@ trait HealthService { class HealthServiceImpl @Inject() ( protected val dbConfigProvider: DatabaseConfigProvider, config: Configuration, + environment: Environment, cacheApi: AsyncCacheApi, - healthTable: HealthTable + healthTable: HealthTable, + actorSystem: ActorSystem, + blockingIoEc: BlockingIoExecutionContext )(implicit val ec: ExecutionContext) extends HealthService with HasDatabaseConfigProvider[MyPostgresProfile] { @@ -207,7 +296,8 @@ class HealthServiceImpl @Inject() ( .recover { case e: Exception => logger.warn(s"Health: failed to read pano backup stats: ${e.getMessage}"); None } - val evoF = getStuckEvolutions + val mediaF = getMediaStorage + val evoF = getStuckEvolutions for { env <- envF @@ -218,6 +308,7 @@ class HealthServiceImpl @Inject() ( bloat <- bloatF conn <- connF pano <- panoF + media <- mediaF } yield DbHealthData( generatedAt = OffsetDateTime.now().toString, currentDatabase = env.database, @@ -230,6 +321,7 @@ class HealthServiceImpl @Inject() ( tableBloat = bloat, connections = conn, panoBackups = pano, + mediaStorage = media, thresholds = thresholds ) } @@ -256,6 +348,112 @@ class HealthServiceImpl @Inject() ( } } + // Ceiling on `story_media` rows the scan will pull into memory. Prod holds a single-digit number today; this is a + // guard against a future where stories take off, not a working limit. Exceeding it reports the scan as unavailable + // rather than silently comparing a truncated set, which would invent orphans out of the rows it never fetched. + private val maxStoryMediaRows = 200000 + + // Wall-clock ceiling on the filesystem half of the scan. The directories can sit on a network mount, and a stat + // against a dead mount never returns — the thread stays parked on the blocking pool, but the poll must not. + private val mediaScanTimeout: FiniteDuration = 5.seconds + + // Schema -> city id, read from configuration alone. The database knows the schema, the directory is named for the + // city, and nothing but this mapping joins them. ConfigService.availableCityIds would do it with one existence + // query per city — the ~50-connection fan-out this dashboard exists to catch (#4559). + private lazy val cityIdBySchema: Map[String, String] = config + .get[Seq[String]]("city-params.city-ids") + .flatMap(cityId => config.getOptional[String](s"city-params.db-schema.$cityId").map(_ -> cityId)) + .toMap + + /** + * Where this instance keeps persistent media, and whether any `story_media` row has lost its bytes (#4926). + * + * The whole signal is cached at the pano TTL: it is the slowest one here (a database round trip plus a directory + * listing per city) and the thing it detects — a deploy having deleted a directory — does not change minute to + * minute. A failure yields None so the panel can say so, rather than sinking the rest of the dashboard. + */ + private def getMediaStorage: Future[Option[MediaStorageHealth]] = { + cacheApi + .getOrElseUpdate[Option[MediaStorageHealth]]("health.media", panoTtl) { + val scan = for { + dirs <- Future(MediaIntegrity.directoryStatuses(config, environment))(blockingIoEc) + integrity <- storyMediaIntegrity + } yield Some(MediaStorageHealth(dirs, environment.mode == play.api.Mode.Prod, integrity._1, integrity._2)) + withTimeout(scan, "media storage scan") + } + .recover { case e: Exception => + logger.warn(s"Health: failed to read media storage: ${e.getMessage}"); None + } + } + + /** + * Compares every visible city's `story_media` rows against the files on disk. + * + * Reads the schemas, then their row counts, then their ids — three cheap round trips behind a five-minute cache + * rather than one query per city. If the base directory itself is unreadable, the scan reports itself unavailable + * instead of declaring every row lost: a monitor that cries data loss over a missing mount would be worse than no + * monitor at all. + * + * @return The scan, or the reason it couldn't run. + */ + private def storyMediaIntegrity: Future[(Option[StoryMediaIntegrity], Option[String])] = { + // Resolve inside the blocking future: MediaDirs.baseDir throws on an unusable value, and a synchronous throw + // here would escape the caller's `.recover` instead of degrading to an unavailable panel. + Future { + val baseDir = MediaDirs.baseDir(config, environment, "story.media.directory") + (baseDir, baseDir.isDirectory) + }(blockingIoEc).flatMap { + case (baseDir, false) => + // Nothing has been uploaded on this stage yet, or the directory is gone. Either way there is nothing to + // compare against, and the directory panel above already reports the state of the path itself. + Future.successful((None, Some(s"No media directory at ${baseDir.getAbsolutePath} to scan."))) + case (baseDir, true) => + db.run(healthTable.getStoryMediaSchemas).map(_.filter(_.matches("^[A-Za-z0-9_]+$"))).flatMap { schemas => + if (schemas.isEmpty) + Future.successful((Some(StoryMediaIntegrity(baseDir.getAbsolutePath, Seq.empty, 0, 0)), None)) + else + db.run(healthTable.getStoryMediaCounts(schemas)).flatMap { counts => + val total = counts.map(_.rows).sum + if (total > maxStoryMediaRows) { + Future.successful((None, Some(s"$total story_media rows is more than this scan will load at once."))) + } else { + val populated = counts.filter(_.rows > 0).map(_.schema) + val idsF = + if (populated.isEmpty) Future.successful(Seq.empty[SchemaMediaId]) + else db.run(healthTable.getStoryMediaIds(populated)) + idsF.flatMap(ids => scanCities(baseDir, counts, ids).map(cities => (Some(cities), None))) + } + } + } + } + } + + /** Lists each city's directory once and diffs it against that city's ids — one listing per city, whatever the row count. */ + private def scanCities( + baseDir: File, + counts: Seq[SchemaRowCount], + ids: Seq[SchemaMediaId] + ): Future[StoryMediaIntegrity] = { + val idsBySchema = ids.groupBy(_.schema).view.mapValues(_.map(_.storyMediaId)).toMap + Future { + counts.sortBy(_.schema).map { case SchemaRowCount(schema, _) => + val cityId = cityIdBySchema.get(schema) + val fileNames = cityId.flatMap(id => MediaIntegrity.listFileNames(new File(baseDir, id))) + MediaIntegrity.compareCity(cityId, schema, idsBySchema.getOrElse(schema, Seq.empty), fileNames) + } + }(blockingIoEc).map { cities => + StoryMediaIntegrity(baseDir.getAbsolutePath, cities, cities.map(_.missing).sum, cities.map(_.orphans).sum) + } + } + + /** Fails a future that outlives the media-scan budget, so one unreachable mount can't hold the poll open. */ + private def withTimeout[T](f: Future[T], label: String): Future[T] = { + val timeout = after(mediaScanTimeout, actorSystem.scheduler)( + Future.failed(new java.util.concurrent.TimeoutException(s"$label did not finish within $mediaScanTimeout")) + ) + Future.firstCompletedOf(Seq(f, timeout)) + } + private def logAndEmpty[T](label: String): PartialFunction[Throwable, Seq[T]] = { case e: Exception => logger.warn(s"Health: failed to read $label: ${e.getMessage}") Seq.empty[T] @@ -265,13 +463,17 @@ class HealthServiceImpl @Inject() ( object HealthService { implicit private val jsonConfig: JsonConfiguration = JsonConfiguration(JsonNaming.SnakeCase) - implicit val blockingSessionWrites: Writes[BlockingSession] = Json.writes[BlockingSession] - implicit val idleTxnSessionWrites: Writes[IdleTxnSession] = Json.writes[IdleTxnSession] - implicit val activeQueryWrites: Writes[ActiveQuery] = Json.writes[ActiveQuery] - implicit val stuckEvolutionWrites: Writes[StuckEvolution] = Json.writes[StuckEvolution] - implicit val tableBloatWrites: Writes[TableBloat] = Json.writes[TableBloat] - implicit val connCountWrites: Writes[ConnCount] = Json.writes[ConnCount] - implicit val panoBackupStatsWrites: Writes[PanoBackupStats] = Json.writes[PanoBackupStats] - implicit val healthThresholdsWrites: Writes[HealthThresholds] = Json.writes[HealthThresholds] - implicit val dbHealthDataWrites: Writes[DbHealthData] = Json.writes[DbHealthData] + implicit val blockingSessionWrites: Writes[BlockingSession] = Json.writes[BlockingSession] + implicit val idleTxnSessionWrites: Writes[IdleTxnSession] = Json.writes[IdleTxnSession] + implicit val activeQueryWrites: Writes[ActiveQuery] = Json.writes[ActiveQuery] + implicit val stuckEvolutionWrites: Writes[StuckEvolution] = Json.writes[StuckEvolution] + implicit val tableBloatWrites: Writes[TableBloat] = Json.writes[TableBloat] + implicit val connCountWrites: Writes[ConnCount] = Json.writes[ConnCount] + implicit val panoBackupStatsWrites: Writes[PanoBackupStats] = Json.writes[PanoBackupStats] + implicit val mediaDirStatusWrites: Writes[MediaDirStatus] = Json.writes[MediaDirStatus] + implicit val cityStoryMediaWrites: Writes[CityStoryMedia] = Json.writes[CityStoryMedia] + implicit val storyMediaIntegrityWrites: Writes[StoryMediaIntegrity] = Json.writes[StoryMediaIntegrity] + implicit val mediaStorageHealthWrites: Writes[MediaStorageHealth] = Json.writes[MediaStorageHealth] + implicit val healthThresholdsWrites: Writes[HealthThresholds] = Json.writes[HealthThresholds] + implicit val dbHealthDataWrites: Writes[DbHealthData] = Json.writes[DbHealthData] } diff --git a/app/service/LostMediaLog.scala b/app/service/LostMediaLog.scala new file mode 100644 index 0000000000..9910025173 --- /dev/null +++ b/app/service/LostMediaLog.scala @@ -0,0 +1,56 @@ +package service + +import play.api.Logger + +import java.util.Collections +import javax.inject.Singleton + +/** + * Announces media whose bytes have gone missing, once per item. + * + * A media row with no file on disk answers every request with a plain 404, indistinguishable from an id that never + * existed — which is why a destroyed story photo went unnoticed for six days (#4925). The response has to stay a bare + * 404 (telling a prober which ids exist would be worse), so the log is the only place the loss can be stated. + * + * Deduplicated because the interesting event is the loss, not the traffic: one popular page re-requesting a lost file + * would otherwise write thousands of identical lines and bury it. The tracking set is bounded — a lost mount can make + * every pano in a city report at once — so eviction eventually lets a still-lost item announce itself again, which is + * the right way for this to fail. + */ +@Singleton +class LostMediaLog { + private val logger = Logger(this.getClass) + + // Comfortably above the number of distinct items any single incident produces, and small enough that the worst case + // (a whole city's panos reporting at once) costs a few hundred KB rather than growing with the pano table. + private val maxTrackedItems = 2000 + + // Access-ordered LRU: the eldest *least recently reported* key is evicted once the map is full. Only ever touched + // through `put` below, inside the synchronized wrapper — access order mutates on read, so unsynchronized reads + // would corrupt it. + private val reported: java.util.Map[String, java.lang.Boolean] = Collections.synchronizedMap( + new java.util.LinkedHashMap[String, java.lang.Boolean](256, 0.75f, true) { + override def removeEldestEntry(eldest: java.util.Map.Entry[String, java.lang.Boolean]): Boolean = + size() > maxTrackedItems + } + ) + + /** + * Logs a media item whose bytes are missing from disk, unless it has already been reported recently. + * + * Every caller goes through here so the lines share one shape and a loss inventory can be grepped out of the logs + * whole. Severity follows the same tiering as `PersistentMediaDirCheck`: content no rebuild can recreate is an + * error, derived content that can be regenerated is a warning. + * + * @param kind What was lost, named after its table or endpoint (`story_media`, `pano`, `crop`). + * @param id Identifies the item within its kind; included verbatim in the message. + * @param path Where the bytes should have been, for whoever goes looking. + * @param irreplaceable Whether losing this destroys content (error) rather than costing a rebuild (warning). + */ + def reportMissing(kind: String, id: String, path: String, irreplaceable: Boolean): Unit = { + if (reported.put(s"$kind:$id", java.lang.Boolean.TRUE) == null) { + val message = s"$kind $id has no file on disk at $path" + if (irreplaceable) logger.error(message) else logger.warn(message) + } + } +} diff --git a/app/service/MediaIntegrity.scala b/app/service/MediaIntegrity.scala new file mode 100644 index 0000000000..1caf6ee1ab --- /dev/null +++ b/app/service/MediaIntegrity.scala @@ -0,0 +1,117 @@ +package service + +// The dashboard reports on exactly the directories the boot check guards, from the same list, so the page and the +// check can never disagree about which directories matter or which of them is currently unsafe (#4925). +import modules.PersistentMediaDirCheck +import modules.PersistentMediaDirCheck.PersistentDir +import play.api.{Configuration, Environment, Mode} + +import java.io.File +import scala.util.{Failure, Success, Try} + +/** + * The pure logic behind the Health dashboard's media-storage panel (#4926): what each persistent media directory + * looks like from this instance, and which `story_media` rows have lost their bytes. + * + * Split from [[HealthService]] — which owns the database reads, the cache, and the thread pool — so a spec can pin + * the parts that are easy to get quietly wrong (an absent directory reported as data loss, a blank config value + * treated as the filesystem root) without booting an application or touching a database. + */ +object MediaIntegrity { + + /** Longest list of ids the payload carries per city; the counts are the signal, the ids are just a starting point. */ + private val MaxSampleIds = 20 + + /** `StoryService.storyMediaFile` names every file this way, which is what lets one directory listing replace N stats. */ + private val StoryMediaFileName = """^story_(\d+)\.jpg$""".r + + /** + * How each persistent media directory resolves on this instance, in `persistentDirs` order. + * + * @param config Application configuration. + * @param environment Play environment supplying the application root and the run mode. + * @return One status per directory, carrying its own display label and severity so the page holds no + * copy of the rules. + */ + def directoryStatuses(config: Configuration, environment: Environment): Seq[MediaDirStatus] = { + // The check arms only in prod mode, and in a dev checkout the relative defaults are supposed to land in the repo. + // Reporting those as failures would make the dev dashboard permanently red and teach everyone to ignore it. + val enforced = environment.mode == Mode.Prod + val unsafe = PersistentMediaDirCheck.unsafeDirs(config, environment).map(u => u.dir.key -> u.reason).toMap + + PersistentMediaDirCheck.persistentDirs.map { dir => + Try(MediaDirs.baseDir(config, environment, dir.key)) match { + case Failure(e) => + status(dir, "—", "unresolved", "unusable value", "bad", Some(e.getMessage)) + case Success(resolved) => + val path = resolved.getAbsolutePath + unsafe.get(dir.key) match { + case Some(reason) => + val severity = if (!enforced) "ok" else if (dir.irreplaceable) "bad" else "warn" + val label = if (enforced) "a deploy will delete this" else "inside the build tree (dev)" + status(dir, path, "unsafe", label, severity, Some(reason)) + // Not created yet is the normal state until the first upload — the write paths mkdirs on demand. + case None if !resolved.exists() => status(dir, path, "absent", "not created yet", "ok", None) + case None if !resolved.canWrite() => + status( + dir, + path, + "not_writable", + "not writable", + "bad", + Some(s"${dir.envVar} points at a path this process cannot write to, so uploads will fail.") + ) + case None => status(dir, path, "ok", "ok", "good", None) + } + } + } + } + + private def status( + dir: PersistentDir, + path: String, + key: String, + label: String, + severity: String, + detail: Option[String] + ): MediaDirStatus = + MediaDirStatus(dir.key, dir.envVar, dir.irreplaceable, path, key, label, severity, detail) + + /** + * Compares one city's `story_media` rows against the files in its media directory. + * + * Both directions matter. A row with no file is destroyed content (#4925). A file with no row is the opposite + * failure: a retraction whose row delete landed and whose file delete did not, which leaves a photo on disk that + * its author believes they deleted — the hard-delete contract stories were built on (#4054). + * + * @param cityId City the schema belongs to, or None when this instance's config doesn't name it — then the + * directory can't be located and the row is reported unscanned rather than guessed at. + * @param schema Database schema the rows came from. + * @param mediaIds `story_media` ids in that schema. + * @param fileNames Names in the city's media directory, or None when the directory isn't there. An absent + * directory under a readable base is real loss, not an unknown, so its rows count as missing. + * @return Counts in both directions, with a short sample of ids to start looking from. + */ + def compareCity( + cityId: Option[String], + schema: String, + mediaIds: Seq[Int], + fileNames: Option[Seq[String]] + ): CityStoryMedia = { + val rows = mediaIds.distinct + if (cityId.isEmpty) { + CityStoryMedia(cityId, schema, rows.size, 0, 0, Seq.empty, Seq.empty, scanned = false) + } else { + val onDisk = + fileNames.getOrElse(Seq.empty).flatMap { case StoryMediaFileName(id) => id.toIntOption; case _ => None }.toSet + val rowSet = rows.toSet + val missing = (rowSet -- onDisk).toSeq.sorted + val orphans = (onDisk -- rowSet).toSeq.sorted + CityStoryMedia(cityId, schema, rows.size, missing.size, orphans.size, missing.take(MaxSampleIds), + orphans.take(MaxSampleIds), scanned = true) + } + } + + /** Lists a directory's file names, or None when it isn't a readable directory. Blocking; call on a blocking pool. */ + def listFileNames(dir: File): Option[Seq[String]] = Option(dir.list()).map(_.toSeq) +} diff --git a/app/service/PanoDataService.scala b/app/service/PanoDataService.scala index 4f32761c55..28723dd644 100644 --- a/app/service/PanoDataService.scala +++ b/app/service/PanoDataService.scala @@ -328,6 +328,7 @@ trait PanoDataService { def cropExists(labelId: Int, labelType: LabelTypeEnum.Base): Boolean def cropUrl(labelId: Int, labelType: LabelTypeEnum.Base): Option[String] def localBackupImageFile(panoId: String): Option[File] + def backupImageDir(panoId: String): File def getLocalBackupImage(panoId: String): Future[Option[PanoData]] } @@ -704,12 +705,15 @@ class PanoDataServiceImpl @Inject() ( * `///.`. Tries jpg/jpeg/png in order. */ def localBackupImageFile(panoId: String): Option[File] = { - val dir = new File(panosBaseDir, panoId.take(2)) + val dir = backupImageDir(panoId) Seq("jpg", "jpeg", "png").iterator .map(ext => new File(dir, s"$panoId.$ext")) .find(_.exists()) } + /** The directory a pano's backup image lives in, whether or not one is there — named so a miss can say where it looked. */ + def backupImageDir(panoId: String): File = new File(panosBaseDir, panoId.take(2)) + /** * Returns the pano_data row for a pano if a self-hosted image exists AND all required fields are populated. * diff --git a/app/views/admin/dashboard/health.scala.html b/app/views/admin/dashboard/health.scala.html index 9b43486bda..8d02512470 100644 --- a/app/views/admin/dashboard/health.scala.html +++ b/app/views/admin/dashboard/health.scala.html @@ -23,6 +23,7 @@

Health

+
+

Media storage

+

Where this stage keeps the media that has to outlive a deploy, and whether any of it has gone missing. A deploy rebuilds the whole build tree, so anything stored inside it is deleted by the next release while its database rows survive — the failure that destroyed a story photo in #4925. Missing means a story_media row whose file is gone; orphaned means a file whose row is gone, which is a retraction that only half landed.

+
+
+

+
+

Pano downloads

Backup-image coverage for this city's labeled panos. The viewer shows a locally-hosted backup when one exists and otherwise falls back to the live source, so a labeled pano whose source has expired and has no backup can no longer be shown.

diff --git a/conf/application.conf b/conf/application.conf index 664925844d..21caedf8ed 100644 --- a/conf/application.conf +++ b/conf/application.conf @@ -85,6 +85,19 @@ cpu-intensive { } } +# Execution context for blocking filesystem work — today the admin Health dashboard's media-integrity scan, which +# stats and lists the media directories. A thread blocked on an unreachable network mount never comes back, so this +# work is isolated from every other pool: the default pool serves requests, and cpu-intensive doubles as the stream +# materializer below, so a hung mount there would stall the streaming API. Threads, not fork-join tasks, because the +# work is blocking rather than compute-bound. +blocking-io { + executor = "thread-pool-executor" + throughput = 1 + thread-pool-executor { + fixed-pool-size = 4 + } +} + # Sets the execution context that Materializer uses for running streams. We tend to use this for the CPU-intensive API. pekko.stream.materializer.dispatcher = "cpu-intensive" diff --git a/docs/deployment-and-stages.md b/docs/deployment-and-stages.md index 73b243aab5..023b13fd5b 100644 --- a/docs/deployment-and-stages.md +++ b/docs/deployment-and-stages.md @@ -307,7 +307,9 @@ forgotten variable surfaces on test long before it can reach prod. meaningful while it models the exact resolution the write paths use), add it to `persistentDirs` in `PersistentMediaDirCheck`, decide whether its contents are irreplaceable (fatal) or derived (logged), and have the deployment tooling export its variable. Losing a story photo this way (#4925) took three weeks to notice, so the -check — not a comment in `application.conf` — is what holds the contract. +check — not a comment in `application.conf` — is what holds the contract. Adding it to `persistentDirs` also puts it +on the **Media storage** panel of the Owner-only `/admin/health` page, which shows where each directory resolves on +the running instance and counts any `story_media` row whose file has gone missing (#4926). ### Asset caching diff --git a/public/js/admin-dashboard/HealthPage.js b/public/js/admin-dashboard/HealthPage.js index 182c5076e0..572399e064 100644 --- a/public/js/admin-dashboard/HealthPage.js +++ b/public/js/admin-dashboard/HealthPage.js @@ -71,6 +71,7 @@ class HealthPage { this.#renderBloat(data.table_bloat || []); this.#renderConnections(data.connections || []); this.#renderPanos(data.pano_backups || null); + this.#renderMediaStorage(data.media_storage || null); } catch (e) { this.#setHtml('health-pulse', `Could not load health data. ${HealthPage.#esc(e.message)}`); } finally { @@ -127,6 +128,7 @@ class HealthPage { const evolutions = data.stuck_evolutions || []; const conns = (data.connections || []).reduce((sum, c) => sum + (c.count || 0), 0); const atRisk = data.pano_backups?.at_risk; + const media = data.media_storage || null; const bloated = (data.table_bloat || []).filter((b) => this.#bloatTone(b) !== 'good').length; const longIdle = idle.filter((s) => (s.idle_seconds || 0) >= t.idle_txn_warn_seconds).length; const activeBad = active.filter((q) => (q.query_seconds || 0) >= t.active_query_bad_seconds).length; @@ -139,6 +141,25 @@ class HealthPage { // A missing value ("—") means unknown, not healthy, so tone it neutral ('ok') instead of 'good' (green). const panoTone = HealthPage.#nil(atRisk) ? 'ok' : atRisk > 0 ? 'warn' : 'good'; this.#setKpi('kpi-panos', HealthPage.#nil(atRisk) ? '—' : HealthPage.#compact(atRisk), panoTone); + const [mediaValue, mediaTone] = HealthPage.#mediaKpi(media); + this.#setKpi('kpi-media', mediaValue, mediaTone); + } + + /** + * The missing-media KPI: destroyed bytes are the headline, so a directory that a deploy will delete counts as bad + * even before anything has been lost from it. Orphans are a lesser fault and only warn. + * + * @param {?Object} media - The media_storage payload, or null when it couldn't be read. + * @returns {[(string|number), string]} Value and tone, spread into #setKpi. + */ + static #mediaKpi(media) { + // Unknown is not healthy: tone it neutral rather than green. + if (!media) return ['—', 'ok']; + const unsafeDirs = (media.directories || []).filter((d) => d.severity === 'bad').length; + if (!media.story_media) return [unsafeDirs > 0 ? unsafeDirs : '—', unsafeDirs > 0 ? 'bad' : 'ok']; + const { missing, orphans } = media.story_media; + if (missing > 0 || unsafeDirs > 0) return [HealthPage.#compact(missing), 'bad']; + return [HealthPage.#compact(missing), orphans > 0 ? 'warn' : 'good']; } /** Renders the "updated Ns ago · db · role" meta line, including whether other sessions' query text is visible. */ @@ -337,6 +358,95 @@ class HealthPage { + 'and these figures approximate what is actually on disk.'); } + // ---- Panel: media storage -------------------------------------------------------------------------------------- + + /** + * Renders the persistent-media directories and the story-media integrity scan. + * + * Every label, severity and threshold here is server-computed: the page must not hold its own copy of the rules + * that decide when a directory is unsafe, or it would drift from the boot check that enforces them. + * + * @param {?Object} media - The media_storage payload, or null when it couldn't be read. + */ + #renderMediaStorage(media) { + if (!media) { + this.#setHtml('health-media-dirs', '

Media storage status is unavailable.

'); + this.#setHtml('health-media-story', ''); + this.#setHtml('health-media-note', ''); + return; + } + + const dirRows = (media.directories || []).map((d) => { + const holds = d.irreplaceable ? 'content' : 'rebuildable'; + const detail = d.detail ? `
${HealthPage.#esc(d.detail)}` : ''; + return ` + + ${HealthPage.#esc(d.key)} + ${HealthPage.#esc(d.env_var)} + ${holds} + ${HealthPage.#esc(d.path)} + ${HealthPage.#esc(d.label)}${detail} + `; + }).join(''); + this.#table('health-media-dirs', ['Config key', 'Env var', 'Holds', 'Resolves to', 'Status'], dirRows); + + const scan = media.story_media; + if (!scan) { + this.#setHtml('health-media-story', + `

${HealthPage.#esc(media.unavailable || 'Story media was not scanned.')}

`); + } else if (!scan.cities.length) { + this.#renderEmpty('health-media-story', 'No city has any story media yet.'); + } else { + const rows = scan.cities.map((c) => ` + + ${HealthPage.#esc(c.city_id || c.schema)} + ${HealthPage.#num(c.rows)} + ${HealthPage.#mediaCount(c, 'missing', 'bad')} + ${HealthPage.#mediaCount(c, 'orphans', 'warn')} + ${HealthPage.#mediaDetail(c)} + `).join(''); + this.#table('health-media-story', + ['City', ['Media rows', true], ['Missing', true], ['Orphaned', true], 'Ids'], rows); + } + + const notes = [`Story media lives under ${HealthPage.#esc(scan ? scan.base_dir : '—')}, one + subdirectory per city, so this covers every city deployed on this stage — a city hosted elsewhere would read as + unscanned.`]; + if (!media.enforced) { + notes.push(`This instance is not running in production mode, so the boot check that refuses to start on an + unsafe directory is inactive here and the relative defaults landing in the checkout are expected.`); + } + this.#setHtml('health-media-note', notes.join(' ')); + } + + /** + * A missing/orphan count cell, badged only when non-zero so a clean fleet reads as quiet. + * + * @param {Object} city - One city row from the scan. + * @param {string} field - Which count to render. + * @param {string} tone - Badge tone to use when the count is non-zero. + * @returns {string} Cell HTML. + */ + static #mediaCount(city, field, tone) { + if (!city.scanned) return '—'; + const n = city[field] || 0; + return n > 0 ? `${HealthPage.#num(n)}` : HealthPage.#num(n); + } + + /** + * The ids worth looking at for one city, or why it wasn't scanned. + * + * @param {Object} city - One city row from the scan. + * @returns {string} Cell HTML. + */ + static #mediaDetail(city) { + if (!city.scanned) return `no city configured for schema ${HealthPage.#esc(city.schema)}`; + const parts = []; + if (city.missing_ids?.length) parts.push(`missing ${city.missing_ids.join(', ')}`); + if (city.orphan_ids?.length) parts.push(`orphaned ${city.orphan_ids.join(', ')}`); + return parts.length ? `${HealthPage.#esc(parts.join(' · '))}` : '—'; + } + // ---- Small helpers --------------------------------------------------------------------------------------------- /** diff --git a/test/controllers/ImageControllerSpec.scala b/test/controllers/ImageControllerSpec.scala index 37051d2cd4..b1155c89e0 100644 --- a/test/controllers/ImageControllerSpec.scala +++ b/test/controllers/ImageControllerSpec.scala @@ -10,7 +10,8 @@ import play.api.mvc.Cookie import play.api.test.CSRFTokenHelper._ import play.api.test.FakeRequest import play.api.test.Helpers._ -import service.{PanoDataService, ShareImageCache} +import models.label.LabelTypeEnum +import service.{ImageSigningService, PanoDataService, ShareImageCache} import util.AnonSession import java.awt.image.BufferedImage @@ -42,8 +43,9 @@ class ImageControllerSpec extends PlaySpec with AnonSession with GuiceOneAppPerS implicit lazy val mat: Materializer = app.materializer - private val panoDataService: PanoDataService = app.injector.instanceOf[PanoDataService] - private val shareImageCache: ShareImageCache = app.injector.instanceOf[ShareImageCache] + private val panoDataService: PanoDataService = app.injector.instanceOf[PanoDataService] + private val shareImageCache: ShareImageCache = app.injector.instanceOf[ShareImageCache] + private val signingService: ImageSigningService = app.injector.instanceOf[ImageSigningService] // Far outside the range of any real label id, so writing a crop here can't clobber one. private val syntheticLabelId = Int.MaxValue - 4726 @@ -143,4 +145,29 @@ class ImageControllerSpec extends PlaySpec with AnonSession with GuiceOneAppPerS status(resp) mustBe BAD_REQUEST } } + + // A signed serving URL is only ever minted for a file that was on disk at the time (PanoDataService.cropUrl and + // backupImageUrl both check first), so a miss on one of these means the bytes vanished inside the signature's + // ~75-minute life. That is the loss #4925 had no way to notice, and #4926 gives it a log line — but the response + // still has to stay an ordinary 404, which is what these pin. + "Serving media whose bytes are gone" should { + "answer a signed crop URL whose file has been deleted with a plain 404" in { + val session = freshAnonSession() + status(postCrop(session, syntheticLabelId)) mustBe OK + val url = panoDataService.cropUrl(syntheticLabelId, LabelTypeEnum.CurbRamp).value + cropFileFor(syntheticLabelId).delete() mustBe true + + val resp = route(app, FakeRequest(GET, url).withCookies(session: _*)).get + status(resp) mustBe NOT_FOUND + cleanUp(syntheticLabelId) + } + + "answer a signed pano URL with no backup image with a plain 404, and stay quiet on a repeat" in { + // Two requests: the log deduplicates, the responses must not. + val panoId = "sidewalkSpecNoSuchPano4926" + val url = signingService.signedUrl(s"/backupImage/$panoId") + status(route(app, FakeRequest(GET, url)).get) mustBe NOT_FOUND + status(route(app, FakeRequest(GET, url)).get) mustBe NOT_FOUND + } + } } diff --git a/test/service/HealthServiceSpec.scala b/test/service/HealthServiceSpec.scala index 4e3fa54bc9..998a2a4e86 100644 --- a/test/service/HealthServiceSpec.scala +++ b/test/service/HealthServiceSpec.scala @@ -76,6 +76,19 @@ class HealthServiceSpec extends PlaySpec with GuiceOneAppPerSuite { // The single UNION-ALL query must execute across all discovered schemas at once (the fan-out safety property). run(healthTable.getStuckEvolutionsForSchemas(schemas)).size must be >= 0 } + "count and list story_media across every schema in one union query each" in { + // Every city schema gets the table from evolution 339, and the app under test applies evolutions on boot, so an + // empty result means discovery is broken rather than that this database is unusual. + val schemas = run(healthTable.getStoryMediaSchemas).filter(_.matches("^[A-Za-z0-9_]+$")) + schemas must not be empty + val counts = run(healthTable.getStoryMediaCounts(schemas)) + counts.map(_.schema) must contain theSameElementsAs schemas + // The counts decide which schemas the id read covers, so a disagreement between them would make the scan + // report rows it never fetched as missing. What each id/file pairing then *means* is pinned by + // MediaIntegritySpec — seeding a story here would need a user, a label and an audit task, none of which the CI + // seed has. + run(healthTable.getStoryMediaIds(schemas)) must have size counts.map(_.rows).sum.toLong + } } "HealthService.getDbHealth" should { @@ -92,6 +105,26 @@ class HealthServiceSpec extends PlaySpec with GuiceOneAppPerSuite { t.bloatBadRatio must be >= t.bloatWarnRatio } + "report on every media directory the boot check guards" in { + val media = await(healthService.getDbHealth).mediaStorage.value + media.directories.map(_.key) mustBe modules.PersistentMediaDirCheck.persistentDirs.map(_.key) + // Each row has to name the variable that fixes it; a path alone doesn't tell an operator what to change. + media.directories.foreach(_.envVar must not be empty) + } + + "keep the story-media scan's totals consistent with its per-city rows" in { + val media = await(healthService.getDbHealth).mediaStorage.value + // The scan reports itself unavailable when there is no media directory to read, which is the normal state of a + // dev checkout that has never had an upload. Either way it must never report a total its rows don't support. + media.storyMedia match { + case None => media.unavailable mustBe defined + case Some(scan) => + scan.missing mustBe scan.cities.map(_.missing).sum + scan.orphans mustBe scan.cities.map(_.orphans).sum + scan.cities.filterNot(_.scanned).foreach(_.missing mustBe 0) + } + } + "survive a burst of concurrent polls without exhausting the connection pool" in { // Simulate many Owner tabs polling at once, from a cold cache (the worst case). If getDbHealth fanned out one // query per city schema, 30 concurrent calls would each demand ~one-connection-per-schema and, on a many-city diff --git a/test/service/MediaIntegritySpec.scala b/test/service/MediaIntegritySpec.scala new file mode 100644 index 0000000000..7d54cd3289 --- /dev/null +++ b/test/service/MediaIntegritySpec.scala @@ -0,0 +1,171 @@ +package service + +import modules.PersistentMediaDirCheck.persistentDirs +import org.scalatestplus.play.PlaySpec +import play.api.{Configuration, Environment, Mode} + +import java.io.File +import java.nio.file.Files + +/** + * The media-storage panel's judgment calls (#4926): which of them mean data has been lost, and — just as important — + * which of them don't. + * + * A monitor that reports loss when a directory is merely absent, or on a dev checkout where the relative defaults are + * the intended behavior, gets ignored, and an ignored monitor is worth nothing. So the false-alarm cases are pinned + * here as tightly as the real ones. + * + * Pure logic — no app boot and no database; the only filesystem it touches is a temp directory it creates itself. + */ +class MediaIntegritySpec extends PlaySpec { + + private val appRoot = new File("/srv/sidewalk/target/universal/stage") + + private def env(mode: Mode): Environment = Environment(appRoot, getClass.getClassLoader, mode) + + private def config(path: String): Configuration = + Configuration.from(persistentDirs.map(_.key -> path).toMap) + + private def statusFor(path: String, mode: Mode): MediaDirStatus = + MediaIntegrity.directoryStatuses(config(path), env(mode)).head + + /** A temp directory holding the given file names, cleaned up by the JVM's temp handling. */ + private def dirContaining(names: String*): File = { + val dir = Files.createTempDirectory("media-integrity-spec").toFile + dir.deleteOnExit() + names.foreach { name => + val f = new File(dir, name) + f.createNewFile() + f.deleteOnExit() + } + dir + } + + "directoryStatuses" should { + "cover every directory the boot check guards, so the page can't quietly omit one" in { + MediaIntegrity.directoryStatuses(config("/srv/media"), env(Mode.Prod)).map(_.key) mustBe persistentDirs.map(_.key) + } + + "flag a directory a deploy would delete as bad when it holds irreplaceable content" in { + val prod = MediaIntegrity.directoryStatuses(config(".story-media"), env(Mode.Prod)) + prod.filter(_.irreplaceable).foreach { d => + d.status mustBe "unsafe" + d.severity mustBe "bad" + } + prod.filterNot(_.irreplaceable).foreach(_.severity mustBe "warn") + } + + "not alarm on the same directory in dev, where the relative defaults are supposed to land in the checkout" in { + MediaIntegrity.directoryStatuses(config(".story-media"), env(Mode.Dev)).foreach { d => + d.status mustBe "unsafe" + d.severity mustBe "ok" + } + } + + "report a directory that doesn't exist yet as normal, since the write paths create it on first upload" in { + val status = statusFor("/srv/sidewalk-media/nothing-here-yet", Mode.Prod) + status.status mustBe "absent" + status.severity mustBe "ok" + } + + "report a usable directory as ok" in { + val status = statusFor(dirContaining().getAbsolutePath, Mode.Prod) + status.status mustBe "ok" + status.severity mustBe "good" + } + + "surface a blank value rather than resolving it, since it would target the filesystem root" in { + val status = statusFor("", Mode.Prod) + status.status mustBe "unresolved" + status.severity mustBe "bad" + status.detail.value must include("set but empty") + } + + "name the environment variable that fixes each directory" in { + MediaIntegrity.directoryStatuses(config(".story-media"), env(Mode.Prod)).map(_.envVar) mustBe + persistentDirs.map(_.envVar) + } + } + + "compareCity" should { + "report a city whose rows all have files as clean" in { + val result = MediaIntegrity.compareCity( + Some("chicago-il"), + "sidewalk_chicago", + Seq(1, 2), + Some(Seq("story_1.jpg", "story_2.jpg")) + ) + result.missing mustBe 0 + result.orphans mustBe 0 + result.rows mustBe 2 + result.scanned mustBe true + } + + "report a row with no file as missing — the #4925 loss" in { + val result = + MediaIntegrity.compareCity(Some("chicago-il"), "sidewalk_chicago", Seq(1, 2), Some(Seq("story_1.jpg"))) + result.missing mustBe 1 + result.missingIds mustBe Seq(2) + result.orphans mustBe 0 + } + + "report a file with no row as orphaned — a retraction whose file delete didn't land" in { + val result = MediaIntegrity.compareCity( + Some("chicago-il"), + "sidewalk_chicago", + Seq(1), + Some(Seq("story_1.jpg", "story_7.jpg")) + ) + result.orphans mustBe 1 + result.orphanIds mustBe Seq(7) + result.missing mustBe 0 + } + + "count both directions at once, which a row-count comparison alone would call clean" in { + val result = MediaIntegrity.compareCity(Some("chicago-il"), "sidewalk_chicago", Seq(1), Some(Seq("story_9.jpg"))) + result.missing mustBe 1 + result.orphans mustBe 1 + } + + "ignore files that aren't story media, so a stray README isn't reported as an orphan" in { + val result = MediaIntegrity.compareCity( + Some("chicago-il"), + "sidewalk_chicago", + Seq(1), + Some(Seq("story_1.jpg", "README.txt", "story_1.jpg.bak", "story_.jpg")) + ) + result.orphans mustBe 0 + result.missing mustBe 0 + } + + "treat an absent city directory under a readable base as loss, not as an unknown" in { + val result = MediaIntegrity.compareCity(Some("chicago-il"), "sidewalk_chicago", Seq(1, 2), None) + result.missing mustBe 2 + result.scanned mustBe true + } + + "report a city with no rows and no directory as clean rather than as a fault" in { + val result = MediaIntegrity.compareCity(Some("chicago-il"), "sidewalk_chicago", Seq.empty, None) + result.missing mustBe 0 + result.orphans mustBe 0 + result.scanned mustBe true + } + + "decline to guess when a schema maps to no configured city, since its directory can't be located" in { + val result = MediaIntegrity.compareCity(None, "sidewalk_somewhere", Seq(1, 2), None) + result.scanned mustBe false + result.missing mustBe 0 + result.rows mustBe 2 + } + } + + "listFileNames" should { + "read a directory's contents" in { + MediaIntegrity.listFileNames(dirContaining("story_3.jpg")).value must contain("story_3.jpg") + } + + "return nothing for a path that isn't a readable directory, so the caller can tell it apart from an empty one" in { + MediaIntegrity.listFileNames(new File("/srv/sidewalk-media/no-such-directory")) mustBe None + } + } +} From 2d086f38b94574c90335006e00bb2398e74c0165 Mon Sep 17 00:00:00 2001 From: jonfroehlich Date: Wed, 19 Aug 2026 16:41:19 -0700 Subject: [PATCH 2/8] Resolve the media scan's own city the way the write path does (#4926) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- app/models/utils/HealthTable.scala | 8 +++++ app/service/HealthService.scala | 29 +++++++++++++++-- app/service/MediaIntegrity.scala | 28 +++++++++++++++++ test/service/MediaIntegritySpec.scala | 45 +++++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 3 deletions(-) diff --git a/app/models/utils/HealthTable.scala b/app/models/utils/HealthTable.scala index c96be8d0d7..4a3bb28224 100644 --- a/app/models/utils/HealthTable.scala +++ b/app/models/utils/HealthTable.scala @@ -282,6 +282,14 @@ class HealthTable @Inject() (protected val dbConfigProvider: DatabaseConfigProvi """.as[PanoBackupStats].head } + /** + * The schema this instance actually reads and writes, which is what the media scan resolves its own city's + * directory from. Config alone can't answer it: `city-id` and the connection's schema are separate settings and a + * misconfigured instance has them disagreeing, which would make the scan look for files somewhere the app never + * writes them. + */ + def getCurrentSchema: DBIO[String] = bounded { sql"""SELECT current_schema()""".as[String].head } + /** * Schemas holding a readable `story_media` table, for the cross-city media-integrity scan (#4926). * diff --git a/app/service/HealthService.scala b/app/service/HealthService.scala index f01f75ac4b..d315b2a183 100644 --- a/app/service/HealthService.scala +++ b/app/service/HealthService.scala @@ -421,23 +421,46 @@ class HealthServiceImpl @Inject() ( val idsF = if (populated.isEmpty) Future.successful(Seq.empty[SchemaMediaId]) else db.run(healthTable.getStoryMediaIds(populated)) - idsF.flatMap(ids => scanCities(baseDir, counts, ids).map(cities => (Some(cities), None))) + for { + ids <- idsF + current <- db.run(healthTable.getCurrentSchema) + cities <- scanCities(baseDir, current, counts, ids) + } yield (Some(cities), None) } } } } } - /** Lists each city's directory once and diffs it against that city's ids — one listing per city, whatever the row count. */ + /** + * Lists each city's directory once and diffs it against that city's ids — one listing per city, whatever the row + * count, which is what keeps this affordable as stories grow. + * + * This instance's own schema takes its city from `city-id` rather than from the schema mapping, because that is + * what `StoryService` builds its write path from: the two settings are independent, and on an instance where they + * disagree the scan has to look where the files actually are rather than where the mapping says they should be. + * + * @param baseDir Resolved base directory holding the per-city subdirectories. + * @param currentSchema The schema this instance reads and writes. + * @param counts Row counts per schema, which decide the rows reported. + * @param ids Every media id, tagged with its schema. + */ private def scanCities( baseDir: File, + currentSchema: String, counts: Seq[SchemaRowCount], ids: Seq[SchemaMediaId] ): Future[StoryMediaIntegrity] = { val idsBySchema = ids.groupBy(_.schema).view.mapValues(_.map(_.storyMediaId)).toMap + val cityDirs = MediaIntegrity.cityDirsBySchema( + counts.map(_.schema), + currentSchema, + config.get[String]("city-id"), + cityIdBySchema + ) Future { counts.sortBy(_.schema).map { case SchemaRowCount(schema, _) => - val cityId = cityIdBySchema.get(schema) + val cityId = cityDirs.get(schema) val fileNames = cityId.flatMap(id => MediaIntegrity.listFileNames(new File(baseDir, id))) MediaIntegrity.compareCity(cityId, schema, idsBySchema.getOrElse(schema, Seq.empty), fileNames) } diff --git a/app/service/MediaIntegrity.scala b/app/service/MediaIntegrity.scala index 1caf6ee1ab..c94ec52ffe 100644 --- a/app/service/MediaIntegrity.scala +++ b/app/service/MediaIntegrity.scala @@ -77,6 +77,34 @@ object MediaIntegrity { ): MediaDirStatus = MediaDirStatus(dir.key, dir.envVar, dir.irreplaceable, path, key, label, severity, detail) + /** + * Which city's subdirectory each schema's media lives in. + * + * This instance's own schema takes `city-id`, because that is what `StoryService` builds its write path from — + * the schema and `city-id` are independent settings, so on an instance where they disagree the scan has to look + * where the files actually are. That claim is exclusive: no second schema may be pointed at the same directory, + * or it would report the first schema's files as orphans. A schema left without a directory is reported unscanned, + * which is the truth — nothing here can say where its files are. + * + * @param schemas Schemas the scan covers. + * @param currentSchema The schema this instance reads and writes. + * @param currentCity The `city-id` this instance writes media under. + * @param configured Schema to city id, from configuration alone. + * @return Schema to city id, for the schemas whose directory can be located. + */ + def cityDirsBySchema( + schemas: Seq[String], + currentSchema: String, + currentCity: String, + configured: Map[String, String] + ): Map[String, String] = { + val others = schemas + .filter(_ != currentSchema) + .flatMap(schema => configured.get(schema).filter(_ != currentCity).map(schema -> _)) + .toMap + if (schemas.contains(currentSchema)) others + (currentSchema -> currentCity) else others + } + /** * Compares one city's `story_media` rows against the files in its media directory. * diff --git a/test/service/MediaIntegritySpec.scala b/test/service/MediaIntegritySpec.scala index 7d54cd3289..81fb9ab7ab 100644 --- a/test/service/MediaIntegritySpec.scala +++ b/test/service/MediaIntegritySpec.scala @@ -87,6 +87,51 @@ class MediaIntegritySpec extends PlaySpec { } } + private val configuredCities = Map("sidewalk_chicago" -> "chicago-il", "sidewalk_seattle" -> "seattle-wa") + + "cityDirsBySchema" should { + "use the configured city for every schema when nothing disagrees" in { + MediaIntegrity.cityDirsBySchema( + Seq("sidewalk_chicago", "sidewalk_seattle"), + "sidewalk_chicago", + "chicago-il", + configuredCities + ) mustBe configuredCities + } + + "follow this instance's own city-id, since that is what the write path builds its path from" in { + // A dev container left with city-id and the connection's schema disagreeing writes media under the city-id. + val dirs = MediaIntegrity.cityDirsBySchema( + Seq("sidewalk_chicago", "sidewalk_seattle"), + "sidewalk_chicago", + "seattle-wa", + configuredCities + ) + dirs.get("sidewalk_chicago") mustBe Some("seattle-wa") + } + + "not let a second schema claim a directory this instance already writes to" in { + // Otherwise the other schema lists the same files and reports every one of them as an orphan. + val dirs = MediaIntegrity.cityDirsBySchema( + Seq("sidewalk_chicago", "sidewalk_seattle"), + "sidewalk_chicago", + "seattle-wa", + configuredCities + ) + dirs.get("sidewalk_seattle") mustBe None + } + + "leave out a schema no configured city names, so it is reported unscanned rather than guessed at" in { + MediaIntegrity.cityDirsBySchema( + Seq("sidewalk_elsewhere"), + "sidewalk_chicago", + "chicago-il", + configuredCities + ) mustBe + Map.empty + } + } + "compareCity" should { "report a city whose rows all have files as clean" in { val result = MediaIntegrity.compareCity( From bbab1c34194764c4d12959f5ded12cc135102f91 Mon Sep 17 00:00:00 2001 From: jonfroehlich Date: Wed, 19 Aug 2026 21:31:06 -0700 Subject: [PATCH 3/8] Address deep review of the media-loss guardrails (#4926) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .github/workflows/ci.yml | 9 +- app/modules/PersistentMediaDirCheck.scala | 15 +- app/service/HealthService.scala | 72 +++++-- app/service/MediaIntegrity.scala | 217 +++++++++++++++------ public/js/admin-dashboard/HealthPage.js | 21 +- test/controllers/ImageControllerSpec.scala | 4 +- test/service/HealthServiceSpec.scala | 7 +- test/service/LostMediaLogSpec.scala | 100 ++++++++++ test/service/MediaIntegritySpec.scala | 123 +++++++++--- 9 files changed, 445 insertions(+), 123 deletions(-) create mode 100644 test/service/LostMediaLogSpec.scala diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9f3b449df9..1e2796f0c0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -305,10 +305,13 @@ jobs: # which a deploy rebuilds and deletes. Pure logic, no DB — and the failure it guards against is invisible to # every other test, since the broken config behaves correctly right up until the next release. # - MediaIntegritySpec (#4926): the media-storage panel's calls on what counts as data loss. Pure logic, no DB. - # Its false-alarm cases matter as much as its real ones — a monitor that cries loss over an absent directory - # gets ignored, and an ignored monitor leaves us exactly where #4925 found us. + # Its false-alarm cases matter as much as its real ones — a monitor that cries loss over an unreadable + # directory gets ignored, and an ignored monitor leaves us exactly where #4925 found us. + # - LostMediaLogSpec (#4926): the log line that is the only signal a media file has been destroyed. Both ways + # it can fail are silent — announcing every request buries the event, announcing nothing reads as health — + # so the lines themselves are asserted. Pure logic, no DB. - name: Run gating/auth tests (health dashboard + route auth posture + geodesic distances) - run: sbt 'set Test / parallelExecution := false' 'testOnly controllers.HealthDashboardSpec service.HealthServiceSpec controllers.RouteAuthPostureSpec models.street.GeodesicDistanceSpec service.ExploreTutorialRouteSpec controllers.MobileDetectionSpec service.PanoDataServiceSpec models.utils.ConfigTableVoidedArchiveSpec controllers.api.StatsApiSpec controllers.ExploreSubmissionSpec controllers.ValidateSubmissionSpec modules.PersistentMediaDirCheckSpec service.MediaIntegritySpec' + run: sbt 'set Test / parallelExecution := false' 'testOnly controllers.HealthDashboardSpec service.HealthServiceSpec controllers.RouteAuthPostureSpec models.street.GeodesicDistanceSpec service.ExploreTutorialRouteSpec controllers.MobileDetectionSpec service.PanoDataServiceSpec models.utils.ConfigTableVoidedArchiveSpec controllers.api.StatsApiSpec controllers.ExploreSubmissionSpec controllers.ValidateSubmissionSpec modules.PersistentMediaDirCheckSpec service.MediaIntegritySpec service.LostMediaLogSpec' env: DATABASE_URL: jdbc:postgresql://localhost:5432/sidewalk DATABASE_USER: sidewalk diff --git a/app/modules/PersistentMediaDirCheck.scala b/app/modules/PersistentMediaDirCheck.scala index f281667d50..267ca2d1a2 100644 --- a/app/modules/PersistentMediaDirCheck.scala +++ b/app/modules/PersistentMediaDirCheck.scala @@ -1,6 +1,6 @@ package modules -import modules.PersistentMediaDirCheck.unsafeDirs +import modules.PersistentMediaDirCheck.{arms, unsafeDirs} import play.api.{Configuration, Environment, Logger, Mode} import service.MediaDirs @@ -33,7 +33,7 @@ import scala.util.{Failure, Success, Try} class PersistentMediaDirCheck @Inject() (config: Configuration, environment: Environment) { private val logger = Logger(this.getClass) - if (environment.mode == Mode.Prod) { + if (arms(environment)) { val unsafe = unsafeDirs(config, environment) unsafe.foreach(u => logger.error(u.reason)) @@ -67,6 +67,17 @@ object PersistentMediaDirCheck { /** A persistent directory that failed the check, with the loggable reason. */ case class UnsafeDir(dir: PersistentDir, reason: String) + /** + * Whether the check arms in this run mode. Defined here, next to the `if` it gates, because the Health dashboard + * reports what the check makes of each directory and has to agree with it about whether it is even watching — two + * copies of the rule would let the page claim a stage is guarded when it isn't. + * + * @param environment Play environment supplying the run mode. + * @return True in prod mode, which every staged binary runs in; false in the dev and test runs where + * the application root is a hand-managed checkout and the relative defaults are the point. + */ + def arms(environment: Environment): Boolean = environment.mode == Mode.Prod + val persistentDirs: Seq[PersistentDir] = Seq( // Crops and share previews are derived: a crop can be re-cut from pano imagery and a share preview rebuilds on // demand, so losing them costs rebuild time, not content. diff --git a/app/service/HealthService.scala b/app/service/HealthService.scala index d315b2a183..a321fc7180 100644 --- a/app/service/HealthService.scala +++ b/app/service/HealthService.scala @@ -128,9 +128,12 @@ case class MediaDirStatus( * @param rows `story_media` rows in that schema. * @param missing Rows whose bytes are gone — data loss (#4925). * @param orphans Files with no row — a retraction whose file delete didn't land (#4054). - * @param missingIds Sample of the missing ids, to start looking from. - * @param orphanIds Sample of the orphaned ids. - * @param scanned False when the directory couldn't be located, so the counts mean nothing. + * @param missingIds Sample of the missing ids, to start looking from. + * @param orphanIds Sample of the orphaned ids. + * @param scanned False when the directory couldn't be read, so the counts mean nothing. + * @param unscannedReason Why it wasn't scanned, when it wasn't. The reasons differ enough — a schema no city on this + * stage claims, a directory this instance writes under another schema, one it may not read — + * that a single "not scanned" would send an operator looking in the wrong place. */ case class CityStoryMedia( cityId: Option[String], @@ -140,7 +143,8 @@ case class CityStoryMedia( orphans: Int, missingIds: Seq[Int], orphanIds: Seq[Int], - scanned: Boolean + scanned: Boolean, + unscannedReason: Option[String] ) /** @@ -357,6 +361,13 @@ class HealthServiceImpl @Inject() ( // against a dead mount never returns — the thread stays parked on the blocking pool, but the poll must not. private val mediaScanTimeout: FiniteDuration = 5.seconds + // One scan at a time. `withTimeout` abandons the future it gave up waiting on, but nothing can cancel the thread + // underneath it: a filesystem call against an unreachable mount returns when the mount does, or never. Without this + // guard the dashboard's ~20s poll would stack a fresh scan on top of every stuck one and park the whole blocking-io + // pool within a couple of minutes, leaving the panel dead even after the mount came back. Parking one thread is the + // price of asking at all; parking the pool would cost us the panel exactly when storage is the thing going wrong. + private val mediaScanInFlight = new java.util.concurrent.atomic.AtomicBoolean(false) + // Schema -> city id, read from configuration alone. The database knows the schema, the directory is named for the // city, and nothing but this mapping joins them. ConfigService.availableCityIds would do it with one existence // query per city — the ~50-connection fan-out this dashboard exists to catch (#4559). @@ -375,17 +386,32 @@ class HealthServiceImpl @Inject() ( private def getMediaStorage: Future[Option[MediaStorageHealth]] = { cacheApi .getOrElseUpdate[Option[MediaStorageHealth]]("health.media", panoTtl) { - val scan = for { - dirs <- Future(MediaIntegrity.directoryStatuses(config, environment))(blockingIoEc) - integrity <- storyMediaIntegrity - } yield Some(MediaStorageHealth(dirs, environment.mode == play.api.Mode.Prod, integrity._1, integrity._2)) - withTimeout(scan, "media storage scan") + if (!mediaScanInFlight.compareAndSet(false, true)) { + // A scan is still running past its deadline, which all but always means a filesystem call that will not + // return. Reporting that beats both starting another one on top of it and rendering a stale all-clear. + Future.successful(Some(unreachableMedia("A previous media scan has not returned; storage may be offline."))) + } else { + val scan = for { + dirs <- Future(MediaIntegrity.directoryStatuses(config, environment))(blockingIoEc) + integrity <- storyMediaIntegrity + } yield Some(MediaStorageHealth(dirs, enforced, integrity._1, integrity._2)) + // Clears on the underlying scan, not on the timeout, so a stuck one keeps the gate shut until it unsticks. + scan.onComplete(_ => mediaScanInFlight.set(false)) + withTimeout(scan, "media storage scan") + } } .recover { case e: Exception => logger.warn(s"Health: failed to read media storage: ${e.getMessage}"); None } } + /** Whether the boot check arms on this instance; read from the check itself so the page can't claim a false guard. */ + private def enforced: Boolean = modules.PersistentMediaDirCheck.arms(environment) + + /** The panel with nothing but a reason on it, for when even stat-ing the directories would block. */ + private def unreachableMedia(reason: String): MediaStorageHealth = + MediaStorageHealth(Seq.empty, enforced, None, Some(reason)) + /** * Compares every visible city's `story_media` rows against the files on disk. * @@ -401,13 +427,19 @@ class HealthServiceImpl @Inject() ( // here would escape the caller's `.recover` instead of degrading to an unavailable panel. Future { val baseDir = MediaDirs.baseDir(config, environment, "story.media.directory") - (baseDir, baseDir.isDirectory) - }(blockingIoEc).flatMap { - case (baseDir, false) => + val refusal = // Nothing has been uploaded on this stage yet, or the directory is gone. Either way there is nothing to // compare against, and the directory panel above already reports the state of the path itself. - Future.successful((None, Some(s"No media directory at ${baseDir.getAbsolutePath} to scan."))) - case (baseDir, true) => + if (!baseDir.isDirectory) Some(s"No media directory at ${baseDir.getAbsolutePath} to scan.") + // `isDirectory` is true for a directory this process may not read, and every per-city listing beneath one + // comes back empty — which would report every story photo on the stage as destroyed. Decline instead: a + // monitor that cries data loss over a permissions change is worse than no monitor at all. + else if (!baseDir.canRead) Some(s"Media directory ${baseDir.getAbsolutePath} is not readable by this process.") + else None + (baseDir, refusal) + }(blockingIoEc).flatMap { + case (_, Some(refusal)) => Future.successful((None, Some(refusal))) + case (baseDir, None) => db.run(healthTable.getStoryMediaSchemas).map(_.filter(_.matches("^[A-Za-z0-9_]+$"))).flatMap { schemas => if (schemas.isEmpty) Future.successful((Some(StoryMediaIntegrity(baseDir.getAbsolutePath, Seq.empty, 0, 0)), None)) @@ -452,7 +484,7 @@ class HealthServiceImpl @Inject() ( ids: Seq[SchemaMediaId] ): Future[StoryMediaIntegrity] = { val idsBySchema = ids.groupBy(_.schema).view.mapValues(_.map(_.storyMediaId)).toMap - val cityDirs = MediaIntegrity.cityDirsBySchema( + val targets = MediaIntegrity.scanTargets( counts.map(_.schema), currentSchema, config.get[String]("city-id"), @@ -460,9 +492,13 @@ class HealthServiceImpl @Inject() ( ) Future { counts.sortBy(_.schema).map { case SchemaRowCount(schema, _) => - val cityId = cityDirs.get(schema) - val fileNames = cityId.flatMap(id => MediaIntegrity.listFileNames(new File(baseDir, id))) - MediaIntegrity.compareCity(cityId, schema, idsBySchema.getOrElse(schema, Seq.empty), fileNames) + val mediaIds = idsBySchema.getOrElse(schema, Seq.empty) + targets.get(schema) match { + case Some(ScanTarget.Dir(cityId)) => + MediaIntegrity.compareCity(cityId, schema, mediaIds, MediaIntegrity.listing(new File(baseDir, cityId))) + case Some(ScanTarget.Unlocatable(reason)) => MediaIntegrity.unscannedCity(None, schema, mediaIds, reason) + case None => MediaIntegrity.unscannedCity(None, schema, mediaIds, s"no scan target for schema $schema") + } } }(blockingIoEc).map { cities => StoryMediaIntegrity(baseDir.getAbsolutePath, cities, cities.map(_.missing).sum, cities.map(_.orphans).sum) diff --git a/app/service/MediaIntegrity.scala b/app/service/MediaIntegrity.scala index c94ec52ffe..153cea2bae 100644 --- a/app/service/MediaIntegrity.scala +++ b/app/service/MediaIntegrity.scala @@ -4,11 +4,43 @@ package service // check can never disagree about which directories matter or which of them is currently unsafe (#4925). import modules.PersistentMediaDirCheck import modules.PersistentMediaDirCheck.PersistentDir -import play.api.{Configuration, Environment, Mode} +import play.api.{Configuration, Environment} import java.io.File import scala.util.{Failure, Success, Try} +/** + * What listing one city's media directory found. + * + * `File.list` answers null for a directory that isn't there and for one this process may not read alike, and those + * mean opposite things to a data-loss monitor: nothing uploaded yet, versus no idea what is in there. Collapsing them + * would let a permissions change announce a whole city's photos as destroyed, so they stay distinct all the way to + * the page. + */ +sealed trait DirListing +object DirListing { + + /** The names the directory holds. */ + case class Listed(names: Seq[String]) extends DirListing + + /** No directory at that path: no upload has landed for this city yet, or one that had landed is gone. */ + case object Absent extends DirListing + + /** The path is there but unreadable, so nothing can be concluded about what it holds. */ + case object Unreadable extends DirListing +} + +/** Where one schema's story media lives on this instance, or why this instance can't say. */ +sealed trait ScanTarget +object ScanTarget { + + /** The city subdirectory holding that schema's media. */ + case class Dir(cityId: String) extends ScanTarget + + /** No directory can be attributed to the schema, phrased for the operator reading the panel. */ + case class Unlocatable(reason: String) extends ScanTarget +} + /** * The pure logic behind the Health dashboard's media-storage panel (#4926): what each persistent media directory * looks like from this instance, and which `story_media` rows have lost their bytes. @@ -34,9 +66,7 @@ object MediaIntegrity { * copy of the rules. */ def directoryStatuses(config: Configuration, environment: Environment): Seq[MediaDirStatus] = { - // The check arms only in prod mode, and in a dev checkout the relative defaults are supposed to land in the repo. - // Reporting those as failures would make the dev dashboard permanently red and teach everyone to ignore it. - val enforced = environment.mode == Mode.Prod + val enforced = PersistentMediaDirCheck.arms(environment) val unsafe = PersistentMediaDirCheck.unsafeDirs(config, environment).map(u => u.dir.key -> u.reason).toMap PersistentMediaDirCheck.persistentDirs.map { dir => @@ -44,29 +74,57 @@ object MediaIntegrity { case Failure(e) => status(dir, "—", "unresolved", "unusable value", "bad", Some(e.getMessage)) case Success(resolved) => - val path = resolved.getAbsolutePath - unsafe.get(dir.key) match { - case Some(reason) => - val severity = if (!enforced) "ok" else if (dir.irreplaceable) "bad" else "warn" - val label = if (enforced) "a deploy will delete this" else "inside the build tree (dev)" - status(dir, path, "unsafe", label, severity, Some(reason)) - // Not created yet is the normal state until the first upload — the write paths mkdirs on demand. - case None if !resolved.exists() => status(dir, path, "absent", "not created yet", "ok", None) - case None if !resolved.canWrite() => - status( - dir, - path, - "not_writable", - "not writable", - "bad", - Some(s"${dir.envVar} points at a path this process cannot write to, so uploads will fail.") - ) - case None => status(dir, path, "ok", "ok", "good", None) - } + val probe = DirProbe(resolved.exists(), resolved.canRead(), resolved.canWrite()) + dirStatus(dir, resolved.getAbsolutePath, probe, unsafe.get(dir.key), enforced) } } } + /** + * What a stat of one media directory saw — the only facts about it the status rules turn on. + * + * @param exists Whether anything is at the path. + * @param readable Whether this process may read it. + * @param writable Whether this process may write to it. + */ + case class DirProbe(exists: Boolean, readable: Boolean, writable: Boolean) + + /** + * What one directory's observed state means, separated from observing it so every branch is reachable from a spec: + * the permission branches can't be provoked from a test that runs as root, which CI and the dev container both do. + * + * @param dir The directory being judged. + * @param path Where it resolved, for display. + * @param probe What a stat of it saw. + * @param unsafeReason The boot check's objection to it, when it has one. + * @param enforced Whether the boot check arms on this instance. When it doesn't, a directory inside the build + * tree is the intended dev arrangement rather than a fault, and saying otherwise would make the + * dev dashboard permanently red and teach everyone to ignore it. + * @return The status, carrying its own display label and severity so the page holds no copy of the + * rules. + */ + private[service] def dirStatus( + dir: PersistentDir, + path: String, + probe: DirProbe, + unsafeReason: Option[String], + enforced: Boolean + ): MediaDirStatus = unsafeReason match { + case Some(reason) => + val severity = if (!enforced) "ok" else if (dir.irreplaceable) "bad" else "warn" + val label = if (enforced) "a deploy will delete this" else "inside the build tree (dev)" + status(dir, path, "unsafe", label, severity, Some(reason)) + // Not created yet is the normal state until the first upload — the write paths mkdirs on demand. + case None if !probe.exists => status(dir, path, "absent", "not created yet", "ok", None) + case None if !probe.readable => + val detail = s"${dir.envVar} points at a path this process cannot read, so nothing in it can be verified." + status(dir, path, "not_readable", "not readable", "bad", Some(detail)) + case None if !probe.writable => + val detail = s"${dir.envVar} points at a path this process cannot write to, so uploads will fail." + status(dir, path, "not_writable", "not writable", "bad", Some(detail)) + case None => status(dir, path, "ok", "ok", "good", None) + } + private def status( dir: PersistentDir, path: String, @@ -78,32 +136,41 @@ object MediaIntegrity { MediaDirStatus(dir.key, dir.envVar, dir.irreplaceable, path, key, label, severity, detail) /** - * Which city's subdirectory each schema's media lives in. + * Which city's subdirectory each schema's media lives in, or why it can't be located. * * This instance's own schema takes `city-id`, because that is what `StoryService` builds its write path from — * the schema and `city-id` are independent settings, so on an instance where they disagree the scan has to look * where the files actually are. That claim is exclusive: no second schema may be pointed at the same directory, - * or it would report the first schema's files as orphans. A schema left without a directory is reported unscanned, - * which is the truth — nothing here can say where its files are. + * or it would report the first schema's files as orphans. Every schema that loses a directory that way carries + * the reason with it, since "no city configured" and "this instance took that directory" send an operator looking + * in very different places. * * @param schemas Schemas the scan covers. * @param currentSchema The schema this instance reads and writes. * @param currentCity The `city-id` this instance writes media under. * @param configured Schema to city id, from configuration alone. - * @return Schema to city id, for the schemas whose directory can be located. + * @return One target per schema, in no particular order. */ - def cityDirsBySchema( + def scanTargets( schemas: Seq[String], currentSchema: String, currentCity: String, configured: Map[String, String] - ): Map[String, String] = { - val others = schemas - .filter(_ != currentSchema) - .flatMap(schema => configured.get(schema).filter(_ != currentCity).map(schema -> _)) - .toMap - if (schemas.contains(currentSchema)) others + (currentSchema -> currentCity) else others - } + ): Map[String, ScanTarget] = schemas.map { schema => + schema -> { + if (schema == currentSchema) ScanTarget.Dir(currentCity) + else + configured.get(schema) match { + case Some(`currentCity`) => + ScanTarget.Unlocatable( + s"this instance writes $currentCity media under schema $currentSchema, so the $currentCity directory " + + s"can't also be read as $schema" + ) + case Some(cityId) => ScanTarget.Dir(cityId) + case None => ScanTarget.Unlocatable(s"no city on this stage is configured to use schema $schema") + } + } + }.toMap /** * Compares one city's `story_media` rows against the files in its media directory. @@ -112,34 +179,62 @@ object MediaIntegrity { * failure: a retraction whose row delete landed and whose file delete did not, which leaves a photo on disk that * its author believes they deleted — the hard-delete contract stories were built on (#4054). * - * @param cityId City the schema belongs to, or None when this instance's config doesn't name it — then the - * directory can't be located and the row is reported unscanned rather than guessed at. - * @param schema Database schema the rows came from. - * @param mediaIds `story_media` ids in that schema. - * @param fileNames Names in the city's media directory, or None when the directory isn't there. An absent - * directory under a readable base is real loss, not an unknown, so its rows count as missing. - * @return Counts in both directions, with a short sample of ids to start looking from. + * @param cityId City the schema belongs to, which names the directory that was listed. + * @param schema Database schema the rows came from. + * @param mediaIds `story_media` ids in that schema. + * @param listing What the city's directory held. An absent directory is real loss — the write path creates it and + * never removes it — but an unreadable one proves nothing, so it reports unscanned instead. + * @return Counts in both directions, with a short sample of ids to start looking from. */ - def compareCity( - cityId: Option[String], - schema: String, - mediaIds: Seq[Int], - fileNames: Option[Seq[String]] - ): CityStoryMedia = { - val rows = mediaIds.distinct - if (cityId.isEmpty) { - CityStoryMedia(cityId, schema, rows.size, 0, 0, Seq.empty, Seq.empty, scanned = false) - } else { - val onDisk = - fileNames.getOrElse(Seq.empty).flatMap { case StoryMediaFileName(id) => id.toIntOption; case _ => None }.toSet - val rowSet = rows.toSet - val missing = (rowSet -- onDisk).toSeq.sorted - val orphans = (onDisk -- rowSet).toSeq.sorted - CityStoryMedia(cityId, schema, rows.size, missing.size, orphans.size, missing.take(MaxSampleIds), - orphans.take(MaxSampleIds), scanned = true) + def compareCity(cityId: String, schema: String, mediaIds: Seq[Int], listing: DirListing): CityStoryMedia = { + val rows = mediaIds.distinct.toSet + listing match { + case DirListing.Unreadable => + unscannedCity(Some(cityId), schema, mediaIds, s"the $cityId media directory is not readable by this process") + case DirListing.Absent => diff(cityId, schema, rows, Set.empty) + case DirListing.Listed(names) => + diff( + cityId, + schema, + rows, + names.flatMap { case StoryMediaFileName(id) => id.toIntOption; case _ => None }.toSet + ) } } - /** Lists a directory's file names, or None when it isn't a readable directory. Blocking; call on a blocking pool. */ - def listFileNames(dir: File): Option[Seq[String]] = Option(dir.list()).map(_.toSeq) + /** + * One city's row whose directory could not be scanned, so its counts stand for nothing and read as blank. + * + * @param cityId City the schema belongs to, when one is known. + * @param schema Database schema the rows came from. + * @param mediaIds `story_media` ids in that schema, which are still worth reporting as a row count. + * @param reason Why the scan couldn't run, phrased for the operator reading the panel. + */ + def unscannedCity(cityId: Option[String], schema: String, mediaIds: Seq[Int], reason: String): CityStoryMedia = + CityStoryMedia(cityId, schema, mediaIds.distinct.size, 0, 0, Seq.empty, Seq.empty, scanned = false, Some(reason)) + + private def diff(cityId: String, schema: String, rows: Set[Int], onDisk: Set[Int]): CityStoryMedia = { + val missing = (rows -- onDisk).toSeq.sorted + val orphans = (onDisk -- rows).toSeq.sorted + CityStoryMedia( + Some(cityId), schema, rows.size, missing.size, orphans.size, missing.take(MaxSampleIds), + orphans.take(MaxSampleIds), scanned = true, None + ) + } + + /** + * What a media directory holds, telling an absent directory apart from one this process may not read. + * + * Blocking; call on a blocking pool. + * + * @param dir Directory to list. + * @return Its file names, or which of the two null-answering states `File.list` was in. + */ + def listing(dir: File): DirListing = Option(dir.list()) match { + case Some(names) => DirListing.Listed(names.toSeq) + // `list` also answers null on an I/O error against a directory that is there, which is no more conclusive than a + // permissions refusal — so anything that exists but won't list is Unreadable rather than Absent. + case None if dir.exists() => DirListing.Unreadable + case None => DirListing.Absent + } } diff --git a/public/js/admin-dashboard/HealthPage.js b/public/js/admin-dashboard/HealthPage.js index 572399e064..3d5337e6e4 100644 --- a/public/js/admin-dashboard/HealthPage.js +++ b/public/js/admin-dashboard/HealthPage.js @@ -146,8 +146,10 @@ class HealthPage { } /** - * The missing-media KPI: destroyed bytes are the headline, so a directory that a deploy will delete counts as bad - * even before anything has been lost from it. Orphans are a lesser fault and only warn. + * The missing-media KPI. The value is only ever a count of missing files or "—" for unknown — this tile is labelled + * "Missing media files", so putting any other number under it would misreport. A directory a deploy will delete is + * bad news before anything has been lost from it, so it colors the tile red without supplying its number, and the + * table below names which directory. Orphans are a lesser fault and only warn. * * @param {?Object} media - The media_storage payload, or null when it couldn't be read. * @returns {[(string|number), string]} Value and tone, spread into #setKpi. @@ -156,7 +158,7 @@ class HealthPage { // Unknown is not healthy: tone it neutral rather than green. if (!media) return ['—', 'ok']; const unsafeDirs = (media.directories || []).filter((d) => d.severity === 'bad').length; - if (!media.story_media) return [unsafeDirs > 0 ? unsafeDirs : '—', unsafeDirs > 0 ? 'bad' : 'ok']; + if (!media.story_media) return ['—', unsafeDirs > 0 ? 'bad' : 'ok']; const { missing, orphans } = media.story_media; if (missing > 0 || unsafeDirs > 0) return [HealthPage.#compact(missing), 'bad']; return [HealthPage.#compact(missing), orphans > 0 ? 'warn' : 'good']; @@ -376,6 +378,15 @@ class HealthPage { return; } + // A scan that couldn't even stat the directories sends none, and an empty table would read as "none configured". + if (!media.directories?.length) { + const why = HealthPage.#esc(media.unavailable || 'Media storage status is unavailable.'); + this.#setHtml('health-media-dirs', `

${why}

`); + this.#setHtml('health-media-story', ''); + this.#setHtml('health-media-note', ''); + return; + } + const dirRows = (media.directories || []).map((d) => { const holds = d.irreplaceable ? 'content' : 'rebuildable'; const detail = d.detail ? `
${HealthPage.#esc(d.detail)}` : ''; @@ -440,7 +451,9 @@ class HealthPage { * @returns {string} Cell HTML. */ static #mediaDetail(city) { - if (!city.scanned) return `no city configured for schema ${HealthPage.#esc(city.schema)}`; + // The reason is server-side: "no city configured", "this instance writes that directory under another schema" + // and "not readable" send an operator to very different places, and only the backend knows which applies. + if (!city.scanned) return `${HealthPage.#esc(city.unscanned_reason || 'not scanned')}`; const parts = []; if (city.missing_ids?.length) parts.push(`missing ${city.missing_ids.join(', ')}`); if (city.orphan_ids?.length) parts.push(`orphaned ${city.orphan_ids.join(', ')}`); diff --git a/test/controllers/ImageControllerSpec.scala b/test/controllers/ImageControllerSpec.scala index b1155c89e0..2bc63b70a9 100644 --- a/test/controllers/ImageControllerSpec.scala +++ b/test/controllers/ImageControllerSpec.scala @@ -162,8 +162,8 @@ class ImageControllerSpec extends PlaySpec with AnonSession with GuiceOneAppPerS cleanUp(syntheticLabelId) } - "answer a signed pano URL with no backup image with a plain 404, and stay quiet on a repeat" in { - // Two requests: the log deduplicates, the responses must not. + "answer a signed pano URL with no backup image with a plain 404, every time it is asked" in { + // The log deduplicates a repeat (LostMediaLogSpec pins that); the responses must not. val panoId = "sidewalkSpecNoSuchPano4926" val url = signingService.signedUrl(s"/backupImage/$panoId") status(route(app, FakeRequest(GET, url)).get) mustBe NOT_FOUND diff --git a/test/service/HealthServiceSpec.scala b/test/service/HealthServiceSpec.scala index 998a2a4e86..225281fc4d 100644 --- a/test/service/HealthServiceSpec.scala +++ b/test/service/HealthServiceSpec.scala @@ -121,7 +121,12 @@ class HealthServiceSpec extends PlaySpec with GuiceOneAppPerSuite { case Some(scan) => scan.missing mustBe scan.cities.map(_.missing).sum scan.orphans mustBe scan.cities.map(_.orphans).sum - scan.cities.filterNot(_.scanned).foreach(_.missing mustBe 0) + // An unscanned city's counts stand for nothing, and the row has to say which of the several reasons put it + // there or the operator reading it has nowhere to start. + scan.cities.filterNot(_.scanned).foreach { city => + city.missing mustBe 0 + city.unscannedReason mustBe defined + } } } diff --git a/test/service/LostMediaLogSpec.scala b/test/service/LostMediaLogSpec.scala new file mode 100644 index 0000000000..41722aee7b --- /dev/null +++ b/test/service/LostMediaLogSpec.scala @@ -0,0 +1,100 @@ +package service + +import ch.qos.logback.classic.spi.ILoggingEvent +import ch.qos.logback.classic.{Level, Logger => LogbackLogger} +import ch.qos.logback.core.read.ListAppender +import org.scalatestplus.play.PlaySpec +import org.slf4j.LoggerFactory + +import scala.jdk.CollectionConverters._ + +/** + * The one thing standing between a destroyed file and nobody finding out for six days (#4925). + * + * Its whole contract is what it says and how often — and both failure modes are silent. Announcing every request + * buries the loss in its own noise; announcing nothing is indistinguishable from health, which is the state #4926 + * exists to end. So the log lines themselves are the assertions here. + */ +class LostMediaLogSpec extends PlaySpec { + + /** + * Runs `f` against a fresh log with an appender attached, and returns everything it logged. + * + * The level is forced rather than inherited so the spec pins the code's own severity choice instead of whatever + * the ambient logback config happens to permit. + */ + private def captured(f: LostMediaLog => Unit): Seq[ILoggingEvent] = { + val logger = LoggerFactory.getLogger(classOf[LostMediaLog]).asInstanceOf[LogbackLogger] + val appender = new ListAppender[ILoggingEvent] + val original = logger.getLevel + appender.start() + logger.addAppender(appender) + logger.setLevel(Level.WARN) + // Detach from the console for the duration: these cases deliberately report thousands of losses, and a suite that + // buries its own output in fake alarms is the noise problem this class exists to avoid, one level up. + logger.setAdditive(false) + try f(new LostMediaLog) + finally { + logger.setAdditive(true) + logger.setLevel(original) + logger.detachAppender(appender) + appender.stop() + } + appender.list.asScala.toSeq + } + + "LostMediaLog" should { + "say what was lost and where its bytes should have been" in { + val events = captured(_.reportMissing("story_media", "331", "/srv/media/chicago-il/story_331.jpg", true)) + events must have size 1 + val message = events.head.getFormattedMessage + message must include("story_media") + message must include("331") + message must include("/srv/media/chicago-il/story_331.jpg") + } + + "announce a lost item once, however many times it is requested" in { + // One popular page re-requesting a lost file would otherwise write the same line thousands of times and bury + // the event in its own noise. + val events = captured { log => + (1 to 25).foreach(_ => log.reportMissing("story_media", "331", "/srv/media/story_331.jpg", true)) + } + events must have size 1 + } + + "announce every distinct item, so a whole directory going missing reads as a whole directory" in { + val events = captured { log => + Seq("331", "332", "333").foreach(id => log.reportMissing("story_media", id, s"/srv/media/$id.jpg", true)) + } + events.map(_.getFormattedMessage).mkString(" ") must include("333") + events must have size 3 + } + + "keep kinds apart, since a story and a pano can share an id" in { + val events = captured { log => + log.reportMissing("story_media", "1", "/srv/media/story_1.jpg", irreplaceable = true) + log.reportMissing("pano", "1", "/srv/panos/1.jpg", irreplaceable = true) + } + events must have size 2 + } + + "log content no rebuild can recreate at ERROR, and rebuildable content at WARN" in { + // The tiering is the same call PersistentMediaDirCheck makes, and it is what decides whether anyone is paged. + val error = captured(_.reportMissing("pano", "abc", "/srv/panos/abc.jpg", irreplaceable = true)) + error.head.getLevel mustBe Level.ERROR + val warn = captured(_.reportMissing("crop", "CurbRamp/7", "/srv/crops/crop_7.png", irreplaceable = false)) + warn.head.getLevel mustBe Level.WARN + } + + "let a still-lost item announce itself again once it has been crowded out" in { + // The tracking set is bounded, because a dead mount can make every pano in a city report at once. Eviction + // eventually re-announcing a loss is the right way for that bound to fail; going permanently quiet is not. + val events = captured { log => + log.reportMissing("pano", "first", "/srv/panos/first.jpg", irreplaceable = true) + (1 to 5000).foreach(i => log.reportMissing("pano", s"filler$i", s"/srv/panos/$i.jpg", irreplaceable = true)) + log.reportMissing("pano", "first", "/srv/panos/first.jpg", irreplaceable = true) + } + events.last.getFormattedMessage must include("pano first ") + } + } +} diff --git a/test/service/MediaIntegritySpec.scala b/test/service/MediaIntegritySpec.scala index 81fb9ab7ab..0d0b93877d 100644 --- a/test/service/MediaIntegritySpec.scala +++ b/test/service/MediaIntegritySpec.scala @@ -87,58 +87,95 @@ class MediaIntegritySpec extends PlaySpec { } } + private val storyDir = persistentDirs.find(_.irreplaceable).value + + // The permission branches can't be provoked through the filesystem from a suite that runs as root — which CI and + // the dev container both do, and where chmod 000 still reads and writes fine — so they are pinned on the rules + // themselves. + "dirStatus" should { + "call a directory this process cannot read bad, since nothing in it can be verified" in { + val probe = MediaIntegrity.DirProbe(exists = true, readable = false, writable = true) + val status = MediaIntegrity.dirStatus(storyDir, "/srv/media", probe, None, enforced = true) + status.status mustBe "not_readable" + status.severity mustBe "bad" + status.detail.value must include(storyDir.envVar) + } + + "call a directory this process cannot write to bad, since uploads will fail against it" in { + val probe = MediaIntegrity.DirProbe(exists = true, readable = true, writable = false) + val status = MediaIntegrity.dirStatus(storyDir, "/srv/media", probe, None, enforced = true) + status.status mustBe "not_writable" + status.severity mustBe "bad" + } + + "report an unsafe directory before either permission, since a deploy deleting it outranks both" in { + val probe = MediaIntegrity.DirProbe(exists = true, readable = false, writable = false) + val status = MediaIntegrity.dirStatus(storyDir, "/srv/media", probe, Some("in the wipe zone"), enforced = true) + status.status mustBe "unsafe" + } + } + private val configuredCities = Map("sidewalk_chicago" -> "chicago-il", "sidewalk_seattle" -> "seattle-wa") - "cityDirsBySchema" should { + /** The reason a schema has no directory, failing the test if it turned out to have one. */ + private def unlocatableReason(target: ScanTarget): String = target match { + case ScanTarget.Unlocatable(reason) => reason + case ScanTarget.Dir(cityId) => fail(s"expected no directory for this schema, got $cityId") + } + + "scanTargets" should { "use the configured city for every schema when nothing disagrees" in { - MediaIntegrity.cityDirsBySchema( + MediaIntegrity.scanTargets( Seq("sidewalk_chicago", "sidewalk_seattle"), "sidewalk_chicago", "chicago-il", configuredCities - ) mustBe configuredCities + ) mustBe Map( + "sidewalk_chicago" -> ScanTarget.Dir("chicago-il"), + "sidewalk_seattle" -> ScanTarget.Dir("seattle-wa") + ) } "follow this instance's own city-id, since that is what the write path builds its path from" in { // A dev container left with city-id and the connection's schema disagreeing writes media under the city-id. - val dirs = MediaIntegrity.cityDirsBySchema( + val targets = MediaIntegrity.scanTargets( Seq("sidewalk_chicago", "sidewalk_seattle"), "sidewalk_chicago", "seattle-wa", configuredCities ) - dirs.get("sidewalk_chicago") mustBe Some("seattle-wa") + targets("sidewalk_chicago") mustBe ScanTarget.Dir("seattle-wa") } "not let a second schema claim a directory this instance already writes to" in { // Otherwise the other schema lists the same files and reports every one of them as an orphan. - val dirs = MediaIntegrity.cityDirsBySchema( + val targets = MediaIntegrity.scanTargets( Seq("sidewalk_chicago", "sidewalk_seattle"), "sidewalk_chicago", "seattle-wa", configuredCities ) - dirs.get("sidewalk_seattle") mustBe None + // And it has to say which of the two reasons applies: the operator staring at this row on a misconfigured + // container needs to know the city is configured and its directory was taken, not go hunting for a config gap. + val reason = unlocatableReason(targets("sidewalk_seattle")) + reason must include("sidewalk_chicago") + reason must include("seattle-wa") } - "leave out a schema no configured city names, so it is reported unscanned rather than guessed at" in { - MediaIntegrity.cityDirsBySchema( - Seq("sidewalk_elsewhere"), - "sidewalk_chicago", - "chicago-il", - configuredCities - ) mustBe - Map.empty + "say plainly when no city on the stage names a schema, rather than guessing at its directory" in { + val targets = + MediaIntegrity.scanTargets(Seq("sidewalk_elsewhere"), "sidewalk_chicago", "chicago-il", configuredCities) + unlocatableReason(targets("sidewalk_elsewhere")) must include("sidewalk_elsewhere") } } "compareCity" should { "report a city whose rows all have files as clean" in { val result = MediaIntegrity.compareCity( - Some("chicago-il"), + "chicago-il", "sidewalk_chicago", Seq(1, 2), - Some(Seq("story_1.jpg", "story_2.jpg")) + DirListing.Listed(Seq("story_1.jpg", "story_2.jpg")) ) result.missing mustBe 0 result.orphans mustBe 0 @@ -148,7 +185,7 @@ class MediaIntegritySpec extends PlaySpec { "report a row with no file as missing — the #4925 loss" in { val result = - MediaIntegrity.compareCity(Some("chicago-il"), "sidewalk_chicago", Seq(1, 2), Some(Seq("story_1.jpg"))) + MediaIntegrity.compareCity("chicago-il", "sidewalk_chicago", Seq(1, 2), DirListing.Listed(Seq("story_1.jpg"))) result.missing mustBe 1 result.missingIds mustBe Seq(2) result.orphans mustBe 0 @@ -156,10 +193,10 @@ class MediaIntegritySpec extends PlaySpec { "report a file with no row as orphaned — a retraction whose file delete didn't land" in { val result = MediaIntegrity.compareCity( - Some("chicago-il"), + "chicago-il", "sidewalk_chicago", Seq(1), - Some(Seq("story_1.jpg", "story_7.jpg")) + DirListing.Listed(Seq("story_1.jpg", "story_7.jpg")) ) result.orphans mustBe 1 result.orphanIds mustBe Seq(7) @@ -167,50 +204,72 @@ class MediaIntegritySpec extends PlaySpec { } "count both directions at once, which a row-count comparison alone would call clean" in { - val result = MediaIntegrity.compareCity(Some("chicago-il"), "sidewalk_chicago", Seq(1), Some(Seq("story_9.jpg"))) + val result = + MediaIntegrity.compareCity("chicago-il", "sidewalk_chicago", Seq(1), DirListing.Listed(Seq("story_9.jpg"))) result.missing mustBe 1 result.orphans mustBe 1 } "ignore files that aren't story media, so a stray README isn't reported as an orphan" in { val result = MediaIntegrity.compareCity( - Some("chicago-il"), + "chicago-il", "sidewalk_chicago", Seq(1), - Some(Seq("story_1.jpg", "README.txt", "story_1.jpg.bak", "story_.jpg")) + DirListing.Listed(Seq("story_1.jpg", "README.txt", "story_1.jpg.bak", "story_.jpg")) ) result.orphans mustBe 0 result.missing mustBe 0 } - "treat an absent city directory under a readable base as loss, not as an unknown" in { - val result = MediaIntegrity.compareCity(Some("chicago-il"), "sidewalk_chicago", Seq(1, 2), None) + "treat an absent city directory as loss, since the write path creates it and never removes it" in { + val result = MediaIntegrity.compareCity("chicago-il", "sidewalk_chicago", Seq(1, 2), DirListing.Absent) result.missing mustBe 2 result.scanned mustBe true } "report a city with no rows and no directory as clean rather than as a fault" in { - val result = MediaIntegrity.compareCity(Some("chicago-il"), "sidewalk_chicago", Seq.empty, None) + val result = MediaIntegrity.compareCity("chicago-il", "sidewalk_chicago", Seq.empty, DirListing.Absent) result.missing mustBe 0 result.orphans mustBe 0 result.scanned mustBe true } - "decline to guess when a schema maps to no configured city, since its directory can't be located" in { - val result = MediaIntegrity.compareCity(None, "sidewalk_somewhere", Seq(1, 2), None) + "decline to call an unreadable directory data loss, and say why it declined" in { + // A directory this process may not read holds exactly the same photos it held a moment ago. Counting all of + // them as destroyed would put the fleet's whole story archive on the panel as lost over a permissions change, + // and a monitor that does that once gets ignored forever after. + val result = MediaIntegrity.compareCity("chicago-il", "sidewalk_chicago", Seq(1, 2), DirListing.Unreadable) result.scanned mustBe false result.missing mustBe 0 result.rows mustBe 2 + result.unscannedReason.value must include("not readable") + } + } + + "unscannedCity" should { + "still report the rows it knows about, so the city doesn't read as empty" in { + val result = MediaIntegrity.unscannedCity(None, "sidewalk_somewhere", Seq(1, 2), "no city names this schema") + result.scanned mustBe false + result.rows mustBe 2 + result.missing mustBe 0 + result.unscannedReason.value mustBe "no city names this schema" } } - "listFileNames" should { + "listing" should { "read a directory's contents" in { - MediaIntegrity.listFileNames(dirContaining("story_3.jpg")).value must contain("story_3.jpg") + MediaIntegrity.listing(dirContaining("story_3.jpg")) mustBe DirListing.Listed(Seq("story_3.jpg")) + } + + "call a path with nothing at it absent, which for a city directory means no upload has landed" in { + MediaIntegrity.listing(new File("/srv/sidewalk-media/no-such-directory")) mustBe DirListing.Absent } - "return nothing for a path that isn't a readable directory, so the caller can tell it apart from an empty one" in { - MediaIntegrity.listFileNames(new File("/srv/sidewalk-media/no-such-directory")) mustBe None + "call something that is there but won't list unreadable rather than absent" in { + // `File.list` answers null for both, and the two are opposite verdicts: nothing was ever written here, versus + // this process cannot see what is here. A regular file is the case a root-running suite can actually provoke. + val notADirectory = new File(dirContaining("story_3.jpg"), "story_3.jpg") + MediaIntegrity.listing(notADirectory) mustBe DirListing.Unreadable } } } From ff9dda24e0d447333e9d3f8f7b8139f5b3e1cc1d Mon Sep 17 00:00:00 2001 From: jonfroehlich Date: Wed, 19 Aug 2026 22:23:16 -0700 Subject: [PATCH 4/8] Make the media storage panel readable (#4926) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- app/views/admin/dashboard/health.scala.html | 2 +- .../css/admin-dashboard/admin-dashboard.css | 9 ++++++ public/js/admin-dashboard/HealthPage.js | 29 ++++++++++++------- 3 files changed, 29 insertions(+), 11 deletions(-) diff --git a/app/views/admin/dashboard/health.scala.html b/app/views/admin/dashboard/health.scala.html index 8d02512470..d15e68ed0a 100644 --- a/app/views/admin/dashboard/health.scala.html +++ b/app/views/admin/dashboard/health.scala.html @@ -66,7 +66,7 @@

DB connections

Media storage

-

Where this stage keeps the media that has to outlive a deploy, and whether any of it has gone missing. A deploy rebuilds the whole build tree, so anything stored inside it is deleted by the next release while its database rows survive — the failure that destroyed a story photo in #4925. Missing means a story_media row whose file is gone; orphaned means a file whose row is gone, which is a retraction that only half landed.

+

Media that must outlive a deploy, and whether any of it is gone. A deploy rebuilds the whole build tree, so anything kept inside it dies with the next release while its database rows survive — how #4925 destroyed a story photo. Missing is a row with no file; orphaned is a file with no row.

diff --git a/public/css/admin-dashboard/admin-dashboard.css b/public/css/admin-dashboard/admin-dashboard.css index 0a9318d677..f736f697b7 100644 --- a/public/css/admin-dashboard/admin-dashboard.css +++ b/public/css/admin-dashboard/admin-dashboard.css @@ -1974,6 +1974,15 @@ img.activity-feed-thumb:hover { box-shadow: 0 2px 6px rgb(0 0 0 / 18%); } } .ac-table tbody tr:hover { background: var(--color-neutral-100); } +/* A filesystem path is the only unbounded string in these tables, and a deploy-tree path is long enough to push the + Status column past the edge — the one column an operator is reading. Let a path break rather than scroll; scoped to + the path cell so the config keys and variable names beside it stay whole. */ +.ac-table td.ac-path code { + /* The shared `code` styling is nowrap, which suppresses wrapping outright — overflow-wrap alone can't act. */ + white-space: normal; + overflow-wrap: anywhere; +} + .ac-num { text-align: right; white-space: nowrap; diff --git a/public/js/admin-dashboard/HealthPage.js b/public/js/admin-dashboard/HealthPage.js index 3d5337e6e4..55c0ce31d9 100644 --- a/public/js/admin-dashboard/HealthPage.js +++ b/public/js/admin-dashboard/HealthPage.js @@ -388,18 +388,23 @@ class HealthPage { } const dirRows = (media.directories || []).map((d) => { - const holds = d.irreplaceable ? 'content' : 'rebuildable'; - const detail = d.detail ? `
${HealthPage.#esc(d.detail)}` : ''; + const lost = d.irreplaceable + ? 'gone for good' + : 'rebuildable'; + // The wipe-zone reason names the key and the path, which are already columns, and then repeats the same + // sentence about deploys on every row — four copies of it in the narrowest column is what made these rows six + // times taller than every other table on the page. The badge says the state; the note below says the fix once. + const detail = d.detail && d.status !== 'unsafe' ? `
${HealthPage.#esc(d.detail)}` : ''; return ` ${HealthPage.#esc(d.key)} ${HealthPage.#esc(d.env_var)} - ${holds} - ${HealthPage.#esc(d.path)} + ${lost} + ${HealthPage.#esc(d.path)} ${HealthPage.#esc(d.label)}${detail} `; }).join(''); - this.#table('health-media-dirs', ['Config key', 'Env var', 'Holds', 'Resolves to', 'Status'], dirRows); + this.#table('health-media-dirs', ['Config key', 'Env var', 'If lost', 'Resolves to', 'Status'], dirRows); const scan = media.story_media; if (!scan) { @@ -417,15 +422,19 @@ class HealthPage { ${HealthPage.#mediaDetail(c)} `).join(''); this.#table('health-media-story', - ['City', ['Media rows', true], ['Missing', true], ['Orphaned', true], 'Ids'], rows); + ['City', ['Media rows', true], ['Missing', true], ['Orphaned', true], 'Notes'], rows); } const notes = [`Story media lives under ${HealthPage.#esc(scan ? scan.base_dir : '—')}, one - subdirectory per city, so this covers every city deployed on this stage — a city hosted elsewhere would read as - unscanned.`]; + subdirectory per city; a city hosted elsewhere reads as unscanned.`]; + // Said once here rather than repeated in every unsafe row, which is where it used to live. + if (media.directories.some((d) => d.status === 'unsafe')) { + notes.push(`A directory inside the build tree is deleted by the next release: point its environment variable at + storage outside the application (docs/deployment-and-stages.md).`); + } if (!media.enforced) { - notes.push(`This instance is not running in production mode, so the boot check that refuses to start on an - unsafe directory is inactive here and the relative defaults landing in the checkout are expected.`); + notes.push(`Not production mode, so that boot check is inactive here and the relative defaults landing in the + checkout are expected.`); } this.#setHtml('health-media-note', notes.join(' ')); } From 8930626c52bc09a47f04fe80e6b0fd31ff161e06 Mon Sep 17 00:00:00 2001 From: jonfroehlich Date: Thu, 20 Aug 2026 06:35:53 -0700 Subject: [PATCH 5/8] Cover the media-loss guardrails with tests (#4926) 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) --- .github/workflows/ci.yml | 11 +- app/service/HealthService.scala | 46 ++-- app/service/MediaIntegrity.scala | 21 ++ app/service/SingleFlightGate.scala | 47 ++++ test/controllers/ImageControllerSpec.scala | 94 ++++++- test/js/healthMediaPanel.test.js | 287 +++++++++++++++++++++ test/service/HealthMediaPayloadSpec.scala | 95 +++++++ test/service/HealthServiceSpec.scala | 81 +++++- test/service/MediaIntegritySpec.scala | 22 ++ test/service/SingleFlightGateSpec.scala | 90 +++++++ 10 files changed, 751 insertions(+), 43 deletions(-) create mode 100644 app/service/SingleFlightGate.scala create mode 100644 test/js/healthMediaPanel.test.js create mode 100644 test/service/HealthMediaPayloadSpec.scala create mode 100644 test/service/SingleFlightGateSpec.scala diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1e2796f0c0..5114674c57 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -310,8 +310,17 @@ jobs: # - LostMediaLogSpec (#4926): the log line that is the only signal a media file has been destroyed. Both ways # it can fail are silent — announcing every request buries the event, announcing nothing reads as health — # so the lines themselves are asserted. Pure logic, no DB. + # - SingleFlightGateSpec (#4926): keeps a stuck media scan from being joined by a fresh copy every poll until + # the blocking-io pool is gone. Its whole job is invisible until storage hangs, and by then the panel that + # would have reported it is the thing that went dark. Pure logic, no DB. + # - HealthMediaPayloadSpec (#4926): the field names the health page reads the media panel by. A renamed case + # class field emits a different key, every value goes undefined, and a monitor reporting nothing looks + # exactly like a monitor reporting nothing wrong. Pure logic, no DB. + # - ImageControllerSpec (#4415, #4726, #4926): the crop write path and its share-preview invalidation, plus the + # loss lines a signed crop/pano URL emits when the bytes it was signed for are gone. Nothing else asserts the + # controller calls LostMediaLog at all, so without this the tripwire could be deleted with CI still green. - name: Run gating/auth tests (health dashboard + route auth posture + geodesic distances) - run: sbt 'set Test / parallelExecution := false' 'testOnly controllers.HealthDashboardSpec service.HealthServiceSpec controllers.RouteAuthPostureSpec models.street.GeodesicDistanceSpec service.ExploreTutorialRouteSpec controllers.MobileDetectionSpec service.PanoDataServiceSpec models.utils.ConfigTableVoidedArchiveSpec controllers.api.StatsApiSpec controllers.ExploreSubmissionSpec controllers.ValidateSubmissionSpec modules.PersistentMediaDirCheckSpec service.MediaIntegritySpec service.LostMediaLogSpec' + run: sbt 'set Test / parallelExecution := false' 'testOnly controllers.HealthDashboardSpec service.HealthServiceSpec controllers.RouteAuthPostureSpec models.street.GeodesicDistanceSpec service.ExploreTutorialRouteSpec controllers.MobileDetectionSpec service.PanoDataServiceSpec models.utils.ConfigTableVoidedArchiveSpec controllers.api.StatsApiSpec controllers.ExploreSubmissionSpec controllers.ValidateSubmissionSpec modules.PersistentMediaDirCheckSpec service.MediaIntegritySpec service.LostMediaLogSpec service.SingleFlightGateSpec service.HealthMediaPayloadSpec controllers.ImageControllerSpec' env: DATABASE_URL: jdbc:postgresql://localhost:5432/sidewalk DATABASE_USER: sidewalk diff --git a/app/service/HealthService.scala b/app/service/HealthService.scala index a321fc7180..8cd44366ed 100644 --- a/app/service/HealthService.scala +++ b/app/service/HealthService.scala @@ -361,12 +361,9 @@ class HealthServiceImpl @Inject() ( // against a dead mount never returns — the thread stays parked on the blocking pool, but the poll must not. private val mediaScanTimeout: FiniteDuration = 5.seconds - // One scan at a time. `withTimeout` abandons the future it gave up waiting on, but nothing can cancel the thread - // underneath it: a filesystem call against an unreachable mount returns when the mount does, or never. Without this - // guard the dashboard's ~20s poll would stack a fresh scan on top of every stuck one and park the whole blocking-io - // pool within a couple of minutes, leaving the panel dead even after the mount came back. Parking one thread is the - // price of asking at all; parking the pool would cost us the panel exactly when storage is the thing going wrong. - private val mediaScanInFlight = new java.util.concurrent.atomic.AtomicBoolean(false) + // One scan at a time: `withTimeout` protects the poll from a stuck filesystem call, but only this keeps a stuck one + // from being joined by a fresh copy every poll until the whole blocking-io pool is parked. See [[SingleFlightGate]]. + private val mediaScanGate = new SingleFlightGate // Schema -> city id, read from configuration alone. The database knows the schema, the directory is named for the // city, and nothing but this mapping joins them. ConfigService.availableCityIds would do it with one existence @@ -386,19 +383,19 @@ class HealthServiceImpl @Inject() ( private def getMediaStorage: Future[Option[MediaStorageHealth]] = { cacheApi .getOrElseUpdate[Option[MediaStorageHealth]]("health.media", panoTtl) { - if (!mediaScanInFlight.compareAndSet(false, true)) { - // A scan is still running past its deadline, which all but always means a filesystem call that will not - // return. Reporting that beats both starting another one on top of it and rendering a stale all-clear. - Future.successful(Some(unreachableMedia("A previous media scan has not returned; storage may be offline."))) - } else { - val scan = for { - dirs <- Future(MediaIntegrity.directoryStatuses(config, environment))(blockingIoEc) - integrity <- storyMediaIntegrity - } yield Some(MediaStorageHealth(dirs, enforced, integrity._1, integrity._2)) - // Clears on the underlying scan, not on the timeout, so a stuck one keeps the gate shut until it unsticks. - scan.onComplete(_ => mediaScanInFlight.set(false)) - withTimeout(scan, "media storage scan") - } + // A scan still running past its deadline all but always means a filesystem call that will not return. + // Reporting that beats both starting another one on top of it and rendering a stale all-clear. + val busy = Some(unreachableMedia("A previous media scan has not returned; storage may be offline.")) + // The gate opens on the underlying scan, not on the timeout, so a stuck one keeps the next poll out. + withTimeout( + mediaScanGate.runOrElse(busy) { + for { + dirs <- Future(MediaIntegrity.directoryStatuses(config, environment))(blockingIoEc) + integrity <- storyMediaIntegrity + } yield Some(MediaStorageHealth(dirs, enforced, integrity._1, integrity._2)) + }, + "media storage scan" + ) } .recover { case e: Exception => logger.warn(s"Health: failed to read media storage: ${e.getMessage}"); None @@ -427,16 +424,7 @@ class HealthServiceImpl @Inject() ( // here would escape the caller's `.recover` instead of degrading to an unavailable panel. Future { val baseDir = MediaDirs.baseDir(config, environment, "story.media.directory") - val refusal = - // Nothing has been uploaded on this stage yet, or the directory is gone. Either way there is nothing to - // compare against, and the directory panel above already reports the state of the path itself. - if (!baseDir.isDirectory) Some(s"No media directory at ${baseDir.getAbsolutePath} to scan.") - // `isDirectory` is true for a directory this process may not read, and every per-city listing beneath one - // comes back empty — which would report every story photo on the stage as destroyed. Decline instead: a - // monitor that cries data loss over a permissions change is worse than no monitor at all. - else if (!baseDir.canRead) Some(s"Media directory ${baseDir.getAbsolutePath} is not readable by this process.") - else None - (baseDir, refusal) + (baseDir, MediaIntegrity.scanRefusal(baseDir.getAbsolutePath, baseDir.isDirectory, baseDir.canRead)) }(blockingIoEc).flatMap { case (_, Some(refusal)) => Future.successful((None, Some(refusal))) case (baseDir, None) => diff --git a/app/service/MediaIntegrity.scala b/app/service/MediaIntegrity.scala index 153cea2bae..5a95315adc 100644 --- a/app/service/MediaIntegrity.scala +++ b/app/service/MediaIntegrity.scala @@ -135,6 +135,27 @@ object MediaIntegrity { ): MediaDirStatus = MediaDirStatus(dir.key, dir.envVar, dir.irreplaceable, path, key, label, severity, detail) + /** + * Whether the story-media scan can run against this base directory at all, and if not, what to say instead. + * + * Both refusals report the scan as unavailable rather than as loss, and the second is the one that is easy to get + * wrong: `isDirectory` answers true for a directory this process may not read, and every per-city listing beneath + * an unreadable base comes back empty — which would announce every story photo on the stage as destroyed. A monitor + * that cries data loss over a permissions change gets muted, and a muted monitor leaves us where #4925 found us. + * + * @param path Where the base directory resolved, named in the message so an operator knows what to look at. + * @param isDirectory Whether a directory is there. + * @param canRead Whether this process may read it. + * @return The reason the scan declined, or None when it can proceed. + */ + def scanRefusal(path: String, isDirectory: Boolean, canRead: Boolean): Option[String] = { + // Nothing has been uploaded on this stage yet, or the directory is gone. Either way there is nothing to compare + // against, and the directory panel already reports the state of the path itself. + if (!isDirectory) Some(s"No media directory at $path to scan.") + else if (!canRead) Some(s"Media directory $path is not readable by this process.") + else None + } + /** * Which city's subdirectory each schema's media lives in, or why it can't be located. * diff --git a/app/service/SingleFlightGate.scala b/app/service/SingleFlightGate.scala new file mode 100644 index 0000000000..29e1cfb9d0 --- /dev/null +++ b/app/service/SingleFlightGate.scala @@ -0,0 +1,47 @@ +package service + +import java.util.concurrent.atomic.AtomicBoolean +import scala.concurrent.{ExecutionContext, Future} +import scala.util.control.NonFatal + +/** + * Lets one piece of work run at a time, answering everyone who arrives while it is running with a stand-in value + * instead of starting a second copy. + * + * A deadline and a gate solve different halves of the same problem, and only the deadline is obvious. `withTimeout` + * protects the *caller* — it stops one slow call from holding a poll open — but it cannot cancel the work it gave up + * waiting on: a filesystem call against an unreachable mount returns when the mount does, or never. So a poller that + * only had a deadline would start a fresh copy every cycle, and each one would park a thread that never comes back; + * the pool is gone within minutes, and the panel that exists to report storage trouble stays dead long after the + * mount recovers. The gate protects the *pool*: parking one thread is the price of asking at all, parking all of them + * is not. + * + * The gate opens when the underlying work completes, never when a caller stops waiting for it — that distinction is + * the whole point, and is what [[service.SingleFlightGateSpec]] pins. + */ +class SingleFlightGate { + + private val inFlight = new AtomicBoolean(false) + + /** + * Runs `work` if nothing is already running, and otherwise answers `busy` without starting anything. + * + * @param busy Value to answer callers who arrive while work is in flight. By-name, so building it costs nothing on + * the common path. + * @param work The work to run. Failing — or throwing before it returns a future at all — reopens the gate, since a + * failure is a completion: only work that is genuinely still running should keep the next caller out. + * @return The work's own future, so a caller can time it out without affecting the gate. + */ + def runOrElse[T](busy: => T)(work: => Future[T]): Future[T] = { + if (!inFlight.compareAndSet(false, true)) Future.successful(busy) + else { + val started = + try work + catch { case NonFatal(e) => Future.failed(e) } + // parasitic: reopening the gate is a flag write, so it runs on whichever thread completed the work rather than + // waiting for a scheduler that a saturated pool may not get to. + started.onComplete(_ => inFlight.set(false))(ExecutionContext.parasitic) + started + } + } +} diff --git a/test/controllers/ImageControllerSpec.scala b/test/controllers/ImageControllerSpec.scala index 2bc63b70a9..67a24acad1 100644 --- a/test/controllers/ImageControllerSpec.scala +++ b/test/controllers/ImageControllerSpec.scala @@ -14,10 +14,16 @@ import models.label.LabelTypeEnum import service.{ImageSigningService, PanoDataService, ShareImageCache} import util.AnonSession +import ch.qos.logback.classic.spi.ILoggingEvent +import ch.qos.logback.classic.{Level, Logger => LogbackLogger} +import ch.qos.logback.core.read.ListAppender +import org.slf4j.LoggerFactory + import java.awt.image.BufferedImage import java.io.{ByteArrayOutputStream, File} import java.util.Base64 import javax.imageio.ImageIO +import scala.jdk.CollectionConverters._ /** * Functional tests for `POST /saveImage` (#4415, #4726). Boots the real app and drives the endpoint over HTTP through @@ -146,28 +152,94 @@ class ImageControllerSpec extends PlaySpec with AnonSession with GuiceOneAppPerS } } + /** + * Runs `f` with an appender on `LostMediaLog`'s logger and returns every line it wrote. + * + * The level is forced rather than inherited so these cases pin the controller's own severity choice instead of + * whatever the ambient logback config permits, and the console is detached for the duration so a suite that + * deliberately provokes data-loss alarms doesn't print any. + */ + private def capturedLoss(f: => Any): Seq[ILoggingEvent] = { + val logger = LoggerFactory.getLogger(classOf[service.LostMediaLog]).asInstanceOf[LogbackLogger] + val appender = new ListAppender[ILoggingEvent] + val original = logger.getLevel + appender.start() + logger.addAppender(appender) + logger.setLevel(Level.WARN) + logger.setAdditive(false) + try { val _ = f } + finally { + logger.setAdditive(true) + logger.setLevel(original) + logger.detachAppender(appender) + appender.stop() + } + appender.list.asScala.toSeq + } + // A signed serving URL is only ever minted for a file that was on disk at the time (PanoDataService.cropUrl and // backupImageUrl both check first), so a miss on one of these means the bytes vanished inside the signature's - // ~75-minute life. That is the loss #4925 had no way to notice, and #4926 gives it a log line — but the response - // still has to stay an ordinary 404, which is what these pin. + // ~75-minute life. That is the loss #4925 had no way to notice: the response has to stay an ordinary 404, since + // telling a prober which ids exist would be worse, which leaves the log as the only place it can be said (#4926). "Serving media whose bytes are gone" should { - "answer a signed crop URL whose file has been deleted with a plain 404" in { + "answer a signed crop URL whose file has been deleted with a plain 404, and say the crop is gone" in { val session = freshAnonSession() status(postCrop(session, syntheticLabelId)) mustBe OK - val url = panoDataService.cropUrl(syntheticLabelId, LabelTypeEnum.CurbRamp).value - cropFileFor(syntheticLabelId).delete() mustBe true + val url = panoDataService.cropUrl(syntheticLabelId, LabelTypeEnum.CurbRamp).value + val file = cropFileFor(syntheticLabelId) + file.delete() mustBe true - val resp = route(app, FakeRequest(GET, url).withCookies(session: _*)).get - status(resp) mustBe NOT_FOUND - cleanUp(syntheticLabelId) + try { + val events = capturedLoss { + val resp = route(app, FakeRequest(GET, url).withCookies(session: _*)).get + status(resp) mustBe NOT_FOUND + } + + events must have size 1 + // A crop can be re-cut from pano imagery, so it is the warning tier rather than the paging one. + events.head.getLevel mustBe Level.WARN + val message = events.head.getFormattedMessage + message must include("crop") + message must include(syntheticLabelId.toString) + // Whoever reads this line goes looking on disk, so it has to name the exact path that was checked. + message must include(file.getAbsolutePath) + } finally cleanUp(syntheticLabelId) } "answer a signed pano URL with no backup image with a plain 404, every time it is asked" in { - // The log deduplicates a repeat (LostMediaLogSpec pins that); the responses must not. + // For a pano whose source imagery has expired this store holds the only copy left anywhere, so the loss is the + // error tier. The log deduplicates the repeat (LostMediaLogSpec pins that); the responses must not. val panoId = "sidewalkSpecNoSuchPano4926" val url = signingService.signedUrl(s"/backupImage/$panoId") - status(route(app, FakeRequest(GET, url)).get) mustBe NOT_FOUND - status(route(app, FakeRequest(GET, url)).get) mustBe NOT_FOUND + + val events = capturedLoss { + status(route(app, FakeRequest(GET, url)).get) mustBe NOT_FOUND + status(route(app, FakeRequest(GET, url)).get) mustBe NOT_FOUND + } + + events must have size 1 + events.head.getLevel mustBe Level.ERROR + val message = events.head.getFormattedMessage + message must include("pano") + message must include(panoId) + // The store sharded by the pano id's first two characters, which is where someone restoring a backup must look. + message must include(panoDataService.backupImageDir(panoId).getAbsolutePath) + } + + "stay silent when the crop is where it should be, so the log holds losses and nothing else" in { + // A different label id from the case above: reporting is once per item, so reusing one would let dedup supply + // the silence this is trying to check for. And a false alarm is worse than noise here — it burns the single + // line that item will ever get. + val session = freshAnonSession() + try { + status(postCrop(session, otherSyntheticLabelId)) mustBe OK + val url = panoDataService.cropUrl(otherSyntheticLabelId, LabelTypeEnum.CurbRamp).value + + val events = capturedLoss { + status(route(app, FakeRequest(GET, url).withCookies(session: _*)).get) mustBe OK + } + events mustBe empty + } finally cleanUp(otherSyntheticLabelId) } } } diff --git a/test/js/healthMediaPanel.test.js b/test/js/healthMediaPanel.test.js new file mode 100644 index 0000000000..079002273a --- /dev/null +++ b/test/js/healthMediaPanel.test.js @@ -0,0 +1,287 @@ +/** + * Tests for the Health dashboard's Media storage panel (public/js/admin-dashboard/HealthPage.js, issue #4926). + * + * The panel is the human-readable half of the tripwire #4925 went without: a story photo was destroyed by a deploy + * and answered 404 for six days with nothing anywhere saying so. Its failure mode is quiet in both directions — + * showing zeros for a city it could not actually scan reads as "all present", and painting a dev checkout red for + * the relative defaults it is supposed to use teaches everyone to ignore the panel entirely. + * + * So every judgment call here belongs to the server (MediaIntegrity computes each row's label and severity, and + * HealthMediaPayloadSpec pins the field names below). What is tested here is that the page renders what it was told + * and invents nothing: an unscanned city must not read as a clean one, and the KPI must never call an unknown + * healthy. + * + * The class is a plain top-level declaration (no window assignment), so the source is eval'd with an explicit + * window epilogue, the way the other class suites do it. + */ + +const fs = require('fs'); +const path = require('path'); + +const PAGE_SRC = fs.readFileSync( + path.resolve(__dirname, '..', '..', 'public/js/admin-dashboard/HealthPage.js'), 'utf8' +); + +/** The panel's containers, plus the KPI tile it fills. Absent ids are no-ops, so only what's asserted is needed. */ +const MARKUP = ` +
+ +
+
+

+`; + +/** Every threshold the untested panels read, so a render can't throw before reaching the media panel. */ +const THRESHOLDS = { + idle_txn_warn_seconds: 60, idle_txn_bad_seconds: 300, lock_wait_warn_seconds: 5, lock_wait_bad_seconds: 30, + active_query_warn_seconds: 30, active_query_bad_seconds: 120, bloat_warn_ratio: 0.2, bloat_bad_ratio: 0.4, + bloat_min_dead_tuples: 1000, vacuum_age_warn_seconds: 86400, conn_pool_max: 25, conn_warn_active: 15, + conn_bad_active: 20 +}; + +const OK_DIR = { + key: 'story.media.directory', env_var: 'SIDEWALK_STORY_MEDIA_DIR', irreplaceable: true, + path: '/srv/sidewalk/story-media', status: 'ok', label: 'ok', severity: 'good' +}; + +const UNSAFE_DIR = { + key: 'pano.images.directory', env_var: 'SIDEWALK_PANO_DIR', irreplaceable: true, + path: '/app/target/universal/stage/.pano-images', status: 'unsafe', label: 'a deploy will delete this', + severity: 'bad', detail: 'pano.images.directory resolves inside the build output tree that a deploy deletes.' +}; + +/** One city whose rows and files all line up. */ +const cleanCity = (overrides = {}) => ({ + city_id: 'chicago-il', schema: 'sidewalk_chicago', rows: 3, missing: 0, orphans: 0, + missing_ids: [], orphan_ids: [], scanned: true, ...overrides +}); + +/** A payload with the media panel populated and every other panel empty. */ +const payloadWith = (mediaStorage) => ({ + generated_at: '2026-08-20T00:00:00Z', current_database: 'sidewalk', current_role: 'sidewalk', + can_see_all_queries: true, blocking_sessions: [], idle_in_transaction: [], active_queries: [], + stuck_evolutions: [], table_bloat: [], connections: [], pano_backups: null, + media_storage: mediaStorage, thresholds: THRESHOLDS +}); + +describe('HealthPage media storage panel', () => { + let HealthPage; + + beforeEach(() => { + // init() installs poll intervals; fake timers keep them from firing into a torn-down DOM. + jest.useFakeTimers(); + document.body.innerHTML = MARKUP; + window.eval(`${PAGE_SRC}\nwindow.HealthPage = HealthPage;`); + HealthPage = window.HealthPage; + }); + + afterEach(() => { + jest.useRealTimers(); + delete global.fetch; + }); + + /** Renders one payload through the real load path and hands back the panel's containers. */ + async function render(mediaStorage) { + global.fetch = jest.fn().mockResolvedValue({ ok: true, json: async () => payloadWith(mediaStorage) }); + await new HealthPage({ healthUrl: '/adminapi/dbHealth' }).init(); + // A render that threw is caught and reported in the pulse line, which would otherwise leave the media + // assertions failing against empty containers with no hint of why. + expect(document.getElementById('health-pulse').innerHTML).not.toContain('Could not load'); + return { + dirs: document.getElementById('health-media-dirs').innerHTML, + story: document.getElementById('health-media-story').innerHTML, + note: document.getElementById('health-media-note').innerHTML, + kpi: document.getElementById('kpi-media') + }; + } + + describe('the directory table', () => { + it('names each directory, the variable that fixes it, and where it resolved', async () => { + const { dirs } = await render({ directories: [OK_DIR], enforced: true, story_media: null }); + + expect(dirs).toContain('story.media.directory'); + // A path with no variable name beside it doesn't tell an operator what to change. + expect(dirs).toContain('SIDEWALK_STORY_MEDIA_DIR'); + expect(dirs).toContain('/srv/sidewalk/story-media'); + }); + + it('shows the server\'s own label and severity rather than deciding either here', async () => { + const { dirs } = await render({ directories: [UNSAFE_DIR], enforced: true, story_media: null }); + + expect(dirs).toContain('ac-badge--bad'); + expect(dirs).toContain('a deploy will delete this'); + }); + + it('says how to fix an unsafe directory once, below the table', async () => { + // The wipe-zone reason repeats the same sentence about deploys on every row, and four copies of it in the + // narrowest column is what made these rows tower over every other table on the page. + const { dirs, note } = await render({ + directories: [UNSAFE_DIR, { ...OK_DIR, status: 'unsafe', severity: 'bad', detail: UNSAFE_DIR.detail }], + enforced: true, story_media: null + }); + + expect(dirs).not.toContain('resolves inside the build output tree'); + expect(note).toContain('deleted by the next release'); + }); + + it('keeps a non-wipe-zone explanation, which the table is the only place to read', async () => { + const notReadable = { + ...OK_DIR, status: 'not_readable', label: 'not readable', severity: 'bad', + detail: 'SIDEWALK_STORY_MEDIA_DIR points at a path this process cannot read.' + }; + const { dirs } = await render({ directories: [notReadable], enforced: true, story_media: null }); + + expect(dirs).toContain('cannot read'); + }); + + it('explains that a dev checkout is meant to look like this', async () => { + // Without this the dev dashboard is permanently red for doing exactly what it is configured to do, and a + // panel that is always red is a panel nobody reads on the day it matters. + const { note } = await render({ directories: [UNSAFE_DIR], enforced: false, story_media: null }); + + expect(note).toContain('Not production mode'); + }); + + it('says why there is nothing to show when the scan could not even stat the directories', async () => { + const { dirs, story } = await render({ + directories: [], enforced: true, story_media: null, + unavailable: 'A previous media scan has not returned; storage may be offline.' + }); + + expect(dirs).toContain('storage may be offline'); + // An empty table here would read as "no media directories are configured", which is a different problem. + expect(dirs).not.toContain(' { + const scanOf = (cities, missing, orphans) => ({ + directories: [OK_DIR], enforced: true, + story_media: { base_dir: '/srv/sidewalk/story-media', cities, missing, orphans } + }); + + it('badges a city that has lost files, and lists ids to start looking from', async () => { + const lost = cleanCity({ missing: 1, missing_ids: [331] }); + const { story } = await render(scanOf([lost], 1, 0)); + + expect(story).toContain('chicago-il'); + expect(story).toContain('ac-badge--bad'); + expect(story).toContain('missing 331'); + }); + + it('badges orphaned files at the lesser tone, since they are a half-finished retraction', async () => { + const orphaned = cleanCity({ orphans: 2, orphan_ids: [7, 8] }); + const { story } = await render(scanOf([orphaned], 0, 2)); + + expect(story).toContain('ac-badge--warn'); + expect(story).toContain('orphaned 7, 8'); + }); + + it('leaves a clean city unbadged, so a healthy fleet reads as quiet', async () => { + const { story } = await render(scanOf([cleanCity()], 0, 0)); + + expect(story).toContain('chicago-il'); + expect(story).not.toContain('ac-badge--bad'); + expect(story).not.toContain('ac-badge--warn'); + }); + + it('shows an unscanned city as unknown rather than as zero losses', async () => { + // Rendering a 0 here is the panel's worst possible lie: it says "every photo is present" about a city it + // never managed to look at. + const unscanned = cleanCity({ + city_id: null, scanned: false, unscanned_reason: 'no city on this stage is configured to use schema X' + }); + const { story } = await render(scanOf([unscanned], 0, 0)); + + expect(story).toContain('sidewalk_chicago'); + expect(story).toContain('no city on this stage is configured'); + expect(story).not.toContain('ac-badge--bad'); + expect(story).toContain('—'); + }); + + it('says plainly when no city has any story media, rather than showing an empty table', async () => { + const { story } = await render(scanOf([], 0, 0)); + + expect(story).toContain('No city has any story media yet.'); + }); + + it('names the directory the per-city subdirectories live under', async () => { + const { note } = await render(scanOf([cleanCity()], 0, 0)); + + expect(note).toContain('/srv/sidewalk/story-media'); + }); + + it('escapes a server-supplied reason instead of writing it into the page as markup', async () => { + const hostile = cleanCity({ scanned: false, unscanned_reason: '' }); + const { story } = await render(scanOf([hostile], 0, 0)); + + expect(story).not.toContain(' { + const kpiFor = async (mediaStorage) => { + const { kpi } = await render(mediaStorage); + return { text: kpi.textContent, tone: kpi.className }; + }; + + it('shows the missing count and reds the tile when anything is gone', async () => { + const { text, tone } = await kpiFor({ + directories: [OK_DIR], enforced: true, + story_media: { base_dir: '/srv/media', cities: [cleanCity({ missing: 2 })], missing: 2, orphans: 0 } + }); + + expect(text).toBe('2'); + expect(tone).toContain('health-kpi--bad'); + }); + + it('reds the tile for a directory a deploy will delete, before anything has been lost from it', async () => { + const { text, tone } = await kpiFor({ + directories: [UNSAFE_DIR], enforced: true, + story_media: { base_dir: '/srv/media', cities: [cleanCity()], missing: 0, orphans: 0 } + }); + + // The tile is labelled "Missing media files", so it can only ever show a count of those; the directory + // table below is what names the doomed directory. + expect(text).toBe('0'); + expect(tone).toContain('health-kpi--bad'); + }); + + it('warns, without reddening, when the only fault is files nobody has a row for', async () => { + const { text, tone } = await kpiFor({ + directories: [OK_DIR], enforced: true, + story_media: { base_dir: '/srv/media', cities: [cleanCity({ orphans: 1 })], missing: 0, orphans: 1 } + }); + + expect(text).toBe('0'); + expect(tone).toContain('health-kpi--warn'); + }); + + it('goes green only when the scan ran and found nothing wrong', async () => { + const { tone } = await kpiFor({ + directories: [OK_DIR], enforced: true, + story_media: { base_dir: '/srv/media', cities: [cleanCity()], missing: 0, orphans: 0 } + }); + + expect(tone).toContain('health-kpi--good'); + }); + + it('shows unknown, never a zero, when the scan could not run', async () => { + // A count of unsafe directories under a tile labelled "Missing media files" would misreport, and a 0 + // would claim a clean result the scan never produced. + const { text, tone } = await kpiFor({ directories: [OK_DIR], enforced: true, story_media: null }); + + expect(text).toBe('—'); + expect(tone).toContain('health-kpi--ok'); + }); + + it('tones unknown neutral rather than green when the whole payload is missing', async () => { + const { text, tone } = await kpiFor(null); + + expect(text).toBe('—'); + expect(tone).toContain('health-kpi--ok'); + }); + }); +}); diff --git a/test/service/HealthMediaPayloadSpec.scala b/test/service/HealthMediaPayloadSpec.scala new file mode 100644 index 0000000000..20d9392309 --- /dev/null +++ b/test/service/HealthMediaPayloadSpec.scala @@ -0,0 +1,95 @@ +package service + +import org.scalatestplus.play.PlaySpec +import play.api.libs.json.{JsObject, Json} + +/** + * The wire contract between the media-storage panel's payload and the page that renders it (#4926). + * + * `HealthPage.js` reads these fields by name, and nothing else connects the two: rename a case-class field and the + * writer quietly emits a different key, every value the panel reads goes undefined, and the page renders a table of + * blanks — a storage monitor that reports nothing, which is indistinguishable from a storage monitor reporting that + * nothing is wrong. Each key set below is exactly what `#renderMediaStorage` and `#mediaKpi` consume. + * + * Pure serialization — no app boot, no database. + */ +class HealthMediaPayloadSpec extends PlaySpec { + + import HealthService._ + + private val dir = MediaDirStatus( + key = "story.media.directory", envVar = "SIDEWALK_STORY_MEDIA_DIR", irreplaceable = true, + path = "/srv/sidewalk/story-media", status = "unsafe", label = "a deploy will delete this", severity = "bad", + detail = Some("resolves inside the build output tree") + ) + + private val city = CityStoryMedia( + cityId = Some("chicago-il"), + schema = "sidewalk_chicago", + rows = 3, + missing = 1, + orphans = 2, + missingIds = Seq(1), + orphanIds = Seq(7, 8), + scanned = true, + unscannedReason = Some("not scanned") + ) + + private val health = MediaStorageHealth( + directories = Seq(dir), + enforced = true, + storyMedia = Some(StoryMediaIntegrity("/srv/sidewalk/story-media", Seq(city), 1, 2)), + unavailable = Some("storage may be offline") + ) + + private def keysOf(json: JsObject): Set[String] = json.keys.toSet + + "The media storage payload" should { + "carry every field the panel's directory table reads, under the names it reads them by" in { + keysOf(Json.toJson(dir).as[JsObject]) mustBe + Set("key", "env_var", "irreplaceable", "path", "status", "label", "severity", "detail") + } + + "carry every field the panel's per-city table reads" in { + keysOf(Json.toJson(city).as[JsObject]) mustBe + Set("city_id", "schema", "rows", "missing", "orphans", "missing_ids", "orphan_ids", "scanned", + "unscanned_reason") + } + + "carry the scan's own totals and base directory" in { + keysOf(Json.toJson(StoryMediaIntegrity("/srv/media", Seq(city), 1, 2)).as[JsObject]) mustBe + Set("base_dir", "cities", "missing", "orphans") + } + + "carry the panel's own four fields" in { + keysOf(Json.toJson(health).as[JsObject]) mustBe Set("directories", "enforced", "story_media", "unavailable") + } + + "hang the whole panel off media_storage, which is the key the page looks for" in { + val payload = DbHealthData( + generatedAt = "2026-08-20T00:00:00Z", + currentDatabase = "sidewalk", + currentRole = "sidewalk", + canSeeAllQueries = true, + blockingSessions = Seq.empty, + idleInTransaction = Seq.empty, + activeQueries = Seq.empty, + stuckEvolutions = Seq.empty, + tableBloat = Seq.empty, + connections = Seq.empty, + panoBackups = None, + mediaStorage = Some(health), + thresholds = HealthThresholds(1, 2, 3, 4, 5, 6, 0.2, 0.4, 1000, 60, 40, 20, 30) + ) + (Json.toJson(payload) \ "media_storage" \ "story_media" \ "missing").as[Int] mustBe 1 + } + + "omit the optional fields rather than send nulls, which is what the page's fallbacks expect" in { + // `unscanned_reason` absent is how a scanned city says it has nothing to explain, and `detail` absent is how a + // healthy directory does; the page falls back on absence, so emitting an explicit null would be a change. + val clean = Json.toJson(city.copy(unscannedReason = None)).as[JsObject] + clean.keys must not contain "unscanned_reason" + Json.toJson(dir.copy(detail = None)).as[JsObject].keys must not contain "detail" + } + } +} diff --git a/test/service/HealthServiceSpec.scala b/test/service/HealthServiceSpec.scala index 225281fc4d..3c53a173d1 100644 --- a/test/service/HealthServiceSpec.scala +++ b/test/service/HealthServiceSpec.scala @@ -1,6 +1,7 @@ package service import models.utils.{HealthTable, MyPostgresProfile} +import org.scalatest.BeforeAndAfterAll import org.scalatestplus.play.PlaySpec import org.scalatestplus.play.guice.GuiceOneAppPerSuite import play.api.Application @@ -8,6 +9,8 @@ import play.api.db.slick.DatabaseConfigProvider import play.api.inject.guice.GuiceApplicationBuilder import slick.dbio.DBIO +import java.io.File +import java.nio.file.Files import scala.concurrent.ExecutionContext.Implicits.global import scala.concurrent.duration._ import scala.concurrent.{Await, Future} @@ -29,13 +32,25 @@ import scala.concurrent.{Await, Future} * Read-only. Requires a Postgres+PostGIS database (DATABASE_URL / DATABASE_USER / DATABASE_PASSWORD, as in dev/CI). * Scheduling actors are disabled so background actors can't contend for the pool during the run. */ -class HealthServiceSpec extends PlaySpec with GuiceOneAppPerSuite { +class HealthServiceSpec extends PlaySpec with GuiceOneAppPerSuite with BeforeAndAfterAll { + + // The scan compares database rows against files on disk, so a checkout that has never had an upload has no + // directory to read and the whole panel degrades to "unavailable" — under which every assertion below would pass + // without testing anything. Pointing the app at a directory this spec owns makes the interesting path the only one. + private val mediaDir: File = Files.createTempDirectory("health-service-spec-media").toFile + + /** The subdirectory `StoryService` would write this instance's uploads into: named for `city-id`, not the schema. */ + private lazy val cityDir: File = new File(mediaDir, config.get[String]("city-id")) override def fakeApplication(): Application = - new GuiceApplicationBuilder().disable[modules.ActorModule].build() + new GuiceApplicationBuilder() + .disable[modules.ActorModule] // No eager background actors during tests. + .configure("story.media.directory" -> mediaDir.getAbsolutePath) + .build() private val healthService = app.injector.instanceOf[HealthService] private val healthTable = app.injector.instanceOf[HealthTable] + private val config = app.injector.instanceOf[play.api.Configuration] // Keep the DatabaseConfig as a stable val and call .db.run inline; binding .db to its own val would infer a // path-dependent existential type that needs -language:existentials. private val dbConfig = app.injector.instanceOf[DatabaseConfigProvider].get[MyPostgresProfile] @@ -43,6 +58,26 @@ class HealthServiceSpec extends PlaySpec with GuiceOneAppPerSuite { private def await[T](f: Future[T], d: Duration = 60.seconds): T = Await.result(f, d) private def run[T](action: DBIO[T]): T = Await.result(dbConfig.db.run(action), 60.seconds) + // A file with no `story_media` row behind it. Whether the scan attributes it to this instance's own schema is the + // whole question: it can only do that by naming the directory the way the write path does. + private val orphanId: Int = Int.MaxValue - 4926 + private lazy val orphanFile: File = new File(cityDir, s"story_$orphanId.jpg") + + // Seeded before any test, because the scan is cached: a first call made against an empty directory would answer + // every later assertion from that cached all-clear. + override def beforeAll(): Unit = { + super.beforeAll() + val _ = cityDir.mkdirs() + val _ = orphanFile.createNewFile() + } + + override def afterAll(): Unit = { + val _ = orphanFile.delete() + val _ = cityDir.delete() + val _ = mediaDir.delete() + super.afterAll() + } + "HealthTable catalog queries" should { // Each of these asserts the SQL executes and maps into its DTO against a real PostGIS DB; the result may legitimately // be empty (a healthy DB has no blocking locks), so the value is that `run` completes without throwing. @@ -130,6 +165,48 @@ class HealthServiceSpec extends PlaySpec with GuiceOneAppPerSuite { } } + "scan the story-media directory it was configured with" in { + val scan = await(healthService.getDbHealth).mediaStorage.value.storyMedia.value + scan.baseDir mustBe mediaDir.getAbsolutePath + } + + "find this instance's own files under the directory the write path builds, not the one the schema implies" in { + // `city-id` and the connection's schema are independent settings, and on an instance where they disagree the + // scan has to look where StoryService writes rather than where the schema mapping says it should — resolving + // it the other way reported a photo sitting right there as destroyed. The file seeded here has no row, so the + // current schema's row can only account for it if the directory was resolved the write path's way. + val scan = await(healthService.getDbHealth).mediaStorage.value.storyMedia.value + val mine = scan.cities.find(_.schema == run(healthTable.getCurrentSchema)).value + + mine.cityId.value mustBe config.get[String]("city-id") + mine.scanned mustBe true + mine.orphanIds must contain(orphanId) + } + + "claim that directory exclusively, so no other schema reads those same files as its own orphans" in { + val scan = await(healthService.getDbHealth).mediaStorage.value + + val claimed = scan.storyMedia.value.cities.flatMap(_.cityId) + claimed mustBe claimed.distinct + // Every city that lost the directory has to say why, or the operator reading the row has nowhere to start. + scan.storyMedia.value.cities.filterNot(_.scanned).foreach(_.unscannedReason mustBe defined) + } + + "cover every schema holding a story_media table, in a stable order" in { + // The panel is read down the page, and a scan that silently dropped a city would look exactly like a city with + // nothing to report. + val scan = await(healthService.getDbHealth).mediaStorage.value.storyMedia.value + val schemas = run(healthTable.getStoryMediaSchemas).filter(_.matches("^[A-Za-z0-9_]+$")) + + scan.cities.map(_.schema) mustBe schemas.sorted + } + + "agree with the boot check about whether the media-directory contract is being enforced" in { + // Two copies of the arming rule would let the page claim a stage is guarded when the check isn't watching. + val media = await(healthService.getDbHealth).mediaStorage.value + media.enforced mustBe modules.PersistentMediaDirCheck.arms(app.injector.instanceOf[play.api.Environment]) + } + "survive a burst of concurrent polls without exhausting the connection pool" in { // Simulate many Owner tabs polling at once, from a cold cache (the worst case). If getDbHealth fanned out one // query per city schema, 30 concurrent calls would each demand ~one-connection-per-schema and, on a many-city diff --git a/test/service/MediaIntegritySpec.scala b/test/service/MediaIntegritySpec.scala index 0d0b93877d..f2858f2390 100644 --- a/test/service/MediaIntegritySpec.scala +++ b/test/service/MediaIntegritySpec.scala @@ -256,6 +256,28 @@ class MediaIntegritySpec extends PlaySpec { } } + "scanRefusal" should { + "run the scan against a readable directory" in { + MediaIntegrity.scanRefusal("/srv/media", isDirectory = true, canRead = true) mustBe None + } + + "decline, naming the path, when there is no directory to compare against" in { + // No upload has landed on this stage yet, or the whole directory is gone; the per-directory table above already + // reports which, and inventing per-city loss counts from it would say the wrong one. + val refusal = MediaIntegrity.scanRefusal("/srv/media", isDirectory = false, canRead = true).value + refusal must include("/srv/media") + refusal must include("No media directory") + } + + "decline rather than report loss when the directory is there but unreadable" in { + // The trap this exists for: `isDirectory` is true for a directory this process may not read, and every listing + // beneath it comes back empty — which would announce every story photo on the stage as destroyed. + val refusal = MediaIntegrity.scanRefusal("/srv/media", isDirectory = true, canRead = false).value + refusal must include("/srv/media") + refusal must include("not readable") + } + } + "listing" should { "read a directory's contents" in { MediaIntegrity.listing(dirContaining("story_3.jpg")) mustBe DirListing.Listed(Seq("story_3.jpg")) diff --git a/test/service/SingleFlightGateSpec.scala b/test/service/SingleFlightGateSpec.scala new file mode 100644 index 0000000000..ac35d3b042 --- /dev/null +++ b/test/service/SingleFlightGateSpec.scala @@ -0,0 +1,90 @@ +package service + +import org.scalatest.concurrent.ScalaFutures +import org.scalatestplus.play.PlaySpec + +import java.util.concurrent.atomic.AtomicInteger +import scala.concurrent.duration._ +import scala.concurrent.{Await, Future, Promise} + +/** + * The gate that keeps a stuck media scan from being joined by a fresh copy on every poll (#4926). + * + * Its whole job is invisible when storage is healthy and decisive when it isn't: a filesystem call against a dead + * mount never returns and cannot be cancelled, so a dashboard polling every ~20 seconds parks a thread per poll until + * the blocking-io pool is gone — and the panel that exists to report storage trouble goes dark exactly when storage + * is the trouble. The cases below pin the one distinction that makes that work: the gate opens when the *work* + * finishes, never when a caller stops waiting for it. + * + * Pure logic — no app boot, no database, no filesystem. + */ +class SingleFlightGateSpec extends PlaySpec with ScalaFutures { + + private def await[T](f: Future[T]): T = Await.result(f, 5.seconds) + + "SingleFlightGate" should { + "run the work and hand back its result when nothing else is running" in { + val gate = new SingleFlightGate + await(gate.runOrElse("busy")(Future.successful("scanned"))) mustBe "scanned" + } + + "answer a caller who arrives mid-flight with the stand-in, without starting a second copy" in { + val gate = new SingleFlightGate + val started = new AtomicInteger(0) + val blocker = Promise[String]() + + val first = gate.runOrElse("busy") { started.incrementAndGet(); blocker.future } + val second = gate.runOrElse("busy") { started.incrementAndGet(); Future.successful("second") } + + await(second) mustBe "busy" + // The point isn't the answer the second caller got, it's that no second scan exists to park a second thread. + started.get mustBe 1 + blocker.success("first") + await(first) mustBe "first" + } + + "keep the gate shut while the work runs on, even after the caller has given up waiting for it" in { + // The deadline protects the poll; only this protects the pool. A timeout that reopened the gate would let the + // next poll start another scan on top of a filesystem call that has not returned and may never. + val gate = new SingleFlightGate + val blocker = Promise[String]() + val first = gate.runOrElse("busy")(blocker.future) + + // What a caller timing out looks like from here: it stops waiting, and the work keeps running. + first.isCompleted mustBe false + await(gate.runOrElse("busy")(Future.successful("second"))) mustBe "busy" + + blocker.success("late") + await(first) mustBe "late" + await(gate.runOrElse("busy")(Future.successful("third"))) mustBe "third" + } + + "reopen once the work completes, so a recovered mount is reported instead of a permanent stand-in" in { + val gate = new SingleFlightGate + await(gate.runOrElse("busy")(Future.successful("first"))) mustBe "first" + await(gate.runOrElse("busy")(Future.successful("second"))) mustBe "second" + } + + "treat a failure as a completion, since only work still running should keep the next caller out" in { + val gate = new SingleFlightGate + val failed = gate.runOrElse("busy")(Future.failed(new RuntimeException("mount gone"))) + whenReady(failed.failed)(_.getMessage mustBe "mount gone") + await(gate.runOrElse("busy")(Future.successful("after"))) mustBe "after" + } + + "reopen when the work throws before it returns a future at all" in { + // A synchronous throw is the easiest way to wedge a gate shut forever: nothing ever completes to reopen it. + val gate = new SingleFlightGate + val thrown = gate.runOrElse("busy")(throw new IllegalStateException("bad config")) + whenReady(thrown.failed)(_.getMessage mustBe "bad config") + await(gate.runOrElse("busy")(Future.successful("after"))) mustBe "after" + } + + "never build the stand-in on the common path, since it is only meaningful when something is stuck" in { + val gate = new SingleFlightGate + val built = new AtomicInteger(0) + await(gate.runOrElse { built.incrementAndGet(); "busy" }(Future.successful("scanned"))) mustBe "scanned" + built.get mustBe 0 + } + } +} From 45398136d434e6606663f92c95ff89171bd4f84e Mon Sep 17 00:00:00 2001 From: jonfroehlich Date: Thu, 20 Aug 2026 06:40:38 -0700 Subject: [PATCH 6/8] Pin the upload grace window the loss tripwire depends on (#4926) 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) --- .github/workflows/ci.yml | 4 +- app/controllers/StoryController.scala | 26 ++++++++--- test/controllers/StoryUploadWindowSpec.scala | 49 ++++++++++++++++++++ 3 files changed, 71 insertions(+), 8 deletions(-) create mode 100644 test/controllers/StoryUploadWindowSpec.scala diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5114674c57..f730c2712a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -319,8 +319,10 @@ jobs: # - ImageControllerSpec (#4415, #4726, #4926): the crop write path and its share-preview invalidation, plus the # loss lines a signed crop/pano URL emits when the bytes it was signed for are gone. Nothing else asserts the # controller calls LostMediaLog at all, so without this the tripwire could be deleted with CI still green. + # - StoryUploadWindowSpec (#4926): the grace window that stops the loss tripwire firing at its own upload path. + # Reporting is once per media id, so a false alarm spends the line a real loss would have needed. Pure logic. - name: Run gating/auth tests (health dashboard + route auth posture + geodesic distances) - run: sbt 'set Test / parallelExecution := false' 'testOnly controllers.HealthDashboardSpec service.HealthServiceSpec controllers.RouteAuthPostureSpec models.street.GeodesicDistanceSpec service.ExploreTutorialRouteSpec controllers.MobileDetectionSpec service.PanoDataServiceSpec models.utils.ConfigTableVoidedArchiveSpec controllers.api.StatsApiSpec controllers.ExploreSubmissionSpec controllers.ValidateSubmissionSpec modules.PersistentMediaDirCheckSpec service.MediaIntegritySpec service.LostMediaLogSpec service.SingleFlightGateSpec service.HealthMediaPayloadSpec controllers.ImageControllerSpec' + run: sbt 'set Test / parallelExecution := false' 'testOnly controllers.HealthDashboardSpec service.HealthServiceSpec controllers.RouteAuthPostureSpec models.street.GeodesicDistanceSpec service.ExploreTutorialRouteSpec controllers.MobileDetectionSpec service.PanoDataServiceSpec models.utils.ConfigTableVoidedArchiveSpec controllers.api.StatsApiSpec controllers.ExploreSubmissionSpec controllers.ValidateSubmissionSpec modules.PersistentMediaDirCheckSpec service.MediaIntegritySpec service.LostMediaLogSpec service.SingleFlightGateSpec service.HealthMediaPayloadSpec controllers.ImageControllerSpec controllers.StoryUploadWindowSpec' env: DATABASE_URL: jdbc:postgresql://localhost:5432/sidewalk DATABASE_USER: sidewalk diff --git a/app/controllers/StoryController.scala b/app/controllers/StoryController.scala index 5abdf9af18..a92351f618 100644 --- a/app/controllers/StoryController.scala +++ b/app/controllers/StoryController.scala @@ -228,11 +228,6 @@ class StoryController @Inject() ( } } - // The upload flow commits the media row before the file move lands (StoryService's place-before-commit windows), so - // a row this young with no bytes is almost certainly mid-upload, not loss. The window is sub-second; a minute is - // generous slack. - private val lostMediaGrace = Duration.ofMinutes(1) - /** * Reports a media row whose bytes are missing from disk (#4925). Kept high-signal by skipping the upload grace * window, since a false alarm on this tripwire has real cost; `LostMediaLog` handles the rest. @@ -241,8 +236,7 @@ class StoryController @Inject() ( * @param file Where the bytes should have been. */ private def logLostMedia(media: StoryMedia, file: File): Unit = { - val inUploadWindow = media.createdAt.isAfter(OffsetDateTime.now.minus(lostMediaGrace)) - if (!inUploadWindow) { + if (!StoryController.withinUploadWindow(media.createdAt, OffsetDateTime.now)) { lostMediaLog.reportMissing("story_media", media.storyMediaId.toString, file.getAbsolutePath, irreplaceable = true) } } @@ -307,4 +301,22 @@ object StoryController { /** Cap on the stories the /stories page renders, so the page stays bounded as a city's story count grows. */ val ListingMax: Int = 500 + + // The upload flow commits the media row before the file move lands (StoryService's place-before-commit windows), + // so a row this young with no bytes is almost certainly mid-upload. The window is sub-second; a minute is generous + // slack. + private val lostMediaGrace = Duration.ofMinutes(1) + + /** + * Whether a media row is young enough that missing bytes read as an upload in progress rather than as loss. + * + * The data-loss log deduplicates per item, so a false alarm doesn't merely add noise — it spends the one line that + * media id will ever get, and a real loss discovered later says nothing at all. + * + * @param createdAt When the media row was committed. + * @param now The instant to judge it against. + * @return True while the row is inside the upload window and its file may still be landing. + */ + private[controllers] def withinUploadWindow(createdAt: OffsetDateTime, now: OffsetDateTime): Boolean = + createdAt.isAfter(now.minus(lostMediaGrace)) } diff --git a/test/controllers/StoryUploadWindowSpec.scala b/test/controllers/StoryUploadWindowSpec.scala new file mode 100644 index 0000000000..6f68452a63 --- /dev/null +++ b/test/controllers/StoryUploadWindowSpec.scala @@ -0,0 +1,49 @@ +package controllers + +import org.scalatestplus.play.PlaySpec + +import java.time.OffsetDateTime +import java.time.temporal.ChronoUnit + +/** + * The grace window that keeps the story-media loss tripwire from crying wolf at its own upload path (#4925, #4926). + * + * `StoryService` commits the media row before the file move lands, so for a moment a perfectly healthy upload looks + * exactly like a destroyed one. Reporting it would cost more than noise: the loss log reports once per media id, so a + * false alarm spends the single line that id will ever get, and the real loss — if it ever comes — passes in silence. + * A window too generous is the opposite failure, a stretch of time in which real loss goes unannounced. + * + * No app or database boot: the interesting cases are the two boundaries, which a live clock can't pin down. + */ +class StoryUploadWindowSpec extends PlaySpec { + + private val committed = OffsetDateTime.parse("2026-08-20T10:00:00Z") + + "StoryController.withinUploadWindow" should { + "treat a row committed moments ago as an upload still landing" in { + StoryController.withinUploadWindow(committed, committed.plus(200, ChronoUnit.MILLIS)) mustBe true + } + + "still cover a row a few seconds old, since the window is slack over a sub-second race" in { + StoryController.withinUploadWindow(committed, committed.plusSeconds(30)) mustBe true + } + + "call a row past the window loss, which is the whole point of having one" in { + StoryController.withinUploadWindow(committed, committed.plusSeconds(61)) mustBe false + } + + "close the window on the minute rather than leaving it ambiguous" in { + StoryController.withinUploadWindow(committed, committed.plusMinutes(1)) mustBe false + } + + "call a row committed long ago loss, however far back it goes" in { + StoryController.withinUploadWindow(committed, committed.plusDays(30)) mustBe false + } + + "treat a row timestamped ahead of the clock as still landing, since skew is not evidence of loss" in { + // Self-correcting rather than permanent: once the clock passes the row's timestamp by a minute, it reports. + StoryController.withinUploadWindow(committed.plusMinutes(5), committed) mustBe true + StoryController.withinUploadWindow(committed.plusMinutes(5), committed.plusMinutes(7)) mustBe false + } + } +} From 530e62d76cc852dc37b199a7ffc49ec34f4ddaa7 Mon Sep 17 00:00:00 2001 From: jonfroehlich Date: Thu, 20 Aug 2026 07:13:50 -0700 Subject: [PATCH 7/8] Make the media directory table say what it means (#4926) 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) --- app/service/MediaIntegrity.scala | 4 +++- public/js/admin-dashboard/HealthPage.js | 10 +++++----- test/js/healthMediaPanel.test.js | 24 ++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/app/service/MediaIntegrity.scala b/app/service/MediaIntegrity.scala index 5a95315adc..c2f3af0e9c 100644 --- a/app/service/MediaIntegrity.scala +++ b/app/service/MediaIntegrity.scala @@ -112,7 +112,9 @@ object MediaIntegrity { ): MediaDirStatus = unsafeReason match { case Some(reason) => val severity = if (!enforced) "ok" else if (dir.irreplaceable) "bad" else "warn" - val label = if (enforced) "a deploy will delete this" else "inside the build tree (dev)" + // The dev label leads with the verdict, because a status that only names a location leaves the reader to work + // out whether it is a problem — and here it isn't one. + val label = if (enforced) "a deploy will delete this" else "ok for dev (inside the build tree)" status(dir, path, "unsafe", label, severity, Some(reason)) // Not created yet is the normal state until the first upload — the write paths mkdirs on demand. case None if !probe.exists => status(dir, path, "absent", "not created yet", "ok", None) diff --git a/public/js/admin-dashboard/HealthPage.js b/public/js/admin-dashboard/HealthPage.js index 55c0ce31d9..13e22d59d6 100644 --- a/public/js/admin-dashboard/HealthPage.js +++ b/public/js/admin-dashboard/HealthPage.js @@ -388,9 +388,9 @@ class HealthPage { } const dirRows = (media.directories || []).map((d) => { - const lost = d.irreplaceable - ? 'gone for good' - : 'rebuildable'; + // Muted text rather than a badge: this is what losing the contents would cost, not something that has + // happened, and an amber badge next to the live Status badge reads as a second alarm on a healthy row. + const recoverable = d.irreplaceable ? 'No — nothing to rebuild it from' : 'Yes — rebuilt on demand'; // The wipe-zone reason names the key and the path, which are already columns, and then repeats the same // sentence about deploys on every row — four copies of it in the narrowest column is what made these rows six // times taller than every other table on the page. The badge says the state; the note below says the fix once. @@ -399,12 +399,12 @@ class HealthPage { ${HealthPage.#esc(d.key)} ${HealthPage.#esc(d.env_var)} - ${lost} ${HealthPage.#esc(d.path)} + ${recoverable} ${HealthPage.#esc(d.label)}${detail} `; }).join(''); - this.#table('health-media-dirs', ['Config key', 'Env var', 'If lost', 'Resolves to', 'Status'], dirRows); + this.#table('health-media-dirs', ['Config key', 'Env var', 'Resolves to', 'Recoverable?', 'Status'], dirRows); const scan = media.story_media; if (!scan) { diff --git a/test/js/healthMediaPanel.test.js b/test/js/healthMediaPanel.test.js index 079002273a..46314b0d39 100644 --- a/test/js/healthMediaPanel.test.js +++ b/test/js/healthMediaPanel.test.js @@ -105,6 +105,30 @@ describe('HealthPage media storage panel', () => { expect(dirs).toContain('/srv/sidewalk/story-media'); }); + it('states recoverability as flat text, since it is a consequence and not a live alarm', async () => { + // An amber badge here sat beside the Status badge on a perfectly healthy row and read as a second alarm, + // which is what made the column unreadable. + const { dirs } = await render({ directories: [OK_DIR], enforced: true, story_media: null }); + + expect(dirs).toContain('No — nothing to rebuild it from'); + expect(dirs).not.toContain('ac-badge--warn'); + }); + + it('says of a rebuildable directory that losing it costs a rebuild, not content', async () => { + const crops = { ...OK_DIR, key: 'cropped.image.directory', irreplaceable: false }; + const { dirs } = await render({ directories: [crops], enforced: true, story_media: null }); + + expect(dirs).toContain('Yes — rebuilt on demand'); + }); + + it('leads a dev checkout\'s status with the verdict, not with the location', async () => { + // "inside the build tree" alone left the reader to work out whether that was a problem. + const devUnsafe = { ...UNSAFE_DIR, label: 'ok for dev (inside the build tree)', severity: 'ok' }; + const { dirs } = await render({ directories: [devUnsafe], enforced: false, story_media: null }); + + expect(dirs).toContain('ok for dev'); + }); + it('shows the server\'s own label and severity rather than deciding either here', async () => { const { dirs } = await render({ directories: [UNSAFE_DIR], enforced: true, story_media: null }); From 7ff1cdfeb7ebfdea0c16889ceee1aea2e0a4cc1a Mon Sep 17 00:00:00 2001 From: jonfroehlich Date: Thu, 20 Aug 2026 10:58:37 -0700 Subject: [PATCH 8/8] Treat label crops as content, not cache (#4926) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- app/controllers/ImageController.scala | 7 +++-- app/modules/PersistentMediaDirCheck.scala | 29 ++++++++++--------- conf/application.conf | 10 ++++--- docs/deployment-and-stages.md | 8 +++-- test/controllers/ImageControllerSpec.scala | 9 +++--- test/js/healthMediaPanel.test.js | 4 +-- .../modules/PersistentMediaDirCheckSpec.scala | 13 +++++---- test/service/LostMediaLogSpec.scala | 2 +- test/service/MediaIntegritySpec.scala | 11 +++---- 9 files changed, 53 insertions(+), 40 deletions(-) diff --git a/app/controllers/ImageController.scala b/app/controllers/ImageController.scala index cde507c988..569216c88e 100644 --- a/app/controllers/ImageController.scala +++ b/app/controllers/ImageController.scala @@ -140,7 +140,7 @@ class ImageController @Inject() ( case None => // Reaching here means the file was on disk when this URL was signed (backupImageUrl and // getBackupImageMetadata both check first) and is gone within the signature's ~75-minute life. For an - // expired pano this store holds the only copy left anywhere, so say so (#4926) — the 404 stays bare. + // pano the provider no longer serves nothing can re-fetch it, so say so (#4926) — the 404 stays bare. lostMediaLog.reportMissing( "pano", panoId, @@ -199,8 +199,9 @@ class ImageController @Inject() ( Future.successful(Ok.sendFile(file, inline = true).as("image/png")) } else { // Same signed-URL reasoning as serveBackupImage above: cropUrl only signs a crop it just saw on disk, so a - // miss here is a file that vanished. A crop can be re-cut from pano imagery, so this is the warning tier. - lostMediaLog.reportMissing("crop", s"$labelType/$labelId", file.getAbsolutePath, irreplaceable = false) + // miss here is a file that vanished — and nothing recreates it: the crop was captured in the labeler's + // browser at labeling time, from a pano the provider may no longer serve. + lostMediaLog.reportMissing("crop", s"$labelType/$labelId", file.getAbsolutePath, irreplaceable = true) Future.successful(NotFound("Crop image not found")) } } diff --git a/app/modules/PersistentMediaDirCheck.scala b/app/modules/PersistentMediaDirCheck.scala index 267ca2d1a2..f81c729eb1 100644 --- a/app/modules/PersistentMediaDirCheck.scala +++ b/app/modules/PersistentMediaDirCheck.scala @@ -23,10 +23,10 @@ import scala.util.{Failure, Success, Try} * incomplete file (the #4925 failure) would disarm the guard exactly when it is needed. Dev and test runs skip it; * there the application root is the hand-managed repo checkout, where the relative defaults are the point. * - * A directory holding irreplaceable bytes — user uploads, or our only copies of provider-expired imagery — is - * **fatal**: refusing to boot is far cheaper than accepting content we already know will be destroyed, and the test - * stage redeploys on every push to `develop` while prod waits for a release tag, so a missing variable surfaces on - * test long before it can reach prod. The rest is derived data whose loss costs rebuild time, so it logs and lets + * A directory holding irreplaceable bytes — user uploads, label crops, or imagery that cannot be re-fetched — is + * **fatal**: refusing to boot is far cheaper than accepting content we already know will be destroyed, + * and the test stage redeploys on every push to `develop` while prod waits for a release tag, so a missing variable + * surfaces on test long before it can reach prod. Cached share previews rebuild on demand, so that one logs and lets * the app run. */ @Singleton @@ -59,8 +59,8 @@ object PersistentMediaDirCheck { * @param key Config key naming the directory. * @param envVar Environment variable a deployment sets it with, named in the failure message so the fix * doesn't require reading the config. - * @param irreplaceable Whether it holds bytes no rebuild can recreate — user uploads, or the only surviving copy - * of provider-expired imagery. These form the fatal tier; the rest only log. + * @param irreplaceable Whether it holds bytes no rebuild can recreate. These form the fatal tier; the rest only + * log. */ case class PersistentDir(key: String, envVar: String, irreplaceable: Boolean) @@ -79,14 +79,17 @@ object PersistentMediaDirCheck { def arms(environment: Environment): Boolean = environment.mode == Mode.Prod val persistentDirs: Seq[PersistentDir] = Seq( - // Crops and share previews are derived: a crop can be re-cut from pano imagery and a share preview rebuilds on - // demand, so losing them costs rebuild time, not content. - PersistentDir("cropped.image.directory", "SIDEWALK_IMAGES_DIR", irreplaceable = false), - PersistentDir("share.image.directory", "SIDEWALK_SHARE_IMAGES_DIR", irreplaceable = false), - // The self-hosted pano store backs up GSV imagery Google has already expired (pano_data.has_backup) — for those - // panos it is the only copy left anywhere, as unrecoverable as a user upload. + // A crop is a screenshot of the pano canvas taken in the labeler's browser as the label was placed + // (Canvas.saveCanvasScreenshot); nothing here rebuilds one, and the Static API still we fall back to is a + // different image that only exists while the provider still serves that pano — about half the labels on prod sit + // on panos already marked expired. + PersistentDir("cropped.image.directory", "SIDEWALK_IMAGES_DIR", irreplaceable = true), + // Locally stored pano imagery (pano_data.has_backup) the providers no longer serve, so it cannot be re-fetched. PersistentDir("pano.images.directory", "SIDEWALK_PANO_DIR", irreplaceable = true), - PersistentDir("story.media.directory", "SIDEWALK_STORY_MEDIA_DIR", irreplaceable = true) + PersistentDir("story.media.directory", "SIDEWALK_STORY_MEDIA_DIR", irreplaceable = true), + // Share previews are the one cache here: each rebuilds on demand from the label's crop, or from a Street View + // still when the crop is gone. + PersistentDir("share.image.directory", "SIDEWALK_SHARE_IMAGES_DIR", irreplaceable = false) ) /** diff --git a/conf/application.conf b/conf/application.conf index 21caedf8ed..f62a2a9420 100644 --- a/conf/application.conf +++ b/conf/application.conf @@ -265,13 +265,15 @@ custom.news.ribbon.link = null # at boot in prod mode, and every consumer resolves its path through service.MediaDirs so the check can't drift from # the write paths. -# Directory to store cropped images. +# Directory for label crops, organized as ///crop_.png. Each crop is a screenshot +# of the pano canvas taken in the labeler's browser as the label was placed, and nothing here rebuilds one. +# Irreplaceable, like the pano and story dirs. cropped.image.directory = ".crops" cropped.image.directory = ${?SIDEWALK_IMAGES_DIR} -# Directory containing self-hosted pano images, organized as ///.. These back -# up GSV imagery Google has already expired (pano_data.has_backup), so for those panos this holds the only copy -# anywhere — irreplaceable, like the story media below. +# Directory containing self-hosted pano images, organized as ///.. Holds +# imagery the providers no longer serve (pano_data.has_backup), which cannot be re-fetched — irreplaceable, like the +# story media below. pano.images.directory = ".panos" pano.images.directory = ${?SIDEWALK_PANO_DIR} diff --git a/docs/deployment-and-stages.md b/docs/deployment-and-stages.md index 023b13fd5b..02a9984326 100644 --- a/docs/deployment-and-stages.md +++ b/docs/deployment-and-stages.md @@ -289,10 +289,14 @@ outside the build tree** via its environment variable (a variable that is set bu | Config key | Env var | Holds | Missing on a deployed stage | |---|---|---|---| | `story.media.directory` | `SIDEWALK_STORY_MEDIA_DIR` | User-uploaded story photos (**irreplaceable**) | **App refuses to start** | -| `pano.images.directory` | `SIDEWALK_PANO_DIR` | Self-hosted pano store — the only copies of GSV imagery Google has expired (**irreplaceable**) | **App refuses to start** | -| `cropped.image.directory` | `SIDEWALK_IMAGES_DIR` | Label crops (re-derivable from pano imagery) | Error logged at boot | +| `pano.images.directory` | `SIDEWALK_PANO_DIR` | Locally stored pano imagery the app serves itself (**irreplaceable**) | **App refuses to start** | +| `cropped.image.directory` | `SIDEWALK_IMAGES_DIR` | Label crops — browser captures of the pano as it was labeled, with no rebuild path (**irreplaceable**) | **App refuses to start** | | `share.image.directory` | `SIDEWALK_SHARE_IMAGES_DIR` | Cached social-share previews (regenerable) | Error logged at boot | +Crops sit in the fatal tier because nothing in this app regenerates one: `/saveImage` stores a canvas screenshot the +labeler's browser took as the label was placed, and the Street View Static still used as a fallback elsewhere is a +different, smaller image of a pano the provider must still serve — which it often no longer does. + `PersistentMediaDirCheck` enforces this at boot in **prod mode** — what every staged binary runs in — so it covers every deployed stage *and* a staged binary run by hand (export the four variables to `/tmp` paths for that; CI's `e2e-smoke` job does exactly this). It deliberately does not key on `ENV_TYPE`: that variable arrives through the diff --git a/test/controllers/ImageControllerSpec.scala b/test/controllers/ImageControllerSpec.scala index 67a24acad1..e9e2e2d9df 100644 --- a/test/controllers/ImageControllerSpec.scala +++ b/test/controllers/ImageControllerSpec.scala @@ -196,8 +196,9 @@ class ImageControllerSpec extends PlaySpec with AnonSession with GuiceOneAppPerS } events must have size 1 - // A crop can be re-cut from pano imagery, so it is the warning tier rather than the paging one. - events.head.getLevel mustBe Level.WARN + // A crop exists in one place only — it was captured in the labeler's browser as the label was placed — so + // its disappearance is the error tier, not a rebuild cost. + events.head.getLevel mustBe Level.ERROR val message = events.head.getFormattedMessage message must include("crop") message must include(syntheticLabelId.toString) @@ -207,8 +208,8 @@ class ImageControllerSpec extends PlaySpec with AnonSession with GuiceOneAppPerS } "answer a signed pano URL with no backup image with a plain 404, every time it is asked" in { - // For a pano whose source imagery has expired this store holds the only copy left anywhere, so the loss is the - // error tier. The log deduplicates the repeat (LostMediaLogSpec pins that); the responses must not. + // Nothing can re-fetch a pano the provider no longer serves, so the loss is the error tier. The log + // deduplicates the repeat (LostMediaLogSpec pins that); the responses must not. val panoId = "sidewalkSpecNoSuchPano4926" val url = signingService.signedUrl(s"/backupImage/$panoId") diff --git a/test/js/healthMediaPanel.test.js b/test/js/healthMediaPanel.test.js index 46314b0d39..11713723f6 100644 --- a/test/js/healthMediaPanel.test.js +++ b/test/js/healthMediaPanel.test.js @@ -115,8 +115,8 @@ describe('HealthPage media storage panel', () => { }); it('says of a rebuildable directory that losing it costs a rebuild, not content', async () => { - const crops = { ...OK_DIR, key: 'cropped.image.directory', irreplaceable: false }; - const { dirs } = await render({ directories: [crops], enforced: true, story_media: null }); + const shareImages = { ...OK_DIR, key: 'share.image.directory', irreplaceable: false }; + const { dirs } = await render({ directories: [shareImages], enforced: true, story_media: null }); expect(dirs).toContain('Yes — rebuilt on demand'); }); diff --git a/test/modules/PersistentMediaDirCheckSpec.scala b/test/modules/PersistentMediaDirCheckSpec.scala index 466b28fef6..605d4a3bc2 100644 --- a/test/modules/PersistentMediaDirCheckSpec.scala +++ b/test/modules/PersistentMediaDirCheckSpec.scala @@ -9,8 +9,8 @@ import scala.io.Source import scala.util.Using /** - * The deployment contract behind the media directories: anything irreplaceable — user uploads, our only copies of - * provider-expired panos — has to land outside the build output tree, because a deploy deletes that whole tree + * The deployment contract behind the media directories: anything irreplaceable — user uploads, imagery that cannot + * be re-fetched — has to land outside the build output tree, because a deploy deletes that whole tree * (`sbt clean`) and rebuilds it (#4925). * * Nothing else can catch a violation. The configuration that lost a story photo was correct in dev, correct in CI, @@ -74,11 +74,12 @@ class PersistentMediaDirCheckSpec extends PlaySpec { } "the fatal set" should { - // Refusing to boot is only justified for bytes no rebuild can recreate: the story photos users gave us, and the - // self-hosted pano store — it backs up GSV imagery Google has already expired, so for those panos it is the only - // copy anywhere. Crops and share previews re-derive from them, so they must stay warn-only. + // Refusing to boot is only justified for bytes no rebuild can recreate: the story photos users gave us, imagery + // the providers no longer serve, and the label crops, each captured once in a labeler's browser. Cached share + // previews rebuild on demand, so that one must stay warn-only. "be exactly the irreplaceable directories" in { - persistentDirs.filter(_.irreplaceable).map(_.key) mustBe Seq("pano.images.directory", "story.media.directory") + persistentDirs.filter(_.irreplaceable).map(_.key) mustBe + Seq("cropped.image.directory", "pano.images.directory", "story.media.directory") } // The failure message tells the operator which variable to set. If this mapping drifts from application.conf, diff --git a/test/service/LostMediaLogSpec.scala b/test/service/LostMediaLogSpec.scala index 41722aee7b..00d0a11a98 100644 --- a/test/service/LostMediaLogSpec.scala +++ b/test/service/LostMediaLogSpec.scala @@ -82,7 +82,7 @@ class LostMediaLogSpec extends PlaySpec { // The tiering is the same call PersistentMediaDirCheck makes, and it is what decides whether anyone is paged. val error = captured(_.reportMissing("pano", "abc", "/srv/panos/abc.jpg", irreplaceable = true)) error.head.getLevel mustBe Level.ERROR - val warn = captured(_.reportMissing("crop", "CurbRamp/7", "/srv/crops/crop_7.png", irreplaceable = false)) + val warn = captured(_.reportMissing("share_image", "7", "/srv/share/share_7.jpg", irreplaceable = false)) warn.head.getLevel mustBe Level.WARN } diff --git a/test/service/MediaIntegritySpec.scala b/test/service/MediaIntegritySpec.scala index f2858f2390..eeeaebde17 100644 --- a/test/service/MediaIntegritySpec.scala +++ b/test/service/MediaIntegritySpec.scala @@ -87,7 +87,7 @@ class MediaIntegritySpec extends PlaySpec { } } - private val storyDir = persistentDirs.find(_.irreplaceable).value + private val irreplaceableDir = persistentDirs.find(_.irreplaceable).value // The permission branches can't be provoked through the filesystem from a suite that runs as root — which CI and // the dev container both do, and where chmod 000 still reads and writes fine — so they are pinned on the rules @@ -95,22 +95,23 @@ class MediaIntegritySpec extends PlaySpec { "dirStatus" should { "call a directory this process cannot read bad, since nothing in it can be verified" in { val probe = MediaIntegrity.DirProbe(exists = true, readable = false, writable = true) - val status = MediaIntegrity.dirStatus(storyDir, "/srv/media", probe, None, enforced = true) + val status = MediaIntegrity.dirStatus(irreplaceableDir, "/srv/media", probe, None, enforced = true) status.status mustBe "not_readable" status.severity mustBe "bad" - status.detail.value must include(storyDir.envVar) + status.detail.value must include(irreplaceableDir.envVar) } "call a directory this process cannot write to bad, since uploads will fail against it" in { val probe = MediaIntegrity.DirProbe(exists = true, readable = true, writable = false) - val status = MediaIntegrity.dirStatus(storyDir, "/srv/media", probe, None, enforced = true) + val status = MediaIntegrity.dirStatus(irreplaceableDir, "/srv/media", probe, None, enforced = true) status.status mustBe "not_writable" status.severity mustBe "bad" } "report an unsafe directory before either permission, since a deploy deleting it outranks both" in { val probe = MediaIntegrity.DirProbe(exists = true, readable = false, writable = false) - val status = MediaIntegrity.dirStatus(storyDir, "/srv/media", probe, Some("in the wipe zone"), enforced = true) + val status = + MediaIntegrity.dirStatus(irreplaceableDir, "/srv/media", probe, Some("in the wipe zone"), enforced = true) status.status mustBe "unsafe" } }