perf(daemon-client): cache remote health by daemon identity - #3015
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 14 files
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="test/wire-compat/ledger.json">
<violation number="1" location="test/wire-compat/ledger.json:176">
P3: This block, plus `LeaseRpcCommand`, `SessionIsolationMode`, and `SessionRuntimePlatform`, was deleted from its previous position and re-added here (or re-sorted) with byte-identical digests — ~18 ledger lines that are not wire changes. This README says "every ledger line in a diff is a deliberate wire change someone chose to make," and the re-sort buries the real instance-ID digest changes. Keep the re-sort in its own commit or revert it; the moved entries carry unchanged hashes, so nothing about them needed to change.</violation>
</file>
<file name="test/wire-compat/surface.ts">
<violation number="1" location="test/wire-compat/surface.ts:74">
P2: The manifest declares the two new instance-header constants as wire surface but does not list their consumer, `readRemoteInstanceHeaders` in src/daemon-client/daemon-client-transport.ts. Per the README's "both sides of every boundary are now listed" rule, a future narrowing of that reader (e.g. dropping `upstreamInstanceId` handling, or reading a wrong header name) would silently disable instance-change detection and restart re-probing without moving any digest, which is exactly the silent-misparse class the gate exists to catch. List `readRemoteInstanceHeaders` in the CLIENT_TRANSPORT consumer block and paste its digest into the ledger (with a compatibleChanges ack, per ADR 0006 additive rule).</violation>
</file>
<file name="src/daemon-client/daemon-client-transport.ts">
<violation number="1" location="src/daemon-client/daemon-client-transport.ts:511">
P3: The identity recheck runs `readRemoteDaemonHealth` (up to REMOTE_DAEMON_HEALTHCHECK_TIMEOUT_MS = 3000 ms plus network latency) inside the RPC's original request-envelope timeout, because `timeoutHandle` keeps running while `identityCheck` probes. The first command after a daemon or proxy restart can therefore be rejected as a request timeout even though the restarted daemon is reachable and its response has already arrived. Consider excluding the recheck probe from the request envelope (clear the envelope while rechecking, or give the probe its own budget) so a detected restart surfaces the response after verification instead of racing the command timeout.</violation>
<violation number="2" location="src/daemon-client/daemon-client-transport.ts:529">
P2: The progress reader clears the RPC timer before this identity recheck finishes, so a final NDJSON envelope can make a command outlive its configured `timeoutMs` while `/health` hangs. Keep the request timer active until identity verification and response handling complete.</violation>
</file>
<file name="src/daemon-client/__tests__/daemon-client-health-cache.test.ts">
<violation number="1" location="src/daemon-client/__tests__/daemon-client-health-cache.test.ts:67">
P3: This assertion matches the error by message text, which the repo convention explicitly avoids ('Key on typed reasons/details, never error text; no message sniffs' in AGENTS.md). The thrown AppError carries the typed detail `remoteRpcProtocolVersion` (and `code: 'COMMAND_FAILED'` shared with every other daemon failure), so the failure can be distinguished by details instead of the message. Same for the second `/RPC protocol is incompatible/` assertion at the `incompatible-instance` step.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| ...from( | ||
| DAEMON_HTTP, | ||
| 'DAEMON_HTTP_BASE_PATH', | ||
| 'DAEMON_HTTP_INSTANCE_HEADER', |
There was a problem hiding this comment.
P2: The manifest declares the two new instance-header constants as wire surface but does not list their consumer, readRemoteInstanceHeaders in src/daemon-client/daemon-client-transport.ts. Per the README's "both sides of every boundary are now listed" rule, a future narrowing of that reader (e.g. dropping upstreamInstanceId handling, or reading a wrong header name) would silently disable instance-change detection and restart re-probing without moving any digest, which is exactly the silent-misparse class the gate exists to catch. List readRemoteInstanceHeaders in the CLIENT_TRANSPORT consumer block and paste its digest into the ledger (with a compatibleChanges ack, per ADR 0006 additive rule).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/wire-compat/surface.ts, line 74:
<comment>The manifest declares the two new instance-header constants as wire surface but does not list their consumer, `readRemoteInstanceHeaders` in src/daemon-client/daemon-client-transport.ts. Per the README's "both sides of every boundary are now listed" rule, a future narrowing of that reader (e.g. dropping `upstreamInstanceId` handling, or reading a wrong header name) would silently disable instance-change detection and restart re-probing without moving any digest, which is exactly the silent-misparse class the gate exists to catch. List `readRemoteInstanceHeaders` in the CLIENT_TRANSPORT consumer block and paste its digest into the ledger (with a compatibleChanges ack, per ADR 0006 additive rule).</comment>
<file context>
@@ -68,7 +68,14 @@ export const WIRE_SURFACE: readonly WireSurfaceGroup[] = [
+ ...from(
+ DAEMON_HTTP,
+ 'DAEMON_HTTP_BASE_PATH',
+ 'DAEMON_HTTP_INSTANCE_HEADER',
+ 'DAEMON_HTTP_UPSTREAM_INSTANCE_HEADER',
+ 'buildDaemonHttpUrl',
</file context>
| reject, | ||
| }), | ||
| handleResponseBody: (body) => { | ||
| void identityCheck |
There was a problem hiding this comment.
P2: The progress reader clears the RPC timer before this identity recheck finishes, so a final NDJSON envelope can make a command outlive its configured timeoutMs while /health hangs. Keep the request timer active until identity verification and response handling complete.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon-client/daemon-client-transport.ts, line 529:
<comment>The progress reader clears the RPC timer before this identity recheck finishes, so a final NDJSON envelope can make a command outlive its configured `timeoutMs` while `/health` hangs. Keep the request timer active until identity verification and response handling complete.</comment>
<file context>
@@ -429,27 +508,49 @@ async function sendHttpRequest(
- reject,
- }),
+ handleResponseBody: (body) => {
+ void identityCheck
+ .then(() =>
+ handleDaemonHttpResponseBody(body, {
</file context>
| "src/remote/upload-stream.ts#streamFileToHttpRequest": "sha256:ce07ea33a275e06e4cceaf0c75a079bd0c7938f36df6817407dbb1cc8c4ff900", | ||
| "src/remote/upload-stream.ts#streamFileToHttpRequestAttempt": "sha256:aa73fb51890ca5e43d4aa80c1d2124b574568380a73b19eae7da0ee3e3e7acf8" | ||
| "src/remote/upload-stream.ts#streamFileToHttpRequestAttempt": "sha256:aa73fb51890ca5e43d4aa80c1d2124b574568380a73b19eae7da0ee3e3e7acf8", | ||
| "src/request-progress-protocol.ts#DaemonProgressEnvelope": "sha256:16162d01cfc43fc6a0198dbba3358981c1a278a70f7494ebe70d051f517cfb22", |
There was a problem hiding this comment.
P3: This block, plus LeaseRpcCommand, SessionIsolationMode, and SessionRuntimePlatform, was deleted from its previous position and re-added here (or re-sorted) with byte-identical digests — ~18 ledger lines that are not wire changes. This README says "every ledger line in a diff is a deliberate wire change someone chose to make," and the re-sort buries the real instance-ID digest changes. Keep the re-sort in its own commit or revert it; the moved entries carry unchanged hashes, so nothing about them needed to change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/wire-compat/ledger.json, line 176:
<comment>This block, plus `LeaseRpcCommand`, `SessionIsolationMode`, and `SessionRuntimePlatform`, was deleted from its previous position and re-added here (or re-sorted) with byte-identical digests — ~18 ledger lines that are not wire changes. This README says "every ledger line in a diff is a deliberate wire change someone chose to make," and the re-sort buries the real instance-ID digest changes. Keep the re-sort in its own commit or revert it; the moved entries carry unchanged hashes, so nothing about them needed to change.</comment>
<file context>
@@ -178,14 +172,17 @@
"src/remote/upload-stream.ts#streamFileToHttpRequest": "sha256:ce07ea33a275e06e4cceaf0c75a079bd0c7938f36df6817407dbb1cc8c4ff900",
- "src/remote/upload-stream.ts#streamFileToHttpRequestAttempt": "sha256:aa73fb51890ca5e43d4aa80c1d2124b574568380a73b19eae7da0ee3e3e7acf8"
+ "src/remote/upload-stream.ts#streamFileToHttpRequestAttempt": "sha256:aa73fb51890ca5e43d4aa80c1d2124b574568380a73b19eae7da0ee3e3e7acf8",
+ "src/request-progress-protocol.ts#DaemonProgressEnvelope": "sha256:16162d01cfc43fc6a0198dbba3358981c1a278a70f7494ebe70d051f517cfb22",
+ "src/request-progress-protocol.ts#DaemonResponseEnvelope": "sha256:202ae64836af890549a1d95963903f9a506294b80bd0862c1b708e72d7f286d0",
+ "src/request-progress-protocol.ts#isDaemonProgressEnvelope": "sha256:e38ec26b64d20e257ff00860548e9c12ac22faf5a6308749d4715129814b381f",
</file context>
| headers, | ||
| }, | ||
| (res) => { | ||
| const identityCheck = verifyRemoteInstance( |
There was a problem hiding this comment.
P3: The identity recheck runs readRemoteDaemonHealth (up to REMOTE_DAEMON_HEALTHCHECK_TIMEOUT_MS = 3000 ms plus network latency) inside the RPC's original request-envelope timeout, because timeoutHandle keeps running while identityCheck probes. The first command after a daemon or proxy restart can therefore be rejected as a request timeout even though the restarted daemon is reachable and its response has already arrived. Consider excluding the recheck probe from the request envelope (clear the envelope while rechecking, or give the probe its own budget) so a detected restart surfaces the response after verification instead of racing the command timeout.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon-client/daemon-client-transport.ts, line 511:
<comment>The identity recheck runs `readRemoteDaemonHealth` (up to REMOTE_DAEMON_HEALTHCHECK_TIMEOUT_MS = 3000 ms plus network latency) inside the RPC's original request-envelope timeout, because `timeoutHandle` keeps running while `identityCheck` probes. The first command after a daemon or proxy restart can therefore be rejected as a request timeout even though the restarted daemon is reachable and its response has already arrived. Consider excluding the recheck probe from the request envelope (clear the envelope while rechecking, or give the probe its own budget) so a detected restart surfaces the response after verification instead of racing the command timeout.</comment>
<file context>
@@ -429,27 +508,49 @@ async function sendHttpRequest(
headers,
},
(res) => {
+ const identityCheck = verifyRemoteInstance(
+ info,
+ res.headers ?? {},
</file context>
| await assert.rejects(request('second')); | ||
| failRpc = false; | ||
| rpcProtocolVersion = DAEMON_RPC_PROTOCOL_VERSION + 1; | ||
| await assert.rejects(request('second'), /RPC protocol is incompatible/); |
There was a problem hiding this comment.
P3: This assertion matches the error by message text, which the repo convention explicitly avoids ('Key on typed reasons/details, never error text; no message sniffs' in AGENTS.md). The thrown AppError carries the typed detail remoteRpcProtocolVersion (and code: 'COMMAND_FAILED' shared with every other daemon failure), so the failure can be distinguished by details instead of the message. Same for the second /RPC protocol is incompatible/ assertion at the incompatible-instance step.
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-health-cache.test.ts, line 67:
<comment>This assertion matches the error by message text, which the repo convention explicitly avoids ('Key on typed reasons/details, never error text; no message sniffs' in AGENTS.md). The thrown AppError carries the typed detail `remoteRpcProtocolVersion` (and `code: 'COMMAND_FAILED'` shared with every other daemon failure), so the failure can be distinguished by details instead of the message. Same for the second `/RPC protocol is incompatible/` assertion at the `incompatible-instance` step.</comment>
<file context>
@@ -0,0 +1,110 @@
+ await assert.rejects(request('second'));
+ failRpc = false;
+ rpcProtocolVersion = DAEMON_RPC_PROTOCOL_VERSION + 1;
+ await assert.rejects(request('second'), /RPC protocol is incompatible/);
+ assert.deepEqual(paths.slice(5), ['POST /rpc', 'GET /health']);
+ rpcProtocolVersion = DAEMON_RPC_PROTOCOL_VERSION;
</file context>
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
Reviewed at 71df684. With a cached health entry, one RPC can still reach an incompatible daemon after a restart on the same URL. When the cached entry for Could that pre-dispatch check replace the post-RPC identity code? Then Not blocking: the cache in CI: both Smoke Tests jobs are still queued. Smoke uses the local daemon route, and the remote cache is active only when |
71df684 to
997f73f
Compare
There was a problem hiding this comment.
All reported issues were addressed across 12 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
Reviewed at 997f73f. The evidence gap from the earlier review is closed, and the code is ready for human review. Not blocking: the daemon refuses a stale instance before the auth hook, but the proxy checks auth first. So an unauthenticated request with a stale header gets 409 from the daemon and 401 from the proxy (http-server.ts). Also, both transport tests use a hand-written 409 response as the daemon instead of a real One question: the instance check covers one proxy and its upstream. Would a chain of two proxies miss a restart of the innermost daemon? Main has the same limit, so this is only worth handling if chained proxies are supported. CI: both Smoke Tests jobs are still queued. They run the local daemon route, which never sends the instance header, so overlap with this change is small. |
thymikee
added
the
ready-for-human
1a291fd to
54d8598
Compare
54d8598 to
1a285e2
Compare
|
Reviewed at 1a285e2. The follow-up commits look correct: the stale-instance refusal now runs after authentication on both the daemon and the proxy, and the restart probe and retry stay inside the original request deadline. This is ready for human review. Not blocking: the new token check in http-server.ts repeats the request-router.ts check and changes the JSON-RPC error code for a bad-token RPC from -32000 to -32001 (HTTP 401 and The two Smoke Tests jobs are still queued. Local clients send the right token and never send the instance header, so overlap with this change is small. |
|
1 participant
Summary
Cache successful remote
/healthprobes by daemon URL, token, PID, and advertised instance. Subsequent commands use one RPC. The daemon and proxy refuse stale instance preconditions before dispatch; the client re-probes compatibility and retries once within the original request deadline. Authentication precedes instance refusal. Transport failures invalidate the cache; ordinary command errors do not. Legacy peers retain per-command probes. Closes #2650.The daemon refusal lives in a focused HTTP module;
http-server.tsis 995 lines. The early bad-token response preserves the prior JSON-RPC-32000code, HTTP 401, andUNAUTHORIZEDdata. ADR 0006, the wire ledger, and planted-red mutations cover direct and proxied framing. Scope: 17 files, 996 changed lines.Validation
Commit: b154c20, rebased on
main8fe03de. Exact-headpnpm check:affected --runpassed: format, lint, typecheck, layering, fallow, build, 167 related files / 1,081 tests, and released wire compatibility against v0.21.16. Focused tests cover authentication order, restart refusal, bounded retry, protocol skew, proxy forwarding, and legacy peers. The new wire assertion failed on the prior head (-32001versus-32000) and passes after the fix. The static import passes all 753 eager-closure budget cases. Restoring the old uncapped health call made the delayed-probe test fail after about one second; the fixed test passes in about 160 ms. New-head CI pending.