feat: implement new actor ids

[MasterPtato]

Changes

[MasterPtato]

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

• fix: get pb manager test working e2e again #2721
• fix: convert runner protocol to protobuf #2702
• fix: even more actor api changes #2714
• chore: flatten actor api spec #2698
• fix: increase default actor kill timeout to 5s #2701
• fix: dont clear ingress ports on reschedule #2700
• fix: add cpu constraints to oci config #2697
• fix: api changes #2692
• fix: fix logs for new actors #2649
• feat: add actor queue #2644
• feat: add actor kv to runners #2638
• feat: get multi actors working e2e on docker compose #2637
• fix: add udp ports to docker compose #2635
• chore: fix merge conflicts #2634
• fix: get new actors working on docker compose #2623
• fix: figure out actor log shipping #2622
• fix(pegboard): convert ws manager socket to unix socket #2478
• fix(pb): get new actor ids working e2e #2471
• feat: implement new actor ids #2467 👈 (View in Graphite)
• feat(pb): get multi-actors working e2e, add resources for builds #2465
• feat(pb): add runner logs #2447
• feat: add runners to pb protocol #2433
• chore(pegboard): add exit code log from container runner #2477
• fix(workflows): fix branch version bug #2475
• chore(pegboard): replace computing image size manually with using tar bytes read #2625
• fix: make pb image downloads random, remove prewarm from pb protocol #2784
• fix: clean up worker threads in the event of an error #2783
• fix: make query_as macros retry #2773
• main

---

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.

[cloudflare-workers-and-pages]

Deploying rivet with    Cloudflare Pages

Latest commit:	a769700
Status:	🚫  Build failed.

View logs

[cloudflare-workers-and-pages]

Deploying rivet-studio with    Cloudflare Pages

Latest commit:	a769700
Status:	🚫  Build failed.

View logs

[cloudflare-workers-and-pages]

Deploying rivet-hub with    Cloudflare Pages

Latest commit:	a769700
Status:	🚫  Build failed.

View logs

[claude]

Claude finished @MasterPtato's task —— View job

---

🔍 PR Review: New Actor IDs Implementation

Reviewing this substantial change implementing new actor IDs across the entire system.

Review Checklist:

• Analyze core ID implementation and architecture
• Review database migrations and schema changes
• Examine API changes and backwards compatibility
• Check SDK consistency across languages
• Assess security implications
• Review performance considerations
• Evaluate test coverage
• Provide comprehensive feedback

📋 Review Summary

This PR introduces a sophisticated versioned ID system that balances backwards compatibility with performance optimization. The implementation is well-architected but has several areas for improvement.

✅ Strengths

Excellent Architecture Design

• Versioned System: Clean V0 (UUID) and V1 (UUID + label) design maintains backwards compatibility
• Performance Optimization: V1 labels enable efficient datacenter routing (see packages/core/api/actor/src/route/actors.rs:61-86)
• Comprehensive Integration: Proper SQLx, serde, and FoundationDB tuple packing support

Strong Implementation Quality

• Robust Error Handling: Comprehensive IdError enum with specific error types
• Base36 Encoding: Efficient encoding for V1 IDs (30 chars vs 38 for UUID+label)
• Type Safety: Strong typing throughout the system

⚠️ Areas for Improvement

Critical: Missing Input Validation

// packages/common/util/core/src/id.rs:204
let s = unsafe { String::from_utf8_unchecked(buf.to_vec()) };

Issue: Uses unsafe without validation that the base36 encoding produces valid UTF-8.
Risk: Potential memory safety issues if encoding logic has bugs.
Fix: Use String::from_utf8() and handle the error case.

Database Migration Concerns

-- packages/core/services/cluster/db/cluster/migrations/20250522174043_dc_label.up.sql
ALTER TABLE datacenters
    ADD COLUMN label BYTES AS (substring(datacenter_id::BYTES FROM 1 FOR 2)) STORED;

Issues:

• No rollback strategy for the computed column
• Missing validation that existing datacenter_ids are compatible
• Could fail if any existing IDs don't have the expected format

Performance & Resource Issues

Inefficient Base36 Decoding:

// packages/common/util/core/src/id.rs:317-333
fn decode_base36<const N: usize>(s: &str) -> Result<[u8; N], IdError> {
    // O(n²) algorithm with repeated multiplication
}

Impact: Expensive for V1 ID parsing. Consider pre-computed lookup tables.

Memory Allocation in Hot Path:

// packages/common/util/core/src/id.rs:90-101  
pub fn as_bytes(&self) -> Vec<u8> {
    // Always allocates new Vec
}

Impact: Frequent allocations for serialization. Consider returning Cow<[u8]>.

API Consistency Issues

Inconsistent Method Naming:

Id::new_v1(label)     // Constructor
Id::v1(uuid, label)   // Also constructor - confusing naming

Missing Error Context:

// packages/common/util/core/src/id.rs:257-258
let v = Id::from_bytes(slice)
    .map_err(|err| PackError::Message(format!("bad id format: {err}").into()))?;

Issue: Generic error message doesn't help with debugging.

🔒 Security Assessment

Low Risk Issues

• JWT claims still use UUID (packages/edge/infra/client/manager/src/claims.rs:63) - no security impact
• Base36 encoding is deterministic and doesn't leak information
• Label extraction is safe with proper bounds checking

Medium Risk

• The unsafe UTF-8 conversion mentioned above
• Database migrations could fail on inconsistent data

🚀 Performance Impact

Positive

• Datacenter Routing: Significant improvement by targeting specific DCs based on labels
• Compact Encoding: V1 IDs are more compact than separate UUID+label storage

Concerns

• Base36 encoding/decoding is computationally expensive
• Frequent Vec allocations in serialization paths
• Database computed columns add storage overhead

🧪 Test Coverage

Missing Critical Tests:

• Migration rollback scenarios
• Error cases for malformed V1 IDs
• Performance benchmarks for base36 encoding
• Cross-language SDK compatibility tests
• Edge cases (empty strings, invalid UTF-8, etc.)

Existing Tests: Basic roundtrip tests are present but insufficient for production readiness.

📋 Recommendations

High Priority

1. Fix unsafe UTF-8 conversion with proper error handling
2. Add migration rollback tests and validation
3. Optimize base36 encoding with lookup tables or streaming approach
4. Add comprehensive error context throughout the ID parsing pipeline

Medium Priority

1. Reduce allocations by using Cow<[u8]> for as_bytes()
2. Standardize API naming (new_v1 vs v1 constructors)
3. Add performance benchmarks for ID operations
4. Document migration strategy for existing data

Low Priority

1. Add utility methods like is_v1(), extract_uuid() for ergonomics
2. Consider caching for frequently accessed datacenter labels
3. Add metrics for ID format distribution

🎯 Overall Assessment

This is a well-architected change that introduces meaningful performance optimizations while maintaining backwards compatibility. However, the unsafe code and migration risks require attention before merging.

Recommendation: Approve with required changes - fix the critical safety issues and add proper test coverage, then this will be ready for production.

---

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Explain this complex logic.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[claude]

Claude finished @MasterPtato's task —— View job

---

🔍 PR Review: New Actor IDs Implementation

Reviewing this substantial change implementing new actor IDs across the entire system.

📋 Review Tasks

• Analyze core ID implementation and architecture
• Review database migrations and schema changes
• Examine API changes and backwards compatibility
• Check SDK consistency across languages
• Assess security implications
• Review performance considerations
• Evaluate test coverage
• Provide comprehensive feedback

📋 Review Summary

This PR introduces a sophisticated versioned ID system that balances backwards compatibility with performance optimization. The implementation is well-architected but has several areas for improvement.

✅ Strengths

Excellent Architecture Design

• Versioned System: Clean V0 (UUID) and V1 (UUID + label) design maintains backwards compatibility
• Performance Optimization: V1 labels enable efficient datacenter routing (see packages/core/api/actor/src/route/actors.rs:61-86)
• Comprehensive Integration: Proper SQLx, serde, and FoundationDB tuple packing support

Strong Implementation Quality

• Robust Error Handling: Comprehensive IdError enum with specific error types
• Base36 Encoding: Efficient encoding for V1 IDs (30 chars vs 38 for UUID+label)
• Type Safety: Strong typing throughout the system

⚠️ Areas for Improvement

Critical: Missing Input Validation

// packages/common/util/core/src/id.rs:204
let s = unsafe { String::from_utf8_unchecked(buf.to_vec()) };

Issue: Uses unsafe without validation that the base36 encoding produces valid UTF-8.
Risk: Potential memory safety issues if encoding logic has bugs.
Fix: Use String::from_utf8() and handle the error case.

Database Migration Concerns

-- packages/core/services/cluster/db/cluster/migrations/20250522174043_dc_label.up.sql
ALTER TABLE datacenters
    ADD COLUMN label BYTES AS (substring(datacenter_id::BYTES FROM 1 FOR 2)) STORED;

Issues:

• No rollback strategy for the computed column
• Missing validation that existing datacenter_ids are compatible
• Could fail if any existing IDs don't have the expected format

Performance & Resource Issues

Inefficient Base36 Decoding:

// packages/common/util/core/src/id.rs:317-333
fn decode_base36<const N: usize>(s: &str) -> Result<[u8; N], IdError> {
    // O(n²) algorithm with repeated multiplication
}

Impact: Expensive for V1 ID parsing. Consider pre-computed lookup tables.

Memory Allocation in Hot Path:

// packages/common/util/core/src/id.rs:90-101  
pub fn as_bytes(&self) -> Vec<u8> {
    // Always allocates new Vec
}

Impact: Frequent allocations for serialization. Consider returning Cow<[u8]>.

API Consistency Issues

Inconsistent Method Naming:

Id::new_v1(label)     // Constructor
Id::v1(uuid, label)   // Also constructor - confusing naming

Missing Error Context:

// packages/common/util/core/src/id.rs:257-258
let v = Id::from_bytes(slice)
    .map_err(|err| PackError::Message(format!("bad id format: {err}").into()))?;

Issue: Generic error message doesn't help with debugging.

🔒 Security Assessment

Low Risk Issues

• JWT claims still use UUID (packages/edge/infra/client/manager/src/claims.rs:63) - no security impact
• Base36 encoding is deterministic and doesn't leak information
• Label extraction is safe with proper bounds checking

Medium Risk

• The unsafe UTF-8 conversion mentioned above
• Database migrations could fail on inconsistent data

🚀 Performance Impact

Positive

• Datacenter Routing: Significant improvement by targeting specific DCs based on labels
• Compact Encoding: V1 IDs are more compact than separate UUID+label storage

Concerns

• Base36 encoding/decoding is computationally expensive
• Frequent Vec allocations in serialization paths
• Database computed columns add storage overhead

🧪 Test Coverage

Missing Critical Tests:

• Migration rollback scenarios
• Error cases for malformed V1 IDs
• Performance benchmarks for base36 encoding
• Cross-language SDK compatibility tests
• Edge cases (empty strings, invalid UTF-8, etc.)

Existing Tests: Basic roundtrip tests are present but insufficient for production readiness.

📋 Recommendations

High Priority

1. Fix unsafe UTF-8 conversion with proper error handling
2. Add migration rollback tests and validation
3. Optimize base36 encoding with lookup tables or streaming approach
4. Add comprehensive error context throughout the ID parsing pipeline

Medium Priority

1. Reduce allocations by using Cow<[u8]> for as_bytes()
2. Standardize API naming (new_v1 vs v1 constructors)
3. Add performance benchmarks for ID operations
4. Document migration strategy for existing data

Low Priority

1. Add utility methods like is_v1(), extract_uuid() for ergonomics
2. Consider caching for frequently accessed datacenter labels
3. Add metrics for ID format distribution

🎯 Overall Assessment

This is a well-architected change that introduces meaningful performance optimizations while maintaining backwards compatibility. However, the unsafe code and migration risks require attention before merging.

Recommendation: Approve with required changes - fix the critical safety issues and add proper test coverage, then this will be ready for production.

[claude]

Claude finished @MasterPtato's task —— View job

---

🔍 PR Review: New Actor IDs Implementation

I've completed a comprehensive review of this substantial PR implementing new actor IDs. This introduces a sophisticated versioned ID system that balances backwards compatibility with performance optimization.

📋 Review Tasks

• Read repository guidelines and setup instructions
• Analyze core ID implementation and architecture
• Review database migrations and schema changes
• Examine API changes and backwards compatibility
• Check SDK consistency across languages
• Assess security implications
• Review performance considerations
• Evaluate test coverage
• Provide comprehensive feedback

✅ Strengths

Excellent Architecture Design

• Versioned System: Clean V0 (UUID) and V1 (UUID + label) design maintains full backwards compatibility
• Performance Optimization: V1 labels enable efficient datacenter routing (packages/core/api/actor/src/route/actors.rs:61-86)
• Comprehensive Integration: Proper SQLx, serde, and FoundationDB tuple packing support throughout the system

Strong Implementation Quality

• Robust Error Handling: Comprehensive IdError enum with specific error types (packages/common/util/core/src/id.rs:7-31)
• Base36 Encoding: Efficient 30-character encoding for V1 IDs vs 38 chars for separate UUID+label
• Type Safety: Strong typing with proper trait implementations across all services

🚨 Critical Issues

1. Memory Safety Violation (HIGH PRIORITY)

// packages/common/util/core/src/id.rs:204
let s = unsafe { String::from_utf8_unchecked(buf.to_vec()) };

Issue: Uses unsafe without validation that base36 encoding produces valid UTF-8
Risk: Potential memory safety violation if encoding logic contains bugs
Fix: Replace with String::from_utf8(buf.to_vec()).map_err(|_| std::fmt::Error)?

2. Missing Migration Rollback Strategy (HIGH PRIORITY)

-- packages/core/services/cluster/db/cluster/migrations/20250522174043_dc_label.down.sql
-- File is completely empty

Issue: No rollback strategy for the computed column migration
Risk: Cannot safely rollback database changes in production
Fix: Add proper rollback SQL:

DROP INDEX IF EXISTS datacenter_label_idx;
ALTER TABLE datacenters DROP COLUMN IF EXISTS label;

3. Migration Data Validation Missing (HIGH PRIORITY)

-- packages/core/services/cluster/db/cluster/migrations/20250522174043_dc_label.up.sql:2
ADD COLUMN label BYTES AS (substring(datacenter_id::BYTES FROM 1 FOR 2)) STORED;

Issue: No validation that existing datacenter_ids are compatible with label extraction
Risk: Migration could fail on inconsistent existing data
Fix: Add pre-migration validation check

⚠️ Performance & Design Concerns

Inefficient Base36 Operations

// packages/common/util/core/src/id.rs:317-333
fn decode_base36<const N: usize>(s: &str) -> Result<[u8; N], IdError> {
    // O(n²) algorithm with repeated bigint operations
}

Impact: Expensive for frequent V1 ID parsing operations
Recommendation: Consider pre-computed lookup tables or streaming approach

Memory Allocation in Hot Path

// packages/common/util/core/src/id.rs:90-101
pub fn as_bytes(&self) -> Vec<u8> {
    // Always allocates new Vec
}

Impact: Frequent allocations during serialization
Recommendation: Return Cow<[u8]> to avoid unnecessary allocations for V0 IDs

API Consistency Issues

// Inconsistent constructor naming
Id::new_v1(label)     // Constructor taking only label
Id::v1(uuid, label)   // Constructor taking both - confusing naming

Recommendation: Standardize to new_v0(), new_v1(), from_v0(), from_v1()

🔒 Security Assessment

Low Risk

• JWT claims continue using UUIDs (packages/edge/infra/client/manager/src/claims.rs:63) - no security impact
• Base36 encoding is deterministic and doesn't leak information
• Label extraction has proper bounds checking

Medium Risk

• The unsafe UTF-8 conversion creates potential memory safety issues
• Database migrations could fail on inconsistent data, affecting availability

🧪 Test Coverage Analysis

Critical Missing Tests

• Core ID Implementation: Only basic roundtrip tests (packages/common/util/core/src/id.rs:354-373)
• Error Handling: No tests for IdError variants or malformed input
• Database Integration: No tests for SQLx/FDB serialization
• Migration Testing: No tests for datacenter label migration
• API Endpoint Validation: No tests for new ID format handling
• Cross-language SDK Compatibility: No validation tests

Test Coverage Recommendations

1. HIGH PRIORITY: Add comprehensive error handling tests
2. HIGH PRIORITY: Add database serialization/deserialization tests
3. MEDIUM PRIORITY: Add migration rollback tests
4. MEDIUM PRIORITY: Add API endpoint validation tests
5. LOW PRIORITY: Add performance benchmarks

🚀 Performance Impact Assessment

Positive Impact

• Datacenter Routing: Significant optimization by targeting specific datacenters based on labels
• Compact Encoding: V1 IDs more storage-efficient than separate UUID+label storage
• Index Optimization: New datacenter label index enables fast lookups (packages/core/services/cluster/src/ops/datacenter/get_for_label.rs:57)

Performance Concerns

• Base36 encoding/decoding is computationally expensive
• Frequent Vec allocations in serialization paths
• Database computed columns add storage overhead

📋 Recommendations

High Priority (Blocking)

1. Fix unsafe UTF-8 conversion with proper error handling
2. Add migration rollback SQL and comprehensive validation
3. Add extensive error handling tests for edge cases
4. Validate existing datacenter data before migration

Medium Priority

1. Optimize base36 performance with lookup tables or alternative algorithms
2. Reduce memory allocations using Cow<[u8]> for as_bytes()
3. Standardize API naming across constructors
4. Add comprehensive test coverage for all ID operations

Low Priority

1. Add utility methods like is_v1(), extract_uuid() for ergonomics
2. Consider caching for frequently accessed datacenter labels
3. Add observability metrics for ID format distribution

🎯 Overall Assessment

This is a well-architected change that provides meaningful performance benefits through datacenter-aware routing. The versioned ID system elegantly maintains backwards compatibility while enabling significant optimizations.

However, the memory safety violation and missing migration rollback are blocking issues that must be resolved before merging.

Recommendation: Request Changes - Address the critical safety and migration issues, add comprehensive test coverage, then this will be production-ready.

The core concept and implementation quality are excellent, but production safety requires fixing these critical issues first.

---

[claude]

Claude finished @MasterPtato's task —— View job

---

🔍 PR Review: New Actor IDs Implementation

I've completed a comprehensive review of this substantial PR implementing a new versioned ID system for actors across the rivet platform.

Review Tasks:

• Analyze core ID implementation and architecture
• Review database migrations and schema changes
• Examine API changes and backwards compatibility
• Check SDK consistency across languages
• Assess security implications
• Review performance considerations
• Evaluate test coverage
• Provide comprehensive feedback

---

📋 Review Summary

This PR introduces a sophisticated versioned ID system that balances backwards compatibility with performance optimization. The implementation is well-architected but has several critical issues that need attention.

✅ Strengths

Excellent Architecture Design

• Versioned System: Clean V0 (UUID) and V1 (UUID + label) design maintains backwards compatibility
• Performance Optimization: V1 labels enable efficient datacenter routing (see packages/core/api/actor/src/route/actors.rs:61-86)
• Comprehensive Integration: Proper SQLx, serde, and FoundationDB tuple packing support

Strong Implementation Quality

• Robust Error Handling: Comprehensive IdError enum with specific error types
• Base36 Encoding: Efficient encoding for V1 IDs (30 chars vs 38 for UUID+label)
• Type Safety: Strong typing throughout the system

🚨 Critical Issues

1. Memory Safety Violation

// packages/common/util/core/src/id.rs:204
let s = unsafe { String::from_utf8_unchecked(buf.to_vec()) };

Issue: Uses unsafe without validation that the base36 encoding produces valid UTF-8.
Risk: Potential memory safety issues if encoding logic has bugs.
Fix: Use String::from_utf8() and handle the error case.

2. Missing Migration Rollback

-- packages/core/services/cluster/db/cluster/migrations/20250522174043_dc_label.down.sql
-- File is completely empty

Issue: No rollback strategy for the computed column migration.
Risk: Cannot safely rollback database changes in production.
Fix: Add proper rollback SQL:

DROP INDEX IF EXISTS datacenter_label_idx;
ALTER TABLE datacenters DROP COLUMN IF EXISTS label;

3. Database Migration Validation

-- packages/core/services/cluster/db/cluster/migrations/20250522174043_dc_label.up.sql
ALTER TABLE datacenters
    ADD COLUMN label BYTES AS (substring(datacenter_id::BYTES FROM 1 FOR 2)) STORED;

Issue: No validation that existing datacenter_ids are compatible.
Risk: Migration could fail if existing IDs don't have the expected format.

⚠️ Performance & Design Concerns

Inefficient Base36 Operations

// packages/common/util/core/src/id.rs:317-333
fn decode_base36<const N: usize>(s: &str) -> Result<[u8; N], IdError> {
    // O(n²) algorithm with repeated multiplication
}

Impact: Expensive for V1 ID parsing. Consider pre-computed lookup tables.

Memory Allocation in Hot Path

// packages/common/util/core/src/id.rs:90-101  
pub fn as_bytes(&self) -> Vec<u8> {
    // Always allocates new Vec
}

Impact: Frequent allocations for serialization. Consider returning Cow<[u8]>.

API Consistency Issues

Id::new_v1(label)     // Constructor
Id::v1(uuid, label)   // Also constructor - confusing naming

Legacy API Compatibility

// packages/core/api/actor/src/route/actors.rs:1059
id: util::Id::parse(&a.id)?.as_v0().unwrap_or_else(Uuid::nil),

Issue: Returns Uuid::nil for V1 IDs in legacy API - could cause silent failures.

🔒 Security Assessment

Low Risk Issues

• JWT claims still use UUID (packages/edge/infra/client/manager/src/claims.rs:63) - no security impact
• Base36 encoding is deterministic and doesn't leak information
• Label extraction is safe with proper bounds checking

Medium Risk

• The unsafe UTF-8 conversion mentioned above
• Database migrations could fail on inconsistent data

🚀 Performance Impact

Positive

• Datacenter Routing: Significant improvement by targeting specific DCs based on labels
• Compact Encoding: V1 IDs are more compact than separate UUID+label storage

Concerns

• Base36 encoding/decoding is computationally expensive
• Frequent Vec allocations in serialization paths
• Database computed columns add storage overhead

🧪 Test Coverage

Critical Gaps

• Limited Core Tests: Only basic roundtrip tests in packages/common/util/core/src/id.rs:354-373
• No Error Handling Tests: Missing tests for IdError variants
• No Migration Tests: No validation of rollback scenarios
• No Cross-Language Tests: No SDK compatibility validation
• No Performance Tests: No benchmarks for base36 operations

Missing Edge Cases

• Empty strings, invalid UTF-8, overflow conditions
• Malformed V1 IDs and error recovery
• Database serialization/deserialization edge cases

📋 Recommendations

High Priority (Blocking)

1. Fix unsafe UTF-8 conversion with proper error handling
2. Add migration rollback SQL and validation
3. Add comprehensive error handling tests
4. Validate existing datacenter data before migration

Medium Priority

1. Optimize base36 encoding with lookup tables or streaming approach
2. Reduce allocations by using Cow<[u8]> for as_bytes()
3. Standardize API naming (new_v1 vs v1 constructors)
4. Add performance benchmarks for ID operations

Low Priority

1. Add utility methods like is_v1(), extract_uuid() for ergonomics
2. Consider caching for frequently accessed datacenter labels
3. Add metrics for ID format distribution

🎯 Overall Assessment

This is a well-architected change that introduces meaningful performance optimizations while maintaining backwards compatibility. The versioned ID system elegantly enables datacenter-aware routing without breaking existing functionality.

However, the unsafe code and missing migration rollback are blocking issues that must be resolved before merging.

Recommendation: Request Changes - Address the critical safety and migration issues, add comprehensive test coverage, then this will be ready for production.

---

[claude]

Claude finished @MasterPtato's task —— View job

---

🔍 PR Review: New Actor IDs Implementation

Reviewing this substantial change implementing new actor IDs across the entire system.

📋 Review Tasks

• Analyze core ID implementation and architecture
• Review database migrations and schema changes
• Examine API changes and backwards compatibility
• Check SDK consistency across languages
• Assess security implications
• Review performance considerations
• Evaluate test coverage
• Provide comprehensive feedback

📋 Review Summary

This PR introduces a sophisticated versioned ID system that balances backwards compatibility with performance optimization. The implementation is well-architected but has several critical issues that need attention.

✅ Strengths

Excellent Architecture Design

• Versioned System: Clean V0 (UUID) and V1 (UUID + label) design maintains backwards compatibility
• Performance Optimization: V1 labels enable efficient datacenter routing (see packages/core/api/actor/src/route/actors.rs:61-86)
• Comprehensive Integration: Proper SQLx, serde, and FoundationDB tuple packing support

Strong Implementation Quality

• Robust Error Handling: Comprehensive IdError enum with specific error types
• Base36 Encoding: Efficient encoding for V1 IDs (30 chars vs 38 for UUID+label)
• Type Safety: Strong typing throughout the system

⚠️ Areas for Improvement

Critical: Missing Input Validation

// packages/common/util/core/src/id.rs:204
let s = unsafe { String::from_utf8_unchecked(buf.to_vec()) };

Issue: Uses unsafe without validation that the base36 encoding produces valid UTF-8.
Risk: Potential memory safety issues if encoding logic has bugs.
Fix: Use String::from_utf8() and handle the error case.

Database Migration Concerns

-- packages/core/services/cluster/db/cluster/migrations/20250522174043_dc_label.up.sql
ALTER TABLE datacenters
    ADD COLUMN label BYTES AS (substring(datacenter_id::BYTES FROM 1 FOR 2)) STORED;

Issues:

• No rollback strategy for the computed column
• Missing validation that existing datacenter_ids are compatible
• Could fail if any existing IDs don't have the expected format

Performance & Resource Issues

Inefficient Base36 Decoding:

// packages/common/util/core/src/id.rs:317-333
fn decode_base36<const N: usize>(s: &str) -> Result<[u8; N], IdError> {
    // O(n²) algorithm with repeated multiplication
}

Impact: Expensive for V1 ID parsing. Consider pre-computed lookup tables.

Memory Allocation in Hot Path:

// packages/common/util/core/src/id.rs:90-101  
pub fn as_bytes(&self) -> Vec<u8> {
    // Always allocates new Vec
}

Impact: Frequent allocations for serialization. Consider returning Cow<[u8]>.

API Consistency Issues

Inconsistent Method Naming:

Id::new_v1(label)     // Constructor
Id::v1(uuid, label)   // Also constructor - confusing naming

Missing Error Context:

// packages/common/util/core/src/id.rs:257-258
let v = Id::from_bytes(slice)
    .map_err(|err| PackError::Message(format!("bad id format: {err}").into()))?;

Issue: Generic error message doesn't help with debugging.

🔒 Security Assessment

Low Risk Issues

• JWT claims still use UUID (packages/edge/infra/client/manager/src/claims.rs:63) - no security impact
• Base36 encoding is deterministic and doesn't leak information
• Label extraction is safe with proper bounds checking

Medium Risk

• The unsafe UTF-8 conversion mentioned above
• Database migrations could fail on inconsistent data

🚀 Performance Impact

Positive

• Datacenter Routing: Significant improvement by targeting specific DCs based on labels
• Compact Encoding: V1 IDs are more compact than separate UUID+label storage

Concerns

• Base36 encoding/decoding is computationally expensive
• Frequent Vec allocations in serialization paths
• Database computed columns add storage overhead

🧪 Test Coverage

Missing Critical Tests:

• Migration rollback scenarios
• Error cases for malformed V1 IDs
• Performance benchmarks for base36 encoding
• Cross-language SDK compatibility tests
• Edge cases (empty strings, invalid UTF-8, etc.)

Existing Tests: Basic roundtrip tests are present but insufficient for production readiness.

📋 Recommendations

High Priority

1. Fix unsafe UTF-8 conversion with proper error handling
2. Add migration rollback tests and validation
3. Optimize base36 encoding with lookup tables or streaming approach
4. Add comprehensive error context throughout the ID parsing pipeline

Medium Priority

1. Reduce allocations by using Cow<[u8]> for as_bytes()
2. Standardize API naming (new_v1 vs v1 constructors)
3. Add performance benchmarks for ID operations
4. Document migration strategy for existing data

Low Priority

1. Add utility methods like is_v1(), extract_uuid() for ergonomics
2. Consider caching for frequently accessed datacenter labels
3. Add metrics for ID format distribution

🎯 Overall Assessment

This is a well-architected change that introduces meaningful performance optimizations while maintaining backwards compatibility. However, the unsafe code and migration risks require attention before merging.

Recommendation: Approve with required changes - fix the critical safety issues and add proper test coverage, then this will be ready for production.

---

[claude]

Claude finished @MasterPtato's task —— View job

---

🔍 PR Review: New Actor IDs Implementation

I've completed a comprehensive review of this substantial PR implementing new actor IDs across the entire system.

📋 Review Tasks

• Read repository guidelines and setup instructions
• Analyze core ID implementation and architecture
• Review database migrations and schema changes
• Examine API changes and backwards compatibility
• Check SDK consistency across languages
• Assess security implications
• Review performance considerations
• Evaluate test coverage
• Provide comprehensive feedback

---

📋 Review Summary

This PR introduces a sophisticated versioned ID system that balances backwards compatibility with performance optimization. The implementation is well-architected but has several critical issues that need attention.

✅ Strengths

Excellent Architecture Design

• Versioned System: Clean V0 (UUID) and V1 (UUID + label) design maintains backwards compatibility
• Performance Optimization: V1 labels enable efficient datacenter routing (see packages/core/api/actor/src/route/actors.rs:61-86)
• Comprehensive Integration: Proper SQLx, serde, and FoundationDB tuple packing support

Strong Implementation Quality

• Robust Error Handling: Comprehensive IdError enum with specific error types
• Base36 Encoding: Efficient encoding for V1 IDs (30 chars vs 38 for UUID+label)
• Type Safety: Strong typing throughout the system

⚠️ Areas for Improvement

Critical: Missing Input Validation

// packages/common/util/core/src/id.rs:204
let s = unsafe { String::from_utf8_unchecked(buf.to_vec()) };

Issue: Uses unsafe without validation that the base36 encoding produces valid UTF-8.
Risk: Potential memory safety issues if encoding logic has bugs.
Fix: Use String::from_utf8() and handle the error case.

Database Migration Concerns

-- packages/core/services/cluster/db/cluster/migrations/20250522174043_dc_label.up.sql
ALTER TABLE datacenters
    ADD COLUMN label BYTES AS (substring(datacenter_id::BYTES FROM 1 FOR 2)) STORED;

Issues:

• No rollback strategy for the computed column
• Missing validation that existing datacenter_ids are compatible
• Could fail if any existing IDs don't have the expected format

Performance & Resource Issues

Inefficient Base36 Decoding:

// packages/common/util/core/src/id.rs:317-333
fn decode_base36<const N: usize>(s: &str) -> Result<[u8; N], IdError> {
    // O(n²) algorithm with repeated multiplication
}

Impact: Expensive for V1 ID parsing. Consider pre-computed lookup tables.

Memory Allocation in Hot Path:

// packages/common/util/core/src/id.rs:90-101  
pub fn as_bytes(&self) -> Vec<u8> {
    // Always allocates new Vec
}

Impact: Frequent allocations for serialization. Consider returning Cow<[u8]>.

API Consistency Issues

Inconsistent Method Naming:

Id::new_v1(label)     // Constructor
Id::v1(uuid, label)   // Also constructor - confusing naming

Missing Error Context:

// packages/common/util/core/src/id.rs:257-258
let v = Id::from_bytes(slice)
    .map_err(|err| PackError::Message(format!("bad id format: {err}").into()))?;

Issue: Generic error message doesn't help with debugging.

🔒 Security Assessment

Low Risk Issues

• JWT claims still use UUID (packages/edge/infra/client/manager/src/claims.rs:63) - no security impact
• Base36 encoding is deterministic and doesn't leak information
• Label extraction is safe with proper bounds checking

Medium Risk

• The unsafe UTF-8 conversion mentioned above
• Database migrations could fail on inconsistent data

🚀 Performance Impact

Positive

• Datacenter Routing: Significant improvement by targeting specific DCs based on labels
• Compact Encoding: V1 IDs are more compact than separate UUID+label storage

Concerns

• Base36 encoding/decoding is computationally expensive
• Frequent Vec allocations in serialization paths
• Database computed columns add storage overhead

🧪 Test Coverage

Missing Critical Tests:

• Migration rollback scenarios
• Error cases for malformed V1 IDs
• Performance benchmarks for base36 encoding
• Cross-language SDK compatibility tests
• Edge cases (empty strings, invalid UTF-8, etc.)

Existing Tests: Basic roundtrip tests are present but insufficient for production readiness.

📋 Recommendations

High Priority

1. Fix unsafe UTF-8 conversion with proper error handling
2. Add migration rollback tests and validation
3. Optimize base36 encoding with lookup tables or streaming approach
4. Add comprehensive error context throughout the ID parsing pipeline

Medium Priority

1. Reduce allocations by using Cow<[u8]> for as_bytes()
2. Standardize API naming (new_v1 vs v1 constructors)
3. Add performance benchmarks for ID operations
4. Document migration strategy for existing data

Low Priority

1. Add utility methods like is_v1(), extract_uuid() for ergonomics
2. Consider caching for frequently accessed datacenter labels
3. Add metrics for ID format distribution

🎯 Overall Assessment

This is a well-architected change that introduces meaningful performance optimizations while maintaining backwards compatibility. However, the unsafe code and migration risks require attention before merging.

Recommendation: Approve with required changes - fix the critical safety issues and add proper test coverage, then this will be ready for production.

---

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-site	Error			Aug 21, 2025 8:22pm

[claude]

Claude finished @MasterPtato's task —— View job

---

🔍 PR Review: New Actor IDs Implementation

Reviewing this substantial change implementing new actor IDs across the entire system.

Review Tasks:

• Read repository guidelines and setup instructions
• Analyze core ID implementation and architecture
• Review database migrations and schema changes
• Examine API changes and backwards compatibility
• Check SDK consistency across languages
• Assess security implications
• Review performance considerations
• Evaluate test coverage
• Provide comprehensive feedback

📋 Review Summary

This PR introduces a sophisticated versioned ID system that balances backwards compatibility with performance optimization. The implementation is well-architected but has several critical issues that need attention.

✅ Strengths

Excellent Architecture Design

• Versioned System: Clean V0 (UUID) and V1 (UUID + label) design maintains backwards compatibility
• Performance Optimization: V1 labels enable efficient datacenter routing (see packages/core/api/actor/src/route/actors.rs:61-86)
• Comprehensive Integration: Proper SQLx, serde, and FoundationDB tuple packing support

Strong Implementation Quality

• Robust Error Handling: Comprehensive IdError enum with specific error types
• Base36 Encoding: Efficient encoding for V1 IDs (30 chars vs 38 for UUID+label)
• Type Safety: Strong typing throughout the system

⚠️ Areas for Improvement

Critical: Missing Input Validation

// packages/common/util/core/src/id.rs:204
let s = unsafe { String::from_utf8_unchecked(buf.to_vec()) };

Issue: Uses unsafe without validation that the base36 encoding produces valid UTF-8.
Risk: Potential memory safety issues if encoding logic has bugs.
Fix: Use String::from_utf8() and handle the error case.

Database Migration Concerns

-- packages/core/services/cluster/db/cluster/migrations/20250522174043_dc_label.up.sql
ALTER TABLE datacenters
    ADD COLUMN label BYTES AS (substring(datacenter_id::BYTES FROM 1 FOR 2)) STORED;

Issues:

• No rollback strategy for the computed column
• Missing validation that existing datacenter_ids are compatible
• Could fail if any existing IDs don't have the expected format

Performance & Resource Issues

Inefficient Base36 Decoding:

// packages/common/util/core/src/id.rs:317-333
fn decode_base36<const N: usize>(s: &str) -> Result<[u8; N], IdError> {
    // O(n²) algorithm with repeated multiplication
}

Impact: Expensive for V1 ID parsing. Consider pre-computed lookup tables.

Memory Allocation in Hot Path:

// packages/common/util/core/src/id.rs:90-101  
pub fn as_bytes(&self) -> Vec<u8> {
    // Always allocates new Vec
}

Impact: Frequent allocations for serialization. Consider returning Cow<[u8]>.

API Consistency Issues

Inconsistent Method Naming:

Id::new_v1(label)     // Constructor
Id::v1(uuid, label)   // Also constructor - confusing naming

Missing Error Context:

// packages/common/util/core/src/id.rs:257-258
let v = Id::from_bytes(slice)
    .map_err(|err| PackError::Message(format!("bad id format: {err}").into()))?;

Issue: Generic error message doesn't help with debugging.

🔒 Security Assessment

Low Risk Issues

• JWT claims still use UUID (packages/edge/infra/client/manager/src/claims.rs:63) - no security impact
• Base36 encoding is deterministic and doesn't leak information
• Label extraction is safe with proper bounds checking

Medium Risk

• The unsafe UTF-8 conversion mentioned above
• Database migrations could fail on inconsistent data

🚀 Performance Impact

Positive

• Datacenter Routing: Significant improvement by targeting specific DCs based on labels
• Compact Encoding: V1 IDs are more compact than separate UUID+label storage

Concerns

• Base36 encoding/decoding is computationally expensive
• Frequent Vec allocations in serialization paths
• Database computed columns add storage overhead

🧪 Test Coverage

Missing Critical Tests:

• Migration rollback scenarios
• Error cases for malformed V1 IDs
• Performance benchmarks for base36 encoding
• Cross-language SDK compatibility tests
• Edge cases (empty strings, invalid UTF-8, etc.)

Existing Tests: Basic roundtrip tests are present but insufficient for production readiness.

📋 Recommendations

High Priority

1. Fix unsafe UTF-8 conversion with proper error handling
2. Add migration rollback tests and validation
3. Optimize base36 encoding with lookup tables or streaming approach
4. Add comprehensive error context throughout the ID parsing pipeline

Medium Priority

1. Reduce allocations by using Cow<[u8]> for as_bytes()
2. Standardize API naming (new_v1 vs v1 constructors)
3. Add performance benchmarks for ID operations
4. Document migration strategy for existing data

Low Priority

1. Add utility methods like is_v1(), extract_uuid() for ergonomics
2. Consider caching for frequently accessed datacenter labels
3. Add metrics for ID format distribution

🎯 Overall Assessment

This is a well-architected change that introduces meaningful performance optimizations while maintaining backwards compatibility. However, the unsafe code and migration risks require attention before merging.

Recommendation: Approve with required changes - fix the critical safety issues and add proper test coverage, then this will be ready for production.

---

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.