Support config file and graceful reload (SIGHUP)#50
Conversation
Greptile SummaryThis PR refactors all mutable config globals into a single Confidence Score: 4/5Safe to merge after fixing the unhandled ValueError in STORAGE_UPLOAD_THRESHOLD parsing, which is a new startup-crash regression introduced by this PR. The core design is well-executed - atomic ReloadableConfig swap, per-message snapshot in reset(), mode-switch guards, and missing-file abort all work correctly. One P1 remains: the int() cast for the newly-configurable storage_upload_threshold has no error handling and will hard-crash the milter on an invalid config value (this path did not exist before this PR). The test teardown issue in TestEndToEndConfigFile is P2 and does not cause current test failures since it is the last class in the file. milter/primitivemail_milter.py line 419 (storage_upload_threshold int() cast), test_milter.py TestEndToEndConfigFile.test_webhook_uses_config_file_url teardown Important Files Changed
Sequence DiagramsequenceDiagram
participant OS as OS (SIGHUP)
participant RC as reload_config()
participant AC as _apply_config(reloadable_only=True)
participant Global as Module globals
participant Handler as Handler thread
OS->>RC: SIGHUP signal
RC->>RC: _read_config_file(CONFIG_FILE_PATH)
alt file was present at startup but is now missing
RC-->>OS: abort (preserve current state)
else file OK or never existed
RC->>AC: _apply_config(file_data, reloadable_only=True)
AC->>AC: build new ReloadableConfig
AC->>AC: Guard: mode-switch check
AC->>AC: Guard: webhook_url without webhook_secret
AC->>Global: _rcfg = new_cfg (GIL-atomic swap)
AC-->>RC: return
end
Note over Handler: self._cfg snapshotted in reset() before SIGHUP
Handler->>Handler: uses self._cfg for full message duration
Note over Handler: Next message reset() picks up new _rcfg
Reviews (4): Last reviewed commit: "Fix stale env var name in module docstri..." | Re-trigger Greptile |
| STORAGE_UPLOAD_THRESHOLD = int(_cfg(file_data, 'storage_upload_threshold', | ||
| 'STORAGE_UPLOAD_THRESHOLD', '3000000')) |
There was a problem hiding this comment.
Unguarded
int() cast will crash the milter at startup on bad input
storage_upload_threshold was previously hardcoded (3_000_000); this PR is the first time it's read from the config file or an env var. A config file with "storage_upload_threshold": "unlimited" (or any non-integer string) raises an unhandled ValueError inside _apply_config, killing the process before it can serve any connections.
Consider wrapping with a try/except and logging a warning + falling back to the default:
try:
STORAGE_UPLOAD_THRESHOLD = int(_cfg(file_data, 'storage_upload_threshold',
'STORAGE_UPLOAD_THRESHOLD', '3000000'))
except (ValueError, TypeError):
logger.warning("Invalid storage_upload_threshold value - using default 3000000")
STORAGE_UPLOAD_THRESHOLD = 3_000_000
Summary
/etc/primitive/milter.json) in addition to environment variables. Config file values take precedence; missing keys fall back to env vars. If no config file exists, behavior is identical to before.Why
kill -HUP) that ops teams expectTest plan
kill -HUP <pid>→ verify webhook_url changes, mydomain doesn't