apache / apache/iggy

connectors: a failed source open still leaves an INSTANCES entry behind

Open
#4,100 0 comments 0 reactions 1 assignee Claimed by @mlevkov View on GitHub
bug connectors rust
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.