Tests Cleanup | Fix resource leaks in test suites

[cheenamalhotra]

Description

Test fixtures create GUID-named SQL Server objects (tables, procs, table types, CMKs/CEKs, logins, queues, databases) and certificates. When setup or cleanup fails these leak permanently, eventually exhausting object identifiers (error 3807) and littering CI agents with certificates and private-key containers.

Test infrastructure only. No product code is changed.

Three root causes:

1. Constructor failures bypass cleanup. xUnit skips Dispose when a fixture or test-class constructor throws. Affected constructors now clean up and rethrow.
2. Cleanup ran as one unguarded batch. A single failing DROP aborted the rest. Drops are now guarded and run independently, best-effort with a console diagnostic.
3. Certificates leaked key material. PersistKeySet writes a private-key container that store removal does not delete. CertificateFixtureBase now deletes it.

Per review feedback, tests were also consolidated onto the shared RAII types in tests/Common/Fixtures/DatabaseObjects/*, replacing four ad-hoc create/delete mechanisms. New primitives: Schema, ScalarFunction, TemporalTable.

Notes for reviewers

• SpecialCharacterNames.cs keeps unguarded drops on purpose — OBJECT_ID guarding would break its intentional apostrophe / ]] escaping.
• Guarded drops need SQL Server 2016+, matching existing precedent.
• Cleanup failures log rather than throw, so they cannot mask the real test failure.
• TemporalTable's DDL ordering is verified by CI, not locally.
• Deferred to its own PR: folding the parallel Always Encrypted Setup/ hierarchy into Common/Fixtures/DatabaseObjects.

Issues

N/A - proactive cleanup, not tracked by a specific issue.

Testing

No new tests; these are setup/cleanup paths, so the existing suites are the verification. All three test projects build clean, and the repo was re-scanned afterwards for create/drop mismatches, unguarded drops, drops outside finally, and unsafe fixture constructors — no further findings.

Validation

• Tests added or updated (cleanup paths in existing test infrastructure)
• Public API changes documented (N/A - no public API changes)
• Verified against customer repro (N/A)
• Ensure no breaking changes introduced (test-only, no product code touched)

Guidelines

Please review the contribution guidelines before submitting a pull request:

• Contributing
• Code of Conduct
• Best Practices
• Coding Style
• Review Process

[Copilot]

Pull request overview

This PR hardens the SqlClient test infrastructure to prevent SQL Server object leaks (GUID-named tables/procs/types/keys/logins/databases) and Windows certificate private-key container leaks when test setup/cleanup fails—especially in constructor-failure scenarios where xUnit never calls Dispose.

Changes:

• Makes fixture/test constructors exception-safe (cleanup-on-failure) to avoid leaking partially-created SQL objects/certificates.
• Refactors cleanup to be idempotent and best-effort (guarded DROP ... IF EXISTS / OBJECT_ID checks; per-statement execution so one failure doesn’t block the rest).
• Adds shared BulkCopy cleanup helpers and improves certificate + AKV key deletion/purge behavior.

Reviewed changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/Microsoft.Data.SqlClient/tests/UnitTests/Fixtures/AlwaysEncrypted/NativeColumnEncryptionKeyCertificateBaselineFixture.cs	Avoids disposing cert too early; ensures cleanup runs if ctor fails.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/VectorBackwardCompatTestBase.cs	Ctor exception-safety + best-effort disposal of created DB objects.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/NativeVectorTestsBase.cs	Ctor exception-safety + best-effort disposal of created DB objects.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/TransactionTest.cs	Drops temp tables independently and guarded to prevent cascading leaks.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlNotificationTest/SqlNotificationTest.cs	Ensures ctor failures clean up; guarded/idempotent per-statement cleanup.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCredentialTest/SqlCredentialTest.cs	Splits cleanup into guarded, independent statements to prevent login leaks.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/TvpTest.cs	Ensures server objects are always dropped via finally; independent guarded drops.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParallelTransactionsTest/ParallelTransactionsTest.cs	Guards drop statement to make cleanup idempotent.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/JsonTest/JsonBulkCopyTest.cs	Adds IDisposable to drop tables and delete scratch files.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/ConnectivityTest.cs	Ensures DB drop isn’t blocked by SINGLE_USER failure; guards ALTER/DROP.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionTestWithSSLCert/CertificateTestWithTdsServer.cs	Makes cert + registry/service cleanup resilient and independent.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/UnprivilegedLogin.cs	Ctor exception-safety + best-effort cleanup to avoid leaking instance-wide login.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/TableLock.cs	Moves setup into try and centralizes best-effort table drops.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/SpecialCharacterNames.cs	Runs cleanup statements independently to avoid masking failures/leaks.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/KeepNulls.cs	Moves setup into try and centralizes best-effort table drops.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/Helpers.cs	Adds shared guarded/best-effort cleanup helpers (TryCleanup, DropTable(s), ProcessCleanupBatch).
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/FireTrigger.cs	Ensures epilogue cleanup runs and is independent per statement.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/DestinationTableNameWithSpecialChar.cs	Uses cleanup batch that continues on failures to prevent leaks.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/DataConversionErrorMessageTest.cs	Guards drop + ensures dispose closes connection even on cleanup errors.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/CopyVariants.cs	Uses guarded/best-effort drops via shared helpers.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/ColumnCollation.cs	Adds try/finally to ensure table drop on assertion failure.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/CheckConstraints.cs	Uses guarded/best-effort drops via shared helpers.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/CacheMetadata.cs	Uses guarded/best-effort drops via shared helpers.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/Bug98182.cs	Uses cleanup batch that continues on failures to prevent leaks.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/Bug903514.cs	Ensures table drop runs even if bulk copy/assertions fail.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/Bug85007.cs	Uses guarded/best-effort drops via shared helpers.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/Bug84548.cs	Uses guarded/best-effort drops via shared helpers.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/SqlSetupStrategyCspProvider.cs	Uses SetupOrCleanUp and makes CSP key deletion cleanup more resilient.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/SQLSetupStrategyCertStoreProvider.cs	Uses SetupOrCleanUp to prevent ctor failure leaks.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/SQLSetupStrategyAzureKeyVault.cs	Uses SetupOrCleanUp; awaits delete and best-effort purges AKV keys.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/SQLSetupStrategy.cs	Adds SetupOrCleanUp; fixes reverse-in-place; makes drops best-effort per object/connection.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/Setup/Table.cs	Adds guarded drop for idempotent cleanup.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/Setup/ColumnMasterKey.cs	Adds guarded drop for idempotent cleanup.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/Setup/ColumnEncryptionKey.cs	Adds guarded drop for idempotent cleanup.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/ConversionTestFixture.cs	Makes ctor exception-safe by disposing on partial setup failure.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/SqlNullValues.cs	Makes ctor exception-safe; makes cleanup per-step best-effort.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ExceptionsGenericError.cs	Makes ctor exception-safe; makes cleanup best-effort and prevents masking failures.
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/EnclaveAzureDatabaseTests.cs	Makes ctor exception-safe; fixes reverse-in-place and makes drops best-effort.
src/Microsoft.Data.SqlClient/tests/Common/Fixtures/CspCertificateFixture.cs	Makes ctor exception-safe to avoid leaking cert + persisted key container.
src/Microsoft.Data.SqlClient/tests/Common/Fixtures/ColumnMasterKeyCertificateFixture.cs	Makes ctor exception-safe to avoid leaking cert + persisted key container.
src/Microsoft.Data.SqlClient/tests/Common/Fixtures/ColumnEncryptionCertificateFixture.cs	Makes ctor exception-safe; avoids early disposal that breaks cleanup tracking.
src/Microsoft.Data.SqlClient/tests/Common/Fixtures/CertificateFixtureBase.cs	Tracks created certs and deletes persisted private-key containers during cleanup.
src/Microsoft.Data.SqlClient/tests/Common/Fixtures/AzureKeyVaultKeyFixtureBase.cs	Awaits key deletion and attempts best-effort purge; avoids leaving soft-deleted keys.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 64 out of 64 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 65 out of 65 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/VectorBackwardCompatTestBase.cs:55

• _connection.Open() is executed before the constructor’s try/catch. If Open() throws, xUnit won’t call Dispose() and the SqlConnection instance will not be disposed (which defeats the leak-prevention goal of this change). Move the Open() call inside the try so the catch path can run cleanup consistently.
  src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/UnprivilegedLogin.cs:62
• _managementConnection.Open() is outside the constructor’s try/catch. If opening the connection throws, xUnit won’t call Dispose() and the connection won’t be disposed. Move the Open() call inside the existing try so the catch cleanup path also covers connection-open failures.

[Copilot]

Pull request overview

Copilot reviewed 65 out of 65 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/VectorBackwardCompatTestBase.cs:104

• DisposeSafely is written to accept nulls (disposable?.Dispose()), but its parameter is declared non-nullable. In nullable-enabled builds this pattern is inconsistent and makes it easy to introduce CS8604/CS8625 warnings if a nullable IDisposable is ever passed (especially during constructor-failure cleanup paths). Declare the parameter as IDisposable? to match the implementation intent.

[Copilot]

Pull request overview

Copilot reviewed 74 out of 74 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParallelTransactionsTest/ParallelTransactionsTest.cs:157

• DropTempTable creates a SqlCommand but never disposes it. Wrap the command in a using (or explicitly call Dispose()) so repeated runs don’t accumulate command-related resources.

[Copilot]

Pull request overview

Copilot reviewed 74 out of 74 changed files in this pull request and generated no new comments.

Suppressed comments (6)

Previously missed (6) — in code that hasn't changed since the last review.

src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/Transaction1.cs:38

• SqlCommand created for the explicit BEGIN/ROLLBACK transaction isn't disposed. In a test cleanup-focused PR, this can still leak handles/resources when many tests run. Use a using declaration for deterministic cleanup.
  src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/Transaction2.cs:33
• The SqlTransaction created here is rolled back but never disposed. Disposing transactions deterministically avoids leaking server-side state if tests fail mid-flight; a using declaration is sufficient.
  src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/Transaction2.cs:42
• SqlCommand myCmd is created and used to execute a query but never disposed. Wrap it in a using declaration so command handles are released deterministically during test runs.
  src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/Transaction3.cs:37
• The transaction created on conn3 is never disposed. Even though the connection will eventually be disposed, using a using declaration keeps the test deterministic and ensures rollback/dispose happens even if later code throws.
  src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/TransactionTestAsync.cs:45
• The transaction is rolled back but not disposed. Prefer a using declaration so the transaction object is deterministically cleaned up even if WriteToServerAsync throws.
  src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/Transaction3.cs:46
• SqlCommand myCmd is created to execute the mismatched-transaction reader call but never disposed. Use a using declaration to avoid leaking command resources across the test suite.

[Copilot]

Pull request overview

Copilot reviewed 76 out of 76 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Microsoft.Data.SqlClient/tests/Common/Fixtures/CertificateFixtureBase.cs:319

• If opening the cert store for cleanup fails, the code silently falls back to opened = false with no diagnostic. Since a store-open failure means certificates won’t be removed (and may leak across runs), logging the failure would make leaks actionable without changing the best-effort behavior.

            try
            {
                store.Open(OpenFlags.ReadWrite);
                opened = true;
            }
            catch (Exception)
            {
                opened = false;
            }

[Copilot]

Pull request overview

Copilot reviewed 76 out of 76 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParallelTransactionsTest/ParallelTransactionsTest.cs:153

• The temp-table cleanup runs from a finally block, but DropTempTable can still throw (e.g., permission/locking), which could mask the original test failure. To align with the PR’s “best-effort cleanup” goal, catch and log exceptions in this helper so cleanup failures don’t override test results.
  src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/ErrorOnRowsMarkedAsDeleted.cs:151
• This DROP TABLE runs in a finally block but is unguarded and not best-effort. If the drop fails, it can replace/mask the real test failure. Wrap the drop in try/catch (and preferably use DROP TABLE IF EXISTS) so cleanup can’t override the test outcome.

[paulmedynski]

I have reviewed the DatabaseObjects changes. I'll look at the rest once my feedback has been addressed.

Please to not resolve my feedback - I will do that once I'm happy with the changes.

[paulmedynski]

Why would we ever want shouldCreate = true and shouldDrop = false? Can we eliminate these flags entirely? They are creating scenarios where we don't clean up.

[paulmedynski]

If we remove Table.AdoptExisting() (which is not used anywhere), then we can eliminate these flags.

[cheenamalhotra]

Removed in 8db1e6d. You are right that it has no callers — I added it speculatively while consolidating the BulkCopy tests and then never needed it. Deleted rather than kept "just in case".

[edwardneal]

We need to keep at least one of these flags. I introduced them with HiddenTargetColumn and temporal tables in mind. This creates both a table and its history table in a single statement, then needs to drop both of them. Doing so requires us to be able to at least represent shouldCreate = false, and I felt it'd be tidier to have symmetry between drops and creations.

My end-to-end idea at the time was to introduce a TemporalTable primitive which created the main table, "adopted" the history table into a HistoryTable property, and dropped both tables upon disposal.

[cheenamalhotra]

Thanks — that context changes the picture, and you're right. I removed it purely on the evidence that it had no callers, which missed the case it was there to serve.

I've built out the TemporalTable primitive you describe in 36a765c. It creates the main table, adopts the history table into a HistoryTable property, and drops both on disposal in the order the server requires (SYSTEM_VERSIONING = OFF, then DROP PERIOD FOR SYSTEM_TIME, then the tables).

On the flags themselves, I tried to satisfy both your point and Paul's. shouldCreate is genuinely needed, but it does not have to be a bool: adoption is now a separate constructor selected by an ExistingObject discriminator, so the call site reads Table.AdoptExisting(connection, name) rather than shouldCreate: false. shouldDrop stays removed — it was true at every call site including the adopted history table, so the symmetry would have been decorative. If a genuine no-drop case turns up it can come back the same way.

Two things fell out of doing this:

• HiddenTargetColumn is now converted and its cleanup bug is gone. The old finally ran the two ALTER statements through a separate RunNonQuery before the two DropTable calls, so any failure in the ALTERs skipped both drops and orphaned both tables — exactly the failure mode this PR is about.
• HISTORY_TABLE needs a schema-qualified name. Your original test hardcoded dbo., which I nearly lost when generating the name; TemporalTable now qualifies it with [dbo] explicitly.

One caveat worth flagging: I can only compile locally, not run integration tests, so the temporal DDL ordering is reasoned from the original test rather than verified against a server. Worth a close look.

[cheenamalhotra]

Reopening this one: @edwardneal pointed out that AdoptExisting exists for a real case I could not see from call sites — a temporal table's history table is created by the server as a side effect, so it has no CREATE of its own but still must be dropped.

I have restored adoption in 36a765c, but not as a flag. It is now a separate constructor selected by an ExistingObject discriminator, so nothing passes shouldCreate: false; the call site reads Table.AdoptExisting(connection, name). Your objection to the bool still holds and shouldDrop remains deleted.

It also now has a genuine caller: the new TemporalTable primitive, which HiddenTargetColumn uses.

[cheenamalhotra]

Human comment: This is addressed now, please resolve if satisfactory.

[paulmedynski]

I would prefer these RAII helpers to all adopt idempotent Dispose() and just call that on any failures during construction. That keeps all of the unwinding in one place.

[cheenamalhotra]

Done in 8db1e6d. Dispose() now guards on a _disposed flag, and the constructor's catch calls Dispose() instead of duplicating the drop, so all unwinding goes through one path.

Worth noting the pre-emptive drop at the top of the constructor (clearing anything left by an earlier run) deliberately does not go through Dispose() — it shares the same no-throw TryDrop() helper but must not mark the object disposed, since it runs before the object exists.

[paulmedynski]

Can we log the data source and database so a human or agent can perform the drop manually?

[cheenamalhotra]

Added in 8db1e6d — the report now includes the data source and database:

Failed to drop Table '[foo]' on data source 'tcp:myserver', database 'mydb'; it may be orphaned there. ...

One wrinkle: I capture those before the reconnect attempt rather than reading them in the catch. By the time we get there the connection has already failed and been closed, and reading DataSource/Database off it can itself throw — which would have thrown away the very message that identifies the leak. There is a fallback string if even that fails.

[cheenamalhotra]

Human comment: Addressed feedback

[paulmedynski]

shouldCreate is unnecessary here - it it only called by WithName() which always specifies true.

[cheenamalhotra]

Removed in 8db1e6d, along with shouldDrop.

[paulmedynski]

Ths IF and DROP are not atomic, so the DROP can still fail.

As mentioned above on the declaration of DropObject(), I don't think any errors should escape.

This applies to all of these derived classes.

[cheenamalhotra]

Good catch — the check and the DROP are separate statements, so the guard is racy by construction.

I have treated it as an optimisation rather than a guarantee: it avoids the common case of a noisy failure, and the swallow in TryDrop() is what actually makes it safe. That is now written down in the DropObject() remarks so the next person does not mistake the IF for correctness.

I did not switch to DROP ... IF EXISTS since it needs SQL Server 2016+ and would not close the race anyway — the drop can still fail for reasons unrelated to existence.

[cheenamalhotra]

Human comment: TryDrop in parent type handles any failures well, so no further catching is required here.

[paulmedynski]

I don't think DropObject() should ever throw. All of this cleanup is best-effort. If the implementation wants to re-try on a new connection, that's fine, but I don't think any errors should ever escape. This is essentially a disposal helper.

[cheenamalhotra]

Agreed, and made explicit in 8db1e6d.

I kept the guarantee in the base class rather than pushing it onto every override, because an implementation genuinely cannot promise not to throw — the connection can die mid-DROP no matter how the statement is written. So TryDrop() now owns it completely: it swallows the first attempt, swallows the reconnect retry, and swallows a failure in the reporting path itself. Nothing from DropObject() can reach a caller.

I also found a real hole while doing this. TryDrop() previously called TryDropAfterReconnect() from inside its catch without guarding it, and that method touched Connection — which, on the path where it is called, has just failed. A throw there would have escaped Dispose() and replaced whatever exception the test was already unwinding with, i.e. exactly the failure mode this PR exists to remove. Now guarded, and the connection details are captured before the reconnect attempt for the same reason.

The DropObject() doc comment now states the narrowed contract: be idempotent and tolerate the object not existing; everything else is the base class's problem.

[cheenamalhotra]

Human comment: Addressed.

[paulmedynski]

shouldCreate is not necessary.

[cheenamalhotra]

Removed in 8db1e6d, along with shouldDrop.

[paulmedynski]

shouldCreate is not necessary.

[cheenamalhotra]

Removed in 8db1e6d, along with shouldDrop.

[paulmedynski]

This is not used - please remove it.

[cheenamalhotra]

Both flags are gone in 8db1e6d. With AdoptExisting() removed, shouldCreate: false had no remaining caller and shouldDrop was true at literally every call site, so neither carried any information.

One thing that fell out of this: shouldCreate was quietly doubling as an overload discriminator. Table(connection, prefix, definition) and the private Table(connection, name, definition) have identical signatures — one treats the string as a prefix to generate a name from, the other takes it verbatim — so dropping the flag made them ambiguous. Schema already had a private NameIsVerbatim enum solving exactly this, so I promoted it to a shared internal enum and used it in Table, ScalarFunction and StoredProcedure. That keeps the distinction explicit instead of reintroducing a bool that means something else.

[cheenamalhotra]

Partially revised, flagging it here since it touches your point directly.

shouldDrop is still gone. shouldCreate had to come back in substance — see @edwardneal's note about temporal history tables — but not as a bool: it is now a separate adopting constructor behind an ExistingObject discriminator, so no call site passes an opaque flag. Details in 36a765c.

[Copilot]

Pull request overview

Copilot reviewed 76 out of 76 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlNotificationTest/SqlNotificationTest.cs:344

• Cleanup currently calls RunSQL(statement) in a loop; RunSQL opens a new SqlConnection each call and also creates a SqlCommand without disposing it. This increases connection churn and can leave command objects undisposed during cleanup. Consider using a single connection/command and executing each cleanup statement inside its own try/catch so cleanup stays best-effort without repeatedly allocating resources.

[Copilot]

Pull request overview

Copilot reviewed 82 out of 82 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 84 out of 84 changed files in this pull request and generated no new comments.