mirror of
https://github.com/domainaware/parsedmarc.git
synced 2026-09-13 17:38:00 +00:00
Sean WhalenandClaude Fable 5.1
b41230dc07
* fix: make the SIGHUP configuration reload atomic The reload built the replacement output clients, then immediately closed the old ones and swapped them in -- and only afterwards ran seven more fallible steps: load_reverse_dns_map() (which reloads the PSL overrides first), load_ip_db(), configure_ipinfo_api(), the setattr loop onto opts, _build_parser_config(), the watch-parameter int() conversions, and the log-level/FileHandler refresh. An exception in any of them was caught and logged as "Config reload failed, continuing with previous config", which was false: the new clients were already live and receiving reports, the old ones were closed and unrecoverable, opts could be half-applied, and both loaders empty their target before reading anything -- so a bad map path emptied REVERSE_DNS_MAP until some later uncached lookup happened to refill it from the previous configuration's paths (both get_service_from_reverse_dns_base_domain() and get_ip_address_info() reload a map they find empty), and a bad PSL overrides path emptied parsedmarc.utils.psl_overrides without touching the map, so nothing triggered that lazy reload and base domains were folded without the overrides until a restart. The reload is now three phases. Phase 1 stages every fallible step off to the side, in this order: _load_config/_parse_config, _init_output_clients, the derived values (_build_parser_config and the three watch-parameter int()s), load_reverse_dns_map into a fresh dict, load_ip_db, configure_ipinfo_api. The derived values run early because they write nothing at all, and the two loaders that write process-wide state run last, so as little as possible can fail behind them -- load_ip_db()'s download in particular rewrites the shared MMDB cache file before it assigns _IP_DB_PATH, and that is the one phase-1 side effect no snapshot can undo (what it leaves is the same refresh startup performs). The parsedmarc.utils globals the loaders assign are snapshotted by a new _utils_globals_snapshot(). Enumerated from the loaders' bodies, those are psl_overrides (cleared and repopulated in place, no `global` statement -- the list object itself is the state), _IP_DB_PATH (the only global load_ip_db assigns) and _IPINFO_API_TOKEN (the only one configure_ipinfo_api assigns, and assigned before the probe that can fail). _LAST_LOGGED_IP_DB_PATH is deliberately not covered: it is assigned by _get_ip_database_path(), which no loader the reload calls reaches, and it only decides whether the database path is logged again. Phase 2 commits with assignments only, REVERSE_DNS_MAP in place -- cli.py imports that name from parsedmarc, so rebinding it here would desync this module from parsedmarc.REVERSE_DNS_MAP and from the config.REVERSE_DNS_MAP that ParserConfig.__setstate__ rebinds to in a worker, and the ParserConfig staged in phase 1 already holds the pre-reload object. Phase 3 closes the replaced clients last and best effort, so a dying connection cannot undo the commit. On failure the replacement clients are closed, then the search backends' default alias is restored -- that order, because a fully built _ElasticsearchHandle releases the alias as it closes (see #906 and #902) -- and then the utils globals are put back. REVERSE_DNS_MAP, opts, clients, parser_config and the three watch parameters are never written before the commit, so there is nothing to undo for them; the per-step end state is traced in a comment at the rollback site, including which restores are load-bearing at which depth and which are defensive. configure_ipinfo_api's sys.exit(1) on an invalid token still bypasses all of this: SystemExit is not an Exception, and the process is leaving. Closing the replaced log file was the one statement in the commit block that could still raise (a full disk, a stale mount); it is now caught and reported after the handler is detached, since by that point the reloaded configuration is live and rolling back to report a failed reload would be a lie. Nothing runs concurrently with the swap: the reload happens in the main thread between watch_inbox() calls, after the mailbox backend has returned at a batch boundary, so no save callback is in flight, and parallel.py creates and tears down its ProcessPoolExecutor inside a single parallel_map() call -- its workers are separate processes with their own copies of REVERSE_DNS_MAP and the utils globals either way. The previous code mutated the same state at the same point, so this is not a change in exposure. #906 made _init_output_clients() itself all-or-nothing; this does the same for the whole reload. The docs already promised that a failed reload leaves "the previous configuration fully active"; that promise is now true, and usage.md spells out what it covers and the one thing it does not. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix: open the replacement log file during reload staging, not after commit The logging refresh was the one fallible step left after the commit: phase 2 closed and removed the old FileHandler, then tried to open the new log_file inside a try/except that only warned. On an unwritable path the old handler was already gone, file logging was dead, opts.active_log_file was set to the new path anyway, and the reload was logged as successful. Because the old-versus-new comparison uses active_log_file, a later SIGHUP with the same path -- after the operator fixed permissions -- was a no-op, so only a restart recovered file logging. The replacement FileHandler is now constructed in phase 1, after the watch-parameter conversions and before the loaders (constructing it opens the file immediately, and that is the only thing in the refresh that can fail on a bad path). The rollback closes it if a later step fails; phase 2 only attaches it. An unwritable log_file is therefore a reload failure, with the previous log file still attached and receiving the traceback. Startup keeps its warning-only behaviour: there is no previous configuration to fall back to there. Found by Copilot's review of #910. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix: retry a log file that could not be opened at startup on the next reload Startup recorded opts.active_log_file = opts.log_file even when opening the FileHandler failed (a warning there, since there is no previous configuration to keep). The SIGHUP reload only stages a replacement handler when the new config's log_file differs from active_log_file, so a reload naming the same path never retried the open: file logging stayed dead until restart, even after the operator fixed the directory or permissions. active_log_file now starts as None and is set only after the handler is attached, so an unchanged log_file that failed at startup is retried by the next reload, and one that opened at startup is not reopened. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix: make the rollback's staged log handler close best-effort; correct the docs on what aborts a reload FileHandler.close() can raise while flushing or closing (a full disk, a stale network mount). In the reload's rollback that raise escaped the except block: the search-alias and utils-globals restores never ran, the original staging failure was replaced, and the watcher exited. The close is now logged and swallowed like the commit path's close of the replaced handler, so the restores that follow it always run. usage.md said "unreachable output destinations" abort a reload. That is true of PostgreSQL, Kafka, and TCP/TLS syslog, whose clients connect in their constructors, but not of Elasticsearch or OpenSearch: set_hosts() builds the client without connecting, and migrate_indexes() catches every cluster call and warns, so a reload pointing at an unreachable search cluster still commits and fails on the first save. The list now names what actually raises during staging and calls out the exception. Both found by Copilot's second review of #910. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>