Conversation
|
🚅 Deployed to the rivet-pr-4553 environment in rivet-frontend
|
9df8c59 to
f789515
Compare
f789515 to
53cca42
Compare
53cca42 to
25b2a62
Compare
PR Review: feat: actor query in gatewayThis is a well-structured PR adding query-based actor routing to the gateway, allowing clients to route by actor name+namespace+key using matrix parameters rather than requiring a pre-resolved actor ID. The architecture is clean, the error handling is thorough, and test coverage is good. IssuesBase64 encoding inconsistency (potential bug) In actor_path.rs, the input parameter is decoded from URL-safe no-padding base64 (URL_SAFE_NO_PAD). In resolve_actor_query.rs, the raw bytes are re-encoded using standard base64 (STANDARD, with +/slash and padding) before being passed to the actor creation op. If downstream consumers expect URL-safe base64 consistent with what the client sent, inputs whose bytes map to + or slash would be encoded differently. Verify what encoding pegboard::ops::actor::create::Input.input expects and whether the TypeScript mirror in gateway.ts uses the same encoding. Potential panic in production path -- actor_path.rs, build_actor_query_from_gateway_params The validation above guards the runner_name expect() call, but expect panics in a hot request path. Prefer ok_or_else(|| errors::QueryMissingRunnerName.build())? to make the invariant explicit in the type system rather than relying on a panic that would crash the process if the guard is refactored away. Security: CBOR parsing without size bounds -- actor_path.rs, validate_cbor There is no maximum byte-length check before CBOR deserialization. A deeply-nested CBOR structure could exhaust stack space via recursive deserialization. The decoded bytes should be bounded (e.g., reject if len > 64 KB) before calling this function. Unused error artifact -- guard.query_ambiguous_runner_configs.json The artifact is defined but resolve_query_target_dc_label never raises it. It silently picks the first matching DC via .into_iter().next(). Either raise this error when more than one DC matches, or remove the artifact. A defined error that is never emitted will confuse anyone reading the error catalog. Error mapping gap -- guard.query_no_runner_configs.json This artifact exists, but the no-runner-config case raises pegboard::errors::Actor::NoRunnerConfigConfigured instead. Verify whether this maps to query_no_runner_configs on the wire. If not, clients receive an opaque pegboard error rather than the documented guard error. Style: missing newline at end of artifact JSON files All 13 new files under engine/artifacts/errors/ are missing the final newline. Positive observations
Questions
The base64 encoding inconsistency is the only potentially breaking item. The expect panic and CBOR size bound are the main items to address before merge. |
25b2a62 to
99c27ce
Compare
99c27ce to
41c6f98
Compare
41c6f98 to
34f50a8
Compare
34f50a8 to
b554e3d
Compare
b554e3d to
5ed38c2
Compare
Merge activity
|
5ed38c2 to
148d3f6
Compare

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: