A review found that an Open-Meteo response with no wind_direction_10m key still defaulted the direction to 0°, so the reply read ⬆️N; a null direction was already left out. Both now give the wind speed with no direction.
_open_meteo_current defaulted every missing current field to 0, so a response without current conditions read as clear skies, 0° and 0% humidity, and called int() on present-but-null fields, which failed the whole reply. A missing current temperature now returns None (the caller replies with error_fetching and logs a warning); null or absent humidity, wind, gusts, feels-like and weather code are each left out. The no_current and null_temp distortion goldens move accordingly; nothing else does.
A codex review of this port found solarforecast relying on the enabled_attr hook, which dev's BaseCommand does not have, so [Solarforecast_Command] enabled = false stopped working; its can_execute override is back as on dev. It also found names an earlier refactor commit had dropped as unused, which local plugins could still read: WxCommand.forecast_duration, num_wx_alerts, use_metric, zulu_time and geolocator, geolocator on gwx and solarforecast, the WXSIM_PARSER_AVAILABLE and WX_INTERNATIONAL_AVAILABLE flags, and the _mqtt_weather_error_key hook (now a WeatherCommandMixin method that _mqtt_weather_line calls through self, so an override takes effect again). tests/unit/test_weather_compat_surface.py pins all of them.
This brings the weather slices of my long-running refactor branch to dev ahead of converging wx and gwx, so the convergence PRs build on one copy of the code instead of rewriting it twice.
- `modules/weather_common.py` (WeatherCommandMixin): code wx and gwx shared by copy (MQTT and WXSIM source lookup, multi-day sending, high/low formatting, the forecast-option parser, the Open-Meteo model loader).
- `modules/nws_alerts.py`: one NWS alert entry parser for wx and the weather service, keeping each caller's own Special Weather Statement rules.
- `modules/http_retry.py`: one retrying session builder for the weather HTTP clients.
- wx: get_noaa_weather, the hourly, tomorrow and multi-day formatters, _add_period_details and the extract_* readers broken up or made table-driven; NOAA fetches go through `_noaa_fetch`.
- gwx: one fetch path; `_open_meteo_current` and `_open_meteo_daily_tail` split out of get_open_meteo_weather.
- solarforecast: `_format_forecast` built from one timestamp parser and one day builder (418 → 155 lines).
- aurora, rain, wx, gwx: companion and bot location lookups through `modules.location` (which gains optional logging arguments).
The changes outside these files that the refactor branch made (the base command's enabled_attr hook, the service base's run_periodic and _cancel_tasks) stay out: aurora and rain keep their can_execute overrides and the weather service its own loops, exactly as on dev.
Tests: characterization goldens over real captured NOAA, NWS, Open-Meteo and forecast.solar responses (tests/characterization, fixtures in tests/fixtures), 50 tests and several hundred cases, all of which also pass against dev's unmodified weather code. Each refactor step was additionally checked on the branch with an old-vs-new differential run (tens of thousands of generated inputs, including log calls and exceptions) and reviewed by codex.
Open-Meteo always returns the requested number of days but fills the ones past the model's horizon with nulls: the last one or two with the default model, nine of sixteen with icon_seamless. format_multiday_forecast called int() on them, so the whole reply became the multi-day error (gwx 15d for London and Reykjavik on 2026-10-01; gwx 7d under icon_seamless). It now stops at the first day without a high and low. The tomorrow forecast and the default reply compared a null precipitation probability with 30, which raised too; a null probability now just leaves the precipitation out, and a tomorrow with null temperatures reports that it is not available.
abbreviate_wind_direction matched full words (NORTHWEST, NORTH, ...), but NOAA's forecast windDirection is always an abbreviation like WNW, SSE or N. Every value fell through to the fallback, which kept the first two characters behind a generic wind emoji, so replies read '💨WN2' or '💨SS1' and even plain N and NE lost their arrow. The 16 compass abbreviations now return the nearest 8-point arrow plus the abbreviation; full words and other values behave as before. Found while pinning wx's formatting on real NOAA responses for the maintainability refactor.
Review found the same reply-limiter drop in two more places: the earthquake service's link post right after its alert, and the announcements command's 'sent' confirmation right after the announcement. Both now skip the reply limiter. multitest called wait_for_tx itself just before send_response, which waits again; with wait_for_tx claiming the slot that pre-wait made the reply wait a full extra interval, so it goes. A test that only documented RateLimiter's behavior, and guarded nothing, is replaced by an earthquake test that drives _check_earthquakes.
A user who raised bot_tx_rate_limit_seconds to 5 noticed wx's weather-then-alerts reply did not seem to honor it. The gap between those two messages was in fact longer than the limit (every send already waits on the bot TX limiter in _check_rate_limits, and wx sleeps limit + 1 s on top), but looking at it turned up two real bugs.
The second part of wx's and gwx's two-part reply, and every part after the first of 'alert all', went through the reply rate limiter (rate_limit_seconds, 10 s by default) a few seconds after the first part had used it, so it was refused: with default settings the alert message was never sent. These now pass skip_user_rate_limit=True, as the other multi-part senders already did. The repeater command's two-part branch gets the same change for consistency.
BotTxRateLimiter.wait_for_tx only checked the time since the last recorded transmission, and record_tx runs after the radio confirms, so concurrent senders (two replies, a reply and a scheduled message) all passed the wait before any of them recorded and transmitted back to back. wait_for_tx now claims the slot under the lock when it returns. A coverage test that monkeypatched can_tx to drive the old loop is rewritten against the new one.
A second review of the restart fix found more of the same class. The map uploader's stop() removes its file log handler and start() never added it back, so after one restart it logged nothing to file; start() now restores it. DARC MoWaS set _running before binding its port, so a port in use left it dead but healthy, and werkzeug's make_server calls sys.exit when the port is taken, which got past every except Exception and stopped the bot; start() now binds first and turns that exit into an OSError.
Boot-time starts go through _start_service_at_boot, which records a restart failure when start() raises, so that service also waits out the backoff, and no longer reports a disabled service as a configuration problem. The health-loop step is _restart_unhealthy_services. New tests cover both, the bridges' session cleanup and the map uploader restart; all 13 lifecycle tests fail on dev.
Review found that marking a not-running service inactive for good also retired services that only returned early for a transient reason: packet capture and the map uploader give up after waiting 30 s for the radio, and the bridges return when meshcore is momentarily unset, which is exactly when the health loop restarts them. Such a service now records a restart failure, so the next attempt waits out service_restart_backoff_seconds; a transient failure recovers, a missing configuration costs one attempt per backoff instead of one every 5 s. The restart decision is _service_restart_due, which the tests now exercise directly.
Also from the review: packet capture and the map uploader never cleared the should_exit flag stop() sets, so after a restart their background loops exited at once while the service reported healthy; start() now clears it. The Discord and Telegram bridges close the HTTP session they open when they return early because meshcore is not available.
configparser's getfloat accepts 'nan', and NaN fails every comparison, so it passed both the negative-value check and the min() cap and reached send_msg_with_retry as the ACK wait. It now falls back to the default like a negative value. 'inf' was already capped at 60 s; both cases are in the parametrized test, and the nan case fails on dev.
A fifth review found two gaps. A scheduled trial that returned False was re-armed at once, but False also follows exhausted no_event_received retries, so the radio could be tried again and again without another answered probe. And a reply's success settlement ran in a worker thread that a cancelled caller could cancel while it was still queued, stranding the trial in flight.
_end_offline_trial ends a trial that went out without succeeding (False or an exception, from a reply or a scheduled send) under the state lock alone; only paths where nothing reached the send still re-arm. The success settlement is shielded, so it completes even if the caller is cancelled. The cancellation test queues the settlement behind a busy one-worker executor and fails without the shield.
The health loop restarted every enabled service whose is_healthy() was False. A service that returns from start() without running because it has nothing to do (Discord or Telegram with no channels, the map uploader with no key) is never healthy, and its restart raised nothing, so it counted as a success and cleared the backoff: the service was restarted every 5 seconds, logging a warning each time.
Services whose start() returns without running are now recorded as inactive, logged once, and skipped by the health loop. A restart that leaves a service running but unhealthy records a failure, so the next attempt waits out service_restart_backoff_seconds.
Restarts also double-subscribed: services subscribed to meshcore events in start() and never unsubscribed (the comments said meshcore cleans up; it does not). BaseServicePlugin gains _subscribe and _unsubscribe_all; the Discord, Telegram, map uploader, packet capture and prefix-collision services subscribe through it, unsubscribe in stop(), and drop the old instance's subscription before re-subscribing after a transport reconnect. Tests use meshcore's real EventDispatcher and fail on dev.
Only scheduled messages and interval adverts could claim the trial armed by an answered health probe, so a bot with neither stayed offline however healthy the radio became. send_dm and send_channel_message now claim an armed trial when offline and run the send through _send_as_offline_trial, under the trial's identity; its result settles the trial (True clears the outage, False or an exception fails it until the next answered probe), so a dead radio still gets at most one attempt per probe. Settlement runs in a worker thread because it can write bot_metadata. Tests drive the real CommandManager guard, and the alert-thread counting in the recovery tests no longer stops asyncio.to_thread's workers.
A third review found four more gaps. A trial send that hit the 60 s wait kept running and could transmit later, during a newer trial; _record_send_success cleared any outage without checking whose success it was, so an old send finishing late could clear a newer one; the wrappers claimed the trial before logging and time lookup, outside the code that settles it; and the startup advert's timeout path recorded its failure on the event loop, where tripping the outage writes bot_metadata.
_record_send_success now takes the trial id and clears only when that trial is still in flight, deciding and resetting under the state lock (_reset_offline_state_locked); success of an ordinary send while offline leaves the outage alone. A trial whose wait fails is cancelled. Each wrapper settles the trial if anything after admission raises. The startup advert records its failure through asyncio.to_thread.
A second review found five gaps. A loop that closed between is_running() and submission stranded the trial in flight; fallback sends (no running main loop) settled every trial as inconclusive; a timed-out send that kept running passed the guard during a later trial, because the context flag was a bare True; the viewer clear checked the outage generation, released the lock, then cleared, so it could clear a newer outage; and the health loop ran the metadata sync, with its database retries, on the event loop.
Trials now carry an id: _admit_measured_send returns it, _as_offline_trial sets it in the context, and is_radio_offline and the settle methods only honor the trial currently in flight. The scheduler's two wrappers share _run_measured_send, which submits inside its error handling (closing the coroutine if submission fails), settles a fallback trial by its real outcome, and settles any trial left open in a finally. _clear_radio_offline_state takes an expected generation and checks it under the state lock. The health loop checks the 30 s throttle on the loop and runs the sync itself in a worker thread. The delayed-write test now waits for the newer outage to trip before releasing the stalled writer, so it fails without publish serialization.
Review of the circuit breaker found four problems. The trial was a shared flag that let every sender through, interactive sends never settled it, and it could stay open forever. The scheduler threw away send_channel_message's result, so a send the radio rejected counted as a success and cleared the outage. Metadata writes happened outside the state lock, so a stalled clear could overwrite a newer outage's 'true' and the viewer check would then cancel it. And one failed read-back of the persisted 'true' disabled the viewer clear for the rest of the outage.
Now a probe arms one trial, which the scheduler's next measured send (scheduled message or interval advert) claims atomically through _admit_measured_send; that send runs under a context flag so only it passes the send-path offline guards. Its real outcome settles it: success clears the outage, a timeout ends the trial until the next answered probe, and a False result or nothing sent re-arms it. _send_scheduled_message_async now returns whether the message went out. Metadata is published under its own lock from the current state, confirmed per outage generation, and retried from the health loop until confirmed. The in-process viewer clear calls the full clear. Tests go through the real scheduler on a live loop, race admission across threads, stall a clear write while a new outage trips, and count alerts from the first trip.
Moving the geocode decision ahead of the await meant every advert that arrived while another was still geocoding read the same uncommitted row and geocoded again; six adverts for one contact made nine or more Nominatim requests instead of two, at the 1 req/s budget. track_contact_advertisement now holds a per-contact asyncio lock across the read, geocode and write, which restores the old one-at-a-time behavior for a contact while different contacts still proceed independently. The lock entry is dropped when its last holder leaves.
Also: geocoding_cache reads in the worker-thread helpers use .get() so the loop-thread expiry pass cannot raise KeyError between the check and the read, the write-block duplicate guard calls _is_duplicate_advert_packet, and the guard test loses leftover scaffolding.
Review follow-up. A migration script containing COMMIT, BEGIN, ROLLBACK, END, SAVEPOINT, RELEASE or VACUUM would end or escape the runner's transaction and bring back the non-atomic failure; _execute_statements now refuses those statements. Added tests for a rollback that undoes a column added to a pre-existing table (the legacy-database case), for a trailing comment after the last statement, and for the refusal. The changelog now notes that the write lock is held for the whole migration run.
Review follow-up. A get_time probe is a host-to-companion frame with no RF transmit, so it answers in exactly the failure mode the offline state exists for; clearing on it would flap offline and online every probe interval and send an alert email each time. A probe success now only opens a trial: sends are let through, and the next send the scheduler measures either clears the state or closes it again, without another alert. The alert goes out once per outage.
The reconnect clear is gone (a connect can still fail after the socket is up, and the probe covers recovery after it). The viewer-clear check runs only after this process has read back its own 'true', so a read that races the trip cannot cancel it; it is throttled to every 30 s and runs before the transport check, so it works while disconnected. State changes take a lock, since the scheduler thread and the event loop both write them.
track_contact_advertisement, _background_geocoding, scan_and_catalog_repeaters and populate_missing_geolocation_data are async but called rate-limited Nominatim synchronously (with the rate limiter's time.sleep), so the whole bot stalled for each lookup. The async-path guard test only scanned commands and service plugins, so it never saw this file.
The calls now go through asyncio.to_thread. track_contact_advertisement decides whether to geocode, and does it, before its write block, which stays free of awaits so concurrent adverts cannot interleave between its reads and writes; the duplicate-packet guard runs both before geocoding and again inside the write block. The guard test now scans repeater_manager.py and knows the state/country and full-location helpers; a new test shows advert geocoding runs off the loop thread and fails without the change.
MigrationRunner.run wraps all pending migrations in BEGIN IMMEDIATE and rolls back on failure, but nine migrations called cursor.executescript, which issues an implicit COMMIT before running. A failure in a later migration therefore left every earlier one, and its schema_version row, committed. Reproduced: appending a failing migration to a fresh database left 23 versions applied.
_execute_statements runs a script statement by statement with execute, splitting on ';' only where sqlite3.complete_statement agrees, so semicolons in string literals and comments are left alone. A fresh database migrates to a byte-identical sqlite_master and schema_version.
After radio_offline_threshold consecutive send timeouts the bot enters radio-offline state and every send path refuses to transmit. The only thing that cleared it was _record_send_success, which runs after a send goes out, so once offline the bot stayed offline until restarted. The web viewer's Clear Offline Flag could not help either: the viewer runs in its own process, so it only wrote bot_metadata, which the bot never read back.
Three paths now clear the state: a health probe the radio answers, a successful (re)connect, and the health loop noticing that bot_metadata says the flag was cleared. The new tests for the probe and viewer paths fail without the change.
send_channel_message compared the set_flood_scope reply's type to the string "ERROR", but it is an EventType whose value is "command_error", so the comparison never matched. A rejected regional scope sent the message at whatever scope the radio held, and a rejected restore to global flood left the radio pinned to the region for every later send, both without a log line. Compare against EventType.ERROR.
The existing tests for this path built their error reply with the same "ERROR" string, so they passed against the broken check; they now use EventType.ERROR and fail without the fix.
Review follow-up. The firmware's direct-path estimate includes hop count and about 750 ms of fixed allowance, not airtime alone, so the comments and changelog now say that. Values above 60 seconds are capped, since a value meant as milliseconds would otherwise hold one DM for hours, and the changelog notes that an unacknowledged DM now takes at least 24 seconds to report failure. The late-ACK test has a wider margin so it cannot flake on a loaded machine.
Review follow-up. The scope read could raise (a bare '%' under interpolation) after the 'sent' row and mesh cooldown were reserved, and the rollback never ran, so the log showed a warning that never went on the air. The scope and body are now prepared before anything is reserved, and a failure is recorded as a failed attempt. [Region_Warnings] flood_scope is read raw and validated, because set_flood_scope's failure path sends anyway at whatever scope the radio holds. Dry runs now log the truncated body the live send would use.
send_msg_with_retry was called with timeout=0, so each attempt waited 1.2 times the radio's suggested_timeout. The companion firmware derives that from airtime alone (500 + (6 x airtime + 250) x (hops + 1) ms direct, 500 + 16 x airtime flood), but the round trip also carries fixed costs that do not shrink with airtime: the recipient's ACK delay, repeater retransmit and rx delays, the radio's TX queue and the host link. On a fast preset a zero-hop attempt gave up after about 1.3 seconds, the ACK of a delivered message arrived after the last attempt, and the send reported failure. That is the residual #290 symptom: webhook DMs erroring within a couple of seconds for messages the recipient had.
New [Bot] dm_min_ack_timeout (default 8 seconds) is passed as min_timeout. An ACK still ends the wait at once, so successful sends are no slower, and since the radio lock is per-frame the longer wait on a failing DM does not hold up other sends. 0 restores the old behavior. The new test drives the real meshcore retry loop with an ACK slower than the radio's estimate and fails without the floor.
Channel-delivered region warnings were hard-coded to scope='*', on the belief that a sender with no region could not hear a scoped message. That is not how MeshCore works: the companion firmware's filterRecvFloodPacket drops nothing, so a client accepts a regional flood whatever region it has set itself, and only repeaters decide how far it goes. The override only meant that a bot configured with outgoing_flood_scope_override flooded the whole mesh to tell someone not to, which is exactly what #306 reports (the log shows the override 'was not applied (explicit scope="*")').
The warning now resolves its scope like any other proactive channel send: [Region_Warnings] flood_scope, then flood_scope.<channel>, then outgoing_flood_scope_override, then global. Its body is sized with channel_body_budget so the regional overhead is accounted for, and the web viewer's byte limit reserves that overhead whenever a regional scope could apply. flood_scope = * restores the old behavior.
Builds on #303. Channel text is capped at 155 bytes of "<name>: <text>" instead of 124. The firmware encrypts a 4-byte timestamp and a 1-byte text type ahead of the text in 16-byte AES blocks, so 155 fills the 160-byte block exactly (163-byte payload), and the last 5 bytes up to the firmware's 160 would spill into another block (179-byte payload).
124 was set on the premise that repeaters drop 165-byte frames. They don't: allowPacketForward has no size check and MAX_PACKET_PAYLOAD is 184, and 163-byte GRP_TXT payloads are routinely seen at 3+ hops. The frame-budget tests now pin the limit within the firmware's 160 and to a whole cipher block instead of to a 131-byte relay ceiling.
Also corrects the payload model's plaintext header from 4 bytes to 5, and moves a test import back above the helper that had split the import block.
* feat(web_viewer): split anonymous read access from an admin session
When web_viewer_password is set, Dashboard, Contacts, and Mesh Graph are readable without logging in, along with the read-only APIs they need. Radio, Config, Logs, every mutating API, /api/channels, and the live Socket.IO streams require an admin session, since those can expose channel keys and decrypted traffic. The Realtime page still renders publicly but shows "Admin login required" instead of connecting. Anything not on the public allowlist fails closed.
Login now compares HMAC digests with hmac.compare_digest and clears the session before setting authenticated_admin. Logout clears the whole session and returns to the dashboard.
Co-authored-by: Gerard Hickey <hickey@kinetic-compute.com>
* fix(web_viewer): stop Contacts redeclaring IS_ADMIN and hide banner admin buttons
contacts.html declared its own top-level const IS_ADMIN, but base.html already declares one in an earlier script on the same page. Classic scripts share one global lexical scope, so the redeclaration threw a SyntaxError and the whole Contacts script never ran, leaving the table on its loading placeholder in every mode. Contacts now uses the global from base.html.
The zombie-recover and clear-offline banner buttons were still rendered for anonymous viewers even though their /api/admin/* endpoints now return 401. They are gated on the same auth_enabled/is_admin check as the other admin controls.
* fix(web_viewer): drop sockets on logout, block off-site login redirects, allow HEAD on public paths
Logging out cleared the HTTP session but left that login's Socket.IO connections open, and a socket keeps the session it connected with, so a /realtime tab kept streaming packets, decrypted messages, and logs after logout. Each login now gets an admin_login_id, sockets record it on connect, and logout disconnects the sockets opened under that login. Other admin logins stay connected.
The login next check passed ///host and /\host because urlparse sees no netloc in either, but browsers resolve both as another origin. Next targets are now rejected if they start with //, contain a backslash, or contain a tab or newline. This check predates the admin split.
Public pages and GET APIs only accepted literal GET, so HEAD (and OPTIONS) on them returned 401 and made uptime checks report the viewer as down. They now accept HEAD and OPTIONS too; admin paths are unchanged.
* docs(changelog): note the login redirect fix
* fix(web_viewer): pass IS_ADMIN through a body data attribute so ESLint can parse base.html
The frontend lint job failed with "Parsing error: Unexpected token {" because base.html's inline script assigned IS_ADMIN from a raw Jinja expression, which isn't valid JS until rendered. The flag now lives in data-is-admin on <body> and the script reads it from document.body.dataset. I also dropped the unused AUTH_ENABLED constant and declared IS_ADMIN as a readonly ESLint global, since contacts.html reads it from base.html.
---------
Co-authored-by: Gerard Hickey <hickey@kinetic-compute.com>
Decoded packet_stream rows had no way back to the original packet, so alternate routes could not be joined. The hash is already on routing_info; keep it in the existing JSON blob.
An oversized webhook or channel body was handed to the firmware whole. The device dropped it without an ack, retries stalled the transport, and the radio looked dead. Split to the frame budget, keep links intact, and tag multi-part sends so they can be reassembled.
Reserve the wx location prefix from that same budget, and let NOAA alert polling be turned off without disabling the rest of the weather service.
waev.app rejects a token whose exp is more than an hour past its iat, so the
project-wide 24-hour default could never authenticate there — the operator saw
only a bare auth failure with no hint that the lifetime was the problem, while
the same public key worked from meshcoretomqtt.
waev.app hosts now resolve to a 3600s TTL with renewal at 3500s when no value
was configured for them, per broker or globally; a configured value still wins,
and the substitution is logged. Matching is the apex and its subdomains only, so
a lookalike host like waev.app.example.com does not qualify.
agessaman