URL-test: await external plugin loopback readiness (#46)

[hawkff]

Summary

URL-test reliability fix. External-plugin profiles (naive / mieru / masterdnsvpn) intermittently
showed "connection refused" during URL Test because the test dialed the plugin's local SOCKS
loopback before the sidecar had bound it. This was a pre-existing race (proven by A/B against a
pre-#44 build) — not a regression — but worth fixing.

The live VPN-start path already waits for sidecar readiness via awaitExternalProcessesReady()
(polls each plugin loopback port until it accepts). The URL test still used a fixed delay(500),
which often lost the race under concurrent testing.

Changes

• BoxInstance.awaitExternalProcessesReady(strict = false) — add a strict flag. When strict
  (URL test), a sidecar that never binds within the readiness window is a clear error
  (sidecar listener not ready on port(s): …) instead of the live-path behavior of logging and
  continuing (the live path keeps a long-lived connection sing-box retries; a one-shot test has
  no such retry).
• TestInstance.doTest() — replace delay(500) with awaitExternalProcessesReady(strict = true).

Acceptance

External-plugin URL tests should show real latency or a clear "plugin listener not ready"
error, not flaky "connection refused".

Testing

• CodeRabbit CLI: No findings.
• Verified on Android: after the fix, Naive-media → 189ms and Mieru → 194ms (previously
  "Unavailable"/refused); a genuinely slow/unbound sidecar (MasterDnsVPN under concurrent test)
  yields the clear sidecar listener not ready error rather than "connection refused". Native
  profiles (Hysteria2, AmneziaWG) unaffected.

Notes

• For MasterDnsVPN (long DNS/MTU setup) the readiness window is the existing 60s; under heavy
  concurrent testing it can still time out and report "not ready" — which is the intended clear
  signal. It connects normally when actually used (single instance, no concurrency).
• No security implications; this is purely URL-test UX/reliability.

Greptile Summary

Replaces the fixed delay(500) in TestInstance.doTest() with awaitExternalProcessesReady(strict = true), eliminating the race between URL-test dialling and an external plugin sidecar binding its loopback SOCKS port.

• BoxInstance.awaitExternalProcessesReady(strict: Boolean = false) — adds a strict flag that (a) caps the non-MasterDnsVPN readiness window at min(2_000, max(1_000, connectionTestTimeout)) ms and (b) promotes a timeout from a warning-and-continue into a hard IOException with a descriptive message.
• TestInstance.doTest() — swaps the blind 500 ms sleep for awaitExternalProcessesReady(strict = true) inside the existing processes.processCount > 0 guard; the now-unused kotlinx.coroutines.delay import is removed.

Confidence Score: 5/5

Safe to merge — the change is narrowly scoped to the URL-test path and leaves the live VPN-start path unmodified.

Both changed files have clear, correct logic. The strict flag is defaulted to false so existing callers of awaitExternalProcessesReady() (the live-start path) are unaffected. The new 2 s cap for non-MasterDnsVPN strict mode is a straightforward improvement over the previous unbounded wait, and exception propagation through tryResumeWithException in TestInstance is handled correctly. No data-loss, auth, or crash risk is introduced.

No files require special attention.

Important Files Changed

Filename	Overview
app/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt	Adds strict parameter to awaitExternalProcessesReady(): caps readiness window at 2 s for non-MasterDnsVPN profiles and promotes a timeout to a hard IOException in strict mode.
app/src/main/java/io/nekohasekai/sagernet/bg/proto/TestInstance.kt	Replaces the fixed delay(500) guard before urlTest with awaitExternalProcessesReady(strict = true), and removes the now-unused kotlinx.coroutines.delay import.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant U as Caller
    participant TI as TestInstance.doTest()
    participant BI as BoxInstance.awaitExternalProcessesReady(strict=true)
    participant SP as Sidecar Process (Naive/Mieru/MasterDnsVPN)
    participant LC as Libcore.urlTest()

    U->>TI: doTest()
    TI->>TI: init() + launch()
    alt "processes.processCount > 0"
        TI->>BI: "awaitExternalProcessesReady(strict=true)"
        loop poll every 50ms until deadline
            BI->>SP: TCP connect(loopback:port, 100ms)
            alt port accepts
                SP-->>BI: connected - remove from pending
            else refused
                SP-->>BI: IOException (not ready yet)
            end
        end
        alt all ports ready
            BI-->>TI: return (success)
        else "timeout and strict=true"
            BI-->>TI: throw IOException(sidecar listener not ready on port(s))
            TI-->>U: tryResumeWithException(e)
        end
    end
    TI->>LC: urlTest(box, link, timeout)
    LC-->>TI: latency ms
    TI-->>U: tryResume(latency)

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant U as Caller
    participant TI as TestInstance.doTest()
    participant BI as BoxInstance.awaitExternalProcessesReady(strict=true)
    participant SP as Sidecar Process (Naive/Mieru/MasterDnsVPN)
    participant LC as Libcore.urlTest()

    U->>TI: doTest()
    TI->>TI: init() + launch()
    alt "processes.processCount > 0"
        TI->>BI: "awaitExternalProcessesReady(strict=true)"
        loop poll every 50ms until deadline
            BI->>SP: TCP connect(loopback:port, 100ms)
            alt port accepts
                SP-->>BI: connected - remove from pending
            else refused
                SP-->>BI: IOException (not ready yet)
            end
        end
        alt all ports ready
            BI-->>TI: return (success)
        else "timeout and strict=true"
            BI-->>TI: throw IOException(sidecar listener not ready on port(s))
            TI-->>U: tryResumeWithException(e)
        end
    end
    TI->>LC: urlTest(box, link, timeout)
    LC-->>TI: latency ms
    TI-->>U: tryResume(latency)

Loading

Comments Outside Diff (1)

1. app/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt, line 279-283 (link)

   Readiness window reuses the URL-test timeout, doubling visible wait time

   readinessTimeoutMs for non-MasterDnsVPN profiles is maxOf(1_000L, connectionTestTimeout) — the same value passed to Libcore.urlTest. In the worst case (sidecar never binds), a user with the default 3 000 ms timeout will now wait up to 3 s for readiness and then 3 s for the URL test itself, for a total of ~6 s. Previously the fixed 500 ms delay kept the total close to the configured timeout. For users who set a tighter timeout this doubling may be surprising. Consider capping the readiness window to a fraction of the URL-test timeout (e.g. half), or using a separate, smaller constant for the non-MasterDnsVPN strict case.

_{Reviews (3): Last reviewed commit: "ci: trigger build on latest commit" | Re-trigger Greptile}

[coderabbitai]

📝 Walkthrough

Walkthrough

awaitExternalProcessesReady in BoxInstance gains a strict: Boolean = false parameter; when strict is enabled, a sidecar listener timeout is treated as fatal. TestInstance.doTest() removes the fixed 500 ms delay and instead calls awaitExternalProcessesReady(strict = true) for a deterministic readiness check before running the URL test.

Changes

Strict sidecar readiness for URL testing

Layer / File(s)	Summary
awaitExternalProcessesReady strict parameter and timeout handling app/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt	Adds strict: Boolean = false to the method signature and KDoc, introduces a strict-specific bounded timeout branch, and updates the listener-not-ready timeout error path to throw when strict is true or hasMasterDnsVpn is present.
TestInstance replaces delay(500) with strict readiness call app/src/main/java/io/nekohasekai/sagernet/bg/proto/TestInstance.kt	Removes the fixed 500 ms wait in doTest() and replaces it with awaitExternalProcessesReady(strict = true) so an unbound sidecar SOCKS port produces a clear error instead of a silent connection refusal.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

• hawkff/NekoBoxForAndroid#27: Also modifies BoxInstance.awaitExternalProcessesReady's sidecar readiness handling to change when "sidecar listener not ready" is treated as fatal.

Poem

🐇 No more sleeping half a second in the dark,
The rabbit now listens for the sidecar's spark!
If the port never binds, we cry loud and clear—
No silent refusals allowed 'round here.
Strict mode hops in, delay hops out,
That's what determinism is all about! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: adding external plugin loopback readiness waiting to the URL-test flow to fix intermittent connection refused errors.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The PR description comprehensively explains the race condition being fixed, the changes made, and testing results with specific examples.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[hawkff]

Addressed in 195fff1: capped the strict (URL-test) readiness window at min(2s, connectionTestTimeout) so a slow sidecar can't ~double the perceived test time. MasterDnsVPN keeps its required 60s window (its readiness is fatal, not best-effort).