-
Notifications
You must be signed in to change notification settings - Fork 104
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
base: 05-16-feat_pb_add_runner_logs
Are you sure you want to change the base?
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
BuildsAllocation
types in SDKs to support both single and multi-slot allocations, with proper serialization and validation - Made actor resources optional by changing
resources
field from required to optional inActorsActor
type, affecting database schema and API endpoints - Renamed build-related types for better consistency (e.g.,
BuildsBuildKind
->BuildsKind
,BuildsBuildCompression
->BuildsCompression
) - Fixed critical bug in
managerClient.ts
wheresignal_actor
handler incorrectly referencespacket.start_actor
instead ofpacket.signal_actor
- Added proper resource validation in
pegboard
service 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
@@ -41,8 +37,7 @@ export function createAndStartServer( | |||
const query = c.req.query("code"); | |||
const exitCode = query ? Number(query) : 0; | |||
|
|||
if (typeof Deno != "undefined") Deno.exit(exitCode); | |||
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
6a71c38
to
8eb564a
Compare
869e278
to
3d46e7d
Compare
8eb564a
to
f1efbe3
Compare
3d46e7d
to
3917cbb
Compare
f1efbe3
to
bad492a
Compare
3917cbb
to
55980f3
Compare
bad492a
to
952db69
Compare
55980f3
to
5677a5b
Compare
5677a5b
to
bbadc72
Compare
952db69
to
5a56a42
Compare
Changes