Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

Copy link
Contributor Author

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


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
Copy link

pkg-pr-new bot commented Jan 14, 2026

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3899

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3899

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3899

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3899

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3899

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3899

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3899

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@3899

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3899

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3899

commit: 868b254

@MasterPtato MasterPtato force-pushed the 01-14-fix_engine-runner_handle_shutdown_close_codes_correctly branch from 600206d to 55a38d5 Compare January 14, 2026 22:45
@MasterPtato MasterPtato force-pushed the 01-14-fix_pegboard_cache_none_runner_pool_errors_too branch 2 times, most recently from 19b864f to 166bf69 Compare January 14, 2026 22:47
@MasterPtato MasterPtato force-pushed the 01-14-fix_engine-runner_handle_shutdown_close_codes_correctly branch from 55a38d5 to 715b5e8 Compare January 14, 2026 22:47
@claude
Copy link

claude bot commented Jan 14, 2026

PR Review: fix(pegboard): cache none runner pool errors too

Summary

This PR fixes a caching issue where runner pool states with no active error were not being cached. The fix introduces a separate cache entry type that can represent both error and non-error states, ensuring all runner pool states are cached regardless of error presence.

Code Quality ✅

Positive Changes:

  • Clean separation of concerns with RunnerPoolErrorCacheEntry (internal) vs RunnerPoolErrorEntry (public API)
  • The filter_map pattern (lines 56-62) is idiomatic Rust for handling the Option
  • Maintains the public API contract by still returning only entries with errors

Style Compliance:

  • Follows project conventions with structured logging
  • Proper use of workspace dependencies
  • Consistent naming patterns

Logic Analysis ✅

Before: Only runner pools with active errors were added to the cache result, meaning pools with no errors were not cached at all.

After: All runner pool states are cached (with error: None for healthy pools), then filtered when returning to maintain API compatibility.

Key Benefit: This ensures the cache correctly represents "no error" states rather than having cache misses, which prevents unnecessary re-fetching of healthy runner pool states.

Potential Issues 🔍

1. Early Return Behavior (Lines 97-99)

This early return means runners without workflows will not get cached entries. Depending on the semantics:

  • If missing workflows should be treated as "no error" they should be included in results with error: None
  • If missing workflows are invalid states current behavior is correct

Question: Should runners without associated workflows be cached as "no error" states? Currently they result in cache misses.

2. Partial Results Handling (Lines 106-109)

The code silently skips workflows not in the lookup map. While unlikely to occur, this could lead to incomplete cache entries. Consider logging at debug level for observability.

3. State Parsing Failures (Lines 115-127)

When state parsing fails or returns None, the runner is skipped entirely rather than being cached as "no error". This maintains existing behavior but may cause cache thrashing for runners with corrupted state.

Suggestion: Consider whether parse failures should cache a default "no error" entry to prevent repeated parse attempts on every request.

Performance Considerations ✅

  • The filter_map operation is efficient and avoids unnecessary allocations
  • Cache TTL of 500ms is unchanged, appropriate for error states that can change quickly
  • No additional database queries or workflow lookups introduced

Security Considerations ✅

  • No security implications identified
  • Error data is already typed and validated
  • No new external inputs or data flows

Test Coverage ⚠️

Recommendation: Add test cases for:

  1. Runner pools with no active errors verify they are cached with error: None
  2. Multiple runners with mixed error/non-error states verify filtering works correctly
  3. Repeated requests for healthy pools verify cache hit behavior

Minor Suggestions

Line 116: The log message could be more specific to indicate the workflow exists but has no state.

Verdict: ✅ Approve with Minor Suggestions

This is a well-implemented fix that addresses a real caching gap. The core logic is sound and maintains API compatibility while fixing the underlying issue.

Action Items:

  1. Consider the early return behavior for missing workflows (lines 97-99)
  2. Add test coverage for the new caching behavior
  3. Optional: Add debug logging for skipped workflows (line 108)

Great work fixing this caching issue! The solution is clean and maintains backward compatibility.

@MasterPtato MasterPtato force-pushed the 01-14-fix_engine-runner_handle_shutdown_close_codes_correctly branch from 715b5e8 to d6cf484 Compare January 14, 2026 22:52
@MasterPtato MasterPtato force-pushed the 01-14-fix_pegboard_cache_none_runner_pool_errors_too branch 2 times, most recently from 7ae571e to 9d5adb3 Compare January 14, 2026 23:02
@MasterPtato MasterPtato force-pushed the 01-14-fix_engine-runner_handle_shutdown_close_codes_correctly branch from d6cf484 to 13e2511 Compare January 14, 2026 23:02
@MasterPtato MasterPtato force-pushed the 01-14-fix_engine-runner_handle_shutdown_close_codes_correctly branch from 13e2511 to ce0f27d Compare January 14, 2026 23:07
@MasterPtato MasterPtato force-pushed the 01-14-fix_pegboard_cache_none_runner_pool_errors_too branch from 9d5adb3 to 868b254 Compare January 14, 2026 23:07
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.

2 participants