connectors: a failed source open still leaves an INSTANCES entry behind
- Dominant language
- Rust
- Stars
- 4.9k
- Forks
- 432
- Avg merge
- 2d 10h
- Merged PRs (30d)
- 173
Description
## What happens
`SourceContainer::open` in `core/connectors/sdk/src/source.rs` stores the source before it knows
whether opening worked, and returns a failure code afterwards:
```rust
let mut source = factory(id, config, state);
let runtime = get_runtime();
let result = runtime.block_on(source.open());
self.id = id;
self.source = Some(Arc::new(source)); // unconditional
if result.is_ok() { 0 } else { 1 }
```
The `source_connector!` macro then inserts into `INSTANCES` regardless of that code:
```rust
let result = instance.open(/* ... */);
INSTANCES.insert(id, container); // unconditional
result
```
So a plugin whose `open()` fails leaves a live `INSTANCES` entry under its `plugin_id`, holding the
constructed `Source`.
Nothing on the runtime side closes it. `init_source` returns `Err`, and the boot path records the
plugin with `error` set and moves on, while the restart path returns through `?`. The guard added in
#4064 is armed only after `init_source` succeeds, so it does not cover this case by design.
## Why it matters, and where it does not
For a plugin whose `open()` only allocates, this is wasted memory until the process exits.
It becomes a live fault for a plugin that acquires a process-global resource and can then fail
*after* acquiring it, since nothing will ever release it.
Worth stating plainly, because it came up while reviewing #3798: the HTTP source is **not** such a
plugin. Its `open()` calls `server::join` as the last fallible step and does nothing fallible after,
and every `join` failure path returns before the instance reaches the published route table. A
failed `open()` there leaves an inert object, not a joined listener. This issue is about the general
case.
## Fix
Skip the insert and drop the container when `open()` returned non-zero.
Not by closing from the runtime side: the SDK has already stored the source, so a close would run
`Source::close()` on an instance whose `open()` never completed, which plugins are not written to
expect.
## Provenance
Raised by @hubcio in review on #4064, out of that PR's diff and explicitly non-blocking for it, with
the fix above. Filed separately so it does 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.