test(daemon-client): a restart probe that fails outright near the RPC deadline reports the daemon unavailable - #3058
Conversation
… deadline reports the daemon unavailable
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/daemon-client/__tests__/daemon-client-transport.test.ts">
<violation number="1" location="src/daemon-client/__tests__/daemon-client-transport.test.ts:296">
P3: The elimination rationale in this comment doesn't match how the test actually rejects a skipped probe in the refused case. Once `server.close()` runs, a skipped probe isn't followed by budget exhaustion: `sendRequestWithTransport` retries the RPC, the closed listener refuses it immediately, and the request errors out as `daemon_transport_failure` ('Failed to communicate with daemon'). That error is rejected by `assert.equal(error.message, 'Remote daemon is unavailable')` and the `notEqual(reason, ...)` check, not by any budget-driven `daemon_transport_timeout`. The timeout path only applies to the still-open 503/closed-connection cases, where a skipped probe keeps looping on 409 until the 150 ms budget caps it. Suggest rewording the comment so the guard it describes is the one that actually fires.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| // A closed listener cannot count the probe; its attempt is proven by elimination: a skipped | ||
| // probe would exhaust the budget and report daemon_transport_timeout, which the error check rejects. |
There was a problem hiding this comment.
P3: The elimination rationale in this comment doesn't match how the test actually rejects a skipped probe in the refused case. Once server.close() runs, a skipped probe isn't followed by budget exhaustion: sendRequestWithTransport retries the RPC, the closed listener refuses it immediately, and the request errors out as daemon_transport_failure ('Failed to communicate with daemon'). That error is rejected by assert.equal(error.message, 'Remote daemon is unavailable') and the notEqual(reason, ...) check, not by any budget-driven daemon_transport_timeout. The timeout path only applies to the still-open 503/closed-connection cases, where a skipped probe keeps looping on 409 until the 150 ms budget caps it. Suggest rewording the comment so the guard it describes is the one that actually fires.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon-client/__tests__/daemon-client-transport.test.ts, line 296:
<comment>The elimination rationale in this comment doesn't match how the test actually rejects a skipped probe in the refused case. Once `server.close()` runs, a skipped probe isn't followed by budget exhaustion: `sendRequestWithTransport` retries the RPC, the closed listener refuses it immediately, and the request errors out as `daemon_transport_failure` ('Failed to communicate with daemon'). That error is rejected by `assert.equal(error.message, 'Remote daemon is unavailable')` and the `notEqual(reason, ...)` check, not by any budget-driven `daemon_transport_timeout`. The timeout path only applies to the still-open 503/closed-connection cases, where a skipped probe keeps looping on 409 until the 150 ms budget caps it. Suggest rewording the comment so the guard it describes is the one that actually fires.</comment>
<file context>
@@ -256,6 +256,65 @@ test('a restart health probe cut short by the RPC deadline reports the deadline
+ return true;
+ });
+ assert.equal(rpcCount, 1);
+ // A closed listener cannot count the probe; its attempt is proven by elimination: a skipped
+ // probe would exhaust the budget and report daemon_transport_timeout, which the error check rejects.
+ if (failure !== 'refused-connection') assert.equal(healthProbes, 1);
</file context>
| // A closed listener cannot count the probe; its attempt is proven by elimination: a skipped | |
| // probe would exhaust the budget and report daemon_transport_timeout, which the error check rejects. | |
| // A closed listener may refuse the probe before the handler can count it, so the counter is | |
| // not relied on here; a skipped probe still breaks the 'Remote daemon is unavailable' message check. |
|
I reviewed 00833fb and found no problems in the code. The new loopback-HTTP tests cover the case where a restart probe fails outright near the RPC deadline and the daemon is reported unavailable. The change only adds unit tests and touches no production code. The one red check is the Android emulator fixture E2E, where the alert result element did not become visible after five scrolls. It looks unrelated, because this diff does not touch the device route that job exercises. Please rerun Smoke Tests. No code change is needed on this PR. There are no conflicts. I did not run the new tests or the mutation locally. I judged that they fail without the branch by reading the code. I did not read readRemoteDaemonHealth to confirm that a closed connection sets timedOut=false. The healthProbes==1 assertion and the reason check keep the test valid either way. I could not rerun the Android job, so the CI call rests on the log excerpt and on the diff not overlapping that route. |
|
* origin/main: 0.21.17 feat(daemon): report the host CPU architecture in /health (callstack#3048) feat: add daemon policy to confine devices, commands, and device shutdown (callstack#3064) test(web): wait for the killed fake daemon to be reaped before asserting it is gone (callstack#3066) fix(ios): write the simulator clipboard from the runner (callstack#3065) test(daemon-client): a restart probe that fails outright near the RPC deadline reports the daemon unavailable (callstack#3058) fix(daemon-client): a client whose daemon lost the start race adopts the winner (callstack#3057) fix(android): honor boot --timeout as the emulator boot deadline (callstack#3059) test(ios-smoke): wait once more when the runner is still starting behind a deep link (callstack#3063)
Summary
A restart health probe that fails outright (503, closed connection, refused connection) with a small remaining RPC budget must report 'Remote daemon is unavailable' with daemonBaseUrl, never daemon_transport_timeout. #3056 tested only the timeout direction.
Follow-up to #3056.
Design: one parameterized helper and three cases next to #3056's test, against real loopback servers. Catches the mutation that marks every unreachable probe timedOut. The refused case cannot count the probe on a closed listener, so it proves the attempt by elimination (a skipped probe would report a timeout); the comment says so. Rejected: a production hook or socket spy to count the attempt (production change for a test), and a separate file (no size ratchet trip).
Overlap: open #3050 touches the lifecycle and launch-spec files, not the transport test file.
Touched files: 1.
Validation