Repository navigation
feat(remote): name the held lease on upload, artifact download and diagnostics requests - #3594
Conversation
…agnostics requests
Size Report
Startup median (7 runs, lower is better):
|
|
The lease header change in 0bd157d looks right to me, but Coverage fails on the test-file-size ratchet that this PR caused: Not blocking, and you can take or leave these: the scope headers (auth, tenant, lease) are now built by hand at six sites in Could No conflicts. |
* origin/main: (24 commits) feat(ios): interpret React Native accessibility value semantics (#3589) fix(android): reinstall and reselect the test IME helper after it disappears (#3606) feat(snapshot): carry provider keyboard observations through acquisition (#3593) chore(web): bump managed agent-browser backend to 0.39.0 (#3591) fix(fold): report a dispatched pose as unverified when devicectl has no hinge-angle stream (#3587) feat(android): expose system-bar membership on snapshot nodes (#3585) fix(android): align ADB and Appium field values (#3584) test(closure): read committed JSON modules on the merge-base side (#3602) fix(ci): preserve manual Android suite timing (#3554) refactor(daemon): make the layer folders the declaration (PR 10) (#3520) refactor(move): extract the daemon server into @agent-device/daemon (11b) (#3546) refactor(daemon): package-local runtime fakes for the remaining daemon ports [11a-4] (#3544) refactor(daemon): cut daemon test edges into root data fixtures and fakes [11a-3] (#3517) refactor(11a-2): package-local fakes at the daemon ports; root-own command-surface composition tests (#3519) test(replay): verify Settings queries and bound scrolling (#3560) feat(snapshot): measure Android Appium viewport (#3586) fix(ios): verify a fill whose unidentified field moved while it kept focus (#3596) refactor(coverage): one fallow config with per-function CRAP ceilings (#3588) test: bound CLI promotion cases and align provider budget (#3590) fix(metro): report what the dev-server stop proved instead of assuming success (#3575) ... # Conflicts: # src/remote/daemon-artifacts.ts
…eader constant Collapse the four upload lease tests into one table over the direct, legacy and foreign-origin flows, each run with and without a lease, so upload-client.test.ts stays under its merge-base length. Export DAEMON_HTTP_LEASE_HEADER so tests read the header name from contracts.
|
New head is 373ee4d. I merged main into the branch. The only conflict was an import in
Checks run: |
|
The PR is ready at 373ee4d. The lease header now reaches upload, artifact download and diagnostics requests, and the end-to-end lease-header test is in place, so what the earlier review at 0bd157d was waiting on is now covered. Not blocking, and fine to take or leave: artifact download reads the lease from |
There was a problem hiding this comment.
2 issues found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/remote/upload-client.ts">
<violation number="1" location="src/remote/upload-client.ts:357">
P2: A daemon-origin ticket can redirect to a different origin, and `streamFileToHttpRequest` reuses these headers for every redirect, sending the lease ID to that foreign host. Strip the lease header whenever a redirect crosses origins.</violation>
</file>
<file name="test/wire-compat/ledger.json">
<violation number="1" location="test/wire-compat/ledger.json:413">
P2: The new lease header’s shared contract is not covered by the wire ledger: changes to its name or to `buildDaemonHttpLeaseHeaders`’ omission behavior will not change a tracked digest. Add both declarations to the surface and ledger, with compatible-change acknowledgements.</violation>
</file>
Reply to a comment to ask cubic a question or push back. It learns from your replies.
View guided diff | Turn on auto-fix | Re-trigger cubic
| // too; a ticket elsewhere (a signed storage URL) gets only the headers the daemon handed out. | ||
| headers: | ||
| url.origin === new URL(options.normalizedBase).origin | ||
| ? { ...ticket.headers, ...buildDaemonHttpLeaseHeaders(options.leaseId) } |
There was a problem hiding this comment.
P2: A daemon-origin ticket can redirect to a different origin, and streamFileToHttpRequest reuses these headers for every redirect, sending the lease ID to that foreign host. Strip the lease header whenever a redirect crosses origins.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/remote/upload-client.ts, line 357:
<comment>A daemon-origin ticket can redirect to a different origin, and `streamFileToHttpRequest` reuses these headers for every redirect, sending the lease ID to that foreign host. Strip the lease header whenever a redirect crosses origins.</comment>
<file context>
@@ -317,15 +336,26 @@ function isStringRecord(value: unknown): value is Record<string, string> {
+ // too; a ticket elsewhere (a signed storage URL) gets only the headers the daemon handed out.
+ headers:
+ url.origin === new URL(options.normalizedBase).origin
+ ? { ...ticket.headers, ...buildDaemonHttpLeaseHeaders(options.leaseId) }
+ : ticket.headers,
payloadPath: artifact.payloadPath,
</file context>
| "rationale": "The health-probe budget includes requester loading and response completion. The caller AbortSignal joins the probe timer only in-process; caller cancellation remains distinct from probe expiry. No signal, timedOut flag, request route, auth header or accepted health payload field changes on the wire, so released daemons and proxies parse and respond as before (ADR 0006)." | ||
| }, | ||
| { | ||
| "declaration": "src/remote/remote-request-diagnostics.ts#RemoteDiagnosticsEndpoint", |
There was a problem hiding this comment.
P2: The new lease header’s shared contract is not covered by the wire ledger: changes to its name or to buildDaemonHttpLeaseHeaders’ omission behavior will not change a tracked digest. Add both declarations to the surface and ledger, with compatible-change acknowledgements.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At test/wire-compat/ledger.json, line 413:
<comment>The new lease header’s shared contract is not covered by the wire ledger: changes to its name or to `buildDaemonHttpLeaseHeaders`’ omission behavior will not change a tracked digest. Add both declarations to the surface and ledger, with compatible-change acknowledgements.</comment>
<file context>
@@ -433,6 +408,51 @@
"rationale": "The health-probe budget includes requester loading and response completion. The caller AbortSignal joins the probe timer only in-process; caller cancellation remains distinct from probe expiry. No signal, timedOut flag, request route, auth header or accepted health payload field changes on the wire, so released daemons and proxies parse and respond as before (ADR 0006)."
+ },
+ {
+ "declaration": "src/remote/remote-request-diagnostics.ts#RemoteDiagnosticsEndpoint",
+ "digest": "sha256:c1d5fe2fe90c425502cf8b7c87d6606374d6af65b4e9faf942a416f683930bdc",
+ "rationale": "Clients name a held agent-device lease in an optional x-agent-device-lease-id request header on upload, artifact download and request diagnostics HTTP calls, so a gateway can route them. Released daemons ignore unknown request headers and the proxy forwards only its allowlist, so routes, bodies and responses parse as before; with no lease held the request is unchanged (ADR 0006)."
</file context>
* origin/main: feat(leasing): proxy CLI names each new lease, daemon keeps a caller-chosen lease id (#3479) feat(remote): name the held lease on upload, artifact download and diagnostics requests (#3594) fix(android): record the launched intent owner from the device (#3600) fix(selectors): type rect-requiring targets as addressable (#3608) feat(snapshot): interpret iOS node facts once at producer seams (#3592) refactor(gates): own the client-only replay rule as daemon-process reachability (R82) (#3604)
* origin/main: feat(remote): name the held lease on upload, artifact download and diagnostics requests (#3594) fix(android): record the launched intent owner from the device (#3600) fix(selectors): type rect-requiring targets as addressable (#3608) feat(snapshot): interpret iOS node facts once at producer seams (#3592) refactor(gates): own the client-only replay rule as daemon-process reachability (R82) (#3604)
* origin/main: feat(remote): name the held lease on upload, artifact download and diagnostics requests (#3594) fix(android): record the launched intent owner from the device (#3600) fix(selectors): type rect-requiring targets as addressable (#3608) feat(snapshot): interpret iOS node facts once at producer seams (#3592) refactor(gates): own the client-only replay rule as daemon-process reachability (R82) (#3604)
Summary
A gateway in front of many daemons routes each request to the daemon that holds the request's lease. RPC calls already name the lease in their body, but the plain HTTP calls do not, so such a gateway cannot route them:
POST /upload,POST /upload/preflight,PUT /upload/direct/<ticket>,POST /upload/finalizeGET /artifacts/<id>GET /sessions/<s>/requests/<r>/diagnosticsWhen the client talks to a remote daemon and holds a lease, it now sends
x-agent-device-lease-id: <leaseId>on these requests. This is the same lease id it puts in its RPC calls. With no lease held, it sends no header. A direct upload ticket on another origin, such as a signed storage URL, gets only the headers the daemon handed out.Nothing reads the header yet. The daemon ignores it, and the proxy forwards only its allowlist of headers, so it drops this one. The client never sends
GET /artifacts(the list), so that route is unchanged.The wire-compat ledger gets new digests and additive acks for the changed client declarations. The stale #2946 acks on the same upload functions are dropped.
Validation
pnpm check:affected --run: all runnable checks passed (format, lint, typecheck, layering, fallow, build, vitest-related 366 files / 2465 tests, daemon-wire-compat).daemon-client-lease-header.test.tsrunssendToDaemonagainst a loopback remote daemon (install upload, screenshot download, failing command diagnostics). With a lease, every one of these requests carries the header. Without a lease, none does. The lease test fails when the call-site wiring is removed.upload-client,artifact-download,remote-request-diagnosticsanddaemon-httpcover the header on each request kind, its absence without a lease, and that a foreign-origin ticket gets no header.