fix(connectors): close the source instance a failed start leaves behind - #4064
Conversation
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
|
/ready |
|
/request-review @hubcio |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4064 +/- ##
=============================================
- Coverage 86.71% 67.43% -19.28%
Complexity 1455 1455
=============================================
Files 1267 1267
Lines 212095 175633 -36462
Branches 177320 140858 -36462
=============================================
- Hits 183913 118445 -65468
- Misses 23705 52608 +28903
- Partials 4477 4580 +103
🚀 New features to boost your workflow:
|
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
a few things outside the diff, none of them blocking this PR:
core/connectors/sdk/src/source.rs:597-iggy_source_openinserts intoINSTANCESeven whenopen()failed, andSourceContainer::openstores the source before returning 1. a plugin whoseopen()bound a listener and then errored keeps it for the life of the process. same leak class as this PR, one statement earlier. the fix is to skip the insert and drop the container, not to close from the runtime side - the SDK already stored the source, so that would runSource::close()on an instance that never opened.core/connectors/runtime/src/source.rs:71-SOURCE_SENDERSbeing a process global is why an orphaned forwarding loop can't die. the sink owns itswatch::SenderinSinkDetails, so its identical window self-heals. an RAII registration stored next tohandler_taskswould fix the source side properly.core/connectors/runtime/src/manager/sink.rs:200-223- same unrecorded-instance window on the sink path,details.info.idonly set at 223. narrower than the source case, since the consume tasks exit when thewatch::Senderdrops, so the same guard alone is enough there.core/connectors/runtime/src/manager/source.rs:186-190-stop_connectornever clearsdetails.info.id, so after a failed start every later stop re-closes a dead id and line 168 logs "Closed" for it. the shutdown sweep hits this too.core/connectors/runtime/src/manager/source.rs:167- the stop path drops theiggy_source_closeresult and logs "Closed" unconditionally. a -1 there means teardown was skipped while theINSTANCESentry is already gone, so nothing can retry.core/connectors/runtime/src/manager/source.rs:311- a failed restart leaves the connector Stopped withlast_errorcleared, soGET /sourcesshows nothing wrong. oneset_erroronstart_connector(..).await?covers all five fallible steps.core/connectors/runtime/src/source.rs:384-427-setup_source_producerbuilds andinit()s a producer per configured stream but keeps only the last, so two configured streams silently produce to one.
github-actions
Bot
added
S-waiting-on-author
1d20ef5 to
e88133e
Compare
e88133e to
eed068f
Compare
|
All 18 inline findings are in, at
You were right that the fix was incomplete both ways. A1 is a bit worse than Rebased onto master twice since this was posted, so the shas above are the Five places I did not do exactly what you askedA3, the A2, both of your options rather than one. A4, reported rather than deleted. Dropping the status write outright leaves a C7, the C1, not the stop path. You noted the helper would also suit What the integration tests do and do not reachThey do reach more than I first credited them with. Two things are still not pinned by any test:
Everything else was mutation-checked, with each mutant confirmed to compile Out of diffFiled the multi-stream producer bug as #4097. |
|
/ready |
|
/request-review @hubcio |
github-actions
Bot
added
S-waiting-on-review
b3a4000 to
efb9ab0
Compare
github-actions
Bot
added
S-waiting-on-author
Closes apache#4062. `start_connector` allocates a fresh plugin id, calls `init_source`, and only records that id on `SourceDetails` once the handler tasks are spawned. In between, the instance exists inside the plugin and nothing outside it knows the id: `stop_connector` closes whatever `details.info.id` holds, which is still the previous instance. `setup_source_producer` returning early through `?` therefore stranded the new one for the life of the process, while the boot path in `source::init` cleaned up on the identical failure. For a plugin whose open only allocates, the orphan is wasted memory. For one that takes a process-global resource, it is a live fault: a shared listener stays bound and answering into a queue nothing drains, and every retried restart then fails on the identity the orphan never released. A guard rather than a cleanup branch at the one call that can fail today, because the window is defined by the two statements that open and record the instance, not by which call between them happens to be fallible. Adding a `?` inside it stays correct. The close-and-report itself is shared with the boot path so the two cannot drift.
The guard held a bare `extern "C" fn` read out of a dlopened `.so`. Nothing tied it to the `Container` that owns the mapping. It worked only because `container` is declared before the guard and therefore drops after it. A declaration order dependency is thin support for an FFI call into a loaded library, and it stops being enough at all once the call is deferred off the calling thread. `for_container` now captures the `Arc<Container<SourceApi>>` inside the closure the guard calls, so the mapping is owned for as long as the close is reachable. The field is `Arc<dyn Fn(u32) -> i32 + Send + Sync>` rather than an `Arc<Container<SourceApi>>`. A `Container` only exists via dlopen, so a guard carrying that field cannot be constructed in a unit test at all, and this guard needs more tests rather than fewer. The single production constructor takes the container, so the ownership stays enforced by the API and not by convention. `close_failed_source` now takes one callable shape. An `extern "C" fn` does not implement `Fn`, so the boot path wraps its pointer at the call site. The doc comment claimed the plugin id was "durably recorded". `SourceDetails` is memory only and the id never reaches the state store, so it now says recorded on `SourceDetails`.
`SourceContainer::close` drives the plugin's own `close()` under `block_on`, and `block_on(handle)` before it. Calling that from the guard's `drop` put an unbounded plugin teardown on a tokio worker, in the one place no timeout can ever be added, because drop glue cannot await. `drop` now hands the close to the blocking pool. The closure it carries owns the container, so the library stays mapped until the call returns. That is what makes deferring safe. Deferring costs the ordering between teardown and the returned error, so the known failure arm no longer relies on `drop`. `setup_source_producer`'s error arm awaits `close()`: off the worker and ordered, so by the time the error reaches an operator the instance is gone and a restart retried straight away cannot collide with it. `drop` stays the net for a cancellation and for any `?` added in the window later. `Handle::try_current` picks between them. A guard dropped outside a runtime has no worker to protect and nothing to hand the work to, so it closes inline. That is also the path the plain `#[test]` cases take. The deferred test asserts the close lands on a different thread from the one that dropped the guard, which is the only observable difference between handing it over and running it inline. The ordered test asserts exactly one call after `close()` returns, so the teardown was awaited and the following drop did not repeat it. `instance` became `instance_guard` while these lines were being rewritten. It holds a guard, and `instance.disarm()` read as disarming the instance.
The guard closed the plugin instance but could not reach the runtime half of the same leak. `spawn_source_handler` registers the `SOURCE_SENDERS` entry and spawns both tasks, and the id that reaches them was recorded only after the next `details.lock().await`. A cancellation in that gap, which is what a client disconnect dropping the axum handler future does, left the entry and both tasks behind with nothing naming them, so the forwarding loop ran for the life of the process. The lock is now taken before the spawn, so the spawn and the id record sit in one block with no await between them. `spawn_source_handler` is synchronous, so holding the lock across it costs a spawn and nothing else. The forwarding loop's own first act is to take the same lock, so it waits for the block to end instead of racing it. No test covers this. Reaching it means cancelling the future at one specific await with a dlopened container and a live broker in place, and both integration routes were already rejected for this PR for reasons that still hold. What enforces it is that the block contains no await, and an await added inside it would reopen the window silently.
|
/ready |
github-actions
Bot
added
S-waiting-on-review
|
/request-review @spetz |
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
warning: core/connectors/runtime/src/manager/sink.rs:207 - this pre-existing setup failure strands the opened sink. guard it, await cleanup on failure, and keep task spawning and id registration under the manager lock before disarming.
warning: core/connectors/runtime/src/manager/source.rs:162 - this pre-existing close call blocks an async worker. use spawn_blocking with the library owner, keeping the restart lock and remaining cleanup alive if the caller is cancelled.
warning: core/connectors/runtime/src/manager/source.rs:183 - this pre-existing race leaves the gauge elevated when an initial Running report follows Stopping and batch draining is aborted. apply the final Stopped through apply_status.
warning: core/connectors/runtime/src/sink.rs:737 - this pre-existing call ignores declared sink failures and skips runtime error accounting. propagate nonzero results before success accounting, and update sink status and gauge together when enabling that error path.
github-actions
Bot
added
S-waiting-on-author
`iggy_source_open` stored the container in the instance map whatever the open returned, and `SourceContainer::open` assigns the source before it looks at the result, so a failed open left a fully constructed instance behind. The runtime gets its error back before it has recorded the plugin id, so nothing outside the plugin can name that instance to close it, and it stays for the life of the process holding whatever the plugin took before it failed. The rollback is not registering it. Dropping the container releases the instance the same way any other failed construction is released, and the duplicate id guard above is untouched, so reopening the same id still refuses rather than silently replacing a live instance. Sinks had the same shape and get the same fix. Not covered by a test: the FFI entry points are `cfg(not(test))`, so a unit test cannot call them, and the map lives inside the plugin where the runtime cannot observe it. The change is small enough to read, which is the argument for it rather than around it.
…led runtime The wait for a gauge value reported its last read by issuing another request, and that request was the only one with no budget over it. It runs only after the wait has already timed out, which is precisely when the runtime is stalled, and the client had no timeout either, so the assertion that should have failed hung instead and reported nothing. The loop now carries the value it saw, and every request the file makes is built with the wait timeout on it. The three stats readers were also the same request and the same decode written out three times. They share one helper that hands back the `Result`, which is what keeps the retry loop treating a failed read as "not yet" while the two direct readers keep failing on it. The settle window was named for state storage and used for a gauge. Both waits are waiting on the same thing, a report that may land just after the poll before it, so the constant says that instead.
…wnership The guard carried a close callback and an `armed` flag that had to agree with it, so both teardown paths cloned the callback to leave the flag behind them. `Option<SourceClose>` is the same state said once: `disarm` clears it, the awaited close and `Drop` each take it, and neither clones. Awaited close still finishes before its caller returns, and `Drop` still hands the work to the blocking pool with the container captured so the library stays mapped. `record_started` is inlined at its one call site. It existed so the spawn was passed as a closure and the compiler would refuse an await between the spawn and the id record. That enforcement goes with it, so the requirement is written where the statements are: an await between them strands the `SOURCE_SENDERS` entry and both tasks with nothing naming them. Registration, the status transition and the disarm stay inside the one lock hold, in that order. Two tests go too. The callback's test tested the callback. The disarmed-guard drop is already covered by the success half of the fallible-step helper, which runs a guard through `disarm` and asserts nothing was closed.
`close_plugin_instance` took the word "source" or "sink" as a string its signature did not constrain, while `ConnectorType` already defines exactly those two labels. It takes the enum now, and `as_label` is visible in the crate rather than only to the encoder. The helper also sat between two connector struct declarations; it moves above them. The cleanup argument was written out across the close type, the guard type, its constructor, the awaited close, the `Drop` body and both call sites, mostly repeating itself. It is on the guard type now, once, and still says all of it: the window and why it is a guard rather than a branch per fallible call, that startup and restart both hand off through it, that the library must stay mapped for a deferred call, that `Drop` offloads to the blocking pool, and that awaited close and `Drop` are not interchangeable because only one gives the caller an ordering. The other sites point at it. A test comment recorded which person measured an interleaving and on which PR. It states the interleaving instead.
|
All 11 addressed and each one answered on its own thread. Four commits: the failed-open rollback, the stats-wait fixes, the guard and start window, and the naming and prose pass. Two things I did not decide on my own, both on their threads and repeated here so they are not missed:
The One thing beyond what you asked: 213 runtime and 165 SDK unit tests pass, and all 26 |
|
/ready |
github-actions
Bot
added
S-waiting-on-review
|
/request-review @hubcio |
3 participants
Closes #4062.
The leak
SourceManager::start_connectortakes a freshplugin_id, callsinit_source, and records that id onSourceDetailsonly after the handler tasks are spawned. In between, the instance exists inside the plugin and nothing outside it knows the id:stop_connectorcloses whateverdetails.info.idholds, which is still the previous instance.setup_source_producerreturning early through?therefore stranded the new one for the life of the process.source::initalready cleaned up on the identical failure, which is the asymmetry @hubcio pointed at.For a plugin whose open only allocates, the orphan is wasted memory. For one that takes a process-global resource it is a live fault: a shared listener stays bound and answering into a queue nothing drains, and every retried restart then fails on the identity the orphan never released.
A guard, not a cleanup branch
This deviates from the fix in the review, which was to mirror
source::init's error arm at the call site, so it is worth saying why rather than leaving it to be found.The window is defined by the two statements that open the instance and record its id, not by which call between them happens to be fallible today. A cleanup branch is correct only for the one
?that exists now, and silently wrong for the next one somebody adds.SourceInstanceGuardis armed atinit_sourceand disarmed once the id is recorded, so every path out of that window closes the instance, including a panic.It also made the behaviour testable.
Container<SourceApi>only comes fromdlopen, sostart_connectorcannot be exercised in a unit test at all, while a guard holding the bareextern "C" fncan be driven directly.The close-and-report itself is now one function shared with
source::init, so the two sites cannot drift.source::initkeeps its existing control flow; only the duplicated body moved.No
cleanup_senderon this path:spawn_source_handleris what registers the sender, and it has not run yet.Tests
Three, each mutation-checked, each mutant confirmed to compile first:
-1, the code the SDK returns for an unknown id) is reported and not propagated, because unwinding out ofdropwould be worse than the leak it is cleaning up afterEach test owns its stub and statics rather than sharing a pair, which would have made two of them race in the same process.
What is not covered, and why
The guard's placement in
start_connectorhas no test. I verified that rather than assuming it: disarming the guard immediately after construction restores the original leak, compiles, and the suite still passes.Reaching that path needs a real
Container, so it cannot be a unit test, and the only route intostart_connectorisPOST /sources/{key}/restart. Makingsetup_source_producerfail there means either a config the local provider will serve on restart but not at boot, which today works only because of the version selection in #3848 and would break when that is fixed, or stopping the broker mid-test. Both couple this regression test to something unrelated to it, so I left it out rather than write a test that fails for the wrong reason later. Happy to add either if you would rather have the coverage than the independence.source::init's cleanup remains covered only byerror_isolation.rsasserting the connector reportsError, which it did before this change too.Verification
cargo fmt,cargo sort --no-format, clippy at both feature sets, rustdoc under-D warnings, 198 unit tests iniggy-connectors, andstdout_sink+random_sourcestill build.