fix: scheduler reconcile

backend/index.ts

@@ -222,6 +222,10 @@ async function postBoot() {
   // -> The icon cache is derived from the db and starts empty on a fresh instance
   await WIKI.models.icons.ensureCacheDir()
 
+  // -> The system's cron entries are defined in code, so they are brought in line here rather than
+  //    only seeded on a fresh database — must precede the scheduler start that queues from them
+  await WIKI.models.jobs.reconcileSchedule()
+
   await WIKI.dbManager.subscribeToNotifications()
   // -> Its own postgres listener, on its own channel: collaboration traffic is far heavier than the
   //    event bus's and has nothing to do with it. Must follow the sites cache, which the websocket

backend/locales/en.json

@@ -904,6 +904,7 @@
   "admin.storage.stateActive": "Healthy",
   "admin.storage.stateError": "Error",
   "admin.storage.stateInactive": "Not in use",
+  "admin.storage.stateLastRun": "Last activity {date}",
   "admin.storage.stateNoContentTypes": "No content type",
   "admin.storage.stateWarning": "Degraded",
   "admin.storage.status": "Status",

backend/models/jobs.ts

@@ -16,6 +16,43 @@ export interface JobHistoryPage {
   jobs: (typeof jobHistoryTable.$inferSelect)[]
 }
 
+/** One entry of the system's own cron schedule. */
+interface SystemScheduleEntry {
+  task: string
+  cron: string
+}
+
+/**
+ * The system's cron schedule, in full.
+ *
+ * This is the definition, not a seed: `reconcileSchedule()` makes the `jobSchedule` rows of type
+ * `system` match it on every boot. Adding a task here is therefore all it takes to have every
+ * instance start running it — a list that was only inserted on first run would leave a task nobody's
+ * database had ever heard of unscheduled forever, which is exactly what happened to
+ * `syncStorageTargets`.
+ *
+ * A `type: 'user'` row is nothing to do with this and is never touched.
+ */
+export const SYSTEM_SCHEDULE: SystemScheduleEntry[] = [
+  { task: 'checkVersion', cron: '0 0 * * *' },
+  { task: 'cleanJobHistory', cron: '5 0 * * *' },
+  // { task: 'refreshAutocomplete', cron: '0 */6 * * *' },
+  { task: 'purgeRateLimits', cron: '10 * * * *' },
+  { task: 'updateLocales', cron: '0 0 * * *' },
+  // -> Every minute, and the task decides which sites are actually due: the interval is a per-site
+  //    setting, so the tick has to be as fine as the shortest one anybody can ask for
+  { task: 'syncStorageTargets', cron: '* * * * *' }
+]
+
+/**
+ * Advisory lock held for the length of `reconcileSchedule()`'s transaction.
+ *
+ * Every instance reconciles as it boots, and a restarted HA set boots them together — without this
+ * they all read the same missing row and all insert it. Transaction-scoped, so it is released with
+ * the commit whatever happens.
+ */
+const SCHEDULE_LOCK_KEY = 4210001
+
 /**
  * Jobs model
  *
@@ -26,45 +63,11 @@ export interface JobHistoryPage {
 class Jobs {
   /**
    * Initialize jobs table
+   *
+   * Only the cron lock: the schedule itself is `reconcileSchedule()`'s, on this boot and every one
+   * after it.
    */
   async init(): Promise<void> {
-    WIKI.logger.info('Inserting scheduled jobs...')
-
-    await WIKI.db.insert(jobScheduleTable).values([
-      {
-        task: 'checkVersion',
-        cron: '0 0 * * *',
-        type: 'system'
-      },
-      {
-        task: 'cleanJobHistory',
-        cron: '5 0 * * *',
-        type: 'system'
-      },
-      // {
-      //   task: 'refreshAutocomplete',
-      //   cron: '0 */6 * * *',
-      //   type: 'system'
-      // },
-      {
-        task: 'purgeRateLimits',
-        cron: '10 * * * *',
-        type: 'system'
-      },
-      {
-        task: 'updateLocales',
-        cron: '0 0 * * *',
-        type: 'system'
-      },
-      {
-        // -> Every minute, and the task decides which sites are actually due: the interval is a
-        //    per-site setting, so the tick has to be as fine as the shortest one anybody can ask for
-        task: 'syncStorageTargets',
-        cron: '* * * * *',
-        type: 'system'
-      }
-    ])
-
     await WIKI.db.insert(jobLockTable).values({
       key: 'cron',
       lastCheckedBy: 'init',
@@ -77,6 +80,83 @@ class Jobs {
     })
   }
 
+  /**
+   * Bring the `system` cron entries in line with `SYSTEM_SCHEDULE`.
+   *
+   * Runs on every boot, which is what makes the code the authority on what the wiki runs on a
+   * schedule: a task added to the list appears on an existing instance, a task removed from it stops
+   * being queued, and a changed cron takes effect. A row deleted by hand comes back, too.
+   *
+   * Pending iterations of anything that changed are dropped, since `scheduler.addScheduled()` only
+   * ever adds: left alone, a task whose cron just changed would keep running at its old times for
+   * the next 24 hours, and one that no longer exists would be queued for a task the scheduler cannot
+   * load.
+   */
+  async reconcileSchedule(): Promise<void> {
+    await WIKI.db.transaction(async (trx: any) => {
+      await trx.execute(sql`SELECT pg_advisory_xact_lock(${SCHEDULE_LOCK_KEY}::bigint)`)
+
+      const existing = await trx
+        .select()
+        .from(jobScheduleTable)
+        .where(eq(jobScheduleTable.type, 'system'))
+
+      const wanted = new Map(SYSTEM_SCHEDULE.map((entry) => [entry.task, entry]))
+      const kept = new Set<string>()
+      const staleIds: string[] = []
+      // -> Tasks whose queued iterations no longer match what is scheduled for them
+      const dirtyTasks = new Set<string>()
+
+      for (const row of existing) {
+        const entry = wanted.get(row.task)
+        // -> A task that is gone, or a duplicate of one already kept: either way this row goes
+        if (!entry || kept.has(row.task)) {
+          staleIds.push(row.id)
+          dirtyTasks.add(row.task)
+          continue
+        }
+        kept.add(row.task)
+        if (row.cron !== entry.cron) {
+          await trx
+            .update(jobScheduleTable)
+            .set({
+              cron: entry.cron,
+              updatedAt: new Date(Temporal.Now.instant().epochMilliseconds)
+            })
+            .where(eq(jobScheduleTable.id, row.id))
+          dirtyTasks.add(row.task)
+        }
+      }
+
+      if (staleIds.length > 0) {
+        await trx.delete(jobScheduleTable).where(inArray(jobScheduleTable.id, staleIds))
+      }
+
+      const missing = SYSTEM_SCHEDULE.filter((entry) => !kept.has(entry.task))
+      if (missing.length > 0) {
+        await trx.insert(jobScheduleTable).values(
+          missing.map((entry) => ({
+            task: entry.task,
+            cron: entry.cron,
+            type: 'system'
+          }))
+        )
+        for (const entry of missing) {
+          dirtyTasks.add(entry.task)
+        }
+      }
+
+      if (dirtyTasks.size > 0) {
+        await trx
+          .delete(jobsTable)
+          .where(and(eq(jobsTable.isScheduled, true), inArray(jobsTable.task, [...dirtyTasks])))
+        WIKI.logger.info(
+          `Scheduled tasks reconciled: ${[...dirtyTasks].sort().join(', ')} [ UPDATED ]`
+        )
+      }
+    })
+  }
+
   /**
    * Whether the scheduler is keeping up with its cron duties.
    *

backend/models/storage.ts

@@ -92,6 +92,17 @@ const SIZE_UNIT_BYTES: Record<string, number> = {
  */
 const TARGET_CACHE_TTL_MS = 30_000
 
+/**
+ * How stale a target's recorded state may get before an unchanged outcome is written again.
+ *
+ * `recordState` runs on the success path of every upload, every page copy and every read, so writing
+ * each one would put a row update behind each of them. Writing none of them was worse: an outcome
+ * that never changes never gets a timestamp, which is how a target that had only ever succeeded came
+ * to report no activity at all. A minute is under the resolution the Status card reads the timestamp
+ * back at, so the cost buys nothing observable beyond it.
+ */
+const STATE_REFRESH_INTERVAL_MS = 60_000
+
 /** An action a module knows how to run on demand, as declared by its `definition.yml`. */
 /**
  * How a target is behaving, as opposed to how it is configured.
@@ -1678,16 +1689,27 @@ class Storage {
    * a full disk that gets emptied stops being reported without anybody dismissing anything — and it
    * is why the timestamp travels with it, so that "healthy" can be read as of when.
    *
-   * Written only when it says something new, because the success path runs on every upload and every
-   * page save. The cached target is patched in step, so the check keeps holding within the cache's
-   * lifetime rather than costing a read.
+   * So `updatedAt` is when the target was last *used*, not when its status last *changed* — the two
+   * only differ for an outcome that repeats, and the first is the more useful of them: a target
+   * reading healthy as of last March is a target nothing is reaching, which the other reading cannot
+   * tell apart from one that is working. A repeating failure likewise reads as still failing rather
+   * than as having failed once.
+   *
+   * A new outcome is always written. An unchanged one is written at most once every
+   * `STATE_REFRESH_INTERVAL_MS`, because the success path runs on every upload and every page save
+   * and each write is a row update. The cached target is patched in step, so both checks hold within
+   * the cache's lifetime rather than costing a read.
    */
   async recordState(
     target: StorageTarget,
     status: StorageTargetStatus,
     message = ''
   ): Promise<void> {
-    if (target.state.status === status && target.state.message === message) {
+    if (
+      target.state.status === status &&
+      target.state.message === message &&
+      !this.isStateStale(target.state)
+    ) {
       return
     }
     const state: StorageTargetState = {
@@ -1700,6 +1722,22 @@ class Storage {
     await WIKI.db.update(storageTable).set({ state }).where(eq(storageTable.id, target.id))
   }
 
+  /**
+   * Whether a target's recorded state is old enough to be worth writing again unchanged.
+   *
+   * A state with no timestamp counts as stale: that is a target whose outcome has never been written
+   * at all, which is precisely the one that most needs recording.
+   */
+  isStateStale(state: StorageTargetState): boolean {
+    if (!state.updatedAt) {
+      return true
+    }
+    return (
+      Temporal.Now.instant().since(Temporal.Instant.from(state.updatedAt)).total('milliseconds') >=
+      STATE_REFRESH_INTERVAL_MS
+    )
+  }
+
   /**
    * How this site's storage is behaving, as little as a status indicator needs to know.
    *

frontend/src/pages/AdminStorage.vue

@@ -5,7 +5,9 @@
         <img class="admin-icon animated fadeInLeft" src="/_assets/icons/fluent-ssd-animated.svg" />
       </div>
       <div class="min-w-0 flex-1 pl-4">
-        <div class="text-h5 admin-page-title animated fadeInLeft">{{ t('admin.storage.title') }}</div>
+        <div class="text-h5 admin-page-title animated fadeInLeft">
+          {{ t('admin.storage.title') }}
+        </div>
         <div class="text-subtitle1 text-grey animated fadeInLeft wait-p2s">
           {{ t('admin.storage.subtitle') }}
         </div>
@@ -419,22 +421,35 @@
                     {{ currentState.label }}
                   </w-item-label>
                   <!-- -> What actually went wrong, which is the whole use of the two unhealthy
-                       states: "Error" on its own only sends an administrator to the server log.
+                       states: "Error" on its own only sends an administrator to the server log. Only
+                       those two set it, so a healthy card goes straight from the status to the
+                       timestamp under it.
 
                        The captions carry their own top margins rather than the section carrying a
                        gap: a gap belongs to `WItemSection`, which every item in the admin area uses
-                       for the tight label-and-hint pairing that wants no space at all. Only the two
-                       unhealthy states set either of these, and they set both, so a one-line card
-                       never ends up with a margin hanging off it.
+                       for the tight label-and-hint pairing that wants no space at all. Each is set
+                       only where the label is rendered, so a card missing either never ends up with
+                       a margin hanging off it.
 
-                       Uneven on purpose. The wider gap under the status separates the heading from
-                       the detail, and the narrow one keeps the message and the moment it happened
-                       reading as the one thing they are. -->
+                       Uneven on purpose. The wider gap separates the status from what follows it,
+                       and the narrow one keeps a message and the moment it happened reading as the
+                       one thing they are -- which is why the timestamp takes the wide gap itself
+                       when there is no message above it to sit against. -->
                   <w-item-label caption class="mt-3" v-if="currentState.message">
                     {{ currentState.message }}
                   </w-item-label>
-                  <w-item-label caption class="mt-1" v-if="currentState.since">
-                    {{ relativeDate(currentState.since) }}
+                  <!-- -> Named rather than a bare relative date, which under "Not in use" would
+                       read as how long the target has been unused rather than when it was last
+                       asked for anything. Absent entirely on a target nothing has been dispatched
+                       to yet: `relativeDate`'s own `---` would claim an activity there was none
+                       of. -->
+                  <w-item-label
+                    caption
+                    :class="currentState.message ? `mt-1` : `mt-3`"
+                    v-if="currentState.since">
+                    {{
+                      t('admin.storage.stateLastRun', { date: relativeDate(currentState.since) })
+                    }}
                   </w-item-label>
                 </w-item-section>
               </w-item>
@@ -721,17 +736,30 @@ const assetContentTypes = [
  */
 const currentState = computed(() => {
   const saved = state.target?.saved
+  /*
+    Read before the configuration branches below return, because `since` belongs to every one of
+    them: it is when the target was last asked to do anything, which is as much a fact about one
+    that has since been turned off as about a healthy one. Only the target having never been used at
+    all leaves it out.
+  */
+  const health = state.target?.state ?? {}
+  const since = health.updatedAt ?? null
   if (!saved?.isEnabled) {
-    return { label: t('admin.storage.stateInactive'), text: 'text-grey', dot: 'bg-grey-5' }
+    return {
+      label: t('admin.storage.stateInactive'),
+      text: 'text-grey',
+      dot: 'bg-grey-5',
+      since
+    }
   }
   if (saved.activeTypes.length < 1) {
     return {
       label: t('admin.storage.stateNoContentTypes'),
       text: 'text-negative',
-      dot: 'bg-negative'
+      dot: 'bg-negative',
+      since
     }
   }
-  const health = state.target.state ?? {}
   if (health.status === 'error') {
     return {
       label: t('admin.storage.stateError'),
@@ -739,7 +767,7 @@ const currentState = computed(() => {
       dot: 'bg-negative',
       flash: true,
       message: health.message,
-      since: health.updatedAt
+      since
     }
   }
   if (health.status === 'warning') {
@@ -749,10 +777,15 @@ const currentState = computed(() => {
       dot: 'bg-warning',
       flash: true,
       message: health.message,
-      since: health.updatedAt
+      since
     }
   }
-  return { label: t('admin.storage.stateActive'), text: 'text-positive', dot: 'bg-positive' }
+  return {
+    label: t('admin.storage.stateActive'),
+    text: 'text-positive',
+    dot: 'bg-positive',
+    since
+  }
 })
 
 /** Whether the selected target is enabled *as saved* — see `savedSnapshot`. */