[release/v26.0] [DEV-1850] Fix connector accounting when connectors stop on their own

[w1am]

Cherry picked from #5694

[linear-code]

DEV-1850

[qodo-code-review]

PR Summary by Qodo

Fix connector deactivation & metrics when connectors self-stop

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Ensure connector deactivation removes entries safely even when connectors stop/fault on their own.
• Make connector close metrics fire reliably, even when sink/source disposal throws.
• Add regression tests covering self-termination, idempotent deactivation, and active-connector
 metrics.

Diagram

graph TD
  A["ConnectorsActivator"] --> B[("Activated connectors")]
  B --> C["IConnector instance"]
  D["SystemConnectorsFactory"] --> E["Sink/Source wrapper"] --> F{{"Connector metrics"}}
  G["Tests"] --> A --> C
  G --> D --> F
  subgraph Legend
    direction LR
    _svc["Service/Component"] ~~~ _store[("State store") ] ~~~ _ext{{"Telemetry"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Decrement active metrics on Stopped completion

• ➕ Metrics would reflect connectors that terminate without explicit DisposeAsync
• ➕ Avoids coupling correctness to disposal paths and proxy shutdown behavior
• ➖ Requires reliable Stopped completion semantics across all connector implementations
• ➖ More complex to avoid double-counting when both Stopped and DisposeAsync occur

2. Centralize lifecycle in a dedicated ConnectorRegistry

• ➕ Single owner for add/remove/teardown, easier to reason about idempotency
• ➕ Could unify Activate/Deactivate/WaitForDeactivation paths
• ➖ Larger refactor and higher regression risk for a release-branch fix
• ➖ More code churn for a narrowly-scoped bug

Recommendation: Keep the PR’s approach: it is a minimal, release-safe fix that makes teardown idempotent (via conditional TryRemove) and guarantees close metrics via per-connector callbacks and try/finally disposal. The alternatives are viable longer-term but add complexity or require broader refactoring.

Files changed (4) +220 / -101

Bug fix (2) +78 / -35

SystemConnectorsFactory.csMake close metrics and disposal reliable per connector instance +44/-19

Make close metrics and disposal reliable per connector instance

• Removes a shared static dispose callback and replaces it with per-connector callbacks captured at creation time, preventing cross-connector metric misattribution. Wraps sink/source disposal in try/finally to ensure processors are disposed and close metrics are emitted even if proxy disposal throws.

src/Connectors/KurrentDB.Connectors/Infrastructure/Connect/Components/Connectors/SystemConnectorsFactory.cs

ConnectorsActivator.csFix teardown semantics when connectors stop/fault without explicit removal +34/-16

Fix teardown semantics when connectors stop/fault without explicit removal

• Changes Deactivate/WaitForDeactivation to look up connectors without immediately removing them, then tears down using a conditional TryRemove to ensure only one caller disposes. Treats faulted Stopped tasks as terminal, and adds a TryTeardown helper for best-effort cleanup during activation failures or revision replacement.

src/Connectors/KurrentDB.Connectors/Planes/Control/ConnectorsActivator.cs

Tests (2) +142 / -66

SystemConnectorsFactoryTests.csAdd metrics regression test for closed connector accounting +62/-0

Add metrics regression test for closed connector accounting

• Introduces an integration test that listens to the connectors meter and asserts only disposed connectors emit the close (negative) measurement. Validates per-connector close tracking when multiple connectors are running concurrently.

src/Connectors/KurrentDB.Connectors.Tests/Infrastructure/SystemConnectorsFactoryTests.cs

ConnectorsActivatorTests.csExpand activator tests for idempotent deactivation and self-termination +80/-66

Expand activator tests for idempotent deactivation and self-termination

• Refactors tests for clarity and adds coverage for: single-shot deactivation, deactivating connectors that self-stop, and waiting for deactivation when Stopped completes faulted. Updates the test connector to track dispose count and simulate self-termination reliably.

src/Connectors/KurrentDB.Connectors.Tests/Planes/Control/ConnectorsActivatorTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Stop faults are hidden ✗ Dismissed 🐞 Bug ◔ Observability

Description

ConnectorsActivator.Teardown swallows exceptions from connector.Instance.Stopped, so
Deactivate/WaitForDeactivation can return Deactivated even when the connector stop task
faulted (e.g., connector crash). This prevents the control service from surfacing/persisting the
underlying failure because it only records error details on DeactivateResult.Failure.

Code

src/Connectors/KurrentDB.Connectors/Planes/Control/ConnectorsActivator.cs[R95-98]

+            await connector.Instance.Stopped;
+        }
+        catch {
+            // a faulted stop is still a stop

Evidence

Teardown explicitly catches and ignores failures from Stopped, and Deactivate relies on
Teardown for its success path. The control service only persists error details on deactivation
failure, so swallowing stop faults makes connector crashes appear as successful deactivations.

src/Connectors/KurrentDB.Connectors/Planes/Control/ConnectorsActivator.cs[57-100]
src/Connectors/KurrentDB.Connectors/Planes/Control/ConnectorsControlService.cs[131-157]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ConnectorsActivator.Teardown` catches and ignores all exceptions from `connector.Instance.Stopped`. As a result, `Deactivate()` can return `Deactivated` even when the connector actually faulted while stopping, which suppresses crash diagnostics and prevents downstream error recording.

### Issue Context
- `Deactivate()` delegates to `Teardown()`, and returns `Deactivated()` when `Teardown()` completes without throwing.
- `Teardown()` currently treats a faulted `Stopped` task as success by swallowing exceptions.
- `ConnectorsControlService` only writes error details when `DeactivateResult.Failure` is true, so swallowed stop faults won’t be recorded.

### Fix Focus Areas
- src/Connectors/KurrentDB.Connectors/Planes/Control/ConnectorsActivator.cs[57-109]
- src/Connectors/KurrentDB.Connectors/Planes/Control/ConnectorsControlService.cs[131-157]

### Suggested fix
One of:
1) **Propagate stop faults to the result**: capture the exception from `Stopped` and have `Deactivate()` (and optionally `WaitForDeactivation()`) return `DeactivateResult.UnknownError(ex)` when `Stopped` faulted, while still ensuring dictionary removal and `DisposeAsync()` happens.

2) **Keep success but preserve diagnostics**: keep returning `Deactivated`, but attach the exception to the result (e.g., `new DeactivateResult(DeactivateResultType.Deactivated, ex)`), and/or explicitly log it, so operators can see that the connector stopped faulted.

Either approach preserves the accounting fix while avoiding silent loss of crash information.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[cloudflare-workers-and-pages]

Deploying eventstore with    Cloudflare Pages

Latest commit:	11ccff7
Status:	✅  Deploy successful!
Preview URL:	https://39e98c09.eventstore.pages.dev
Branch Preview URL:	https://cherry-pick-5694-w1am-dev-18-ua0z.eventstore.pages.dev

View logs