mlevkov commented on PR #4064:
URL: https://github.com/apache/iggy/pull/4064#issuecomment-5593322797
All 18 inline findings are in, at `17e1df8a4`. Seven commits, one per finding
where that made sense. Gate green: fmt, sort, clippy at both feature sets,
rustdoc `-D warnings`, `--locked`, 211 unit tests (was 206), three clean
suite
runs (**212** unit tests after the two additions below), plus **64 connectors
integration tests** across `runtime`, `api`, `random`, `stdout`,
`http_config_provider` and `postgres`.
| batch | findings | commit |
| --- | --- | --- |
| A3 | close pointer tied to its library | `86fbb571e` |
| A2 and its follow-up | teardown off the worker | `2d9835229` |
| A1 | cancelled start leaking both tasks | `f7da2aa74` |
| A4 | the `sources_running` ratchet | `56cf99a91` |
| B1-B4 | the guard tests | `147e80d55` |
| C1, C5, C8 | one close for a plugin instance | `d46d501e2` |
| C2, C3, C4, C6 | naming, `must_use`, docs | `17e1df8a4` |
You were right that the fix was incomplete both ways. A1 is a bit worse than
"the tasks are left behind": dropping the `JoinHandle`s detaches them, so the
forwarding loop keeps running against a channel nothing feeds. The lock now
comes before the spawn, so nothing can await between registering the tasks
and
recording the id that reaches them.
### Five places I did not do exactly what you asked
**A3, the `Arc`.** Held as `Arc<dyn Fn(u32) -> i32 + Send + Sync>`, built by
a
`for_container` constructor that captures the `Arc<Container<SourceApi>>`,
rather than as an `Arc<Container<SourceApi>>` field. A `Container` only
exists
via dlopen, so a guard carrying that field cannot be built in a unit test at
all, and B1 and B2 ask for more guard tests, not fewer. The single production
constructor takes the container, so the ownership stays enforced by the API.
Same lifetime tie, still testable.
**A2, both of your options rather than one.** `drop` defers to the blocking
pool, and the error arms await an explicit `close()`. Deferring on its own
costs
the ordering you named, and that ordering matters here: a restart retried
inside
the teardown window would collide with the instance being torn down, which is
the failure #4062 exists to prevent. `drop` stays the net for a cancellation
and
for any `?` added in the window later.
**A4, reported rather than deleted.** Dropping the status write outright
leaves a
connector that just started answering `Stopped` to `GET /sources` until the
forwarding loop takes the lock. `start_connector` now reports through
`update_status`, so the gauge still has exactly one owner.
**C7, the `key` field.** The lifetime param and the `impl<'a>` are gone,
which
was the objection. `key` stays, as an owned `String`: now that the close can
be
deferred, its `warn!` is emitted outside the caller's log context, so the
key is
the only thing tying that line to a connector. Happy to drop it for
`plugin_id`
alone if you would rather.
**C1, not the stop path.** You noted the helper would also suit
`manager/source.rs:167`. That path drops the `iggy_source_close` result and
logs
"Closed" regardless, which is your own out-of-diff finding, so wiring it in
here
would fix a separate bug inside this PR.
### What the integration tests do and do not reach
They do reach more than I first credited them with.
`source_with_invalid_config`
in `error_isolation` fails inside `setup_source_producer`, which is the arm
the
guard now sits on in `init`, and `random_source_produces_messages` is what
would
break if the `disarm()` in the other arm were wrong. For the restart path,
`given_restart_when_state_exists_should_resume_from_served_state` and the two
CDC restart tests POST `/sources/{key}/restart` and then wait for the source
to
reload state and resume, so A1's restructure is exercised end to end.
Two things are still not pinned by any test:
- **A1's cancellation window.** Nothing can drop the `start_connector`
future at
that one await, so it still has no test. It is no longer held by a comment
either: `SourceDetails::record_started` takes the spawn as a closure and is
deliberately not `async`, so an await added between the spawn and the id
record does not compile. Checked by adding one, `error[E0728]`. Same
argument
you made for `#[must_use]`, applied to the window rather than the guard. It
also made the step testable, which nothing in that region was.
- **A4's arithmetic.** There is now a test for it, and it is worth telling
you
what it showed. `sources_running_does_not_climb_across_restarts` restarts
the
random source three times and requires the gauge back at 1. It passes, but
it
does not isolate the fix: revert the report through `update_status` and it
still passes 3 of 3, because taking the lock before the spawn already makes
`start_connector` win the race to that lock. Revert that too, so the code
is
what you reviewed, and it still passes 6 of 6. `start_connector` takes an
uncontended mutex immediately, while the spawned loop has to be scheduled
and
walk a DashMap to reach the same lock, so it does not get there first.
So the double count is real in the code and, as far as I can make it
behave,
latent. The fix stands on not wanting correctness to rest on that lock
ordering, not on a ratchet I could reproduce. Said plainly rather than let
the
commit imply I had seen it climb.
Everything else was mutation-checked, with each mutant confirmed to compile
first: closing inline instead of deferring, `close()` not awaiting (10
rounds of
10), `disarm()` moved ahead of the fallible step, a guard that never arms,
the
transition guard removed from `update_status`, and a bare
`SourceInstanceGuard::new(..);` statement, which `#[must_use]` now rejects at
build time.
### Out of diff
Filed the multi-stream producer bug as #4097. `setup_source_producer`
`init()`s a
producer per `[[streams]]` entry and keeps only the last, so two configured
streams silently produce to one. Both auto-create flags default on, so every
configured stream and topic really is created and all but the last stay
empty,
which is what makes it hard to spot. The other six are not filed yet.
--
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]
