A room config without a "type" key authenticated as a repeater, so the
empty-guest-password rule admitted blank logins to a room with
allow_read_only off. The login callback now passes the registered type.
A blank-password newcomer on a full table could evict a stored read-only or
read-write grant and delete its row. It now evicts only entries that are not
stored, and is refused when none are left.
Firmware simple_repeater compares a blank password from a sender not in the
ACL with guest_password, which defaults to empty, so anyone may log in as a
guest and fetch base telemetry. The repeater refused it unless
allow_read_only was on. Room servers are unchanged.
mesh_cli is imported on every boot, so it reads openhop_core.__version__
with getattr rather than importing it, and /api/stats does the same instead
of failing the whole response. Registering the repeater version with the
companion CLI cannot stop a companion loading if the core's signature
changes.
MeshCLI's `ver` read config["version"], which nothing sets, so every node
answered the fallback "openHop_repeater v13". Report the installed packages
instead: "openHop_repeater v<repeater> (core v<core>)".
The virtual companion's CLI `ver` now names the repeater too: the bridge
registers its version with the core's companion CLI when that core has one.
neighbor.remove now resolves its targets in SQL: zero-hop repeaters, the
nodes `neighbors` lists, all of them or those whose key starts with the
given prefix, with their cached scopes. A keyed remove no longer deletes a
companion's, room's or distant repeater's advert history that happens to
share the prefix, and a locked database leaves every neighbour in place
instead of some of them. The prefix is compared literally rather than
LIKE-matched.
The prefix delete's None branch, which cleared every advert, is gone along
with get_neighbors' raise_errors flag, which nothing uses now.
get_neighbors answers {} on a storage error, which `neighbor.remove all` took
for "no neighbours" and answered OK. Add an opt-in raise_errors to
get_neighbors (and the storage collector passthrough) and use it here, so a
failed read surfaces as an error.
The storage call behind a clear-all deletes every advert, so
`neighbor.remove all` also wiped companions, room servers and multi-hop
nodes, with their first-seen and advert counts. Firmware clears only its
neighbour table. Remove just the zero-hop repeaters that `neighbors` lists,
each by its full key, and cover it against real SQLite storage.
handle_command strips the line, so `neighbor.remove ` with nothing after it
never matched the "neighbor.remove " route and answered "Unknown command";
the empty and whitespace branches in _cmd_neighbor_remove were reachable only
from a unit test. Firmware (CommonCLI.cpp) clears every neighbour for an
empty key, which a stray trailing space can trigger.
- `neighbor.remove all` clears them all; a missing key answers
"ERR: Missing pubkey. Do you mean `neighbor.remove all`?" and deletes
nothing.
- An odd-length or over-32-byte key answers "ERR: bad pubkey", as firmware's
fromHex does. Non-hex stays refused.
- Tests go through handle_command, the real entry path.
A table full of admins refused every newcomer, and now that entries outlive
a restart that lockout was permanent; the upgrade placeholder made it likely
with max_clients: 1. The placeholder is now 32 (firmware MAX_CLIENTS), and
an admin password login or setperm to an admin role evicts the admin seen
least recently, by its stored last login after a restart. Blank-password and
guest newcomers are still refused, so they cannot strip admins one by one.
An eviction now waits for the newcomer: a refused login (a replay) puts the
evicted entry back, and its stored row is deleted only once the newcomer's
grant is stored, or once that grant's next write succeeds after a failure.
Evicted clients' replay watermarks are kept (capped, in RAM) so an evicted
admin's captured login cannot be replayed when it comes back.
unregister_identity also detaches an ACL listed only by name, after a hash
collision displaced it from the hash index.
A setperm on a full ACL deleted the evicted client's stored entry before
trying to store the newcomer, so a failed write reported an error but still
revoked someone. The eviction is now deleted from the store only once the
grant is stored, and put back in the table if it isn't.
Deleting a room whose rename was not hot-reloaded looked up its live ACL by
the new name only, so the ACL listed under the old name stayed attached and
the next grant wrote the deleted room's rows back. Fall back to the store
key, as move_room_acl does.
Stored entries now record when they last logged in, so the web UI can
show whether a provisioned grant is still in use after a restart.
Firmware keeps this in RAM only; last_activity still starts at 0 on
load, so "not active since restart" keeps its meaning.
- acl_entries gains a nullable last_login column, added beside the
table's CREATE so a failed earlier migration cannot lock stored
admins out of a load that reads it.
- A login by a stored entry writes its time, best-effort, and not when
the stored second already matches it.
- A guest promoted by setperm keeps its login time; a row taken over
from another identity's label does not inherit that identity's.
- Key changes, renames and label adoption carry the time; a retried
load keeps the later of the live and stored times and writes a login
made while the store was unreadable.
Codex confirmation round: the sweep of rows already under a name was
best-effort, so when it failed the rename or creation went ahead and the
leftovers became the room's grants. The sweep is now required: a rename
fails before the config is saved, and create_identity refuses before
adding the room.
Codex confirmation round: after a delete whose ACL cleanup failed, a room
created on that key and renamed to the deleted room's name took over its
leftover rows. Names are unique, so no live room holds the label a
rename takes or a new room gets; a rename (before taking the label) and
create_identity now drop every row already stored under it.
Codex confirmation round: rows a rekey's failed cleanup left at an old key
survived a later rename under the old label, which the rename did not
touch and the delete (by the new label) did not reach, so a room given
the old name and key could load them. A room owns only the rows under
its label at its current key; a rename, a rekey and a delete now drop
every other row under the labels involved. A failed sweep is retried by
the next one. Loading does not sweep: a room whose key was edited by hand
would otherwise lose rows that setting the key back recovers.
Codex confirmation round: deleting a room removed only its rows at its
current key. Rows a rekey's failed cleanup left at an old key kept its
label, so a room re-created with that name on that key loaded them. A
delete now drops every row under the room's label; names are unique, so
no other room holds it.
Codex round 5 (after the usage limit reset):
- A rekeyed or deleted room's old key kept answering logins, messages and
requests until a restart: its login, text and protocol handlers stayed
registered. The daemon now unregisters an identity from every packet
helper (_unregister_identity_everywhere), and update_identity and
delete_identity call it for the retired identity. Each helper removes
only its own handler, never another identity's on the same hash byte.
- Rows left at a key by a failed cleanup could grant their admins to a
later room on that key. A room loads only rows under its own label and
logs any others; moves copy, relabel and delete only the room's own
label's rows. A same-key rename relabels before the config is saved
and puts the label back if the save fails, so a room's own rows follow
its name. delete_identity reports an ACL cleanup that failed.
- Two overlapping hot reloads could start a superseded replacement
RoomServer whose sync loop then ran untracked. A swap starts its room
only if it is still the registered one.
A hot reload from PUT /api/update_identity re-runs TextHelper.register_identity,
which replaced self.room_servers[hash] and started a new sync loop without
stopping the old one, so the room pushed posts twice (the old loop with its
stale ACL). Take the RoomServer under the same hash, and any registered under
another hash by the same or previous name (a key change), out of room_servers,
and stop them before starting the replacement, on the running loop or via
run_coroutine_threadsafe from a request thread. update_identity passes the
original name through _register_identity_everywhere as previous_name.
Fifth review round (opencode: "the ACL design is clean"; residual nits):
- The old identity entry is put back whenever the new registration did not
take its place, including when registration raises.
- A room whose update leaves it with no password is not hot-reloaded: it
is not registered for logins, so its ACL would stay under the old name.
The update waits for a restart and says so.
- delete_identity releases the room's name and hash through
unregister_identity, so the room can be re-created live rather than
conflicting with a room that no longer exists until a restart.
- LoginHelper.move_room_acl checks for a store before any path, the
superseded-ACL loop again skips the ACL being registered, and the
test daemon accepts the previous_name argument of the RoomServer
teardown fix on feat/quirky-lamarr-dfeae2, which should land with this
branch: the hot reload that now applies is what reaches that bug.
Fourth review round (opencode; codex hit its usage limit):
- update_identity's hot reload was always refused: the identity manager
rejects a name or hash already registered, which the running room's own
entry always is. The ACL name index then lagged the config, so a renamed
room was unmanageable by name, and a later rekey left a second live,
writable ACL. The old entry is now unregistered first and restored if
the new registration is refused (IdentityManager.unregister_identity),
and a move falls back to the store key when the name misses.
- A key change whose save and cleanup both failed left a copy at the new
key that blocked every retry. A copy under the room's own label is
replaced on retry; another identity's entries still refuse the move.
- A registration now detaches the ACL it replaces before loading, warns
when it displaces another identity sharing its hash byte, and the no-store
guard for a key change lives in LoginHelper.move_room_acl.
- /acl_stats is documented as it has always answered; store_error and
store_errors say they clear once a later read succeeds; the example
config says room servers keep their own security settings.
Third review round (codex, opencode):
- A failed save whose rollback also failed left the ACL under a key the
config did not name. A key change now copies the entries, saves the
config, then drops the old ones, all under the ACL's lock: any failure
leaves the key the saved config names holding its entries.
- Moves and deletes found the live ACL by scanning for its store key, and a
rekey left two ACL objects for one identity, so a second move or a delete
could act on the stale one. LoginHelper finds ACLs by name, and a new ACL
detaches and drops the one it replaces.
- A room deleted and re-added with the same key reused the detached ACL and
never stored again. A detached ACL is never reused.
- A failed read at startup was never retried; the next login or change
retries it, and acl_clients reports it as store_errors.
- A key change with no store anywhere was treated as moved. It is refused.
- The owner lookup fell back to the 1-byte hash for a room with no ACL,
attributing another identity's entries to it. acl_stats used the hash too.
- A refused update_identity left its merged settings in the live config;
every refusal now restores the entry. A key that yields no public key is
refused rather than skipping the shared-key check.
- Removing from every ACL stopped at the first failed write; it now reports
which failed and which succeeded. Room eviction carries on past one.
- Settings on a reused ACL are applied under its lock.
Second review round (codex, opencode):
- Moving a room's rows and pointing its live ACL at them happened in two
steps, so a grant could land under the old key in between.
ACL.move_store now does both under the ACL's lock.
- update_identity accepted a key another identity already uses, and the
move merged the two ACLs. It now refuses such a key before changing
anything, and the store refuses to move onto a key that has entries.
- update_identity saved the new key before moving the ACL, so a failed move
left a saved key with an empty ACL. The move now runs first, and a failed
save moves it back. It also runs when the daemon has no login helper.
- A failed read at startup looked like an empty ACL. The ACL records the
error and /api/acl_info reports it as store_error.
- A deleted room's handlers stay live until a restart, and a login there
wrote its entries back. Deleting now detaches the live ACL from storage.
- /api/acl_info looked ACLs up by the 1-byte hash, so identities sharing one
could show each other's counts. It uses the name lookup acl_clients does.
- The acl_clients filter is an object in the spec, as returned.
Review (codex, opencode) found:
- A room renamed and rekeyed in one update lost its ACL, and a room later
created under the old name could adopt it. Room servers are no longer
adopted by label; update_identity moves their rows to the new key and
name in one transaction and points the live ACL at them. Only the
repeater, which is unique, still adopts by label after a key change.
- A removal racing a grant could leave the grant stored but not listed.
Each ACL change and its write now happen under one lock.
- A failed write was reported as OK. The store now raises; setperm answers
"Err - failed to save" and leaves the table as it was, and the API
returns an error. A login still succeeds, as its session is valid.
- Re-registering an identity (a rename) built a new ACL, zeroing every
session's activity and replay watermark, so the room sync loop stopped
pushing to logged-in clients. The live ACL is now reused.
- The repeater's max_clients default rises to firmware's 32: admins are
never evicted and now persist, so 5 or 10 filled permanently. Stored
entries past max_clients all load, with a warning.
- atoi takes ASCII digits only; non-repeater identity types get their own
label; the sync-loop skip logs at debug; `get acl` has help.
API for the web UI:
- POST /api/acl_set_permissions sets an entry by identity name.
- /api/acl_clients lists entries by identity name rather than the 1-byte
hash two identities can share, with permissions_value, persisted and
identity_pubkey. /api/acl_info adds acl_entries and stored_entries, and
authenticated_clients counts entries that have been active.
- /api/acl_remove_client reads client_pubkey, as its schema always said
(the UI sent it, and the endpoint read only public_key), and takes
identity_name.
Firmware keeps its ACL in /s_contacts, so a key provisioned over serial with
`setperm <pubkey> 3` logs in with a blank password forever after. Ours was
RAM-only and setperm was a stub, so that workflow could not work.
- New acl_entries table, keyed by the local identity's full 32-byte public
key rather than its 1-byte hash, which collides between identities. An
identity_label ("repeater", "room_server:<name>") lets an identity adopt
its rows after a key change, as firmware's ACL survives a new private key.
Shared secrets are derived on load, never stored.
- The repeater stores every entry with permissions, a room server admins
only (firmware saveFilter). A write happens only when an entry's stored
permissions change, so a returning admin costs no I/O.
- setperm follows ClientACL::applyPermissions and firmware's replies: atoi
permissions stored as the whole byte, a guest role deletes by key prefix,
other roles need the full key. It refuses an empty prefix and a key that
is not a valid ed25519 point, both of which firmware accepts.
- `get acl` works from the web CLI (the serial stand-in, passed local=True)
and still errors remotely. It was unreachable: "get " matched it first.
- A full table evicts the least active non-admin, as firmware putClient,
instead of refusing; it still refuses when every entry is an admin.
- Room servers keep a stored admin through inactivity eviction and do not
push to entries that have not logged in since load, as firmware skips
clients with no activity.
- Deleting a room server drops its stored entries.
The three readers of the CRC scalar all answered for one receiver. On a bridge
that is half the node: a backhaul going deaf moved no number an operator or an
observer could see, while the sum of radios[].errors in the same status message
already said so.
All three now read RepeaterHandler.get_crc_error_count(), which sums the
radios -- stats.errors in the MQTT status message, crc_error_count in /stats,
and n_recv_errors in the MeshCore wire response, which previously read
self.radio directly and so described the default endpoint. stats.errors and the
per-radio entries beside it now reconcile.
This matches what node_stats() already does with the airtime counters that sit
next to it: a node-level counter describes the node, and which radio
contributed what stays available per radio, in radios[] and in the crc_errors
table.
The sum prefers each radio's live hardware counter and falls back to the last
count the sampler read from a radio the fabric cannot hand back. These are
lifetime counters, so dropping a radio would walk the node's total backwards
and hand an observer reading deltas a negative interval. A single-radio node
still reports its one unlabelled counter, read live, exactly as before.
The filter added in the previous commit asked whether the record carried
a drop_reason. Every TRACE record carries one by design - trace_received
on arrival, trace_forwarded once relayed - and TraceHelper reaches
storage through log_trace_record, which took the default. Traces
therefore stopped reaching the brokers entirely.
Classifying a packet as invalid is the caller's job, and the engine
already does it; drop_reason is not that classification, since
duplicates, policy drops and traces all carry one and all belong on the
observer feed. So record_packet takes the caller's judgement as final
and defaults to publishing: skip_mqtt_if_invalid (default True, meaning
"decide for me") becomes skip_mqtt (default False, meaning "this one is
invalid").
TRACE also builds its own record rather than going through
_build_packet_record, so it never picked up the ingress radio. Read it
off the packet there. There is no egress counterpart: a forwarded trace
is injected into the router later and only mutates the in-memory record,
so what reaches the brokers is always the reception.
A duty cycle is a limit on a channel, not on a node, and one AirtimeManager
charging every radio at the top-level radio block's modulation is wrong twice
over on a bridge. The same packet occupies a 62.5 kHz channel about eight times
longer than a 500 kHz one, so whichever bandwidth that block names, the other
radio is mis-charged by that ratio; and the two share one allowance, so a busy
local radio spends the backhaul's airtime on a band it never transmits on.
There is now one budget per channel, built from each radio's own profile and
its own band's limit -- 868.0-868.6 MHz allows 1% where 869.4-869.65 allows
10%, and one number cannot describe both. Two radios tuned alike share a
budget, because they contend for the same spectrum. duty_cycle.shared_budget
restores a single node-wide allowance for anyone who wants the conservative
reading, and a single-radio node is metered exactly as it was.
Metering a channel means knowing which radio will carry a packet, and the
engine used to work that out by restating openhop_core's dispatch rules. It
asks now. openhop_core passes a packet's ingress radio to the TX policy, so the
choice is a function of the packet rather than of whatever arrived most
recently; the repeater asks before scheduling and gets the answer the send will
reach. That also closes a race that predates this: sticky and bridge read the
node's last RX at send time, which after a retransmit delay on a busy node is
another packet's arrival, and moves a reply onto the wrong band.
The core change is optional. The keyword in resolve_tx_radio_id's signature is
the version check, and against a core without it the tx_mode selectors are
registered in their original one-argument form and behave exactly as they did.
What changes there is the gate: with no way to know which radio will transmit,
every radio the fabric could pick is held to its own budget, so a node cannot
transmit past a legal limit by guessing the quiet channel. Which mode is in
force is logged at startup.
Also fixed, each reproduced by an adversarial review of the earlier attempt:
- A node with one radios[] entry had its modulation and limit overwritten by
the stale top-level block on every radio save from the web UI. The refresh
path now branches on whether profiles were read, not on how many radios
there are.
- build_radio_profiles is all-or-nothing by design -- it feeds the UI -- so one
radio_type: none collapsed metering onto the top-level block for the radio
still transmitting. Metering has its own builder that degrades per radio.
- Carrying the whole node's spend onto an unrecognised channel compounded over
repeated rebuilds (100 -> 400 -> 1600 ms) and wedged transmission for the
rest of the window. Carry-over is keyed by channel, which makes an unchanged
rebuild idempotent, and an unrecognised one starts from the busiest single
channel rather than the sum of them.
- A rebuild worked the previous default manager out from the config again,
which by then already named the new default -- preserving the wrong object
and orphaning the one NeighborScopeHelper holds for the life of the process.
It is remembered rather than recomputed.
- Every caller that reports one airtime figure was reporting the default
radio's channel: the MeshCore wire field total_air_time_secs, companion
stats, the RRD and SQLite history, and /stats. They read a node total again
-- totals summed across channels, utilisation the highest of them, because a
node at its limit on one channel is at a limit however quiet the other is.
tests/test_fabric_fanout.py's rig never bound a tx_mode selector, so all 49 of
those tests ran against a fabric with no TX policy at all. It binds the real
one now; that gap is why an on-air change went unnoticed through two reviews.
pushPostToClient sizes the body with strlen(post.text) for the payload and
the expected ACK alike. The web API will store "a\0b", and pushing it whole
sends text the receiver cannot see -- it stops at the NUL -- while hashing
the pending ACK over a span the receiver never reproduces. The push would
then never be acknowledged and would retry into the failure backoff.
Same defect the core builder had; this is a second, hand-rolled builder.
Also proves MC-R4 end to end at last: a genuinely encrypted guest PLAIN
through a real registered room identity, watching the injector for an ACK
that must not appear. The seam was only covered in three separate halves
before -- core honours a veto, _may_ack computes one, a mocked constructor
receives one -- none of which would have caught the wiring being dropped.
The core text handler answers by BaseChatMesh's rule -- plain and signed
text, never a CLI type -- and it runs before this helper's gates. So a
signed post, a non-admin command or a guest's post was acknowledged and
only then thrown away: the sender saw delivery confirmed for a message
that went nowhere.
_may_ack is firmware's rule for a server, handed to the core handler as
should_ack_fn at register_identity. simple_repeater builds an ACK only
under `if (flags == TXT_TYPE_PLAIN)`, inside a branch already gated on
isAdmin(); simple_room_server sets send_ack only on the non-guest PLAIN
path. Neither answers a CLI type, an unsupported type or a replayed
timestamp -- but both still answer a retry, because firmware suppresses
the work, not the reply. It reads no state it does not also leave alone.
It predicts eligibility, not the outcome: a post that clears the role and
replay checks and is then refused by openHop's own rate limit or a
database error is still ACKed. Those refusals have no firmware
counterpart, so closing that gap needs the store result fed back here.
Guarded by a signature check: passing should_ack_fn to a core that does
not take it raises TypeError out of register_identity, which would leave
the node with no text handler at all. pyproject.toml tracks
openhop_core@dev with no version floor, so that is an ordinary state --
and in it the ACK goes back to being sent, availability over the
invariant.
Two things firmware does that openHop did not.
Dispatch by type, not by text. simple_room_server::onPeerDataRecv routes
CLI_DATA/CLI_COMMAND to the admin CLI and turns PLAIN into a post;
simple_repeater has no chat function, so it runs every accepted type
through handleCommand with no text test at all. openHop asked
_is_cli_command(message_text) for both, and so got both wrong: a PLAIN
"set ..." ran as a command instead of being posted, a command whose text
was not in the prefix list was published to the room, and an admin's
mistyped repeater command vanished silently instead of being answered.
Guard replays. Both servers open the branch with
`sender_timestamp >= client->last_timestamp`, treat equality as a retry
they accept but do not re-run, and drop anything older. openHop checked
neither, so replaying an admin command with the same timestamp and
different attempt bits re-executed it -- packet dedup does not catch that
because the bytes differ. Note ACL._is_replay is not reusable here: its
strict <= rejects the equal case this branch accepts.
Both read from packet.decrypted, which openhop_core now publishes. A core
too old to report them falls back to the previous text-prefix behaviour
and logs a warning rather than dropping every message.
simple_room_server::onPeerDataRecv takes the
`(client->permissions & PERM_ACL_ROLE_MASK) == PERM_ACL_GUEST` branch for a
PLAIN message and stores nothing. openHop called add_post for anyone who
authenticated, so knowing the guest password was enough to write to a room:
add_post caps length and rate-limits per client, but never looks at the role.
Also warns once per message when openhop_core does not publish txt_type,
so the open text-type gate is visible in the log rather than silent.
simple_repeater::onPeerDataRecv and simple_room_server::onPeerDataRecv
both open their TXT_MSG branch by rejecting any flags outside
{PLAIN, CLI_DATA, CLI_COMMAND}. openHop had no such gate: it decided what
a message was purely from whether the text looked like a command, so a
SIGNED_PLAIN -- a room post, whose 4-byte author prefix the core handler
has already stripped, leaving bare text -- ran as an admin CLI command
whenever it happened to start with a command prefix, and was stored as a
post on a room server that firmware would have dropped.
CLI_COMMAND (3) joined the accepted set when firmware split "a CLI
command" out of CLI_DATA (MeshCore 2c0ace25); CLI_DATA stays in it
because that is what every released client still sends.
Reads the type from packet.decrypted, which the core text handler now
publishes. A core too old to report it leaves the gate open rather than
dropping every message.
The room CLI advert read only settings.room_name while the daemon
scheduler and the HTTP endpoint read settings.node_name -- so when the
two keys differed, a single `advert` issued over the mesh CLI renamed
the room on the mesh to the identity-name fallback. Both read sites now
resolve node_name -> room_name -> identity name.
Seen in production on 2026-08-19: a room configured with node_name only
renamed itself after one CLI advert and clients tracked it under the
identity name until the next scheduler advert.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
acl.py kept its own copy of the role constants, so an installation could
pair the new numbering with a core that still derives is_admin from the
0x02 bit. That combination announces a room server's READ_WRITE clients
(role 2) as admins on the login reply's is_admin byte — silently, and
exactly where this change is meant to make the two bytes agree. The
dependency is a mutable openhop_core@dev, so nothing prevented it.
Import the roles and acl_is_admin/acl_role from openhop_core instead of
duplicating them, and raise an actionable ImportError when they are
absent: refusing to start beats misannouncing a client's privileges. The
drift test no longer skips on an old core, so CI reports the mismatch
rather than hiding it.
OpenHop numbered the ACL roles GUEST=0x01, ADMIN=0x02, READ_WRITE=0x01.
Firmware ClientACL.h uses GUEST=0, READ_ONLY=1, READ_WRITE=2, ADMIN=3 in
the low two bits, so our admin byte decoded as READ_WRITE in every stock
MeshCore client (KiekR showed admin sessions as guest and hid the admin
controls), and GUEST/READ_WRITE were indistinguishable from each other.
Adopt the firmware values and split the guest password by server type,
as firmware does: a repeater's guest password grants GUEST (fetch base
telemetry, no settings changes) and a room server's grants READ_WRITE
(post and read messages). Blank-password read-only access stays GUEST,
matching simple_room_server's allow_read_only branch.
Also fix the mesh-CLI admin gate in text.py, which shadowed a local
PERM_ACL_ADMIN = 0x02 and tested the bit rather than the role — under
the corrected numbering that would have let READ_WRITE clients run admin
commands. The web ACL list now reports the real role name instead of
collapsing every non-admin to "guest".
Requires openhop_core with acl_is_admin() in the login reply
(openhop_core fix/login-perms); the two must land together.
Fixes#388
Enhanced the `persist_discovery_result` function to retain advert metadata such as `node_name`, `latitude`, and `longitude` from existing records when updating neighbor information. This prevents overwriting valuable metadata with `None` values during SQLite updates. Added a corresponding test to verify that existing metadata is preserved correctly during the discovery process.
Follow-up work on the neighbours feature, plus one crash found while reviewing it.
The crash: `mqtt_brokers.neighbors` is a settings block, but the per-broker key of
the same name is a boolean and config.yaml.example documents both, so
`neighbors: true` under mqtt_brokers is an easy hand-edit to make. Every reader
called .get() on it directly, raising AttributeError inside
NeighborScopeHelper.refresh_config -- which is built during daemon init, so the
daemon would not start at all. A shared neighbors_config_block() accessor now
ignores a non-mapping (with one startup warning instead of per-tick noise) and
the API save rewrites it as a proper block.
Payload counters, mirroring firmware buildNeighborsMessage:
- total_neighbors and queried_neighbors. The latter cannot be derived from
`status`: a neighbour the sweep never reached reports `timeout` exactly like one
that was asked and stayed silent. ScopeResult therefore carries a `transmitted`
flag, set only past the point where the injector confirms the request is on air.
- Firmware's third field, `truncated`, is deliberately not emitted -- it reports a
fixed PSRAM JSON buffer overflowing, which openhop does not have. total_neighbors
here consequently always equals the published row count.
- self.default_scope: the region this node stamps on outgoing floods, or `*` when
unset. Read from live config so `region default <name>` applies without a
restart, and `#`-stripped like the scopes string beside it. Firmware tracks this
internally but does not publish it, so this field is openhop-only.
POST /api/publish_neighbors runs a cycle immediately, the HTTP twin of the
`discover.scopes` mesh CLI command. Authenticated by default along with the rest
of /api, and it schedules the cycle on the event loop rather than holding the
request open for the minutes a cycle takes.
Schedule persistence: _next_publish_at is monotonic and _last_publish_at was
in-memory only, so every restart read as "due" and spent a discovery broadcast
plus one serialized scope query per neighbour. A generic daemon_state table
(migration 14) now records the last publish and start() resumes from it, never
sooner than a five-minute grace window and never later than one interval. Two
details this had to get right:
- The schedule keys off the last *successful* publish. A failed publish also sets
_last_publish_at and reschedules on the short retry delay; persisting that would
silently turn the retry into a full interval.
- _tick's disabled branch no longer clears the schedule before the feature has
been enabled in this process. At boot the MQTT connections may not be up, so
enabled() can briefly be false, and clearing there would discard the restored
schedule and re-run the sweep anyway -- the exact thing this prevents.
MeshCLI._auto_add_discovery_result reached for storage_handler.record_advert and
returned early when it was absent. record_advert lives on StorageCollector, but
MeshCLI is constructed with the SQLiteHandler (main.py -> TextHelper ->
MeshCLI), which exposes only store_advert. The guard therefore always failed in
production: `discover.neighbors` discovered nodes and recorded none of them, so
`neighbors` listed whatever adverts happened to arrive rather than what
discovery found. Tests missed it because they injected a hand-rolled double that
did have record_advert.
Extract the shared persistence into discovery.persist_discovery_result, which
accepts either storage object its callers actually hold: record_advert when
present (store plus the MQTT/Glass advert publish), falling back to
store_advert. neighbors_publisher._enrich_discovery_result, which had its own
working copy against the StorageCollector, now calls it too, so the two paths
build an identical advert row from one implementation.
The regression test specs its mock off the real SQLiteHandler class rather than
hand-rolling the surface, so a double can no longer claim a method the wired-in
object does not have. It fails against the previous code with KeyError
'auto_added'.
- api_endpoints: the broker rebuild is a strict field whitelist, so a client
that omits `neighbors` (every UI build predating this feature) reset every
broker to false on any MQTT save, silently disabling the feature. Fall back
to the stored per-broker value instead. An explicit false still turns it off.
- neighbor_scopes: `_query_one` cleared `_pending` in an `except Exception`,
which does not catch CancelledError. A shutdown cancel during the injector
await -- where a query spends most of its wall time -- left a dead pending
query that kept consuming RESPONSE packets and hiding them from every
companion bridge. One try/finally around the whole method.
- neighbor_scopes: refresh_config had no caller after construction despite
promising live updates; it now re-reads before each sweep, so a live
delays.direct_tx_delay_factor change resizes the response window.
- neighbor_scopes: truncate scopes at the first NUL as the responder's C-string
writer intends, rather than only rstrip-ing the cipher's zero padding.
- api_endpoints: accept the documented max_sweep_seconds and
duty_cycle_abort_seconds, reject unknown keys instead of reporting success on
a typo, and reject non-bool `enabled` / fractional interval_hours rather than
silently coercing them.
- mqtt_handler: check paho's rc before counting a neighbours publish as
delivered; the payload is uncapped, so a size/queue rejection is realistic.
- neighbors_publisher: retry after 15 minutes when a cycle publishes nothing,
instead of burning the full 24h interval.
- neighbors_publisher: track the manual `discover.scopes` cycle so shutdown can
cancel it, and move the already-running check onto the event loop.
Tests: router-hook integration (a matching response is consumed, an unrelated
one still reaches companions), cancellation, sweep re-entry, malformed and
hostile response payloads, config validation, and the broker save round-trip.
All ten regression tests fail against the pre-fix code.
Python port of the firmware's WITH_MQTT_NEIGHBORS feature (MeshCore PR #35 plus
the aba571ed pacing rework). Each cycle refreshes the zero-hop neighbour table
with a node-discovery broadcast, asks every neighbour for its region scopes, and
publishes the assembled table to the MQTT neighbors topic.
- NeighborScopeHelper: client side of the anon-regions request (the server side
already existed in openhop_core). Queries are strictly serialized -- firing
them as a burst makes the responses collide, which is what the firmware fix
addressed. router.inject_packet resolves at the firmware's logTx/logTxFail
boundary, so the response deadline is armed only once the request is on air
and the firmware's QUEUED/PENDING state machine collapses into an await.
The response window is sized from radio parameters, mirroring
neighborDiscoverQueryTimeoutMs().
- NeighborsPublisher: owns the schedule, the immutable per-cycle snapshot, and
the payload. The snapshot merges live discovery responses over the stored
table because get_neighbors() serves a 60s cache that store_advert() does not
invalidate.
- packet_router: offer PAYLOAD_TYPE_RESPONSE to the scope helper before the
companion fan-out; it consumes only on an authenticated tag match.
- Per-broker `neighbors: true` opt-in (default off) plus a
mqtt_brokers.neighbors.enabled master switch; interval is 12-336h as in
firmware, rejected rather than clamped.
- `discover.scopes` mesh CLI command triggers one cycle immediately.
The whole zero-hop table is published including neighbours that did not answer
(timeout / send_failed), matching the firmware payload. No 10 KB cap: that
existed for a fixed PSRAM buffer.
Rightup