Ver código fonte

Add .learnings/ERRORS.md with three recurring Lua footguns

Three silent bug classes caught during the routing/wg/ctl wiring:
  1. gsub('\0', ...) — Lua 5.1 treats \0 as zero-width match; corrupts
     every character of every string field.
  2. X and Y or Z — when Y is false, expression silently falls through
     to Z. Coerces explicit false back to the default. Killed config
     'option icmp_enabled 0' → loaded as true.
  3. local function new() + return mod — new was invisible to callers;
     main.lua boot crashed at 'attempt to call field new (a nil value)'.

End-to-end boot is the only test that catches all three. Per-module
unit tests, lint, and parse checks all passed; only the daemon actually
trying to run exposed them.
netbot 1 mês atrás
pai
commit
890db43dce
1 arquivos alterados com 128 adições e 0 exclusões
  1. 128 0
      .learnings/ERRORS.md

+ 128 - 0
.learnings/ERRORS.md

@@ -0,0 +1,128 @@
+# balancer-lite-lua — Lessons Learned
+
+Three recurring footguns caught during the routing/wg/ctl wiring (commit `daf240f`).
+All three were *silent*: code parsed, tests passed, but end-to-end boot was broken.
+**Per-module unit tests alone don't catch any of them** — they only surface when you
+try to actually use the module from another module or boot the daemon.
+
+---
+
+## 1. `gsub` patterns treat `\0` as zero-width match (Lua 5.1)
+
+**Symptom:** JSON output looked like `{"kind":"\u0000s\u0000w\u0000i\u0000t\u0000c\u0000h\u0000",...}` —
+the `\u0000` text was injected between every character of every string field.
+
+**Cause:** In Lua 5.1, the pattern character `\0` matches a *zero-width* position
+between every character (it's the "match anywhere" anchor, like `\b` in some regex
+flavors). So `s:gsub('\0', 'X')` on `"abc"` returns `"XaXbXcX"`, not `"abc"`.
+
+**Same trap applies to:** `\0` as a character class, and possibly other `\d`-style
+shortcuts. The null byte (0x00) is treated as the special "any position" anchor.
+
+**Fix:** Don't use `\0` in patterns. To match a literal null byte, use `string.find(s,
+'\0', i, true)` with the `plain=true` flag (which disables pattern matching), then
+rebuild the string manually. Example from `src/balancerlite/json.lua`:
+
+```lua
+-- BAD: silent zero-width match
+v:gsub('\0', '\\u0000')
+
+-- GOOD: explicit find + manual rebuild
+local out, i = {}, 1
+while true do
+  local p = string.find(v, '\0', i, true)
+  if not p then out[#out+1] = v:sub(i); break end
+  out[#out+1] = v:sub(i, p-1) .. '\\u0000'
+  i = p + 1
+end
+table.concat(out)
+```
+
+**Reproduction:**
+```lua
+lua5.1 -e "print(('abc'):gsub('\0', 'X'))"
+-- Output: XaXbXcX  4   (NOT 'abc')
+```
+
+---
+
+## 2. `s and get_opt(...) or default` silently coerces `false` → `default`
+
+**Symptom:** UCI option `option icmp_enabled '0'` was loaded back as `true` (the
+default), so the smoke test's "all probes disabled" config actually ran all probes.
+
+**Cause:** Lua's `and`/`or` short-circuit semantics: `A and B or C` evaluates to `B`
+when `A` is truthy, else `C`. When `B` itself is `false` (a valid `bool` opt value),
+the expression returns `C` instead — so an explicit `false` becomes the default.
+
+**Fix:** Never use `X and Y or Z` when `Y` can be falsy. Use an explicit helper:
+
+```lua
+-- BAD: silently breaks for false / 0 / ""
+icmp_enabled = sh and get_opt(sh, "icmp_enabled", true, "bool") or true
+-- When sh is truthy AND get_opt returns false, expression returns the right-side true
+
+-- GOOD: explicit branch
+local function opt(s, k, default, conv)
+  if not s then return default end
+  return get_opt(s, k, default, conv)
+end
+icmp_enabled = opt(sh, "icmp_enabled", true, "bool")
+```
+
+**Rule of thumb:** `A and B or C` is safe only when `B` is guaranteed truthy
+(string, table, non-zero number, or `true`). For all other types use an explicit `if`.
+
+**Reproduction:**
+```lua
+lua5.1 -e "local sh = {_options={enabled='0'}}
+local get_opt = function() return false end
+print(sh and get_opt() or true)"
+-- Output: true  (the default, NOT the configured false)
+```
+
+---
+
+## 3. Module `new()` defined as local but accessed via the module table
+
+**Symptom:** `lua5.1 src/balancerlite/main.lua --dry-run` crashed with
+`attempt to call field 'new' (a nil value)` at the first `mod.new(...)` call.
+
+**Cause:** Two of the existing modules had `local function new(cfg)` instead of
+`function mod.new(cfg)`. The local function existed but was invisible to the
+return table — only `mod.foo` style functions were exposed.
+
+```lua
+-- src/balancerlite/probes.lua  (BEFORE)
+local function new(cfg) ... end
+function probes.all(...) ... end
+return probes          -- probes.new is nil!
+
+-- AFTER
+local function new(cfg) ... end
+function probes.all(...) ... end
+probes.new = new
+probes.all = probes.all  -- self-redundant but makes exports explicit
+return probes
+```
+
+**Fix:** Either (a) define `new` as `function mod.new(cfg)` like the other exported
+functions, or (b) explicitly assign all exports before `return mod`. Option (a) is
+shorter; option (b) gives you a single "public API" section at the bottom.
+
+**Rule:** When defining a module, every function intended to be called from outside
+must be reachable via the return table. `local function foo` is invisible to callers
+unless you re-bind it: `mod.foo = foo` or `function mod.foo(...)`.
+
+---
+
+## General lesson
+
+End-to-end boot/parse/run is the only test that catches cross-module wiring bugs.
+Per-module unit tests catch logic bugs inside the module; lint catches syntax;
+smoke tests catch everything else. **Always do an end-to-end run before committing.**
+
+The wiring work that exposed all three bugs here was: "run the actual daemon in
+dry-run mode after adding the new modules." That single command — `lua5.1 main.lua
+--dry-run` — found three silent failures in ~10 seconds. Cheaper than any amount
+of code review.