Skip to content

fix(core): make sure Ryuk acknowledges the session filter - #1124

Open
RitiGrover wants to merge 2 commits into
testcontainers:mainfrom
RitiGrover:fix/ryuk-wait-for-ack
Open

RitiGrover wants to merge 2 commits into
testcontainers:mainfrom
RitiGrover:fix/ryuk-wait-for-ack

Conversation

@RitiGrover

Copy link
Copy Markdown

Fixes #1114

Reaper._create_instance could report success even though the session filter never reached Ryuk, so nothing was reaped. Two things caused it:

  • waiting_for(...) was set after .start(), so the wait for Ryuk's start log never ran. It is now set before .start(). The pattern is now \bStarted\b, because Ryuk 0.8 logs Started! while 0.11 and newer log msg=Started. With the old pattern the wait would time out on newer images.
  • A connect was treated as registration. docker-proxy accepts on the published port before Ryuk listens, and the filter was sent without reading the reply. Connect, send and reading Ryuk's ACK are now one step in Reaper._connect_and_register. It is retried until ACK arrives and raises if it never does.

Tested with two unit tests against a local fake Ryuk. One resets the first connection and ignores the second, and the client must retry until the third answers ACK. The other never answers, and the client must raise. tests/core/test_ryuk.py passes against a real Ryuk, including test_wait_for_reaper. The other core tests I ran show the same failures as on main (four FileExistsWaitStrategy path tests, on Windows), and mypy reports the same errors as main. I don't have a Linux docker-proxy host to reproduce the original race end to end.

@PHcz PHcz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. Both halves match what we measured on Docker Desktop (details in #1114): setting the wait before start() removed almost all the dropped connections, and reading the ACK catches the rest. A few things I noticed at 75104cd:

1. The failure paths leave the ryuk container running under its fixed name.

  • When _connect_and_register gives up (container.py L520), _create_instance re-raises with the started ryuk still running. Reaper._container is set, _instance is still None, and no atexit hook is registered. The next get_instance() (the next container start in the session) tries to create testcontainers-ryuk-{SESSION_ID} again and gets a 409 "name already in use". That goes on until the old ryuk exits on its 60 s first-connection timeout, which is the failure in #1093. Before this PR the no-ACK case returned as if it had succeeded, so this path is new.
  • The same happens with the wait (L504). If it times out, or raises because ryuk exited, .start() raises inside the chained expression before Reaper._container is assigned, so nothing references the container at all.

Suggestion: build the container, keep the reference, and remove it (e.g. Reaper.delete_instance()) before re-raising on either path.

2. A failure can take up to ~2 minutes. The retry budget is 50 attempts. Each can take up to 1 s to connect, 1 s waiting for a reply and 0.5 s of sleep. Against a peer that accepts and never answers (your silent case), _create_instance blocks for about 75-125 s. Ryuk itself exits 60 s after starting if no client has registered, so the later attempts can't succeed. A deadline no longer than ryuk's connection timeout would bound it.

3. test_reaper_retries_until_ryuk_acknowledges_the_filter has about 0.5 s of timing margin.

  • Attempt 2 hits the silent branch, which holds the server thread in sleep(1.5).
  • Attempt 3 connects through the listen backlog about 1.0 s in and waits 1 s for a reply.
  • So the thread has to wake, accept and send ACK within about 0.5 s.

If it's late on a busy runner, attempt 3 times out and the thread has used up its three scripted connections. Nothing answers attempts 4 and 5 with ACK, and the test fails. Having the silent server give up before the client's timeout, or giving the client a longer timeout in this test, would remove the race.

4. The wait move has no test. The new tests exercise _connect_and_register against a fake peer only. Nothing checks that the Started wait runs before the connect, or that \bStarted\b matches both Started! (0.8) and msg=Started (0.11+). test_wait_for_reaper is skipped on macOS, so moving waiting_for back after .start() would still pass CI. On Docker Desktop you can make the race happen every time by giving ryuk nano_cpus=10_000_000 (--cpus 0.01). It then logs Started hundreds of ms after start() returns, so a spy on the connect can check that ryuk's log already says Started. There's a script in the #1114 comment linked above.

If start() or the registration failed, the Ryuk container kept running
under its fixed name and the next start hit a name conflict. It is now
removed before the error is re-raised.

The registration retries now stop after a 30 s deadline instead of 50
attempts, which could take about two minutes against a silent peer.
Ryuk itself exits 60 s after start without a client.

Tests: the silent fake peer now waits for the client to close, which
removes a timing race. New tests cover the deadline, the cleanup on
both failure paths, and the wait strategy set before start().
@RitiGrover

Copy link
Copy Markdown
Author

Thanks for the detailed review. All four are addressed in bff1678.

  1. The Ryuk container is now kept in Reaper._container before start(). If the wait or the registration fails, delete_instance() removes it before the error is re-raised. delete_instance() now also clears a reference to a container that was never created.
  2. The retries stop at a 30 s deadline instead of after 50 attempts. The per-attempt reply timeout is now a parameter.
  3. The silent fake peer now waits until the client closes, then accepts the next connection. The test uses a 0.2 s reply timeout. There is no sleep left to race against.
  4. New test test_reaper_removes_its_container_when_setup_fails covers both failure paths with a fake start. It checks that the container is stopped and the class state cleared. It also checks that the wait strategy is set when start() runs and that it matches both Started! and msg=Started, but not Starting.

I haven't added the slowed-down ryuk (nano_cpus) repro as a test, since it depends on the Docker host. Happy to add it if you want it in the suite.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Reaper reports success while the session filter never reaches Ryuk (silent leak on Linux/docker-proxy)

2 participants