fix(binkp): apply config.hjson reloads without a restart (#721)

config.hjson is watched and reloaded at runtime, but the BinkP module read
most of its configuration once during startup and threaded the values into
long-lived closures. The inbound listener in particular captured a BsoSpool,
the local address list, and the whole binkp config block, then reused them
for every connection for the life of the process. After a reload, inbound
sessions kept resolving outbound directories against the old paths and
networks, advertised the old local addresses, and authenticated against the
old node passwords -- silently, until the BBS was restarted. The pull cycle
dialed the node set as it was at boot, and the crashmail debounce window was
fixed at its startup value.

Nothing here needed a new mechanism: binkp.js binds Config as a call-time
indirection already, so the fix is to stop reading early.

  * _handleConnection() resolves the spool, local addresses, node/FREQ
    config, and tempDir per connection -- once at the top, so values can't
    shift mid-session. Both listeners now close over nothing but `this`.
  * _pullAddresses() defaults to the live config; the parameter stays for
    callers evaluating some other node set.
  * the crashmail debounce window is read per burst.

Two things can't be fixed by reading later, and are reconciled on the
ConfigChanged event instead:

  * pullSchedule is compiled into a later.js timer, so the timer is rebuilt
    when the expression changes (and cleared when it is removed).
  * the inbound listener bindings -- enabled, port, address, tls.* -- are
    baked into a socket at listen() time. Rebinding under live sessions is
    not something a reload should do, so a change to those keys is reported
    as restart-required rather than silently ignored.

Also consolidates the three near-identical BsoSpool constructions into a
single buildSpool() in binkp/util.js, whose header already asked for exactly
that ("...so the three callsites can't drift").

Author: NuSkooler

core/binkp/caller.js

@@ -11,28 +11,20 @@ const Events = require('../events.js');
 const configModule = require('../config.js');
 const Address = require('../ftn_address.js');
 const { BinkpSession } = require('./session.js');
-const { BsoSpool, attachSpoolToSession } = require('./bso_spool.js');
-const { localAddresses, addressKey, findBestNodeMatch } = require('./util.js');
+const { attachSpoolToSession } = require('./bso_spool.js');
+const {
+    localAddresses,
+    addressKey,
+    findBestNodeMatch,
+    buildSpool,
+} = require('./util.js');
 
 const Config = () => configModule.get();
 
 const CONNECT_TIMEOUT_MS = 30_000;
 
 // ── Helpers ───────────────────────────────────────────────────────────────────
 
-function buildSpool() {
-    const config = Config();
-    return new BsoSpool({
-        paths: _.get(config, 'scannerTossers.ftn_bso.paths'),
-        networks: _.get(config, 'messageNetworks.ftn.networks', {}),
-        defaultNetwork: _.get(config, 'scannerTossers.ftn_bso.defaultNetwork'),
-        staleLockMaxAgeMs: _.get(
-            config,
-            'scannerTossers.ftn_bso.binkp.staleLockMaxAgeMs'
-        ),
-    });
-}
-
 // Returns the most-specific node config entry whose pattern matches |addr|.
 // Specificity is by Address#getMatchScore — concrete-and-matching parts beat
 // wildcards — so a "21:1/100" override always wins over a "21:*" catch-all
@@ -142,7 +134,7 @@ async function pollNodes(forceAddrs, cb) {
         return cb(null);
     }
 
-    const spool = buildSpool();
+    const spool = buildSpool(config);
 
     let pendingAddrs;
     try {

core/binkp/util.js

@@ -2,6 +2,7 @@
 
 const _ = require('lodash');
 const Address = require('../ftn_address.js');
+const { BsoSpool } = require('./bso_spool.js');
 
 //  Helpers shared across the BinkP module surface (caller, scanner_tosser).
 //  Anything reaching for `messageNetworks.ftn.networks` or the in-memory
@@ -66,4 +67,24 @@ function findBestNodeMatch(nodes, addr) {
     return bestConf;
 }
 
-module.exports = { localAddresses, addressKey, findBestNodeMatch };
+//
+//  A BsoSpool for the current configuration.
+//
+//  Build one at the point of use and throw it away -- never cache it in a
+//  closure. config.hjson is hot-reloadable, so a poll or an inbound session
+//  starting after a reload has to honour the new paths and networks. Holding
+//  a spool built at startup silently pins the process to boot-time config.
+//
+//  |config| is the full Config() object, as with localAddresses() above.
+//
+function buildSpool(config) {
+    const ftnBsoCfg = _.get(config, 'scannerTossers.ftn_bso', {});
+    return new BsoSpool({
+        paths: ftnBsoCfg.paths,
+        networks: _.get(config, 'messageNetworks.ftn.networks', {}),
+        defaultNetwork: ftnBsoCfg.defaultNetwork,
+        staleLockMaxAgeMs: _.get(ftnBsoCfg, 'binkp.staleLockMaxAgeMs'),
+    });
+}
+
+module.exports = { localAddresses, addressKey, findBestNodeMatch, buildSpool };

core/scanner_tossers/binkp.js

@@ -15,9 +15,14 @@ const configModule = require('../config.js');
 const Address = require('../ftn_address.js');
 const { MessageScanTossModule } = require('../msg_scan_toss_module.js');
 const { BinkpSession } = require('../binkp/session.js');
-const { BsoSpool, attachSpoolToSession } = require('../binkp/bso_spool.js');
+const { attachSpoolToSession } = require('../binkp/bso_spool.js');
 const { pollNodes } = require('../binkp/caller.js');
-const { localAddresses, addressKey, findBestNodeMatch } = require('../binkp/util.js');
+const {
+    localAddresses,
+    addressKey,
+    findBestNodeMatch,
+    buildSpool,
+} = require('../binkp/util.js');
 const { FreqResolver, attachFreqToSession } = require('../binkp/freq.js');
 
 const Config = () => configModule.get();
@@ -28,6 +33,20 @@ const Config = () => configModule.get();
 //  scannerTossers.ftn_bso.binkp.crashmailDebounceMs.
 const DEFAULT_CRASHMAIL_DEBOUNCE_MS = 500;
 
+//  Settings baked into a socket at listen() time. Unlike everything else in
+//  the binkp block, these cannot be picked up by re-reading config: applying
+//  them means tearing down and rebinding a listener, potentially under live
+//  sessions. A reload that touches them is reported instead.
+const LISTENER_BOUND_KEYS = [
+    'inbound.enabled',
+    'inbound.port',
+    'inbound.address',
+    'inbound.tls.enabled',
+    'inbound.tls.port',
+    'inbound.tls.certFile',
+    'inbound.tls.keyFile',
+];
+
 //  Inbound temp file (binkp_in_*.dt) startup-sweep age threshold. Anything
 //  older than this in tempDir at startup is treated as a leaked partial from
 //  a prior crashed session and removed. Tunable via
@@ -85,6 +104,9 @@ exports.getModule = class BinkpModule extends MessageScanTossModule {
         this._crashmailTimer = null;
         this._crashmailPending = new Map(); // zone:net/node -> Address
         this._crashmailListener = null;
+        this._configChangedListener = null;
+        this._listenerConfig = null; // boot-time LISTENER_BOUND_KEYS snapshot
+        this._pullSchedule = null;
     }
 
     startup(cb) {
@@ -97,11 +119,12 @@ exports.getModule = class BinkpModule extends MessageScanTossModule {
 
         async.series(
             [
-                callback => this._reapStaleLocks(binkpCfg, ftnBsoCfg, callback),
+                callback => this._reapStaleLocks(callback),
                 callback => this._reapInboundTemps(binkpCfg, callback),
-                callback => this._startInbound(binkpCfg, ftnBsoCfg, callback),
+                callback => this._startInbound(binkpCfg, callback),
                 callback => this._startPullSchedule(binkpCfg, callback),
-                callback => this._startCrashmailListener(binkpCfg, callback),
+                callback => this._startCrashmailListener(callback),
+                callback => this._startConfigWatch(binkpCfg, callback),
             ],
             cb
         );
@@ -117,6 +140,13 @@ exports.getModule = class BinkpModule extends MessageScanTossModule {
             this._crashmailTimer = null;
         }
         this._crashmailPending.clear();
+        if (this._configChangedListener) {
+            Events.removeListener(
+                Events.getSystemEvents().ConfigChanged,
+                this._configChangedListener
+            );
+            this._configChangedListener = null;
+        }
         if (this._crashmailListener) {
             Events.removeListener(
                 Events.getSystemEvents().NewOutboundBSO,
@@ -154,14 +184,8 @@ exports.getModule = class BinkpModule extends MessageScanTossModule {
     //  Sweep orphaned .bsy locks left by a prior crashed run before we start
     //  anything that depends on them. Runs unconditionally — outbound sessions
     //  acquire locks too, so this matters even when inbound is disabled.
-    _reapStaleLocks(binkpCfg, ftnBsoCfg, cb) {
-        const spool = new BsoSpool({
-            paths: ftnBsoCfg.paths,
-            networks: _.get(Config(), 'messageNetworks.ftn.networks', {}),
-            defaultNetwork: ftnBsoCfg.defaultNetwork,
-            staleLockMaxAgeMs: binkpCfg.staleLockMaxAgeMs,
-        });
-        spool
+    _reapStaleLocks(cb) {
+        buildSpool(Config())
             .reapStaleLocks()
             .then(reaped => {
                 if (reaped > 0) {
@@ -208,23 +232,17 @@ exports.getModule = class BinkpModule extends MessageScanTossModule {
             });
     }
 
-    _startInbound(binkpCfg, ftnBsoCfg, cb) {
+    _startInbound(binkpCfg, cb) {
         const inbound = _.get(binkpCfg, 'inbound', {});
         if (!inbound.enabled) return cb(null);
 
-        const spool = new BsoSpool({
-            paths: ftnBsoCfg.paths,
-            networks: _.get(Config(), 'messageNetworks.ftn.networks', {}),
-            defaultNetwork: ftnBsoCfg.defaultNetwork,
-            staleLockMaxAgeMs: binkpCfg.staleLockMaxAgeMs,
-        });
-
-        const addresses = localAddresses(Config());
-        const tempDir = _.get(binkpCfg, 'tempDir', os.tmpdir());
         const bindAddress = inbound.address || '0.0.0.0';
 
+        //  Only the binding itself is fixed for the life of the socket.
+        //  Everything a session reads is resolved per connection in
+        //  _handleConnection() so a config reload applies without a restart.
         this._server = net.createServer(socket => {
-            this._handleConnection(socket, spool, addresses, binkpCfg, tempDir);
+            this._handleConnection(socket);
         });
         this._server.on('error', err => {
             Log.error({ error: err.message }, '[BinkP] Server error');
@@ -257,7 +275,7 @@ exports.getModule = class BinkpModule extends MessageScanTossModule {
         Promise.all([fsp.readFile(tlsCfg.certFile), fsp.readFile(tlsCfg.keyFile)])
             .then(([cert, key]) => {
                 this._tlsServer = tls.createServer({ cert, key }, socket => {
-                    this._handleConnection(socket, spool, addresses, binkpCfg, tempDir);
+                    this._handleConnection(socket);
                 });
                 this._tlsServer.on('error', err => {
                     Log.error({ error: err.message }, '[BinkP] TLS server error');
@@ -312,7 +330,7 @@ exports.getModule = class BinkpModule extends MessageScanTossModule {
         this._pullTimer = later.setInterval(() => {
             if (polling) return;
             polling = true;
-            const addrs = this._pullAddresses(binkpCfg);
+            const addrs = this._pullAddresses();
             Log.info({ count: addrs.length }, '[BinkP] Scheduled pull cycle starting');
             pollNodes(addrs, () => {
                 polling = false;
@@ -331,13 +349,7 @@ exports.getModule = class BinkpModule extends MessageScanTossModule {
     //  the same peer turns into one session, not N. After the debounce
     //  window elapses, dial whatever set of addresses accumulated.
     //
-    _startCrashmailListener(binkpCfg, cb) {
-        const debounceMs = _.get(
-            binkpCfg,
-            'crashmailDebounceMs',
-            DEFAULT_CRASHMAIL_DEBOUNCE_MS
-        );
-
+    _startCrashmailListener(cb) {
         this._crashmailListener = ({ address }) => {
             //  Drop malformed events: a crashmail entry with an undefined
             //  net/node would dedupe to "0:undefined/0" and corrupt the
@@ -351,6 +363,13 @@ exports.getModule = class BinkpModule extends MessageScanTossModule {
             this._crashmailPending.set(key, address);
 
             if (this._crashmailTimer) return; // window already open
+
+            //  Read per burst so a reload retunes the coalescing window
+            const debounceMs = _.get(
+                Config(),
+                'scannerTossers.ftn_bso.binkp.crashmailDebounceMs',
+                DEFAULT_CRASHMAIL_DEBOUNCE_MS
+            );
             this._crashmailTimer = setTimeout(() => {
                 this._crashmailTimer = null;
                 const addrs = Array.from(this._crashmailPending.values());
@@ -374,10 +393,75 @@ exports.getModule = class BinkpModule extends MessageScanTossModule {
         return cb(null);
     }
 
+    //
+    //  config.hjson is hot-reloadable. Almost everything this module needs is
+    //  read at the point of use -- per inbound connection, per poll, per
+    //  crashmail burst -- so a reload applies on its own. Two things can't
+    //  work that way and are reconciled here:
+    //
+    //    * the pull schedule, which is compiled into a timer -> rebuild it
+    //    * the inbound listener bindings, which are baked into a socket ->
+    //      report that a restart is needed rather than silently ignoring them
+    //
+    _startConfigWatch(binkpCfg, cb) {
+        this._listenerConfig = this._listenerSnapshot(binkpCfg);
+        this._pullSchedule = binkpCfg.pullSchedule;
+
+        this._configChangedListener = () => this._applyConfigChange();
+        Events.addListener(
+            Events.getSystemEvents().ConfigChanged,
+            this._configChangedListener
+        );
+        return cb(null);
+    }
+
+    _listenerSnapshot(binkpCfg) {
+        return LISTENER_BOUND_KEYS.reduce((snapshot, key) => {
+            snapshot[key] = _.get(binkpCfg, key);
+            return snapshot;
+        }, {});
+    }
+
+    _applyConfigChange() {
+        const binkpCfg = _.get(Config(), 'scannerTossers.ftn_bso.binkp', {});
+
+        if (binkpCfg.pullSchedule !== this._pullSchedule) {
+            this._pullSchedule = binkpCfg.pullSchedule;
+            if (this._pullTimer) {
+                this._pullTimer.clear();
+                this._pullTimer = null;
+            }
+            //  _startPullSchedule logs the outcome, including a disabled or
+            //  unparsable schedule, and never errors
+            this._startPullSchedule(binkpCfg, () => {});
+            Log.info(
+                { schedule: binkpCfg.pullSchedule || null },
+                '[BinkP] Pull schedule reloaded'
+            );
+        }
+
+        //  Compared against the boot-time snapshot rather than the previous
+        //  reload: the mismatch stands until the BBS is restarted, so it is
+        //  still worth saying on a later reload.
+        const current = this._listenerSnapshot(binkpCfg);
+        const changed = LISTENER_BOUND_KEYS.filter(
+            key => !_.isEqual(current[key], this._listenerConfig[key])
+        );
+        if (changed.length > 0) {
+            Log.warn(
+                { changed },
+                '[BinkP] Inbound listener settings changed; restart required for them to take effect'
+            );
+        }
+    }
+
     //  Build the list of addresses for a pull cycle: every entry in
     //  binkp.nodes whose pattern parses as a concrete address (not a
     //  wildcard) and whose config doesn't set `pull: false`.
+    //  |binkpCfg| defaults to the live config; pass one explicitly only to
+    //  evaluate a set of nodes other than what is currently configured.
     _pullAddresses(binkpCfg) {
+        binkpCfg = binkpCfg || _.get(Config(), 'scannerTossers.ftn_bso.binkp', {});
         const nodes = binkpCfg.nodes || {};
         const out = [];
         for (const [pattern, conf] of Object.entries(nodes)) {
@@ -395,10 +479,20 @@ exports.getModule = class BinkpModule extends MessageScanTossModule {
         return out;
     }
 
-    async _handleConnection(socket, spool, addresses, binkpCfg, tempDir) {
+    async _handleConnection(socket) {
         const remote = `${socket.remoteAddress}:${socket.remotePort}`;
         Log.info({ remote }, '[BinkP] Inbound connection');
 
+        //  Resolved per connection, not per process: config.hjson is
+        //  hot-reloadable and a session opened after a reload must honour the
+        //  new spool paths, local addresses, node passwords and FREQ config.
+        //  Read once here so the values can't shift mid-session.
+        const config = Config();
+        const binkpCfg = _.get(config, 'scannerTossers.ftn_bso.binkp', {});
+        const spool = buildSpool(config);
+        const addresses = localAddresses(config);
+        const tempDir = _.get(binkpCfg, 'tempDir', os.tmpdir());
+
         const session = new BinkpSession(socket, {
             role: 'answering',
             addresses,

docs/_docs/messageareas/binkp.md

@@ -242,6 +242,26 @@ freq: {
 
 With this setup, every new nodelist that arrives via TIC is immediately FREQ-serveable — no path configuration to maintain.
 
+#### Reloading configuration
+
+`config.hjson` is watched and reloaded while the BBS runs. Almost everything under `binkp` is read at the moment it is used, so a reload applies on its own:
+
+| Setting | Applies to | When it takes effect |
+|---------|-----------|----------------------|
+| `nodes` (hosts, ports, passwords, `pull`, TLS) | next session, inbound or outbound | Immediately |
+| `freq` | next inbound session | Immediately |
+| `crashmailDebounceMs` | next crashmail burst | Immediately |
+| `tempDir`, `staleLockMaxAgeMs` | next session | Immediately |
+| `ftn_bso` `paths` and `messageNetworks.ftn.networks` | next session or poll | Immediately |
+| `pullSchedule` | the pull timer | On reload — the timer is rebuilt |
+| `inbound.enabled`, `inbound.port`, `inbound.address`, `inbound.tls.*` | the listening socket | **Restart required** |
+
+The last row is bound into the socket when it starts listening; changing it under live sessions is not something a reload can do safely. A reload that touches those keys logs a warning naming them so the change isn't silently lost:
+
+```
+[BinkP] Inbound listener settings changed; restart required for them to take effect
+```
+
 ---
 
 ### How it works with `ftn_bso`