fix(escrow): circuit-break retrieves against a wedged holder - #245
Merged
Conversation
…er session A holder that accepts but never answers previously stalled the serial main-thread restore for retrieveRecvTimeout per escrowed panel. The new test drives retrieve against a bind+accept-but-never-respond listener with an injectable sub-second timeout and asserts the SECOND call to the same socket path returns instantly without connecting (the listener must see exactly one connection), then that resetting the breaker allows a fresh attempt. Test-only commit by design: the breaker state and reset seam land here so the test compiles; the skip and record behavior land next, so CI shows this failing without them.
One wedged holder (accepts, never answers) now costs one retrieveRecvTimeout stall per launch instead of one per escrowed panel: the first post-connect receive timeout records the socket path, and every retrieve against that same path within 60s returns nil without connecting. Fast-fail paths (missing socket, connection refused, eof, protocol error) never open the circuit — they are already cheap and may be transient. The 60s window is the only reset: long enough to cover a full serial restore sweep, short enough that a recovered holder is retried on the next real request. Turns the previous commit's regression test green.
…ession test The retrieve() circuit breaker keyed recentRetrieveTimeoutsByPath by wall-clock Date, so a backward clock jump (NTP sync, sleep/wake) could keep a path's circuit open past the intended 60s window. Switched to DispatchTime (monotonic CLOCK_UPTIME_RAW) and compare elapsed nanoseconds instead; the path_failed_ago_ms dilog value is unchanged. testEscrowRetrieveCircuitBreakerSkipsSecondCallToWedgedHolder asserted the open-breaker skip completes in < 0.05s (tight against scheduling jitter) and read the accept thread's connection count without waiting for the async accept() to actually land. Loosened the skip bound to < 0.15s (still well under the 0.2s injected timeout) and added AcceptedConnections.waitForCount, a bounded poll used wherever the test expects a new connection to have been accepted, instead of reading count immediately after retrieve() returns.
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.
What this does
Follow-up to #244, from its review: a holder that accepts connections but never answers cost the launch restore 5s of main-thread stall per escrowed panel (they nearly all share one holder socket). Now the first receive timeout opens a 60s circuit for that socket path and every other retrieve against it skips straight to the fallback without connecting — one stall per launch, not per panel. Fast failures (missing socket, refused, eof) never open the circuit; the 60s window is the only reset, so a holder that recovers gets retried on the next real request.
Review order
Test plan