chore(pegboard): only refresh serverless runner metadata when new or udpated endpoint

[NathanFlurry]

No description provided.

[vercel]

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

3 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Oct 18, 2025 8:19am
rivet-inspector	Ignored	Preview		Oct 18, 2025 8:19am
rivet-site	Ignored	Preview		Oct 18, 2025 8:19am

💡 Enable Vercel Agent with $100 free credit for automated AI reviews

[NathanFlurry]

• chore(pegboard): only refresh serverless runner metadata when new or udpated endpoint #3227 👈 (View in Graphite)
• 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.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/rivet-dev/engine/@rivetkit/engine-runner@3227

npm i https://pkg.pr.new/rivet-dev/engine/@rivetkit/engine-runner-protocol@3227

commit: d8a19e4

[claude]

Pull Request Review

Summary

This PR optimizes the serverless runner configuration upsert flow by only refreshing runner metadata when the endpoint configuration (URL or headers) has actually changed. This is a good performance optimization that avoids unnecessary external HTTP calls.

Code Quality & Best Practices ✅

Strengths:

• Clean separation of concerns - the change detection logic is well-isolated in the database operation layer
• Proper use of structured logging following the codebase conventions (lowercase messages with structured fields)
• Good use of pattern matching to detect changes in serverless configuration
• Return value propagation is clear and consistent through the call stack

Observations:

1. The logic correctly handles all change scenarios:

   • New config (no existing config) → triggers refresh
   • Config type change (e.g., Normal ↔ Serverless) → triggers refresh
   • Serverless URL/headers change → triggers refresh
   • No change → skips refresh

2. The comparison at packages/services/namespace/src/ops/runner_config/upsert.rs:48 is correct - it compares both URL and headers using structural equality

Potential Issues 🔍

1. HashMap comparison may be order-dependent (Low Priority)
At packages/services/namespace/src/ops/runner_config/upsert.rs:48:

endpoint_config_changed = old_url != new_url || old_headers != new_headers;

While Rust's HashMap implements PartialEq correctly (order-independent comparison), the headers could semantically be the same but trigger a false positive if:

• Keys have different casing (though HTTP header names are case-insensitive)
• Values have extra whitespace

This is likely fine since the validation in the same file (lines 92-113) already trims and validates headers, but consider if this could cause unnecessary metadata refreshes.

2. Edge case: Multiple datacenters with different configs
At packages/core/api-public/src/runner_configs/upsert.rs:88-116:
The code uses any_endpoint_config_changed across all datacenters. If datacenter A changes but datacenter B doesn't, the metadata refresh still triggers. This is probably the correct behavior (conservative approach), but worth noting.

3. Missing OpenAPI response schema update
The response type changed in packages/core/api-peer/src/runner_configs.rs:95-96 to include endpoint_config_changed, but this field is added to an internal peer API response that may not be visible to end users. However, verify that the OpenAPI schema generation picks this up correctly for the peer API documentation.

Performance Considerations ✅

Positive Impact:

• Significantly reduces unnecessary HTTP requests to serverless endpoints when configs are updated but URLs/headers remain the same
• The database read for comparison is minimal overhead (single key lookup)
• The change detection happens before the expensive metadata fetch

Efficiency:

• The comparison logic is O(1) for URL and O(n) for headers HashMap (where n is number of headers, max 16)
• No noticeable performance overhead added

Security Concerns ✅

No issues identified:

• The change maintains existing validation (URL parsing, header validation at lines 80-113)
• No new attack surface introduced
• Proper error handling maintained
• The metadata refresh still occurs when configuration actually changes

Test Coverage ⚠️

Missing:

• No automated tests found for this change
• Consider adding tests for:
  1. Metadata refresh is skipped when only non-endpoint config changes (e.g., min_runners, max_runners)
  2. Metadata refresh triggers when URL changes
  3. Metadata refresh triggers when headers change
  4. Metadata refresh triggers for new configs
  5. Metadata refresh triggers when switching from Normal to Serverless

Recommendation: Add integration tests to verify the optimization works as expected and doesn't skip refreshes when it shouldn't.

Additional Notes

Logging:
The logging at packages/core/api-public/src/runner_configs/upsert.rs:148,161 is excellent - it clearly indicates when metadata refresh is skipped vs performed.

Type Safety:
Good use of bool return type for signaling change detection. Consider if a more descriptive enum might be clearer in the future (e.g., ConfigChangeStatus), but the current approach is fine.

Recommendations

1. Add tests - Most important recommendation. This optimization changes behavior and should have tests
2. Consider header normalization - If you want to be extra defensive, normalize header names/values before comparison
3. Documentation - Consider adding a code comment in the upsert operation explaining why we track endpoint_config_changed specifically (vs all config changes)

Verdict: ✅ Approve with suggestions

This is a solid performance optimization with clean implementation. The main gap is test coverage. The code quality follows repository conventions well, and the logic is sound. I'd recommend adding tests before merging, but the core implementation looks good.

---

Review generated by Claude Code

[claude]

PR Review: Only refresh serverless runner metadata when endpoint changes

Overview

This PR optimizes the runner config upsert operation by tracking whether the serverless endpoint configuration has changed, and only refreshing metadata from the remote endpoint when necessary. This is a good performance optimization that reduces unnecessary network calls.

Code Quality ✅

Strengths:

• Clean implementation with clear change tracking logic
• Proper propagation of the endpoint_config_changed flag through all layers (database → API peer → API public)
• Good use of structured logging with appropriate log levels
• Follows repository conventions (lowercase log messages, structured parameters)

Minor observations:

1. The comparison logic in packages/services/namespace/src/ops/runner_config/upsert.rs:36-53 only checks url and headers for serverless configs, which is correct since these are the fields that affect metadata fetching
2. The wildcard pattern .. is used appropriately to ignore other serverless fields that don't affect the metadata endpoint

Logic & Correctness ✅

Change detection logic (packages/services/namespace/src/ops/runner_config/upsert.rs:24-57):

• ✅ Correctly returns true for new configs
• ✅ Correctly compares url and headers for existing serverless configs
• ✅ Correctly returns true when config type changes (e.g., Normal ↔ Serverless)
• ✅ Handles non-serverless configs properly by returning true (conservative approach)

Multi-datacenter handling (packages/core/api-public/src/runner_configs/upsert.rs:88-116):

• ✅ Correctly tracks if ANY datacenter reports a change via any_endpoint_config_changed
• ✅ Uses OR logic to aggregate changes across datacenters, which is the right approach

Potential Issues ⚠️

1. HashMap comparison for headers (MINOR)

At packages/services/namespace/src/ops/runner_config/upsert.rs:48:

old_headers != new_headers

Since headers is a HashMap<String, String>, this comparison should work correctly. However, be aware that:

• HashMap equality checks both keys and values
• Order doesn't matter (which is correct for headers)
• This is the desired behavior ✅

2. Missing change detection for Normal → Serverless transitions

When transitioning from Normal to Serverless config, the logic at line 49-52 returns true, which will trigger metadata refresh. However, consider the reverse case:

• Serverless → Normal: Returns true ✅ (but metadata refresh is skipped anyway since serverless_config will be None)
• Normal → Serverless: Returns true ✅ (metadata refresh will occur)

This is handled correctly, but it's worth noting that the metadata refresh is only attempted when serverless_config is Some at packages/core/api-public/src/runner_configs/upsert.rs:146, so the flag being true for non-serverless cases is harmless.

3. Namespace resolution moved earlier (GOOD CHANGE)

The namespace resolution was moved from after the datacenter loop to before at packages/core/api-public/src/runner_configs/upsert.rs:63-68. This is actually an improvement because:

• ✅ Fails fast if namespace doesn't exist
• ✅ Avoids unnecessary work in other datacenters
• ✅ The namespace_id is needed for metadata refresh anyway

Performance Considerations ✅

Positive impacts:

• ✅ Eliminates unnecessary HTTP requests to serverless endpoints when config hasn't changed
• ✅ Reduces load on serverless runners during config updates that don't affect endpoints
• ✅ The additional comparison logic is O(n) where n = number of headers, which is capped at 16 (see validation at line 85)

No negative impacts identified

Security Concerns ✅

No security issues identified. The change:

• Doesn't modify authentication or authorization logic
• Doesn't expose sensitive data
• Uses the same validation as before
• Only affects when metadata is refreshed, not how

Test Coverage 📋

Missing tests:
The PR doesn't include tests for the new change detection logic. Consider adding tests for:

1. Scenario: No change - Same URL and headers → endpoint_config_changed = false
2. Scenario: URL changed - Different URL → endpoint_config_changed = true
3. Scenario: Headers changed - Different headers → endpoint_config_changed = true
4. Scenario: Headers added/removed - Header count changes → endpoint_config_changed = true
5. Scenario: New config - No existing config → endpoint_config_changed = true
6. Scenario: Type change - Normal ↔ Serverless → endpoint_config_changed = true

Suggestion: Add unit tests for the change detection logic in the operation to ensure correctness.

Documentation 📝

Logging is well done:

• Line 148: tracing::debug!("endpoint config changed, refreshing metadata");
• Line 161: tracing::debug!("endpoint config unchanged, skipping metadata refresh");

These log messages make it easy to understand the optimization in action.

OpenAPI Schema ✅

The response schema was correctly updated to include the new field:

"endpoint_config_changed": {
  "type": "boolean"
}

Minor Suggestions

1. Consider renaming for clarity: The field name endpoint_config_changed is good, but you might want to document that it specifically refers to changes that affect metadata fetching (URL and headers only, not other serverless config fields).

2. Code comment opportunity at packages/services/namespace/src/ops/runner_config/upsert.rs:35:

// Check if serverless endpoint config changed
// Only URL and headers affect metadata fetching, so we ignore other fields
match (&existing_config.kind, &input.config.kind) {

Summary

This is a solid optimization with clean implementation. The logic is correct, performance impact is positive, and there are no security concerns. The main suggestion is to add test coverage for the change detection logic to prevent regressions.

Recommendation: ✅ Approve with minor suggestion to add tests.

---

🤖 Generated with Claude Code

[claude]

PR Review: Optimize serverless runner metadata refresh

Summary

This PR adds an optimization to only refresh serverless runner metadata when the endpoint configuration (URL or headers) has actually changed, avoiding unnecessary metadata refresh calls on every upsert operation.

Code Quality & Best Practices ✅

Strengths:

• Clean implementation following the existing codebase patterns
• Proper use of structured logging with tracing::debug!() for observability
• Follows the project's error handling conventions (returning Result<bool>)
• Good separation of concerns - detection logic lives in the database operation layer

Adherence to CLAUDE.md conventions:

• ✅ Structured logging: tracing::debug!("endpoint config changed, refreshing metadata")
• ✅ Lowercase log messages
• ✅ Returns Result<bool> following standard patterns
• ✅ Proper use of workspace dependencies

Logic Analysis

The change detection logic (lines 35-53 in namespace/src/ops/runner_config/upsert.rs):

The implementation correctly identifies when endpoint config has changed:

1. New config: Returns true (line 55-56)
2. Serverless → Serverless: Compares URL and headers (line 48)
3. Any other change: Returns true (line 49-52)

Minor Issue - Incomplete change detection:

The current logic only compares url and headers for serverless configs, but RunnerConfigKind::Serverless contains additional fields that might affect metadata:

• request_lifespan
• slots_per_runner
• min_runners
• max_runners
• runners_margin

Question: Should changes to these fields also trigger a metadata refresh? Looking at the code, these appear to be scaling/configuration parameters that might not affect the metadata endpoint response. However, if the metadata endpoint uses any of these values as context, they should be included in the comparison.

Recommendation: Add a comment explaining why only url and headers are compared, or if uncertain, consider comparing the entire Serverless variant.

Potential Issues

1. Multi-datacenter consistency 🟡

In api-public/src/runner_configs/upsert.rs:89-114, the code collects endpoint_config_changed from multiple datacenters using an OR operation:

if response.endpoint_config_changed {
    any_endpoint_config_changed = true;
}

Scenario: If configs differ across datacenters initially and only one datacenter changes, this could lead to:

• Datacenter A: config unchanged → returns false
• Datacenter B: config changed → returns true
• Result: Metadata refresh happens (correct behavior)

However, there's a subtle edge case: The serverless config is extracted before the datacenter loop (lines 70-86). If different datacenters have different serverless configs, only the first one found will be used for metadata refresh. This might be intentional (perhaps serverless configs must be identical across DCs), but worth documenting.

2. Namespace resolution timing ✅

Good catch moving namespace resolution earlier (lines 62-68)! This ensures the namespace exists before attempting any upserts, providing better error handling.

Performance Considerations ✅

Positive impact:

• Reduces unnecessary HTTP calls to serverless metadata endpoints
• Database comparison is cheap (string equality checks)
• No additional database reads (reuses existing read from upsert operation)

No performance concerns identified.

Security Considerations ✅

No security issues identified:

• No new authentication/authorization logic
• No data exposure changes
• Metadata refresh still validates responses properly

Test Coverage 🟡

Missing test coverage:

This change would benefit from tests covering:

1. Metadata refresh is skipped when only non-endpoint fields change (e.g., min_runners)
2. Metadata refresh occurs when URL changes
3. Metadata refresh occurs when headers change
4. Metadata refresh occurs for new configs
5. Multi-datacenter behavior with mixed change states

Recommendation: Add integration tests to verify the optimization works as expected.

Minor Code Quality Suggestions

1. Type safety for change detection

Consider extracting the endpoint comparison into a helper method:

impl RunnerConfigKind {
    fn endpoint_config_eq(&self, other: &Self) -> bool {
        match (self, other) {
            (
                RunnerConfigKind::Serverless { url: url1, headers: headers1, .. },
                RunnerConfigKind::Serverless { url: url2, headers: headers2, .. },
            ) => url1 == url2 && headers1 == headers2,
            _ => false,
        }
    }
}

This would make the intent clearer and easier to test.

2. Log message consistency

Line 74: Changed dc_name to _dc_name to indicate it's unused. Consider removing it entirely if not needed:

.filter_map(|(_dc_name, runner_config)| {
// Could be:
.filter_map(|(_, runner_config)| {

OpenAPI Schema ✅

The OpenAPI change correctly adds the new response field with proper typing:

• endpoint_config_changed: boolean (required)

This is a breaking change to the API contract, but appears intentional.

Overall Assessment

Strong PR with one clarification needed:

✅ Approve with minor suggestions

The optimization is well-implemented and will provide tangible performance benefits by avoiding unnecessary metadata refresh calls. The logic is sound, though I'd recommend:

1. Clarify whether non-endpoint serverless fields should trigger metadata refresh
2. Add test coverage for the change detection logic
3. Consider documenting the multi-datacenter serverless config behavior

Great work on this optimization!

---

Review generated with attention to the codebase conventions in CLAUDE.md