fix(pb): optimize actor key allocation

[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
• fix(pb): backport serverless fix #4611
• fix(pb): optimize actor key allocation #4610 👈 (View in Graphite)
• 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(pb): optimize actor key allocation

NOTE: This PR is already merged. Feedback is provided for future reference.

SUMMARY

This PR eliminates two separate workflow activities (GenerateReservationId and ResolveTargetReplicas) by folding their work into a single LookupKeyOptimistic activity. The optimization is sound -- the target replicas are needed to scope the optimistic read anyway, so fetching them there (and generating the reservation ID in the same pass) removes two unnecessary round trips. The supporting refactor moving resolve_quorum_members to a shared util is clean.

---

POSITIVES

Utility extraction (utils.rs): resolve_active_quorum_members is the right place for this logic. Both epoxy_propose and epoxy_kv_get_optimistic now share the same validated quorum resolution path, and the rename from resolve_quorum_members to resolve_active_quorum_members is more descriptive.

Test migration: All 6 unit tests were correctly moved and renamed alongside the function. No coverage was lost.

Documentation comment on target_replicas: The doc comment clearly explains the caller-side scope stability invariant, which is important for a subtle correctness property.

Enum output design: LookupKeyOptimisticOutput with serde rename_all snake_case is idiomatic and the serialized form is stable.

---

CONCERNS

In-flight workflow compatibility: GenerateReservationId and ResolveTargetReplicas are removed entirely without a version guard. Workflows that already completed LookupKeyOptimistic (returning Option) but have not yet replayed through GenerateReservationId or ResolveTargetReplicas will fail on replay after deployment. The old ResolveTargetReplicas was added behind .v(2), so there is prior art for using version gates here. Worth confirming this codepath had no in-flight workflows at deploy time, or that the workflow engine handles missing activity types gracefully.

Minor - observability loss: The removed ResolveTargetReplicas activity included timing instrumentation (tracing::debug! with op_duration_ms). The inlined replica fetch in lookup_key_optimistic has no equivalent timing. If this op is slow in production it will be invisible in traces.

Minor - into_iter().next() vs .first().copied(): The forwarding branch now uses target_replicas.into_iter().next() where the old code used target_replicas.first().copied(). Both are correct here (the Vec is consumed before the early return), but .first().copied() reads more clearly as "get the first element" without the ownership implication. Not a bug, just a style note.

---

OVERALL

Clean optimization. The two activity round trips removed represent real distributed latency savings for every key reservation. The in-flight compatibility question is the only thing worth double-checking on a future similar change.

[NathanFlurry]

outdated

[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:36 AM UTC: Graphite rebased this pull request as part of a merge.
• Apr 13, 12:36 AM UTC: @MasterPtato merged this pull request with Graphite.