Conversation
|
🚅 Deployed to the rivet-pr-4463 environment in rivet-frontend
|
PR Review: feat(pb): actors v3This is a large, ambitious draft PR introducing actor v3 infrastructure. Since it's a draft, I'm flagging issues to address before it's ready for merge. Critical Issues (must fix before merge)1. Package name mismatch in 2. Literal
These will fail to compile and need to be resolved before this can be tested. 3. Undefined type 4. Missing 5. Wrong struct name in 6. Mismatched input type: Dependency Issues7. Code Quality8. Commented-out dead code 9. Metrics label change is a breaking change 10. Duplicated Design / Architecture Questions11. Naming: envoy vs runner 12. Epoxy key reservation failure modes 13. Unresolved TODO comments in production paths
These should be resolved or tracked as follow-up issues before merge. Minor
Overall this is a substantial architectural improvement with sleep/wake support, the envoy protocol rename, and Epoxy-based key reservation. The main blockers are compile errors from placeholder TODO tokens and naming inconsistencies. Happy to re-review once those are addressed. |
Design / Logic Issues5. Double-counted eviction metrics 6. Silent JSON deserialization errors in conn.rs 7. Wrong field mapping in Mk1 KV metadata 8. Unverified abort safety in demuxer GC |
Code Quality9. Significant KV code duplication 10. Unimplemented module 11. TODOs indicating known gaps Minor / Style12. Workflow rename is a breaking change |
Actors V3 ReviewThis is a substantial draft PR (~12k lines) introducing an envoy architecture for Actor V2 alongside the existing V1 system. Since it's a draft, some issues below are likely already on your radar, but I've flagged everything worth tracking before merge. Summary of architectural changes
Issues to address before mergeUnresolved workspace dependency pinningBoth Silent JSON deserialization failure in `conn.rs`Malformed actor metadata silently becomes an empty map via Task abort safety unverified (`actor_event_demuxer.rs:82`)The TODO comment "Verify aborting is safe here" above KV ops block the WebSocket receive loop (`ws_to_tunnel_task.rs`)Every KV operation (get/put/list/delete) blocks the envoy receive loop for a full database round-trip, serializing all actor KV requests through a single connection. The TODO ("Add queue and bg thread for processing kv ops") should be tracked as a follow-up issue even if not blocking the initial rollout. Wake retry count changed for V1 (`pegboard_gateway.rs:390`)The V1 actor path now retries 8 times instead of 16. If this is intentional alignment with V2, a comment explaining the rationale would help; if accidental, it should be reverted. Separately, the added Minor feedback
What looks good
This is a draft, so the above TODOs may already be tracked. Happy to re-review once the draft is ready. |
b395fae to
ec32bdf
Compare
3f2996f to
9d9049a
Compare
ec32bdf to
ae0f886
Compare
9d9049a to
7da9651
Compare
7da9651 to
dc6bd10
Compare
ae0f886 to
4e8c22e
Compare
dc6bd10 to
8a1d9d6
Compare
4e8c22e to
e5b3f53
Compare
703e3b6 to
446bf80
Compare
949143d to
afb8d48
Compare
afb8d48 to
d52568e
Compare
08159ac to
5113d59
Compare
d52568e to
fb6f723
Compare
5113d59 to
b0cc12a
Compare
b0cc12a to
3d99705
Compare
67b71f3 to
fddb429
Compare
3d99705 to
3fbc6dc
Compare
fddb429 to
e4d920a
Compare
Merge activity
|
e4d920a to
509d767
Compare

Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: