feat: basic envoy tunnel impl#4534
feat: basic envoy tunnel impl#4534MasterPtato wants to merge 1 commit into03-27-chore_envoy_clientfrom
Conversation
|
🚅 Deployed to the rivet-pr-4534 environment in rivet-frontend
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Code Review: PR 4534 - feat: basic envoy tunnel implSummary: This draft PR implements a basic HTTP tunnel from Guard through the Envoy to actors, allowing actors to handle HTTP requests via the fetch callback. It also refactors startEnvoy to return an EnvoyHandle, adds eviction support on the connection task, and fixes several pre-existing bugs. Critical Bugs1. Map keys using array literals will never match JavaScript Map uses reference equality for object/array keys. Two separately created arrays [gatewayId, requestId] will never be equal by reference, so .get() and .delete() always return undefined or false. Affected files:
The entire streaming request flow (request chunks and aborts) is silently broken. Fix: use a composite string key, e.g., idToStr(gatewayId) + ":" + idToStr(requestId) using the existing idToStr helper in utils.ts. 2. protocolMetadata maxPayloadSize references a non-existent field In tunnel.ts line 83, the field maxPayloadSize does not exist on ProtocolMetadata. The correct field name is maxResponsePayloadSize. This comparison is always against undefined, so the payload size limit is never enforced. 3. requestToActor map is never populated handleRequestStart forwards the message to the actor but never inserts into ctx.requestToActor. The map exists but has no .set() call, so subsequent request-chunk and request-abort messages always fail silently to find the associated actor. Logic / Correctness Issues4. startEnvoy can hang forever if init is never received In engine/sdks/typescript/envoy-client/src/tasks/envoy/index.ts, startRx.changed() awaits indefinitely if the WebSocket connection fails before ToEnvoyInit is received. There is no timeout or error path. 5. Errors in spawned response handlers are silently dropped In actor.ts lines 215 and 235, errors thrown inside spawn() (e.g., from sendResponse when body is too large) are not caught, so failed response sends go unlogged. 6. SIGINT handler logs SIGTERM In test-envoy/src/index.ts, the SIGINT handler prints received SIGTERM instead of received SIGINT. Code Quality Issues7. Commented-out code in production files Large blocks of commented-out code were committed and should be removed: the webSockets map and initialization in actor.ts, and the large streamSSE and WebSocket handler bodies in test-envoy/src/index.ts. Per project conventions, documenting removed code is not useful. 8. as any cast without explanation actor.ts line 204 casts to any for duplex half which is not in standard RequestInit. The cast is necessary for a Node.js fetch quirk but deserves a comment. 9. let instead of const for variables never reassigned tunnel.ts lines 50, 64: let actor = findActor() should be const. 10. Log level too alarming for expected reconnect window connection.ts line 166: wsSend logs at error level when the WebSocket is not connected. During normal reconnect windows this is expected, so warn is more appropriate. Rust Changes - Positive ObservationsThe Rust-side changes look clean and correct:
Summary
|
1 participant
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: