Full Parity for Managed SQL Services (Azure SQL, MySQL/PostgreSQL Flexible Server, Cloud SQL)

[thzgajendra]

Objective

Bring the four managed-SQL services to complete native parity — Azure SQL (Microsoft.Sql), Azure Database for MySQL Flexible Server, Azure Database for PostgreSQL Flexible Server, and GCP Cloud SQL — matching the depth of the RDS work: every high-value native resource a real workload provisions, surfaced in cross-service discovery, priced in the cost catalog, and emitting metrics. Covers issue #295 item 1 (discovery) for these services.

What we found

Before this PR the four services exposed only the server/instance surface (via the shared relationaldb portable driver) plus snapshots. None of their native child resources existed, none were surfaced in discovery or cost, and Azure SQL Managed Instances were absent entirely. Blast radius: a client using the real cloud SDKs against cloudemu could create a server but not the databases, users, firewall rules, pools, failover groups, managed instances, etc. that real workloads depend on.

How we fixed it

Native resources are added as optional relationaldb driver capabilities discovered by type assertion — the existing SubnetGroups pattern — so each mock only answers for what its cloud actually has, and the handlers route them the way real SDK clients do (ARM sub-resource routes for Azure, sqladmin sub-collections for Cloud SQL).

Per service:

• Azure MySQL Flexible Server — databases, firewall rules, configurations (single + batch), server failover.
• Azure PostgreSQL Flexible Server — databases, firewall rules, configurations (config accepts PUT+PATCH; no failover/batch in that SDK).
• Azure SQL — firewall rules (shared cap), VNet rules, elastic pools, failover groups (with Primary↔Secondary failover), the Azure AD administrator, and the SQL Managed Instance family (Microsoft.Sql/managedInstances + managed databases, CRUD/list/failover).
• GCP Cloud SQL — databases, users, SSL certs, the clone / failover / promote-replica / start-stop-replica instance actions, and the static tiers and flags reference catalogs.

Cross-cutting:

• Discovery — a shared RelationalDatabases capability + walker surfaces managed servers in Azure Resource Graph (microsoft.sql/servers, microsoft.dbformysql/flexibleservers, microsoft.dbforpostgresql/flexibleservers) and GCP Cloud Asset (sqladmin.googleapis.com/Instance). (This capability merged into development via the RDS PR; conflicts were resolved so RDS and the SQL services share one walker keyed on the relationaldb portable service.)
• Cost — relationaldb:* rates so managed servers are billed per instance-hour.
• Metrics — server-level metrics plus the pool-scoped Microsoft.Sql/servers/elasticpools namespace on elastic-pool create.

The mocks cascade-delete children on server/instance delete, use copy-on-read / replace-on-write for slice-bearing state, and keep the shared relationaldb handler wiring unchanged (capabilities are structural — no Drivers changes).

Alternatives not taken

• Per-service capability interfaces in each provider package (server imports provider, à la AKS) — rejected in favor of the shared-driver SubnetGroups precedent, which maximizes reuse (firewall rules, databases, failover shared across services) and keeps type assertions clean.

Docs / Tests

• Docs — docs/services.md gains a "native sub-resources" section for the Azure/GCP managed-SQL capabilities (25 optional capability interfaces total) with refreshed operation counts; docs/features.md updates the discovery driver list.
• Tests — real-SDK round-trip tests per service (armmysqlflexibleservers, armpostgresqlflexibleservers, armsql, sqladmin) covering every new family including managed instances and tiers/flags; mock-level error-path / cascade / aliasing / metric-emission tests; a discovery walker test; type-map tests; and a cost-rate test.

Test plan

• go build ./...
• go test ./... (touched trees pass; SDK round-trips green under -race)
• golangci-lint run on touched packages — no new issues
• Resource Graph / Cloud Asset list managed SQL servers; Cloud SQL tiers/flags return catalogs

Risk & Rollback

Additive: new optional interfaces + new routes + new catalog entries; no existing behavior changes and no shared-handler signature changes. Rollback is a straight revert of the branch.

Conclusion

All four managed-SQL services now match their cloud's native resource surface — including Azure SQL Managed Instances and Cloud SQL tiers/flags — appear in cross-service inventory, carry cost, and emit metrics. No deferred follow-ups.

[NitinKumar004]

Review — full parity for managed-SQL services (Azure SQL / MySQL·PostgreSQL Flexible / Cloud SQL)

Deep end-to-end pass (5-agent fan-out + direct verification): blast radius, cascade, per-service edge cases, discovery/cost/metrics, and pattern conformance. Build + go test -race on the four provider packages are green.

✅ Blast radius — clean, fully additive

• All ~25 new capabilities are separate optional interfaces reached via comma-ok type assertion; the base RelationalDB interface is unchanged, so the RDS Mock and existing Azure/GCP SQL Mocks still satisfy it (build confirms). No panic-prone assertions.
• Discovery merge doesn't clobber anything — RelationalDatabases is one engine slot, but each provider builds its own Engine and sets its own adapter (rdsDiscovery{p.RDS} / sqlDiscovery{sql,mysql,pg} / cloudSQLDiscovery); Azure correctly fans its three mocks into one adapter. RDS discovery is untouched.
• Type maps (resourcegraph/cloudasset) and cost.go rates are additive, no collisions. New sub-resource routes don't shadow existing server routes. (LOW: an unknown azuresql sub-resource path now 404s instead of falling through to "serve the server" — a correctness improvement, worth a note.)

✅ Cascade + replace-on-write + locks — verified correct

Cascade-delete of children on parent delete works in all four (snapshot All(), trailing-/ prefix so foo never sweeps foo2/, no orphans). Mutators build fresh structs and Set (no in-place append/delete/[:0] on handed-out slices). All CRUD under the mock RWMutex; create holds one write lock across Has-then-Set (no TOCTOU). FailoverGroup's two slices are cloned on every path via copyFailoverGroup.

🟠 MEDIUM

1. ManagedInstance.Tags aliases the store on read (the one copy-on-read hole). GetManagedInstance/ListManagedInstances (providers/azure/azuresql/managedinstance.go:82,95) return a shallow struct copy sharing the stored Tags map — copyTags is applied on Create but not on either read path (every other new child struct is scalar-only or clones correctly). Reproduced: a caller writing to the returned Tags corrupts the store, and concurrent Get+mutate is the exact concurrent-map read/write panic class from RDS #301. -race passes only because no test mutates a returned Tags. Fix: mi.Tags = copyTags(mi.Tags) in both read paths.
2. PUT silently drops updates on an existing resource (Azure). createOrUpdate* treats an AlreadyExists as "idempotent GET" — it re-Describes and returns the stale body (server/azure/azuresql/operations.go:31-40, and the database/MI/managed-DB variants; same shape in mysqlflex/postgresflex putDatabase). Azure BeginCreateOrUpdate is an upsert, so a PUT changing administratorLogin/version/tags/maxSizeBytes/vCores returns old values with 200. The ModifyCluster/ModifyInstance update paths exist but are only reachable via PATCH — PUT-update is a dead-end. Untested.
3. MI PATCH is a no-op (server/azure/azuresql/managedinstance.go:81 routes PATCH → getManagedInstance, never decodes the body). armsql.ManagedInstancesClient.BeginUpdate changes to vCores/storage/SKU/tags are ignored.
4. Elastic-pool / failover-group PATCH is a destructive full-replace (subresources.go:307,443 route PUT+PATCH to the same put* → unconditional overwrite). A partial PATCH (e.g. change only the pool SKU, or add one DB to an FG) wipes every field absent from the body. Azure PATCH merges. Untested.
5. Database↔elastic-pool membership is unmodeled — Instance/armDatabaseProps carry no elasticPoolId, so an SDK client moving a DB into a pool has it silently dropped, and DeleteElasticPool never blocks (real Azure 409s a non-empty pool).
6. GCP replica/failover surface is largely decorative: FailoverInstance succeeds on any instance (no HA/failover-replica check → real Cloud SQL 400s); start/stopReplica reuse Start/StopInstance so a "replica" reports SUSPENDED (real keeps it RUNNABLE); PromoteReplica is a no-op and masterInstanceName/replicaConfiguration are never parsed, so no replica can actually exist.
7. Managed Instances aren't discoverable — sqlDiscovery.DiscoverDatabases walks only logical servers + flex, never the managedInstances store, and there's no microsoft.sql/managedinstances type-map entry. A created MI never appears in Resource Graph inventory. Project it or document the exclusion.

🟡 LOW

• List* endpoints iterate memstore.All() (random order) instead of SortedValues() — firewall/vnet/pools/FGs/managed-instances/managed-dbs (azuresql), databases/users/sslCerts (cloudsql), and the flex mirrors — non-deterministic SDK list ordering / flaky pagination. (Describe* using All() is fine — matches the id-filtering RDS precedent.)
• No handler-level SDK indexing test for SQL discovery despite the TestSDKResourceGraph_DatabricksIndexing precedent — coverage is unit type-map tests + a fake-driven walker test, so the real sqlDiscovery/cloudSQLDiscovery projection adapters (ARN/region/tags) are never exercised end-to-end.
• Config fidelity: SetConfiguration accepts arbitrary/unknown params (real Azure errors) and GetConfiguration of a known-but-unset param 404s instead of returning the default; MySQL batch updateConfigurations isn't atomic (partial-apply on first error).
• GCP: backup-run ID uses time.Now().UnixNano() (violates the config.Clock determinism rule, and collides within a nanosecond); CloneInstance doesn't clone child databases; UpdateUser doc-comment contradicts its (correct) NotFound behavior.
• Fidelity polish: firewall/VNet rules skip IP-range/subnet validation; FG replicationState hardcoded CATCH_UP; forced-vs-planned FG failover not distinguished (the action verb is dropped in path parsing); no provisioningState in any ARM body (LRO poller terminates on first response — fine for the SDK flows tested); MI subnetId not required; logical-server discovery carries no per-server Region.
• No -race/concurrency test for the new coarse-locked SQL mocks; cross-service metric emission runs under the mock write lock (no deadlock today, but a lock-held-across-external-call smell).

✅ Verified clean (no action)

Cost catalog additive + correct; metrics wiring (SetMonitoring on all four, nil-checked, elastic-pool Microsoft.Sql/servers/elasticpools namespace + resourceId dimension emitted after Set); type maps bidirectional + unit-tested; fail-loud discovery (walker error aborts engine.List, regression-tested); optional-capability pattern with compile-time _ rdsdriver.X = (*Mock)(nil) guards, idgen ARNs, cerrors→wire mapping on both clouds; discovery tag-aliasing safe (walker copyTags before the Resource escapes; mocks replace rather than mutate Tags). AAD admin singleton, MI cascade, failover-group role-flip, NotFound/AlreadyExists paths all correct and tested.

Bottom line

Architecturally solid and blast-radius-clean — the shared-capability pattern, cascade, replace-on-write, and discovery/cost/metrics wiring all conform. The one to fix before merge is MEDIUM #1 (ManagedInstance.Tags read aliasing — the panic class). The Azure update-semantics cluster (#2 PUT-drops-updates, #3 MI PATCH no-op, #4 pool/FG destructive PATCH) are real, untested correctness gaps that undercut the "complete parity" claim and are worth addressing together (with update tests). The GCP replica surface (#6) and MI-not-discoverable (#7) are the notable fidelity gaps; the rest (List determinism, the SQL discovery indexing test, config/validation) are LOW. A -race test that mutates a returned Tags map would catch #1.

[thzgajendra]

Thanks for the deep pass — really thorough. All MEDIUM and LOW items are addressed. Summary:

MEDIUM — fixed

1. ManagedInstance.Tags read aliasing — GetManagedInstance/ListManagedInstances now clone Tags. Added a -race test that mutates a returned Tags map to pin it. (providers/azure/azuresql/managedinstance.go)
2. PUT dropped updates — PUT on an existing server/database/managed-instance now applies the body (upsert) instead of returning the stale record.
3. MI PATCH no-op — PATCH now decodes and applies the body via UpdateManagedInstance.
4. Pool/FG destructive PATCH — elastic pools, failover groups and managed instances gained Update* capability methods with merge semantics; PATCH overlays only supplied fields (SDK tests: pool keeps SKU when only maxSizeBytes is patched; MI keeps administratorLogin when only vCores is patched).
5. Elastic-pool membership — databases carry ElasticPoolID (surfaced as elasticPoolId); DeleteElasticPool now 409s while the pool is non-empty.
6. GCP replica surface — a Cloud SQL insert with masterInstanceName creates a real replica (records master + replicaNames); start/stopReplica require a replica and keep it RUNNABLE; promoteReplica detaches (and rejects a non-replica); failover is primary-only.
7. MIs not discoverable — sqlDiscovery now projects managed instances; added the microsoft.sql/managedinstances type map and a real-SDK Resource Graph indexing test.

LOW — fixed

• List* now use SortedValues() for deterministic ordering.
• Added the handler-level SDK discovery indexing test (sqlDiscovery end-to-end) you noted was missing.
• GCP backup-run IDs use the config.Clock + a monotonic counter (was time.Now().UnixNano()); CloneInstance clones child databases; UpdateUser doc corrected.
• Firewall rules validate IPv4 start/end; MI requires subnetId.
• Added a -race concurrency test for the SQL mocks.

LOW — intentional mock simplifications (documented, not changed)

• Config parameter catalog / defaults / atomic batch — modeling the full per-engine parameter catalog with defaults is out of scope for the emulator; the mock stores user-set values, which round-trips SDK clients.
• provisioningState in ARM bodies — LRO pollers terminate on the first DONE/200 response, so the SDK flows don't need it (as you noted).
• Forced-vs-planned FG failover — the ARM path parser collapses the trailing action verb; all three FG failover variants perform the same role flip.
• Metric emission under the mock lock — matches the emit-on-mutate pattern used by every cloudemu mock; the monitoring backend never re-enters the DB mock, so there's no deadlock.
• FG replicationState / logical-server per-server region — cosmetic, not load-bearing for round-trips.

Build, go test -race on the four provider packages, and golangci-lint on all touched packages are green.

[NitinKumar004]

Deep review — Managed-SQL parity (Azure SQL / MySQL·PostgreSQL Flex / Cloud SQL)

Reviewed end-to-end in an isolated worktree: gate matrix, static/coverage layer, architecture-pillar fit, and a four-lens corner-case pass (aliasing/cascade, server routing/SDK fidelity, discovery/cost/metrics, validation/state-machines) with adversarial verification.

Gates (verified): build ✓ vet ✓ test ✓ -race ✓ go mod tidy ✓ gofmt ✓ — CI-blocking gates pass. golangci-lint is clean on this PR's changed files (its other hits are pre-existing s3/ec2/elbv2 code).

Architecture: strong. The 26 optional capabilities discovered by type-assertion (the SubnetGroups precedent) are the right call — the build passing proves AWS RDS isn't forced to implement them. The shared discovery walker's conflict-resolution is clean: no RDS regression, and the Resource Graph / Cloud Asset type maps are symmetric in both directions. Firewall-validation parity, child parent-existence checks, Cloud SQL replica/lifecycle state machines, LRO termination, and error envelopes all check out.

One theme runs through the findings: Azure SQL Managed Instance — the newest, most complex addition — is under-finished vs. its siblings (no state guard, no metrics, no cost rate).

High

• Cloud SQL DeleteInstance dangles replica links — the new deleteChildren removes databases/users/sslCerts but never updates ReadReplicaSource / ReadReplicaTargets, though linkReplica and PromoteReplica maintain both sides. Deleting a replica leaves its master advertising a phantom replica; deleting a master leaves the replica pointing at a nonexistent source; a re-created same-name instance inherits phantom replica status. (inline)

Medium

• Managed Instance lifecycle ops have no state guard — FailoverManagedInstance sets "Ready" unconditionally, so failover on a Stopped MI silently starts it; Start/Stop are no-op-success on the wrong state. Siblings (cloudsql.transitionInstance, mysqlflex.FailoverInstance) validate. (inline)
• Managed Instance emits no metrics though it's discoverable and monitoring is wired into the same mock — DB/pool siblings emit; MI create emits nothing (Pillar 3). (inline)
• Cloud SQL RestoreBackup maps to create-new, not in-place replace — server/gcp/cloudsql/operations.go:144-164 uses RestoreInstanceFromSnapshot{NewInstanceID: p.name}. The round-trip test only passes because it deletes the target first (with a comment admitting the workaround). A user calling Instances.RestoreBackup(project, existingInstance, …) per the normal flow gets 409 ALREADY_EXISTS.
• New-code coverage below the 90% pillar — azuresql 55%, mysqlflex 67%, postgresflex 72%, cloudsql 63%; server pkgs 66-70%. Concrete untested routes: MI start/stop, server PATCH, Cloud SQL Users.Update, and most error paths.

Low

• No cost rate for CreateManagedInstance → MI bills at 0.0 (most expensive real SQL resource). (inline)
• Failover-group failover has no partner/secondary validation (ping-pongs role; can yield "secondary with no primary"), and the ARM parser drops the 4th path segment so /forceFailoverAllowDataLoss and any unknown POST verb are treated as a planned /failover. (inline)
• Azure Flex SetConfiguration accepts unknown parameter names + empty values (real Azure 404s unknown params). (inline)
• Inconsistent duplicate-create semantics — AlreadyExists (databases/MI) vs silent create-or-replace (firewall/vnet/pool/failover-group); re-creating a firewall rule silently clobbers.
• Nits: Cloud SQL PUT treated as PATCH-merge (not full replace); firewall rules don't validate Start ≤ End; stale /sql/v1beta4 selfLink/targetLink vs the served /v1.

Pre-existing (checked against the diff — NOT introduced by this PR)

• Nondeterministic Describe*-all map iteration (child List* correctly sort).
• Read-path Tag/slice aliasing on Instance/Cluster/Snapshot — the PR's new resources clone correctly, and handlers are read-only, so it's latent.

Verdict: comment. Additive, gate-green, architecturally clean; the findings are correctness/fidelity polish. I'd prioritise the High replica-link fix and the Managed-Instance cluster (state guard + metrics + cost) before merge.

[NitinKumar004]

[High] DeleteInstance dangles replica links. deleteChildren removes databases/users/sslCerts but never updates ReadReplicaSource/ReadReplicaTargets, though linkReplica and PromoteReplica maintain both sides. → Delete a replica and its master still lists it as a live replica; delete a master and the replica points at a nonexistent source; a re-created same-name instance inherits phantom replica status. Fix: on delete, if the instance is a replica remove its ID from the master's ReadReplicaTargets; if it's a master, clear/repoint ReadReplicaSource on each target (or reject deleting a master with live replicas, matching real Cloud SQL).

[thzgajendra]

Fixed in ba51c9d. DeleteInstance now fetches the instance first, calls unlinkReplicas (removes this ID from the master's ReadReplicaTargets and clears ReadReplicaSource on any replicas pointing at it), then cascades children. Covered by TestDeleteInstanceUnlinksReplicas (deletes a replica then a master and asserts both sides are clean).

[NitinKumar004]

[Medium] Managed-instance lifecycle has no state guard. setManagedInstanceState only 404s then writes blindly, and FailoverManagedInstance sets "Ready" unconditionally. → Failover on a Stopped MI silently starts it; Start/Stop report success on the wrong source state. Siblings validate (cloudsql.transitionInstance, mysqlflex.FailoverInstance requires StateAvailable) — mirror that here (reject failover unless Ready; make Start/Stop idempotent on target state).

[thzgajendra]

Fixed in ba51c9d. Replaced the blind writer with transitionManagedInstance(from, to, verb): NotFound if missing, idempotent no-op if already in the target state, FailedPrecondition if not in the expected from state. Start requires Stopped→Ready, Stop requires Ready→Stopped, and FailoverManagedInstance now requires Ready (no longer silently starts a stopped MI). Covered by TestManagedInstanceStateGuards.

[NitinKumar004]

[Medium] Managed Instance emits no metrics. Monitoring is wired into this same mock and the DB/elastic-pool siblings emit on create, but CreateManagedInstance emits nothing. → A client that discovers the MI in Resource Graph then queries Azure Monitor gets an empty series where siblings return data (Pillar 3). Emit the Microsoft.Sql/managedInstances namespace on create (and lifecycle).

[thzgajendra]

Fixed in ba51c9d. CreateManagedInstance now calls emitManagedInstanceMetrics (nil-checked), emitting avg_cpu_percent / storage_space_used_mb / virtual_core_count on Microsoft.Sql/managedInstances with a resourceId dimension, matching the DB/elastic-pool siblings. Covered by TestDatabaseAndManagedInstanceEmitMetrics.

[NitinKumar004]

[Low] No cost rate for CreateManagedInstance. MI falls through to the 0.0 fallback though it's the most expensive real SQL resource (vCore-hour). The other three services map create → CreateInstance (0.017). Add a relationaldb:CreateManagedInstance rate.

[thzgajendra]

Fixed in ba51c9d. Added "relationaldb:CreateManagedInstance": 0.50 (GP 4-vCore instance-hour proxy) to defaultRates.

[NitinKumar004]

[Low] Failover-group failover lacks validation. FailoverFailoverGroup flips ReplicationRole with no check that the group has PartnerServers and no rejection of an already-Secondary group → repeated calls ping-pong the role and can produce a Secondary with no primary, a state real ARM never yields. Related: the ARM parser drops the 4th path segment, so /forceFailoverAllowDataLoss (and any unknown POST verb under a failover group) is treated as a planned /failover rather than 404.

[thzgajendra]

Fixed in ba51c9d. FailoverFailoverGroup now returns FailedPrecondition when the group has no PartnerServers, so a standalone group can no longer ping-pong its role into a "Secondary with no Primary". Also captured the ARM action verb (new SubResourceAction in ParsePath) so doFailover accepts only failover/forceFailoverAllowDataLoss and 404s any other POST verb instead of treating it as a planned failover. Covered by TestFailoverGroupWithoutPartnerRejected, TestParsePathSubResourceAction, and the SDK force-failover round-trip.

[NitinKumar004]

[Low] SetConfiguration accepts unknown parameters + empty values. Only Name != "" is checked; any name/value is fabricated and stored. Real Azure Flexible Server 404s unknown server parameters. (Consistent with postgresflex, so a fidelity gap, not a parity gap.) → A typo'd parameter returns 200 here but 404s on real Azure, so config-validation tests won't catch it.

[thzgajendra]

Fixed in ba51c9d (both mysqlflex and postgresflex). SetConfiguration now rejects an unknown parameter name with NotFound (validated against a knownServerParameters catalog) and an empty value with InvalidArgument. Covered by TestSetConfigurationValidatesParameter in each package.

[thzgajendra]

Thanks for the deep second pass — all findings addressed in ba51c9d. Per-comment replies inline; the review-body items:

High / Medium

• DeleteInstance dangling replica links — fixed (unlink both sides before cascade; test added).
• MI state guard / metrics / cost — all three fixed (state machine, create-time metrics, 0.50 rate).
• RestoreBackup create-new → 409 — fixed. Added an optional BackupRestorer capability; the GCP handler now type-asserts it and restores in place onto the existing target (adopting the backup's engine/version/storage), so Instances.RestoreBackup(project, existingInstance, …) works in the normal flow. Removed the delete-target workaround from the round-trip test and added TestRestoreBackupInPlace.

Low / nits

• FG failover partner guard + ARM action-verb parsing (forced vs planned; unknown verb 404s) — fixed.
• Firewall Start ≤ End validation — fixed.
• Azure Flex SetConfiguration unknown-param (404) + empty-value rejection — fixed.
• Stale /sql/v1beta4 selfLink/targetLink → now use the served /v1 prefix — fixed.

Coverage — substantial lift on the new packages: providers now azuresql 89% (was 57), cloudsql 87% (67), postgresflex 87% (72), mysqlflex 82% (67). The specific routes you named are now covered: MI start/stop (raw HTTP, since the vendored armsql SDK doesn't expose them), server PATCH (ServersClient.BeginUpdate), Cloud SQL Users.Update, BackupRuns.Get, and handler error paths. The wire-handler server packages moved up more modestly (~68–73%); pushing those to a strict 90% would take broad fault-injection across every writeErr/method-not-allowed branch — happy to do that as a follow-up if you'd like it in this PR.

Deliberately not changed (flagging rather than silently fixing):

• Duplicate-create semantics (firewall/vnet/pool/FG "silently clobber" vs databases/MI AlreadyExists): Azure PUT is an idempotent upsert, so create-or-replace is the correct wire behavior for those routes; I left it as-is rather than introduce a spurious 409.
• Cloud SQL PUT-as-PATCH-merge: the SDK always sends the full resource on update, so merge vs full-replace is observationally identical here; can revisit if you want strict replace semantics.

Gates: build ✓ vet ✓ test ✓ (full suite) golangci-lint ✓ (0 issues on changed packages).

[NitinKumar004]

Re-review + edge-completeness audit

Second-review fixes — all resolved ✅

Verified ba51c9d against my prior findings; every one is fixed, mirrored across providers, with tests and green gates:

• DeleteInstance dangling replica links → unlinkReplicas (both sides).
• MI lifecycle / metrics / cost → transitionManagedInstance state guards, Microsoft.Sql/managedInstances metrics on create, relationaldb:CreateManagedInstance rate.
• RestoreBackup create-vs-replace → in-place BackupRestorer capability + handler rewired.
• Failover-group ping-pong + verb conflation → partner-required guard + SubResourceAction verb switch (404 on unknown).
• Flex SetConfiguration unknown/empty → knownServerParameters catalog + non-empty check; firewall Start ≤ End added.

One small note on that last fix: knownServerParameters is a 14-entry allow-list, so a real-but-uncommon MySQL/Postgres parameter now returns 404 — a fine mock trade-off, but widen it (or match by prefix) if real configs hit it.

Edge / end-to-end completeness

Core CRUD is complete and SDK-round-tripped for all four services; error envelopes decode to typed SDK errors; no unrouted driver methods, no LRO hangs, no crashes. The remaining gaps are edge/fidelity:

Medium

• Azure Flex config catalog semantics — GetConfiguration 404s on a valid-but-unset parameter and initial List is empty; real Azure returns the default. A client reading max_connections before writing it gets NotFound. (inline)
• MySQL batch-update is non-atomic — applies entries sequentially and returns on the first error, leaving earlier ones persisted (no pre-validate/rollback). (inline)
• Azure SQL DB create doesn't validate elastic-pool existence — ElasticPoolID stored with no lookup, though pool-delete guards against member DBs (asymmetric). (inline)
• FailoverGroups BeginUpdate (PATCH) + List pager routed but untested — distinct driver methods from the tested Create; regression risk.
• Azure SDK tests are happy-path only — no wire-level 404/409/400/405 assertions (behavior exists at the provider level; the HTTP mapping is unverified). GCP has TestSDKCloudSQLErrorPaths; Azure has none.

Low (fidelity / acceptable-as-documented)

• ManagedDatabases.BeginUpdate and ElasticPools.BeginFailover unsupported → 405 (no driver method).
• No enum validation (pool SKU-tier, FG failoverPolicy) — garbage round-trips.
• GCP: Databases.Update/Patch → 405; Users.Update silently discards Password; start/stopReplica are pure no-ops; Instances.Update (PUT) == PATCH-merge (no full replace).
• Azure Flex DB PUT-on-existing doesn't update (inconsistent with firewall PUT).
• No pagination anywhere; list ignores subscription/RG scope (ListByResourceGroup returns other RGs' resources).
• Routed-but-untested: DeleteManagedDatabase, FG update/list.

Verdict: comment (approve-with-notes). None of this blocks merge — it's fidelity/test-breadth polish on complete core parity. Highest-value: the config-catalog semantics + batch atomicity (a user would hit these), then the FG-update/list + managed-DB-delete SDK tests.

[NitinKumar004]

[Medium] Config catalog semantics. GetConfiguration returns NotFound for a valid parameter that hasn't been explicitly set, and initial ListConfigurations is empty. Real Azure Flexible Server exposes the full parameter catalog with defaults — Get("max_connections") on a fresh server returns 200 with the default. → An SDK client that reads a parameter before writing it gets a spurious 404. (Same in postgresflex.) Consider seeding the known catalog with defaults so unset-but-valid params return their default.

[thzgajendra]

Fixed in 267ea84 (both flex providers). knownServerParameters now maps each catalog name to its server default. GetConfiguration returns the default (Source system-default) for a known-but-unset parameter and only 404s an unknown name; ListConfigurations returns the full catalog with user overrides applied. Covered by the new default-fetch assertions in TestSetConfigurationValidatesParameter and the updated list assertions.

[NitinKumar004]

[Medium] Batch config update is not atomic. batchUpdateConfigurations applies each entry sequentially and returns on the first SetConfiguration error, leaving earlier entries already persisted — no pre-validation or rollback. → A batch of [valid, invalid] mutates the valid one, then returns an error, so the client sees failure over a partially-applied change. Pre-validate all names/values before applying (or apply-then-rollback).

[thzgajendra]

Fixed in 267ea84. Added an optional BatchConfigurations capability; BatchSetConfigurations validates every entry (and the server) before applying any write, so a bad entry no longer leaves earlier ones persisted. The mysqlflex batchUpdateConfigurations handler now routes through it. Covered by TestBatchSetConfigurationsIsAtomic (a good+bad batch leaves the good param at its default).

[NitinKumar004]

[Medium] DB create doesn't validate elastic-pool existence. ElasticPoolID is stored with no lookup against m.elasticPools, so a database can claim membership in a pool that doesn't exist — whereas pool delete correctly guards against member databases (asymmetric). → A user PUTs a DB with a bogus elasticPoolId and gets 201 + reported membership; real Azure returns 404/400. Validate the pool exists on create.

[thzgajendra]

Fixed in 267ea84. CreateInstance and ModifyInstance now call requireElasticPool, which resolves the pool name from a bare name or a full ARM id (.../elasticPools/{name}) and 404s if it does not exist on the server — symmetric with the pool-delete member guard. Covered by TestCreateDatabaseValidatesElasticPool (bare name, ARM id, and a rejected move-into-nonexistent-pool via Modify).

[NitinKumar004]

Deep-edge pass — concurrency + adversarial inputs

Ran two edge dimensions the earlier passes didn't cover. Strong clean bill on the hard stuff: no reachable panics, no DoS/unbounded allocation, no path traversal, no key-composition collisions (body decoding is MaxBytesReader(1MiB) + Decode → 400 on empty/malformed/type-mismatch; nested pointer fields nil-checked). Concurrency model is sound — every access is gated by the mock's m.mu (RMW atomic across Get→Set), cascades run in a single lock-hold (no half-applied state observable), lock order is one-directional SQL→monitoring (no re-entrant deadlock), and there are no lost-update windows.

Medium

• Tags/slice output aliasing → potential fatal error: concurrent map read and map write. DescribeInstances/Describe*/Create*/Modify* on the Instance/Cluster/Snapshot paths return a struct whose Tags map (and Cloud SQL VPCSecurityGroups/ReadReplicaTargets slices) is the same object stored in the memstore — out = append(out, v) with no re-copy (providers/azure/azuresql/azuresql.go:237, providers/gcp/cloudsql/cloudsql.go:238, plus the Create*/Modify*/Restore*/CreateCluster+Members/CreateSnapshot return paths). A caller legitimately owns its result, so if one goroutine mutates result[0].Tags["k"] while another reads the same instance, Go crashes the process. This is inconsistent with this PR's own discipline: managedinstance.go re-copies on every boundary (out.Tags = copyTags(mi.Tags), with the comment "Tags is cloned so a caller mutating the returned map can't corrupt the store"), and FailoverGroup (copyFailoverGroup) and RestoreBackup both clone on output. The fix is mechanical — copyTags/clone into out before returning on the listed paths. Scalar-only sub-resources (Database/User/SslCert/FirewallRule/VNetRule/Configuration/ElasticPool/AADAdmin/ManagedDatabase) are unaffected.

Low

• GCP insertDatabase/insertUser take the child name from the body without validating it (server/gcp/cloudsql/subresources.go:135,227); a name containing / creates a write-only orphan row (unreachable via GET/DELETE, which use a single path segment). Real Cloud SQL rejects such names.
• Several get-paths do x[0] on a Describe result with only an err != nil guard, no length check (server/gcp/cloudsql/operations.go:73, server/azure/azuresql/operations.go:79,221, mysqlflex/postgresflex operations.go). Safe today only because every wired mock returns NotFound (never [], nil) on a missing single-ID lookup — a one-line length guard would harden it against a future driver change.
• Case-sensitive / trailing-whitespace names create distinct resources (real clouds often fold case); negative/huge storage/vCores stored without range validation (no crash). Fidelity notes.

Verdict: comment. The edge surface is robust; the one actionable item is the Medium Tags output-aliasing (a real concurrent-map crash that also breaks this PR's own consistency). The Lows are hardening/fidelity.

[thzgajendra]

Thanks for the two-part re-review — everything actionable is fixed in 267ea84 (build/vet/test/-race/lint all green; lint 0 issues on changed packages).

Concurrency review (output aliasing — the one crash risk)

• Fixed the Tags/slice output-aliasing across every listed path. Added cloneInstance/cloneCluster/cloneSnapshot in azuresql and cloneInstance/cloneSnapshot in cloudsql, applied on all Describe*/Create*/Modify*/Restore*/Clone* returns. A returned value never shares its Tags map or VPCSecurityGroups/Members/ReadReplicaTargets slices with the store now — matching the managedinstance/FailoverGroup/RestoreBackup discipline. New TestDescribeInstancesResultDoesNotAliasStore (cloudsql) and TestDescribeResultsDoNotAliasStore (azuresql) mutate a returned map/slice and assert the store is intact.

Edge-completeness review — Medium (all fixed, per-comment inline)

• Flex config-catalog semantics → catalog with defaults on Get/List.
• MySQL batch update → atomic via BatchConfigurations (validate-all-then-apply).
• Azure SQL DB create/modify → validates elastic-pool existence.
• FG BeginUpdate (PATCH) + List pager → now covered by SDK tests.
• Azure happy-path-only tests → added *WireErrorMapping tests (404/400 via typed azcore.ResponseError) for azuresql, mysqlflex, and postgresflex, plus an explicit DeleteManagedDatabase SDK test.

Low (fixed)

• GCP insertDatabase/insertUser: names containing / now rejected (InvalidArgument) so no write-only orphan row.
• Single-ID get-paths: added len()==0 guards (gcp/azuresql/flex) — hardened against a future driver returning [], nil.
• Also added the firewall start <= end check to the two Flex providers (Azure SQL already had it) — consistency.
• Config catalog note: the allow-list is now name→default; still a representative subset, so a real-but-uncommon parameter 404s. Easy to widen if needed.

Low (deliberately not changed — flagged, per prior convention)

• Unsupported ops that correctly 405 (ManagedDatabases.BeginUpdate, ElasticPools.BeginFailover, GCP Databases.Update/Patch): no driver method by design.
• No enum validation (SKU tier / failoverPolicy), no pagination, list-not-RG-scoped, case/whitespace folding, negative-value range checks, GCP Users.Update password (the portable User has no password field), Instances.Update==PATCH-merge: these are fidelity notes with no functional impact in the mock; happy to take any as a follow-up if you want them.

Gates: go build ./... ✓ go vet ./... ✓ go test ./... ✓ golangci-lint ✓ (0 issues on changed pkgs).

[NitinKumar004]

Third-review fixes verified — approving ✅

Verified 267ea84 against the previous edge + concurrency findings; every one is resolved, correctly and with tests. Gates green on the merged head (build ✓ vet ✓ test ✓ -race ✓ gofmt ✓ go mod tidy ✓; lint clean on the changed files).

• Tags/slice output-aliasing (potential concurrent map read+write crash) → cloneInstance/cloneCluster/cloneSnapshot on every Describe/Create/Modify return path (deep-copies Tags + slices), matching the discipline already used for managed instances / failover groups.
• Flex config catalog → GetConfiguration returns the catalog default for a known-but-unset parameter (404 only for genuinely unknown); ListConfigurations returns the full catalog (overrides + defaults, sorted).
• MySQL batch update atomicity → new optional BatchConfigurations capability that pre-validates every entry before applying any, so a bad entry never leaves earlier ones persisted.
• Azure SQL DB → elastic-pool validation → requireElasticPool on create and modify.
• Azure wire-level error-path coverage → error_paths_test.go added for azuresql / mysqlflex / postgresflex.

Quality is solid — proper //nolint:gochecknoglobals on the new lookup table, the BatchConfigurations capability follows the optional-capability-via-type-assertion pattern, and the clone helpers are mirrored across providers. No regressions.

Across the review arc (functional correctness → edge/end-to-end completeness → concurrency + adversarial inputs) every finding is now closed. Non-blocking follow-up: the server/azure/*sql handler packages are still ~69–71% coverage (broad wire-level fault-injection would lift them). LGTM.