fix(db): label routes replica_is_writer when READ_DATABASE_URL is not a standby - #4014
Open
rshmdev wants to merge 1 commit into
Open
fix(db): label routes replica_is_writer when READ_DATABASE_URL is not a standby#4014rshmdev wants to merge 1 commit into
rshmdev wants to merge 1 commit into
Conversation
… a standby Pointing READ_DATABASE_URL at the writer was accepted silently, and route telemetry then reported decision="replica" for pages the writer served. The freshness fence is trivially satisfied in that configuration -- the "replica" has zero lag because it IS the writer -- so nothing is unsafe, but buzz_db_route_decision becomes semantically misleading: operators watching the rollout dashboards read decision="replica" as the offload percentage and would believe replica offload is working while every byte still comes off the writer. The boot ping now reads cluster identity on the reader connection it already holds and settles a cached verdict. Pages the reader serves are labeled decision="replica_is_writer" while that verdict stands, so decision="replica" stays a truthful offload signal. The reason label (fresh/covered) is unchanged. Warn-only by design. Refusing the reader pool would trade a reporting defect for lost capacity, and it would latch the wrong answer on Aurora, where cluster-ro legitimately resolves to the writer while a cluster has no readers and starts resolving to a real reader as soon as one is added. The gate is pg_is_in_recovery() on the reader rather than an identifier match: system_identifier comes from pg_control_system(), which is superuser-only by default, so requiring a match would silently no-op wherever the relay's role is not a superuser. The identifier is read when permitted and reported in the warning to separate "the same node as the writer" from "some other primary", both of which offload the writer just as little. Refs block#3644 Signed-off-by: Rian <fortpcnite21@gmail.com>
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
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.
Summary
Pointing
READ_DATABASE_URLat the writer is accepted silently, and route telemetry then reportsdecision="replica"for pages the writer served. The freshness fence is trivially satisfied in that configuration — the "replica" has zero lag because it is the writer — so nothing is unsafe and no wrong rows are served. The damage is observability:decision="replica"is the offload percentage on the rollout dashboards, so an operator reads a healthy offload while every byte still comes off the writer.What this changes:
replica_fence::cluster_identity()readspg_is_in_recovery()and, when the role is permitted,pg_control_system().system_identifierfor one session.Db::spawn_read_pool_boot_pingsettles a cached verdict on the reader connection it already holds (no extra acquire on the read path), andWARNs when the reader endpoint is not a standby, reporting both identifiers and whether they match.buzz_db_route_decision{decision="replica_is_writer"}instead ofdecision="replica", sodecision="replica"stays a truthful offload signal. Thereasonlabel (fresh/covered) is unchanged — the predicate verdict is orthogonal to where the page came from.Routing behavior is untouched: identity is evidence, never a gate — the same posture as the existing Aurora identity probe.
Three decisions worth a reviewer's eye
1. The gate is
pg_is_in_recovery(), not the identifier match the issue suggested.system_identifiercomes frompg_control_system(), which is superuser-only by default; requiring a match would make this silently no-op on every deployment where the relay's role is not a superuser. A reader that is not replaying WAL is not a replica whatever its identifier says, so the recovery flag alone is the gate, and the identifier is read when permitted and logged to separate "the same node as the writer" from "some other primary" — both of which offload the writer just as little. That does mean an unrelated non-standby primary is also labeledreplica_is_writer; I kept the label name from the issue rather than inventing one, but happy to rename if you'd rather the label describe the gate.2. Warn-only, not fail-closed. The issue offered refusing the reader pool as an alternative. I didn't take it: it trades a reporting defect for lost capacity, and it would latch the wrong answer on Aurora, where
cluster-rolegitimately resolves to the writer while a cluster has no readers and starts resolving to a real reader the moment one is added.3. A new⚠️ Dashboards that assume
decisionvalue, not a newreason.decision ∈ {replica, writer}need the new value added. Putting it inreasoninstead would have leftdecision="replica"counting writer-served pages, which is the defect.buzz_db_route_decisionappears nowhere else in this repo, so nothing here needed updating — the rollout dashboards live inblock-coder-tf-stacks.Also boot-only, and only on the boot ping's successful path: a reader unreachable at boot leaves the verdict unset and routes are labeled
replicaas before. Re-probing per request would put a privileged catalog call on the read path to serve a dashboard.Related issue
Fixes #3644.
Duplicate check: no open PR touches reader identity or replica route telemetry. Closest neighbours are #3643 (NIP-50 search pool bypassing replica fencing) and #3622 (CI never runs the replica-routing Postgres fixtures), both split out of the same redteam thread as this issue; neither overlaps. Builds on #3268 (merged).
Testing
New tests:
cluster_identity_compares_only_when_both_identifiers_are_readable— no infrastructure. Pins that an unreadable identifier reports unknown, never different, so a non-superuser role is not misdiagnosed as a distinct cluster.cluster_identity_reads_a_primary_with_or_without_control_privileges— Postgres.Okeither way, identifier present orNone, and the privilege error must not poison the session for the statements the boot ping runs next.reader_pointing_at_the_writer_is_labeled_replica_is_writer— Postgres. Both URLs point at the same scratch database, which is the misconfiguration rather than a stand-in for it. Asserts the label before probing too, so a constructor that prejudged every reader would fail the test rather than pass it.The two Postgres-gated tests carry the crate's existing
#[ignore = "requires Postgres"], so per #3622 they will not run in CI — they needjust test(orcargo test -p buzz-db --lib -- --ignored) locally.Existing routing coverage re-run green:
count_events_routed_is_bounded_only,query_events_routed_defaults_dark_and_routes_covered_when_enabled,head_fetch_routes_by_configured_budget,channel_window_routes_head_to_writer_and_cursor_pages_to_replica,routed_fallback_spends_one_acquire_budget_when_aurora_cache_is_cold,thread_replies_cursor_pages_route_to_replica_with_writer_terminal_verification,routed_reads_are_confined_to_the_requested_community,routed_request_holds_one_snapshot_across_page_and_aux, plus the sixreplica_fencetests.Three failures in the full
--ignoredrun are pre-existing and unrelated (create_community_with_owner_enforces_per_owner_limit,transfer_ownership_returns_limit_reached_for_maxed_transferee,duplicate_event_keeps_first_community_provenance) — they depend on residual per-owner state in the shared dev database and reproduce identically with this branch stashed. A further 12 pass serially but fail at--test-threads 4, the same shared-database interference as #3929 / #2458.No UI change, so no screenshots.