Commit a4d3230
authored
Fix RemoveAllNodeEvents callers passing only nodeid (silent no-op cleanup) (Ylianst#7799)
`RemoveAllNodeEvents(domain, nodeid)` in every db.js backend starts with
`if ((domain == null) || (nodeid == null)) return;` so a caller that passes
only one argument leaves `nodeid` undefined and the early-return swallows
the whole call. As a result, every device-removal path that should also be
clearing the device's event history has been silently leaving those rows
behind on every backend (NeDB/MongoDB TTL-expires them eventually; SQLite,
PostgreSQL, MariaDB/MySQL and AceBase keep them until manual cleanup).
Four callers fixed here:
db.js:244 // removeInactiveDevices
meshcentral.js:1101 // test/diagnostic-agent cleanup
meshuser.js:2855 // user-triggered manual device removal
meshagent.js:101 // agent self-removal on close (temporary/recovery flags)
For each, the domain string is already in scope -- `node.domain` on the
node-iteration paths and `domain.id` in meshagent (parameter at line 18 of
CreateMeshAgent).
A fifth caller exists at meshipkvm.js:244 with the same shape, but it is
deliberately left for a follow-up. The IP-KVM auto-remove subsystem
(Raritan / WebPowerSwitch, gated on the per-mesh auto-remove flag) is
niche enough that I could not personally verify it and would rather not
patch a path I cannot test.
Loosely related (not duplicates): Ylianst#7787 / Ylianst#7788 / Ylianst#7246 are about the
time-based event-retention sweep being slow on large Postgres event
tables. The per-device cleanup fixed here is a separate code path, but
it contributes to the same observed symptom (event tables on SQL
backends growing larger than expected) since every device removal since
the bug landed has been skipping its event history.
== How this was verified ==
Tested locally with a small harness using Node's built-in `node --test`
(no new dependencies). Kept local to my fork rather than introducing a
`test/` directory in MeshCentral; happy to send the harness as a separate
PR if useful.
1. Arity scan: walks every .js file in the repo, finds every
`RemoveAllNodeEvents(...)` call (paren-depth-aware, comment- and
string-aware), and asserts each has exactly 2 args. Run on master,
the test reports 5 buggy call sites. After this fix, only the
intentionally-deferred meshipkvm.js:244 site remains -- the test
makes that one easy to revisit.
2. Function-contract test: pins the canonical shape -- null/undefined
domain or nodeid short-circuits (the F-DB-08 shape), valid pair
reaches the datastore exactly once with the right query, and empty
string '' (the default-domain id) is passed through (the guard is
`== null`, not `!domain`, so '' is not nullish).1 parent 15ff3af commit a4d3230
4 files changed
Lines changed: 4 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
241 | 241 | | |
242 | 242 | | |
243 | 243 | | |
244 | | - | |
| 244 | + | |
245 | 245 | | |
246 | 246 | | |
247 | 247 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
98 | 98 | | |
99 | 99 | | |
100 | 100 | | |
101 | | - | |
| 101 | + | |
102 | 102 | | |
103 | 103 | | |
104 | 104 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1098 | 1098 | | |
1099 | 1099 | | |
1100 | 1100 | | |
1101 | | - | |
| 1101 | + | |
1102 | 1102 | | |
1103 | 1103 | | |
1104 | 1104 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2852 | 2852 | | |
2853 | 2853 | | |
2854 | 2854 | | |
2855 | | - | |
| 2855 | + | |
2856 | 2856 | | |
2857 | 2857 | | |
2858 | 2858 | | |
| |||
0 commit comments