Plug tag_map leak in job_not_running that flaked test_mem_leak_in_event_listener - #70153
Open
dwoz wants to merge 1 commit into
Open
Plug tag_map leak in job_not_running that flaked test_mem_leak_in_event_listener#70153dwoz wants to merge 1 commit into
dwoz wants to merge 1 commit into
Conversation
saltnado._disbatch_local spawns job_not_running as an IOLoop.spawn_callback coroutine to ping saltutil.find_job while the outer job is in flight. When the outer job's is_finished future completes first, job_not_running resolves the in-flight ping future via event.set_result(None) and returns. set_result() alone does not remove the future from event_listener.tag_map or timeout_map -- the cleanup path in _handle_event_socket_recv only fires when a matching event arrives on the socket, and clean_by_request only fires from the handler's on_finish. Because spawn_callback runs independently of the handler, the ping future may be registered *after* on_finish -> clean_by_request has already emptied request_map, so the leaked entry only clears when the gather_job_timeout callback fires (10s default, 30s under the netapi test fixture). The intermittent AssertionError in test_mem_leak_in_event_listener catches this leak whenever the 1s poll window closes before the timeout callback runs -- the race is triggered by ordinary event-loop scheduling variance, which explains why the test flakes even on 3008.x nightlies that then pass on retry. Fix: after set_result(None), explicitly cancel the timeout callback and call _timeout_future(...) to drop the entry from tag_map, matching the cleanup that clean_by_request would have performed had the request still been tracked. Fixes saltstack#70152
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.
Summary
saltnado._disbatch_localspawnsjob_not_runningas anIOLoop.spawn_callbackcoroutine. When the outer job'sis_finishedfuture completes first,job_not_runningresolves the in-flight ping future viaevent.set_result(None)and returns — but never removes the future fromevent_listener.tag_map/timeout_map.spawn_callbackruns independently of the handler, the ping may be registered after the handler'son_finish -> clean_by_requesthas already emptiedrequest_map, so the leaked entry only clears when thegather_job_timeoutcallback fires (10 s default, 30 s under the netapi test fixture).test_mem_leak_in_event_listenerallows only a 1 s poll window before asserting the maps are empty, so any leaked ping tripsAssertionError: assert 2 == 0— the exact flake caught on the 3008.x nightlyAmazon Linux 2 integration zeromq 4(run 32913591224 job 98023096704, first attempt failed / retry passed) and reproduced on merge-forward PR Merge forward 3007.x into 3008.x #70132.set_result(None), cancel the tornado timeout callback and call_timeout_future(...)to drop the entry fromtag_map, matching the cleanup thatclean_by_requestwould have performed had the request still been tracked.Fixes #70152
Test plan
tests/pytests/integration/netapi/rest_tornado/test_minions_api_handler.py::test_mem_leak_in_event_listenerpasses on Amazon Linux 2 integration zeromq 4 without needing pytest-salt-factories retryrest_tornadointegration split otherwise unchanged