From 179283b23338fa32fdfbb25f8ba6f48f0ba3b100 Mon Sep 17 00:00:00 2001 From: adamecker Date: Sat, 31 Oct 2026 06:52:27 +0000 Subject: [PATCH 1/5] feat(plugins): push plugin storage writes as plugin.storage.changed (#347) Per-plugin storage counters on the #339 path: a write moves only its own plugin's counter, pushed over the event stream within a second and caught up by the 15s GET /api/changes check, so plugins need polling only as the older-server fallback. The event carries the plugin id only, never a key; the frame reloads what it uses. Counters are created on a plugin's first storage write and dropped when its data is purged, so the set grows and shrinks with installed plugins; the ETag is built from the five fixed kinds plus the plugin kinds in sorted order. A frame receives only its own plugin's events: the dashboard subscribes per plugin id and forwards nothing else into the frame. Covered write paths: PUT, DELETE and POST increment through the request hook, server-side reactions, and uninstall with purge; reactions never fire on a storage change itself, which would self-trigger. The guide documents the no-key payload and the per-plugin scoping. Tests: per-path and isolation coverage in server/tests (unit + real server, incl. the stream carrying no key) and client tracker tests (a frame's subscription hears only its own plugin, baseline rules, prune on uninstall, reinstall reading as changed). --- client/src/app.jsx | 4 + client/src/components/PluginWidgetWrapper.jsx | 30 ++- client/src/utils/householdChanges.js | 121 ++++++++-- client/src/utils/householdChanges.test.js | 110 ++++++++- docs/guides/plugin-development.md | 11 + server/index.js | 19 +- server/services/pluginEvents.js | 4 + server/tests/householdChanges.test.js | 70 ++++++ server/tests/pluginStorageChanges.test.js | 226 ++++++++++++++++++ server/utils/householdChanges.js | 118 ++++++++- 10 files changed, 684 insertions(+), 29 deletions(-) create mode 100644 server/tests/pluginStorageChanges.test.js diff --git a/client/src/app.jsx b/client/src/app.jsx index 28039718..41e495b9 100644 --- a/client/src/app.jsx +++ b/client/src/app.jsx @@ -1411,6 +1411,10 @@ const App = () => { colors={themeColors} transparentBackground={pSettings.transparent || false} events={plugin.manifest?.events || []} + // The manifest id is the plugin id the server counts storage + // under (issue #347); null for legacy widgets, which declare no + // events and never get one. + pluginId={plugin.pluginId || plugin.manifest?.id || null} hiddenControls={unprefixedHiddenControlsFor(hiddenControls, plugin.manifest?.id)} touch={frameTouch} />, diff --git a/client/src/components/PluginWidgetWrapper.jsx b/client/src/components/PluginWidgetWrapper.jsx index a53fb8cc..ad35c394 100644 --- a/client/src/components/PluginWidgetWrapper.jsx +++ b/client/src/components/PluginWidgetWrapper.jsx @@ -4,7 +4,7 @@ import { useTranslation } from 'react-i18next'; import { API_BASE_URL } from '../utils/apiConfig.js'; import { getDeviceName } from '../utils/deviceName.js'; import { subscribePluginEvents } from '../utils/pluginEventBridge.js'; -import { HOUSEHOLD_DOMAINS, knownHouseholdRevisions, subscribeHouseholdChanges } from '../utils/householdChanges.js'; +import { HOUSEHOLD_DOMAINS, knownHouseholdRevisions, subscribeHouseholdChanges, subscribePluginStorageChanges } from '../utils/householdChanges.js'; import { acceptPluginDataMessage, emitPluginDataChanged } from '../utils/pluginDataBridge.js'; import { acceptPluginCelebrateMessage, requestCelebration } from '../utils/celebrations.js'; import { THEME_TOKENS_APPLIED_EVENT, buildPluginThemeMessage, readRoleTokens } from '../utils/pluginThemeBridge.js'; @@ -36,6 +36,10 @@ const PluginWidgetWrapper = ({ transparentBackground = false, refreshNonce = 0, events = [], + // The manifest's plugin id, for plugin.storage.changed (issue #347): the + // frame hears only its own plugin's storage writes. Null for legacy + // widgets, which declare no events and never get one. + pluginId = null, // Control Limits: this plugin's own hidden control ids, already unprefixed by // the dashboard. HomeGlow cannot reach into the iframe, so the plugin is told // and does its own hiding — see docs/guides/plugin-development.md. @@ -73,7 +77,7 @@ const PluginWidgetWrapper = ({ return subscribePluginEvents((message) => { // Delivered below, from the same source as the dashboard's own // widgets, so it arrives once and where the stream doesn't reach. - if (message.event === 'household.changed') return; + if (message.event === 'household.changed' || message.event === 'plugin.storage.changed') return; if (!declared.has(message.event)) return; const target = iframeRef.current?.contentWindow; if (!target) return; @@ -104,6 +108,28 @@ const PluginWidgetWrapper = ({ }); }, [eventsKey]); + // plugin.storage.changed (issue #347), for a plugin that declared it: only + // this frame's own plugin's counter moves reach it — the subscription is + // per plugin, so a frame never hears another plugin's writes, and the + // payload carries the plugin id only, never a key, so the frame reloads + // what it uses and learns nothing else. + useEffect(() => { + if (!pluginId || !eventsKey.split(',').includes('plugin.storage.changed')) return undefined; + return subscribePluginStorageChanges(pluginId, () => { + const target = iframeRef.current?.contentWindow; + if (!target) return; + target.postMessage( + { + type: 'homeglow:event', + event: 'plugin.storage.changed', + payload: { pluginId }, + emittedAt: new Date().toISOString(), + }, + TO_FRAME + ); + }); + }, [eventsKey, pluginId]); + // The reverse direction: a plugin that wrote core data through the REST API // tells the host, so widgets showing that data can refetch instead of // waiting out their own refresh timer. diff --git a/client/src/utils/householdChanges.js b/client/src/utils/householdChanges.js index 1b91e2b4..0196b7c4 100644 --- a/client/src/utils/householdChanges.js +++ b/client/src/utils/householdChanges.js @@ -19,17 +19,37 @@ import { subscribePluginEvents } from './pluginEventBridge.js'; // Coarse on purpose: a subscriber learns that a kind of data changed, never // what, and reloads all of it. No write can be missed by a widget asking // the wrong question. +// +// Plugin storage rides the same two paths (issue #347): the server keeps +// one counter per plugin under `plugin:` in the same snapshot, +// created on a plugin's first storage write and dropped when its data is +// purged. A plugin's frame hears only its own plugin's counter move, never +// another plugin's — the per-plugin subscription below is where that +// scoping lives, and the frame itself gets the plugin id only, never a key. export const HOUSEHOLD_DOMAINS = Object.freeze(['chores', 'users', 'prizes', 'settings', 'calendar']); export const CHECK_INTERVAL_MS = 15 * 1000; +const PLUGIN_DOMAIN_PREFIX = 'plugin:'; +const pluginDomain = (pluginId) => `${PLUGIN_DOMAIN_PREFIX}${pluginId}`; + /** The kinds whose counters differ between two snapshots; none before the first. Pure. */ export function changedDomains(known, next) { if (!known || !next) return []; return HOUSEHOLD_DOMAINS.filter((domain) => next[domain] !== undefined && next[domain] !== known[domain]); } +/** The plugin ids whose storage counters differ, or that newly appeared; none before the first. Pure. */ +export function changedPluginIds(known, next) { + if (!known || !next) return []; + return Object.keys(next) + .filter((key) => key.startsWith(PLUGIN_DOMAIN_PREFIX)) + .map((key) => key.slice(PLUGIN_DOMAIN_PREFIX.length)) + .filter((pluginId) => next[pluginDomain(pluginId)] !== known[pluginDomain(pluginId)]); +} + const subscribers = new Set(); +const pluginSubscribers = new Set(); let known = null; let etag = null; let mode = 'interval'; @@ -40,15 +60,7 @@ let stopEvents = null; const isVisible = () => typeof document === 'undefined' || document.visibilityState !== 'hidden'; -function apply(next) { - if (!next || typeof next !== 'object') return; - if (!known) { - known = { ...next }; - return; - } - const moved = changedDomains(known, next); - if (!moved.length) return; - known = { ...known, ...next }; +function notifyHousehold(moved) { for (const { domains, handler } of subscribers) { const theirs = moved.filter((domain) => domains.has(domain)); if (!theirs.length) continue; @@ -60,6 +72,61 @@ function apply(next) { } } +function notifyPlugins(movedPluginIds) { + if (!movedPluginIds.length) return; + for (const { pluginId, handler } of pluginSubscribers) { + if (!movedPluginIds.includes(pluginId)) continue; + try { + handler(pluginId); + } catch (error) { + console.error('Plugin storage change subscriber failed:', error); + } + } +} + +// A full snapshot, from GET /api/changes only: household kinds plus plugin +// counters. Plugin keys that vanished (uninstall with purge) drop out of +// what this page knows, so they can't linger as stale counters. +function applySnapshot(next) { + if (!next || typeof next !== 'object') return; + if (!known) { + known = { ...next }; + return; + } + const moved = changedDomains(known, next); + const movedPlugins = changedPluginIds(known, next); + if (!moved.length && !movedPlugins.length) return; + const pruned = { ...known, ...next }; + for (const key of Object.keys(pruned)) { + if (key.startsWith(PLUGIN_DOMAIN_PREFIX) && !(key in next)) delete pruned[key]; + } + known = pruned; + notifyHousehold(moved); + notifyPlugins(movedPlugins); +} + +// Event-stream household revisions: household kinds only, merged into what +// this page knows without pruning and without establishing the baseline — +// the GET baseline follows right behind, and the stream is only a fast +// path, so a message that arrives before it is safely dropped. +function applyRevisions(revisions) { + if (!revisions || typeof revisions !== 'object' || !known) return; + const moved = changedDomains(known, revisions); + if (!moved.length) return; + known = { ...known, ...revisions }; + notifyHousehold(moved); +} + +// One plugin's counter from the event stream. Same baseline rule as above: +// before the first GET answer, there is nothing to compare against. +function applyPluginRevision(pluginId, revision) { + if (!known || typeof pluginId !== 'string' || typeof revision !== 'number') return; + const domain = pluginDomain(pluginId); + if (known[domain] === revision) return; + known = { ...known, [domain]: revision }; + notifyPlugins([pluginId]); +} + /** Ask the server now. Resolves when the answer has been applied. */ export async function checkHouseholdChanges() { if (checking) return; @@ -71,7 +138,7 @@ export async function checkHouseholdChanges() { }); if (response.status === 200) { etag = response.headers?.etag || null; - apply(response.data); + applySnapshot(response.data); } } catch { // Offline or restarting: the next check, or the next visibility, retries. @@ -103,7 +170,10 @@ function start() { // checks alone keep up. if (typeof EventSource !== 'undefined') { stopEvents = subscribePluginEvents((message) => { - if (message?.event === 'household.changed') apply(message.payload?.revisions); + if (message?.event === 'household.changed') applyRevisions(message.payload?.revisions); + else if (message?.event === 'plugin.storage.changed') { + applyPluginRevision(message.payload?.pluginId, message.payload?.revision); + } }); } } @@ -140,17 +210,40 @@ export function subscribeHouseholdChanges(domains, handler) { start(); return () => { subscribers.delete(entry); - if (!subscribers.size) stop(); + if (!subscribers.size && !pluginSubscribers.size) stop(); + }; +} + +/** + * Call `handler(pluginId)` whenever that plugin's storage counter moves on + * the server — from the event stream within a second, or from the 15-second + * check where the stream doesn't reach. A subscriber hears only its own + * plugin: another plugin's writes never reach it, which is what keeps one + * plugin's frames from learning about another plugin's storage (issue + * #347). Returns an unsubscribe function. The checks run while anything is + * subscribed. + */ +export function subscribePluginStorageChanges(pluginId, handler) { + const entry = { pluginId, handler }; + pluginSubscribers.add(entry); + start(); + return () => { + pluginSubscribers.delete(entry); + if (!subscribers.size && !pluginSubscribers.size) stop(); }; } -/** The counters this page last saw, or null before the first answer. */ -export const knownHouseholdRevisions = () => (known ? { ...known } : null); +/** The household counters this page last saw, or null before the first answer. Plugin counters are never included: a frame that declared household.changed must not learn about plugins' storage through it. */ +export const knownHouseholdRevisions = () => { + if (!known) return null; + return Object.fromEntries(HOUSEHOLD_DOMAINS.map((domain) => [domain, known[domain]])); +}; /** Tests only: forget everything. */ export function resetHouseholdChangesForTests() { stop(); subscribers.clear(); + pluginSubscribers.clear(); known = null; etag = null; mode = 'interval'; diff --git a/client/src/utils/householdChanges.test.js b/client/src/utils/householdChanges.test.js index b317e1b7..1bced01a 100644 --- a/client/src/utils/householdChanges.test.js +++ b/client/src/utils/householdChanges.test.js @@ -1,16 +1,20 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; vi.mock('axios', () => ({ default: { get: vi.fn() } })); -vi.mock('./pluginEventBridge.js', () => ({ subscribePluginEvents: vi.fn(() => () => {}) })); +let sseHandler = null; +vi.mock('./pluginEventBridge.js', () => ({ subscribePluginEvents: vi.fn((handler) => { sseHandler = handler; return () => {}; }) })); const axios = (await import('axios')).default; const { CHECK_INTERVAL_MS, changedDomains, + changedPluginIds, checkHouseholdChanges, + knownHouseholdRevisions, resetHouseholdChangesForTests, setHouseholdChangesMode, subscribeHouseholdChanges, + subscribePluginStorageChanges, } = await import('./householdChanges.js'); const revisions = (overrides = {}) => ({ chores: 10, users: 10, prizes: 10, settings: 10, calendar: 10, ...overrides }); @@ -151,3 +155,107 @@ describe('household changes', () => { expect(handler).toHaveBeenCalledWith(['chores']); }); }); + +describe('changedPluginIds', () => { + it('names the plugins whose counters differ or newly appeared, and none before the first answer', () => { + const before = revisions({ 'plugin:plug-a': 5, 'plugin:plug-b': 7 }); + const after = revisions({ 'plugin:plug-a': 6, 'plugin:plug-b': 7, 'plugin:plug-c': 1 }); + expect(changedPluginIds(before, after)).toEqual(['plug-a', 'plug-c']); + expect(changedPluginIds(before, before)).toEqual([]); + expect(changedPluginIds(before, revisions())).toEqual([], 'a removed plugin is a prune, not a change'); + expect(changedPluginIds(null, after)).toEqual([]); + }); +}); + +describe('plugin storage changes', () => { + let visibility; + beforeEach(() => { + resetHouseholdChangesForTests(); + axios.get.mockReset(); + visibility = stubDocument(); + }); + afterEach(() => { + resetHouseholdChangesForTests(); + vi.useRealTimers(); + delete globalThis.document; + delete globalThis.EventSource; + }); + + it('a plugin subscriber hears only its own plugin, from the check and from the stream', async () => { + globalThis.EventSource = function () {}; + axios.get.mockResolvedValueOnce(answer(revisions({ 'plugin:plug-a': 5, 'plugin:plug-b': 7 }))); + const handlerA = vi.fn(); + const handlerB = vi.fn(); + subscribePluginStorageChanges('plug-a', handlerA); + subscribePluginStorageChanges('plug-b', handlerB); + await flush(); + expect(handlerA).not.toHaveBeenCalled(); + expect(handlerB).not.toHaveBeenCalled(); + + axios.get.mockResolvedValueOnce(answer(revisions({ 'plugin:plug-a': 6, 'plugin:plug-b': 7 }))); + await checkHouseholdChanges(); + expect(handlerA).toHaveBeenCalledWith('plug-a'); + expect(handlerB).not.toHaveBeenCalled(); + + axios.get.mockResolvedValueOnce(answer(revisions({ 'plugin:plug-a': 6, 'plugin:plug-b': 7, 'plugin:plug-c': 1 }))); + await checkHouseholdChanges(); + expect(handlerA).toHaveBeenCalledTimes(1); + expect(handlerB).not.toHaveBeenCalled(); + + sseHandler({ event: 'plugin.storage.changed', payload: { pluginId: 'plug-b', revision: 8 } }); + expect(handlerB).toHaveBeenCalledWith('plug-b'); + expect(handlerA).toHaveBeenCalledTimes(1); + + sseHandler({ event: 'plugin.storage.changed', payload: { pluginId: 'plug-c', revision: 1 } }); + expect(handlerA).toHaveBeenCalledTimes(1); + expect(handlerB).toHaveBeenCalledTimes(1); + + sseHandler({ event: 'household.changed', payload: { domains: ['chores'], revisions: { chores: 11 } } }); + expect(handlerA).toHaveBeenCalledTimes(1); + expect(handlerB).toHaveBeenCalledTimes(1); + }); + + it('a stream message before the baseline is dropped, and a reinstall reads as changed', async () => { + globalThis.EventSource = function () {}; + const handler = vi.fn(); + subscribePluginStorageChanges('plug-b', handler); + // Let start's own first check settle (it had no answer queued) before + // queueing the baseline, or it would swallow the queue. + await flush(); + sseHandler({ event: 'plugin.storage.changed', payload: { pluginId: 'plug-b', revision: 9 } }); + expect(handler).not.toHaveBeenCalled(); + + axios.get.mockResolvedValueOnce(answer(revisions({ 'plugin:plug-b': 7 }))); + await checkHouseholdChanges(); + // The uninstall prune: the next check drops the key, and nothing fires. + axios.get.mockResolvedValueOnce(answer(revisions())); + await checkHouseholdChanges(); + expect(handler).not.toHaveBeenCalled(); + // A reinstall starts from a fresh counter, which reads as a change. + axios.get.mockResolvedValueOnce(answer(revisions({ 'plugin:plug-b': 3 }))); + await checkHouseholdChanges(); + expect(handler).toHaveBeenCalledWith('plug-b'); + }); + + it('a household subscriber never hears plugin counters, and household revisions exclude them', async () => { + axios.get.mockResolvedValueOnce(answer(revisions({ 'plugin:plug-a': 5 }))); + const handler = vi.fn(); + subscribeHouseholdChanges(['chores'], handler); + await flush(); + expect(knownHouseholdRevisions()).toEqual(revisions()); + axios.get.mockResolvedValueOnce(answer(revisions({ 'plugin:plug-a': 6 }))); + await checkHouseholdChanges(); + expect(handler).not.toHaveBeenCalled(); + }); + + it('stops checking when the last plugin subscriber leaves', async () => { + vi.useFakeTimers(); + axios.get.mockResolvedValue(notModified); + const stop = subscribePluginStorageChanges('plug-a', () => {}); + await vi.advanceTimersByTimeAsync(0); + stop(); + await vi.advanceTimersByTimeAsync(CHECK_INTERVAL_MS * 3); + expect(axios.get).toHaveBeenCalledTimes(1); + expect(visibility.listeners.size).toBe(0); + }); +}); diff --git a/docs/guides/plugin-development.md b/docs/guides/plugin-development.md index 60134be5..b93b6d88 100644 --- a/docs/guides/plugin-development.md +++ b/docs/guides/plugin-development.md @@ -418,10 +418,21 @@ const off = HomeGlow.on('clam.withdrawn', (payload, meta) => { | `prize.redeemed` | a prize-store request is approved | `{ userId, prizeId, offerId, prizeName, cost, newTotal }` | | `ha.state` | Home Assistant entities changed | `{ entityIds }`: a nudge to read state again (§5a), never the state itself | | `household.changed` | household data changed, on any display or by the server | `{ domains, revisions }`: which kinds changed (`chores`, `users`, `prizes`, `settings`, `calendar`). Reload what you show; it never says what changed. Delivered within a second where the event stream works, and within 15 seconds where it doesn't | +| `plugin.storage.changed` | your plugin's storage changed, on any display or by the server | `{ pluginId }`: only your plugin's id — never a key. Reload what you use. Delivered within a second where the event stream works, and within 15 seconds where it doesn't | Declaring an event not in the catalog rejects the install — typos fail loudly. Only declared events are ever delivered to your iframe. +**On `plugin.storage.changed`:** declare it and your widget stops needing to +poll its own storage: every write — a PUT or DELETE on a key, an increment, +or a server-side reaction to another event — pushes this event to your frame +within a second, with the 15-second changes check behind it where the stream +doesn't reach. Keep polling as a fallback for older servers, which never send +this event. The event is scoped to your plugin alone: a frame only ever hears +its own plugin's writes, never another plugin's, and the payload carries no +key, so don't expect one — reload what you use, and nothing about another +plugin's storage can reach your frame. + **On `chore.allCompleted`:** it fires alongside the daily-completion bonus, so it inherits that behaviour exactly — once per user per day, from whichever route emptied their list (completing the last chore, receiving a transfer, or snoozing diff --git a/server/index.js b/server/index.js index 130a5015..12913e7c 100644 --- a/server/index.js +++ b/server/index.js @@ -231,13 +231,16 @@ axios.interceptors.request.use((config) => { let calendarSyncService = null; const pluginEvents = require('./services/pluginEvents'); -const { DOMAINS: HOUSEHOLD_DOMAINS, createHouseholdChanges, domainsForRequest } = require('./utils/householdChanges'); +const { DOMAINS: HOUSEHOLD_DOMAINS, createHouseholdChanges, domainsForRequest, pluginStorageWrite } = require('./utils/householdChanges'); // A revision counter for each kind of household data, so every display can // tell what changed and reload it (issue #325, utils/householdChanges.js). // Each burst of changes also goes out on the event stream, the fast path // where it reaches the browser. const householdChanges = createHouseholdChanges({ onBump: (domains, revisions) => pluginEvents.emit('household.changed', { domains, revisions }), + // A plugin's storage write goes out under its own event (issue #347): the + // frame learns only that its own plugin's storage changed, never a key. + onPluginStorage: (pluginId, revision) => pluginEvents.emit('plugin.storage.changed', { pluginId, revision }), }); const haPanelRules = require('./services/haPanels'); const { HaLive } = require('./services/haLive'); @@ -678,6 +681,10 @@ fastify.addHook('onRequest', async (request, reply) => { fastify.addHook('onResponse', async (request, reply) => { const domains = domainsForRequest(request.method, request.routeOptions?.url, reply.statusCode); if (domains.length) householdChanges.bump(domains); + // A plugin storage write moves only that plugin's counter (issue #347), + // never a household kind — and only on success, like the hook above. + const pluginId = pluginStorageWrite(request.method, request.routeOptions?.url, reply.statusCode, request.params); + if (pluginId) householdChanges.bumpPlugin(pluginId); }); fastify.register(multipart, { @@ -1408,6 +1415,9 @@ fastify.delete('/api/widgets/:filename', async (request, reply) => { ); } } + // The data is gone, so its counter goes with it (issue #347): the + // set shrinks, and a reinstall starts from a fresh counter. + householdChanges.removePlugin(pluginId); } if (result.changes === 0) { @@ -1844,6 +1854,10 @@ function resolveReactionDelta(deltaSpec, manifest, pluginId, payload) { } function runDeclarativeReactions(message) { + // A reaction never fires on a storage change itself: the reaction writes + // plugin storage, which would emit plugin.storage.changed, which would + // fire the reaction again — a self-triggering loop (issue #347). + if (message.event === 'plugin.storage.changed') return; try { const rows = db.prepare( 'SELECT plugin_id, manifest_json FROM plugins WHERE plugin_id IS NOT NULL AND manifest_json IS NOT NULL' @@ -1864,6 +1878,9 @@ function runDeclarativeReactions(message) { () => incrementPluginStorage(row.plugin_id, reaction.key, pathSegments, delta) ); applyReaction(); + // A server-side storage write moves the plugin's counter exactly + // like a request write does (issue #347). + householdChanges.bumpPlugin(row.plugin_id); } catch (error) { console.error(`Plugin reaction failed (${row.plugin_id} on ${message.event}):`, error.message); } diff --git a/server/services/pluginEvents.js b/server/services/pluginEvents.js index 2f150441..2f3f6a35 100644 --- a/server/services/pluginEvents.js +++ b/server/services/pluginEvents.js @@ -17,6 +17,10 @@ const PLUGIN_EVENT_CATALOG = Object.freeze([ 'prize.redeemed', // { userId, prizeId, offerId, prizeName, cost, newTotal } 'ha.state', // { entityIds } — Home Assistant entities changed (issue #252); a nudge to read state, never the state itself 'household.changed', // { domains, revisions } — household data changed (issue #325): chores, users, prizes, settings or calendar; reload what you show + // { pluginId, revision } — that plugin's storage changed (issue #347). + // No key, ever: the frame reloads what it uses and learns nothing about + // any other plugin's storage. Scoped per frame by the dashboard bridge. + 'plugin.storage.changed', ]); const catalogSet = new Set(PLUGIN_EVENT_CATALOG); diff --git a/server/tests/householdChanges.test.js b/server/tests/householdChanges.test.js index 8e89c93d..9e45efac 100644 --- a/server/tests/householdChanges.test.js +++ b/server/tests/householdChanges.test.js @@ -56,3 +56,73 @@ test('every server starts from its own base, so a restart reads as "everything c const b = createHouseholdChanges().snapshot().chores; assert.notEqual(a, b); }); + +// Plugin storage counters (issue #347): one per plugin, in the same map. +const { pluginStorageWrite } = require('../utils/householdChanges'); + +test('pluginStorageWrite routes only successful storage writes, by plugin id', () => { + const params = { pluginId: 'lists' }; + assert.equal(pluginStorageWrite('PUT', '/api/plugin/v1/storage/:pluginId/:key', 200, params), 'lists'); + assert.equal(pluginStorageWrite('DELETE', '/api/plugin/v1/storage/:pluginId/:key', 204, params), 'lists'); + assert.equal(pluginStorageWrite('POST', '/api/plugin/v1/storage/:pluginId/:key/increment', 200, params), 'lists'); + assert.equal(pluginStorageWrite('GET', '/api/plugin/v1/storage/:pluginId/:key', 200, params), null, 'reads move nothing'); + assert.equal(pluginStorageWrite('PUT', '/api/plugin/v1/storage/:pluginId/:key', 400, params), null, 'failed writes move nothing'); + assert.equal(pluginStorageWrite('PUT', '/api/plugin/v1/settings/:pluginId', 200, params), null, 'settings are not storage'); + assert.equal(pluginStorageWrite('POST', '/api/plugin/v1/storage/:pluginId/:key', 200, params), null, 'POST on a key is not a write path'); + assert.equal(pluginStorageWrite('PUT', '/api/plugin/v1/storage/:pluginId/:key', 200, {}), null, 'no plugin id, no counter'); + assert.equal(pluginStorageWrite('PUT', undefined, 200, params), null, 'no route, no counter'); +}); + +test('a plugin write moves only that plugin counter, in its own message', async () => { + const household = []; + const plugins = []; + const changes = createHouseholdChanges({ + base: 100, + coalesceMs: 20, + onBump: (domains, revisions) => household.push({ domains, revisions }), + onPluginStorage: (pluginId, revision) => plugins.push({ pluginId, revision }), + }); + assert.deepEqual(Object.keys(changes.snapshot()).sort(), [...DOMAINS].sort(), 'no plugin counters before any plugin write'); + + changes.bumpPlugin('lists'); + changes.bumpPlugin('lists'); + changes.bumpPlugin('other'); + changes.bump('chores'); + assert.equal(changes.snapshot()['plugin:lists'], 102); + assert.equal(changes.snapshot()['plugin:other'], 101); + assert.equal(changes.snapshot().chores, 101); + assert.equal(changes.snapshot().users, 100, 'a plugin write moves no household kind'); + + await new Promise((r) => setTimeout(r, 40)); + assert.equal(household.length, 1); + assert.deepEqual(household[0].domains, ['chores']); + assert.deepEqual(Object.keys(household[0].revisions).sort(), [...DOMAINS].sort(), 'a household message carries household kinds only'); + assert.deepEqual(plugins, [ + { pluginId: 'lists', revision: 102 }, + { pluginId: 'other', revision: 101 }, + ]); +}); + +test('removing a plugin drops its counter, and the etag stays deterministic', async () => { + const plugins = []; + const changes = createHouseholdChanges({ base: 100, coalesceMs: 20, onPluginStorage: (pluginId) => plugins.push(pluginId) }); + changes.bumpPlugin('lists'); + changes.bumpPlugin('other'); + assert.equal(changes.snapshot()['plugin:lists'], 101); + + changes.removePlugin('lists'); + assert.equal(changes.snapshot()['plugin:lists'], undefined); + assert.equal(changes.snapshot()['plugin:other'], 101); + await new Promise((r) => setTimeout(r, 40)); + assert.deepEqual(plugins, ['other'], 'a removed plugin sends no event, even for a pending bump'); + + // Same counters in a different insertion order build the same etag. + const a = createHouseholdChanges({ base: 100 }); + a.bumpPlugin('zeta'); + a.bumpPlugin('alpha'); + const b = createHouseholdChanges({ base: 100 }); + b.bumpPlugin('alpha'); + b.bumpPlugin('zeta'); + assert.equal(a.etag(), b.etag()); + assert.equal(a.snapshot()['plugin:alpha'], b.snapshot()['plugin:alpha']); +}); diff --git a/server/tests/pluginStorageChanges.test.js b/server/tests/pluginStorageChanges.test.js new file mode 100644 index 00000000..44faddb8 --- /dev/null +++ b/server/tests/pluginStorageChanges.test.js @@ -0,0 +1,226 @@ +// Per-plugin storage counters on a real server (issue #347): every write +// path moves only that plugin's counter, the stream carries the event with +// no key, and uninstall with purge drops the counter. +const test = require('node:test'); +const assert = require('node:assert/strict'); +const { spawn } = require('node:child_process'); +const fs = require('node:fs'); +const http = require('node:http'); +const os = require('node:os'); +const path = require('node:path'); +const { freePort } = require('./freePort'); + +const serverDir = path.resolve(__dirname, '..'); +const delay = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); + +let child; +let base; +let dataDir; + +test.before(async () => { + dataDir = fs.mkdtempSync(path.join(os.tmpdir(), 'homeglow-plugin-storage-')); + const port = await freePort(); + child = spawn(process.execPath, ['index.js'], { + cwd: serverDir, + env: { + ...process.env, PORT: String(port), DB_PATH: path.join(dataDir, 'tasks.db'), TZ: 'UTC', DEMO_MODE: 'false', + ENCRYPTION_KEY: Buffer.alloc(32, 6).toString('base64'), + HOMEGLOW_DISABLE_BACKGROUND_JOBS: '1', HOMEGLOW_DISABLE_CALENDAR_SYNC: '1', HOMEGLOW_DISABLE_UPDATE_CHECK: '1', + }, + stdio: ['ignore', 'ignore', 'ignore'], + }); + base = `http://127.0.0.1:${port}`; + const start = Date.now(); + while (Date.now() - start < 30000) { + try { if ((await fetch(`${base}/api/test`)).ok) break; } catch { /* starting */ } + await delay(250); + } + await upload('plug-a.html', manifestPage(pluginManifest('plug-a'))); + await upload('plug-b.html', manifestPage(pluginManifest('plug-b'))); + await upload('plug-react.html', manifestPage(pluginManifest('plug-react', { + reactions: [{ on: 'chore.completed', action: 'increment', key: 'done', path: 'count', delta: 1 }], + }))); +}); + +test.after(async () => { + await new Promise((resolve) => { + if (child.exitCode !== null) return resolve(); + child.once('close', resolve); + child.kill(); + setTimeout(resolve, 5000); + }); + fs.rmSync(dataDir, { recursive: true, force: true }); +}); + +const manifestPage = (manifest) => `tp`; +const pluginManifest = (id, extra = {}) => ({ manifestVersion: 1, id, name: id, apiVersion: 'v1', storage: true, events: ['plugin.storage.changed'], ...extra }); + +async function api(pathname, options = {}) { + const res = await fetch(`${base}${pathname}`, options); + const text = await res.text(); + let body; + try { body = JSON.parse(text); } catch { body = text; } + return { status: res.status, headers: res.headers, body }; +} + +async function upload(filename, html) { + const form = new FormData(); + form.append('file', new Blob([html], { type: 'text/html' }), filename); + const res = await api('/api/widgets/upload', { method: 'POST', body: form }); + assert.equal(res.status, 200, JSON.stringify(res.body)); +} + +// The token a page was served with, read from the frame script. +async function tokenOf(filename) { + const page = await api(`/widgets/${filename}`); + assert.equal(page.status, 200); + const match = page.body.match(/var TOKEN="([^"]+)"/); + assert.ok(match, 'the page carries a token'); + return match[1]; +} + +// As a plugin's frame asks: Origin null, its page token. +const asPlugin = (pathname, token, options = {}) => api(pathname, { + ...options, + headers: { + Origin: 'null', 'Sec-Fetch-Mode': 'cors', ...(token ? { 'X-HomeGlow-Plugin-Token': token } : {}), + ...(options.headers || {}), + }, +}); + +const changes = async () => (await api('/api/changes')).body; +const moved = (before, after) => Object.keys(after).filter((key) => after[key] !== before[key]).sort(); + +test('each storage write path moves only that plugin counter; reads and failures move nothing', async () => { + const tokenA = await tokenOf('plug-a.html'); + const tokenB = await tokenOf('plug-b.html'); + + let before = await changes(); + assert.equal(before['plugin:plug-a'], undefined, 'no counter before the first write'); + + const put = await asPlugin('/api/plugin/v1/storage/plug-a/list', tokenA, { + method: 'PUT', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ items: [] }), + }); + assert.equal(put.status, 200, JSON.stringify(put.body)); + let after = await changes(); + assert.deepEqual(moved(before, after), ['plugin:plug-a']); + + before = after; + const inc = await asPlugin('/api/plugin/v1/storage/plug-a/count/increment', tokenA, { + method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ path: 'total', delta: 1 }), + }); + assert.equal(inc.status, 200, JSON.stringify(inc.body)); + after = await changes(); + assert.deepEqual(moved(before, after), ['plugin:plug-a']); + + before = after; + const del = await asPlugin('/api/plugin/v1/storage/plug-a/list', tokenA, { method: 'DELETE' }); + assert.equal(del.status, 200, JSON.stringify(del.body)); + after = await changes(); + assert.deepEqual(moved(before, after), ['plugin:plug-a']); + + before = after; + const other = await asPlugin('/api/plugin/v1/storage/plug-b/list', tokenB, { + method: 'PUT', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify([]), + }); + assert.equal(other.status, 200, JSON.stringify(other.body)); + after = await changes(); + assert.deepEqual(moved(before, after), ['plugin:plug-b'], "another plugin's write never moves this plugin's counter"); + + before = after; + await asPlugin('/api/plugin/v1/storage/plug-a', tokenA); + await asPlugin('/api/plugin/v1/storage/plug-a/count', tokenA); + await asPlugin('/api/plugin/v1/storage/plug-a/missing', tokenA, { method: 'DELETE' }); + await asPlugin('/api/plugin/v1/storage/plug-a/count/increment', tokenA, { + method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ path: 'total' }), + }); + await asPlugin('/api/plugin/v1/storage/no-such-plugin/list', tokenA, { + method: 'PUT', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(1), + }); + after = await changes(); + assert.deepEqual(moved(before, after), [], 'reads, a missing-key delete, a bad increment and an unknown plugin move nothing'); +}); + +test('a server-side reaction moves the plugin counter, and the stream carries the event with no key', async () => { + const user = await api('/api/users', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ username: 'Ava', email: 'ava@example.com' }) }); + const chore = await api('/api/chores', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ title: 'Dishes', clam_value: 2 }) }); + const schedule = await api('/api/chore-schedules', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ chore_id: chore.body.id, user_id: user.body.id, crontab: '0 0 * * *' }) }); + assert.equal(schedule.status, 200, JSON.stringify(schedule.body)); + + const before = await changes(); + const messages = []; + const req = http.get(`${base}/api/plugin/v1/events/stream`, (res) => { + let buffer = ''; + res.on('data', (chunk) => { + buffer += chunk; + let idx; + while ((idx = buffer.indexOf('\n')) !== -1) { + const line = buffer.slice(0, idx).replace(/\r$/, ''); + buffer = buffer.slice(idx + 1); + if (!line.startsWith('data: ')) continue; + messages.push(JSON.parse(line.slice(6))); + } + }); + }); + await delay(300); + + const done = await api('/api/chores/complete', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ chore_schedule_id: schedule.body.id, user_id: user.body.id, date: new Date().toISOString().slice(0, 10) }) }); + assert.equal(done.status, 200, JSON.stringify(done.body)); + await delay(500); + req.destroy(); + + const after = await changes(); + assert.deepEqual(moved(before, after).filter((key) => key.startsWith('plugin:')), ['plugin:plug-react']); + const stored = await asPlugin('/api/plugin/v1/storage/plug-react/done', await tokenOf('plug-react.html')); + assert.equal(stored.body.value?.count ?? stored.body.count, 1, JSON.stringify(stored.body)); + + const storageEvents = messages.filter((message) => message.event === 'plugin.storage.changed'); + assert.equal(storageEvents.length, 1, JSON.stringify(messages)); + assert.equal(storageEvents[0].payload.pluginId, 'plug-react'); + assert.equal(typeof storageEvents[0].payload.revision, 'number'); + assert.equal('key' in storageEvents[0].payload, false, 'the event carries no key'); + assert.equal(messages.some((message) => message.event === 'household.changed' && message.payload.domains.some((domain) => domain.startsWith('plugin:'))), false, 'no household event carries a plugin counter'); +}); + +test('uninstall with purge drops the counter; without purge it stays', async () => { + const tokenA = await tokenOf('plug-a.html'); + await asPlugin('/api/plugin/v1/storage/plug-a/keep', tokenA, { + method: 'PUT', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(1), + }); + let before = await changes(); + assert.ok(before['plugin:plug-a'] !== undefined); + + const kept = await api('/api/widgets/plug-a.html', { method: 'DELETE' }); + assert.equal(kept.status, 200, JSON.stringify(kept.body)); + let after = await changes(); + assert.equal(after['plugin:plug-a'], before['plugin:plug-a'], 'without purge, the counter stays with the kept data'); + + const tokenB = await tokenOf('plug-b.html'); + await asPlugin('/api/plugin/v1/storage/plug-b/keep', tokenB, { + method: 'PUT', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(1), + }); + before = await changes(); + assert.ok(before['plugin:plug-b'] !== undefined); + + const purged = await api('/api/widgets/plug-b.html?purgeData=true', { method: 'DELETE' }); + assert.equal(purged.status, 200, JSON.stringify(purged.body)); + after = await changes(); + assert.equal(after['plugin:plug-b'], undefined, 'with purge, the counter goes with the data'); +}); + +test('the etag moves with a plugin write, and 304 holds while nothing changes', async () => { + await upload('plug-etag.html', manifestPage(pluginManifest('plug-etag'))); + const token = await tokenOf('plug-etag.html'); + + const first = await api('/api/changes'); + const tag = first.headers.get('etag'); + assert.ok(tag); + + await asPlugin('/api/plugin/v1/storage/plug-etag/mark', token, { + method: 'PUT', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(1), + }); + const second = await api('/api/changes'); + assert.notEqual(second.headers.get('etag'), tag, 'a plugin write moves the etag, since the counter list grew'); + const again = await fetch(`${base}/api/changes`, { headers: { 'If-None-Match': second.headers.get('etag') } }); + assert.equal(again.status, 304); +}); diff --git a/server/utils/householdChanges.js b/server/utils/householdChanges.js index a7c563bd..bfcd4b14 100644 --- a/server/utils/householdChanges.js +++ b/server/utils/householdChanges.js @@ -13,6 +13,15 @@ // clock alone (a snooze ending, midnight, an expiry) aren't writes: displays // work those out from their own clock. // +// Plugin storage has counters in the same map (issue #347): one per plugin, +// under `plugin:`. A plugin's counter is created on its first +// storage write and dropped when its data is purged, so the set grows and +// shrinks with installed plugins; the ETag is built from the five fixed +// kinds plus the plugin kinds in sorted order, so it stays deterministic. +// A plugin's storage write moves only that plugin's counter, never a +// household kind, and goes out as its own event (see pluginEvents) carrying +// the plugin id only — never a key. +// // Counters live in memory and start from a random base at boot, so after a // restart every counter differs from what any display last saw, and every // display reloads everything once. @@ -20,10 +29,29 @@ const crypto = require('crypto'); const DOMAINS = Object.freeze(['chores', 'users', 'prizes', 'settings', 'calendar']); +const PLUGIN_DOMAIN_PREFIX = 'plugin:'; + +// The counter kind for a plugin's storage, and the plugin id behind one. +function pluginDomain(pluginId) { + return `${PLUGIN_DOMAIN_PREFIX}${pluginId}`; +} + +function isPluginDomain(domain) { + return typeof domain === 'string' && domain.startsWith(PLUGIN_DOMAIN_PREFIX); +} + +function pluginIdOfDomain(domain) { + return domain.slice(PLUGIN_DOMAIN_PREFIX.length); +} + // Which kinds of data a write under each route changes. A chore completion // and a prize redemption both move clam totals, which are user data. const ROUTE_DOMAINS = [ [/^\/api\/(chores|chore-schedules|chore-history|routines|routine-steps)(\/|$)/, ['chores', 'users']], + // A manual Google Tasks sync rewrites chore data, so it moves chores as + // well as users. It must come before the generic /api/users entry below + // (first match wins). + [/^\/api\/users\/:id\/google-tasks\/sync$/, ['chores', 'users']], [/^\/api\/users(\/|$)/, ['users']], [/^\/api\/(prizes|prize-offers)(\/|$)/, ['prizes', 'users']], [/^\/api\/(settings|timezone|themes|appearance|sounds)(\/|$)/, ['settings']], @@ -45,24 +73,64 @@ function domainsForRequest(method, route, statusCode) { return match ? match[1] : []; } +/** + * The plugin whose storage a finished request wrote, if any (issue #347): + * PUT or DELETE on a storage key, or POST on a key's /increment — and only + * on success, like domainsForRequest. Reads, failed writes, other plugin + * routes (settings, notify, ha) and other routes return null. `params` is + * the request's route params, so the id comes from the path, not the body. + */ +function pluginStorageWrite(method, route, statusCode, params) { + if (!route || statusCode >= 400) return null; + const pluginId = params?.pluginId; + if (typeof pluginId !== 'string' || pluginId.length === 0) return null; + switch (String(method).toUpperCase()) { + case 'PUT': + case 'DELETE': + return route === '/api/plugin/v1/storage/:pluginId/:key' ? pluginId : null; + case 'POST': + return route === '/api/plugin/v1/storage/:pluginId/:key/increment' ? pluginId : null; + default: + return null; + } +} + /** * A set of counters. `onBump(domains, revisions)` is called once per burst * of bumps, after `coalesceMs`, so a write touching several kinds, or a - * nightly job touching many rows, goes out as one message. + * nightly job touching many rows, goes out as one message. `onPluginStorage` + * is called once per plugin whose storage counter moved in the burst, with + * that plugin's id and its new revision. Household revisions handed to + * onBump never include plugin counters: a display may see every plugin's + * counter through the changes check, but a household event carries only + * household kinds. */ -function createHouseholdChanges({ onBump = () => {}, coalesceMs = 100, base = crypto.randomInt(1e6, 1e9) } = {}) { +function createHouseholdChanges({ onBump = () => {}, onPluginStorage = () => {}, coalesceMs = 100, base = crypto.randomInt(1e6, 1e9) } = {}) { const revisions = Object.fromEntries(DOMAINS.map((domain) => [domain, base])); let pending = new Set(); let timer = null; const snapshot = () => ({ ...revisions }); - const etag = () => `"${DOMAINS.map((domain) => revisions[domain]).join('-')}"`; + const householdSnapshot = () => Object.fromEntries(DOMAINS.map((domain) => [domain, revisions[domain]])); + const etag = () => { + const pluginKeys = Object.keys(revisions).filter(isPluginDomain).sort(); + return `"${[...DOMAINS, ...pluginKeys].map((domain) => revisions[domain]).join('-')}"`; + }; function flush() { timer = null; - const domains = DOMAINS.filter((domain) => pending.has(domain)); + const householdMoved = DOMAINS.filter((domain) => pending.has(domain)); + const pluginMoved = [...pending].filter(isPluginDomain).sort(); pending = new Set(); - if (domains.length) onBump(domains, snapshot()); + if (householdMoved.length) onBump(householdMoved, householdSnapshot()); + for (const domain of pluginMoved) onPluginStorage(pluginIdOfDomain(domain), revisions[domain]); + } + + function scheduleFlush() { + if (!timer) { + timer = setTimeout(flush, coalesceMs); + timer.unref?.(); + } } function bump(...domains) { @@ -73,13 +141,41 @@ function createHouseholdChanges({ onBump = () => {}, coalesceMs = 100, base = cr pending.add(domain); moved = true; } - if (moved && !timer) { - timer = setTimeout(flush, coalesceMs); - timer.unref?.(); - } + if (moved) scheduleFlush(); + } + + // A plugin's storage write moves only that plugin's counter, creating it + // on first use: a display that has never seen the plugin reads the new + // key as a change, and one that has sees the moved counter. Household + // kinds never move here. + function bumpPlugin(pluginId) { + if (typeof pluginId !== 'string' || pluginId.length === 0) return; + const domain = pluginDomain(pluginId); + if (!Object.prototype.hasOwnProperty.call(revisions, domain)) revisions[domain] = base; + revisions[domain] += 1; + pending.add(domain); + scheduleFlush(); + } + + // The plugin's data is gone (uninstall with purge): its counter goes + // with it, so the set shrinks. No event — there is no frame left to tell, + // and the next changes check shows the key gone. + function removePlugin(pluginId) { + if (typeof pluginId !== 'string' || pluginId.length === 0) return; + const domain = pluginDomain(pluginId); + delete revisions[domain]; + pending.delete(domain); } - return { bump, snapshot, etag, flush }; + return { bump, bumpPlugin, removePlugin, snapshot, etag, flush }; } -module.exports = { DOMAINS, ROUTE_DOMAINS, domainsForRequest, createHouseholdChanges }; +module.exports = { + DOMAINS, + ROUTE_DOMAINS, + PLUGIN_DOMAIN_PREFIX, + pluginDomain, + domainsForRequest, + pluginStorageWrite, + createHouseholdChanges, +}; From a5f387929e5ff8a880c3af4402c5f006abe84e11 Mon Sep 17 00:00:00 2001 From: adamecker Date: Sat, 31 Oct 2026 17:48:49 +0000 Subject: [PATCH 2/5] Address review on plugin storage push (#347) - Reaction stream test: wait out the coalescing window before opening the stream, and count only this test's own plugin's events, so an earlier test's burst can't be mistaken for this test's event. - Plugin counters now start from a server-wide sequence, never reused within a boot, so uninstall-with-purge plus reinstall can't repeat the old counter or ETag; the client also prunes vanished plugin keys on every 200, so a reinstall reads as changed even at an equal value. - Drop the dead /api/users/:id/google-tasks/sync route entry: HomeGlow has no such route. - Installing a manifest with a reaction on plugin.storage.changed now fails loudly instead of silently never running. - Guide: declaring the event requires a server with this support (no older-server fallback), and a frame hears its own writes too, so a write-on-load plus reload-on-event widget would loop. --- client/src/utils/householdChanges.js | 6 ++++-- client/src/utils/householdChanges.test.js | 18 ++++++++++++++++ docs/guides/plugin-development.md | 14 +++++++----- server/index.js | 5 +++++ server/tests/householdChanges.test.js | 26 ++++++++++++++++++----- server/tests/pluginPlatform.test.js | 11 ++++++++++ server/tests/pluginStorageChanges.test.js | 7 +++++- server/utils/householdChanges.js | 17 +++++++++------ 8 files changed, 84 insertions(+), 20 deletions(-) diff --git a/client/src/utils/householdChanges.js b/client/src/utils/householdChanges.js index 0196b7c4..5c30fdb8 100644 --- a/client/src/utils/householdChanges.js +++ b/client/src/utils/householdChanges.js @@ -86,7 +86,9 @@ function notifyPlugins(movedPluginIds) { // A full snapshot, from GET /api/changes only: household kinds plus plugin // counters. Plugin keys that vanished (uninstall with purge) drop out of -// what this page knows, so they can't linger as stale counters. +// what this page knows on every 200, not only when something else moved: +// otherwise the stale key lingers, and a reinstall at the same counter +// value would read as unchanged and its first write would be lost. function applySnapshot(next) { if (!next || typeof next !== 'object') return; if (!known) { @@ -95,12 +97,12 @@ function applySnapshot(next) { } const moved = changedDomains(known, next); const movedPlugins = changedPluginIds(known, next); - if (!moved.length && !movedPlugins.length) return; const pruned = { ...known, ...next }; for (const key of Object.keys(pruned)) { if (key.startsWith(PLUGIN_DOMAIN_PREFIX) && !(key in next)) delete pruned[key]; } known = pruned; + if (!moved.length && !movedPlugins.length) return; notifyHousehold(moved); notifyPlugins(movedPlugins); } diff --git a/client/src/utils/householdChanges.test.js b/client/src/utils/householdChanges.test.js index 1bced01a..624b6c08 100644 --- a/client/src/utils/householdChanges.test.js +++ b/client/src/utils/householdChanges.test.js @@ -237,6 +237,24 @@ describe('plugin storage changes', () => { expect(handler).toHaveBeenCalledWith('plug-b'); }); + it('a reinstall reads as changed even when its counter value repeats the old one', async () => { + globalThis.EventSource = function () {}; + const handler = vi.fn(); + subscribePluginStorageChanges('plug-b', handler); + await flush(); + axios.get.mockResolvedValueOnce(answer(revisions({ 'plugin:plug-b': 7 }))); + await checkHouseholdChanges(); + // The uninstall prune: the next check drops the key, and nothing fires — + // but the stale key must not linger, or a reinstall at the same value + // would read as unchanged and its first write would be lost. + axios.get.mockResolvedValueOnce(answer(revisions())); + await checkHouseholdChanges(); + expect(handler).not.toHaveBeenCalled(); + axios.get.mockResolvedValueOnce(answer(revisions({ 'plugin:plug-b': 7 }))); + await checkHouseholdChanges(); + expect(handler).toHaveBeenCalledWith('plug-b'); + }); + it('a household subscriber never hears plugin counters, and household revisions exclude them', async () => { axios.get.mockResolvedValueOnce(answer(revisions({ 'plugin:plug-a': 5 }))); const handler = vi.fn(); diff --git a/docs/guides/plugin-development.md b/docs/guides/plugin-development.md index b93b6d88..0d4b06e2 100644 --- a/docs/guides/plugin-development.md +++ b/docs/guides/plugin-development.md @@ -427,11 +427,15 @@ Only declared events are ever delivered to your iframe. poll its own storage: every write — a PUT or DELETE on a key, an increment, or a server-side reaction to another event — pushes this event to your frame within a second, with the 15-second changes check behind it where the stream -doesn't reach. Keep polling as a fallback for older servers, which never send -this event. The event is scoped to your plugin alone: a frame only ever hears -its own plugin's writes, never another plugin's, and the payload carries no -key, so don't expect one — reload what you use, and nothing about another -plugin's storage can reach your frame. +doesn't reach. Declaring this event requires a server with this support: an +older server rejects the install, since the event isn't in its catalog, so +there is no older-server fallback to keep. The event is scoped to your plugin +alone: a frame only ever hears its own plugin's writes, never another +plugin's, and the payload carries no key, so don't expect one — reload what +you use, and nothing about another plugin's storage can reach your frame. +A frame hears its own writes too: a widget that writes on load and reloads on +this event will loop about once a second, on every display, so only reload +for writes you didn't just make. **On `chore.allCompleted`:** it fires alongside the daily-completion bonus, so it inherits that behaviour exactly — once per user per day, from whichever route diff --git a/server/index.js b/server/index.js index 12913e7c..80ac2a6a 100644 --- a/server/index.js +++ b/server/index.js @@ -1101,6 +1101,11 @@ function extractPluginManifest(htmlContent) { if (typeof reaction.on !== 'string' || !pluginEvents.isKnownEvent(reaction.on)) { errors.push(`reactions[${index}].on must be a known event (catalog: ${pluginEvents.PLUGIN_EVENT_CATALOG.join(', ')}).`); } + // A reaction on a storage change would silently never run (the + // loop guard drops it), so fail the install loudly instead. + if (reaction.on === 'plugin.storage.changed') { + errors.push(`reactions[${index}].on must not be 'plugin.storage.changed' (reactions never run on a storage change).`); + } if (reaction.action !== 'increment') { errors.push(`reactions[${index}].action must be 'increment' (the only supported action).`); } diff --git a/server/tests/householdChanges.test.js b/server/tests/householdChanges.test.js index 9e45efac..9f02b5ef 100644 --- a/server/tests/householdChanges.test.js +++ b/server/tests/householdChanges.test.js @@ -89,7 +89,7 @@ test('a plugin write moves only that plugin counter, in its own message', async changes.bumpPlugin('other'); changes.bump('chores'); assert.equal(changes.snapshot()['plugin:lists'], 102); - assert.equal(changes.snapshot()['plugin:other'], 101); + assert.equal(changes.snapshot()['plugin:other'], 102, 'a new plugin starts from the server-wide sequence, not the boot base'); assert.equal(changes.snapshot().chores, 101); assert.equal(changes.snapshot().users, 100, 'a plugin write moves no household kind'); @@ -99,7 +99,7 @@ test('a plugin write moves only that plugin counter, in its own message', async assert.deepEqual(Object.keys(household[0].revisions).sort(), [...DOMAINS].sort(), 'a household message carries household kinds only'); assert.deepEqual(plugins, [ { pluginId: 'lists', revision: 102 }, - { pluginId: 'other', revision: 101 }, + { pluginId: 'other', revision: 102 }, ]); }); @@ -112,17 +112,33 @@ test('removing a plugin drops its counter, and the etag stays deterministic', as changes.removePlugin('lists'); assert.equal(changes.snapshot()['plugin:lists'], undefined); - assert.equal(changes.snapshot()['plugin:other'], 101); + assert.equal(changes.snapshot()['plugin:other'], 102); await new Promise((r) => setTimeout(r, 40)); assert.deepEqual(plugins, ['other'], 'a removed plugin sends no event, even for a pending bump'); - // Same counters in a different insertion order build the same etag. + // The same sequence of operations builds the same etag. (Different + // insertion orders no longer do: a new counter starts from the + // server-wide sequence, so its value depends on what came before it.) const a = createHouseholdChanges({ base: 100 }); a.bumpPlugin('zeta'); a.bumpPlugin('alpha'); const b = createHouseholdChanges({ base: 100 }); - b.bumpPlugin('alpha'); b.bumpPlugin('zeta'); + b.bumpPlugin('alpha'); assert.equal(a.etag(), b.etag()); assert.equal(a.snapshot()['plugin:alpha'], b.snapshot()['plugin:alpha']); }); + +test('a reinstalled plugin never reuses its old counter, so its first write reads as changed', () => { + const changes = createHouseholdChanges({ base: 100 }); + changes.bumpPlugin('lists'); + const oldValue = changes.snapshot()['plugin:lists']; + const oldEtag = changes.etag(); + assert.equal(oldValue, 101); + + changes.removePlugin('lists'); + changes.bumpPlugin('other'); + changes.bumpPlugin('lists'); + assert.notEqual(changes.snapshot()['plugin:lists'], oldValue, 'purge + reinstall must not repeat the counter'); + assert.notEqual(changes.etag(), oldEtag, 'purge + reinstall must not repeat the etag'); +}); diff --git a/server/tests/pluginPlatform.test.js b/server/tests/pluginPlatform.test.js index 969aa4fe..68af6f66 100644 --- a/server/tests/pluginPlatform.test.js +++ b/server/tests/pluginPlatform.test.js @@ -969,6 +969,17 @@ test('reaction declarations are validated at install time', async () => { const upload3 = await uploadWidget('reaction-bad-factor.html', widgetHtml(badFactor)); assert.equal(upload3.status, 400); assert.match(upload3.body.error, /factor must be a finite number/); + + // A reaction on plugin.storage.changed would silently never run. + const storageReaction = { + manifestVersion: 1, + id: 'reaction-storage-event', + storage: true, + reactions: [{ on: 'plugin.storage.changed', action: 'increment', key: 'k', path: 'p', delta: 1 }], + }; + const upload4 = await uploadWidget('reaction-storage-event.html', widgetHtml(storageReaction)); + assert.equal(upload4.status, 400); + assert.match(upload4.body.error, /plugin\.storage\.changed/); }); test('a factor:-1 mirror reaction compensates uncomplete, so re-completing does not double-count', async () => { diff --git a/server/tests/pluginStorageChanges.test.js b/server/tests/pluginStorageChanges.test.js index 44faddb8..c78a74f6 100644 --- a/server/tests/pluginStorageChanges.test.js +++ b/server/tests/pluginStorageChanges.test.js @@ -147,6 +147,9 @@ test('a server-side reaction moves the plugin counter, and the stream carries th const schedule = await api('/api/chore-schedules', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ chore_id: chore.body.id, user_id: user.body.id, crontab: '0 0 * * *' }) }); assert.equal(schedule.status, 200, JSON.stringify(schedule.body)); + // Wait out the 100 ms coalescing window before opening the stream, so no + // burst from an earlier test is still pending when this stream attaches. + await delay(300); const before = await changes(); const messages = []; const req = http.get(`${base}/api/plugin/v1/events/stream`, (res) => { @@ -174,7 +177,9 @@ test('a server-side reaction moves the plugin counter, and the stream carries th const stored = await asPlugin('/api/plugin/v1/storage/plug-react/done', await tokenOf('plug-react.html')); assert.equal(stored.body.value?.count ?? stored.body.count, 1, JSON.stringify(stored.body)); - const storageEvents = messages.filter((message) => message.event === 'plugin.storage.changed'); + // Only this test's own plugin: an earlier test's burst must never be + // counted here, even if it lands in this stream. + const storageEvents = messages.filter((message) => message.event === 'plugin.storage.changed' && message.payload?.pluginId === 'plug-react'); assert.equal(storageEvents.length, 1, JSON.stringify(messages)); assert.equal(storageEvents[0].payload.pluginId, 'plug-react'); assert.equal(typeof storageEvents[0].payload.revision, 'number'); diff --git a/server/utils/householdChanges.js b/server/utils/householdChanges.js index bfcd4b14..41e316a0 100644 --- a/server/utils/householdChanges.js +++ b/server/utils/householdChanges.js @@ -48,10 +48,6 @@ function pluginIdOfDomain(domain) { // and a prize redemption both move clam totals, which are user data. const ROUTE_DOMAINS = [ [/^\/api\/(chores|chore-schedules|chore-history|routines|routine-steps)(\/|$)/, ['chores', 'users']], - // A manual Google Tasks sync rewrites chore data, so it moves chores as - // well as users. It must come before the generic /api/users entry below - // (first match wins). - [/^\/api\/users\/:id\/google-tasks\/sync$/, ['chores', 'users']], [/^\/api\/users(\/|$)/, ['users']], [/^\/api\/(prizes|prize-offers)(\/|$)/, ['prizes', 'users']], [/^\/api\/(settings|timezone|themes|appearance|sounds)(\/|$)/, ['settings']], @@ -107,6 +103,10 @@ function pluginStorageWrite(method, route, statusCode, params) { */ function createHouseholdChanges({ onBump = () => {}, onPluginStorage = () => {}, coalesceMs = 100, base = crypto.randomInt(1e6, 1e9) } = {}) { const revisions = Object.fromEntries(DOMAINS.map((domain) => [domain, base])); + // One server-wide sequence for plugin counters: a new plugin's counter + // starts from here, never from the boot base alone, so a value is never + // reused within a boot — even after uninstall-with-purge and a reinstall. + let pluginSeq = base; let pending = new Set(); let timer = null; @@ -147,12 +147,15 @@ function createHouseholdChanges({ onBump = () => {}, onPluginStorage = () => {}, // A plugin's storage write moves only that plugin's counter, creating it // on first use: a display that has never seen the plugin reads the new // key as a change, and one that has sees the moved counter. Household - // kinds never move here. + // kinds never move here. A new counter starts from the server-wide + // sequence, not the boot base: after uninstall-with-purge and a + // reinstall, the first write must not repeat the old value, or a display + // holding the old ETag would read a 304 and miss it. function bumpPlugin(pluginId) { if (typeof pluginId !== 'string' || pluginId.length === 0) return; const domain = pluginDomain(pluginId); - if (!Object.prototype.hasOwnProperty.call(revisions, domain)) revisions[domain] = base; - revisions[domain] += 1; + if (!Object.prototype.hasOwnProperty.call(revisions, domain)) revisions[domain] = ++pluginSeq; + else revisions[domain] += 1; pending.add(domain); scheduleFlush(); } From 49c3da066b89d3f4ee6a1f71195c35bd50b9cd28 Mon Sep 17 00:00:00 2001 From: adamecker Date: Sat, 31 Oct 2026 11:43:30 -0700 Subject: [PATCH 3/5] Take the next plugin sequence value on every plugin bump (#347) A create-only sequence still lets a plugin's own count climb back onto a value it had before a purge (two writes, purge, reinstall, one write repeats the old counter and ETag), so a display holding the old ETag reads a 304 and drops the stream event as already known between checks. Every bump now takes ++pluginSeq, so no plugin counter value is ever reused within a boot. The reinstall test no longer leans on an intervening write to another plugin: two writes, purge, reinstall, one write must give a fresh counter and ETag. The moves-only-that-counter test expectation shifts accordingly (other takes 103, since lists took 101 and 102). Generated with Muse AI --- server/tests/householdChanges.test.js | 13 +++++++++---- server/utils/householdChanges.js | 20 +++++++++++--------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/server/tests/householdChanges.test.js b/server/tests/householdChanges.test.js index 9f02b5ef..a7df9a2b 100644 --- a/server/tests/householdChanges.test.js +++ b/server/tests/householdChanges.test.js @@ -89,7 +89,7 @@ test('a plugin write moves only that plugin counter, in its own message', async changes.bumpPlugin('other'); changes.bump('chores'); assert.equal(changes.snapshot()['plugin:lists'], 102); - assert.equal(changes.snapshot()['plugin:other'], 102, 'a new plugin starts from the server-wide sequence, not the boot base'); + assert.equal(changes.snapshot()['plugin:other'], 103, 'every plugin bump takes the next server-wide sequence value'); assert.equal(changes.snapshot().chores, 101); assert.equal(changes.snapshot().users, 100, 'a plugin write moves no household kind'); @@ -99,7 +99,7 @@ test('a plugin write moves only that plugin counter, in its own message', async assert.deepEqual(Object.keys(household[0].revisions).sort(), [...DOMAINS].sort(), 'a household message carries household kinds only'); assert.deepEqual(plugins, [ { pluginId: 'lists', revision: 102 }, - { pluginId: 'other', revision: 102 }, + { pluginId: 'other', revision: 103 }, ]); }); @@ -131,14 +131,19 @@ test('removing a plugin drops its counter, and the etag stays deterministic', as test('a reinstalled plugin never reuses its old counter, so its first write reads as changed', () => { const changes = createHouseholdChanges({ base: 100 }); + // Two writes before the purge: with a create-only sequence, the + // reinstall's first write would climb back onto 102 and repeat both the + // counter and the ETag. An intervening write to another plugin must not + // be what saves this test. + changes.bumpPlugin('lists'); changes.bumpPlugin('lists'); const oldValue = changes.snapshot()['plugin:lists']; const oldEtag = changes.etag(); - assert.equal(oldValue, 101); + assert.equal(oldValue, 102); changes.removePlugin('lists'); - changes.bumpPlugin('other'); changes.bumpPlugin('lists'); + assert.equal(changes.snapshot()['plugin:lists'], 103); assert.notEqual(changes.snapshot()['plugin:lists'], oldValue, 'purge + reinstall must not repeat the counter'); assert.notEqual(changes.etag(), oldEtag, 'purge + reinstall must not repeat the etag'); }); diff --git a/server/utils/householdChanges.js b/server/utils/householdChanges.js index 41e316a0..25852a0a 100644 --- a/server/utils/householdChanges.js +++ b/server/utils/householdChanges.js @@ -103,9 +103,12 @@ function pluginStorageWrite(method, route, statusCode, params) { */ function createHouseholdChanges({ onBump = () => {}, onPluginStorage = () => {}, coalesceMs = 100, base = crypto.randomInt(1e6, 1e9) } = {}) { const revisions = Object.fromEntries(DOMAINS.map((domain) => [domain, base])); - // One server-wide sequence for plugin counters: a new plugin's counter - // starts from here, never from the boot base alone, so a value is never - // reused within a boot — even after uninstall-with-purge and a reinstall. + // One server-wide sequence for plugin counters: every plugin bump takes + // the next value, so a value is never reused within a boot — even after + // uninstall-with-purge and a reinstall. Taking the sequence only when a + // counter is created is not enough: a plugin's own count could climb + // back onto a value it had before the purge (two writes, purge, + // reinstall, one write repeats the old counter and ETag). let pluginSeq = base; let pending = new Set(); let timer = null; @@ -147,15 +150,14 @@ function createHouseholdChanges({ onBump = () => {}, onPluginStorage = () => {}, // A plugin's storage write moves only that plugin's counter, creating it // on first use: a display that has never seen the plugin reads the new // key as a change, and one that has sees the moved counter. Household - // kinds never move here. A new counter starts from the server-wide - // sequence, not the boot base: after uninstall-with-purge and a - // reinstall, the first write must not repeat the old value, or a display - // holding the old ETag would read a 304 and miss it. + // kinds never move here. Every bump takes the next server-wide sequence + // value, not +1 on the plugin's own counter: after uninstall-with-purge + // and a reinstall, the first write must not repeat the old value, or a + // display holding the old ETag would read a 304 and miss it. function bumpPlugin(pluginId) { if (typeof pluginId !== 'string' || pluginId.length === 0) return; const domain = pluginDomain(pluginId); - if (!Object.prototype.hasOwnProperty.call(revisions, domain)) revisions[domain] = ++pluginSeq; - else revisions[domain] += 1; + revisions[domain] = ++pluginSeq; pending.add(domain); scheduleFlush(); } From bf48ce4199d48fbae260f2cb17e48fb6aa302367 Mon Sep 17 00:00:00 2001 From: adamecker Date: Sun, 1 Nov 2026 04:19:50 +0000 Subject: [PATCH 4/5] Address mrramam's design review on plugin storage push (#347) 1. Restart seeding: seedPlugins() at boot seeds a counter for every plugin with stored data (distinct plugin_storage rows), so frames read as moved after a restart instead of silently pruning a vanished key and showing stale data until the next write. 2. Self-write detection: storage writes return their revision (PUT/DELETE { success, revision }, increment adds revision), bumped in-route so the response carries the exact revision the event will carry; the tracker and wrapper pass the revision through to the frame, which compares it against its own writes' revisions. The event still carries no key and no value. 3. Visible-mode (phones) deafness: in visible mode the stream now establishes the baseline instead of dropping events until a first GET succeeds; interval-mode behavior is unchanged. 4. Install rejection: declaring plugin.storage.changed without storage: true now fails at install, like reactions do. 5. Docs: backend-api.md describes the dynamic plugin: counters alongside the fixed household kinds; the plugin guide covers the restart promise, revision comparison, phone behavior, and the install failure. Tests: server unit + real-server revision/restart tests, client tracker visible-mode tests, platform install-rejection test; full client (891) and server (713) suites green. --- client/src/components/PluginWidgetWrapper.jsx | 9 +- client/src/utils/householdChanges.js | 72 ++++++++++----- client/src/utils/householdChanges.test.js | 30 +++++- docs/guides/plugin-development.md | 31 ++++--- docs/reference/backend-api.md | 2 +- server/index.js | 45 ++++++++- server/plugin-sdk/v1.js | 9 +- server/tests/householdChanges.test.js | 50 ++++++++++ server/tests/pluginPlatform.test.js | 20 ++++ server/tests/pluginStorageChanges.test.js | 92 +++++++++++++++++++ server/utils/householdChanges.js | 34 ++++++- 11 files changed, 342 insertions(+), 52 deletions(-) diff --git a/client/src/components/PluginWidgetWrapper.jsx b/client/src/components/PluginWidgetWrapper.jsx index ad35c394..f0cb0df0 100644 --- a/client/src/components/PluginWidgetWrapper.jsx +++ b/client/src/components/PluginWidgetWrapper.jsx @@ -111,18 +111,19 @@ const PluginWidgetWrapper = ({ // plugin.storage.changed (issue #347), for a plugin that declared it: only // this frame's own plugin's counter moves reach it — the subscription is // per plugin, so a frame never hears another plugin's writes, and the - // payload carries the plugin id only, never a key, so the frame reloads - // what it uses and learns nothing else. + // payload carries the plugin id and the write's revision, never a key, so + // the frame reloads what it uses, learns nothing else, and can skip its + // own writes by comparing revisions. useEffect(() => { if (!pluginId || !eventsKey.split(',').includes('plugin.storage.changed')) return undefined; - return subscribePluginStorageChanges(pluginId, () => { + return subscribePluginStorageChanges(pluginId, (id, revision) => { const target = iframeRef.current?.contentWindow; if (!target) return; target.postMessage( { type: 'homeglow:event', event: 'plugin.storage.changed', - payload: { pluginId }, + payload: { pluginId: id, revision }, emittedAt: new Date().toISOString(), }, TO_FRAME diff --git a/client/src/utils/householdChanges.js b/client/src/utils/householdChanges.js index 5c30fdb8..538da98a 100644 --- a/client/src/utils/householdChanges.js +++ b/client/src/utils/householdChanges.js @@ -11,7 +11,10 @@ import { subscribePluginEvents } from './pluginEventBridge.js'; // checks every 15 seconds while it's visible, and right away when it // becomes visible again. A phone checks only when it becomes visible: // it's opened, glanced at and put away, so a timer would mostly spend -// its battery while nobody looks. +// its battery while nobody looks. In visible mode the stream also +// establishes the baseline: if the first check fails, stream events +// still reach subscribers instead of being dropped until a visibility +// change (a phone has no timer to retry with). // - The event stream, where it reaches the browser: the same counters // arrive within a second of the write. Where it doesn't (some proxies // and tunnels), the check above still catches up. @@ -23,9 +26,12 @@ import { subscribePluginEvents } from './pluginEventBridge.js'; // Plugin storage rides the same two paths (issue #347): the server keeps // one counter per plugin under `plugin:` in the same snapshot, // created on a plugin's first storage write and dropped when its data is -// purged. A plugin's frame hears only its own plugin's counter move, never -// another plugin's — the per-plugin subscription below is where that -// scoping lives, and the frame itself gets the plugin id only, never a key. +// purged — and seeded at boot for every plugin that already has stored +// data, so a restart reads as moved for its frames instead of silently +// pruning a vanished key. A plugin's frame hears only its own plugin's +// counter move, never another plugin's — the per-plugin subscription below +// is where that scoping lives — and the frame gets the plugin id and the +// write's revision, never a key, so it can skip its own writes. export const HOUSEHOLD_DOMAINS = Object.freeze(['chores', 'users', 'prizes', 'settings', 'calendar']); export const CHECK_INTERVAL_MS = 15 * 1000; @@ -72,12 +78,13 @@ function notifyHousehold(moved) { } } -function notifyPlugins(movedPluginIds) { - if (!movedPluginIds.length) return; +function notifyPlugins(moved) { + if (!moved.length) return; for (const { pluginId, handler } of pluginSubscribers) { - if (!movedPluginIds.includes(pluginId)) continue; + const entry = moved.find((item) => item.pluginId === pluginId); + if (!entry) continue; try { - handler(pluginId); + handler(pluginId, entry.revision); } catch (error) { console.error('Plugin storage change subscriber failed:', error); } @@ -96,7 +103,7 @@ function applySnapshot(next) { return; } const moved = changedDomains(known, next); - const movedPlugins = changedPluginIds(known, next); + const movedPlugins = changedPluginIds(known, next).map((pluginId) => ({ pluginId, revision: next[pluginDomain(pluginId)] })); const pruned = { ...known, ...next }; for (const key of Object.keys(pruned)) { if (key.startsWith(PLUGIN_DOMAIN_PREFIX) && !(key in next)) delete pruned[key]; @@ -110,9 +117,19 @@ function applySnapshot(next) { // Event-stream household revisions: household kinds only, merged into what // this page knows without pruning and without establishing the baseline — // the GET baseline follows right behind, and the stream is only a fast -// path, so a message that arrives before it is safely dropped. +// path, so a message that arrives before it is safely dropped. Except in +// visible mode (phones), where there is no timer to retry the check: if +// the first check failed, the stream establishes the baseline here +// instead, and subscribers hear it — otherwise the phone would stay deaf +// until a visibility change. function applyRevisions(revisions) { - if (!revisions || typeof revisions !== 'object' || !known) return; + if (!revisions || typeof revisions !== 'object') return; + if (!known) { + if (mode !== 'visible') return; + known = { ...revisions }; + notifyHousehold(HOUSEHOLD_DOMAINS.filter((domain) => revisions[domain] !== undefined)); + return; + } const moved = changedDomains(known, revisions); if (!moved.length) return; known = { ...known, ...revisions }; @@ -120,13 +137,23 @@ function applyRevisions(revisions) { } // One plugin's counter from the event stream. Same baseline rule as above: -// before the first GET answer, there is nothing to compare against. +// before the first GET answer, there is nothing to compare against — +// except in visible mode, where the event establishes the baseline and +// notifies, for the same reason. A later successful check adopts the full +// snapshot; household kinds it never saw read as moved then, which is the +// initial load this display never had. function applyPluginRevision(pluginId, revision) { - if (!known || typeof pluginId !== 'string' || typeof revision !== 'number') return; + if (typeof pluginId !== 'string' || typeof revision !== 'number') return; const domain = pluginDomain(pluginId); + if (!known) { + if (mode !== 'visible') return; + known = { [domain]: revision }; + notifyPlugins([{ pluginId, revision }]); + return; + } if (known[domain] === revision) return; known = { ...known, [domain]: revision }; - notifyPlugins([pluginId]); + notifyPlugins([{ pluginId, revision }]); } /** Ask the server now. Resolves when the answer has been applied. */ @@ -217,13 +244,16 @@ export function subscribeHouseholdChanges(domains, handler) { } /** - * Call `handler(pluginId)` whenever that plugin's storage counter moves on - * the server — from the event stream within a second, or from the 15-second - * check where the stream doesn't reach. A subscriber hears only its own - * plugin: another plugin's writes never reach it, which is what keeps one - * plugin's frames from learning about another plugin's storage (issue - * #347). Returns an unsubscribe function. The checks run while anything is - * subscribed. + * Call `handler(pluginId, revision)` whenever that plugin's storage counter + * moves on the server — from the event stream within a second, or from the + * 15-second check where the stream doesn't reach (phones: on becoming + * visible, and from the stream even before a first successful check). The + * revision is the write's new counter: a frame compares it against the + * revisions its own writes returned to skip its own writes. A subscriber + * hears only its own plugin: another plugin's writes never reach it, which + * is what keeps one plugin's frames from learning about another plugin's + * storage (issue #347). Returns an unsubscribe function. The checks run + * while anything is subscribed. */ export function subscribePluginStorageChanges(pluginId, handler) { const entry = { pluginId, handler }; diff --git a/client/src/utils/householdChanges.test.js b/client/src/utils/householdChanges.test.js index 624b6c08..7f472168 100644 --- a/client/src/utils/householdChanges.test.js +++ b/client/src/utils/householdChanges.test.js @@ -194,7 +194,7 @@ describe('plugin storage changes', () => { axios.get.mockResolvedValueOnce(answer(revisions({ 'plugin:plug-a': 6, 'plugin:plug-b': 7 }))); await checkHouseholdChanges(); - expect(handlerA).toHaveBeenCalledWith('plug-a'); + expect(handlerA).toHaveBeenCalledWith('plug-a', 6); expect(handlerB).not.toHaveBeenCalled(); axios.get.mockResolvedValueOnce(answer(revisions({ 'plugin:plug-a': 6, 'plugin:plug-b': 7, 'plugin:plug-c': 1 }))); @@ -203,7 +203,7 @@ describe('plugin storage changes', () => { expect(handlerB).not.toHaveBeenCalled(); sseHandler({ event: 'plugin.storage.changed', payload: { pluginId: 'plug-b', revision: 8 } }); - expect(handlerB).toHaveBeenCalledWith('plug-b'); + expect(handlerB).toHaveBeenCalledWith('plug-b', 8); expect(handlerA).toHaveBeenCalledTimes(1); sseHandler({ event: 'plugin.storage.changed', payload: { pluginId: 'plug-c', revision: 1 } }); @@ -234,7 +234,7 @@ describe('plugin storage changes', () => { // A reinstall starts from a fresh counter, which reads as a change. axios.get.mockResolvedValueOnce(answer(revisions({ 'plugin:plug-b': 3 }))); await checkHouseholdChanges(); - expect(handler).toHaveBeenCalledWith('plug-b'); + expect(handler).toHaveBeenCalledWith('plug-b', 3); }); it('a reinstall reads as changed even when its counter value repeats the old one', async () => { @@ -252,7 +252,29 @@ describe('plugin storage changes', () => { expect(handler).not.toHaveBeenCalled(); axios.get.mockResolvedValueOnce(answer(revisions({ 'plugin:plug-b': 7 }))); await checkHouseholdChanges(); - expect(handler).toHaveBeenCalledWith('plug-b'); + expect(handler).toHaveBeenCalledWith('plug-b', 7); + }); + + it('a phone hears the stream before the first check succeeds', async () => { + globalThis.EventSource = function () {}; + setHouseholdChangesMode('visible'); + const handler = vi.fn(); + subscribePluginStorageChanges('plug-b', handler); + // start's own first check fails (no answer queued), so there is no + // baseline yet — and in visible mode there is no timer to retry with. + await flush(); + sseHandler({ event: 'plugin.storage.changed', payload: { pluginId: 'plug-b', revision: 9 } }); + expect(handler).toHaveBeenCalledWith('plug-b', 9); + }); + + it('a phone hears household stream revisions before the first check succeeds', async () => { + globalThis.EventSource = function () {}; + setHouseholdChangesMode('visible'); + const handler = vi.fn(); + subscribeHouseholdChanges(['chores'], handler); + await flush(); + sseHandler({ event: 'household.changed', payload: { domains: ['chores'], revisions: revisions({ chores: 11 }) } }); + expect(handler).toHaveBeenCalledWith(['chores']); }); it('a household subscriber never hears plugin counters, and household revisions exclude them', async () => { diff --git a/docs/guides/plugin-development.md b/docs/guides/plugin-development.md index 0d4b06e2..64065c21 100644 --- a/docs/guides/plugin-development.md +++ b/docs/guides/plugin-development.md @@ -417,8 +417,8 @@ const off = HomeGlow.on('clam.withdrawn', (payload, meta) => { | `chore.allCompleted` | a user's last **regular** chore for the day is done | `{ userId, username, date, reward }` | | `prize.redeemed` | a prize-store request is approved | `{ userId, prizeId, offerId, prizeName, cost, newTotal }` | | `ha.state` | Home Assistant entities changed | `{ entityIds }`: a nudge to read state again (§5a), never the state itself | -| `household.changed` | household data changed, on any display or by the server | `{ domains, revisions }`: which kinds changed (`chores`, `users`, `prizes`, `settings`, `calendar`). Reload what you show; it never says what changed. Delivered within a second where the event stream works, and within 15 seconds where it doesn't | -| `plugin.storage.changed` | your plugin's storage changed, on any display or by the server | `{ pluginId }`: only your plugin's id — never a key. Reload what you use. Delivered within a second where the event stream works, and within 15 seconds where it doesn't | +| `household.changed` | household data changed, on any display or by the server | `{ domains, revisions }`: which kinds changed (`chores`, `users`, `prizes`, `settings`, `calendar`). Reload what you show; it never says what changed. Delivered within a second where the event stream works, and behind it by the changes check (every 15 seconds on wall and desktop displays while visible; on becoming visible on phones) | +| `plugin.storage.changed` | your plugin's storage changed, on any display or by the server | `{ pluginId, revision }`: only your plugin's id and the write's new counter — never a key. Reload what you use. Delivered within a second where the event stream works, and behind it by the changes check (every 15 seconds on wall and desktop displays while visible; on becoming visible on phones) | Declaring an event not in the catalog rejects the install — typos fail loudly. Only declared events are ever delivered to your iframe. @@ -426,16 +426,23 @@ Only declared events are ever delivered to your iframe. **On `plugin.storage.changed`:** declare it and your widget stops needing to poll its own storage: every write — a PUT or DELETE on a key, an increment, or a server-side reaction to another event — pushes this event to your frame -within a second, with the 15-second changes check behind it where the stream -doesn't reach. Declaring this event requires a server with this support: an -older server rejects the install, since the event isn't in its catalog, so -there is no older-server fallback to keep. The event is scoped to your plugin -alone: a frame only ever hears its own plugin's writes, never another -plugin's, and the payload carries no key, so don't expect one — reload what -you use, and nothing about another plugin's storage can reach your frame. -A frame hears its own writes too: a widget that writes on load and reloads on -this event will loop about once a second, on every display, so only reload -for writes you didn't just make. +within a second, with the changes check behind it where the stream doesn't +reach (every 15 seconds on wall and desktop displays while visible; on +becoming visible on phones). Declaring this event requires a server with this +support: an older server rejects the install, since the event isn't in its +catalog, so there is no older-server fallback to keep. Declaring it without +`"storage": true` also rejects the install — the event only fires for +plugins that store data. The event is scoped to your plugin alone: a frame +only ever hears its own plugin's writes, never another plugin's, and the +payload carries no key, so don't expect one — reload what you use, and +nothing about another plugin's storage can reach your frame. After a server +restart, every plugin with stored data reads as moved once, so your frame +reloads once — the same promise household data has. A frame hears its own +writes too: a widget that writes on load and reloads on this event will loop +about once a second, on every display. Each storage write returns its new +revision (`HomeGlow.storage.set` resolves it), and the event carries that +revision — remember the revisions your writes returned and skip events +carrying one of them, so you only reload for writes you didn't just make. **On `chore.allCompleted`:** it fires alongside the daily-completion bonus, so it inherits that behaviour exactly — once per user per day, from whichever route diff --git a/docs/reference/backend-api.md b/docs/reference/backend-api.md index 050f9980..c9543435 100644 --- a/docs/reference/backend-api.md +++ b/docs/reference/backend-api.md @@ -96,7 +96,7 @@ segment (the browser's `localStorage` UUID). | GET | `/api/update-status` | Running version vs the latest GitHub release, checked at most every 6 hours. `HOMEGLOW_DISABLE_UPDATE_CHECK=1` turns it off. | | GET | `/api/timezone` | Server timezone. | | GET | `/api/system/backgroundTasks` | Manually trigger nightly processing. | -| GET | `/api/changes` | A revision counter for each kind of household data, `{ chores, users, prizes, settings, calendar }`, with an `ETag`; `304` for a matching `If-None-Match`. Displays check it to reload what changed (issue #325). Every successful write under the household routes moves its counters (`utils/householdChanges.js`); a chore or prize write also moves `users`, for clam totals. The nightly job, calendar sync and calendar routines move them too. The counters start from a random base on each start, so a restart reads as everything changed. | +| GET | `/api/changes` | A revision counter for each kind of household data, `{ chores, users, prizes, settings, calendar }`, plus one counter per plugin with stored data under `plugin:` (issue #347), with an `ETag`; `304` for a matching `If-None-Match`. Displays check it to reload what changed (issue #325). Every successful write under the household routes moves its counters (`utils/householdChanges.js`); a chore or prize write also moves `users`, for clam totals. The nightly job, calendar sync and calendar routines move them too. The counters start from a random base on each start, so a restart reads as everything changed — plugin counters are seeded at boot for every plugin with stored data, so their frames reload once too. | | GET | `/index.css` | Serves the app CSS for custom widgets (with fallback). | ### Custom widgets / plugins diff --git a/server/index.js b/server/index.js index 80ac2a6a..3214f15e 100644 --- a/server/index.js +++ b/server/index.js @@ -683,8 +683,11 @@ fastify.addHook('onResponse', async (request, reply) => { if (domains.length) householdChanges.bump(domains); // A plugin storage write moves only that plugin's counter (issue #347), // never a household kind — and only on success, like the hook above. + // Storage handlers bump in-route (so the response can carry the revision + // the event will carry) and set request.pluginStorageBumped; the hook + // stays as the backstop for any storage write path that doesn't. const pluginId = pluginStorageWrite(request.method, request.routeOptions?.url, reply.statusCode, request.params); - if (pluginId) householdChanges.bumpPlugin(pluginId); + if (pluginId && !request.pluginStorageBumped) householdChanges.bumpPlugin(pluginId); }); fastify.register(multipart, { @@ -1085,6 +1088,12 @@ function extractPluginManifest(htmlContent) { } } } + // Declaring the storage event without storage would install but never + // fire, so fail the install loudly — the same rule as reactions, which + // also need storage to run. + if (Array.isArray(manifest.events) && manifest.events.includes('plugin.storage.changed') && manifest.storage !== true) { + errors.push('events: "plugin.storage.changed" requires "storage": true (the event only fires for plugins that store data).'); + } } if (manifest.reactions !== undefined) { if (!Array.isArray(manifest.reactions)) { @@ -1527,7 +1536,12 @@ fastify.put('/api/plugin/v1/storage/:pluginId/:key', async (request, reply) => { updated_at = CURRENT_TIMESTAMP `).run(pluginId, key, valueJson); - return { success: true }; + // Bump in-route, not just in the onResponse hook: the response carries + // the exact revision the event will carry, so a frame can tell its own + // writes from others' (issue #347). The hook sees the flag and skips. + const revision = householdChanges.bumpPlugin(pluginId); + request.pluginStorageBumped = true; + return { success: true, revision }; }); fastify.delete('/api/plugin/v1/storage/:pluginId/:key', async (request, reply) => { @@ -1539,7 +1553,10 @@ fastify.delete('/api/plugin/v1/storage/:pluginId/:key', async (request, reply) = if (result.changes === 0) { return reply.status(404).send({ error: 'Key not found.' }); } - return { success: true }; + // Same in-route bump as PUT, for the same reason. + const revision = householdChanges.bumpPlugin(pluginId); + request.pluginStorageBumped = true; + return { success: true, revision }; }); // Atomic numeric delta on a JSON path inside a stored document — the primitive @@ -1627,7 +1644,10 @@ fastify.post('/api/plugin/v1/storage/:pluginId/:key/increment', async (request, // concurrent HTTP requests cannot interleave a read-modify-write here. const applyIncrement = db.transaction(() => incrementPluginStorage(pluginId, key, pathSegments, delta)); const { result, value } = applyIncrement(); - return { success: true, result, value }; + // Same in-route bump as PUT, for the same reason. + const revision = householdChanges.bumpPlugin(pluginId); + request.pluginStorageBumped = true; + return { success: true, result, value, revision }; } catch (error) { if (error.statusCode) { return reply.status(error.statusCode).send({ error: error.message }); @@ -7646,6 +7666,23 @@ const start = async () => { // Backup to a file and restore from one, without Google (issue #305). registerLocalBackupRoutes(fastify, { db: haDb, dbPath, uploadsDir: UPLOADS_ROOT, demoMode: DEMO_MODE }); + // Seed a counter for every plugin that already has stored data (issue + // #347): after a restart, household counters all differ from what + // displays last saw, but a plugin counter would simply be gone — and + // the client prunes vanished keys silently, so a display that missed a + // write just before the restart would keep showing stale data until + // that plugin's next write. Seeding makes those frames read as moved, + // matching the "reloads everything once" promise. Best effort: a + // missing table must never fail boot. + try { + if (doesTableExist('plugin_storage')) { + const rows = db.prepare('SELECT DISTINCT plugin_id FROM plugin_storage').all(); + householdChanges.seedPlugins(rows.map((row) => row.plugin_id)); + } + } catch (error) { + console.warn('Plugin storage seeding failed, continuing without it:', error.message); + } + await fastify.listen({ port: process.env.PORT || 5000, host: '0.0.0.0' }); console.log(`Server running on port ${process.env.PORT || 5000}`); // Dates and places for photos uploaded before the server read them, once. diff --git a/server/plugin-sdk/v1.js b/server/plugin-sdk/v1.js index 0d102982..49dfb0d1 100644 --- a/server/plugin-sdk/v1.js +++ b/server/plugin-sdk/v1.js @@ -318,7 +318,11 @@ throw error; }); }, - /** Upsert a JSON value under a key. */ + /** Upsert a JSON value under a key. Resolves { success, revision }: + * the revision is this write's new storage counter — remember the + * revisions your writes returned and skip plugin.storage.changed + * events carrying one of them, so you only reload for writes you + * didn't just make. */ set: function (key, value) { return request('PUT', '/storage/' + requirePluginId() + '/' + encodeURIComponent(key), value); }, @@ -334,7 +338,8 @@ /** * Atomically add `delta` to the number at dot-separated `path` inside the * document stored under `key` (created as {} if missing). - * -> Promise<{ result, value }> with the new leaf number and full document. + * -> Promise<{ result, value, revision }> with the new leaf number, + * full document, and this write's storage revision (see set). */ increment: function (key, path, delta) { return request('POST', '/storage/' + requirePluginId() + '/' + encodeURIComponent(key) + '/increment', { diff --git a/server/tests/householdChanges.test.js b/server/tests/householdChanges.test.js index a7df9a2b..526e2804 100644 --- a/server/tests/householdChanges.test.js +++ b/server/tests/householdChanges.test.js @@ -147,3 +147,53 @@ test('a reinstalled plugin never reuses its old counter, so its first write read assert.notEqual(changes.snapshot()['plugin:lists'], oldValue, 'purge + reinstall must not repeat the counter'); assert.notEqual(changes.etag(), oldEtag, 'purge + reinstall must not repeat the etag'); }); + +test('bumpPlugin returns the new revision, so a write route can answer with it', () => { + const changes = createHouseholdChanges({ base: 100 }); + assert.equal(changes.bumpPlugin('lists'), 101); + assert.equal(changes.bumpPlugin('lists'), 102); + assert.equal(changes.bumpPlugin('other'), 103); + assert.equal(changes.bumpPlugin(''), undefined, 'no plugin id, no revision'); +}); + +test('seeding at boot makes plugin frames read as moved after a restart', async () => { + const before = createHouseholdChanges({ base: 100 }); + before.bumpPlugin('lists'); + before.bumpPlugin('lists'); + const knownValue = before.snapshot()['plugin:lists']; + assert.equal(knownValue, 102); + + // A restart: a new instance, seeded with every plugin that has stored + // data. The seeded counter differs from what the display last saw, so + // the frame reads as moved instead of silently pruning a vanished key + // and showing stale data until the plugin's next write. + const pluginEvents = []; + const after = createHouseholdChanges({ + base: 5000, + coalesceMs: 20, + onPluginStorage: (pluginId, revision) => pluginEvents.push({ pluginId, revision }), + }); + after.seedPlugins(['lists']); + const seeded = after.snapshot()['plugin:lists']; + assert.notEqual(seeded, knownValue); + assert.ok(seeded !== undefined); + + // Seeding is deterministic (sorted order, sequence values) and never + // clobbers an existing counter — and it schedules no event: nothing + // was written, and no display is listening yet. + const again = createHouseholdChanges({ base: 5000 }); + again.seedPlugins(['lists']); + assert.equal(again.snapshot()['plugin:lists'], seeded, 'same ids seed the same values'); + const two = createHouseholdChanges({ base: 5000 }); + two.seedPlugins(['zeta', 'alpha']); + assert.notEqual(two.snapshot()['plugin:zeta'], two.snapshot()['plugin:alpha'], 'seeded counters are distinct per plugin'); + after.seedPlugins(['lists']); + assert.equal(after.snapshot()['plugin:lists'], seeded, 're-seeding leaves an existing counter alone'); + await new Promise((r) => setTimeout(r, 40)); + assert.deepEqual(pluginEvents, [], 'seeding sends no event'); + + // The first real write after the seed takes the next sequence value, + // never the seed itself. + after.bumpPlugin('lists'); + assert.notEqual(after.snapshot()['plugin:lists'], seeded); +}); diff --git a/server/tests/pluginPlatform.test.js b/server/tests/pluginPlatform.test.js index 68af6f66..156d60d6 100644 --- a/server/tests/pluginPlatform.test.js +++ b/server/tests/pluginPlatform.test.js @@ -683,6 +683,26 @@ test('manifests may only declare catalog events', async () => { assert.equal(ok.status, 200); }); +test('declaring plugin.storage.changed without storage rejects the install', async () => { + const noStorage = { + manifestVersion: 1, + id: 'event-no-storage', + events: ['plugin.storage.changed'], + }; + const upload = await uploadWidget('event-no-storage.html', widgetHtml(noStorage)); + assert.equal(upload.status, 400); + assert.match(upload.body.error, /requires "storage": true/); + + const withStorage = { + manifestVersion: 1, + id: 'event-with-storage', + storage: true, + events: ['plugin.storage.changed'], + }; + const ok = await uploadWidget('event-with-storage.html', widgetHtml(withStorage)); + assert.equal(ok.status, 200); +}); + // --- Manifest description (issue #147) --- test('a manifest description is stored and returned with the plugin', async () => { diff --git a/server/tests/pluginStorageChanges.test.js b/server/tests/pluginStorageChanges.test.js index c78a74f6..fdbd6e7f 100644 --- a/server/tests/pluginStorageChanges.test.js +++ b/server/tests/pluginStorageChanges.test.js @@ -229,3 +229,95 @@ test('the etag moves with a plugin write, and 304 holds while nothing changes', const again = await fetch(`${base}/api/changes`, { headers: { 'If-None-Match': second.headers.get('etag') } }); assert.equal(again.status, 304); }); + +test('storage writes return their revision, and the stream event carries the same one', async () => { + await upload('plug-rev.html', manifestPage(pluginManifest('plug-rev'))); + const tokenA = await tokenOf('plug-rev.html'); + const put = await asPlugin('/api/plugin/v1/storage/plug-rev/rev', tokenA, { + method: 'PUT', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ n: 1 }), + }); + assert.equal(put.status, 200, JSON.stringify(put.body)); + assert.equal(typeof put.body.revision, 'number', 'a PUT answers with its revision'); + const afterPut = await changes(); + assert.equal(afterPut['plugin:plug-rev'], put.body.revision, 'the changes snapshot carries the returned revision'); + + const removed = await asPlugin('/api/plugin/v1/storage/plug-rev/rev', tokenA, { method: 'DELETE' }); + assert.equal(removed.status, 200, JSON.stringify(removed.body)); + assert.equal(typeof removed.body.revision, 'number', 'a DELETE answers with its revision'); + assert.notEqual(removed.body.revision, put.body.revision, 'each write takes the next sequence value'); + + const inc = await asPlugin('/api/plugin/v1/storage/plug-rev/cnt/increment', tokenA, { + method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ path: 'n', delta: 2 }), + }); + assert.equal(inc.status, 200, JSON.stringify(inc.body)); + assert.equal(typeof inc.body.revision, 'number', 'an increment answers with its revision'); + assert.equal(inc.body.result, 2); + + // The stream event for a write carries the exact revision the write + // returned, so a frame can compare it against its own writes. + await delay(300); + const messages = []; + const req = http.get(`${base}/api/plugin/v1/events/stream`, (res) => { + let buffer = ''; + res.on('data', (chunk) => { + buffer += chunk; + let idx; + while ((idx = buffer.indexOf('\n')) !== -1) { + const line = buffer.slice(0, idx).replace(/\r$/, ''); + buffer = buffer.slice(idx + 1); + if (!line.startsWith('data: ')) continue; + messages.push(JSON.parse(line.slice(6))); + } + }); + }); + await delay(300); + const put2 = await asPlugin('/api/plugin/v1/storage/plug-rev/rev2', tokenA, { + method: 'PUT', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ n: 2 }), + }); + assert.equal(put2.status, 200, JSON.stringify(put2.body)); + await delay(500); + req.destroy(); + const events = messages.filter((message) => message.event === 'plugin.storage.changed' && message.payload?.pluginId === 'plug-rev'); + assert.ok(events.length >= 1, JSON.stringify(messages)); + assert.equal(events[events.length - 1].payload.revision, put2.body.revision, 'the event carries the revision the write returned'); + assert.equal('key' in events[events.length - 1].payload, false, 'the event still carries no key'); +}); + +// Last: it restarts the server, so nothing after it may assume the old one. +test('a restart seeds counters for plugins with stored data, so frames read as moved', async () => { + await upload('plug-restart.html', manifestPage(pluginManifest('plug-restart'))); + const tokenA = await tokenOf('plug-restart.html'); + await asPlugin('/api/plugin/v1/storage/plug-restart/restart-mark', tokenA, { + method: 'PUT', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(1), + }); + const before = await changes(); + const oldValue = before['plugin:plug-restart']; + assert.equal(typeof oldValue, 'number', 'plug-restart has a counter before the restart'); + + // Restart on the same database: uploads and storage rows persist. + await new Promise((resolve) => { + if (child.exitCode !== null) return resolve(); + child.once('close', resolve); + child.kill(); + setTimeout(resolve, 5000); + }); + const port = await freePort(); + child = spawn(process.execPath, ['index.js'], { + cwd: serverDir, + env: { + ...process.env, PORT: String(port), DB_PATH: path.join(dataDir, 'tasks.db'), TZ: 'UTC', DEMO_MODE: 'false', + ENCRYPTION_KEY: Buffer.alloc(32, 6).toString('base64'), + HOMEGLOW_DISABLE_BACKGROUND_JOBS: '1', HOMEGLOW_DISABLE_CALENDAR_SYNC: '1', HOMEGLOW_DISABLE_UPDATE_CHECK: '1', + }, + stdio: ['ignore', 'ignore', 'ignore'], + }); + base = `http://127.0.0.1:${port}`; + const start = Date.now(); + while (Date.now() - start < 30000) { + try { if ((await fetch(`${base}/api/test`)).ok) break; } catch { /* starting */ } + await delay(250); + } + const after = await changes(); + assert.ok(after['plugin:plug-restart'] !== undefined, 'the restarted server seeds a counter for the plugin'); + assert.notEqual(after['plugin:plug-restart'], oldValue, 'the seeded counter differs, so the frame reads as moved'); +}); diff --git a/server/utils/householdChanges.js b/server/utils/householdChanges.js index 25852a0a..c93369b2 100644 --- a/server/utils/householdChanges.js +++ b/server/utils/householdChanges.js @@ -24,7 +24,13 @@ // // Counters live in memory and start from a random base at boot, so after a // restart every counter differs from what any display last saw, and every -// display reloads everything once. +// display reloads everything once. Plugin counters need the same treatment +// explicitly: a plugin counter is created on its first write, so after a +// restart it would simply be gone — and the client prunes vanished keys +// silently, so a display that missed a write just before the restart would +// keep showing stale data until that plugin's next write. seedPlugins() +// (called at boot with every plugin that already has stored data) gives +// those frames the same "read as moved" restart the household kinds get. const crypto = require('crypto'); const DOMAINS = Object.freeze(['chores', 'users', 'prizes', 'settings', 'calendar']); @@ -153,13 +159,33 @@ function createHouseholdChanges({ onBump = () => {}, onPluginStorage = () => {}, // kinds never move here. Every bump takes the next server-wide sequence // value, not +1 on the plugin's own counter: after uninstall-with-purge // and a reinstall, the first write must not repeat the old value, or a - // display holding the old ETag would read a 304 and miss it. + // display holding the old ETag would read a 304 and miss it. Returns the + // new revision, so a storage write route can answer with the exact value + // its event will carry (a frame compares it against its own writes). function bumpPlugin(pluginId) { - if (typeof pluginId !== 'string' || pluginId.length === 0) return; + if (typeof pluginId !== 'string' || pluginId.length === 0) return undefined; const domain = pluginDomain(pluginId); revisions[domain] = ++pluginSeq; pending.add(domain); scheduleFlush(); + return revisions[domain]; + } + + // At boot, seed a counter for every plugin that already has stored data + // (the server passes the distinct plugin ids from its storage rows). + // Values take the next server-wide sequence value each, in sorted plugin + // order, so they are distinct and never equal to `base`; a display that + // knew the plugin before the restart therefore reads the seeded counter + // as moved. Existing counters are left alone, and seeding schedules no + // event: nothing was written, and no display is listening yet — the + // seeded counters simply appear in the first post-boot check. + function seedPlugins(pluginIds) { + const ids = [...new Set((pluginIds || []).filter((id) => typeof id === 'string' && id.length > 0))].sort(); + for (const id of ids) { + const domain = pluginDomain(id); + if (Object.prototype.hasOwnProperty.call(revisions, domain)) continue; + revisions[domain] = ++pluginSeq; + } } // The plugin's data is gone (uninstall with purge): its counter goes @@ -172,7 +198,7 @@ function createHouseholdChanges({ onBump = () => {}, onPluginStorage = () => {}, pending.delete(domain); } - return { bump, bumpPlugin, removePlugin, snapshot, etag, flush }; + return { bump, bumpPlugin, removePlugin, seedPlugins, snapshot, etag, flush }; } module.exports = { From def672331cd5da4304eed09ff225e7cb946383b9 Mon Sep 17 00:00:00 2001 From: adamecker Date: Tue, 3 Nov 2026 05:44:42 -0800 Subject: [PATCH 5/5] Address jherforth's re-review on plugin storage push (#347) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Guide: drop the skip-your-own-revisions advice — one event can stand for several writes (100 ms coalescing, latest-counter-only changes check), so skipping your own revision can lose another display's write. Keep the warning reworded: don't write storage in response to this event, or you'll loop. Revision fields stay, harmless. - Remove the onResponse hook's plugin-storage backstop: it only matched the three storage routes, all of which bump in-route, so it could never fire. Drop the now-unused pluginStorageWrite helper and its test with it; storage counters move in-route only. Server suite 712/712. --- docs/guides/plugin-development.md | 15 ++++++++++----- server/index.js | 21 +++++++-------------- server/tests/householdChanges.test.js | 15 --------------- server/utils/householdChanges.js | 23 ----------------------- 4 files changed, 17 insertions(+), 57 deletions(-) diff --git a/docs/guides/plugin-development.md b/docs/guides/plugin-development.md index 64065c21..9b1b8df7 100644 --- a/docs/guides/plugin-development.md +++ b/docs/guides/plugin-development.md @@ -438,11 +438,16 @@ payload carries no key, so don't expect one — reload what you use, and nothing about another plugin's storage can reach your frame. After a server restart, every plugin with stored data reads as moved once, so your frame reloads once — the same promise household data has. A frame hears its own -writes too: a widget that writes on load and reloads on this event will loop -about once a second, on every display. Each storage write returns its new -revision (`HomeGlow.storage.set` resolves it), and the event carries that -revision — remember the revisions your writes returned and skip events -carrying one of them, so you only reload for writes you didn't just make. +writes too: don't write storage in response to this event, or you'll loop — +a widget that writes on load and reloads on this event will loop about once +a second, on every display. Each storage write returns its new revision +(`HomeGlow.storage.set` resolves it), and the event carries that revision, +but don't skip events carrying your own revisions: one event can stand for +several writes — writes within the 100 ms coalescing window go out as one +event carrying only the latest revision, and the changes check returns only +the latest counter — so skipping your own revision can lose another +display's write. Reloading after your own write costs one extra fetch; +skipping can lose data. **On `chore.allCompleted`:** it fires alongside the daily-completion bonus, so it inherits that behaviour exactly — once per user per day, from whichever route diff --git a/server/index.js b/server/index.js index 3214f15e..32eed8c0 100644 --- a/server/index.js +++ b/server/index.js @@ -231,7 +231,7 @@ axios.interceptors.request.use((config) => { let calendarSyncService = null; const pluginEvents = require('./services/pluginEvents'); -const { DOMAINS: HOUSEHOLD_DOMAINS, createHouseholdChanges, domainsForRequest, pluginStorageWrite } = require('./utils/householdChanges'); +const { DOMAINS: HOUSEHOLD_DOMAINS, createHouseholdChanges, domainsForRequest } = require('./utils/householdChanges'); // A revision counter for each kind of household data, so every display can // tell what changed and reload it (issue #325, utils/householdChanges.js). // Each burst of changes also goes out on the event stream, the fast path @@ -681,13 +681,10 @@ fastify.addHook('onRequest', async (request, reply) => { fastify.addHook('onResponse', async (request, reply) => { const domains = domainsForRequest(request.method, request.routeOptions?.url, reply.statusCode); if (domains.length) householdChanges.bump(domains); - // A plugin storage write moves only that plugin's counter (issue #347), - // never a household kind — and only on success, like the hook above. - // Storage handlers bump in-route (so the response can carry the revision - // the event will carry) and set request.pluginStorageBumped; the hook - // stays as the backstop for any storage write path that doesn't. - const pluginId = pluginStorageWrite(request.method, request.routeOptions?.url, reply.statusCode, request.params); - if (pluginId && !request.pluginStorageBumped) householdChanges.bumpPlugin(pluginId); + // Plugin storage counters move in-route only (issue #347): the three + // storage handlers bump so the response can carry the revision the event + // will carry. There is no hook backstop here — the old one could never + // fire, since it only matched those same three routes. }); fastify.register(multipart, { @@ -1536,11 +1533,9 @@ fastify.put('/api/plugin/v1/storage/:pluginId/:key', async (request, reply) => { updated_at = CURRENT_TIMESTAMP `).run(pluginId, key, valueJson); - // Bump in-route, not just in the onResponse hook: the response carries - // the exact revision the event will carry, so a frame can tell its own - // writes from others' (issue #347). The hook sees the flag and skips. + // Bump in-route so the response carries the exact revision the event + // will carry (issue #347). const revision = householdChanges.bumpPlugin(pluginId); - request.pluginStorageBumped = true; return { success: true, revision }; }); @@ -1555,7 +1550,6 @@ fastify.delete('/api/plugin/v1/storage/:pluginId/:key', async (request, reply) = } // Same in-route bump as PUT, for the same reason. const revision = householdChanges.bumpPlugin(pluginId); - request.pluginStorageBumped = true; return { success: true, revision }; }); @@ -1646,7 +1640,6 @@ fastify.post('/api/plugin/v1/storage/:pluginId/:key/increment', async (request, const { result, value } = applyIncrement(); // Same in-route bump as PUT, for the same reason. const revision = householdChanges.bumpPlugin(pluginId); - request.pluginStorageBumped = true; return { success: true, result, value, revision }; } catch (error) { if (error.statusCode) { diff --git a/server/tests/householdChanges.test.js b/server/tests/householdChanges.test.js index 526e2804..8ac04607 100644 --- a/server/tests/householdChanges.test.js +++ b/server/tests/householdChanges.test.js @@ -58,21 +58,6 @@ test('every server starts from its own base, so a restart reads as "everything c }); // Plugin storage counters (issue #347): one per plugin, in the same map. -const { pluginStorageWrite } = require('../utils/householdChanges'); - -test('pluginStorageWrite routes only successful storage writes, by plugin id', () => { - const params = { pluginId: 'lists' }; - assert.equal(pluginStorageWrite('PUT', '/api/plugin/v1/storage/:pluginId/:key', 200, params), 'lists'); - assert.equal(pluginStorageWrite('DELETE', '/api/plugin/v1/storage/:pluginId/:key', 204, params), 'lists'); - assert.equal(pluginStorageWrite('POST', '/api/plugin/v1/storage/:pluginId/:key/increment', 200, params), 'lists'); - assert.equal(pluginStorageWrite('GET', '/api/plugin/v1/storage/:pluginId/:key', 200, params), null, 'reads move nothing'); - assert.equal(pluginStorageWrite('PUT', '/api/plugin/v1/storage/:pluginId/:key', 400, params), null, 'failed writes move nothing'); - assert.equal(pluginStorageWrite('PUT', '/api/plugin/v1/settings/:pluginId', 200, params), null, 'settings are not storage'); - assert.equal(pluginStorageWrite('POST', '/api/plugin/v1/storage/:pluginId/:key', 200, params), null, 'POST on a key is not a write path'); - assert.equal(pluginStorageWrite('PUT', '/api/plugin/v1/storage/:pluginId/:key', 200, {}), null, 'no plugin id, no counter'); - assert.equal(pluginStorageWrite('PUT', undefined, 200, params), null, 'no route, no counter'); -}); - test('a plugin write moves only that plugin counter, in its own message', async () => { const household = []; const plugins = []; diff --git a/server/utils/householdChanges.js b/server/utils/householdChanges.js index c93369b2..c6daff30 100644 --- a/server/utils/householdChanges.js +++ b/server/utils/householdChanges.js @@ -75,28 +75,6 @@ function domainsForRequest(method, route, statusCode) { return match ? match[1] : []; } -/** - * The plugin whose storage a finished request wrote, if any (issue #347): - * PUT or DELETE on a storage key, or POST on a key's /increment — and only - * on success, like domainsForRequest. Reads, failed writes, other plugin - * routes (settings, notify, ha) and other routes return null. `params` is - * the request's route params, so the id comes from the path, not the body. - */ -function pluginStorageWrite(method, route, statusCode, params) { - if (!route || statusCode >= 400) return null; - const pluginId = params?.pluginId; - if (typeof pluginId !== 'string' || pluginId.length === 0) return null; - switch (String(method).toUpperCase()) { - case 'PUT': - case 'DELETE': - return route === '/api/plugin/v1/storage/:pluginId/:key' ? pluginId : null; - case 'POST': - return route === '/api/plugin/v1/storage/:pluginId/:key/increment' ? pluginId : null; - default: - return null; - } -} - /** * A set of counters. `onBump(domains, revisions)` is called once per burst * of bumps, after `coalesceMs`, so a write touching several kinds, or a @@ -207,6 +185,5 @@ module.exports = { PLUGIN_DOMAIN_PREFIX, pluginDomain, domainsForRequest, - pluginStorageWrite, createHouseholdChanges, };