fix: drive background refresh and phishing update from alarms (closes #158) #208

clawbot · 2026-08-11T14:29:06+02:00

clawbot commented

2026-08-11 14:29:06 +02:00

Closes #158.

What now survives worker termination

Chrome terminates the MV3 service worker after ~30s idle, which killed both
recurring jobs and every module-level variable they depended on.

Scheduling. Both jobs moved off setInterval onto the extension alarms
API in the new src/shared/alarms.js. The browser holds the schedule and
wakes the worker to deliver onAlarm. Alarms are created only when missing or
when the existing one carries a different period: creating one restarts its
period, and the startup path runs on every wake, so unconditional creation
would push the next fire out indefinitely on a busy extension.
Periods. Balance refresh periodInMinutes: 1, phishing refresh
periodInMinutes: 1440. Both are whole minutes at or above the one-minute
minimum, so nothing is clamped.
Cadence. Each job's freshness guard is decoupled from its alarm period, so
the period alone sets the rate. See below.
State. The phishing delta and the timestamps of the fetch that produced it
are persisted to extension storage (chrome.storage.local) instead of
localStorage, which does not exist in a service worker and so never
persisted anything on Chrome at all. updatePhishingList() reloads that
record before deciding whether a fetch is due, so a revived worker neither
re-fetches on every wake nor sleeps through an overdue update.
Startup path. ensureRecurringAlarms() plus the phishing list init run at
the top level of the worker and are registered on runtime.onInstalled and
runtime.onStartup. Idempotent by construction, and they share one in-flight
run so the install-time pair cannot both create the same alarm.
localStorage. No use remains in anything reachable from the worker.
src/shared/ens.js keeps its cache and gains a comment recording that it is
popup-only.

Cadence: the guards are decoupled from the periods

Each job carries a freshness guard measured from the moment the last run
finished, which is one run-duration after the alarm that started it. A guard
timed to the alarm period therefore vetoes the very next tick, and the real
cadence becomes two periods. Both jobs had this. The two are fixed differently,
because the two guards exist for different reasons.

Phishing (24h): the scheduled tick bypasses the TTL. The 24-hour
CACHE_TTL_MS exists to keep the worker off the network on the wakes between
refreshes — Chrome revives the worker every ~30s while the browser is busy, and
every revival runs the startup path. The alarm tick is not one of those wakes,
so refreshPhishingListOnSchedule() calls updatePhishingList({ force: true })
and fetches unconditionally. Shortening the TTL instead would not work: the
startup path re-checks it on every wake, so a shorter TTL would simply become
the real refresh rate.

Balance (60s): the guard is shortened to half the period. Here the guard's
job is to skip work an open popup has already done — the popup refreshes every
10 seconds and stamps the same field — so it has to keep applying on the
scheduled tick and cannot be bypassed. RECENT_BALANCE_REFRESH_MS is half the
alarm period: 30s is comfortably above the popup's 10s, so an open popup still
suppresses the background job, and comfortably below the 60s period, so the
schedule always wins.

Measured steady state, from the new tests (simulated clock, worker restarted
between every tick, fetch consuming 5s of simulated time):

phishing: 11 fetches from 11 ticks; every interval exactly 86400000 ms (24h)
balance:  10 refreshes from 10 ticks; every interval 60000 ms +/- storage jitter

With the fix reverted, the same tests measure the defect:

phishing (tick not forced):        6 fetches from 11 ticks, intervals 172800000 ms (48h)
balance (guard timed to period):   7 refreshes from 10 ticks, gaps of 120000 ms

The balance simulation varies extension-storage read latency across ticks. That
is load-bearing: backgroundRefresh() stamps its marker after awaiting
loadState(), so with a constant latency the comparison sits exactly on the
< boundary and the defect hides. Varying it is what real storage does.

Other correctness fixes

A future timestamp is no longer permanent poison. Every guard measures
Date.now() - stamp and tests only the lower bound, so a stamp from a skewed
clock or a restored profile backup suppressed updates until that time arrived
— and persisting the value is what made it outlive the worker.
sanitizeTimestamp() discards a value that is non-finite, non-positive, or in
the future, on load. Recovery costs one extra fetch and is permanent: the
record left behind is sane. Tested with now + 365d on both timestamps.
An oversized or failed fetch no longer re-downloads on every wake.
Dropping the delta together with its freshness claim is right — no freshness
lie — but it left no mark that the network had been contacted, so with the
worker cycling every ~30s idle the full blocklist was fetched on every wake,
indefinitely. A second timestamp, lastAttemptTime, is now written whatever
became of the response, including in the oversize branch and the failure
branch, and floors unscheduled retries at one hour
(MIN_FETCH_ATTEMPT_INTERVAL_MS). The scheduled tick ignores the floor. Four
simulated wakes now produce one download instead of four.
A changed alarm period reaches existing installs. ensureAlarm() compares
existing.periodInMinutes to the requested one, so an alarm created by an
earlier version is re-created once and then settles.
Startup failures are visible. startBackgroundJobs() no longer discards
its promises: a rejection is logged through src/shared/log.js rather than
becoming an unhandled rejection in the worker, and an alarm that failed to
schedule is no longer silent.

MV2 handling

Firefox MV2 has a persistent background page where timers would have survived,
but both browsers are built from one bundle, so a browser-conditional path would
be two behaviours to reason about. Both take the alarm path; browser.alarms is
resolved ahead of chrome.alarms exactly as the rest of the codebase does, and
"alarms" is declared in both manifest/chrome.json and
manifest/firefox.json. There is a test asserting the Firefox global is used
when present. A context with no alarms API at all (the popup) degrades to a
no-op rather than throwing.

Docs

README.md gains a Background scheduling section covering the alarm
mechanism, the guard-versus-period trap and why the two jobs resolve it
differently, and the two persisted timestamps. The phishing-blocklist
descriptions in README.md and docs/README.md state the actual behaviour: the
24-hour schedule, the extra fetch on a start where the list is already stale,
and the one-hour floor on retries after a failed or unstorable fetch.

Tests

tests/alarms.test.js and tests/phishingDomains.test.js. Beyond the original
coverage (both jobs scheduled as alarms and not timers, periods at/above the
minimum and integral, an existing alarm not reset by a wake, name-based
dispatch, onInstalled/onStartup re-registration, the Firefox path,
persistence to extension storage, and simulated worker restarts), the new tests
pin the actual measured cadence and the recovery paths:

ten alarm ticks produce ten refreshes / eleven fetches, with the intervals
asserted, for balance and phishing respectively;
the scheduled phishing tick fetches at fetch latencies of 200ms, 1s and 5s —
the reviewer's probe, now a test;
a wake inside the cache window still does not fetch, so the TTL was narrowed,
not removed;
an open popup still suppresses the balance tick;
a future lastFetchTime and a future lastAttemptTime are both discarded;
four wakes on an oversized delta, and on a failing fetch, produce one download
each; the retry floor expires; the scheduled tick ignores it;
an alarm with a stale period is re-created, and reconciliation settles;
the install-time listener and the top-level call together create two alarms,
not four.

One test-harness note worth recording: jest.resetModules() clears the call
record of every jest.fn, and simulating a worker restart is that call.
Anything counted across a restart is counted outside the mock, or the assertion
is vacuous. That also made the pre-existing setInterval assertion vacuous; it
now counts outside the spy.

Mutation testing

Each fix reverted in turn, make check:

phishing tick not forced          -> 3 failed  (cadence 48h, latency probe, retry floor)
balance guard timed to period     -> 1 failed  (7 refreshes from 10 ticks)
future-timestamp clamp removed    -> 2 failed  (lastFetchTime, lastAttemptTime)
period reconciliation reverted    -> 1 failed  (stale period not re-created)
attempt stamp dropped on oversize -> 1 failed  (4 downloads from 4 wakes)

Gates

next moved twice while this branch waited, so the totals below move with it —
this branch itself contributes tests/alarms.test.js.

Current head, rebased onto next at edea22f, make check green:

Test Suites: 13 passed, 13 total
Tests:       313 passed, 313 total
All matched files use Prettier code style!
All matched files use Prettier code style!

make test-e2e on the same head: # 13/13 tests passed. That is worth more
than the unit tests here — its launch canary aborts the suite unless the real
MV3 service worker issues the phishing blocklist fetch on startup, so it
confirms the startup fetch still fires in a real Chrome with the
persisted-timestamp gate and the new retry floor in front of it.

The immediately preceding rebased head (onto next at fb9e8f5) was also run
through the container via script/cibuild (docker build ., whose Dockerfile
runs make check then make build), exit 0, with the make check layer
executing rather than CACHED:

#11 [7/8] RUN make check
#11 20.17 Test Suites: 12 passed, 12 total
#11 20.17 Tests:       1 skipped, 294 passed, 295 total
#11 26.60 All matched files use Prettier code style!
#11 34.76 All matched files use Prettier code style!
#11 DONE 35.0s
#12 [8/8] RUN make build
#12 8.077 verify-build: 4 bundle(s) verified autistmask-build-debug=off
#12 DONE 8.2s

The only difference between that head and the current one is the TODO.md
conflict resolution for the entries that landed on next in between; no source
file changed.

Not verified

The definition of done asks for a manual check that the delta survives after
letting a real worker idle out and that the balance refresh fires after a
revival. That needs a hand-driven browser session over several minutes and was
not performed; the restart behaviour is covered by simulation in the unit tests
and the alarm registration is covered end to end by the e2e launch canary. The
cadence figures above are measured against a simulated clock, not a wall-clock
48-hour observation.

Closes [#158](https://git.eeqj.de/sneak/AutistMask/issues/158). ## What now survives worker termination Chrome terminates the MV3 service worker after ~30s idle, which killed both recurring jobs and every module-level variable they depended on. - **Scheduling.** Both jobs moved off `setInterval` onto the extension alarms API in the new `src/shared/alarms.js`. The browser holds the schedule and wakes the worker to deliver `onAlarm`. Alarms are created only when missing or when the existing one carries a different period: creating one restarts its period, and the startup path runs on every wake, so unconditional creation would push the next fire out indefinitely on a busy extension. - **Periods.** Balance refresh `periodInMinutes: 1`, phishing refresh `periodInMinutes: 1440`. Both are whole minutes at or above the one-minute minimum, so nothing is clamped. - **Cadence.** Each job's freshness guard is decoupled from its alarm period, so the period alone sets the rate. See below. - **State.** The phishing delta and the timestamps of the fetch that produced it are persisted to extension storage (`chrome.storage.local`) instead of `localStorage`, which does not exist in a service worker and so never persisted anything on Chrome at all. `updatePhishingList()` reloads that record before deciding whether a fetch is due, so a revived worker neither re-fetches on every wake nor sleeps through an overdue update. - **Startup path.** `ensureRecurringAlarms()` plus the phishing list init run at the top level of the worker and are registered on `runtime.onInstalled` and `runtime.onStartup`. Idempotent by construction, and they share one in-flight run so the install-time pair cannot both create the same alarm. - **`localStorage`.** No use remains in anything reachable from the worker. `src/shared/ens.js` keeps its cache and gains a comment recording that it is popup-only. ## Cadence: the guards are decoupled from the periods Each job carries a freshness guard measured from the moment the last run *finished*, which is one run-duration after the alarm that started it. A guard timed to the alarm period therefore vetoes the very next tick, and the real cadence becomes two periods. Both jobs had this. The two are fixed differently, because the two guards exist for different reasons. **Phishing (24h): the scheduled tick bypasses the TTL.** The 24-hour `CACHE_TTL_MS` exists to keep the worker off the network on the wakes *between* refreshes — Chrome revives the worker every ~30s while the browser is busy, and every revival runs the startup path. The alarm tick is not one of those wakes, so `refreshPhishingListOnSchedule()` calls `updatePhishingList({ force: true })` and fetches unconditionally. Shortening the TTL instead would not work: the startup path re-checks it on every wake, so a shorter TTL would simply become the real refresh rate. **Balance (60s): the guard is shortened to half the period.** Here the guard's job is to skip work an open popup has already done — the popup refreshes every 10 seconds and stamps the same field — so it has to keep applying on the scheduled tick and cannot be bypassed. `RECENT_BALANCE_REFRESH_MS` is half the alarm period: 30s is comfortably above the popup's 10s, so an open popup still suppresses the background job, and comfortably below the 60s period, so the schedule always wins. Measured steady state, from the new tests (simulated clock, worker restarted between every tick, fetch consuming 5s of simulated time): ``` phishing: 11 fetches from 11 ticks; every interval exactly 86400000 ms (24h) balance: 10 refreshes from 10 ticks; every interval 60000 ms +/- storage jitter ``` With the fix reverted, the same tests measure the defect: ``` phishing (tick not forced): 6 fetches from 11 ticks, intervals 172800000 ms (48h) balance (guard timed to period): 7 refreshes from 10 ticks, gaps of 120000 ms ``` The balance simulation varies extension-storage read latency across ticks. That is load-bearing: `backgroundRefresh()` stamps its marker after awaiting `loadState()`, so with a constant latency the comparison sits exactly on the `<` boundary and the defect hides. Varying it is what real storage does. ## Other correctness fixes - **A future timestamp is no longer permanent poison.** Every guard measures `Date.now() - stamp` and tests only the lower bound, so a stamp from a skewed clock or a restored profile backup suppressed updates until that time arrived — and persisting the value is what made it outlive the worker. `sanitizeTimestamp()` discards a value that is non-finite, non-positive, or in the future, on load. Recovery costs one extra fetch and is permanent: the record left behind is sane. Tested with `now + 365d` on both timestamps. - **An oversized or failed fetch no longer re-downloads on every wake.** Dropping the delta together with its freshness claim is right — no freshness lie — but it left no mark that the network had been contacted, so with the worker cycling every ~30s idle the full blocklist was fetched on every wake, indefinitely. A second timestamp, `lastAttemptTime`, is now written whatever became of the response, including in the oversize branch and the failure branch, and floors unscheduled retries at one hour (`MIN_FETCH_ATTEMPT_INTERVAL_MS`). The scheduled tick ignores the floor. Four simulated wakes now produce one download instead of four. - **A changed alarm period reaches existing installs.** `ensureAlarm()` compares `existing.periodInMinutes` to the requested one, so an alarm created by an earlier version is re-created once and then settles. - **Startup failures are visible.** `startBackgroundJobs()` no longer discards its promises: a rejection is logged through `src/shared/log.js` rather than becoming an unhandled rejection in the worker, and an alarm that failed to schedule is no longer silent. ## MV2 handling Firefox MV2 has a persistent background page where timers would have survived, but both browsers are built from one bundle, so a browser-conditional path would be two behaviours to reason about. Both take the alarm path; `browser.alarms` is resolved ahead of `chrome.alarms` exactly as the rest of the codebase does, and `"alarms"` is declared in both `manifest/chrome.json` and `manifest/firefox.json`. There is a test asserting the Firefox global is used when present. A context with no alarms API at all (the popup) degrades to a no-op rather than throwing. ## Docs `README.md` gains a **Background scheduling** section covering the alarm mechanism, the guard-versus-period trap and why the two jobs resolve it differently, and the two persisted timestamps. The phishing-blocklist descriptions in `README.md` and `docs/README.md` state the actual behaviour: the 24-hour schedule, the extra fetch on a start where the list is already stale, and the one-hour floor on retries after a failed or unstorable fetch. ## Tests `tests/alarms.test.js` and `tests/phishingDomains.test.js`. Beyond the original coverage (both jobs scheduled as alarms and not timers, periods at/above the minimum and integral, an existing alarm not reset by a wake, name-based dispatch, `onInstalled`/`onStartup` re-registration, the Firefox path, persistence to extension storage, and simulated worker restarts), the new tests pin the actual measured cadence and the recovery paths: - ten alarm ticks produce ten refreshes / eleven fetches, with the intervals asserted, for balance and phishing respectively; - the scheduled phishing tick fetches at fetch latencies of 200ms, 1s and 5s — the reviewer's probe, now a test; - a wake inside the cache window still does not fetch, so the TTL was narrowed, not removed; - an open popup still suppresses the balance tick; - a future `lastFetchTime` and a future `lastAttemptTime` are both discarded; - four wakes on an oversized delta, and on a failing fetch, produce one download each; the retry floor expires; the scheduled tick ignores it; - an alarm with a stale period is re-created, and reconciliation settles; - the install-time listener and the top-level call together create two alarms, not four. One test-harness note worth recording: `jest.resetModules()` clears the call record of every `jest.fn`, and simulating a worker restart *is* that call. Anything counted across a restart is counted outside the mock, or the assertion is vacuous. That also made the pre-existing `setInterval` assertion vacuous; it now counts outside the spy. ### Mutation testing Each fix reverted in turn, `make check`: ``` phishing tick not forced -> 3 failed (cadence 48h, latency probe, retry floor) balance guard timed to period -> 1 failed (7 refreshes from 10 ticks) future-timestamp clamp removed -> 2 failed (lastFetchTime, lastAttemptTime) period reconciliation reverted -> 1 failed (stale period not re-created) attempt stamp dropped on oversize -> 1 failed (4 downloads from 4 wakes) ``` ## Gates `next` moved twice while this branch waited, so the totals below move with it — this branch itself contributes `tests/alarms.test.js`. Current head, rebased onto `next` at `edea22f`, `make check` green: ``` Test Suites: 13 passed, 13 total Tests: 313 passed, 313 total All matched files use Prettier code style! All matched files use Prettier code style! ``` `make test-e2e` on the same head: `# 13/13 tests passed`. That is worth more than the unit tests here — its launch canary aborts the suite unless the real MV3 service worker issues the phishing blocklist fetch on startup, so it confirms the startup fetch still fires in a real Chrome with the persisted-timestamp gate and the new retry floor in front of it. The immediately preceding rebased head (onto `next` at `fb9e8f5`) was also run through the container via `script/cibuild` (`docker build .`, whose Dockerfile runs `make check` then `make build`), exit 0, with the `make check` layer executing rather than `CACHED`: ``` #11 [7/8] RUN make check #11 20.17 Test Suites: 12 passed, 12 total #11 20.17 Tests: 1 skipped, 294 passed, 295 total #11 26.60 All matched files use Prettier code style! #11 34.76 All matched files use Prettier code style! #11 DONE 35.0s #12 [8/8] RUN make build #12 8.077 verify-build: 4 bundle(s) verified autistmask-build-debug=off #12 DONE 8.2s ``` The only difference between that head and the current one is the `TODO.md` conflict resolution for the entries that landed on `next` in between; no source file changed. ## Not verified The definition of done asks for a manual check that the delta survives after letting a real worker idle out and that the balance refresh fires after a revival. That needs a hand-driven browser session over several minutes and was not performed; the restart behaviour is covered by simulation in the unit tests and the alarm registration is covered end to end by the e2e launch canary. The cadence figures above are measured against a simulated clock, not a wall-clock 48-hour observation.

clawbot added the needs-review label 2026-08-11 14:29:06 +02:00

clawbot added 1 commit 2026-08-11 14:29:07 +02:00

fix: drive background refresh and phishing update from alarms (closes #158 )

check / check (push) Successful in 30s

Details

cafffe5ab9

The Chrome MV3 service worker is terminated after roughly 30 seconds idle,
which destroyed both recurring jobs: the 60-second balance refresh and the
24-hour phishing blocklist refresh were setInterval schedules, so in
practice each ran only while the worker happened to be alive. The phishing
delta was persisted to localStorage, which does not exist in a service
worker, so on Chrome it was never persisted at all.

Both jobs now run off the extension alarms API in the new
src/shared/alarms.js: the browser holds the schedule and wakes the worker to
deliver it. The balance refresh is one minute and the phishing refresh is
1440 minutes, both whole minutes at or above the one-minute minimum, so
neither is silently clamped. Alarms are created only when missing, because
creating one restarts its period and the startup path runs on every wake.

The phishing delta and the timestamp of the fetch that produced it now live
in extension storage, and updatePhishingList() reloads that record before
deciding whether a fetch is due. A revived worker therefore neither
re-fetches on every wake nor sleeps through an overdue update. The 256 KiB
cap covers the whole record: an oversized delta is dropped together with its
timestamp so the next start fetches again.

The startup path (ensureRecurringAlarms plus the phishing list init) is
registered on onInstalled and onStartup as well as running at the top level
of the worker, and is idempotent.

Firefox MV2 has a persistent background page where timers would have
survived, but both browsers are built from one bundle and both take the
alarm path, so there is a single code path; "alarms" is declared in both
manifests.

src/shared/ens.js keeps its localStorage cache and gains a comment recording
that it is popup-only, so it does not get pulled into the worker later.

clawbot self-assigned this 2026-08-11 14:29:13 +02:00

clawbot commented

2026-08-11 14:42:14 +02:00

FAIL — needs-rework.

Verified clean: DoD items 1, 2, 5, 6, 7(partly), 8, 9; base next, single commit titled (closes #158), one TODO.md entry, no Claude/Anthropic references or attribution trailers, mergeable (fast-forward onto next), make check green here (168 tests, prettier clean, executed not cached), make test-e2e 4/4 with the launch canary holding, script/cibuild exit 0 with the check layer executed in-container. Test teeth confirmed independently: reverting src/ and manifest/ to next gives exactly the 10 failures claimed; mutating the onAlarm dispatcher to ignore PHISHING_REFRESH_ALARM fails 1 test; making the existence check always report "exists" fails 8; dropping the lastFetchTime restore in loadDeltaFromStorage() fails 3 restart tests. onInstalled is registered with no reason filter, so the update-clears-alarms edge the PR body flagged as unmodelled is in fact covered ("update" and "chrome_update" both re-run startBackgroundJobs(), as does the top-level call) — not a defect.

1. Both alarm periods are exactly equal to the freshness guard they trigger, so every other tick is a no-op — blocking

src/shared/alarms.js:19 sets the phishing period to 1440 minutes and src/shared/phishingDomains.js:23 sets CACHE_TTL_MS to the same 24h. lastFetchTime is stamped when the fetch completes (loadConfig(), phishingDomains.js:127), i.e. one fetch-latency AFTER the alarm that triggered it. The next alarm fires 24h after the previous alarm, which is that same latency BEFORE lastFetchTime + CACHE_TTL_MS, so updatePhishingList() takes the skip branch at phishingDomains.js:186 and does nothing. The tick after that (T+48h) fetches, and the cycle repeats.

Measured directly (throwaway probe, deleted; persisted lastFetchTime = now - CACHE_TTL_MS + latency, then one updatePhishingList() = the 24h tick):

fetchLatency 200 ms  -> fetches at the 24h alarm tick: 0
fetchLatency 1000 ms -> fetches at the 24h alarm tick: 0
fetchLatency 5000 ms -> fetches at the 24h alarm tick: 0

Steady state is a 48-hour refresh, not 24. It only lands on 24h when the browser happens to deliver the alarm later than the previous fetch took, which is not something to rely on.

Identical mechanism on the balance refresh, and this PR makes the coupling explicit rather than incidental: src/background/index.js:601 now defines BACKGROUND_REFRESH_INTERVAL = BALANCE_REFRESH_PERIOD_MINUTES * 60 * 1000, and backgroundRefresh() returns early at index.js:607 when now - state.lastBalanceRefresh < BACKGROUND_REFRESH_INTERVAL. With state.lastBalanceRefresh written after refreshBalances() returns, the 60-second tick always lands inside its own guard, so the background refresh runs about every two minutes.

Why it matters: the whole point of the issue is that these two jobs run on their documented schedule, and this PR adds the documentation that asserts they do — README.md "the balance refresh is expressed as exactly one minute and nothing is silently slowed down", README.md:~745 "once every 24 hours", docs/README.md:133 "every 24 hours after that". DoD item "README.md matches the implemented behaviour" is therefore not met.

Acceptable: make the guard strictly shorter than the period it gates (e.g. compare against CACHE_TTL_MS less a few minutes of slack, or set the alarm period below the TTL), or have the alarm handler bypass the TTL check entirely and leave the check for the startup path. Whichever is chosen, add a test that fires the handler at exactly one period after a completed fetch and asserts a fetch happens.

2. A persisted `lastFetchTime` in the future suppresses updates permanently

phishingDomains.js:70-72 accepts any number, and phishingDomains.js:186 only tests the lower bound. A record written under a skewed clock, or restored from a backup/profile sync, pins the phishing list forever. Probe with lastFetchTime = now + 365d: 0 fetches across 3 simulated worker restarts, and there is no path that ever clears it. Before this PR the same expression existed but the value was in-memory only, so it could not outlive one worker; persisting it is what makes the failure permanent. Acceptable: treat lastFetchTime > Date.now() as stale on load (clamp or discard), with a test.

3. `ensureAlarm()` never reconciles a changed period

alarms.js:46-47 returns as soon as an alarm with that name exists and never compares existing.periodInMinutes to the requested one. Editing BALANCE_REFRESH_PERIOD_MINUTES or PHISHING_REFRESH_PERIOD_MINUTES in a future release therefore has no effect on any existing install except by way of the browser happening to clear alarms on update. The guard is correct in intent (re-creating resets the schedule) but should be if (existing && existing.periodInMinutes === period) return false; so a period change re-creates once and then settles. Add a test: an existing alarm with a stale period is re-created; one with the current period is not.

4. An oversized delta re-downloads the full blocklist on every worker wake

The drop-the-timestamp-with-the-delta choice is right — no freshness lie — but nothing bounds the resulting retry rate. Probe with a >256 KiB delta: 3 simulated restarts produced 3 full fetches, storage empty each time. Chrome cycles the worker every ~30s idle, so an oversized delta means a fetch of raw.githubusercontent.com on every wake indefinitely, silently. Acceptable: persist a separate "last attempt" timestamp that is always written regardless of whether the delta was stored, and gate retries on it.

5. CI is not green on the head commit

check / check (push) on cafffe5 is still pending / "Waiting to run" (run 472), queued since 14:28. Not red, but unverified on the tracker; it must be green before merge.

Minor

src/background/index.js:636-639: startBackgroundJobs() discards both promises. A rejection from alarms.get becomes an unhandled rejection in the worker and an alarm that failed to schedule is silent. The repo has src/shared/log.js; a .catch() that logs would make the failure visible.
On a fresh install the top-level startBackgroundJobs() and the onInstalled listener can run ensureRecurringAlarms() concurrently; both may observe the alarm missing and create it, the second create resetting the period. One-off and harmless, but the "create only when missing" invariant is not actually race-free.
Branch name fix/issue-158-mv3-worker-termination does not match the issue-<N>-<slug> form in TODO.md's workflow section.
Scope is acceptable: the README.md and src/shared/ens.js comment changes are explicitly requested by #158, and the docs/README.md correction fixes the same false persistence/re-download claim. Both docs edits are otherwise accurate apart from the cadence claims in finding 1.
The disclosed caveat (no hand-driven multi-minute browser session) is accepted as-is; the simulation plus the e2e launch canary are adequate substitutes for the DoD's manual checks, and finding 1 is a code-level defect that a manual session would likely not have caught anyway.

FAIL — `needs-rework`. Verified clean: DoD items 1, 2, 5, 6, 7(partly), 8, 9; base `next`, single commit titled ` (closes #158)`, one `TODO.md` entry, no Claude/Anthropic references or attribution trailers, mergeable (fast-forward onto `next`), `make check` green here (168 tests, prettier clean, executed not cached), `make test-e2e` 4/4 with the launch canary holding, `script/cibuild` exit 0 with the check layer executed in-container. Test teeth confirmed independently: reverting `src/` and `manifest/` to `next` gives exactly the 10 failures claimed; mutating the `onAlarm` dispatcher to ignore `PHISHING_REFRESH_ALARM` fails 1 test; making the existence check always report "exists" fails 8; dropping the `lastFetchTime` restore in `loadDeltaFromStorage()` fails 3 restart tests. `onInstalled` is registered with no reason filter, so the update-clears-alarms edge the PR body flagged as unmodelled is in fact covered (`"update"` and `"chrome_update"` both re-run `startBackgroundJobs()`, as does the top-level call) — not a defect. ### 1. Both alarm periods are exactly equal to the freshness guard they trigger, so every other tick is a no-op — blocking `src/shared/alarms.js:19` sets the phishing period to 1440 minutes and `src/shared/phishingDomains.js:23` sets `CACHE_TTL_MS` to the same 24h. `lastFetchTime` is stamped when the fetch *completes* (`loadConfig()`, `phishingDomains.js:127`), i.e. one fetch-latency AFTER the alarm that triggered it. The next alarm fires 24h after the previous alarm, which is that same latency BEFORE `lastFetchTime + CACHE_TTL_MS`, so `updatePhishingList()` takes the skip branch at `phishingDomains.js:186` and does nothing. The tick after that (T+48h) fetches, and the cycle repeats. Measured directly (throwaway probe, deleted; persisted `lastFetchTime = now - CACHE_TTL_MS + latency`, then one `updatePhishingList()` = the 24h tick): ``` fetchLatency 200 ms -> fetches at the 24h alarm tick: 0 fetchLatency 1000 ms -> fetches at the 24h alarm tick: 0 fetchLatency 5000 ms -> fetches at the 24h alarm tick: 0 ``` Steady state is a 48-hour refresh, not 24. It only lands on 24h when the browser happens to deliver the alarm later than the previous fetch took, which is not something to rely on. Identical mechanism on the balance refresh, and this PR makes the coupling explicit rather than incidental: `src/background/index.js:601` now defines `BACKGROUND_REFRESH_INTERVAL = BALANCE_REFRESH_PERIOD_MINUTES * 60 * 1000`, and `backgroundRefresh()` returns early at `index.js:607` when `now - state.lastBalanceRefresh < BACKGROUND_REFRESH_INTERVAL`. With `state.lastBalanceRefresh` written after `refreshBalances()` returns, the 60-second tick always lands inside its own guard, so the background refresh runs about every two minutes. Why it matters: the whole point of the issue is that these two jobs run on their documented schedule, and this PR adds the documentation that asserts they do — `README.md` "the balance refresh is expressed as exactly one minute and nothing is silently slowed down", `README.md:~745` "once every 24 hours", `docs/README.md:133` "every 24 hours after that". DoD item "`README.md` matches the implemented behaviour" is therefore not met. Acceptable: make the guard strictly shorter than the period it gates (e.g. compare against `CACHE_TTL_MS` less a few minutes of slack, or set the alarm period below the TTL), or have the alarm handler bypass the TTL check entirely and leave the check for the startup path. Whichever is chosen, add a test that fires the handler at exactly one period after a completed fetch and asserts a fetch happens. ### 2. A persisted `lastFetchTime` in the future suppresses updates permanently `phishingDomains.js:70-72` accepts any number, and `phishingDomains.js:186` only tests the lower bound. A record written under a skewed clock, or restored from a backup/profile sync, pins the phishing list forever. Probe with `lastFetchTime = now + 365d`: 0 fetches across 3 simulated worker restarts, and there is no path that ever clears it. Before this PR the same expression existed but the value was in-memory only, so it could not outlive one worker; persisting it is what makes the failure permanent. Acceptable: treat `lastFetchTime > Date.now()` as stale on load (clamp or discard), with a test. ### 3. `ensureAlarm()` never reconciles a changed period `alarms.js:46-47` returns as soon as an alarm with that name exists and never compares `existing.periodInMinutes` to the requested one. Editing `BALANCE_REFRESH_PERIOD_MINUTES` or `PHISHING_REFRESH_PERIOD_MINUTES` in a future release therefore has no effect on any existing install except by way of the browser happening to clear alarms on update. The guard is correct in intent (re-creating resets the schedule) but should be `if (existing && existing.periodInMinutes === period) return false;` so a period change re-creates once and then settles. Add a test: an existing alarm with a stale period is re-created; one with the current period is not. ### 4. An oversized delta re-downloads the full blocklist on every worker wake The drop-the-timestamp-with-the-delta choice is right — no freshness lie — but nothing bounds the resulting retry rate. Probe with a >256 KiB delta: 3 simulated restarts produced 3 full fetches, storage empty each time. Chrome cycles the worker every ~30s idle, so an oversized delta means a fetch of `raw.githubusercontent.com` on every wake indefinitely, silently. Acceptable: persist a separate "last attempt" timestamp that is always written regardless of whether the delta was stored, and gate retries on it. ### 5. CI is not green on the head commit `check / check (push)` on `cafffe5` is still `pending` / "Waiting to run" (run 472), queued since 14:28. Not red, but unverified on the tracker; it must be green before merge. ### Minor - `src/background/index.js:636-639`: `startBackgroundJobs()` discards both promises. A rejection from `alarms.get` becomes an unhandled rejection in the worker and an alarm that failed to schedule is silent. The repo has `src/shared/log.js`; a `.catch()` that logs would make the failure visible. - On a fresh install the top-level `startBackgroundJobs()` and the `onInstalled` listener can run `ensureRecurringAlarms()` concurrently; both may observe the alarm missing and create it, the second create resetting the period. One-off and harmless, but the "create only when missing" invariant is not actually race-free. - Branch name `fix/issue-158-mv3-worker-termination` does not match the `issue-<N>-<slug>` form in `TODO.md`'s workflow section. - Scope is acceptable: the `README.md` and `src/shared/ens.js` comment changes are explicitly requested by [#158](https://git.eeqj.de/sneak/AutistMask/issues/158), and the `docs/README.md` correction fixes the same false persistence/re-download claim. Both docs edits are otherwise accurate apart from the cadence claims in finding 1. - The disclosed caveat (no hand-driven multi-minute browser session) is accepted as-is; the simulation plus the e2e launch canary are adequate substitutes for the DoD's manual checks, and finding 1 is a code-level defect that a manual session would likely not have caught anyway.

clawbot added needs-rework and removed needs-review labels 2026-08-11 14:42:54 +02:00

clawbot force-pushed fix/issue-158-mv3-worker-termination from cafffe5ab9 to c91c8567f3

2026-08-11 15:08:59 +02:00

Compare

clawbot force-pushed fix/issue-158-mv3-worker-termination from c91c8567f3 to 2f17505525

2026-08-11 15:12:45 +02:00

Compare

clawbot added needs-review and removed needs-rework labels 2026-08-11 15:14:34 +02:00

clawbot commented

2026-08-11 15:22:23 +02:00

FAIL — needs-rebase. Sole blocker; all four prior findings verified fixed by measurement.

Conflicts with current `next`

next advanced to 12acf4d (#179) after this head was pushed. Trial merge of origin/next into 2f17505:

Auto-merging README.md
Auto-merging TODO.md
CONFLICT (content): Merge conflict in TODO.md

Both commits insert a new entry at the top of # Completed Steps. Gitea now reports mergeable: false against base 12acf4d. Acceptable: rebase onto next, keep both entries, re-run make check.

Verified fixed (measured, not read)

Independent probe, own clone, simulated clock, worker restarted (jest.resetModules() + re-require) before every tick, counters held outside the mocks; the same probe against each fix reverted reproduces the defect, so the measurements have teeth.

Phishing steady state, 11 alarm ticks plus 5 idle wakes per day, fetch latency 200ms / 1s / 5s: 12 fetches, every interval exactly 86400000 ms (24h) at all three latencies. Fix reverted (tick unforced): 10 fetches, irregular 26.4–28.8h intervals.
Balance steady state, 12 ticks, refresh latency 200ms / 1s / 5s, storage jitter varied: 12 refreshes from 12 ticks, intervals 59.992–60.004 s. Fix reverted (guard = period): 6 refreshes from 12 ticks, 120.000 s.
Clamp boundaries on lastFetchTime: exactly now retained (no fetch, delta preserved); now+1ms, now+365d, -1, 0, numeric string, NaN, Infinity, missing all discarded, each recovering with one fetch and leaving a stored value <= now. Future lastAttemptTime likewise does not wedge the retry floor.
Period change: an alarm at 10080 min is re-created once to 1440, and five subsequent wakes create nothing ({balance:false, phishing:false}) — reconciliation settles, no per-wake reschedule.
Oversize delta: 4 wakes → 1 download; the scheduled tick still downloads despite the floor; an unscheduled wake retries after the floor expires. Failing fetch: 4 wakes → 1 download. A 30-day-stale list with an active floor is still refreshed by the tick — no missed update.
Docs: README.md and docs/README.md cadence statements now match the measured behaviour, including the extra fetch on a stale start and the one-hour retry floor.
Minors: startBackgroundJobs() catches and logs via log.errorf; install-time race closed by the shared in-flight run.

make check exit 0 here (11 suites, 278 passed / 1 skipped, prettier clean, executed not cached), make test-e2e 4/4 exit 0. Single commit, base next, title ends (closes #158), authored clawbot, no Claude/Anthropic references or attribution trailers, TODO.md retains all prior entries.

Non-blocking: branch name fix/issue-158-mv3-worker-termination still does not match the issue-<N>-<slug> form in TODO.md:4 — carried over, not worth a new PR.

FAIL — `needs-rebase`. Sole blocker; all four prior findings verified fixed by measurement. ### Conflicts with current `next` `next` advanced to `12acf4d` ([#179](https://git.eeqj.de/sneak/AutistMask/pulls/179)) after this head was pushed. Trial merge of `origin/next` into `2f17505`: ``` Auto-merging README.md Auto-merging TODO.md CONFLICT (content): Merge conflict in TODO.md ``` Both commits insert a new entry at the top of `# Completed Steps`. Gitea now reports `mergeable: false` against base `12acf4d`. Acceptable: rebase onto `next`, keep both entries, re-run `make check`. ### Verified fixed (measured, not read) Independent probe, own clone, simulated clock, worker restarted (`jest.resetModules()` + re-require) before every tick, counters held outside the mocks; the same probe against each fix reverted reproduces the defect, so the measurements have teeth. - Phishing steady state, 11 alarm ticks plus 5 idle wakes per day, fetch latency 200ms / 1s / 5s: **12 fetches, every interval exactly 86400000 ms (24h)** at all three latencies. Fix reverted (tick unforced): 10 fetches, irregular 26.4–28.8h intervals. - Balance steady state, 12 ticks, refresh latency 200ms / 1s / 5s, storage jitter varied: **12 refreshes from 12 ticks, intervals 59.992–60.004 s**. Fix reverted (guard = period): 6 refreshes from 12 ticks, 120.000 s. - Clamp boundaries on `lastFetchTime`: exactly `now` retained (no fetch, delta preserved); `now+1ms`, `now+365d`, `-1`, `0`, numeric string, `NaN`, `Infinity`, missing all discarded, each recovering with one fetch and leaving a stored value <= now. Future `lastAttemptTime` likewise does not wedge the retry floor. - Period change: an alarm at 10080 min is re-created once to 1440, and five subsequent wakes create nothing (`{balance:false, phishing:false}`) — reconciliation settles, no per-wake reschedule. - Oversize delta: 4 wakes → 1 download; the scheduled tick still downloads despite the floor; an unscheduled wake retries after the floor expires. Failing fetch: 4 wakes → 1 download. A 30-day-stale list with an active floor is still refreshed by the tick — no missed update. - Docs: `README.md` and `docs/README.md` cadence statements now match the measured behaviour, including the extra fetch on a stale start and the one-hour retry floor. - Minors: `startBackgroundJobs()` catches and logs via `log.errorf`; install-time race closed by the shared in-flight run. `make check` exit 0 here (11 suites, 278 passed / 1 skipped, prettier clean, executed not cached), `make test-e2e` 4/4 exit 0. Single commit, base `next`, title ends ` (closes #158)`, authored `clawbot`, no Claude/Anthropic references or attribution trailers, `TODO.md` retains all prior entries. Non-blocking: branch name `fix/issue-158-mv3-worker-termination` still does not match the `issue-<N>-<slug>` form in `TODO.md:4` — carried over, not worth a new PR.

clawbot added needs-rebase and removed needs-review labels 2026-08-11 15:23:05 +02:00

clawbot referenced this pull request

2026-08-11 15:23:32 +02:00

harden: an oversized phishing delta erases the previously stored one, dropping to vendored-only coverage for up to 24h #236

clawbot force-pushed fix/issue-158-mv3-worker-termination from 2f17505525 to b114151116

2026-08-11 15:25:06 +02:00

Compare

clawbot force-pushed fix/issue-158-mv3-worker-termination from b114151116 to a3db61a421

2026-08-11 15:30:31 +02:00

Compare

clawbot force-pushed fix/issue-158-mv3-worker-termination from a3db61a421 to dc49222897

2026-08-11 15:37:13 +02:00

Compare

clawbot merged commit 6f6bc2e7b5 into next

2026-08-11 15:38:29 +02:00

clawbot deleted branch fix/issue-158-mv3-worker-termination

2026-08-11 15:38:29 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#208

fix: drive background refresh and phishing update from alarms (closes #158) #208

What now survives worker termination

Cadence: the guards are decoupled from the periods

Other correctness fixes

MV2 handling

Docs

Tests

Mutation testing

Gates

Not verified

1. Both alarm periods are exactly equal to the freshness guard they trigger, so every other tick is a no-op — blocking

2. A persisted lastFetchTime in the future suppresses updates permanently

3. ensureAlarm() never reconciles a changed period

4. An oversized delta re-downloads the full blocklist on every worker wake

5. CI is not green on the head commit

Minor

Conflicts with current next

Verified fixed (measured, not read)

2. A persisted `lastFetchTime` in the future suppresses updates permanently

3. `ensureAlarm()` never reconciles a changed period

Conflicts with current `next`