chore: clean up pools, remove unused crates by MasterPtato · Pull Request #4013 · rivet-dev/rivet

MasterPtato · 2026-01-23T01:09:10Z

No description provided.

railway-app · 2026-01-23T01:09:17Z

🚅 Deployed to the rivet-pr-4013 environment in rivet-frontend

Service	Status	Web	Updated (UTC)
frontend-inspector	✅ Success (View Logs)	Web	Jan 27, 2026 at 3:17 am
frontend-cloud	✅ Success (View Logs)	Web	Jan 27, 2026 at 3:17 am
mcp-hub	✅ Success (View Logs)	Web	Jan 27, 2026 at 3:17 am
website	🕒 Building (View Logs)	Web	Jan 27, 2026 at 3:17 am

MasterPtato · 2026-01-23T01:09:26Z

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: move actor metrics to separate workflow #4044 : 2 dependent PRs (#4014 , #4047 )
chore: clean up pools, remove unused crates #4013 👈 (View in Graphite)
feat: add actor and kv metrics #3952
fix: remove attributes from last metric push metric #4043
fix(pb): always check actor queue on init #3982
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.

claude · 2026-01-23T01:11:39Z

Pull Request Review

Summary

This PR performs a cleanup of the pools package and removes two unused crates (clickhouse-inserter and clickhouse-user-query). It also reorganizes the metric keys by moving them from pegboard/src/keys/ns/metric.rs to namespace/src/keys/metric.rs.

Code Quality ✅

Strengths:

Clean removal of dead code (3565 deletions vs 128 additions)
Proper reorganization of metric keys to the more appropriate namespace package
All imports updated correctly to use namespace::keys::metric
No glob imports from anyhow (follows CLAUDE.md guidelines)

Observations:

pools/src/pools.rs: The cleanup is clean and maintains functionality. The clickhouse pool still exists but no longer depends on the removed clickhouse-inserter and clickhouse-user-query crates.
Metric key relocation: Moving metric keys from pegboard to namespace makes architectural sense since metrics are namespace-scoped, not pegboard-specific.

Potential Issues 🔍

Minor:

Import organization in pegboard-gateway/src/lib.rs:865-950: The metric_inc function has proper usage of the moved metric keys. However, the function is quite long (85 lines) with repetitive patterns. Consider extracting common logic into helper functions in a future refactor.
Error handling consistency: In pools.rs:76, ClickHouse pool uses .context("missing clickhouse pool") while UPS and UDB use custom errors. Consider standardizing the error handling approach.
Comment in pools.rs:43: Comment says "Only for tests" but there is no #[cfg(test)] attribute. If this is truly test-only, consider adding the cfg guard.

Performance Considerations ✅

No performance concerns. The changes are primarily deletions and reorganization:

Removed unused dependencies will improve compile times
Metric recording logic remains the same, just relocated

Security Concerns ✅

No security issues identified:

No new external dependencies added
No changes to authentication, authorization, or data validation logic
Metric recording follows existing patterns

Test Coverage ⚠️

Concerns:

The PR removes extensive test suites for the deleted crates:
- 619 lines from builder_tests.rs
- 307 lines from case_sensitivity_tests.rs
- 540 lines from integration_tests.rs
- 263 lines from query_tests.rs
- 44 lines from schema_tests.rs
Since these crates are being removed as unused, this is expected. However, verification needed: Ensure that no other parts of the codebase were relying on these crates indirectly. Run the full test suite to confirm.
The metric key relocation should be tested to ensure:
- Existing metrics can still be read (backward compatibility)
- New metrics are written to the correct location
- No data migration is needed (appears to be a code-only move)

Recommendations

✅ Verify build: Run cargo build --workspace to ensure no broken dependencies
✅ Run tests: Execute cargo test --workspace to verify no functionality broke
📝 Consider adding migration notes: If there are any running instances that might have issues with the metric key relocation, document the migration path
🔄 Future cleanup opportunity: The metric_inc function in pegboard-gateway could benefit from refactoring to reduce duplication

Verdict: ✅ APPROVE

This is a solid cleanup PR that:

Removes genuinely unused code
Improves code organization by relocating metrics to the appropriate package
Maintains all existing functionality
Follows repository conventions (proper imports, error handling patterns)

The changes are low-risk and improve the codebase maintainability. Just ensure the test suite passes before merging.

Reviewed by Claude Code

claude · 2026-01-23T01:11:52Z

Pull Request Review

Summary

This PR removes two unused ClickHouse-related packages (clickhouse-inserter and clickhouse-user-query) and cleans up the pools package to simplify its dependencies. Additionally, it refactors metric recording in pegboard-gateway to use the namespace package instead of pegboard::keys::ns.

Overall Assessment

✅ Approved - This is a clean refactoring that removes unused code and improves code organization.

Detailed Review

Strengths

Code Cleanup: Removes 3,565 lines of unused code, significantly reducing maintenance burden
Proper Dependency Management: Correctly removes workspace dependencies that are no longer needed
Refactoring: Moves metric recording from pegboard::keys::ns to the namespace package, which appears to be better separation of concerns
No Breaking Changes: Verified that the removed packages aren't referenced anywhere in the codebase

Code Quality

Pools Package Changes (`engine/packages/pools/`)

✅ Removed clickhouse_inserter field and related code
✅ Removed clickhouse_enabled flag that was no longer needed
✅ Simplified PoolsInner structure
✅ Removed corresponding error variants (MissingClickHouseInserter)
⚠️ Minor Issue: The pools code still uses .context("missing clickhouse pool") instead of the custom Error::MissingClickHousePool for consistency with other methods (line 76 in pools.rs)

Pegboard Gateway Changes (`engine/packages/pegboard-gateway/`)

✅ Added namespace.workspace = true dependency
✅ Changed metric calls from pegboard::keys::ns::metric to namespace::keys::metric
✅ Maintained exact same metric recording logic, just moved the location
✅ Proper use of subspaces with namespace::keys::subspace()

Namespace Metric Changes (`engine/packages/namespace/src/keys/metric.rs`)

✅ Made Metric enum public and added proper visibility modifiers
✅ Added pub to MetricKey, MetricSubspaceKey, and inc function
✅ Maintained backward compatibility with existing metric types

Potential Issues

Error Handling Inconsistency (Minor):
In pools.rs:76, the code uses:
```
self.0.clickhouse.clone().context("missing clickhouse pool")
```
But should use the custom error for consistency:
```
self.0.clickhouse.clone().ok_or(Error::MissingClickHousePool.into())
```
This matches the pattern used for ups() and udb() methods.
Documentation (Minor):
The PR has no description. Consider adding a brief explanation of what was removed and why.

Testing Recommendations

✅ Verify no references to removed packages remain (already checked - none found)
⚠️ Build verification needed: Ensure cargo build succeeds after these changes
⚠️ Test the metrics recording: Verify that gateway metrics still work correctly with the namespace package
⚠️ Integration tests: Run any existing tests that use the pools package

Security Considerations

✅ No security concerns - this is purely a cleanup/refactoring PR.

Performance Considerations

✅ No performance impact expected. The metric recording logic is identical, just moved to a different module.

Recommendations

Fix the error handling inconsistency in pools.rs:76 to use Error::MissingClickHousePool instead of a string context
Add a PR description explaining what was removed and why
Run the build and tests to ensure everything still works correctly

Conclusion

This is a solid cleanup PR that removes technical debt. The only issue is a minor inconsistency in error handling. Once that's addressed, this is ready to merge.

Overall Score: 9/10

pkg-pr-new · 2026-01-23T01:12:19Z

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/virtual-websocket

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 7e99948

claude · 2026-01-23T01:12:49Z

PR Review: Clean up pools, remove unused crates

Summary

This PR removes two unused ClickHouse packages (clickhouse-inserter and clickhouse-user-query) and refactors the metrics system by moving namespace metrics from the pegboard package to a dedicated namespace package. Overall, this is a good cleanup that reduces dependencies and improves code organization.

Code Quality & Best Practices ✅

Strengths:

Removes ~3500 lines of unused code, improving maintainability
Consolidates metrics logic into the appropriate namespace package
Removes unnecessary dependencies (bollard, async-channel, docker_credential, etc.)
Improves separation of concerns by moving namespace-specific metrics out of pegboard

Observations:

Metric naming improvement (engine/packages/namespace/src/keys/metric.rs:32): Changed ActorAwakeHours to ActorAwake which is clearer and more consistent with other metric names.
Display trait implementation (engine/packages/namespace/src/keys/metric.rs:60-75): Added a Display implementation for MetricVariant, which is good for debugging and logging. The snake_case formatting is consistent with Rust conventions.
Import organization (engine/packages/pegboard-gateway/src/lib.rs:18): Added namespace dependency to pegboard-gateway, which is appropriate since it now uses namespace::keys::metric.

Potential Issues 🔍

Minor Issues:

Import style (engine/packages/pegboard-gateway/src/lib.rs:3): The file uses use anyhow::Result; which is acceptable per CLAUDE.md (avoids glob imports).
Comment completeness (engine/packages/pools/src/pools.rs:9):
```
// TODO: Automatically shutdown all pools on drop
```
This TODO remains unaddressed. While not critical for this cleanup PR, it's worth tracking.
Error handling consistency (engine/packages/pools/src/pools.rs:76):
```
self.0.clickhouse.clone().context("missing clickhouse pool")
```
This uses context() while the other getters use custom error types (Error::MissingUpsPool, Error::MissingUdbPool). Consider using a custom error for consistency, though using context() is acceptable.

Performance Considerations ✅

No performance regressions expected. The changes are purely organizational:

Removed unused dependencies reduces compile time
Moved metrics code maintains the same runtime behavior
No changes to hot paths or algorithm complexity

Security Concerns ✅

No security issues identified:

Removed dependencies (bollard, docker_credential) eliminate potential attack surface
No changes to authentication, authorization, or data validation logic
Metrics recording logic remains unchanged functionally

Test Coverage ⚠️

Concerns:

Removed extensive test coverage: The deleted clickhouse-user-query package had comprehensive tests (1,773 lines total). If these packages are truly unused, this is fine. If there's any possibility they'll be needed in the future, consider archiving them instead of deleting.
No tests for namespace metrics move: The metrics code was moved from pegboard::keys::ns::metric to namespace::keys::metric. Existing integration tests should cover this, but explicit verification would be valuable.

Recommendation: Run the full test suite to ensure the metrics refactoring doesn't break any existing functionality:

cargo test -p pegboard-gateway
cargo test -p pegboard
cargo test -p namespace

Architecture & Design ✅

Improvements:

Better module organization: Moving namespace metrics to the namespace package improves architectural clarity. Namespace-related functionality should live in the namespace package, not pegboard.
Reduced coupling: Removing clickhouse-inserter from pools reduces unnecessary dependencies between packages.
Cleaner dependency graph: The workspace dependency cleanup (removing bollard, async-channel, etc.) suggests these were transitive dependencies of the removed packages.

Additional Recommendations

Verify unused code claim: Before merging, confirm that clickhouse-inserter and clickhouse-user-query are truly unused:
```
rg "clickhouse-inserter|clickhouse-user-query" --type rust
```
Update documentation: If these packages were documented anywhere (README, architecture docs), update those references.
Verify config schema: The config schema artifact was updated (engine/artifacts/config-schema.json). Ensure this doesn't break any deployment configurations that might reference vector or docker-related settings.

Conclusion

Recommendation: ✅ Approve with minor suggestions

This is a solid cleanup PR that improves code organization and reduces unnecessary dependencies. The metrics refactoring is well-executed, and the removal of unused packages reduces maintenance burden.

Action Items Before Merge:

✅ Verify packages are truly unused (quick grep)
✅ Run relevant test suites to confirm no regressions
⚠️ Consider addressing the error handling consistency in pools.rs (optional)
⚠️ Update any documentation referencing removed packages (if applicable)

Overall Assessment: High quality cleanup with no blocking issues. Safe to merge after verification.

Review generated by Claude Code

graphite-app · 2026-01-27T02:49:38Z

Merge activity

Jan 27, 2:49 AM UTC: MasterPtato added this pull request to the Graphite merge queue.
Jan 27, 2:50 AM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #3982.
Jan 27, 2:50 AM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #3982.
Jan 27, 2:59 AM UTC: MasterPtato added this pull request to the Graphite merge queue.
Jan 27, 3:00 AM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #4043.
Jan 27, 3:00 AM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #4043.
Jan 27, 3:12 AM UTC: MasterPtato added this pull request to the Graphite merge queue.
Jan 27, 3:12 AM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #4043.
Jan 27, 3:12 AM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #4043.
Jan 27, 3:17 AM UTC: MasterPtato added this pull request to the Graphite merge queue.
Jan 27, 3:18 AM UTC: CI is running for this pull request on a draft pull request (#4048) due to your merge queue CI optimization settings.
Jan 27, 3:19 AM UTC: Merged by the Graphite merge queue via draft PR: #4048.

railway-app bot temporarily deployed to rivet-frontend / rivet-pr-4013 January 23, 2026 01:09 Destroyed

This was referenced Jan 23, 2026

feat: add actor and kv metrics #3952

Closed

DO NOT MERGE: add metrics exporter for ee #4014

Closed

MasterPtato force-pushed the 01-22-chore_clean_up_pools_remove_unused_crates branch from 408d468 to 92a7f8a Compare January 23, 2026 01:09

MasterPtato force-pushed the 01-15-feat_add_actor_and_kv_metrics2 branch from fc05f9d to 8246e45 Compare January 23, 2026 01:09

railway-app bot temporarily deployed to rivet-frontend / rivet-pr-4013 January 23, 2026 01:09 Destroyed

MasterPtato force-pushed the 01-15-feat_add_actor_and_kv_metrics2 branch from 8246e45 to d1cdcc5 Compare January 26, 2026 21:23

MasterPtato force-pushed the 01-22-chore_clean_up_pools_remove_unused_crates branch from 92a7f8a to 2146e7b Compare January 26, 2026 21:23

railway-app bot temporarily deployed to rivet-frontend / rivet-pr-4013 January 26, 2026 21:23 Destroyed

This was referenced Jan 26, 2026

fix(pb): always check actor queue on init #3982

Closed

fix: remove attributes from last metric push metric #4043

Closed

fix: move actor metrics to separate workflow #4044

Closed

MasterPtato marked this pull request as ready for review January 26, 2026 21:28

NathanFlurry approved these changes Jan 27, 2026

View reviewed changes

chore: clean up pools, remove unused crates

7e99948

MasterPtato force-pushed the 01-15-feat_add_actor_and_kv_metrics2 branch from d1cdcc5 to 46b43db Compare January 27, 2026 03:16

MasterPtato force-pushed the 01-22-chore_clean_up_pools_remove_unused_crates branch from 2146e7b to 7e99948 Compare January 27, 2026 03:16

railway-app bot temporarily deployed to rivet-frontend / rivet-pr-4013 January 27, 2026 03:16 Destroyed

MasterPtato mentioned this pull request Jan 27, 2026

chore: remove ch provision users #4047

Closed

graphite-app bot pushed a commit that referenced this pull request Jan 27, 2026

chore: clean up pools, remove unused crates (#4013)

7c0ea73

graphite-app bot closed this Jan 27, 2026

graphite-app bot deleted the 01-22-chore_clean_up_pools_remove_unused_crates branch January 27, 2026 03:19

MasterPtato mentioned this pull request Jan 27, 2026

fix: fix active req caculation, add hws metrics #4053

Closed

This was referenced Jan 28, 2026

chore(guard): clean up guard #4057

Closed

fix(gateway): configure cors for error responses #4058

Closed

fix(guard): cache custom serve routes #4059

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: clean up pools, remove unused crates#4013

chore: clean up pools, remove unused crates#4013
MasterPtato wants to merge 1 commit into01-15-feat_add_actor_and_kv_metrics2from
01-22-chore_clean_up_pools_remove_unused_crates

MasterPtato commented Jan 23, 2026

Uh oh!

railway-app bot commented Jan 23, 2026 •

edited

Loading

Uh oh!

MasterPtato commented Jan 23, 2026 •

edited

Loading

Uh oh!

claude bot commented Jan 23, 2026

Uh oh!

claude bot commented Jan 23, 2026

Uh oh!

pkg-pr-new bot commented Jan 23, 2026 •

edited

Loading

Uh oh!

claude bot commented Jan 23, 2026 •

edited

Loading

Uh oh!

graphite-app bot commented Jan 27, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

MasterPtato commented Jan 23, 2026

Uh oh!

railway-app bot commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MasterPtato commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

claude bot commented Jan 23, 2026

Pull Request Review

Summary

Code Quality ✅

Potential Issues 🔍

Performance Considerations ✅

Security Concerns ✅

Test Coverage ⚠️

Recommendations

Verdict: ✅ APPROVE

Uh oh!

claude bot commented Jan 23, 2026

Pull Request Review

Summary

Overall Assessment

Detailed Review

Strengths

Code Quality

Pools Package Changes (engine/packages/pools/)

Pegboard Gateway Changes (engine/packages/pegboard-gateway/)

Namespace Metric Changes (engine/packages/namespace/src/keys/metric.rs)

Potential Issues

Testing Recommendations

Security Considerations

Performance Considerations

Recommendations

Conclusion

Uh oh!

pkg-pr-new bot commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: Clean up pools, remove unused crates

Summary

Code Quality & Best Practices ✅

Potential Issues 🔍

Performance Considerations ✅

Security Concerns ✅

Test Coverage ⚠️

Architecture & Design ✅

Additional Recommendations

Conclusion

Uh oh!

graphite-app bot commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

railway-app bot commented Jan 23, 2026 •

edited

Loading

MasterPtato commented Jan 23, 2026 •

edited

Loading

Pools Package Changes (`engine/packages/pools/`)

Pegboard Gateway Changes (`engine/packages/pegboard-gateway/`)

Namespace Metric Changes (`engine/packages/namespace/src/keys/metric.rs`)

pkg-pr-new bot commented Jan 23, 2026 •

edited

Loading

claude bot commented Jan 23, 2026 •

edited

Loading

graphite-app bot commented Jan 27, 2026 •

edited

Loading