Skip to content

fix(daemon-client): a restart probe cut short by the RPC deadline reports the timeout - #3056

Merged
thymikee merged 2 commits into
callstack:mainfrom
okwasniewski:oskar/probe-timeout-is-request-timeout
Sep 29, 2026
Merged

thymikee merged 2 commits into
callstack:mainfrom
okwasniewski:oskar/probe-timeout-is-request-timeout

Conversation

@okwasniewski

@okwasniewski okwasniewski commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

daemon-client-transport.test.ts > "a delayed restart health probe stops at the RPC deadline without retrying" flakes on CI. It failed the Coverage job on #3055 with Remote daemon is unavailable where it expects daemon_transport_timeout, and it fails the same way locally under load.

This is a real race in the client, not just in the test. After a remote_instance_mismatch, retryAfterRemoteInstanceMismatch probes /health with exactly the RPC's remaining budget, then checks performance.now() against the deadline. Node timers start from the event loop's cached clock, which can lag behind real time. So the probe's timer can expire while performance.now() is still short of the deadline. The remaining budget reads positive, and the client reports the daemon as unavailable when the RPC actually timed out.

The fix:

  • readDaemonHttpHealth now marks a result that ran out of time (timedOut: true), from either its socket timeout or its abort signal.
  • The restart retry reports the RPC timeout when the probe timed out and the RPC deadline had capped its budget.
  • A probe that fails fast (for example, connection refused) still reports Remote daemon is unavailable.

Wire ledger: RemoteDaemonHealth and readDaemonHttpHealth got new digests and new acks. timedOut is client-local: it is never read from a /health payload, and the request and the accepted payload fields are unchanged. The two acks it replaces had expired with the digests.

Validation

  • A new test freezes performance.now(), so the probe's timeout lands before the deadline. On main it fails with the exact CI error (Remote daemon is unavailable); with this change it passes.
  • daemon-client-transport.test.ts 20x in a row with one yes per core: 20/20 green.
  • pnpm check:affected --run: passed. pnpm test:coverage:ci: passed. Changed-line gate: passed (100%, 10/10).

Review in cubic

…orts the timeout

Node timers start from the event loop's cached clock, so the restart
health probe's timer can expire while performance.now() is still short
of the RPC deadline. The retry then threw 'Remote daemon is
unavailable' instead of the request timeout, which flaked 'a delayed
restart health probe stops at the RPC deadline without retrying' on CI.

The health probe now marks a result that ran out of time, and the
restart retry reports a probe the deadline capped that timed out as
the RPC timing out.
Copilot AI balanced review requested due to automatic review settings September 29, 2026 11:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 3 files

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread src/daemon-client/__tests__/daemon-client-transport.test.ts
Copilot AI review requested due to automatic review settings September 29, 2026 12:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@okwasniewski

Copy link
Copy Markdown
Contributor Author

[claude-opus-5-5] responding on behalf of @okwasniewski

iOS Smoke Tests failure on the test-pin commit is unrelated: wait text Automation lab timed out in readinessPhase: runner-start while runner.log shows xcodebuild still precompiling modules (runner build cache miss on this run). The previous commit's iOS smoke was green; the diff only touches the remote restart probe path. A rerun should clear it.

@thymikee

Copy link
Copy Markdown
Member

I reviewed 83cc5a7. A restart probe cut short by the RPC deadline now reports the timeout, and I found no problems in the change.

Not blocking: the probeTimeoutMs <= REMOTE_DAEMON_HEALTHCHECK_TIMEOUT_MS check at https://github.com/callstack/agent-device/blob/83cc5a7/src/daemon-client/daemon-client-transport.ts#L270 has no test where the 3 s probe cap runs out while RPC budget remains, and the refused fast-fail branch after a mismatch is also untested; the caller also rebuilds the Math.min decision from readDaemonHttpHealth at line 267, which the prober could report directly; and the new test is close to a copy of the delayed restart health probe test, so the frozen clock could fold into that one. You can take or leave these.

I did not run the new test on main or on this head, so the claim that it fails on main rests on reading the old code with a frozen performance.now(). I also did not reproduce the 20x loaded loop, the coverage run or check:affected, and I did not check whether spying on global performance.now disturbs Vitest's own timing under concurrency.

CI is still pending, because Smoke Tests was running when I checked. The change only touches the remote instance-mismatch restart retry, and smoke runs drive a local daemon, so I expect no overlap. There are no conflicts. Once Smoke Tests finishes green, this is ready for human review.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 29, 2026
@thymikee

Copy link
Copy Markdown
Member

Smoke Tests has now failed on 83cc5a7, in the live iOS simulator step "wait for Automation lab" (job). That run drives a local daemon and does not reach the remote instance-mismatch retry this PR changes, so it looks unrelated. The code verdict is unchanged; a rerun of Smoke Tests should confirm it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants