From 211ae52d5536b8558dfcddd73e77d37196ea6b6b Mon Sep 17 00:00:00 2001 From: l5y <220195275+l5yth@users.noreply.github.com> Date: Mon, 6 Jul 2026 09:27:36 +0200 Subject: [PATCH] web: fix MeshCore ghost nodes (stale contact enrichment discarded) (#841) * web: fix MeshCore ghost nodes (stale contact enrichment discarded) * web: guard short_name gap-fill against empty strings; codify Known gaps --- ACCEPTANCE.md | 64 +++++++++ data/mesh_ingestor/CONTRACTS.md | 2 +- .../data_processing/node_writes.rb | 71 ++++++++-- web/spec/data_processing_spec.rb | 123 ++++++++++++++++++ 4 files changed, 247 insertions(+), 13 deletions(-) diff --git a/ACCEPTANCE.md b/ACCEPTANCE.md index 50ac85a..08d968f 100644 --- a/ACCEPTANCE.md +++ b/ACCEPTANCE.md @@ -435,6 +435,21 @@ existing codebase, not to the change under review. without comment syntax (JSON fixtures under `tests/`, `*.lock` files, binary assets) cannot carry the notice; there is no committed allow-list or CI check asserting headers. The B4 commands above are the interim verification. +- **A1b — two benign textual matches in the broker grep.** The repo-wide A1b + command matches `.claude/hooks/guard-edits.py` (the anti-broker edit guard's + own pattern list) and the "no broker" documentation comment in + `web/lib/potato_mesh/application/pubsub.rb` (PS1). Both are descriptive or + defensive text *about* the apex ban — neither is a broker dependency or + connection — but they sit outside A1b's `via_mqtt` exemption wording. Treat + these two files as documented exemptions until the A1b filter codifies them. +- **B1 — sandbox DNS breaks the `POST /api/instances` spec block.** In + sandboxed environments whose resolver maps the suite's test domains + (`mesh.example`, …) into the SSRF guard's restricted address ranges, 17 + `spec/app_spec.rb` "POST /api/instances" examples fail with + `{"error":"restricted domain"}` (400 instead of 201). Environmental only: + the failures reproduce identically with and without any change under review + and do not occur where the test domains resolve normally (CI). Attribute to + the environment, not the codebase or the change. --- @@ -2521,3 +2536,52 @@ federation wire, and Flutter app are behaviorally untouched by this batch — the only edits outside the four fixes are the lockstep 0.7.2 version-bump stamps (manifests, lockfiles, iOS plist, README pinned tags, S-A1), verified by `tests/test_version_sync.py`. + +--- + +## Bugfix: MeshCore ghost nodes (stale contact enrichment discarded) + +A MeshCore node first seen via a bare `ADVERTISEMENT` push was upserted as a +minimal placeholder stamped `lastHeard = now` (receiver wall clock). The +follow-up roster contact record — carrying the real name/role/public key — is +stamped `lastHeard = last_advert`, the **sender-side** advert-creation time, +which is always older than the placeholder's receive time (seconds for healthy +clocks, years for broken ones). `upsert_node`'s row-level freshness guard +(`WHERE excluded.last_heard >= nodes.last_heard`) therefore discarded the +entire named update, permanently: every later contact re-post (auto-update, +periodic snapshot, restart) is also sender-stamped and also lost, while each +advertised-position ingest re-bumps the row's `last_heard`. Result: nameless +"ghost" nodes with a hex `short_name`, NULL role (displayed as the CLIENT +default), and an advert-stamped `position_time` — violating the reconciliation +promise in `CONTRACTS.md` ("a later full contact advertisement reconciles it", +SPEC A4e). Fixed web-side (Ruby): after the guarded upsert, a non-synthetic +record additionally **fills identity columns that are still NULL** (`num`, +`short_name`, `long_name`, `macaddr`, `hw_model`, `role`, `public_key`, +`is_unmessagable`) regardless of staleness — stale data can fill gaps but can +never overwrite fresher values, and synthetic placeholders remain barred from +real rows. No ingestor/API/DB-schema change; protocol-neutral (Invariant IV). + +### GH-A1 — Stale contact records name advert-placeholder ghosts +```bash +( cd web && bundle exec rspec spec/data_processing_spec.rb -e "stale contact record enrichment" ) +``` +**Expected:** pass. Replaying the ingestor's wire sequence — bare-advert +placeholder (`lastHeard = now`, no name) followed by the roster contact record +(`lastHeard = last_advert`, older by 17 s and by ~2 years in a second example) — +leaves the node **named** with its real role and public key. The stale record +never regresses `last_heard`, never overwrites an existing name/role, empty +strings never fill `long_name` / `short_name` (the other identity fields of the +same record still fill), and a stale `synthetic=1` chat placeholder still +cannot touch a real row. + +### GH-R1 — Regression: prior acceptance still holds +```bash +( cd web && bundle exec rspec ) && ( cd web && npm test ) && ( cd web && bundle exec rufo --check . ) +( . .venv/bin/activate && pytest -q tests/ ) +``` +**Expected:** all green. At risk and explicitly required to stay green: the +pre-existing `upsert_node` guard specs (`data_processing_spec.rb` — role/ +identity preservation, generic-name fallback, synthetic flag + merge #755/#803) +and `database_spec.rb`'s node-merge suites, since the fix appends a second +NULL-fill statement inside the same `upsert_node` transaction; the Python +ingestor is untouched (A4e's advert-capture suite unchanged). diff --git a/data/mesh_ingestor/CONTRACTS.md b/data/mesh_ingestor/CONTRACTS.md index db4ef61..d6fa671 100644 --- a/data/mesh_ingestor/CONTRACTS.md +++ b/data/mesh_ingestor/CONTRACTS.md @@ -66,7 +66,7 @@ The web application applies the same normalisation as a safety net so legacy ing - *Contact roster (rich).* The startup `ensure_contacts()` fetch plus live `NEW_CONTACT` / `NEXT_CONTACT` pushes carry the full advert (name, role, position) and upsert complete node rows. This covers every node the radio has added to its contact book. - *Auto-update re-fetch (freshness).* The provider sets `mc.auto_update_contacts = True`, so the meshcore library re-fetches **changed** contacts (incrementally, by `lastmod`) whenever an `ADVERTISEMENT` / `PATH_UPDATE` push arrives. A re-advert from a known node therefore refreshes its `last_advert` / position without waiting for a reconnect. -- *Bare advert (reach).* The `ADVERTISEMENT` (pubkey-only) push is also handled directly: for a public key **not** in the contact roster it upserts a minimal "heard now" node (`lastHeard`, `protocol`, `user.shortName`/`publicKey` only — no name/type/position), so radios running with auto-add off still register the advertiser. Known keys are skipped (the auto-update path keeps them fresh). The Ruby web app preserves an existing long name on conflict, so this placeholder never clobbers a richer record, and a later full contact advertisement reconciles it. +- *Bare advert (reach).* The `ADVERTISEMENT` (pubkey-only) push is also handled directly: for a public key **not** in the contact roster it upserts a minimal "heard now" node (`lastHeard`, `protocol`, `user.shortName`/`publicKey` only — no name/type/position), so radios running with auto-add off still register the advertiser. Known keys are skipped (the auto-update path keeps them fresh). The Ruby web app preserves an existing long name on conflict, so this placeholder never clobbers a richer record, and a later full contact advertisement reconciles it. Reconciliation does not depend on timestamp ordering: the contact record carries `lastHeard = last_advert` (the **sender-stamped** advert-creation time), which is always older than the placeholder's wall-clock stamp — the web app's node upsert therefore fills identity fields (name, role, public key, …) that are still NULL even from an older-stamped record, while timestamps/telemetry stay freshness-guarded (ACCEPTANCE GH-A1). New protocols SHOULD likewise treat "node was heard" as a first-class, name-optional upsert so peer discovery does not hinge on a roster being populated. diff --git a/web/lib/potato_mesh/application/data_processing/node_writes.rb b/web/lib/potato_mesh/application/data_processing/node_writes.rb index adc079b..195e607 100644 --- a/web/lib/potato_mesh/application/data_processing/node_writes.rb +++ b/web/lib/potato_mesh/application/data_processing/node_writes.rb @@ -148,13 +148,6 @@ module PotatoMesh updated end - # Insert or update a node row from an inbound NodeInfo-style payload. - # - # @param db [SQLite3::Database] open database handle. - # @param node_id [String] canonical node identifier. - # @param n [Hash] node payload extracted from the ingestor. - # @param protocol [String] protocol identifier (default +meshtastic+). - # @return [void] # Read +hash[primary]+, falling back to the first present alias key. Lets the # node ingest contract accept snake_case fields in addition to the Meshtastic # camelCase the collector emits today; nil-aware so a boolean +false+ from the @@ -171,6 +164,26 @@ module PotatoMesh nil end + # Insert or update a node row from an inbound NodeInfo-style payload. + # + # Two-phase write. Phase one is the freshness-guarded upsert: a record + # whose +lastHeard+ is older than the stored row cannot change + # timestamps, telemetry, or position. Phase two fills identity columns + # (+num+, +short_name+, +long_name+, +macaddr+, +hw_model+, +role+, + # +public_key+, +is_unmessagable+) that are still NULL, regardless of the + # record's staleness — a stale-but-richer record (e.g. a MeshCore roster + # contact stamped with the sender-side +last_advert+, which is always + # older than the wall-clock +lastHeard+ of the bare-advert placeholder + # that created the row) still names the node instead of being discarded + # wholesale, which produced permanently unnamed "ghost" nodes + # (ACCEPTANCE GH-A1). Synthetic chat placeholders never touch real rows + # in either phase. + # + # @param db [SQLite3::Database] open database handle. + # @param node_id [String] canonical node identifier. + # @param n [Hash] node payload extracted from the ingestor. + # @param protocol [String] protocol identifier (default +meshtastic+). + # @return [void] def upsert_node(db, node_id, n, protocol: "meshtastic") user = n["user"] || {} met = pick_alias(n, "deviceMetrics", "device_metrics") || {} @@ -219,6 +232,11 @@ module PotatoMesh # sender names before the real contact advertisement is received. synthetic = user["synthetic"] ? 1 : 0 long_name = pick_alias(user, "longName", "long_name") + short_name = pick_alias(user, "shortName", "short_name") + macaddr = user["macaddr"] + hw_model = pick_alias(user, "hwModel", "hw_model") || pick_alias(n, "hwModel", "hw_model") + public_key = pick_alias(user, "publicKey", "public_key") + is_unmessagable = coerce_bool(pick_alias(user, "isUnmessagable", "is_unmessagable")) # If the incoming long name is a generic placeholder, prefer any real # name already on record so we never stomp known data with fallback @@ -240,13 +258,13 @@ module PotatoMesh row = [ node_id, node_num, - pick_alias(user, "shortName", "short_name"), + short_name, long_name, - user["macaddr"], - pick_alias(user, "hwModel", "hw_model") || pick_alias(n, "hwModel", "hw_model"), + macaddr, + hw_model, role, - pick_alias(user, "publicKey", "public_key"), - coerce_bool(pick_alias(user, "isUnmessagable", "is_unmessagable")), + public_key, + is_unmessagable, coerce_bool(pick_alias(n, "isFavorite", "is_favorite")), pick_alias(n, "hopsAway", "hops_away"), n["snr"], @@ -305,6 +323,35 @@ module PotatoMesh AND NOT (COALESCE(nodes.synthetic,0) = 0 AND excluded.synthetic = 1) SQL + # Ghost-node repair (GH-A1): the guard above skips records whose + # last_heard is older than the stored row — correct for timestamps, + # telemetry, and position, but it also starved identity data. A + # MeshCore roster contact is stamped with the sender-side + # last_advert, which is always older than the wall-clock lastHeard + # of the bare-advert placeholder that created the row, so the + # name/role/public key never landed and the node stayed a + # permanently unnamed ghost. Fill identity columns that are still + # NULL from any non-synthetic record regardless of staleness: gaps + # get filled, fresher values are never regressed, and synthetic + # chat placeholders remain barred from real rows. NULLIF keeps + # empty strings — a MeshCore contact may carry an empty adv_name, + # and shortName is guarded the same way — from filling + # long_name/short_name with blank text. + if synthetic.zero? + db.execute(<<~SQL, [node_num, short_name, long_name, macaddr, hw_model, role, public_key, is_unmessagable, node_id]) + UPDATE nodes SET + num=COALESCE(num, ?), + short_name=COALESCE(short_name, NULLIF(?, '')), + long_name=COALESCE(long_name, NULLIF(?, '')), + macaddr=COALESCE(macaddr, ?), + hw_model=COALESCE(hw_model, ?), + role=COALESCE(role, ?), + public_key=COALESCE(public_key, ?), + is_unmessagable=COALESCE(is_unmessagable, ?) + WHERE node_id = ? + SQL + end + # Reconcile synthetic placeholder rows with their real counterparts # whenever a MeshCore node is upserted. Both directions must fire — # the arrival order of chat messages vs contact advertisements is diff --git a/web/spec/data_processing_spec.rb b/web/spec/data_processing_spec.rb index c26972c..fcd248e 100644 --- a/web/spec/data_processing_spec.rb +++ b/web/spec/data_processing_spec.rb @@ -744,6 +744,129 @@ RSpec.describe PotatoMesh::App::DataProcessing do end end + # --------------------------------------------------------------------------- + # upsert_node — MeshCore ghost nodes: a bare-advert placeholder row (stamped + # with the receiver's wall clock) must not starve the follow-up roster + # contact record, whose lastHeard is the sender-stamped last_advert and is + # therefore always older (seconds for healthy clocks, years for broken + # ones). The freshness guard may skip the stale record's timestamps, but + # its identity fields (name, role, public key, …) must still fill columns + # that are NULL, or the node stays a permanently nameless "ghost". + # --------------------------------------------------------------------------- + describe "#upsert_node — stale contact record enrichment (ghost nodes)" do + include_context "with isolated db" + + let(:pub_key) { "60e53d9b#{"ab" * 28}" } + + # Mirror the ingestor's bare-advert placeholder (_advert_to_node_dict + + # radio metadata): wall-clock lastHeard, shortName/publicKey, no name/role. + def upsert_advert_placeholder(db, heard_at) + dp.upsert_node(db, "!60e53d9b", { + "lastHeard" => heard_at, + "lora_freq" => 869, + "modem_preset" => "SF8/BW62/CR8", + "user" => { "shortName" => "60e5", "publicKey" => pub_key }, + }, protocol: "meshcore") + end + + # Mirror the follow-up roster contact (_contact_to_node_dict): the full + # record named from the advert, stamped with the sender-side last_advert. + def upsert_contact_record(db, last_advert, long_name: "Alpha Repeater") + dp.upsert_node(db, "!60e53d9b", { + "lastHeard" => last_advert, + "lora_freq" => 869, + "modem_preset" => "SF8/BW62/CR8", + "user" => { + "longName" => long_name, + "shortName" => "60e5", + "publicKey" => pub_key, + "role" => "REPEATER", + }, + }, protocol: "meshcore") + end + + def node_row(db) + db.execute("SELECT * FROM nodes WHERE node_id = '!60e53d9b'").first + end + + it "fills name and role from a contact record stamped seconds older (healthy clock)" do + db = open_db + upsert_advert_placeholder(db, now) + upsert_contact_record(db, now - 17) + row = node_row(db) + db.close + expect(row["long_name"]).to eq("Alpha Repeater") + expect(row["role"]).to eq("REPEATER") + expect(row["public_key"]).to eq(pub_key) + end + + it "fills name and role from a contact record stamped years older (broken node clock)" do + db = open_db + upsert_advert_placeholder(db, now) + upsert_contact_record(db, now - 66_988_974) + row = node_row(db) + db.close + expect(row["long_name"]).to eq("Alpha Repeater") + expect(row["role"]).to eq("REPEATER") + end + + it "keeps the newer last_heard when a stale contact record enriches the row" do + db = open_db + upsert_advert_placeholder(db, now) + upsert_contact_record(db, now - 17) + row = node_row(db) + db.close + expect(row["last_heard"]).to eq(now) + end + + it "does not overwrite an existing name or role with stale data" do + db = open_db + upsert_contact_record(db, now, long_name: "Fresh Name") + dp.upsert_node(db, "!60e53d9b", { + "lastHeard" => now - 3600, + "user" => { "longName" => "Old Name", "shortName" => "OLD", "role" => "COMPANION" }, + }, protocol: "meshcore") + row = node_row(db) + db.close + expect(row["long_name"]).to eq("Fresh Name") + expect(row["role"]).to eq("REPEATER") + end + + it "does not fill a missing name with a stale empty adv_name" do + db = open_db + upsert_advert_placeholder(db, now) + upsert_contact_record(db, now - 17, long_name: "") + row = node_row(db) + db.close + expect(row["long_name"]).to be_nil + end + + it "does not fill a missing short_name with a stale empty shortName" do + db = open_db + dp.upsert_node(db, "!60e53d9b", { "lastHeard" => now }, protocol: "meshcore") + dp.upsert_node(db, "!60e53d9b", { + "lastHeard" => now - 17, + "user" => { "shortName" => "", "longName" => "Alpha Repeater" }, + }, protocol: "meshcore") + row = node_row(db) + db.close + expect(row["short_name"]).to be_nil + expect(row["long_name"]).to eq("Alpha Repeater") + end + + it "still ignores stale synthetic placeholders for real rows" do + db = open_db + upsert_advert_placeholder(db, now) + dp.upsert_node(db, "!60e53d9b", { + "lastHeard" => now - 17, + "user" => { "longName" => "Chat Alias", "synthetic" => true }, + }, protocol: "meshcore") + row = node_row(db) + db.close + expect(row["long_name"]).to be_nil + end + end + # --------------------------------------------------------------------------- # upsert_node — synthetic flag + merge # ---------------------------------------------------------------------------