Skip to content

fix(api-peer): refresh runner config list reads#4807

Closed
NathanFlurry wants to merge 1 commit intoengine-stabilize/gasoline-skip-bad-worker-indexfrom
engine-stabilize/runner-config-fresh-peer-list
Closed

fix(api-peer): refresh runner config list reads#4807
NathanFlurry wants to merge 1 commit intoengine-stabilize/gasoline-skip-bad-worker-indexfrom
engine-stabilize/runner-config-fresh-peer-list

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Copy Markdown
Member Author

NathanFlurry commented Apr 27, 2026

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

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

@claude
Copy link
Copy Markdown

claude Bot commented Apr 27, 2026

PR Review: fix(api-peer): refresh runner config list reads

Summary

This is a minimal one-line fix in engine/packages/api-peer/src/runner_configs.rs. The list endpoint for runner configs previously passed bypass_cache: false to pegboard::ops::runner_config::get::Input, meaning reads went through a 5-second TTL cache. The fix changes this to bypass_cache: true, ensuring the list endpoint always reads directly from the store (UniversalDB) rather than serving potentially stale cached data.


What the Change Does

The runner_configs::list handler has two code paths:

  1. Named lookup (when runner_name/runner_names query params are provided) — calls pegboard::ops::runner_config::get. This was previously cached with a 5-second TTL; now it bypasses the cache.
  2. Paginated list (no specific names provided) — calls pegboard::ops::runner_config::list, which does not use the cache at all. This path is unaffected by the PR.

The fix targets path (1) only.


Correctness

The fix is correct and matches the established pattern in the codebase. All other callers that need fresh reads already use bypass_cache: true:

  • pegboard/src/ops/runner_config/upsert.rs — fresh read before write.
  • pegboard/src/workflows/runner_pool_metadata_poller.rs — reads fresh config to drive metadata polling.
  • api-public/src/runner_configs/refresh_metadata.rs — reads fresh config to drive metadata refresh.

The only non-critical callers that tolerate stale data (and correctly use bypass_cache: false) are internal workflow scheduling reads in runner_pool.rs and actor/runtime.rs, where a brief stale view is acceptable for scheduling decisions.

The API list endpoint is a user-facing read operation where freshness is expected. Returning stale data (up to 5 seconds old) after a recent upsert would be a correctness issue, particularly for tools or UIs that poll the list immediately after a configuration change.


Code Quality

No concerns. The change is minimal, targeted, and consistent with surrounding codebase conventions.


Potential Bugs / Gaps

The fix only applies to the named-lookup path. The paginated list path calls pegboard::ops::runner_config::list, which reads directly from UniversalDB without a cache layer and was already always-fresh. There is no inconsistency introduced — both paths now return fresh data.


Performance Considerations

Bypassing the cache increases load on UniversalDB for the named-lookup path. This is the correct tradeoff for a user-facing API endpoint where stale data is worse than the marginal extra latency. The bypass_cache: false path with a 5-second TTL makes sense for high-frequency internal scheduling calls (like runner_pool.rs), not for infrequent API list reads triggered by human operators or CI pipelines.


Security

No security concerns. The change does not affect authorization checks, trust boundaries, or data exposure. It only affects caching behavior of an already-authorized read.


Test Coverage

The existing tests cover the cache-bypass semantics for refresh_metadata but do not directly cover the list endpoint's named-lookup path. A targeted test asserting that an immediate GET /runner-configs?runner_name=X after PUT /runner-configs/X returns the updated config without a 5-second wait would prevent regression. Minor gap, not a blocker.


Verdict

The fix is correct, minimal, and consistent with how bypass_cache: true is used everywhere else that needs authoritative reads. Ready to merge.

@NathanFlurry NathanFlurry force-pushed the engine-stabilize/gasoline-skip-bad-worker-index branch from 1fbd8c3 to f29750e Compare April 27, 2026 07:30
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/runner-config-fresh-peer-list branch from 6953d69 to 9c1aabc Compare April 27, 2026 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant