fix(pegboard): add threshold parameters to runner protocol metadata#4247
Conversation
|
🚅 Deployed to the rivet-pr-4247 environment in rivet-frontend
|
|
test comment - please ignore |
|
simple test 2 |
|
test with gt sign and lt sign |
PR Review: fix(pegboard): add threshold parameters to runner protocol metadataOverviewThis PR bumps the runner protocol to v6, adding Critical BugMissing closing brace in The // Current (broken):
pub fn runner_event_demuxer_max_last_seen_ms(&self) -> u64 {
self.runner_event_demuxer_max_last_seen_ms.unwrap_or(30_000)
pub fn serverless_drain_grace_period(&self) -> u64 {
self.serverless_drain_grace_period.unwrap_or(10_000)
}
}
// Should be:
pub fn runner_event_demuxer_max_last_seen_ms(&self) -> u64 {
self.runner_event_demuxer_max_last_seen_ms.unwrap_or(30_000)
}
pub fn serverless_drain_grace_period(&self) -> u64 {
self.serverless_drain_grace_period.unwrap_or(10_000)
}This will fail to compile as Issues1. Shared definition mutation in
// Lines 509–527: mutates the shared definition
definition.config.options.onSleepTimeout = Math.min(...);
definition.config.options.onDestroyTimeout = Math.min(...);2. Missing documentation update Per the project's CLAUDE.md, config changes in Minor Notes
Positives
|
3008d9c to
13b1948
Compare
13b1948 to
76e52b6
Compare
76e52b6 to
ce7323c
Compare
121bf70 to
3da1f9e
Compare
3da1f9e to
1cefffe
Compare
ce7323c to
30accfe
Compare
1cefffe to
f33dd50
Compare
30accfe to
0bb287d
Compare
f33dd50 to
a49f2da
Compare
0bb287d to
9c03cf4
Compare
PR Review: fix(pegboard): add threshold parameters to runner protocol metadataSummaryThis PR introduces protocol v6, adding Overall this is a solid, well-structured change. A few items worth checking: Issues1. BigInt vs number comparison in
if (
\!this.#runnerLostTimeout &&
this.#protocolMetadata &&
this.#protocolMetadata.runnerLostThreshold > 0 // ← BigInt > number throws TypeError
)The old code explicitly converted to this.#protocolMetadata.runnerLostThreshold > 0nThe division in the log message already uses 2. Wrong comment in
This is a copy-paste artifact from 3. Behavioral change in The old code checked // old
let has_valid_serverless = runner_config_res
.first()
.map(|runner| match &runner.config.kind {
RunnerConfigKind::Serverless { max_runners, .. } => *max_runners \!= 0,
_ => false,
})
.unwrap_or_default();The new code only checks if the 4. Old code: (CrashPolicy::Sleep, false, false) => Ok(AllocateActorOutputV2 {
serverless: for_serverless, // could be true
status: AllocateActorStatus::Sleep,
}),New code: (CrashPolicy::Sleep, false, false) => Ok(AllocateActorOutputV2 {
serverless: false, // always false
status: AllocateActorStatus::Sleep,
}),This is a behavioral change — when a Sleep-policy actor falls into the sleep path, it's now never treated as serverless. If intentional, a comment would help clarify why. 5. if (protocolMetadata.serverlessDrainGracePeriod) {
const drainMax = Math.max(Number(protocolMetadata.serverlessDrainGracePeriod) - 1000, 0);
...
}If the drain grace period is explicitly configured to if (protocolMetadata.serverlessDrainGracePeriod \!= null) {Minor / Nits6. Repeated inline ternary in The override min-clamping pattern is repeated four times: this.overrides.runStopTimeout \!== undefined
? Math.min(this.#config.options.runStopTimeout, this.overrides.runStopTimeout)
: this.#config.options.runStopTimeoutA small private helper (e.g., 7.
Positives
|
|
|
||
| // Create actor instance | ||
| const definition = lookupInRegistry(this.#config, actorConfig.name); | ||
|
|
There was a problem hiding this comment.
a49f2da to
cc4d89e
Compare
9c03cf4 to
209ff35
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: