tincand: attribute external SIGTERM/SIGINT senders, fix shutdown hang#178
Merged
Conversation
…an-97mlk.6) Block SIGINT/SIGTERM process-wide in main() via pthread_sigmask, then attribute and consume them in a dedicated waiter thread via sigwaitinfo, which (unlike signal.signal()) exposes si_pid/si_uid. Logs the sender pid/uid and a bounded /proc parent-chain at WARNING before triggering the existing loop.quit() shutdown. The parent-chain lookup bounds depth (8 -> 4) and truncates each cmdline (200 chars) and the overall string (1000 chars): an earlier unbounded version produced a WARNING line large enough to fill a piped stderr that nothing was draining, deadlocking the waiter thread inside logging's stream.write() and hanging shutdown entirely (root-caused via py-spy; was intermittently failing all 9 tests in test_dbus_client_live.py). Co-Authored-By: Claude Sonnet 5 <[email protected]>
…k.6.1) Covers _block_shutdown_signals, _proc_cmdline, _proc_ppid, _describe_signal_sender, _signal_waiter, _start_signal_waiter per the tincan-97mlk.6.1 acceptance criteria. Verified against the real implementation on builder/tincan-97mlk.6 (16 passed, 1 xfailed pending a reviewer call on quit()'s exception-resilience); fails to collect on this branch until tincan-97mlk.6 merges, which is expected.
…g (tincan-97mlk.6.2) Addresses reviewer REQUEST-CHANGES on tincan-97mlk.6: _signal_waiter called loop.quit() unconditionally after the WARNING attribution log with no try/except, so an exception in that path (e.g. a malicious cmdline value tripping _proc_ppid's strict decode) skipped quit() and hung daemon shutdown entirely — the same class of bug tincan-97mlk.6 was meant to fix. Moves loop.quit() into a finally so it always runs. Also aligns _proc_ppid's /proc/<pid>/status read with _proc_cmdline's existing errors="replace" pattern instead of a strict decode. Un-xfails test_quit_still_called_if_warning_log_raises (tincan-97mlk.6.1) now that it passes, and loosens two read_text mocks in the same file to accept the errors= kwarg the real call now passes. adr_check: skipped — actual CLI not installed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
tincandpreviously handled SIGINT/SIGTERM with a plainsignal.signal()handler that logged only "SIGTERM received — shutting down" and quit the
GLib main loop. When the daemon received an external SIGTERM (e.g. a stray
systemctl stopor another process sending the signal) there was no wayto tell who sent it, and — in the case this was built to fix — the
shutdown path could hang entirely.
Signal handling now blocks SIGINT/SIGTERM process-wide via
signal.pthread_sigmask()as the second statement inmain(), before anythread is spawned, and a dedicated daemon thread consumes the signal with
signal.sigwaitinfo(). Unlike asignal.signal()handler,sigwaitinfo()exposes the sender's pid/uid, so the shutdown log now includes a bounded
parent-process chain (e.g.
pid=1234 cmdline='systemctl stop tincand' <- pid=1 cmdline='systemd') for attribution.Why one PR
N/A — single-bead fix, one cohesive change (signal handling + its tests).
Review notes
packaging/systemd/tincand.serviceis intentionally untouched. Theservice file's own
ExecStopPostattribution attempt was a dead end;the in-process
sigwaitinfoapproach replaces it entirely.max_depth=4ancestors, 200 chars perprocess, 1000 chars overall — roughly 1KB) specifically to avoid
recreating the original bug: an unbounded WARNING log line filling a
piped stdout/stderr and blocking the logging call itself, which is what
caused the shutdown hang in the first place.
loop.quit()runs in afinallyaround the attribution logging so shutdown always completeseven if that logging path raises.
/procreads (_proc_cmdline,_proc_ppid) are best-effort andtolerate the sender process already being gone by the time we read its
entry — they fall back to
<unavailable>rather than raising.Test plan
pytest tests/ -q— 2463 passed, 1 skipped, 0 failedpytest tests/tincand/test_main_signal_attribution.py tests/tincand/test_main.py tests/tincand/test_main_args.py -q— 60 passedSIGTERMto a throwaway process with attribution logging forced to raise — shutdown still completed (recreates the original hang's failure mode, not just the mocked unit test)release-gates/tincand-signal-attribution-97mlk.6-gate.md