connectors: source stop and restart bookkeeping loses errors and ids
- Dominant language
- Rust
- Stars
- 4.9k
- Forks
- 432
- Avg merge
- 2d 10h
- Merged PRs (30d)
- 173
Description
## What happens
Four findings in the source manager's stop and restart paths. They are filed together because they
share one purpose, correct lifecycle bookkeeping in
`core/connectors/runtime/src/manager/source.rs`, and one PR can carry that purpose without becoming
a mixed PR.
### 1. `stop_connector` never clears `details.info.id`
It closes the instance named by `details.info.id` and leaves the field pointing at it. After a
failed start, every later stop re-closes an id the plugin has already dropped and logs `Closed` for
it. The runtime's own view of which instance exists is wrong from then on.
### 2. The stop path drops the `iggy_source_close` result
```rust
(container.iggy_source_close)(plugin_id);
info!("Closed source connector with ID: {plugin_id} for plugin: {key}");
```
A `-1` means the plugin refused the close, so teardown did not happen, and the log says it did. By
the time anyone could notice, the `INSTANCES` entry is gone on the plugin side, so nothing can
retry. Note the boot path and the #4064 guard both report a refusal through
`close_plugin_instance`; this call site was deliberately left alone in #4064 because fixing it there
would have been a second bug fix in that PR.
### 3. A failed restart reports no error
```rust
self.start_connector(key, &config, iggy_client, metrics, context)
.await?;
```
The `?` propagates to the HTTP caller, but the connector is left `Stopped` with `last_error` cleared
by the preceding stop. `GET /sources` then shows a stopped connector and no reason, which is the
state an operator is most likely to look at after a restart fails. One `set_error` on that arm
covers all the fallible steps inside `start_connector`.
### 4. `SOURCE_SENDERS` is a process global
`core/connectors/runtime/src/source.rs` keeps the forwarding channels in a
`LazyLock>`. Nothing owns an entry, so nothing drops one, and an
orphaned forwarding loop can only be cleaned up by an explicit `cleanup_sender` on a path that knows
the id.
The sink side does not have this: it owns its `watch::Sender` in `SinkDetails`, so its equivalent
window self-heals when the details are dropped. An RAII registration stored next to `handler_tasks`
would give the source path the same property.
#4064 closed the cancellation window that made this reachable during a restart, by moving the id
record under the same lock as the spawn. The ownership problem underneath it is unchanged.
## Why together
1, 2 and 3 are all `stop_connector` / `restart_connector` bookkeeping. 4 is what makes a leak on
that path unrecoverable rather than merely untidy. Splitting them would mean four PRs touching the
same two functions in sequence.
Happy to split if a maintainer would rather review them separately.
## Provenance
All four raised by @hubcio in review on #4064, out of that PR's diff and explicitly non-blocking for
it. His own triage note grouped them the same way. Filed so they do not disappear when #4064 merges.
### Contribution
- [x] I'm willing to submit a pull request to fix this bug
Contributor guide
Assessment
This issue has not been assessed yet.