Add exponential backoff to cloud reconnect (LTS-4.3)

[MazurenkoNick]

Pull Request description

Put your PR description here instead of this sentence.

General checklist

• You have reviewed the guidelines document.
• Labels that classify your pull request have been added.
• The milestone is specified and corresponds to fix version.
• Description references specific issue.
• Description contains human-readable scope of changes.
• Description contains brief notes about what needs to be added to the documentation.
• No merge conflicts, commented blocks of code, code formatting issues.
• Changes are backward compatible or upgrade script is provided.
• Similar PR is opened for PE version to simplify merge. Crosslinks between PRs added. Required for internal contributors only.

Front-End feature checklist

• Screenshots with affected component(s) are added. The best option is to provide 2 screens: before and after changes;
• If you change the widget or other API, ensure it is backward-compatible or upgrade script is present.
• Ensure new API is documented here

Back-End feature checklist

• Added corresponding unit and/or integration test(s). Provide written explanation in the PR description if you have failed to add tests.
• If new dependency was added: the dependency tree is checked for conflicts.
• If new service was added: the service is marked with corresponding @TbCoreComponent, @TbRuleEngineComponent, @TbTransportComponent, etc.
• If new REST API was added: the RestClient.java was updated, issue for Python REST client is created.
• If new yml property was added: make sure a description is added (above or near the property).

[MazurenkoNick]

Review summary

Reviewed 4 changed files in Add exponential backoff to cloud reconnect (LTS-4.3). Left 14 comment(s) inline.

The backoff mechanics and the lock around the reconnect state look sound, and the tests drive the loop deterministically. The two things I'd want resolved before merge are the new cloud.reconnect_max_timeout placeholder having no inline default (deb/rpm mark tb-edge.yml as CONFIG | NOREPLACE, so an upgraded install keeps its old yml and the context will fail to start), and the non-atomic teardown in destroy(), which can leave the reconnect loop permanently disarmed on the partition-reassignment path. The rest are quality/design points: no jitter on the backoff, the initial-connect retry loop still has no backoff at all, redundant loop state, and a few test-coverage gaps.

---

This review was auto-generated. Findings may contain errors — please verify before applying changes.

[MazurenkoNick]

The teardown here isn't atomic with respect to reconnectLock: cancelReconnect() takes the lock and releases it, then reconnectExecutor.shutdownNow() / reconnectExecutor = null (lines 112-115) run outside it. A gRPC onError landing in that window calls scheduleReconnect, sees reconnectFuture == null, sets reconnecting = true and schedules a fresh attempt on the not-yet-shutdown executor. shutdownNow() then kills the task, but reconnecting stays true and reconnectFuture is left pointing at a dead future.

That matters because destroy() isn't only the JVM-shutdown path — onDestroy() calls it when the system tenant partition is no longer ours, and a later PartitionChangeEvent revives the manager through establishRpcConnection(). After a revival, scheduleReconnect's reconnectFuture == null guard is permanently false, so the reconnect loop never runs again for the lifetime of the process. Moving the executor shutdown inside reconnectLock (or shutting it down before cancelling) would close the window.

[MazurenkoNick]

reconnectExecutor is a plain non-volatile field written on the connect-executor thread in launchCloudEventsProcessing() and nulled in destroy(), but read here from gRPC callback and reconnect threads. There's no happens-before edge between that write and this read, so this thread can legitimately observe a stale null.

The old code would have NPE'd loudly in that case; now the method just returns, leaving reconnecting == true with nothing scheduled and no log line — the Edge silently stops trying to reconnect until some later onError happens to re-enter scheduleReconnect. Marking the field volatile (or assigning it under reconnectLock) would fix the visibility, and a log.warn on the bail-out would make the "no reconnect scheduled" state diagnosable if it ever does happen.

[MazurenkoNick]

There are now two representations of "a reconnect loop is running": the reconnecting flag and reconnectFuture != null. scheduleReconnect gates on the future being null, scheduleReconnectAttempt gates on the flag, and cancelReconnect has to reset both. Two fields that must be kept in lockstep is the kind of thing the next person gets subtly wrong (see the destroy() comment for one way they can already drift apart). Could reconnecting alone be the single source of truth, with reconnectFuture kept purely as the cancellation handle — set unconditionally, never read as state? That would also make the guard in each method read the same way.

[MazurenkoNick]

This only covers the post-connection reconnect path. The initial-connect path is a separate retry loop — connectToServerAndLaunchEventsProcessing catches a failure and reschedules establishRpcConnection at getReconnectTimeoutMs(), which in turn schedules itself at getReconnectTimeoutMs() — and it still hammers the cloud every 3s forever with no backoff. So an Edge that has never managed a first successful connect (bad host, cloud down at boot) behaves exactly as badly as before, which is arguably the more common version of the reported symptom. Worth either extending the backoff to that loop too, or at minimum calling out in the yml comment that the cap applies only to reconnects after a successful connect, so the next reader doesn't assume reconnect_max_timeout bounds all retries.

[MazurenkoNick]

Three parallel lists that must stay index-aligned, with runScheduledTask(int index) reaching into one of them by position. A single List<ScheduledAttempt> holding a small record (task, delayMs, future) would keep the three facts of one scheduling event together and make it impossible for them to drift; the delay assertions could then read assertThat(attempts).map(ScheduledAttempt::delayMs).containsExactly(...). Minor, but this fixture will likely grow as more cases are added.

[MazurenkoNick]

This test and destroyCancelsReconnectLoop both declare throws InterruptedException, but nothing in either body can throw it — everything runs synchronously on the test thread through the mocked executor. Worth dropping so a future reader doesn't go looking for the blocking call that isn't there.

[MazurenkoNick]

One meaningful new path isn't covered here: the loop's survival when connect/disconnect throws. Under the old scheduleAtFixedRate the next tick fired regardless of what the task did; now continuation depends entirely on the self-reschedule after those catch blocks, so "a throwing attempt still schedules the next one with a doubled delay" is genuinely new behaviour — and it's exactly the scenario this PR targets (an Edge failing under native/heap pressure). Stubbing edgeRpcClient.connect(...) to throw and asserting the delay sequence still progresses would lock that in. Same for the executor == null guard in scheduleReconnectAttempt, which is unreachable in every current test since the executor is always injected.

[MazurenkoNick]

The comment above names onEdgeUpdate as the primary teardown path — it's the only thing that stops the loop in the happy case — but no test goes through it; the tests reach cancelReconnect directly by reflection instead. The destroy path does get a real test, so one of the two parallel entry points is covered and the more important one isn't. Driving onEdgeUpdate with an EdgeConfiguration whose cloudType is "CE" would cover the loop actually terminating on a successful reconnect, which is the behaviour users care about most.

[MazurenkoNick]

These assertions read private fields, so the test is pinned to the current internal representation of "stopped" — if the redundant reconnecting/reconnectFuture pair is ever collapsed into one flag, these tests break even though behaviour is unchanged. The observable contract is "the pending future was cancelled and nothing new gets scheduled": verify(future).cancel(true) plus a verifyNoMoreInteractions(reconnectExecutor) after cancelling would assert the same thing without freezing the field names.

[MazurenkoNick]

Re-review summary

Re-reviewed Add exponential backoff to cloud reconnect (LTS-4.3) — verified 14 finding(s) from previous review.

Status	Count
✅ Resolved	2
💬 Acknowledged	0
❌ Unresolved	12

Also found 12 new issue(s) in the fix commits, commented inline.

The two resolved findings were the two genuine correctness bugs, and both fixes look right. The 12 unresolved ones received no code change and no reply, so they are re-raised as-is.

Finding details

• ✅ BaseGrpcClientManager.java:98 — destroy() teardown was not atomic with respect to reconnectLock — Fixed in code: the new shutdownReconnect() cancels the future and disposes the executor inside a single underReconnectLock block, and it now runs before edgeRpcClient.disconnect(false). destroyShutsDownReconnectExecutorBeforeDisconnecting pins the ordering with InOrder.
• ✅ BaseGrpcClientManager.java:201 — reconnectExecutor was read from gRPC/reconnect threads without a happens-before edge — Fixed in code: every read and write of the field now happens under reconnectLock, so volatile is no longer needed.
• ❌ EdgeInfoHolder.java:40 — @Value placeholder has no inline default, breaking in-place package upgrades; the two coupled properties are still unvalidated — Still present, not addressed.
• ❌ BaseGrpcClientManager.java:82 — redundant representations of "a reconnect loop is running" — Still present, and now three fields rather than two.
• ❌ BaseGrpcClientManager.java:180 — the initial-connect retry loop still has no backoff — Still present, not addressed.
• ❌ BaseGrpcClientManager.java:200 — lock precondition documented only in a comment — Still present, not addressed.
• ❌ BaseGrpcClientManager.java:205 — reconnect lambda mixes three abstraction levels — Still present, not addressed.
• ❌ BaseGrpcClientManager.java:206 — original exception re-logged with a full stack trace on every attempt; attempt number and delay never logged — Still present, not addressed.
• ❌ BaseGrpcClientManager.java:226 — no jitter, so a fleet retries in synchronized waves — Still present, not addressed.
• ❌ BaseGrpcClientManagerTest.java:88 — three parallel index-aligned lists in the fixture — Still present, not addressed.
• ❌ BaseGrpcClientManagerTest.java:113 — unnecessary throws InterruptedException — Still present, and a third occurrence was added.
• ❌ BaseGrpcClientManagerTest.java:123 — no coverage for a throwing attempt still rescheduling — Still present; the executor == null guard is now partly covered.
• ❌ BaseGrpcClientManagerTest.java:144 — the onEdgeUpdate teardown path is still untested — Still present, not addressed.
• ❌ BaseGrpcClientManagerTest.java:251 — assertions read private fields, pinning tests to internal representation — Still present, and the new tests added more reflection.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[MazurenkoNick]

shutdownUplinkExecutor() now nulls the field, and init() calls it before creating the replacement — so uplinkExecutor transiently goes null then new on the connect-executor thread. processMsgPack dereferences it (uplinkExecutor.submit(...), line 131) from the uplink runner thread with no null check and no synchronization, and the field isn't volatile.

That window is reachable on every reconnect, not just at shutdown. CloudEventProcessingLifecycleManager.handleConnectionEvent calls init() on every GrpcConnectionEstablishedEvent, but the runner is only stopped by StopCloudEventProcessingEvent — so after a dropped connection, onEdgeUpdate can set initialized = true (making the runner eligible to send again) before launchCloudEventsProcessing() publishes the event that re-enters init(). The runner can then be inside sendCloudEvents while this line nulls the field: an NPE if it reads the null, or a RejectedExecutionException if it reads the old reference just after shutdownNow(). In KafkaCloudEventUplinkProcessingRunner both surface as "Failed to process all uplink messages" with an uncommitted batch, so the symptom is a confusing stack trace plus a redelivered pack.

Capturing into a local and null-checking in processMsgPack, plus making the field volatile (or guarding it the way reconnectExecutor now is), would close it. Worth noting the previous review flagged this exact shape for reconnectExecutor — that one is now fixed by locking, and this change reintroduces it one class over.

[MazurenkoNick]

shutdownExecutor and reconnectExecutor are both disposed in destroy() now, but connectExecutor is still never shut down and connectFuture is never cancelled. ThingsBoardThreadFactory calls setDaemon(false), so after @PreDestroy the cloud-manager-connect thread stays alive and can hold up JVM exit; a connectFuture still pending at that moment also fires afterwards and re-enters connectToServerAndLaunchEventsProcessing, which would build a fresh reconnect executor after destroy has run.

Given this commit is specifically about executor lifecycle and thread leaks, connectExecutor looks like the one that got missed — or is leaving it running deliberate, because establishRpcConnection reuses it across partition flaps? If so, a note saying that would help, since the other three executors are now all explicitly disposed.

[MazurenkoNick]

Still unresolved: this placeholder still has no inline default. packaging/java/build.gradle declares configurationFile("${pkgInstallFolder}/conf/${pkgName}.yml") with fileType CONFIG | NOREPLACE, so a deb/rpm upgrade leaves the operator's existing tb-edge.yml in place and the new one lands as .dpkg-dist/.rpmnew. That existing file has no cloud.reconnect_max_timeout key at all, so Spring fails with Could not resolve placeholder 'cloud.reconnect_max_timeout' and tb-edge won't boot. The ${CLOUD_RECONNECT_MAX_TIMEOUT:180000} default in the shipped yml doesn't help, because the whole key is absent from the file actually being read. @Value("${cloud.reconnect_max_timeout:180000}") would keep in-place upgrades working — cloud.reconnect_timeout gets away with this style only because it has shipped in every prior yml.

The yml comments do now describe how the two properties relate, which helps. Still nothing validates the relationship though: setting reconnect_max_timeout below reconnect_timeout makes the Math.min shrink the delay below the configured initial value from the second attempt on, and 0 or a negative degrades into a tight retry loop — the opposite of this PR's purpose, with no log line to explain it. A @PostConstruct clamp to at least reconnectTimeoutMs plus a warning would make the pair hard to misconfigure.

[MazurenkoNick]

Still unresolved, and now wider: the latest commit adds reconnectExecutor to the set of fields that must stay in lockstep, so there are three representations of "a reconnect loop is running" rather than two. scheduleReconnect gates on reconnectFuture == null && reconnectExecutor != null, scheduleReconnectAttempt gates on !reconnecting || executor == null, cancelReconnect resets two of them and shutdownReconnect resets all three. Each guard reads differently, which is what makes this easy to get subtly wrong later.

Could reconnecting be the single source of truth, with reconnectFuture kept purely as a cancellation handle — assigned unconditionally, never read as state?

[MazurenkoNick]

Still unresolved: the initial-connect path continues to hammer the cloud at a flat interval. This reschedules establishRpcConnection at getReconnectTimeoutMs(), and establishRpcConnection schedules its own task at getReconnectTimeoutMs() too — 3s forever, with no growth. So an Edge that has never completed a first successful connect (wrong host, cloud down at boot) behaves exactly as it did before this PR, which is arguably the more common form of the reported symptom.

Worth either extending the backoff to this loop, or at minimum saying in the yml comment that reconnect_max_timeout bounds only reconnects after a successful connect, so the next reader doesn't assume it caps all retries.

[MazurenkoNick]

Since this method always nulls the field afterwards, uplinkExecutor can only ever be non-null when it's a freshly created, running executor — so !uplinkExecutor.isShutdown() can never evaluate to false, and it isn't (and can't be) covered by a test. shutdownNow() is idempotent anyway, so a plain null check would say the same thing with one less condition to reason about.

[MazurenkoNick]

A method called validateRoutingKeyAndSecret() now returns false for two different reasons and doubles as the owner of a background logging executor, using the nullness of a field named shutdownExecutor as a hidden "have I already started complaining" flag. The field name is misleading to begin with — it never shuts anything down, it nags every 10s — and this change makes control flow depend on that name. Worth extracting the nagging into something like startMissingCredentialsReporter() with the idempotency guard inside it, so the validator stays a predicate and the flag's meaning is named?

[MazurenkoNick]

Both tests here only inspect the uplinkExecutor field, so nothing verifies the behaviour that actually matters after a re-init: that a subsequent sendCloudEvents(...) is still dispatched (i.e. grpcClientManager.sendUplinkMsg invoked) on the new executor. A test that drives one uplink pack through the sender, calls init() again, and drives a second pack would exercise the real flow and would survive a change in how the executor is stored — worth adding, given "re-init after every reconnect" is the scenario this fix targets.

[MazurenkoNick]

TIMEOUT_MS sits next to INITIAL_TIMEOUT_MS and MAX_TIMEOUT_MS, which are reconnect-backoff values, but this one is a Mockito verification wait — the name invites confusion. AWAIT_TIMEOUT_MS or VERIFY_TIMEOUT_MS would keep the three apart at a glance.

[MazurenkoNick]

Executor cleanup is now scattered per test: a try/finally with destroy() + shutdownConnectExecutor() in one, a bare first.shutdownNow() at the end of another, and nothing in the rest. That's easy to get wrong when the next test is added, and a test that fails before reaching its manual cleanup leaks real non-daemon threads into the remainder of the run. An @AfterEach that invokes destroy() and defensively shuts down whatever executors the manager still holds would cover every test uniformly and let the bodies stay focused on their assertions.

[MazurenkoNick]

Re-review summary

Re-reviewed Add exponential backoff to cloud reconnect (LTS-4.3) — verified 24 finding(s) from previous review.

Status	Count
✅ Resolved	2
💬 Acknowledged	0
❌ Unresolved	22

Also found 9 new issue(s) in the fix commits, commented inline.

The two correctness findings from the last round are both properly fixed. However, the uplink fix introduces a worse failure mode than the one it removes — completing the pack as interrupted turns AbstractPostgresCloudEventUplinkBatchDispatcher's inner loop into an unbounded Postgres-query spin on every stop/partition-move. Details on GrpcCloudEventUplinkSender.java:149; that one is worth acting on before merge.

The 22 unresolved findings received no code change and no reply, so they are re-raised unchanged — see the previous review for the full rationale on each.

Finding details

• ✅ GrpcCloudEventUplinkSender.java:133 — uplinkExecutor dereferenced with no null check while replaced from other threads — Fixed in code: field is now volatile, read once into a local, null-checked, and RejectedExecutionException is caught. See the new issue on line 149 about the chosen recovery behaviour.
• ✅ BaseGrpcClientManager.java:100 — connectExecutor never disposed in destroy(), non-daemon thread outliving the context, queued retry firing after destroy — Fixed in code: shutdownConnect() cancels the future and shuts the executor down, and the executor is configured to discard pending delayed tasks.
• ❌ EdgeInfoHolder.java:40 — @Value placeholder has no inline default, breaking in-place package upgrades; the two coupled properties are still unvalidated — Still present, not addressed.
• ❌ BaseGrpcClientManager.java:83 — redundant representations of "a reconnect loop is running" — Still present, not addressed.
• ❌ BaseGrpcClientManager.java:185 — the initial-connect retry loop still has no backoff — Still present, not addressed.
• ❌ BaseGrpcClientManager.java:204 — lock precondition documented only in a comment — Still present, not addressed.
• ❌ BaseGrpcClientManager.java:210 — reconnect lambda mixes three abstraction levels — Still present, not addressed.
• ❌ BaseGrpcClientManager.java:211 — original exception re-logged with a full stack trace on every attempt; attempt number and delay never logged — Still present, not addressed.
• ❌ BaseGrpcClientManager.java:231 — no jitter, so a fleet retries in synchronized waves — Still present, not addressed.
• ❌ BaseGrpcClientManager.java:263 — shutdownReconnect() duplicates cancelReconnect()'s body — Still present, not addressed.
• ❌ BaseGrpcClientManager.java:174 — assignment expression inside a lock-helper lambda — Still present, not addressed.
• ❌ BaseGrpcClientManager.java:175 — two executors with opposite lifecycle policies — Still present, though narrowed: connectExecutor is now disposed on destroy.
• ❌ BaseGrpcClientManager.java:184 — catch-branch shutdownReconnect() has no test — Still present, not addressed.
• ❌ BaseGrpcClientManager.java:381 — validateRoutingKeyAndSecret() doubles as owner of a logging executor via a misleadingly named field — Still present, not addressed.
• ❌ GrpcCloudEventUplinkSender.java:68 — re-entrant init() diverges from the create-if-absent convention the sibling runners use — Still present, not addressed.
• ❌ GrpcCloudEventUplinkSender.java:78 — !uplinkExecutor.isShutdown() is a condition that can never be false — Still present, not addressed.
• ❌ BaseGrpcClientManagerTest.java:88 — three parallel index-aligned lists in the fixture — Still present, not addressed.
• ❌ BaseGrpcClientManagerTest.java:113 — unnecessary throws InterruptedException (three occurrences) — Still present, not addressed.
• ❌ BaseGrpcClientManagerTest.java:123 — no coverage for a throwing attempt still rescheduling — Still present, not addressed.
• ❌ BaseGrpcClientManagerTest.java:144 — the onEdgeUpdate teardown path is still untested — Still present, not addressed.
• ❌ BaseGrpcClientManagerTest.java:291 — assertions read private fields, pinning tests to internal representation — Still present; the new tests added more reflection.
• ❌ BaseGrpcClientManagerTest.java:60 — TIMEOUT_MS name doesn't say it's a Mockito verification wait — Still present, and now overloaded further; see the new comment on line 252.
• ❌ BaseGrpcClientManagerTest.java:275 — executor cleanup scattered per test instead of an @AfterEach — Still present, not addressed.
• ❌ GrpcCloudEventUplinkSenderTest.java:75 — nothing verifies a pack is actually dispatched on the new executor after re-init — Still present; the new tests exercise only the two failure paths.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[MazurenkoNick]

Completing the pack as interrupted here trades the old NPE for a worse failure mode on the Postgres path — an unbounded busy loop.

AbstractPostgresCloudEventUplinkBatchDispatcher.processUplinkMessages loops on } while (isInterrupted || cloudEvents.hasNext()); (line 90). That inner loop has no interrupt check, no sleep, and never consults edgeInfo.isInitialized() — those guards live only in the caller's outer loop (PostgresCloudEventUplinkProcessingRunner lines 94/96). So once uplinkExecutor is null, every iteration issues a fresh finder.find(...) page query, gets true back from here, and immediately loops again. interruptPreviousSendUplinkMsgsTask()'s get(10, SECONDS) returns instantly because the previous future is already completed, so nothing throttles it.

That state is reachable on every stop, not just JVM exit: CloudEventProcessingLifecycleManager.handleStopEvent calls runner.shutdown() first and sender.shutdown() second, and runner.shutdown()'s shutdownNow() interrupt doesn't break the inner loop. The thread is left spinning against Postgres in a still-running process after a partition moves away.

Before this change the unguarded submit threw NPE, which processUplinkMessages' own catch (Exception) caught — returning to the outer loop where isInitialized() is false, settling into a benign sleep(1) poll.

Throwing instead of completing would keep that control flow while still removing the NPE: the dispatcher's existing catch (Exception) exits the loop, the Kafka runner's catch already logs and declines to commit, and nothing can hang because the future is never returned to a caller when processMsgPack throws. Alternatively, add an isInitialized()/interrupt check to the dispatcher's loop condition — but that's a change in a second file, so throwing here is the smaller fix.

[MazurenkoNick]

This class now has three different synchronization disciplines for its executor/future state, and nothing says which applies where: the reconnect* fields are documented as guarded by reconnectLock with underReconnectLock as the enforcement point, connectExecutor is volatile-only, and shutdownExecutor is unsynchronized. All three are touched from the partition-event thread, the connect thread and gRPC callback threads.

The asymmetry is sharpest within the connect pair: connectExecutor was made volatile but its sibling connectFuture — written by establishRpcConnection on the partition-event thread and read by shutdownConnect() on the Spring shutdown thread — was left plain. So destroy() can still read a stale connectFuture and skip the cancel. In practice the setExecuteExistingDelayedTasksAfterShutdownPolicy(false) policy covers for it (the pending task is discarded either way), which is presumably why it doesn't show up — but that makes the fix depend on the policy for a second, undocumented reason.

Since underReconnectLock already exists as a template, it'd be cheap either to bring the connect state under a similarly named helper, or to add a field-level comment stating that connect state is intentionally publish-only via volatile and why that's sufficient. Mixed disciplines in one class tend to converge on the weakest one.

[MazurenkoNick]

The nullable RejectedExecutionException parameter exists only to be handed to the logger, and one of the two call sites passes a bare null, which reads oddly and invites a third caller that forgets what the second argument is for. Either an overload rejectMsgPack(List<UplinkMsg>) that delegates, or replacing the exception with a String reason, would be clearer.

The String reason option has a side benefit: both paths currently emit the identical "Uplink executor is unavailable" line, so the logs can't distinguish "the field was already cleared by shutdown" from "submission was rejected by a shut-down executor" — different operational stories.

[MazurenkoNick]

This is the only place in the repo that constructs a ScheduledThreadPoolExecutor directly instead of going through Executors.newSingleThreadScheduledExecutor, and the only reason it has to is this policy flag. But the two facts that make it load-bearing sit ~100 lines apart: the flag here, and the shutdown()-instead-of-shutdownNow() teardown in shutdownConnect(). The comment explains the flag but not that shutdownConnect() depends on it, so the plausible "cleanup" of collapsing these three lines back into the one-liner used for reconnectExecutor/shutdownExecutor would silently reintroduce the defect.

Worth pulling into a small createConnectExecutor() factory whose comment names shutdownConnect() explicitly, or at least cross-referencing from the shutdownConnect() comment, so the coupling is visible from both ends?

[MazurenkoNick]

The three-line comment just above now reads as if it justifies both calls, but it's entirely about why the reconnect teardown must precede disconnect(false) — none of it applies to shutdownConnect(). As placed, a reader will assume the connect teardown carries the same ordering constraint; it doesn't. Moving this call above the comment, or giving it its own one-liner, keeps each comment attached to the statement it actually explains.

[MazurenkoNick]

Still unresolved: this comment names onEdgeUpdate as a teardown path — the only thing that stops the loop in the happy case — but no test goes through it; the tests reach cancelReconnect directly by reflection. The destroy path now has four tests, so the less important entry point is well covered and this one still isn't. Driving onEdgeUpdate with an EdgeConfiguration whose cloudType is "CE" would cover the loop terminating on a successful reconnect.

[MazurenkoNick]

Still unresolved, and wider again: these assertions read private fields, pinning the tests to the current internal representation of "stopped". The fix commits added more of the same — the suite now reaches into connectExecutor and connectFuture by name too, and several assertions are about field identity rather than behaviour. verify(future).cancel(true) plus verifyNoMoreInteractions(reconnectExecutor) would assert the same contract here. The root cause is that the class offers no seam: a package-private factory for the scheduled executors, or extracting the reconnect loop into an injectable collaborator, would let all of these be written against a fake.

[MazurenkoNick]

Still unresolved: TIMEOUT_MS sits beside INITIAL_TIMEOUT_MS and MAX_TIMEOUT_MS, which are reconnect-backoff values, but this one is a Mockito verification wait — the name invites confusion. AWAIT_TIMEOUT_MS or VERIFY_TIMEOUT_MS would keep the three apart. The fix commits made this worse by reusing it as a scheduling delay as well; see the comment on line 252.

[MazurenkoNick]

Still unresolved: executor cleanup is scattered per test — a try/finally with destroy() + shutdownConnectExecutor() in one, a bare first.shutdownNow() in another, and nothing in the rest, including the two tests added in the fix commits. A test that fails before reaching its manual cleanup leaks real non-daemon threads into the remainder of the run. An @AfterEach invoking destroy() and defensively shutting down whatever executors the manager still holds would cover every test uniformly.

[MazurenkoNick]

Still unresolved: nothing here verifies the behaviour that actually matters after a re-init — that a subsequent sendCloudEvents(...) is still dispatched (i.e. grpcClientManager.sendUplinkMsg invoked) on the new executor. The fix commits added two tests, but both cover rejection paths only. A test that drives one uplink pack through the sender, calls init() again, and drives a second pack would exercise the real flow and survive a change in how the executor is stored.