Skip to content

feat: tell the owner when a local collector goes silent - #74

Merged
tnunamak merged 9 commits into
mainfrom
feat/collector-silence-detector
Sep 9, 2026
Merged

tnunamak merged 9 commits into
mainfrom
feat/collector-silence-detector

Conversation

@tnunamak

@tnunamak tnunamak commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

A local collector that stops running is invisible to the server. No run fails, so no error is recorded, and the console keeps showing the device as healthy. Two collectors on one machine were dead for eleven days that way.

This adds a silence check to the existing maintenance sweep. Each tick finds device source instances whose last heartbeat is older than 24 hours, opens an attention record for each, and sends one push notification. The record is keyed by the instance and the heartbeat the silence followed, so one outage produces one notice, and a collector that recovers and dies again produces a second.

Instances that never checked in or were revoked are excluded. The push title carries only the connector display name and the body is fixed text, so nothing device-specific reaches a lock screen. The query returns unreported instances before undelivered retries, so the batch cap cannot starve a new outage.

Accepted limits, also stated in the stage comment:

  • A push may still go out within one sweep interval after the owner resolves the notice. The record is never reopened.
  • If the process dies between sending and recording, the next tick may repeat that push.
  • No push crossed a network in tests. The sender is stubbed.

Verify: 40 tests in four files under reference-implementation/test/. Thirty run against the real attention store and device rows — 27 on SQLite, 3 on Postgres when PDPP_TEST_POSTGRES_URL is set, recording an explained skip otherwise. The remaining 10 are pure projections over the health derivation and touch no database.

Stacked on #72, which fixes the health derivation that kept a dead collector green.

Assisted-by: AI

@tnunamak
tnunamak force-pushed the fix/local-device-stale-heartbeat-health branch from 5744a04 to b232dcf Compare September 9, 2026 14:55
@tnunamak
tnunamak force-pushed the feat/collector-silence-detector branch from 8a17506 to 12f2024 Compare September 9, 2026 15:16
@tnunamak
tnunamak force-pushed the feat/collector-silence-detector branch from a6bc572 to ac401ba Compare September 9, 2026 17:30
@tnunamak
tnunamak deleted the branch main September 9, 2026 22:47
@tnunamak tnunamak closed this Sep 9, 2026
@tnunamak tnunamak reopened this Sep 9, 2026
@tnunamak
tnunamak changed the base branch from fix/local-device-stale-heartbeat-health to main September 9, 2026 22:48
A local collector is a one-shot program a host supervisor starts on a timer: it
uploads what it has, posts a heartbeat, and exits. When it dies before its first
network call — a bad install, a missing dependency — the server learns nothing
at all. No run failed, no error arrived, no record of a run was created. The
only thing that changes is that heartbeats stop. A counter that watches for
failures cannot see this, because nothing fails. On one machine two collectors
died this way and the product said nothing for eleven days.

So silence is the detector. A new phase on the connector-maintenance sweep — a
timer that already runs several housekeeping jobs every 60 seconds — finds
device source instances, the rows pairing one device with one connector and
holding that pairing's last heartbeat, whose last check-in is more than 24 hours
old, and opens an attention record for each. An attention record is this
codebase's existing durable "the owner needs to do something", shown in the
console. Nothing new was built to carry the condition.

The query returns only work that is not already finished, which is what keeps
the rest of the design small. It excludes an episode once the owner has been
told, and once the owner has acted — a lifecycle other than `open` means
resolved, acknowledged or cancelled, and the stage upserts a freshly-built
`open` record, so a row that stayed selected would silently overwrite that
decision. Both tests are needed: excluding only on delivery loses decisions made
before a first send succeeded, and excluding on the record merely existing would
drop a notice permanently whenever a notifier failed before handing the push
over, since that path records nothing.

Because handled rows leave the result set, a per-tick batch limit is a complete
answer to bounding the work, with no cursor, no persisted position and no wrap
rule. The one exception is a retry: an undelivered record stays selected on
purpose, and if retries sorted alongside new work then a correlated failure — an
expired push credential, an unreachable endpoint — would fill every batch with
the same rows and starve everything behind them. The ordering therefore puts
rows with no attention record first, so a tick always spends its budget on
collectors nobody has been told about and retries take what is left.

The record's id embeds the heartbeat that preceded the silence, making it an
identifier for one outage rather than one machine. A collector that is fixed,
runs again, and later breaks gets a different id and is announced again, while
the resolved notice for the old outage stays resolved. A successful heartbeat is
what re-arms the detector, and it also holds the row out of the query until the
threshold elapses again, so those two conditions become true together.

24 hours sits above `HEARTBEAT_LEASE_MS`, the existing 30-minute constant
deciding whether a heartbeat still describes a collector's current state. 30
minutes is right for "can I trust this status", where being wrong greys out a
badge, and wrong for "should I interrupt someone": at the documented 15-minute
collection cadence it would report a closed laptop as an incident weekly. 24
hours is chosen rather than derived, and a machine off over a long weekend will
still raise a notice.

The `.sql` artifacts are read only by the SQLite path, so the PostgreSQL
implementation carries a hand-written copy of this statement that must be edited
alongside it. The two differ where the engines do — the exclusion reads a JSON
field, `json_extract` on SQLite and `->>` on PostgreSQL — so a separate test file
covers the PostgreSQL copy directly, gated on `PDPP_TEST_POSTGRES_URL` following
the pattern already used elsewhere in this suite.

Verified on both backends: 259 pass and 3 skip on SQLite, and 229 pass with zero
skips against an isolated PostgreSQL 16.15 server. Tests use the real attention
store and real device rows rather than stand-ins, because the reporting rule
lives in SQL and a stand-in written to match what the author expected would
agree with the author instead of the database.

Assisted-by: AI
Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
An attention record only helps if someone opens the console, and a collector
that has quietly stopped is exactly the case where nobody has a reason to. So a
browser push goes out when one opens, through the push channel this codebase
already has: a sender, a store of browser subscriptions, and a policy module
that decides whether a notification may interrupt someone right now.

The sweep is level-triggered — it re-observes the same silent collector on all
1,440 of a day's 60-second ticks — so the push cannot fire from the tick. It
fires on the detection phase's report of what it opened, and the phase reports
only what the query returned, which excludes anything already delivered. That is
what makes one outage produce one notification rather than 1,440 a day.

Delivery is recorded on the record itself, in the `notification_state` field it
already carries. The notifier re-reads that field immediately before dispatch,
which closes the window where two callers act on the same round. Recording it
durably rather than in memory is what makes a restart safe: the run scheduler's
existing deduplication lives in in-memory sets that are lost on every deploy,
which for a condition that can hit every device at once would mean re-notifying
every affected owner each time the server came up.

Two failure shapes are handled differently on purpose. If the attempt throws
before the push reaches the sender — the notifier resolves a connector display
name first, through a projection that can reject transiently — nothing is
recorded, because nothing is known and the owner was certainly not told; the
notice stays selected and a later tick delivers it. If the sender returns having
delivered to nobody, that is an outcome: it is recorded as failed and later ticks
leave it alone. Collapsing the two either drops a notice permanently or retries a
known failure forever.

The reason reuses `needs_attention`, an existing value of a closed two-value
enum, rather than adding a third; a silent collector is precisely "the owner must
act before collection resumes", and widening the enum would force a matching
update at both existing call sites and in the browser service worker that groups
notifications by that value. The notification is classified informational rather
than action-required, because only the informational tier is subject to quiet
hours and a collector quiet for a day is not made worse by waiting.

One check the existing push path applies is skipped deliberately. It reads the
connector's rendered verdict — the summary the console displays, computed from
the records of recent runs — and confirms it is asking the owner for something.
This condition has no run at all; that absence is the entire signal, so there is
no verdict to read and manufacturing one would assert something the summary never
computed. The attention record's own transition is the stronger authority.

Push bodies render on a lock screen before the owner unlocks the phone, so the
body is a fixed string and the connector's display name is the only variable text
in the title.

Verified with the real attention store on both backends. The load-bearing case
simulates seven days of continuous silence across 10,080 ticks and asserts
exactly one push. Also covered: a restart between ticks does not re-send, checked
by discarding every in-process object and rebuilding from the same database; both
failure shapes; a quiet window recording why it suppressed; and a payload for a
device named "tim-laptop" holding 41 pending records containing neither, and no
digits at all.

Assisted-by: AI
Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
…device

Enrolling a collector against a server built from this branch failed with a 500:
`NOT NULL constraint failed: device_exporters.display_name`. Every real consumer
hit it on the first request it makes, so nothing downstream of enrollment worked
at all.

An enrollment code carries no display name unless the owner supplies one, which
the common path does not. The enroll route names the device from the code's
local binding instead, falling back through `display_name || local_binding_id`.
An earlier commit on this branch removed `display_name` and `local_binding_id`
from the projection used by the silence query, where nothing read them and
`display_name` is a device name the lock-screen tests exist to keep out of
notifications. That edit was made as a text substitution and matched more than
it should have: it also stripped `localBindingId` from `mapEnrollment` and
`mapSourceInstance`, which are shared by every reader of those tables. The
fallback then resolved to `undefined` and the device insert violated its
constraint.

Both mappers are restored. The silence projection stays lean, which was the
point of the original change; it simply should not have reached these two. The
enrollment mapper now carries a comment saying the field is load-bearing at
enrollment and what breaks without it, because nothing about the field's name
suggests that a device insert depends on it.

No default was added. The store already accepts what enrollment sends — a code
with a null display name and a binding — and the route already knows how to name
a device from that. Substituting a placeholder in the store would have made the
symptom disappear while leaving devices named something no owner chose.

The regression goes in the store's shared conformance body, so it runs against
both backends rather than only the one that failed. It asserts what the enroll
route actually depends on: a code created without a display name reads back
`displayName: null` and its binding intact. No store test read that field back
before, which is why a projection change could remove it silently.

Verified by running the consumer path this failed in — `pack-install-run.ts`,
which packs the collector, installs it into a clean project, and enrolls it
against a built server. Every enrollment smoke passes and the script exits 0;
before the fix it aborted on the first `enroll`. Removing the restored line
again fails the new conformance test. The full store, route, silence and
attention suites pass: 298 with 6 Postgres-gated skips on SQLite, and 53 with
zero skips against an isolated PostgreSQL 16.15 container, since removed.

Assisted-by: AI
Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
…cisions

Two counterexamples, each the surviving half of a defect an earlier commit on
this branch fixed only partway.

Retries could still starve, one step further out than before. Ordering
never-recorded rows first fixed first attempts: a collector nobody had heard of
could no longer lose its batch to rows already in retry. But once every instance
owns a record that key ties at 1 for all of them, and the remaining tiebreak was
a fixed heartbeat and id order — so the same rows won every batch forever.
Measured on 201 instances at a batch of 200 with no delivery ever recorded: the
tail is reached once by the count key and then never again, even after its own
failure clears and it would send successfully.

The ordering now falls back to the record's `updated_at`, oldest first, which
the upsert refreshes on every attempt. A row that is tried moves to the back, so
the retry share rotates and every retry gets a turn within a bounded number of
ticks. It is a fairness key, not a backoff: nothing is delayed, the order simply
stops being static once the first key ties.

The second is that the lifecycle filter protected the read, not the write. The
query excludes episodes the owner has acted on, but that is evaluated when the
page is selected and the write happens afterwards. An owner resolving a notice
in between had it silently reopened and re-pushed — one maintenance tick and one
owner request, no concurrency required. A check inside the stage before writing
would not fix this either; it would move the gap, not close it, because the
check and the write are still two statements.

`upsertAttention` gains an optional `onlyIfLifecycleIn`, applied as a WHERE on
the `ON CONFLICT ... DO UPDATE` so the comparison happens inside the statement
that writes. The stage passes `["open"]`. An insert of a genuinely new row is
unaffected; only an update of an existing one is guarded. The store returns what
is actually stored rather than the caller's candidate, so a refused write is
visible to the caller instead of being assumed to have happened, and the stage
reports nothing for a record it did not write.

Both counterexamples are now real-store tests. The retry one asserts a tail
instance is selected again after its first attempt, not merely reached once. The
race one forces the interleaving at the roster read — the row is returned, the
owner's real transition runs, then the stage continues to its write — so
selection, transition and write are all the production path.

Also corrected a comment this branch had left stale: the query's header still
claimed a bounded batch "cannot starve anything", which is false in the two ways
above. It now describes what each ORDER BY key is for and names the condition
each one closes.

Verified by removing each fix: without the `updated_at` key the retry test fails;
without the write guard the race test fails. Restored, 317 pass with 6
Postgres-gated skips on SQLite across the silence, store, route, attention,
query-registry and maintenance-sweep suites, and 55 pass with zero skips against
an isolated PostgreSQL 16.15 container, since removed. The guard's PostgreSQL
form uses different placeholder syntax, so it was also exercised directly
against that server: a guarded write over a resolved record leaves it resolved
and returns the stored row.

Assisted-by: AI
Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
… first

A notification still went out for an episode the owner had already resolved.
The stage and the notifier are separate awaited operations, so an ordinary owner
action lands between them with no concurrency involved: one maintenance round
and one request. The notifier read the record before sending, but tested only
whether a delivery had been recorded, so a record it could plainly see was
resolved was still pushed as needing attention.

Reading and then deciding cannot fix this, and neither can widening what the
read checks — the read and the send are two steps, so a resolve arriving between
them is still missed. The notifier now claims the record instead:
`claimNotificationDispatch` marks it as being sent, conditionally on it still
being open and not yet dispatched, in the statement that does the marking. Only
the caller that wins the claim sends. Losing the claim and someone else having
acted are the same event, so there is no window between them.

A resolve landing after a successful claim is a legitimately late notice rather
than a defect: the send was already committed to. Recording the outcome writes
only the notification axis and leaves lifecycle alone, so such a record stays
resolved and is not reopened. A test pins each side of that boundary.

The claim needs a matching release, which the first version of this change
missed and the existing suite caught. A claim marks a record as being sent, so
leaving it set after a send that never began reads as an attempt and costs the
notice its retry — the property that exists so a transient failure before the
sender does not drop a notification permanently. The pre-send failure path now
returns the claim. The release only clears a claim still in `pending` under an
open lifecycle, so it cannot undo a recorded outcome or an owner decision.

Two comments described mechanisms the code does not use, both flagged in review
and both corrected here rather than left to rot. The stage header still claimed
a bounded batch "cannot starve anything" — the sentence an earlier commit
reported as fixed, corrected in the query header but missed in this copy, and the
more misleading of the two because it asserts the safety property the ordering
now provides by other means. The notifier header claimed the stage carries the
notification axis forward on every re-write; it does not, and does not need to,
because the query excludes any episode with a recorded outcome before the stage
reaches it.

Verified by reverting the claim to the previous read-then-check: the
resolve-before-dispatch test fails. Restored, 319 pass with 6 Postgres-gated
skips on SQLite across the silence, store, route, attention, query-registry and
maintenance-sweep suites, and 57 pass with zero skips against an isolated
PostgreSQL 16.15 container, since removed. The claim and release use different
JSON syntax on PostgreSQL, so both were exercised directly against that server:
a claim wins once and is refused on a second attempt and on a resolved record;
a release restores eligibility; and a release cannot undo a recorded outcome.

Assisted-by: AI
Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
…ll be sent

A dispatch claim had no way to end. The notifier marks a record as being sent
before it sends, and only its own caught-error path gives that mark back — so a
process that died between claiming and recording an outcome held the record
forever. The attention record stayed visible in the console, but the push for
that outage was permanently ineligible: nothing else could claim it, and the
query excluded it on the presence of the mark. That is the failure this whole
feature exists to prevent, reachable by an ordinary process restart.

A claim now expires. `claimNotificationDispatch` takes a lease and accepts a
record whose claim is older than it and still unfinished, and the silence query
stops excluding such a record so a later tick can select it again. The lease is
ten sweep intervals, derived from `CONNECTOR_MAINTENANCE_SWEEP_INTERVAL_MS`
rather than chosen separately, so the two cannot drift; that constant moves from
`server/index.ts` to the sweep module it paces, which already referred to it by
name, and `index.ts` imports it back.

Only a claim still `pending` ages out. A recorded outcome — sent, suppressed or
failed — moves the state off `pending`, so it excludes regardless of age and
nothing already accounted for is resent. A test pins that at ten simulated days.

The residual risk is real and is stated where the decision is made rather than
left to be discovered. If a process dies after its push reached the transport
but before recording the outcome, a later tick reclaims the record and sends
again — a duplicate. The alternative is worse in the direction that matters:
without expiry that outage is silenced forever, and a collector nobody is told
about is the whole defect. The window is bounded by the lease.

Separately, CI's reference-implementation test job failed on a hosted runner
with "unexplained skip" for the three PostgreSQL-gated silence tests. The
accounting gate requires every bare-boolean `skip: !POSTGRES_URL` test title to
be registered in an exact mapping, and these were not. All three are added
together because the parser aborts on the first unexplained skip in a run, so a
partial fix fails serially — a note the mapping file already carries from two
earlier instances of this same regression. A guard test pins the three titles,
matching the pattern used for the previous occurrences.

Verified with a separate-process shape: a claim is taken, every in-process
object is discarded, and a stage rebuilt from the same database delivers the
notice once the lease has passed — while a claim inside its lease is not stolen.
Reverting the query's expiry exception fails that test. 321 pass with 6
Postgres-gated skips on SQLite across the silence, store, route, attention,
query-registry and maintenance-sweep suites; 59 pass with zero skips against an
isolated PostgreSQL 16.15 container, since removed. The claim predicate uses
different JSON syntax per backend, so it was also exercised directly against
that server: a claim is exclusive inside its lease, reclaimable past it, and
never reclaimable once an outcome is recorded.

Assisted-by: AI
Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
Sending one push had acquired a job queue. A claim marked a record as being
sent; a release gave it back when the send never began; a lease bounded the
claim; a reclaim recovered it when a process died holding one. Each of those
was added to prop up the one before it, and none of them is needed to notify
somebody that their collector went quiet.

All four are gone. What remains is the shape the feature actually has: a query
that returns unfinished work with never-reported collectors first and retries
rotated by attempt age, a send, and a conditional write of the outcome. The
outcome write touches only the notification axis, so it cannot reopen a record,
and the stage's own write is already conditional on the stored lifecycle, so
neither path can undo a decision the owner made.

Three consequences follow, and they are stated in the stage's header rather than
left for a reader to derive.

A push can still go out within one sweep interval after the owner resolves a
notice. The query excludes what the owner has acted on, but a tick that has
already selected a row will send for it. That is a late notification about a
real outage, not a false one, and the record is not reopened by it.

Nothing is lost. A send that records no outcome leaves the record selectable, so
the next tick retries it. That is also how a duplicate happens: a process dying
between sending and recording will send that notice again. A duplicate
notification is the price of never dropping one, which is the right way round
for a detector whose whole purpose is that a silent collector does not go
unmentioned. The claim machinery bought a narrower duplicate window in exchange
for a failure mode where an abandoned claim silenced an outage permanently — a
worse trade, discovered only because a reviewer went looking for it.

Every test that encodes behaviour a user sees is kept: fleet coverage across
ticks, retry rotation, recovery re-arm after a heartbeat, transient-failure
retry, restart without re-sending, resolved records staying resolved,
never-checked-in and revoked exclusions, lock-screen payload privacy, and the
PostgreSQL coverage for the hand-written twin query. The tests that existed only
to exercise claim, release, lease and reclaim are removed with them. Two new
tests pin the residual directly: a resolved record is not reopened by a late
push and is not selected again, and an outcome that was never recorded is
retried.

Verified: 319 pass with 6 PostgreSQL-gated skips on SQLite across the silence,
store, route, attention, query-registry and maintenance-sweep suites, and 57
pass with zero skips against an isolated PostgreSQL 16.15 container, since
removed. The skip-accounting mapping added last round still holds.

Additions against the base branch fall from 2,431 to 2,138.

Assisted-by: AI
Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
…gres test file

Two gate findings at the previous head, both narrow.

The conditional outcome write was promised and absent. `recordNotificationOutcomeById`
read the record and then replaced its JSON unconditionally, so two guarantees
failed: an outcome already recorded was overwritten by a later one — a delivered
notice downgraded to failed on a subsequent attempt — and an owner decision taken
between the read and the write was lost, reproduced through the real store on
PostgreSQL. The earlier claim that this write "cannot reopen a record" was true
of the lifecycle column and false of everything else it touched.

The predicate now lives in the UPDATE. An opt-in `onlyIfUnresolvedAndUnrecorded`
restricts the write to a record still open and carrying no outcome, and the
method returns what is actually stored when the write is refused. It is opt-in
because the other callers record outcomes for runs whose lifecycle is not open
and legitimately restate an outcome; only the silence notifier needs
first-write-wins. Both backends implement it, and it is exercised directly
against a live PostgreSQL server as well as through the module tests.

One existing test asserted the weaker behaviour — that a late push's outcome
lands on a record the owner resolved. It now asserts the stronger guarantee: the
push still goes out, which is the accepted late-notice residual, but nothing is
written onto a record the owner closed.

Separately, `test/device-silence-postgres.test.ts` was registered in the
skip-accounting mapping but not in the template-eligibility registry, so the
inventory gate failed and the PostgreSQL lane never classified it. It is
template-eligible: it uses PostgreSQL as an ordinary fixture for query
behaviour, not schema bootstrap as the subject under test. Registering it moves
the eligible count from 122 to 123, which the profile report pins, so that
document is updated with it. This is the same omission class as the accounting
mapping — a Postgres test file has to be registered in two places, and the
previous round fixed one of them.

Verified: 324 pass with 6 PostgreSQL-gated skips on SQLite across the silence,
store, route, attention, query-registry, eligibility-inventory and
maintenance-sweep suites, and 59 pass with zero skips against an isolated
PostgreSQL 16.15 container, since removed. The eligibility inventory gate is
3/3, matching the base branch. Removing the notifier's opt-in fails the
owner-decision test.

Assisted-by: AI
Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
@tnunamak
tnunamak force-pushed the feat/collector-silence-detector branch from 5988371 to 54f7b67 Compare September 9, 2026 22:56
… test names

Three narrow gate findings, none of which changes behaviour.

The notifier's header described an immediate pre-dispatch durable read that no
longer exists — it was deleted with the dispatch-claim machinery. It now says
what the module does: nothing is read before sending, because the query has
already excluded every episode with a recorded outcome and every one the owner
has acted on, and the outcome write afterwards is conditional.

The silence query's header still explained how an unfinished claim was reclaimed
by age. That mechanism is gone too, so the paragraph is removed rather than
reworded; the surrounding text already covers what the exclusion does.

The owner-decision test claimed to force the outcome lookup and write apart but
resolved the record inside the sender, which completes before the outcome writer
reads. It proved the guard works, not that it works against the interleaving it
names. The store is now wrapped so the owner's transition lands between the
writer being invoked and its write being attempted, which is the gap a
caller-side check cannot close. Removing the notifier's guard fails this test;
it passed against the previous version either way.

Verified: 254 pass with 6 Postgres-gated skips on SQLite, and 59 pass with zero
skips against an isolated PostgreSQL 16.15 container, since removed.

Assisted-by: AI
Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
@tnunamak
tnunamak merged commit 902b5fe into main Sep 9, 2026
15 checks passed
@tnunamak
tnunamak deleted the feat/collector-silence-detector branch September 9, 2026 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Sponsor
SponsoredKunjungi sekarang
Promo