Skip to content

feat(plugins): push plugin storage writes as plugin.storage.changed (#347) - #363

Open
adamecker wants to merge 4 commits into
jherforth:mainfrom
adamecker:pr/plugin-storage-push
Open

adamecker wants to merge 4 commits into
jherforth:mainfrom
adamecker:pr/plugin-storage-push

Conversation

@adamecker

Copy link
Copy Markdown
Contributor

Implements #347: plugins can now hear about storage writes without polling.

  • Event: a frame subscribed to a plugin's storage receives plugin.storage.changed with { pluginId } only — no key, no value — and reloads what it uses. The plugin guide says so explicitly, so authors don't expect a key.
  • Counters: each plugin gets a counter on the feat: keep every display in step with one revision counter per kind of data (#325) #339 change-counter path, pushed over the event stream within about a second and caught up by the 15-second GET /api/changes check. Counters live under plugin:<pluginId>, are created on a plugin's first storage write, and drop on uninstall-with-purge (kept without purge, since the data is kept). The ETag stays deterministic (fixed kinds plus plugin kinds in sorted order); GET /api/changes returns the full map; household.changed payloads still carry household kinds only.
  • Scoping: displays see every plugin's counter; a frame hears only its own plugin's events.
  • Write paths covered: PUT, DELETE, and POST increment move only that plugin's counter on success (reads, failed writes, and other plugin routes move nothing); server-side reactions bump explicitly. A reaction never fires on plugin.storage.changed itself, since the reaction writes plugin storage and would otherwise re-trigger in a loop.
  • Polling remains only as the older-server fallback.

Tests: new server unit tests (routing, flush partitioning, ETag determinism, prune), new real-server tests for each write path plus the no-key payload, purge behavior, and ETag movement, and extended client tracker tests including frame isolation (a subscription hears only its own plugin from both the check and the stream). Full client and server suites pass, as do the build and i18n checks.

Generated with Muse AI 🤖

@jherforth

Copy link
Copy Markdown
Owner

Thanks Adam. This follows the shape agreed in #347 closely, and the isolation holds up:

  • the event stream refuses plugin frames;
  • a frame gets { pluginId } and nothing else;
  • knownHouseholdRevisions() no longer carries plugin counters;
  • every server write path moves the counter: PUT, DELETE, increment and reactions. Purge drops it.

A few things to change before merge, most important first.

1. Backend CI fails, from a timing race in a test. a server-side reaction moves the plugin counter… expects exactly one plugin.storage.changed, but on CI it got three. The previous test's plug-a and plug-b writes were still inside the 100 ms coalescing window when this test opened its stream, so they were sent into it. It passes locally (3 of 3 runs here). Fix: filter storageEvents to pluginId === 'plug-react', and/or wait out the window (e.g. await delay(300)) before opening the stream.

2. A reinstalled plugin's counter can repeat an old value, and the ETag can't tell. New counters start from the boot base, and the ETag joins the values without their keys. So after uninstall-with-purge and a reinstall, the first write produces the same counter and the same ETag as before. Calling createHouseholdChanges({ base: 100 }) directly:

lists written, purged, reinstalled, written | counter 101 -> 101 | etag "100-100-100-100-100-101" -> same
alpha purged, then beta's first write       |                    | etag "…-101-101" -> same

What goes wrong on a display:

  • One holding the old ETag gets a 304 from the check.
  • The stream event is dropped too, because the client still remembers the stale value: applySnapshot only prunes when something else moved.

So the first write after a reset is lost on every display, which is the case this PR exists for. Suggested fix: one server-wide sequence for plugin counters, so a value is never reused within a boot:

let pluginSeq = base;
// in bumpPlugin, instead of starting from base:
revisions[domain] = ++pluginSeq;

(or put the keys in the ETag). It's also worth pruning in applySnapshot on every 200, not only when something moved. The client test "a reinstall reads as changed" passes only because it uses 7 and then 3. A test where the values are equal would catch this.

3. Drop the Google Tasks route entry. ROUTE_DOMAINS gains /api/users/:id/google-tasks/sync, but HomeGlow has no such route; it looks like it came from another branch. It's harmless, but it's dead code and unrelated to #347.

4. Reject reactions[].on: "plugin.storage.changed" at install. The loop guard in runDeclarativeReactions is right. But the manifest validation (server/index.js ~1099) still accepts a reaction on this event, and that reaction then never runs, without any error. The catalog's promise is that mistakes fail loudly, so add it to the install errors.

5. Plugin guide wording.

  • "Keep polling as a fallback for older servers": an older server won't install the plugin at all, because declaring an event that isn't in its catalog fails the install (the guide's next paragraph says so). Either say that declaring it requires this version, or drop the fallback sentence.
  • Worth a line that 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.

Smaller: the branch is two commits behind main (#362, #364). A rebase will re-run CI against them.

🤖 Review written with Claude Code

adamecker added 2 commits October 31, 2026 17:41
…herforth#347)

Per-plugin storage counters on the jherforth#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).
- 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.
@adamecker
adamecker force-pushed the pr/plugin-storage-push branch from 11b5d9e to a5f3879 Compare October 31, 2026 17:53
@adamecker

Copy link
Copy Markdown
Contributor Author

Thanks for the review — all five points are addressed in the latest push, and the branch is rebased onto main (#362, #364):

  1. Test race: the reaction test now waits out the 100 ms coalescing window before opening the stream, and counts only plug-react's own events, so an earlier test's burst can't be mistaken for this test's event.
  2. Counter/ETag reuse: plugin counters now start from one server-wide sequence (pluginSeq), never reused within a boot, so uninstall-with-purge + reinstall can't repeat the old counter or ETag. The client also prunes vanished plugin keys on every 200 now, not only when something else moved. Added a server test (purge + reinstall gives a fresh counter and ETag) and a client test where the reinstall value equals the old one, which is the case that previously slipped through.
  3. Dead route: the /api/users/:id/google-tasks/sync ROUTE_DOMAINS entry is gone — verified no such route exists.
  4. Silent reactions: installing a manifest with a reaction on plugin.storage.changed now fails loudly at install instead of never running.
  5. Guide: the older-server fallback sentence is replaced (declaring the event requires a server with this support — an older server rejects the install), and there's a line that a frame hears its own writes too, with the write-on-load + reload-on-event loop warning.

Server tests (householdChanges, pluginStorageChanges, pluginPlatform, householdChangesRoutes: 58 pass) and client householdChanges tests (14 pass) are green locally; CI will re-run against the rebased branch.

Generated with Muse AI 🤖

@jherforth

Copy link
Copy Markdown
Owner

Thanks Adam. I re-checked a5f3879 on the rebased branch. Points 1, 3, 4 and 5 are done and look right:

  • the reaction test now waits out the coalescing window and counts only plug-react; it passes 3 of 3 runs here, and CI is green;
  • the dead route is gone;
  • a reaction on plugin.storage.changed fails at install, with a test;
  • the guide wording is fixed.

The client pruning on every 200 is also good.

One thing left on point 2: the counter can still repeat. My snippet was ambiguous here, sorry. The sequence is only used when a counter is created, and later writes still do revisions[domain] += 1, so a plugin's own count can climb back onto a value it had before:

two writes, purge, reinstall, one write: counter 102 -> 102, etag "100-100-100-100-100-102" -> same
six writes, purge, reinstall, 5 writes:  counter 106 -> 106, etag same

The window is narrower now, thanks to the client prune. It only bites when the purge and the reinstall's first write both happen between two of a display's checks (15 s, or longer while the tab is hidden). In that case the display sends the old ETag and gets 304, and the stream event is dropped as "already known".

The fix is to take the next sequence value on every plugin bump:

function bumpPlugin(pluginId) {
  if (typeof pluginId !== 'string' || pluginId.length === 0) return;
  const domain = pluginDomain(pluginId);
  revisions[domain] = ++pluginSeq;
  pending.add(domain);
  scheduleFlush();
}

Then no plugin counter value is ever reused within a boot. Every write and every purge changes the set of values, so the ETag can't repeat either, even without keys in it. The new server test passes only because it creates other between the purge and the reinstall. Dropping that line, with two writes before the purge, reproduces the first case above. One test expectation shifts: in the "moves only that plugin counter" test, plugin:other becomes 103, since lists took 101 and 102.

With that change I'm happy with this.

🤖 Review written with Claude Code

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
@adamecker

Copy link
Copy Markdown
Contributor Author

Thanks — good catch, and thanks for spelling out the remaining case. Fixed in the latest push:

  • bumpPlugin now takes the next server-wide sequence value on every bump (revisions[domain] = ++pluginSeq), not just when the counter is created, so no plugin counter value is ever reused within a boot and the ETag can't repeat either.
  • The reinstall test no longer leans on an intervening write to another plugin: it now does two writes, purge, reinstall, one write, and asserts a fresh counter (103, not the old 102) and a fresh ETag — the exact case that previously slipped through.
  • The one expectation that shifts, as you predicted: in the "moves only that plugin counter" test, plugin:other is now 103, since lists took 101 and 102.

Server tests (householdChanges, pluginStorageChanges, pluginPlatform, householdChangesRoutes: 58 pass) and client householdChanges tests (14 pass) are green locally; CI will re-run against the new push.

Generated with Muse AI 🤖

@mrramam

mrramam commented Oct 31, 2026

Copy link
Copy Markdown
Collaborator

Read through the latest push, not run. The fixes Jay asked for look right. Three things left, the first one being the one I'd settle before merge:

  1. A restart doesn't reach plugin frames. Household kinds all change on restart, but plugin keys are gone and changedPluginIds only walks next, so the client prunes them silently. A display that missed a write just before the restart keeps stale data until that plugin's next write, and the guide tells authors to drop polling. Seeding a counter for each storage plugin at boot in createHouseholdChanges would make them read as moved, matching the "reloads everything once" promise.
  2. The guide says "only reload for writes you didn't just make," but the wrapper posts only { pluginId }, so a frame can't tell its own writes. Passing revision through, and returning it from storage writes, would let it compare.
  3. In visible mode (phones) there's no timer, and applyRevisions and applyPluginRevision drop everything until a first GET succeeds. If that GET fails, the phone stays deaf until a visibility change. The old apply() could set the baseline from the stream. The guide's 15-second check also doesn't apply on phones.

Smaller: declaring this event without storage: true installs but never fires, so it could fail at install the way reactions do. backend-api.md still lists five fixed keys for /api/changes.

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:<pluginId>
   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.
@adamecker

Copy link
Copy Markdown
Contributor Author

Thanks @mrramam — all three points plus the two smaller fixes are in, pushed as bf48ce4.

  1. Restart seeding (the must-fix): the server now seeds a counter at boot for every plugin with stored data, sourced from the distinct plugin ids in its plugin_storage rows — precise, because a display can only have known a counter for a plugin that wrote storage, and storage rows imply storage:true. Seeded counters take the next server-wide sequence values in sorted plugin order, so they are distinct and never equal to the boot base, and a display that knew the plugin reads the seeded counter as moved. Seeding schedules no event: nothing was written, and no display is listening yet. Uninstalled-with-purge plugins have no rows and are correctly not seeded; uninstalled-without-purge plugins keep their rows and are seeded, since their data is still there.

  2. Self-write detection: storage writes now return their revision — PUT and DELETE answer { success: true, revision }, increment answers { success: true, result, value, revision }. The revision field is purely additive; existing fields are unchanged. The bump happens in the route handler (not just the response hook) so the response carries the exact revision the event will carry; the hook stays as a backstop for any other storage write path. The frame event now carries { pluginId, revision } — still no key, never a value — and the guide tells authors to remember the revisions their writes returned and skip events carrying one of them, so a frame only reloads for writes it didn't just make.

  3. Visible-mode (phones) deafness: in visible mode the stream now establishes the baseline instead of dropping events until a first GET succeeds, so a phone whose first check fails still hears stream events rather than staying deaf until a visibility change. Interval-mode behavior is unchanged — pre-baseline stream events are still dropped there, since the timer retries the check. The guide is corrected too: the 15-second check applies to wall and desktop displays while visible; phones check when they become visible.

  4. Install rejection: declaring plugin.storage.changed without "storage": true now fails at install, with the same wording as the reactions rule — the event only fires for plugins that store data.

  5. Docs: the backend-api.md /api/changes row now describes the dynamic plugin: counters alongside the five fixed household kinds, including the boot-seeding restart promise.

Tests: a server unit test proving a restart makes plugin frames read as moved (seeded value differs, re-seeding is a no-op, the first write after a seed takes the next value, seeding sends no event), plus a real-server restart test (write, restart on the same database, counter present and changed); a real-server test that PUT/DELETE/increment return their revision and the stream event carries the same one; client tests for the visible-mode failed-first-check-then-stream case (plugin and household); a platform test for the install rejection; and updated client assertions for the revision argument. Full client suite 891/891, full server suite 713/713, vite build and i18n check green.

The PR awaits re-review — happy to adjust if you or Jay see anything else.

Generated with Muse AI 🤖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

3 participants