-
Notifications
You must be signed in to change notification settings - Fork 129
feat(pb): get multi-actors working e2e, add resources for builds #2465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(pb): get multi-actors working e2e, add resources for builds #2465
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR implements multi-actor support and resource management for builds, with significant changes across the codebase to support allocation types and optional resource specifications.
- Added new
BuildsAllocationtypes in SDKs to support both single and multi-slot allocations, with proper serialization and validation - Made actor resources optional by changing
resourcesfield from required to optional inActorsActortype, affecting database schema and API endpoints - Renamed build-related types for better consistency (e.g.,
BuildsBuildKind->BuildsKind,BuildsBuildCompression->BuildsCompression) - Fixed critical bug in
managerClient.tswheresignal_actorhandler incorrectly referencespacket.start_actorinstead ofpacket.signal_actor - Added proper resource validation in
pegboardservice based on build allocation type (None, Single, Multi) with CPU millicores and memory MiB specifications
139 file(s) reviewed, 44 comment(s)
Edit PR Review Bot Settings | Greptile
| else process.exit(exitCode); | ||
| process.exit(exitCode); | ||
|
|
||
| return c.text("unreachable"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: unreachable code after process.exit() should be removed
| import { createAndStartUdpServer } from "./udpServer.js"; | ||
| import { connectToManager } from "./managerClient.js"; | ||
|
|
||
| let injectWebSocket: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: avoid using 'any' type - should define proper type from @hono/node-ws
| const server = serve({ fetch: app.fetch, port }); | ||
| injectWebSocket(server); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: server instance should be handled for graceful shutdown (e.g., process.on('SIGTERM'))
|
|
||
| createAndStartUdpServer(); | ||
|
|
||
| if (process.env.MULTI) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: use explicit boolean check for environment variable
| if (process.env.MULTI) { | |
| if (process.env.MULTI === 'true') { |
| }); | ||
|
|
||
|
|
||
| const port2 = Number.parseInt(portEnv); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: port validation should happen before socket creation to avoid creating resources that may need cleanup
| #[serde(rename = "content_length")] | ||
| pub content_length: i64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: content_length is typed as i64 but documented as 'Unsigned 64 bit integer' - this could allow negative values when it shouldn't
| #[serde(rename = "content_length")] | |
| pub content_length: i64, | |
| #[serde(rename = "content_length")] | |
| pub content_length: u64, |
|
|
||
|
|
||
|
|
||
| #[derive(Clone, Debug, PartialEq, Default, Serialize, Deserialize)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Missing explicit serde crate import. While it may work due to prelude, explicit imports improve code clarity
| export interface Allocation { | ||
| single?: Rivet.builds.AllocationSingle; | ||
| multi?: Rivet.builds.AllocationMulti; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Both fields being optional (marked with ?) means an Allocation could potentially have neither single nor multi set. Consider if this is the intended behavior or if at least one should be required.
| ); | ||
|
|
||
| export declare namespace Kind { | ||
| export type Raw = "docker_image" | "oci_bundle" | "javascript"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Raw type should be marked as readonly to prevent accidental modification of valid enum values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Removing this file will break build compression type serialization unless this functionality has been moved elsewhere. Need to verify that equivalent functionality exists in another location before proceeding with deletion.
9b820e0 to
ed03e29
Compare
d94da15 to
14ec025
Compare
ed03e29 to
869e278
Compare
14ec025 to
6a71c38
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @MasterPtato's task —— View job PR Review: Multi-Actor Support and Resource Management
|
|
Claude finished @MasterPtato's task —— View job PR Review: Multi-Actor Support and Resource Management
|
f54d281 to
c170c16
Compare
9750b18 to
459a987
Compare
|
Claude finished @MasterPtato's task —— View job PR Review: Multi-Actor Support and Resource Management
|
|
Claude finished @MasterPtato's task —— View job PR Review: Multi-Actor Support and Resource Management
|
c170c16 to
f028f0c
Compare
459a987 to
355ea0f
Compare
|
Claude finished @MasterPtato's task —— View job PR Review: Multi-Actor Support and Resource Management
|
|
Claude finished @MasterPtato's task —— View job PR Review: Multi-Actor Support and Resource Management
|
f028f0c to
ec0407c
Compare
355ea0f to
4d16edc
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @MasterPtato's task —— View job PR Review: Multi-Actor Support and Resource Management
|
|
Claude finished @MasterPtato's task —— View job PR Review: Multi-Actor Support and Resource Management
|
ec0407c to
3b7227e
Compare
4d16edc to
0a4329c
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |


Changes