mlevkov opened a new issue, #4102:
URL: https://github.com/apache/iggy/issues/4102
## 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<DashMap<u32, SourceSenderEntry>>`. 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
