Tried working through Microsoft support and finally "accepted" them archiving it, basically "works as designed". Apparently, it is my fault for using TRY...CATCH and catching the error they were going to handle via checking @@error. If I called it without TRY...CATCH, sp_dropserver would have rolled back the transaction before returning. I tried to argue that they should be using TRY...CATCH internally to do the rollback instead of relying on @@error, or that they should document that this procedure should NOT be wrapped in TRY...CATCH when calling it. Oh Well!
Every step of that stored proc checks something in order of importance and handles situation where conditions are not met or encounters an error:
setting implicit transaction to off and checking tran count with an if then returning a value after raiserror
Checking to see if you have permissions - raising an error if you dont and returning a value.
Validating if @droplogins is not null and does not equal “droplogins” – if it meets these conditions it throws an error and returns a value
--Then we BEGIN TRANSACTION--
Checking and locking the server name and checking @@error if it is not equal to 0 – then rollback the transaction, raise an error, return a value.
If @droplogins is null and exists from some select statement where and or whatever conditions to detect no sysremotelogins or sysoledbusers except the default oledb-mapping – rollback the transaction, raise an error, return a value.
Check to see if a server is used by replication. If the return value is not 0 or @@error is something other than 0 – rollback the transaction and return a value
We check to see if a linked server loading into @is_linked
Check to see if this is a managed instance (cloud lifter)
Drop the server and remote/linked logins
Now we go through if/else if, commit the transaction, and return 0 for success.
To give you an idea where the code is breaking for your usage testing for replication servers:
-- CHECK TO SEE IF THE SERVER IS USED BY REPLICATION.
if object_id('sp_MSrepl_check_server') is not null
EXEC @ret = sp_MSrepl_check_server @server
if @ret <> 0 or @@error <> 0
@@error is coming back as something other than zero or rather, sp_MSrepl_check_server is throwing an error, that is where your catch block is grabbing it – its not handled in your code which leaves the transaction as open because the code does not continue to rollback the transaction or return 1 for failure.
Consider using the return code instead since conditions are handled inside of the stored procedure.
EXEC @ret = sp_dropserver “servername” “droplogins”
If @ret <> 0
Do stuff like logging results to a table
This also makes It easy to drop in a loop with values reset on the loop because that stored procedure handles its own errors and provides a return value.