chore(pegboard): allow configuring base_retry_timeout, actor_start_threshold, actor_stop_threshold, and retry_reset_duration

[NathanFlurry]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Nov 5, 2025 4:06am

3 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Nov 5, 2025 4:06am
rivet-inspector	Ignored	Preview		Nov 5, 2025 4:06am
rivet-site	Ignored	Preview		Nov 5, 2025 4:06am

[NathanFlurry]

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.
Learn more

• chore(pegboard): allow configuring reschedule_backoff_max_exponent #3370
• fix(next-js): auto shut down runners when source code updates #3369
• fix(rivetkit): fix duplicate runner key in serverless #3366
• chore(runner): improve runner connection logs #3365
• chore(rivetkit): add runner sse ping interval #3362
• chore(rivetkit): log close code when client ws closes #3361
• chore(rivetkit): log runner id with actor logs #3360
• chore(scripts): add endian converter script #3359
• fix(guard): fix url decoding actor id + token #3357
• chore(rivetkit): remove unneeded headers & protocols in requests #3356
• fix(rivetkit): reduce pegboard actor retry for spawned engines #3355
• chore(pegboard): allow configuring base_retry_timeout, actor_start_threshold, actor_stop_threshold, and retry_reset_duration #3354 👈 (View in Graphite)
• feat(pb): add reschedule_ts to actors #3363
• fix: fix all dashboards #3358
• chore: change log levels #3333
• chore: misc bug fixes, add logs for outbound req #3332
• fix: use correct vbare functions for runner tunnel #3300 : 1 other dependent PR (#3301 )
• feat(grafana): fix api dashboards #3193
• fix: docker compose build #3273
• chore: fix remaining warnings for all targets #3305
• fix(gas): fix from_workflow when creating standalone ctx from op ctx #3342
• ci: udpate ci filter to succeed if no relevant changes #3343
• main

---

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[claude]

Code Review

Summary

This PR refactors hard-coded timeout constants in the Pegboard actor workflow system into configurable parameters. This is a solid improvement that increases flexibility for different deployment environments.

✅ Strengths

1. Good Configuration Pattern: The new Pegboard config struct follows the existing pattern used in other config modules (e.g., ApiPublic, Logs) with:

   • Optional fields with sensible defaults
   • Accessor methods that unwrap with default values
   • Proper integration into the root config structure

2. Clean Refactoring: The removal of module-level constants and their replacement with config-driven values is well-executed:

   • BASE_RETRY_TIMEOUT_MS → ctx.config().pegboard().base_retry_timeout()
   • ACTOR_START_THRESHOLD_MS → ctx.config().pegboard().actor_start_threshold()
   • ACTOR_STOP_THRESHOLD_MS → ctx.config().pegboard().actor_stop_threshold()
   • RETRY_RESET_DURATION_MS → ctx.config().pegboard().retry_reset_duration()

3. Documentation: Good inline documentation explaining what each parameter does and noting they are "Experimental".

4. Backward Compatibility: Default values match the original constants, ensuring no behavior changes for existing deployments.

🔍 Issues & Suggestions

1. Type Inconsistency

pub base_retry_timeout: Option<usize>,  // usize
pub actor_start_threshold: Option<i64>, // i64
pub actor_stop_threshold: Option<i64>,  // i64
pub retry_reset_duration: Option<i64>,  // i64

Issue: base_retry_timeout uses usize while the other three use i64. This inconsistency is unusual since they all represent millisecond durations.

Recommendation: Consider using i64 for all timeout fields for consistency, or document why base_retry_timeout needs to be usize. Looking at the usage in runtime.rs:622, the Backoff::new_at function expects a usize, but for config consistency and to avoid potential overflow issues on 32-bit systems, i64 would be more appropriate (with a cast at the usage site if needed).

2. Parameter Passing in LifecycleState::new

pub fn new(runner_id: Id, runner_workflow_id: Id, actor_start_threshold: i64) -> Self {
    LifecycleState {
        // ...
        gc_timeout_ts: Some(util::timestamp::now() + actor_start_threshold),
        // ...
    }
}

Issue: Only actor_start_threshold is passed to the constructor, but the other config values are accessed directly from ctx.config() in the workflow. This creates an inconsistent pattern.

Consideration: This is likely fine since LifecycleState is constructed in the workflow context where config access is available everywhere else. However, for consistency, you might consider either:

• Pass all config values to the constructor, OR
• Don't pass any and access config inside the constructor (though this would require passing ctx or the config)

The current approach is acceptable but worth noting for consistency.

3. Missing Unit Tests

No tests appear to be modified or added to validate:

• Config parsing with custom values
• Config default values
• Behavior with different timeout configurations

Recommendation: Consider adding tests to verify:

• Default values are correctly applied
• Custom config values are properly parsed
• The configuration impacts workflow behavior as expected

4. Documentation Enhancement

The comments say "Unit is in milliseconds" but could be more explicit about:

• Valid ranges (e.g., minimum values)
• Performance implications of different values
• Recommended values for different use cases

Example:

/// How long to wait after creating and not receiving a starting state before setting actor as lost.
///
/// Unit is in milliseconds. Minimum recommended value: 5000 (5 seconds).
/// Default: 30000 (30 seconds).
///
/// **Experimental**
pub actor_start_threshold: Option<i64>,

🔒 Security

No security concerns identified. These are timeout configurations that don't introduce security risks.

⚡ Performance

No performance concerns. The config values are accessed through efficient unwrap_or operations.

📊 Test Coverage

As noted, test coverage for this configuration change appears to be missing. While the code changes are straightforward, tests would increase confidence that:

1. Config parsing works correctly
2. Default values are preserved
3. Custom values override correctly

✨ Overall Assessment

This is a solid, well-structured PR that improves configurability. The main suggestion is to address the type inconsistency for base_retry_timeout and consider adding test coverage. The code quality is good and follows existing patterns in the codebase.

Recommendation: ✅ Approve with minor suggestions for improvement.

[claude]

PR Review: Configure Pegboard Actor Timeouts and Retry Parameters

Summary

This PR extracts hard-coded constants for actor lifecycle timeouts and retry behavior into configurable parameters. The changes enable runtime configuration of:

• base_retry_timeout - Base delay for actor rescheduling after failure
• actor_start_threshold - Timeout for actor start operations
• actor_stop_threshold - Timeout for actor stop operations
• retry_reset_duration - Duration after which retry count resets

Code Quality ✅

Strengths:

1. Good separation of concerns - New config module follows existing patterns in packages/config/
2. Backwards compatibility - All parameters are Option<T> with sensible defaults matching previous constants
3. Clear documentation - Each field has descriptive comments explaining purpose and units
4. Consistent accessor pattern - Helper methods with unwrap_or() follow the repository's config patterns
5. Type consistency - Properly uses usize for base_retry_timeout and i64 for timestamp-based thresholds

Minor observations:

1. The **Experimental** markers in comments suggest these may change - good to document this
2. Import ordering in runtime.rs:1-2 was cleaned up (alphabetized) - good attention to detail

Potential Issues 🔍

1. Type inconsistency in defaults (Minor)

// In pegboard.rs:36
self.base_retry_timeout.unwrap_or(2000)

// vs in pegboard.rs:40
self.actor_start_threshold.unwrap_or(30_000)

The use of underscores for readability is inconsistent. Consider:

self.base_retry_timeout.unwrap_or(2_000)  // Add underscore for consistency

2. Missing validation (Low priority)
The config doesn't validate that values are reasonable. Consider adding validation or at least documenting valid ranges. For example:

• Should base_retry_timeout have a minimum value (e.g., > 0)?
• Should thresholds have maximum values to prevent excessively long timeouts?

This is likely fine for experimental parameters, but worth considering for production.

3. No migration path documented (Documentation)
If operators are currently using these defaults implicitly, they should be aware that:

• Default behavior remains unchanged
• Values can now be overridden via configuration
• The **Experimental** tag suggests values may need tuning

Performance Considerations ⚡

Positive:

• Using LazyLock for default config instances prevents unnecessary allocations
• Config lookups are cheap (pointer dereference + simple arithmetic)
• No performance regression from making these values configurable

Neutral:

• Multiple calls to ctx.config().pegboard().X() throughout the code - this is fine since config access is cheap and keeps code readable

Security Concerns 🔒

Low risk:

• No obvious security issues
• All parameters are internal configuration, not user-controllable inputs
• Validation on configuration values would be defense-in-depth but not critical

Consideration:

• Extremely low timeout values could potentially cause actors to be marked as lost prematurely
• Extremely high retry timeouts could delay recovery
• Document recommended ranges in production deployment guides

Test Coverage 📋

Observation:
No tests were added or modified. Given that:

1. This is configuration plumbing with straightforward logic
2. The existing hardcoded behavior is preserved via defaults
3. Tests would need integration-level setup to exercise these timeouts

This is acceptable, though consider:

• Integration tests that verify custom config values are respected
• Documentation or examples showing how to configure these values

Architecture & Design 🏗️

Excellent:

1. Follows repository patterns exactly (matching api_peer, api_public, etc.)
2. Properly integrated into root config structure
3. Clean refactoring - removed constants and threaded config through call sites

Threading config through the codebase:
The config is accessed via ctx.config().pegboard() at multiple call sites:

• mod.rs:223,311,335 - actor lifecycle transitions
• runtime.rs:42,621,674,731,738 - backoff calculations and threshold checks

This is the correct pattern for this codebase.

Recommendations 📝

1. Add underscore to 2000 for consistency: 2_000 in line 36
2. Document valid ranges in comments or external docs
3. Consider integration tests that verify config values are respected (optional)
4. Add configuration example to deployment docs showing how to tune these values

Verdict ✨

Approved - This is a clean, well-structured change that maintains backwards compatibility while adding needed flexibility. The code follows repository conventions and the refactoring is straightforward.

The minor suggestions above are optional improvements that don't block merging.

---

Review generated by Claude Code

[claude]

PR Review: Configurable Pegboard Timeouts

Summary

This PR extracts hardcoded timeout constants into configuration options for the Pegboard actor management system. The change allows for runtime configuration of retry timeouts, actor start/stop thresholds, and retry reset duration.

Code Quality ✅

Strengths:

• Clean refactoring that moves hardcoded constants to configuration
• Follows the existing configuration pattern using Option<T> with accessor methods providing defaults
• Maintains backward compatibility by preserving the original default values
• Good documentation with inline comments explaining each configuration parameter
• Properly marks new config options as **Experimental**

Consistency:

• Follows the established pattern in packages/config/src/config/mod.rs for optional config sections with LazyLock defaults
• Accessor methods (base_retry_timeout(), etc.) provide clean API with sensible fallbacks
• Integration points properly use ctx.config().pegboard().{method}() pattern

Potential Issues 🔍

1. Type Inconsistency (Minor)
packages/config/src/config/pegboard.rs:12-31

• base_retry_timeout uses Option<usize> while the other three timeout fields use Option<i64>
• This is inconsistent but matches the usage pattern (base_retry_timeout is passed to Backoff::new_at() which expects usize)
• Consider documenting why this field uses a different type, or evaluate if all timeout configs should use the same type for consistency

2. Magic Numbers in Defaults
packages/config/src/config/pegboard.rs:36-48

• Default values are now defined in two places: the config file and previously as constants in mod.rs
• The constants had descriptive names (e.g., ACTOR_START_THRESHOLD_MS, RETRY_RESET_DURATION_MS) that provided context
• The numeric literals in the unwrap_or() calls (e.g., 30_000, 10 * 60 * 1000) are less self-documenting
• Suggestion: Consider using named constants or the util::duration helpers for clarity:

  pub fn actor_start_threshold(&self) -> i64 {
      self.actor_start_threshold.unwrap_or(util::duration::seconds(30))
  }
  
  pub fn retry_reset_duration(&self) -> i64 {
      self.retry_reset_duration.unwrap_or(util::duration::minutes(10))
  }

Performance Considerations ✅

• No performance impact - configuration is accessed through already-existing context objects
• LazyLock ensures default configuration is only initialized once
• No additional allocations or runtime overhead

Security Concerns ✅

• No security issues identified
• Configuration values are runtime timeouts without privilege escalation concerns
• Using milliseconds as the unit is consistent with the codebase and prevents overflow issues with reasonable values

Test Coverage ⚠️

Missing:

• No tests added for the new configuration options
• No validation that invalid timeout values (e.g., negative numbers, zero) are handled appropriately
• No tests verifying the configuration is correctly applied to actor lifecycle management

Recommendations:

1. Add unit tests for the Pegboard configuration accessor methods
2. Consider adding integration tests that verify different timeout configurations affect actor behavior correctly
3. Add validation to prevent unreasonable timeout values (e.g., negative durations, extremely large values)

Best Practices ✅

Follows CLAUDE.md Guidelines:

• ✅ Uses hard tabs (Rust formatting)
• ✅ Follows existing import patterns
• ✅ Maintains workspace dependency structure
• ✅ Uses anyhow for error handling
• ✅ Timestamp values use i64 milliseconds
• ✅ Configuration follows existing patterns

Minor Observations

Documentation:

• The "Unit is in milliseconds" comment is helpful, but consider if there's a way to make this more type-safe (e.g., using a Duration type or newtype wrapper)
• All fields marked as experimental - consider adding a comment explaining when they might be stabilized

Code Organization:

• Clean removal of constants from mod.rs - no dead code left behind
• Import cleanup in runtime.rs properly removes unused constant imports

Recommendations

High Priority:

1. Consider using util::duration helpers in default values for better readability

Medium Priority:
2. Add basic unit tests for configuration accessors
3. Document the type inconsistency for base_retry_timeout (usize vs i64)
4. Consider adding validation for timeout values

Low Priority:
5. Add integration tests for different timeout configurations
6. Consider a Duration type wrapper for compile-time unit safety

Verdict

This is a solid refactoring that improves configurability without breaking existing functionality. The code quality is good and follows established patterns. Main areas for improvement are around testing and code documentation clarity.

Approval Status: ✅ Approved with suggestions

The changes are safe to merge as-is, but would benefit from the recommendations above in follow-up work.

[claude]

Code Review

This PR makes Pegboard actor lifecycle configuration tunable by moving hard-coded constants into a new configuration structure. Overall, the implementation is clean and well-structured. Here are my findings:

Strengths

1. Good separation of concerns - New configuration module follows existing patterns in the codebase
2. Backward compatibility - Default values preserve existing behavior
3. Clear documentation - All fields have descriptive comments explaining their purpose and units
4. Consistent naming - Follows the existing naming conventions in the codebase

Issues and Recommendations

1. Type Inconsistency (Minor Bug Risk)

base_retry_timeout uses usize while the other three fields use i64. This inconsistency could lead to confusion:

engine/packages/config/src/config/pegboard.rs:12

pub base_retry_timeout: Option<usize>,

vs.

engine/packages/config/src/config/pegboard.rs:18,24,31

pub actor_start_threshold: Option<i64>,
pub actor_stop_threshold: Option<i64>,
pub retry_reset_duration: Option<i64>,

Recommendation: Consider using i64 consistently for all timeout/threshold values to match the timestamp patterns in the codebase (per CLAUDE.md: "Store dates as i64 epoch timestamps in milliseconds"). If usize is needed internally, convert at usage sites.

2. Missing Constants Documentation

The old constants had inline comments explaining their purpose. When removing them, the context is now only in the config file comments, which might not be visible to developers reading the workflow code.

engine/packages/pegboard/src/workflows/actor/mod.rs:10-12 (removed)

- /// Time to delay an actor from rescheduling after a rescheduling failure.
- const BASE_RETRY_TIMEOUT_MS: usize = 2000;

Recommendation: Consider adding a brief comment where these values are used, e.g.:

// Get retry timeout from config (experimental tunable parameter)
let mut backoff = reschedule_backoff(
    state.reschedule_state.retry_count,
    ctx.config().pegboard().base_retry_timeout(),
);

3. Magic Number in Default Value

engine/packages/config/src/config/pegboard.rs:48

pub fn retry_reset_duration(&self) -> i64 {
    self.retry_reset_duration.unwrap_or(10 * 60 * 1000)
}

Recommendation: Consider using util::duration::minutes(10) for consistency with the codebase patterns, or add a comment explaining "10 minutes in milliseconds".

4. Experimental Features - Testing Concern

All four parameters are marked as **Experimental**, but there are no tests validating their behavior. This is risky for production configuration changes.

Recommendation: Add integration or unit tests that:

• Verify default values are applied correctly
• Test that custom config values are respected
• Validate the backoff behavior with different base_retry_timeout values
• Test GC timeout behavior with custom thresholds

5. Config Validation Missing

There's no validation for these timeout values. Invalid values (e.g., negative numbers for i64 fields, or extremely small values) could cause unexpected behavior.

Recommendation: Add validation in the config module:

impl Pegboard {
    pub fn validate(&self) -> Result<(), &'static str> {
        if let Some(t) = self.actor_start_threshold {
            if t <= 0 { return Err("actor_start_threshold must be positive"); }
        }
        // Similar for other fields
        Ok(())
    }
}

Performance Considerations

No performance concerns - the config values are cached via LazyLock in the root config, so there's no runtime overhead.

Security Considerations

No security issues identified. These are internal operational parameters.

Code Quality

Style adherence: Code follows the CLAUDE.md conventions:

• Uses unwrap_or for defaults ✓
• Proper derive macros ✓
• Workspace dependencies added correctly ✓

Minor: Import reordering in runtime.rs:1-2 (base64 imports) - this is fine but appears to be an incidental formatting change.

Test Coverage

⚠️ No tests found for this functionality. Given that these are marked experimental and affect critical actor lifecycle behavior, tests are strongly recommended.

Summary

The implementation is solid and well-integrated. The main concerns are:

1. Type consistency for timeout values
2. Lack of validation for config values
3. Missing test coverage for experimental features

Recommendation: Approve with minor changes - consider addressing type consistency and adding basic validation before merge. Tests can be added in a follow-up PR if needed.

[claude]

Code Review for PR #3354

Summary

This PR makes several hardcoded Pegboard timeout and retry parameters configurable through the config system. The implementation follows the existing configuration patterns and appears to be well-structured.

Positive Aspects ✅

1. Good Configuration Pattern: The new pegboard.rs config module follows the established patterns in the codebase:

   • Uses Option<T> for all fields (as required by the config system)
   • Provides getter methods with sensible defaults
   • Properly integrated into the Root config struct with LazyLock

2. Maintains Backwards Compatibility: All default values match the previous hardcoded constants, ensuring no behavioral changes for existing deployments.

3. Clear Documentation: The doc comments explain the purpose and units (milliseconds) for each parameter, and they're marked as "Experimental".

4. Consistent Refactoring: The constants are properly removed and replaced with config calls throughout the affected files.

Issues & Recommendations 🔍

1. Type Inconsistency ⚠️

Severity: Medium

There's a type inconsistency in base_retry_timeout:

• Config defines it as usize (packages/config/src/config/pegboard.rs:12, :36)
• But other similar timeout parameters use i64 (:18, :24, :31)

Recommendation: Consider using i64 for base_retry_timeout for consistency. While usize works, having consistent types across related configuration parameters reduces cognitive load and potential casting issues.

// Current
pub base_retry_timeout: Option<usize>,

// Suggested
pub base_retry_timeout: Option<i64>,

2. Magic Numbers in Defaults 📝

Severity: Low

The default values are still "magic numbers" without context:

• 30_000 - What does 30 seconds represent?
• 10 * 60 * 1000 - 10 minutes, but why?

Recommendation: Add inline comments or use constants to make the defaults more self-documenting:

pub fn actor_start_threshold(&self) -> i64 {
    // Default to 30 seconds - actors are marked lost if they don't start within this window
    self.actor_start_threshold.unwrap_or(30_000)
}

pub fn retry_reset_duration(&self) -> i64 {
    // Default to 10 minutes - reset retry count if no failures occur in this window
    self.retry_reset_duration.unwrap_or(10 * 60 * 1000)
}

3. Potential for Configuration Validation 💭

Severity: Low

The config system doesn't validate that the values are positive or within reasonable bounds. While this may be intentional for flexibility, extremely low values (e.g., 0 or 1ms) could cause issues.

Recommendation: Consider adding validation in Root::validate_and_set_defaults() to ensure values are within reasonable ranges, similar to how datacenter validation is done (packages/config/src/config/mod.rs:194-205).

4. Missing Tests ⚠️

Severity: Medium

There are no tests covering the new configuration functionality. Given that these are critical timeout values affecting actor lifecycle, tests would be valuable.

Recommendation: Add tests to verify:

• Default values are applied correctly
• Custom config values override defaults
• The values are properly propagated to the workflow context

Code Quality Assessment

• Style Compliance: ✅ Follows Rust formatting with hard tabs as specified in CLAUDE.md
• Error Handling: ✅ Not applicable (pure configuration)
• Documentation: ✅ Good inline documentation
• Architecture: ✅ Follows existing config patterns correctly

Security Considerations

No security concerns identified. These are internal configuration parameters that don't expose attack surfaces.

Performance Considerations

• Configuration access is efficient (using LazyLock for defaults)
• No performance regressions expected
• The ability to tune these parameters could help optimize actor scheduling performance in production

Overall Assessment

This is a solid PR that improves operational flexibility. The main concerns are minor inconsistencies and the lack of test coverage. The code follows repository conventions and maintains backwards compatibility.

Recommendation: Approve with suggestions for follow-up improvements (type consistency and test coverage).

---

Review generated using Claude Code following CLAUDE.md guidelines

[claude]

Pull Request Review

This PR adds configurable timeouts and retry parameters for the Pegboard actor system, moving hardcoded constants into the configuration layer. Overall, the implementation is solid and follows the repository conventions well.

✅ Strengths

1. Good separation of concerns: Creating a dedicated pegboard.rs config module follows the existing pattern used by other config modules like logs.rs, cache.rs, etc.

2. Backward compatibility: Using Option types with .unwrap_or() accessor methods preserves existing default values, ensuring no breaking changes.

3. Consistent integration: The config is properly integrated into the Root struct following the pattern of other config sections (using LazyLock for defaults).

4. Clear documentation: Each config field has helpful doc comments explaining units and marking them as experimental.

🔍 Issues & Suggestions

1. Type Inconsistency ⚠️

Location: engine/packages/config/src/config/pegboard.rs:12,18,24,31

There's an inconsistency in types:

• base_retry_timeout uses Option<usize>
• The other three fields use Option<i64>

Issue: All of these represent millisecond durations, so they should use the same type for consistency. Since i64 is used for timestamps throughout the codebase (per CLAUDE.md conventions), and the other timeout values are i64, consider changing base_retry_timeout to i64 as well.

Current:

pub base_retry_timeout: Option<usize>,

Suggested:

pub base_retry_timeout: Option<i64>,

This would require updating:

• The return type in base_retry_timeout() method (line 35)
• The reschedule_backoff function signature in runtime.rs:818 to accept i64 instead of usize

2. Magic Numbers in Default Values 💭

Location: engine/packages/config/src/config/pegboard.rs:36,40,44,48

The default values use raw millisecond numbers:

self.base_retry_timeout.unwrap_or(2000)
self.actor_start_threshold.unwrap_or(30_000)
self.retry_reset_duration.unwrap_or(10 * 60 * 1000)

Observation: While retry_reset_duration shows the calculation (10 * 60 * 1000), others use raw numbers. For better readability and consistency with the codebase patterns, consider using constants or the util::duration helpers that exist in the codebase.

Note: This is a minor style issue. The old constants used raw values too, so this maintains consistency with the previous implementation.

3. Missing Test Coverage ⚠️

Impact: Medium

There are no tests validating:

• Config parsing from TOML/environment variables
• Default value behavior when config is absent
• That the new configurable values are actually used in the workflow logic

Suggestion: Add integration or unit tests to verify:

#[test]
fn test_pegboard_config_defaults() {
    let config = Pegboard::default();
    assert_eq!(config.base_retry_timeout(), 2000);
    assert_eq!(config.actor_start_threshold(), 30_000);
    // ... etc
}

🔒 Security Considerations

✅ No security concerns identified. The configuration values control timing behavior and don't introduce security vulnerabilities. The deny_unknown_fields serde attribute is correctly applied to prevent config injection attacks.

⚡ Performance Considerations

✅ Performance impact is minimal and positive:

• Uses LazyLock for default config instances (good pattern)
• Config access goes through ctx.config().pegboard() - acceptable overhead
• Moving from constants to runtime config lookup has negligible performance impact for these infrequent operations (actor lifecycle events)

📋 Code Quality

• Follows CLAUDE.md conventions: ✅
  • Uses hard tabs for indentation
  • Imports at file top
  • Follows existing config module patterns
• Follows Rust best practices: ✅
  • Proper use of Option types
  • Good use of serde attributes
  • Clean separation of concerns

🎯 Recommendations

Must fix before merge:

1. Consider standardizing types - either all i64 or all usize for duration fields

Should fix:
2. Add test coverage for the new configuration

Nice to have:
3. Consider using named constants or util::duration helpers for default values to improve readability

Overall Assessment

This is a well-implemented PR that successfully makes the Pegboard actor system more configurable. The main concern is the type inconsistency for base_retry_timeout, which should be addressed. Adding test coverage would significantly improve confidence in the changes.

Recommendation: Approve with minor revisions (address type consistency).

[claude]

Code Review

Summary

This PR converts hardcoded Pegboard actor lifecycle timeouts and retry configurations into configurable parameters. The changes look clean and follow good practices overall.

✅ Strengths

1. Good Configuration Design: The new Pegboard config struct follows the existing patterns in the codebase with:

   • Proper Option<T> wrapping for all fields
   • Accessor methods with sensible defaults matching the original constants
   • Clear documentation with units specified
   • Proper use of LazyLock in Root::pegboard()

2. Backwards Compatibility: Default values match the original constants exactly:

   • BASE_RETRY_TIMEOUT_MS: 2000 → base_retry_timeout.unwrap_or(2000)
   • ACTOR_START_THRESHOLD_MS: 30_000 → actor_start_threshold.unwrap_or(30_000)
   • ACTOR_STOP_THRESHOLD_MS: 30_000 → actor_stop_threshold.unwrap_or(30_000)
   • RETRY_RESET_DURATION_MS: 10 * 60 * 1000 → retry_reset_duration.unwrap_or(10 * 60 * 1000)

3. Clean Refactoring: Removed constants and properly threaded the configuration through the workflow context

4. Type Consistency: Maintains the original types (usize for base_retry_timeout, i64 for the others)

🔍 Minor Observations

1. Documentation: The **Experimental** markers are good, but consider adding information about:

   • Valid ranges (e.g., minimum/maximum reasonable values)
   • Impact on actor lifecycle behavior
   • When users might want to tune these values

2. Type Mismatch Consideration: There's a type inconsistency between base_retry_timeout (usize) and the other threshold values (i64). While this matches the original implementation, it might be worth considering if they should all be the same type for consistency. However, this is likely correct as:

   • base_retry_timeout is used in Backoff::new_at() which expects usize
   • The threshold values are compared with timestamps which are i64

3. No Breaking Changes: Since these are all Option fields with working defaults, this is a purely additive change with no breaking effects.

🎯 Suggestions (Optional)

Consider adding validation in Root::validate_and_set_defaults() to ensure:

• Timeout values are positive
• Thresholds are reasonable (e.g., not too small to cause false positives)

Example:

// In Root::validate_and_set_defaults()
let pegboard = self.pegboard();
if pegboard.actor_start_threshold() < 1000 {
    bail!("actor_start_threshold must be at least 1000ms");
}

However, since these are marked Experimental, validation might not be necessary yet.

✅ Verdict

LGTM - This is a well-executed refactoring that:

• Follows existing codebase patterns
• Maintains backwards compatibility
• Has clear documentation
• Makes the system more configurable without introducing risks

The code is production-ready as-is. The optional suggestions above are enhancements that could be added in a follow-up if desired.

[graphite-app]

Merge activity

• Nov 5, 5:07 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 5, 5:08 AM UTC: CI is running for this pull request on a draft pull request (#3371) due to your merge queue CI optimization settings.
• Nov 5, 5:11 AM UTC: Merged by the Graphite merge queue via draft PR: #3371.