Repository navigation
feat: tell the owner when a local collector goes silent - #74
Merged
Merged
Conversation
tnunamak
force-pushed
the
fix/local-device-stale-heartbeat-health
branch
from
September 9, 2026 14:55
5744a04 to
b232dcf
Compare
tnunamak
force-pushed
the
feat/collector-silence-detector
branch
from
September 9, 2026 15:16
8a17506 to
12f2024
Compare
tnunamak
force-pushed
the
feat/collector-silence-detector
branch
from
September 9, 2026 17:30
a6bc572 to
ac401ba
Compare
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
force-pushed
the
feat/collector-silence-detector
branch
from
September 9, 2026 22:56
5988371 to
54f7b67
Compare
… 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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 whenPDPP_TEST_POSTGRES_URLis 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