Skip to content

Remove SIGTERM handler override in AM gather and send plugins - #1653

Open
robjarawan wants to merge 3 commits into
MetPX:developmentfrom
robjarawan:fix/am-sigterm-handler-override
Open

Remove SIGTERM handler override in AM gather and send plugins#1653
robjarawan wants to merge 3 commits into
MetPX:developmentfrom
robjarawan:fix/am-sigterm-handler-override

Conversation

@robjarawan

Copy link
Copy Markdown
Contributor
What

gather/am.py and send/am.py both override SIGTERM with SIG_DFL in __init__. This kills the process immediately on sr3 stop/restart, bypassing the flow's graceful shutdown — on_stop callbacks, message acking, and connection cleanup are all skipped. Data loss on every stop/restart cycle.

Change

Remove the signal.signal(signal.SIGTERM, signal.SIG_DFL) call from both files so the parent flow's handler runs properly. Removed unused signal import.

Both gather/am.py and send/am.py override SIGTERM with SIG_DFL in
__init__, which kills the process immediately on stop/restart. This
bypasses the flow's graceful shutdown — on_stop callbacks, message
acking, and connection cleanup are all skipped.

Remove the override so the parent flow's signal handler runs properly.
@robjarawan robjarawan added the bug Something isn't working label Mar 30, 2026
@robjarawan robjarawan self-assigned this Mar 30, 2026
@robjarawan robjarawan added the bug Something isn't working label Mar 30, 2026
@petersilva

Copy link
Copy Markdown
Contributor

I'm puzzled about how to test this... I'm not sure it has the effect hoped for. would want some testing.

@robjarawan

Copy link
Copy Markdown
Contributor Author

Fair point. The change is small -- just removing the SIG_DFL override so the parent flow's handler runs instead. The expected effect is that cleanup (closing sockets, stopping loops) actually happens on SIGTERM instead of an abrupt exit. I don't have an AM setup to test against but I can verify that the parent signal handler fires correctly with a mock. Would that be enough, or do you want to see it tested against an actual AM feed?

@petersilva

Copy link
Copy Markdown
Contributor

Maybe @andreleblanc11 has some configs he can test this branch with? you need some AM flows, right?

@petersilva petersilva added the Priority 5 - Defect Missing and/or broken functionality - do not forget label Apr 4, 2026
@petersilva

Copy link
Copy Markdown
Contributor

marking low priority because AM traffic is (gradually) being de-commissioned. so... yeah fine to fix it, but not super urgent.

@andreleblanc11

andreleblanc11 commented Apr 13, 2026

Copy link
Copy Markdown
Member

We added the signal handler to the AM code before Reid added it to the sr.py in https://github.com/MetPX/sarracenia/pull/559/changes.

So I think it's probably OK to remove but will need to test.

@robjarawan

robjarawan commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

The diff itself is strictly safer: it only deletes the two SIG_DFL overrides plus imports, restoring the parent flow's graceful handler with zero logic change.

One merge condition: a mock-handler smoke test, since bare fixtures do not construct (Am needs sendTo, fileSizeMax, add_option, and a stubbed socket). I verified the corrected test fails pre-PR on both modules and passes post-PR. Happy to push it on request.

The deeper items I found, the infinite accept loop with no stop check and the fork/execl orphan window, are pre-existing and I am marking them wontfix. AM is being decommissioned, so no loop rework is justified for a 6-line cleanup. @andreleblanc11 a live-flow eyeball on your side would still be welcome before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Priority 5 - Defect Missing and/or broken functionality - do not forget

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants