Stop the readiness probe blacklisting itself, and gate the fleet on wait_for_servers - #356
Open
sibson wants to merge 7 commits into
Open
Stop the readiness probe blacklisting itself, and gate the fleet on wait_for_servers#356sibson wants to merge 7 commits into
sibson wants to merge 7 commits into
Conversation
The base HEALTHCHECK opened an RFB connection and hung up without speaking the protocol. Xvnc counts every connection closed before a successful authentication towards BlacklistThreshold (default 5), so the probe blacklisted 127.0.0.1 about ten seconds after start-up and refused every session after that -- captures against tigervnc-auth silently produced no PNG. The workaround was -BlacklistThreshold=1000000, which turned the brute-force protection off rather than fixing the probe. Measured against TigerVNC 1.12.0, no RFB-level probe avoids the counter: a bare connect, a version exchange, and a version-plus-security-types exchange each trip it after five attempts, and only a successful authentication clears the mark. So the probe now reads the kernel socket table with ss instead, confirming the port is listening without a connection ever existing. Over 25 minutes and four full functional runs the fleet logged 44 connections, all from tests and none from the probe, with no blacklisting. The threshold is reduced to a targeted 50 rather than removed, because the probe was not the only offender: vncservers.py's port_open() guard leaves about seven unauthenticated closes per run, and test_cli.py adds one deliberate wrong password. A full run interleaves enough successful authentications to stay under the default, but running test_cli.py alone repeatedly does not. Also installs x11-utils in the base stage. draw-content.sh waits on xdpyinfo and xwininfo, neither of which existed in the tigervnc image, so both retry loops ran to exhaustion and the readiness marker was really a fixed 21.3s sleep -- long enough to pass by luck, and no kind of check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
draw-content.sh waited with `cmd >/dev/null 2>&1 && break`, which cannot distinguish "not ready yet" from "that program is not installed", and then touched the readiness marker unconditionally once the loops finished. Both waits therefore degraded into sleeps on any image missing the tools, and the marker asserted nothing at all. That is what happened: x11-utils was absent from the tigervnc image, so the marker was really a fixed 21.3s sleep. Installing the package fixed that instance but not the shape -- an xlogo that failed to map would still have produced a marker, and so a green run against a blank framebuffer. Missing tools are now a hard error, and a wait that runs out is a hard error rather than a fall-through to touching the marker. Measured on an image with xdpyinfo and xwininfo removed: the old script reported the container healthy with the marker written, the new one leaves the marker absent, never reports healthy, and logs which tool is missing. On a correct image the marker now appears 0.76s after start rather than 21.27s, because the waits do real work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Readiness for the Docker fleet was a marker file created by draw-content.sh and checked by the image HEALTHCHECK -- a weaker, in-container imitation of the gate the OS-hosted servers already used. It could only ever approximate what the tests need, and its failure mode was a false green: the marker was written unconditionally, so a server that came up blank still reported healthy. wait_for_servers.py now gates both families. wait_until_ready() asserts what test_capture asserts -- connect, capture, and require the content that server is supposed to render -- with the flatness rule extracted so the gate and the test cannot drift apart. renders_desktop already distinguished the servers expected to be blank, so vncev and the OS-hosted servers stop at the handshake as before. That leaves the HEALTHCHECK doing liveness only: the process bound its port. It is now identical in every stage, so the vncev and libvncserver-example overrides are gone, and draw-content.sh no longer writes a marker at all. Verified against a tigervnc image with xdpyinfo removed, which comes up blank: the container still reports healthy, because its port is bound, and the readiness gate fails it by name -- 'capture is flat' on every attempt, then exit 1 out of wait_for_servers.py, which fails servers-up. That is the case the old marker reported ready. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
x11vnc grew its own HEALTHCHECK while this branch sat unmerged, and it kept both habits this branch removes from base: it opens an RFB connection to /dev/tcp, and it gates on draw-content.sh's marker file. Neither belongs now. The connection is harmless against x11vnc, which has no blacklist, but it is the pattern that broke TigerVNC and there is no reason to keep an exception around waiting to be copied. The marker file is the weaker in-container imitation of readiness that wait_for_servers.py now replaces for every server. What is specific to this container is its event sink, so that is what the check keeps beyond base's liveness probe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The comments explained a mechanism that is gone by narrating how it failed. What a reader needs is why the waits here must stay loud, and why the blacklist threshold is 50 rather than the default -- both of which are true whatever came before.
sibson
commented
Aug 15, 2026
| def port_open(host: str, port: int, timeout: float = PORT_PROBE_TIMEOUT) -> bool: | ||
| """Whether ``port`` accepts a connection. Cheap liveness, not readiness. | ||
|
|
||
| This hangs up without authenticating, which TigerVNC counts towards |
sibson
commented
Aug 15, 2026
| @@ -98,6 +98,14 @@ jobs: | |||
| - name: Build and start the VNC test servers | |||
| run: docker compose -f tests/servers/docker-compose.yml up -d --build --wait | |||
|
|
|||
The blacklist paragraph stays: without it the natural fix for a readiness probe is to make it speak more of the protocol, which was measured not to help. Around it went restatement -- where readiness lives, said three times, and what the check does, which the check says.
sibson
commented
Aug 15, 2026
|
|
||
|
|
||
| def distinct_colours(image: Image.Image) -> Optional[int]: | ||
| """How many colours ``image`` contains, or None above ``MAX_COLOURS``.""" |
Owner
Author
There was a problem hiding this comment.
Deleted rather than shortened. port_open's blacklist aside, has_expected_content's flat-capture paragraph, wait_until_ready's essay, and the CI step's comment (which restated its own name) are all gone -- 38 lines out, 8 in.
Two of your three flags were docstring paragraphs, which I had never audited because I scoped the rule to # comments. That was the gap.
Docstring second paragraphs counted as comments and had never been audited; the CI step's comment restated its own name. Gone: the port_open blacklist aside, the flat-capture explanation, the wait_until_ready essay, the readiness gate's rationale. What is left is what a reader cannot recover: that servers accept connections before they can serve, that Screen Sharing's first connection is what starts it, and that the reactor thread outlives its clients.
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.
This branch was written before #351/#353 and never got a PR, so it sat stranded while the stack moved on — and the problem it fixes kept resurfacing. Rebased onto current main and verified against a real fleet.
The problem
The base HEALTHCHECK opened an RFB connection and hung up without speaking the protocol. Xvnc counts every connection closed before a successful authentication towards
BlacklistThreshold(default 5), so the probe blacklisted 127.0.0.1 about ten seconds after start-up and refused every session after that — captures against tigervnc-auth silently produced no PNG.The workaround at the time was
-BlacklistThreshold=1000000, which turns the brute-force protection off rather than fixing the probe.The fix
Measured against TigerVNC 1.12.0, no RFB-level probe avoids the counter: a bare connect, a version exchange, and a version-plus-security-types exchange each trip it after five attempts, and only a successful authentication clears the mark. So the probe reads the kernel socket table with
ssinstead, confirming the port is listening without a connection ever existing.The threshold is then reduced to a targeted 50 rather than removed, because the probe was not the only offender:
vncservers.py'sport_open()guard leaves about seven unauthenticated closes per run, andtest_cli.pyadds one deliberate wrong password.Readiness itself moves out of the container:
wait_for_servers.pygates both server families by connecting, capturing, and requiring the content that server is supposed to render — the same assertion the tests make, so the gate and the tests cannot drift. The old marker file was written unconditionally, so its failure mode was a false green: a server that came up blank still reported healthy.Verification against a real fleet
Healthyon the connectionless probewait_for_servers.py docker: every server ready on the first attemptOne integration fix
x11vnc grew its own HEALTHCHECK while this branch sat unmerged, keeping both habits removed from base: an RFB connection via
/dev/tcp, and the marker-file gate. The connection is harmless there — x11vnc has no blacklist — but it is the pattern that broke TigerVNC, and no reason to leave an exception lying around to be copied. Its check now usesssplus the thing genuinely specific to that container, its event sink.Supersedes #355, which raised the threshold on both entrypoint branches — spreading the workaround instead of deleting it.
🤖 Generated with Claude Code