Repository navigation
Concurrent startup, body retention on restart, and Helm network policy ports - #83
Merged
Merged
Conversation
Every instance applies db/schema.sql, the seeds and the guard-settings conversion when it starts. Two or more starting together against one database - Helm replicaCount above 1, a rolling upgrade, an autoscaler - ran them side by side, and the losers exited with "Database migration failed". schema.sql goes to Postgres as one multi-statement query, so one implicit transaction, and on an up-to-date database two of them deadlock on api_keys: each holds a lock that the other's DROP TRIGGER IF EXISTS / CREATE TRIGGER trg_api_keys_surfaces_valid needs (the relation on one side, the trigger's pg_trigger row on the other). On an empty database they instead race to create the same objects, and all but the first fail with a unique violation on pg_extension_name_index (CREATE EXTENSION IF NOT EXISTS pgcrypto). Reproduced with psql and with four run_migrations at once; 3 of 4 failed in both cases. run_migrations now takes a session-level advisory lock (SCHEMA_LOCK, "twschema") and runs the schema, the seeds and the conversion on the connection that holds it. The next instance waits on pg_advisory_lock, logging that it does, and then finds everything applied: the schema is idempotent, the seeds are ON CONFLICT DO NOTHING, and the conversion finds its marker. The conversion keeps its own transaction-level lock, which a 3.0 or 3.1 instance restarting during the rollout also takes. The lock lives on a pooled connection marked close-on-drop, and release closes it, so a connection still holding the lock never goes back to the pool, whether setup succeeds, fails or is cancelled; an instance that dies holding it loses the session and Postgres releases the lock. (A transaction-level lock releases itself too, but the ClickHouse lock below would then be a transaction left idle for as long as the backfill runs.) An instance of an earlier version does not take the lock, so one of those restarting at the very moment a new one migrates can still collide with it. ClickHouse setup had the same kind of race, without an error to show for it: ensure_clickhouse_tables backfills cost_rollup_hourly, provider_health_5m and mcp_server_call_counts from the logs when it finds them empty, so instances starting together could each find them empty and each copy the logs in - the SummingMergeTree rollups then counted every request once per instance. It now runs under a second lock (CLICKHOUSE_SETUP_LOCK, "twchinit"), taken per attempt rather than across the retry backoff: with ClickHouse down, instances taking turns through each other's ~22 s of retries would outlast the chart's startup probe. Nothing else at start-up writes shared state: the setup wizard already serializes on its own advisory lock and is not run at boot, there is no initial-admin bootstrap, and the TTL and bucket-lifecycle reconciles set values every instance computes the same way. tests/concurrent_startup.rs starts four setups at once on a fresh database, again on the result (a restart), and on a database put back to an earlier schema with the old guard settings, and compares the schema and seeds with a database one instance migrated. It also checks that a holder that dies or drops the lock releases it, and that four ClickHouse setups at once backfill a rollup once. Before this change the Postgres cases failed each time they ran, the ClickHouse one in one run of three (100 requests counted for 25). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With networkPolicy.enabled the server's egress allowed Postgres on 5432, Redis on 6379 and ClickHouse on 8123 whatever postgres/redis/clickhouse .externalUrl said. A database on any other port was blocked: Azure Cache for Redis over TLS (6380), ClickHouse Cloud (8443), a managed Postgres on a port of its own. The server then could not start, or for ClickHouse started without writing its logs. The allowed ports now come from the configured endpoint. A bundled service keeps its fixed port. For an external URL, tw.urlPorts takes every port the URL names - each host, comma-separated hosts included, each node= of a Redis Cluster or Sentinel URL, and a Postgres ?port= - and, for a host without one, the default the client uses for the scheme: 5432 for postgres, 6379 for redis and rediss (the server's Redis client doesn't change it for TLS, so Azure's 6380 has to be in the URL anyway), 26379 for a Sentinel plus 6379 for the primary it points to, 80 for http and 443 for https (not ClickHouse's 8123/8443: the HTTP client goes by the URL). So the policy allows the ports the server dials for what the URL names, with no second setting that could disagree with the URL, as an explicit redis.port could. The URL is read with regexes, not urlParse, which panics on a URL it can't parse: a malformed one still renders, with the default port. What no URL can say - the ports Redis Cluster nodes announce beyond the seeds, a Sentinel's primary on another port, an upstream or MCP server on a port other than 443 - goes in networkPolicy.extraEgress, rules appended to the server's egress as written. Rendered with Helm 3.22.0 and 4.3.0: with bundled databases and with the README's external values the policy is unchanged apart from its comments, and nothing outside networkpolicy.yaml changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ensure_clickhouse_tables runs deploy/clickhouse/initdb.d/01_init.sql on every start, and the file ended its gateway_logs and mcp_logs sections with ALTER TABLE ... MODIFY COLUMN <body column> TTL ... 30 DAY, for request_body, response_body, tool_arguments and tool_result. The configured audit.body_retention_days is put back afterwards, by reconcile_clickhouse_ttls in main.rs, but ClickHouse materializes a TTL on the data already stored as soon as it is set (materialize_ttl_after_modify), so a deployment keeping bodies longer than 30 days lost the ones older than that on each restart; the rows stayed, their bodies became NULL. Reproduced against ClickHouse 26.3: body TTL set to 90 days as the reconcile leaves it, a row from 60 days ago with its bodies, then the start-up sequence. Right after the table setup SHOW CREATE TABLE showed toIntervalDay(30) on all four columns, and once the mutations had run the row's bodies were NULL - also when the 90-day TTL was restored immediately after, as a real start does. The TTL now sits in the ADD COLUMN IF NOT EXISTS that creates each column, and the MODIFY statements are gone: a new column gets the 30-day default (a new column has nothing to lose), an existing one keeps what the server last set. Reading the configured value before the table setup would have needed Postgres in ensure_clickhouse_tables and a second place deciding the TTL; the reconcile stays the only one. The other TTLs at start-up don't have the pattern. Each log table's TTL is only in its CREATE TABLE IF NOT EXISTS, which leaves an existing table alone, and the reconcile sets the configured values. (It re-issues them on every start even when unchanged, which costs a TTL mutation per table but loses nothing.) The chart's own init SQL for the bundled ClickHouse runs once, on an empty data directory, and sets no body TTL. tests/clickhouse_retention_restart.rs runs a deployment with 90-day bodies and 365-day logs through a restart, step by step, and checks every TTL after each step and that the old bodies and rows are still there. Before the change it failed after the table setup (30 instead of 90) and, without that check, on the bodies (NULL). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The server's egress rules allowed port 9000, labelled "ClickHouse native", to any destination. The server never speaks ClickHouse's native protocol: the clickhouse crate it uses talks HTTP, on the port in CLICKHOUSE_URL, which the policy now allows by itself. Nothing else the chart configures uses 9000 either - body offload to S3 needs S3_BUCKET, which the chart doesn't set - so the rule only opened a port. An S3 endpoint on 9000 (RustFS, MinIO) set up outside the chart goes in networkPolicy.extraEgress; values.yaml and the README say so. helm lint and helm template, Helm 3.22.0 and 4.3.0, on the eleven value sets used for the previous change: the server's egress is DNS, 443 and the database ports, and nothing outside networkpolicy.yaml renders differently from dev. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rollout The start-up schema setup now holds a Postgres session-level advisory lock (SCHEMA_LOCK), and a session lock needs the session. Behind a pooler in transaction mode - PgBouncer's pool_mode = transaction, or a managed pooler's transaction port - each statement outside a transaction may go to a different server connection, and PgBouncer runs no reset query when a client leaves in that mode, so the lock can stay held on a server connection the pooler keeps: every instance started afterwards would wait on it with no end. The server has no separate URL for its migrations; it runs them on the connections of DATABASE_URL. So the chart's README (under the external databases, where Postgres is set up), the postgres .externalUrl comment in values.yaml and .env.example say to reach Postgres directly or through a pooler in session mode. The CHANGELOG's "Read before upgrading" gains that, and a sentence on the rollout: instances of earlier versions don't take the lock, so an old instance restarting while the first new one sets up the schema can still collide with it, which a normal rolling update doesn't do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Startup and deployment fixes found while testing Redis over TLS with two server instances.
Concurrent startup (
fix(startup))db/schema.sqlruns as one transaction. Two instances starting together deadlocked on an existing database (DROP TRIGGER IF EXISTS/CREATE TRIGGER trg_api_keys_surfaces_validonapi_keys) and, on a fresh one, failed with a duplicate key onpg_extension_name_index(CREATE EXTENSION IF NOT EXISTS pgcrypto). Reproduced: fourrun_migrationsat once failed 3 of 4. This hits replicas > 1, rolling upgrades and autoscaling.tests/concurrent_startup.rs: concurrent setup on fresh and existing databases, a killed lock holder, the ClickHouse backfill.Body retention reset on every boot (
fix(clickhouse))01_init.sqlranMODIFY COLUMN … TTL … 30 DAYon the four body columns at every start, and ClickHouse applies a new TTL to stored data at once, so deployments keeping bodies longer than 30 days lost older bodies on each restart (reproduced on ClickHouse 26.3: a 60-day-old row's bodies became NULL with 90-day retention). The 30-day default now sits only in theADD COLUMN IF NOT EXISTSthat creates a column; existing columns keep the retention the server set.tests/clickhouse_retention_restart.rs.Helm network policy (
fix(helm))networkPolicy.extraEgressfor ports no URL can name. Unused ClickHouse native port 9000 removed.helm lint/helm templatewith Helm 3.22.0 and 4.3.0 across 11 value sets; onlynetworkpolicy.yamlrenders differently.CHANGELOG [Unreleased]: Read before upgrading (roll out normally; transaction-mode poolers) and Fixed entries.
Local: fmt, clippy (lib/bins and tests), 740 unit tests, 409 integration tests.
🤖 Generated with Claude Code