Fix reshard source-DB drop using empty database name#922
Merged
Conversation
For cnpg sources the admin handler leaves op.SourceDatabase empty at create time (it only fills it for external sources), and recordSource persisted the real name to the op row without updating the in-memory op. cleanupSource then issued DROP DATABASE "" — a zero-length delimited identifier error — leaving the old catalog database orphaned on the source shard after an otherwise successful cnpg-to-cnpg reshard. - recordSource now mirrors source_endpoint/user/database onto the in-memory op after persisting, so cleanup logging, the report, and the ext-to-cnpg rollback all see them. - cleanupSource drops o.source.Database (the copy-path source of truth) and skips loudly if the name is somehow empty instead of ever sending a zero-length identifier. - Regression assertion in TestReshardHappyPathCnpgToCnpg: the dropdb call must name the database recorded from the duckling status (previously "", reproducing the production failure).
Test Impact PlanDeterministic summary of how this PR changes tests, CI runners, and coverage-risk signals. Summary
Signals
Coverage risk: neutral or increased No coverage-reduction warnings detected. |
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.
Bug (observed on a real cnpg→cnpg reshard)
The cleanup step of an otherwise-successful cnpg→cnpg reshard failed like this (note the double space — the database name is empty):
The copy and cutover were fine (cleanup is deliberately non-fatal but loud), but the old catalog database was left orphaned on the source shard.
Root cause
op.SourceEndpoint/SourceUser/SourceDatabaseat create time only for external sources (controlplane/admin/reshard.go); for a cnpg source they stay"".recordSource(runner) reads the real source info from the duckling CR status and persists it to the op row viao.fields(...)— but never updated the in-memoryo.op.cleanupSourcepassedo.op.SourceDatabase(still"") toDropDatabase→DROP DATABASE ""→ SQLSTATE 42601. Theo.sourceendpoint struct built inrecordSourcecarried the correct database all along (the copy used it successfully).Fix
recordSourcenow mirrors the persistedsource_endpoint/source_user/source_databaseonto the in-memoryo.op, keeping every later reader (cleanup logging, the end-of-op report, the ext→cnpg rollback spec) consistent.cleanupSourcedropso.source.Database— the copy-path source of truth — and, belt-and-suspenders, logs an ERROR and skips the drop if the name is ever empty instead of issuing a zero-length identifier.Test
Regression assertion in
TestReshardHappyPathCnpgToCnpg(controlplane/provisioner/reshard_runner_test.go): the fake copier'sdropdbcall must name the database recorded from the duckling status. The test op'sSourceDatabaseis unset exactly like production, so pre-fix it fails with:and passes after the fix.
go build -tags kubernetes ./...andgo test -tags kubernetes ./controlplane/provisioner ./controlplane/admin -run TestReshard -count=1are green.Note on e2e: per the resharding contract in
CLAUDE.md, the cnpg→cnpg positive path (the only path that reachescleanupSource) is unit-only until the second mw-dev shard lands, so the regression lives in the runner unit test.🤖 Generated with Claude Code