Repository navigation
test: wait for virtualenv provisioning and fix a TestServer bind race - #448
Merged
Merged
Conversation
closeServerSocket() closed the listening socket while the worker thread could still be blocked in accept() on it. The JDK keeps that socket listening until the blocked thread wakes, so a test that reopened the same port straight away could fail with "Address already in use", as NettyAtCommandExecutorTest.testOnReadyBeforeConnectionIsAccepted did in CI. The worker now marks when it is inside accept(), and closeServerSocket() spins, for at most a second, until it has left. TestServerTest checks that the worker has left accept() once close returns and that the port rebinds at once. Repeated 50 times on Linux with JDK 25, testOnReadyBeforeConnectionIsAccepted failed 20 times without this change and not at all with it.
VirtualEnv.setUp() waited at most 20 seconds for 40 provisioning log lines and carried on whether or not they had arrived. In each of the 16 recent CI runs checked, it waited the full 20 seconds, so the integration tests could start before install_PKAM_Keys had finished, and an atSign provisioned late failed its first test with "no legacy PKAM credential". setUp() now reads the atSigns the container hosts from its supervisor configuration, and returns only once install_PKAM_Keys has exited, which the compose command now reports on stderr, having provisioned every one of them. It throws, naming the atSigns not provisioned, when the script exits short or has not exited after 180 seconds, and when it finds no hosted atSigns. A failed start stops the container, and later calls rethrow that failure rather than starting another. Every line the container writes is logged, prefixed [virtualenv], so a CI log shows how provisioning went. The compose command now escapes its shell variables as $$, so compose no longer blanks $PID in "wait $PID" and warns that PID is unset. The README describes the wait and the line that marks a manually started virtual env as ready, and its docker compose commands no longer pass -f without a file.
isAccepting sat beside the existing isAccept, which says whether the worker should accept at all, so the flag that says the worker is inside accept() is now isWorkerInAccept. closeServerSocket() waits up to five seconds for the worker to leave accept() rather than one, so a slow runner does not turn a thread descheduled for a moment into a failure.
Every caller now calls VirtualEnv.setUp() unconditionally, and setUp() checks whether something already answers on vip.ve.atsign.zone:64 only when it has neither started a container nor failed. A recorded failure is therefore rethrown even when the failed container could not be stopped and still answers. setUp() fails at once when the container's output ends before install_PKAM_Keys has exited, rather than after 180 seconds. It counts an atSign as provisioned only from a cramAndPkamAuth or cramAuthOnly success line, not from the INFO line install_PKAM_Keys logs after the CRAM login alone. A stop that fails after a failed start no longer replaces the start's failure. New tests cover a non-zero exit status, a second setUp() after a successful start, atSigns compared folded, the interrupt flag, and the compose command's exit line in the form the watch parses. The README says a virtual env that is already running is used as it is.
closeServerSocket read the accepted socket before it waited for the worker to leave accept(). A connection that accept() returned during that wait was then either kept and served, or dropped with nothing referencing it, and stayed open either way. It now closes the listening socket, waits, and then closes whatever the worker accepted.
Contributor
Author
|
@cpswan this should fix the two intermittent flakes |
cpswan
approved these changes
Oct 21, 2026
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
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 I did
- Why
BindException: Address already in useinNettyAtCommandExecutorTest. Attempt 2 failed two Activate scenarios with "no legacy PKAM credential", because@garyhadn't been provisioned yet when the test first used it.VirtualEnv.setUp()waited at most 20 seconds and carried on whatever state the virtualenv was in, andTestServercould try to reopen a port the JDK was still holding, and fail withBindException.- How I did it
See commits
- How to verify it
Tests pass. The integration tests also passed locally against a fresh
dev_env.- Review findings left for later
vip.ve.atsign.zone:64, so a container that publishes that port counts as running even while it's still provisioning. The README says a running env is used without checking it.setUpis recorded like any other failed start, so later IT classes fail fast instead of starting another virtualenv.closeServerSocketspin-waits (for at most 5s) for the worker to leaveaccept(). The wait is normally microseconds.VirtualEnvTest's interrupt test should clear the flag in afinally, so a regression can't leak it into later tests.exitReportedandexitStatuscould be one nullable field.vip.ve.atsign.zone:64is spelled out across the test tree, andVirtualEnvadds one more copy.