fix(config): allow configuring topo dcs via map, fix pg ssl mode config#4456
Conversation
|
🚅 Deployed to the rivet-pr-4456 environment in rivet-frontend
|
|
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.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
CLAUDE.mdImportant Domain InformationALWAYS use
The ALWAYS use Never modify an existing published
CommandsBuild Commands# Build all packages in the workspace
cargo build
# Build a specific package
cargo build -p package-name
# Build with release optimizations
cargo build --releaseTest Commands# Run all tests in the workspace
cargo test
# Run tests for a specific package
cargo test -p package-name
# Run a specific test
cargo test test_name
# Run tests with output displayed
cargo test -- --nocaptureDevelopment Commands# Format code (enforced by pre-commit hooks)
# cargo fmt
# DO NOT RUN CARGO FMT AUTOMATICALLY (note for humans: we need to run cargo fmt when everything is merged together and make sure lefthook is working)
# Run linter and fix issues
./scripts/cargo/fix.sh
# Check for linting issues
cargo clippy -- -W warningsDocker Development Environment# Start the development environment with all services
cd docker/dev
docker-compose up -dGit Commands# Use conventional commits with a single-line commit message, no co-author
git commit -m "chore(my-pkg): foo bar"Never push to Dependency Managementpnpm Workspace
SQLite Package
RivetKit Package ResolutionsThe root {
"resolutions": {
"rivetkit": "workspace:*",
"@rivetkit/react": "workspace:*",
"@rivetkit/workflow-engine": "workspace:*",
// ... other @rivetkit/* packages
}
}When adding RivetKit dependencies to examples in {
"dependencies": {
"rivetkit": "*",
"@rivetkit/react": "*"
}
}If you need to add a new Rust DependenciesDocumentation
Code Blocks in Docs
Content FrontmatterDocs (
|
PR ReviewSummary: Two focused fixes -- allow configuring topology datacenters via a hashmap (with backward-compatible list support) and fix the PostgreSQL SSL mode configuration. The Bug: Inverted SSL behavior (breaking change)File: The SSL logic has been inverted in a breaking way. Before: SSL only when Any existing deployment without let pool = if ssl_disabled {
pool_config.create_pool(Some(Runtime::Tokio1), tokio_postgres::NoTls)...
} else if let Some(config) = &config.ssl_config {
// build TLS config from explicit cert paths
...
} else {
pool_config.create_pool(Some(Runtime::Tokio1), tokio_postgres::NoTls)...
};Bug: DSN-format connection strings not handled for SSL checkFile:
Bug: Test helper sets
|
f301f2d to
f07072d
Compare
f07072d to
ea5642d
Compare
8ee527a to
5c55237
Compare
|
This PR makes two main changes:
Bug: Programmatic Datacenter creation in tests sets name on map entries In engine/packages/test-deps/src/lib.rs, Datacenter is inserted into a HashMap with the name field explicitly set. But the validation in engine/packages/config/src/config/mod.rs rejects map entries where name is non-empty. Both the test-deps setup and Default::default() for Topology create Datacenter values in the map with name already populated, which would fail this check if validate() is called on them. Programmatic map construction should use name: String::new() to let validation populate it, or validation should only run on deserialized user configs, not programmatically constructed configs. Behavioral Change: SSL Logic Inversion The SSL logic has been inverted in engine/packages/universaldb/src/driver/postgres/database.rs:
This is a silent breaking change for existing deployments that do not have ssl_config configured and also do not have sslmode=disable in their connection strings. They would now unexpectedly attempt TLS. This should be explicitly called out with migration notes. Non-deterministic HashMap Ordering DatacentersRepr::Map uses HashMap which has non-deterministic iteration order. Fine for the concurrent health fanout, but callers that need stable ordering should consider IndexMap or BTreeMap. Missing Runtime Deprecation Warning The list format is marked Deprecated in doc comments and JSON schema, but no runtime warning is emitted when a config using the list format is loaded. A tracing::warn! in the validation branch for List would help operators discover the deprecation. Minor: Error Message The validation error shows dc.name (the value) but not the map key. Including the key would make the error more actionable. Positive Observations
Generated by Claude Code |
Code Review: PR #4456 — fix(config): allow configuring topo dcs via map, fix pg ssl mode configSummaryThis PR makes two independent improvements:
Overall the changes are well-scoped and solve a real usability problem with a clean approach. A few issues are worth addressing before merge. IssuesBug: Validation order in validate_and_set_defaults — names checked before they are setFile: engine/packages/config/src/config/mod.rs The valid_hosts validation loop runs before the map-name-injection loop. When the map form is used, dc.name is still empty string at the point of the valid_hosts error message, so users would see datacenter must have valid_hosts configured rather than the helpful name. Move the name-injection block above the valid_hosts validation, or use the map key directly in the error message. Design concern: HashMap iteration order is non-deterministicFile: engine/packages/config/src/config/topology.rs HashMap has no defined iteration order. Any code that depends on a consistent ordering of datacenters (e.g., determining which DC is the first for default/leader selection, or producing deterministic health-check output) will behave non-deterministically across process restarts. Consider using IndexMap (from the indexmap crate) or BTreeMap if ordering needs to be stable. Missing is_empty companion methodFile: engine/packages/config/src/config/topology.rs DatacentersRepr implements len() but not is_empty(). Clippy will flag any x.len() == 0 check. Consider adding an is_empty method that returns self.len() == 0. SSL mode detection — silent fallback on non-URL connection stringsFile: engine/packages/universaldb/src/driver/postgres/database.rs If Url::parse fails (e.g. DSN format host=... dbname=...), ssl_disabled silently defaults to false, meaning TLS will be attempted. This is safe but could be confusing. A debug-level log when the URL parse fails would help. Also worth adding a comment explaining that only disable bypasses TLS — all other modes (including prefer) will use Rustls. Unrelated lockfile churn: removal of @visx/* packagesFile: pnpm-lock.yaml The diff removes all @visx/* specifiers from the frontend package's pnpm-lock.yaml importers section. This appears unrelated to the stated purpose of the PR. Please confirm this is intentional and explain in the PR description, or revert. Minor / Style
Checklist
|
ea5642d to
f6c3742
Compare
5c55237 to
876713f
Compare
876713f to
3bf4304
Compare
f6c3742 to
10c9ce3
Compare
3bf4304 to
9282590
Compare
10c9ce3 to
42a11eb
Compare
9282590 to
bc5319a
Compare
38080c6 to
875b002
Compare
bc5319a to
72b27cf
Compare
PR Review: fix(config): allow configuring topo dcs via map, fix pg ssl mode configThis PR makes two main changes: (1) allows topology.datacenters config to be specified as a map keyed by name in addition to the existing list format, and (2) fixes PostgreSQL SSL mode configuration so that sslmode=disable in the connection string correctly disables TLS. A secondary cleanup converts actor_names deserialization to a typed ActorName struct. Critical IssueDefault Topology will fail validate_and_set_defaults() In Topology::default() (topology.rs), the Datacenter inside the map entry has name set to default string explicitly. But the new validation in validate_and_set_defaults() bails with an error if dc.name is non-empty for map-format datacenters, since the name is auto-derived from the map key. Calling validate_and_set_defaults() on a Root::default() config will fail. Fix: change the default Datacenter to use name: String::new(). Other IssuesValidation error shows empty string for map-format datacenter names (mod.rs): The valid_hosts validation block runs before the map name-auto-derivation block. When datacenters are configured as a map, dc.name is still empty string when the error is constructed. The name-assignment block should be moved before the valid_hosts check. DatacentersRepr has len() without is_empty() (topology.rs): Clippy will warn (len_without_is_empty). An is_empty() method should be added alongside len(). SSL mode detection only handles disable (postgres/database.rs): The fix correctly handles sslmode=disable, but sslmode=allow / sslmode=prefer still enforce TLS via Rustls. A comment explaining why only disable maps to NoTls would help future maintainers. One caller not updated to the new iterator pattern: engine/packages/pegboard/src/ops/runner/list_runner_config_enabled_dcs.rs still calls .clone() on DatacentersRepr directly rather than using the .iter().cloned().collect() pattern that the three other updated callers now use. Positive Changes
Generated with Claude Code (https://claude.ai/claude-code) |
72b27cf to
a9b074b
Compare
875b002 to
14a6bb1
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: