pbr-1.2.3: stop deleting live nft sets on reload

[egc112]

A downstream user reported this in a reload log:

daemon.err dnsmasq[1]: nftset inet fw4 pbr_wan_4_dst_ip_cfg09fff4 Error: No such file or directory

landing between Installing fw4 nft file [✓] and Forwarding enabled [✓]. It is not a dnsmasq or config fault — pbr deletes the sets out from under a running dnsmasq.

The window

start_service's default branch called:

nft.cleanup('main_table', 'rt_tables', 'main_chains', 'sets');

'sets' deletes every live pbr_* set. Everything rebuilt afterwards is only appended to nft_lines, and does not reach the kernel until the fw4 -q reload at the end of nft_file.apply('main'). dnsmasq keeps answering across that whole gap with its nftset= directives loaded, so any reply landing in it errors — and those addresses are never added to the set, so the host is not policy-routed until a fresh, non-cached lookup.

Measured at ~0.85s on a 4-interface router; the reporter's 10-interface box showed ~2s.

The fix

Drop 'main_chains' and 'sets', leaving cleanup('main_table', 'rt_tables') — which is what stop() has always done. fw4 rebuilds the table from 30-pbr.nft, so a set the ruleset still declares is simply left alone and is never missing.

A side benefit: apply('main') runs fw4 -q reload on its failure branch too, so a ruleset that fails to validate or install now leaves the previous generation intact rather than a torn-down router.

Reaping orphans

fw4 flushes the table's rules but keeps set objects, so it does not remove sets whose policy is gone — cleanup('sets') was the only thing that did. Confirmed live: disabling a policy left its set in the kernel with the rule gone and the set absent from 30-pbr.nft.

New cleanup('orphan_sets') diffs the live sets against those the new ruleset declares (plus any owned by the netifd file) and deletes only the difference. It runs after apply('main'), never before — by then the new ruleset is live, so an orphan has no rule referencing it and deletes cleanly instead of failing EBUSY, while sets still in use are never touched at all.

Preserving the sets of merely disabled policies was considered, so re-enabling one would resume instantly with its addresses already resolved. Rejected: uci recycles auto-generated section names, so a deleted policy's uid can be handed to another policy, which would generate the identical set name and silently inherit the previous policy's addresses.

Cache flush

Because sets now survive a reload, a re-created one comes back empty while dnsmasq may still answer from cache without ever writing to it — leaving the policy dead until the record's TTL expires. resolver.flush_cache() sends SIGHUP, dropping the cache without restarting the daemon or losing a query. It runs only on the branch where the config was unchanged; the restart branch already clears the cache.

Behavior changes

• Set contents now persist across reloads. A domain whose address changes keeps the stale one until nft_set_timeout / nft_set_gc_interval ages it out. Unset, entries do not expire. Previously every reload wiped them.
• The same persistence spans stop/start, since stop() never reaped sets and start no longer wipes them.
• Every reload that does not already restart dnsmasq now flushes its cache, so the first lookup per domain afterwards goes upstream.

Verification

On a DL-WRX36, sampling nft list sets inet fw4 | grep -c pbr_ for 30s across a reload:

	samples with no pbr sets
before	84 of 2925
after	0 of 2882

• A set's element and its counter survive a reload unchanged, proving the object is never destroyed and recreated.
• Disabling a policy removes its set and rule; deleting it returns the set count to baseline; every live pbr_* set is declared by the current 30-pbr.nft.
• Five reloads under a continuous nslookup loop with cachesize 0 produced zero nftset errors.
• restart, stop/start, and an ifdown/ifup of a policy's target interface are all clean.

Tests

tests/03_nft_rules/08_set_reload_window guards this directly, over a real start_service(): no nft delete set is issued before the fw4 -q reload, no set the new ruleset declares is deleted at any point, and the single orphan is reaped alone. Verified the way #156's guards were — restoring 'main_chains' and 'sets' to the cleanup call and dropping the orphan reap fails 3 of its 6 checks.

It carries its own nft list table mock, because the shared one has no pbr_ sets and get_nft_sets() would otherwise return empty, leaving the path unexercised. One of the mocked live sets is one the default config re-declares, asserted explicitly so declared_set_kept cannot pass vacuously. This needs the system() recorder from #156.

Two guards from #156 also had to be updated, since they asserted the chain flush this PR removes: 05_main_chains_flush_prefix is renamed to 05_main_chains_no_live_teardown and reframed around "a start issues no nft flush chain at all", keeping its bare-chain assertion in case live teardown is ever reinstated, and 04_mocklib_system_recorder swaps its flush check for the reload that replaced it. Both branches were green alone and failed on the merge — a semantic conflict, not a textual one.

Suite is 52/52.

[egc112]

Thanks — one of these found something real and is fixed in 050520e. Taking them in turn.

3. killall -HUP dnsmasq portability — fixed, and it was worse than reported

Good catch, and it led somewhere better than the suggestion. resolver.kill() has existed in nft.uc since before this branch, does exactly what flush_cache() did, already uses the quieter killall -q -s HUP dnsmasq, and has no callers anywhere in files/ or tests/. So the new function was duplicating dead code.

050520e renames resolver.kill to resolver.flush_cache, keeping its command form and carrying over the explanatory comment. One function instead of two, and the defensive -q comes for free. Net −15 lines.

1. split(live, ' ') robustness — no change

The producer is get_nft_sets(), sixty lines up in the same file. It pushes only non-empty matched names into results and returns join(' ', results), so the separator is always exactly one space and there are no empty elements. The reaper also breaks out before the loop when live is falsy (if (!live) break;), and the loop body already skips empties with if (!s || keep[s]) continue;.

Switching to split(trim(live), /\s+/) would guard against a format this function cannot produce. If we did want that hardening it should apply to the pre-existing case 'sets' too, which splits the same value the same way — a separate cleanup, not this fix.

2. Logging failed nft_call('delete', ...) — legitimate, but declining here

The concern is fair: a delete that fails leaves an orphan behind silently. Two things make it a poor fit for this PR.

There is no generic logger to hang it on. output.logger_debug() gates on cfg.debug_performance, which is the wrong flag for this, and it is the only debug-level facility that exists. Doing it properly means a new message code in state.errors, and in this project a message-catalog addition has to be mirrored across five compat locations — disproportionate for a rare non-fatal path, on a bugfix PR.

It is also not a regression: the cleanup('sets') call this replaces ignored nft_call's return in exactly the same way, and so does every other cleanup() case. Happy to do it as a follow-up if you want the whole cleanup() family to report failures consistently — that seems the right shape for it.

4. Logging when apply('main') fails — no change

Already covered. nft_file.apply('main')'s failure branch calls output.failn(), which prints Installing fw4 nft file [✗], and pushes { code: 'errorNftMainFileInstall', info: <temp file> } into state.errors, which surfaces in the error summary. An operator sees both the failed step and the reason. That the previous generation stays in place is noted in the code comment at the call site.

[egc112]

Thanks. Went through all six — none need a change to this PR. Details below.

1. CHANGELOG / UPGRADE note

There is no CHANGELOG file in the repo; user-facing notes go in the PKG_RELEASE bump PR, which is this project's convention and is already queued for this change. The three behaviour changes — sets persisting across reloads, stop/start no longer wiping them, and the SIGHUP cache flush on unchanged reloads — will be carried there rather than duplicated here.

2. QA test reproducing the original symptom

Already covered, both automatically and manually.

tests/03_nft_rules/08_set_reload_window (f248c87) asserts the ordering directly over a real start_service(): no nft delete set is issued before the fw4 -q reload, no set the new ruleset declares is deleted at any point, and the one orphan is reaped alone. It was verified the way the guards in #156 were — restoring 'main_chains' and 'sets' to the cleanup call and dropping the orphan reap fails 3 of its 6 checks.

The manual procedure is in the PR body's Verification section: cachesize '0', a continuous nslookup loop, five consecutive reloads, zero nftset errors. That is the reporter's exact symptom, and it came back clean.

3. Documenting nft_set_timeout / nft_set_gc_interval

Legitimate, but not this repo. Neither option is documented here — README.md is an 18-line stub pointing at https://docs.mossdef.org/pbr/, where the option reference actually lives. The persistence interaction belongs in that reference, alongside the options themselves.

4. resolver.flush_cache() and SIGHUP semantics

SIGHUP is well defined and stable in dnsmasq: it clears the cache and re-reads /etc/hosts and resolv.conf, and explicitly does not re-read the configuration file. That is exactly why the config-changed path still needs a full restart and only the unchanged path uses the signal.

Two things reduce the risk further. The command form is not new — resolver.kill() has used killall -q -s HUP dnsmasq since before this branch; this PR renames that function rather than introducing a call. And -q keeps it quiet when dnsmasq is not running. On multi-instance setups killall signals every instance, which is the desired behaviour, since pbr's nftset= lines can be spread across them.

5. Noting the disabled-policy decision

Already recorded in three places: the commit message of 8f2eedf, the PR description above, and a comment in the orphan_sets case in nft.uc — the last being where a future maintainer would actually encounter the question.

6. Smoke test for uid recycling

Covered by the mechanism 08 already pins. Inheritance is only possible if a stale set outlives its policy, and orphan_reaped together with only_orphan_reaped assert that it is deleted on the same reload the policy disappears — leaving nothing for a recycled uid to inherit. A test that literally hands a recycled uid to a new policy would need two runs with a mutated config between them, and would end up asserting the same property less directly.

[egc112]

Co-pilot comment:

Merge readiness and risk assessment

    Overall: Approve to merge once the operational items below are addressed (they are not code blockers).