fix(envoy): require runner config for ws conn

[MasterPtato]

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

[MasterPtato]

• fix(epoxy): use vbare #4616
• fix: dont upsert for ensure if missing #4615
• fix(envoy): require runner config for ws conn #4612 👈 (View in Graphite)
• fix(pb): backport serverless fix #4611
• fix(pb): optimize actor key allocation #4610
• fix: move runner configs to epoxy #4605
• chore(epoxy): clean up kv get #4604
• fix(pb-envoy): reduce round trips for ws connection #4603
• fix(envoy-client): fix handling actor event ack #4599
• fix(envoy-client): add tls #4595 : 1 other dependent PR (#4597 )
• chore: fix driver test suite #4591
• chore: tunnel auth #4588
• chore: remove udb as a dependency of envoy-client #4587
• chore: revert actor v2 hack #4586
• fix: fix pool metadata protocol version refresh, rivetkit native ws #4585
• feat: US-001 - Define SqliteKv trait in rivetkit-sqlite-native #4584
• feat(rust/engine-runner): minimal rust engine runner #4289
• chore: delete kv channel #4583
• feat(engine): rust runner #4582
• chore: remove non-engine drivers #4574 : 1 other dependent PR (#4581 )
• feat: dynamic actors #4397
• fix: driver test suite & sqlite reliability #4559
• fix: remove serverless token #4572
• fix: misc token fixes #4569
• fix(pb): clean up actor stop decision handling #4566
• fix(envoy): use global instance, add signal handlers #4565
• chore: misc fixes, add pb snapshot test #4561
• fix(cache): make in memory cache global #4560
• main

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

[claude]

PR Review: fix(envoy): require runner config for ws conn

Summary

This PR tightens the envoy WebSocket connection contract by requiring a runner config to already exist before an envoy can connect, rather than auto-creating a default "normal" config on demand. The change is clean and improves explicitness, but there are a few items worth discussing.

---

Issues

Cache race window (bypass_cache: false) — The runner config is now fetched with bypass_cache: false, which uses the 5-second TTL cache. If a runner config is provisioned and then an envoy connects within that TTL window, it will receive a spurious NoRunnerConfig error because the cache may still hold a None result. The old ensure_normal_if_missing code was an upsert and was not sensitive to this. Consider whether bypass_cache: true is more appropriate here since this is a once-per-connection gating check on the critical path.

Missing trailing newline — engine/artifacts/errors/ws.no_runner_config.json has no trailing newline. POSIX-compliant files should end with a newline, and this may trigger lint warnings or artifact tooling issues.

---

Observations

ensure_normal_if_missing op may now be dead code — This operation was removed from conn.rs but is still present in the codebase. If it has no remaining callers, it should be cleaned up to avoid confusion.

Breaking change for self-hosted / test environments — Any deployment that relied on auto-creation of a normal runner config on envoy connect will now break with a NoRunnerConfig error. It may be worth calling this out explicitly in the PR description or any migration notes for self-hosters.

Error message format is consistent — Using {pool_name:?} (debug format, quoted output) matches the pattern used by other variants in WsError.

---

Code Quality

The refactor is clean. Moving the runner config fetch before tokio::try_join! and computing serverless_drain_grace_period directly from the result is clearer than the prior approach of using .first().and_then(...) with an implicit silent-None fallback. The new NoRunnerConfig error variant is well-structured, uses the RivetError derive macro correctly, and includes actionable metadata (pool_name). The artifact JSON is consistent with other error definitions in the registry.

[claude]

PR Review: fix(envoy): require runner config for ws conn

Summary

This PR changes init_conn in the envoy WebSocket connection path to fail explicitly with NoRunnerConfig when no runner config exists for the requested pool, instead of silently auto-creating a default "normal" config via ensure_normal_if_missing. It also fixes a pre-existing bug where is_serverless was always hardcoded to false on the Conn struct.

Correctness: is_serverless Bug Fix

The fix to is_serverless is correct. Previously the field was hardcoded to false; it is now correctly derived from whether serverless_drain_grace_period is Some. This is a real bug fix.

Concerns

1. Cache race window (bypass_cache: false)

The new gate check uses bypass_cache: false, meaning the 5-second TTL cache applies. If a runner config is created and an envoy connects within that window, the cache may return a stale miss, causing a spurious NoRunnerConfig error. The old ensure_normal_if_missing was an upsert and was cache-insensitive. The strict validation path should arguably use bypass_cache: true, or at minimum include a comment acknowledging this tradeoff.

2. Missing trailing newline in artifact file

engine/artifacts/errors/ws.no_runner_config.json appears to be missing a trailing newline, unlike sibling artifact files (e.g., ws.eviction.json). Minor POSIX hygiene issue.

3. {pool_name:?} debug format in error message

The error message uses {pool_name:?} (debug/quoted format), which emits the pool name surrounded by quotes in output (e.g., "my-pool"). Other parameterized WsError variants use display format ({0}). Confirm this is intentional.

Non-issues

• ensure_normal_if_missing is not dead code after this change. It still has a caller in pegboard/src/workflows/runner2.rs via the EnsureRunnerConfig activity.
• The behavioral change (envoy connections now fail without a pre-existing runner config) is the intended design.

Overall the change is clean and focused. The is_serverless fix is a genuine correctness improvement. The cache race window is the most substantive concern worth addressing before merging.

[MasterPtato]

Merge activity

• Apr 13, 12:21 AM UTC: A user started a stack merge that includes this pull request via Graphite.
• Apr 13, 12:40 AM UTC: Graphite rebased this pull request as part of a merge.
• Apr 13, 12:40 AM UTC: @MasterPtato merged this pull request with Graphite.