fix(lifecycle): harden packet admission and transport ownership - #6598
Open
jeremiah-k wants to merge 5 commits into
Open
fix(lifecycle): harden packet admission and transport ownership#6598jeremiah-k wants to merge 5 commits into
jeremiah-k wants to merge 5 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR introduces boolean transport admission and structured send outcomes, adds lifecycle and connection-epoch tracking, coordinates radio teardown, strengthens transactional settings and discovery restoration flows, and surfaces expected request failures through localized feedback. ChangesTransport and request resilience
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jeremiah-k
force-pushed
the
bugfix/command-transport-lifecycle
branch
from
August 8, 2026 23:10
ee1a32b to
6839180
Compare
jeremiah-k
marked this pull request as ready for review
August 8, 2026 23:29
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/ReplayFuzzTest.kt (1)
86-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStart the transport so this test still exercises the decode path.
handleSendToRadionow returnsfalseimmediately whenstarted.valueisfalse. This transport is never started, so the fuzzed bytes never reachToRadio.ADAPTER.decode. The test would still pass if therunCatchingtolerance inReplayRadioTransport.handleSendToRadiowere removed, so it no longer proves the behavior in its name. Callstart()first.💚 Proposed fix
fun `handleSendToRadio tolerates arbitrary outbound bytes`() = runTest { val transport = ReplayRadioTransport(Sink(), backgroundScope, address = "", frames = validAsset(), packetDelayMs = 0) + transport.start() ReplayFuzz.forSeeds { random, seed ->🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/ReplayFuzzTest.kt` around lines 86 - 98, Start the ReplayRadioTransport before the ReplayFuzz loop in handleSendToRadio tolerates arbitrary outbound bytes, ensuring started.value is true and both fuzzed inputs reach the decode path while preserving the existing assertions and scheduler handling.Source: Coding guidelines
🧹 Nitpick comments (7)
core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/TcpRadioTransport.kt (1)
96-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTie
OPERATION_TIMEOUTto the lifecycle drain bound.
close()relies on every admitted operation finishing withinTransportLifecycleGate.OPERATION_DRAIN_TIMEOUT(15 s). Line 160 bounds each operation atOPERATION_TIMEOUT(10 s), so the invariant holds today. The two values live in separate files, so a later increase ofOPERATION_TIMEOUTpast 15 s would silently makeclose()stop the transport while I/O is still in flight.Consider passing an explicit
operationDrainTimeoutderived fromOPERATION_TIMEOUTwhen constructing the gate, asBleRadioTransportalready does.♻️ Suggested change
- private val lifecycle = TransportLifecycleGate("TCP") + private val lifecycle = TransportLifecycleGate("TCP", operationDrainTimeout = OPERATION_TIMEOUT * 2)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/TcpRadioTransport.kt` around lines 96 - 99, Update TcpRadioTransport’s TransportLifecycleGate construction to pass an explicit operationDrainTimeout derived from OPERATION_TIMEOUT, matching the BleRadioTransport pattern. Ensure the gate’s drain bound remains synchronized with the per-operation timeout used in close()’s operation handling, without changing unrelated lifecycle behavior.core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/ReplayRadioTransport.kt (1)
181-190: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider logging instead of throwing when the lifecycle close times out.
close()throwsIllegalStateExceptionwhenlifecycle.closereturnsfalse.lifecycle.closereturnsfalsefor a drain timeout or a teardown timeout, which are bounded, non-fatal outcomes. A throw from a teardown API can abort the caller's remaining shutdown work.MockRadioTransportandReplayRadioTransportare exercised in the debug/replay path only, so the blast radius is small, but an error log keeps teardown total.♻️ Optional: report the timeout without failing teardown
override suspend fun close() { - check( - lifecycle.close { - handshakeQueue.close() - transportJob.cancelAndJoin() - }, - ) { - "Replay transport teardown did not complete within its lifecycle bounds" + val completed = + lifecycle.close { + handshakeQueue.close() + transportJob.cancelAndJoin() + } + if (!completed) { + Logger.e { "Replay transport teardown did not complete within its lifecycle bounds" } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/ReplayRadioTransport.kt` around lines 181 - 190, Update ReplayRadioTransport.close() to stop throwing when lifecycle.close returns false; instead, log the bounded teardown timeout and allow close() to complete so callers can continue shutdown work. Preserve the existing handshakeQueue.close(), transportJob.cancelAndJoin(), and lifecycle close behavior.feature/discovery/src/commonTest/kotlin/org/meshtastic/feature/discovery/DiscoveryPacketCollectionTest.kt (1)
337-372: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider folding this fake into
SharedInMemoryDiscoveryDao.This PR added
SharedInMemoryDiscoveryDaoin the same package and migratedDiscoveryHistoryBehaviorTestandDiscoveryMapFilterTestto it. This file keeps a second, near-identical fake, and both copies needed the same two new overrides in this PR. The only structural difference is thatInMemoryDiscoveryDaoexposessessions,presetResults, anddiscoveredNodesas public properties.Expose read-only accessors on
SharedInMemoryDiscoveryDaoand delete this copy. That removes the next divergence whenDiscoveryDaogains a method.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/discovery/src/commonTest/kotlin/org/meshtastic/feature/discovery/DiscoveryPacketCollectionTest.kt` around lines 337 - 372, Replace the local InMemoryDiscoveryDao in DiscoveryPacketCollectionTest with SharedInMemoryDiscoveryDao, adding read-only accessors there for sessions, presetResults, and discoveredNodes so existing test usage remains supported. Remove the duplicate fake and update any references to use the shared implementation.feature/discovery/src/commonTest/kotlin/org/meshtastic/feature/discovery/DiscoveryHomeRestorerTest.kt (1)
290-290: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the named settle-delay constant.
The sibling test at Line 242 advances by
DiscoveryHomeRestorer.POST_RESTORE_SETTLE_DELAY_MS. This line hard-codes the same duration. If the constant grows, the restore does not complete andresult.await()at Line 293 blocks until therunTesttimeout instead of failing clearly.♻️ Proposed change
- advanceTimeBy(3_000L) + advanceTimeBy(DiscoveryHomeRestorer.POST_RESTORE_SETTLE_DELAY_MS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/discovery/src/commonTest/kotlin/org/meshtastic/feature/discovery/DiscoveryHomeRestorerTest.kt` at line 290, Replace the hard-coded 3_000L delay in the affected test with DiscoveryHomeRestorer.POST_RESTORE_SETTLE_DELAY_MS, matching the sibling test and keeping the restore timing tied to the named production constant.core/service/src/commonMain/kotlin/org/meshtastic/core/service/AdminControllerImpl.kt (1)
270-336: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving
isLocalDestinationonce per transaction.
EditSettingsSessioncomputesisLocalDestinationat line 273.requireCommitBoundaryAcceptedrecomputes the same predicate at line 351 fromnodeManager.myNodeNum.value. If the local node identity changes between the block and the commit, the write-staging decision and the commit-departure decision disagree for the same transaction.Extract the value once in
editSettingsand pass it to both the session and the commit boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/service/src/commonMain/kotlin/org/meshtastic/core/service/AdminControllerImpl.kt` around lines 270 - 336, Derive the local-destination boolean once in editSettings and pass that captured value to both EditSettingsSession and requireCommitBoundaryAccepted. Remove the session-level recomputation from EditSettingsSession and update the commit-boundary call to use the same transaction-scoped value, preserving consistent behavior if nodeManager.myNodeNum changes during the transaction.core/database/src/commonMain/kotlin/org/meshtastic/core/database/entity/DiscoverySessionStatus.kt (1)
37-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBuild
RECOVERABLEfrom the constants instead of parsing the SQL literal.Line 38 recovers the status values by splitting
RECOVERABLE_SQL_LISTon", "and stripping quotes. The parse depends on the exact spacing of the SQL literal. A future edit that adds a newline, changes the separator spacing, or introduces a status value containing", "makes the derivedSetsilently disagree with the SQL filter. That is the drift the comment aims to prevent.Room requires
RECOVERABLE_SQL_LISTto stay aconst val, so it cannot be derived from aSet. Declare both from the same constants and add a test that asserts they agree.♻️ Proposed refactor
- /** In-memory representation derived from [RECOVERABLE_SQL_LIST] so fakes cannot drift from Room's filters. */ - val RECOVERABLE: Set<String> = RECOVERABLE_SQL_LIST.split(", ").mapTo(linkedSetOf()) { it.removeSurrounding("'") } + /** In-memory counterpart of [RECOVERABLE_SQL_LIST]. A unit test asserts both lists stay in sync. */ + val RECOVERABLE: Set<String> = + linkedSetOf(IN_PROGRESS, INTERRUPTED, RESTORE_PENDING_COMPLETE, RESTORE_PENDING_FAILED, RESTORE_PENDING_STOPPED)Add the guard test in
core/database/src/commonTest:`@Test` fun recoverableSetMatchesSqlList() { val fromSql = DiscoverySessionStatus.RECOVERABLE_SQL_LIST.split(",").map { it.trim().removeSurrounding("'") } assertEquals(fromSql.toSet(), DiscoverySessionStatus.RECOVERABLE) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/database/src/commonMain/kotlin/org/meshtastic/core/database/entity/DiscoverySessionStatus.kt` around lines 37 - 38, Update DiscoverySessionStatus.RECOVERABLE and RECOVERABLE_SQL_LIST to be declared independently from shared status constants rather than parsing the SQL literal; keep RECOVERABLE_SQL_LIST as a const val for Room. Add a common test named recoverableSetMatchesSqlList that trims comma-separated SQL values and asserts they equal DiscoverySessionStatus.RECOVERABLE.feature/discovery/src/commonMain/kotlin/org/meshtastic/feature/discovery/DiscoveryScanEngine.kt (1)
201-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the duplicated refusal blocks and the redundant null check.
Lines 205-210 and 213-218 repeat the same
mutex.withLock { if (!isActive) _scanState.value = Failed(...) }shape. Line 242 testsinitialLoraConfig != nullagain althoughrefusal == nullalready implies it, and line 252 then needsrefusal ?: "Scan cannot start"for a branch that cannot occur.Extract a small refusal helper and bind the non-null config to a local value so the unreachable fallback disappears.
♻️ Proposed refactor sketch
+ private suspend fun refuseScan(reason: String) { + mutex.withLock { if (!isActive) _scanState.value = DiscoveryScanState.Failed(reason) } + }Then replace each guard block with
refuseScan("..."), and inside the mutex use:val homeLora = initialLoraConfig when { homeLora == null -> _scanState.value = DiscoveryScanState.Failed("Home LoRa configuration is not available") initialPrimaryChannel == null && requiresPrimaryRestore -> _scanState.value = DiscoveryScanState.Failed("Primary channel is not available for a custom-channel scan") else -> prepareScanLocked(targets, dwellDurationSeconds, homeLora, initialPrimaryChannel, requiresPrimaryRestore) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/discovery/src/commonMain/kotlin/org/meshtastic/feature/discovery/DiscoveryScanEngine.kt` around lines 201 - 256, In the scan-start flow, extract a local refusal helper that acquires mutex and sets DiscoveryScanState.Failed only when !isActive, then replace both duplicated refusal blocks with refuseScan calls. In the mutex body, bind initialLoraConfig to a local nullable value and use a when branch for the missing-config and missing-primary-channel cases; pass the non-null value directly to prepareScanLocked, removing the redundant refusal == null && initialLoraConfig != null check and unreachable fallback message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/CommandSenderImpl.kt`:
- Around line 353-355: Update requestNeighborInfo to throw
LocalNodeUnavailableException when nodeManager.myNodeNum.value is null, matching
the other request paths in CommandSenderImpl. Remove the ?: 0 fallback so
self-directed NeighborInfo responses are never constructed with node ID 0.
In
`@core/network/src/androidMain/kotlin/org/meshtastic/core/network/radio/AndroidRadioTransportFactory.kt`:
- Around line 68-72: The exact serialDevices lookup removed legacy path-based
USB address recovery. In AndroidRadioTransportFactory.kt lines 68-72, restore
the single-device fallback in isPlatformAddressValid or resolve and persist the
stable USB key; apply the same resolution in SerialRadioTransport.kt lines
217-234 before connect() logs “Serial device not found,” preserving generation
safety and legacy-address self-healing at both sites.
In
`@core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt`:
- Around line 791-813: Update BleRadioTransport.kt lines 791-813 in the close()
teardown flow to replace check(sessionClosed) and check(completed) with warning
logs, allowing expected lifecycle timeouts without throwing or poisoning the
shared gate. Update TcpRadioTransport.kt lines 127-140 to store the
lifecycle.close result and log a warning when it is false instead of asserting
it. Preserve the existing cleanup and fallback behavior in both transports.
In
`@core/network/src/jvmMain/kotlin/org/meshtastic/core/network/SerialTransport.kt`:
- Around line 147-153: Update detectSerialGroup to bound the stat subprocess
wait using a 2-second timeout and TimeUnit.MILLISECONDS, preserving the
"dialout" fallback when it fails or times out. Ensure the process is destroyed
in a finally block so the child process and its streams are released regardless
of the outcome, and add the SERIAL_GROUP_LOOKUP_TIMEOUT_MS constant and required
import.
In
`@feature/discovery/src/commonMain/kotlin/org/meshtastic/feature/discovery/DiscoveryHomeRestorer.kt`:
- Around line 247-256: Update awaitConnected and awaitReconnect to combine
serviceRepository.connectionState with meshPrefs.deviceAddress so the wait
predicate is re-evaluated when either source changes. Add the
kotlinx.coroutines.flow.combine import, preserve the existing connected-state
and plan-matching conditions, and keep returning false when the selected device
no longer matches.
In
`@feature/discovery/src/commonMain/kotlin/org/meshtastic/feature/discovery/DiscoveryInterruptedSessionRecovery.kt`:
- Around line 72-75: The recovery flow in DiscoveryInterruptedSessionRecovery.kt
(lines 72-75) must map completionStatus through finalStatusForPendingRestore,
using UNRESTORABLE only as the fallback; update DiscoveryDao.kt (lines 76-87)
KDoc to note RECOVERABLE_SQL_LIST may return terminal sessions; and update
DiscoveryTerminalCoordinator.kt (lines 120-149) to persist the mapped terminal
status when request.restorePlan is null instead of leaving RESTORE_PENDING_*.
- Around line 48-52: Change the retryable failure log in
DiscoveryInterruptedSessionRecovery’s restoreIfAny handling from Logger.e to
Logger.w, preserving the existing failure details and retry message. Keep
Logger.e reserved for non-retried failures.
In
`@feature/discovery/src/commonMain/kotlin/org/meshtastic/feature/discovery/DiscoveryScanEngine.kt`:
- Around line 529-538: Update requestNeighborInfoAtDwellBoundary to make the
optional neighbor-info request fully best-effort: catch
LocalNodeUnavailableException as well as PacketQueueRejectedException, and
ensure any other request failure is contained so it cannot terminate
runScanLoop. Preserve the existing diagnostic logging and allow the scan loop to
continue without propagating exceptions.
- Around line 221-228: Move the initial home-restore wait in startScanTargets to
before acquiring the engine mutex, removing the pre-schedule await from the
mutex-held path. Retain the second awaitBeforeScan check inside the mutex after
terminalCoordinator.resetForScan so last-minute restore conflicts remain
covered, while stop/reset/packet-collector operations are not blocked by the
bounded wait.
In
`@feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/NodeDetailViewModel.kt`:
- Line 86: Update UiText.resolve(), particularly the UiText.Resource branch, to
use getStringSuspend instead of the blocking getString implementation. Ensure
nested StringResource values are resolved through the suspend/async path as
well, while preserving the existing resolveUiText suspend lambda and safeLaunch
behavior.
In
`@feature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.kt`:
- Around line 1083-1110: Update the test around setResponseStateLoading and
radioConfigUseCase.getOwner to verify the request is actually initiated after
runCurrent(), for example by asserting the configured request callback or
invocation. Keep the existing timeout and failure-state assertions unchanged so
the test proves the timeout originates from the intended request path.
---
Outside diff comments:
In
`@core/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/ReplayFuzzTest.kt`:
- Around line 86-98: Start the ReplayRadioTransport before the ReplayFuzz loop
in handleSendToRadio tolerates arbitrary outbound bytes, ensuring started.value
is true and both fuzzed inputs reach the decode path while preserving the
existing assertions and scheduler handling.
---
Nitpick comments:
In
`@core/database/src/commonMain/kotlin/org/meshtastic/core/database/entity/DiscoverySessionStatus.kt`:
- Around line 37-38: Update DiscoverySessionStatus.RECOVERABLE and
RECOVERABLE_SQL_LIST to be declared independently from shared status constants
rather than parsing the SQL literal; keep RECOVERABLE_SQL_LIST as a const val
for Room. Add a common test named recoverableSetMatchesSqlList that trims
comma-separated SQL values and asserts they equal
DiscoverySessionStatus.RECOVERABLE.
In
`@core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/ReplayRadioTransport.kt`:
- Around line 181-190: Update ReplayRadioTransport.close() to stop throwing when
lifecycle.close returns false; instead, log the bounded teardown timeout and
allow close() to complete so callers can continue shutdown work. Preserve the
existing handshakeQueue.close(), transportJob.cancelAndJoin(), and lifecycle
close behavior.
In
`@core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/TcpRadioTransport.kt`:
- Around line 96-99: Update TcpRadioTransport’s TransportLifecycleGate
construction to pass an explicit operationDrainTimeout derived from
OPERATION_TIMEOUT, matching the BleRadioTransport pattern. Ensure the gate’s
drain bound remains synchronized with the per-operation timeout used in
close()’s operation handling, without changing unrelated lifecycle behavior.
In
`@core/service/src/commonMain/kotlin/org/meshtastic/core/service/AdminControllerImpl.kt`:
- Around line 270-336: Derive the local-destination boolean once in editSettings
and pass that captured value to both EditSettingsSession and
requireCommitBoundaryAccepted. Remove the session-level recomputation from
EditSettingsSession and update the commit-boundary call to use the same
transaction-scoped value, preserving consistent behavior if
nodeManager.myNodeNum changes during the transaction.
In
`@feature/discovery/src/commonMain/kotlin/org/meshtastic/feature/discovery/DiscoveryScanEngine.kt`:
- Around line 201-256: In the scan-start flow, extract a local refusal helper
that acquires mutex and sets DiscoveryScanState.Failed only when !isActive, then
replace both duplicated refusal blocks with refuseScan calls. In the mutex body,
bind initialLoraConfig to a local nullable value and use a when branch for the
missing-config and missing-primary-channel cases; pass the non-null value
directly to prepareScanLocked, removing the redundant refusal == null &&
initialLoraConfig != null check and unreachable fallback message.
In
`@feature/discovery/src/commonTest/kotlin/org/meshtastic/feature/discovery/DiscoveryHomeRestorerTest.kt`:
- Line 290: Replace the hard-coded 3_000L delay in the affected test with
DiscoveryHomeRestorer.POST_RESTORE_SETTLE_DELAY_MS, matching the sibling test
and keeping the restore timing tied to the named production constant.
In
`@feature/discovery/src/commonTest/kotlin/org/meshtastic/feature/discovery/DiscoveryPacketCollectionTest.kt`:
- Around line 337-372: Replace the local InMemoryDiscoveryDao in
DiscoveryPacketCollectionTest with SharedInMemoryDiscoveryDao, adding read-only
accessors there for sessions, presetResults, and discoveredNodes so existing
test usage remains supported. Remove the duplicate fake and update any
references to use the shared implementation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eed564e7-074a-41e3-8019-c952b1a68c77
📒 Files selected for processing (104)
.skills/compose-ui/strings-index.txtandroidApp/src/google/kotlin/org/meshtastic/app/analytics/GooglePlatformAnalytics.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/CommandSenderImpl.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/DataPacketPersistence.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConnectionManagerImpl.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerImpl.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/PacketHandlerImpl.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/CommandSenderImplTest.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/LockdownCoordinatorImplTest.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshConnectionManagerImplTest.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerTest.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/PacketHandlerImplTest.ktcore/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/DiscoveryDao.ktcore/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/SwitchingDiscoveryDao.ktcore/database/src/commonMain/kotlin/org/meshtastic/core/database/entity/DiscoverySessionEntity.ktcore/database/src/commonMain/kotlin/org/meshtastic/core/database/entity/DiscoverySessionStatus.ktcore/database/src/commonTest/kotlin/org/meshtastic/core/database/dao/CommonDiscoveryDaoTest.ktcore/domain/README.mdcore/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/session/EnsureRemoteAdminSessionUseCase.ktcore/domain/src/commonTest/kotlin/org/meshtastic/core/domain/usecase/session/EnsureRemoteAdminSessionUseCaseTest.ktcore/model/src/commonMain/kotlin/org/meshtastic/core/model/ConnectionState.ktcore/model/src/commonMain/kotlin/org/meshtastic/core/model/MeshActivity.ktcore/model/src/commonMain/kotlin/org/meshtastic/core/model/Position.ktcore/network/src/androidHostTest/kotlin/org/meshtastic/core/network/radio/SerialRadioTransportTest.ktcore/network/src/androidMain/kotlin/org/meshtastic/core/network/radio/AndroidRadioTransportFactory.ktcore/network/src/androidMain/kotlin/org/meshtastic/core/network/radio/SerialRadioTransport.ktcore/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.ktcore/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/MockRadioTransport.ktcore/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/NopRadioTransport.ktcore/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/ReplayRadioTransport.ktcore/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/StreamTransport.ktcore/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/TcpRadioTransport.ktcore/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/TransportLifecycleGate.ktcore/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/BleRadioTransportTest.ktcore/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/MockRadioTransportTest.ktcore/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/ReplayFuzzTest.ktcore/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/ReplayRadioTransportTest.ktcore/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/StreamTransportTest.ktcore/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/TcpRadioTransportTest.ktcore/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/TransportLifecycleGateTest.ktcore/network/src/jvmMain/kotlin/org/meshtastic/core/network/SerialTransport.ktcore/repository/README.mdcore/repository/build.gradle.ktscore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/AdminController.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/AwaitedSendResult.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/CommandSender.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/ConnectionStateHolder.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/ConnectionStateProvider.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/EditSettingsTransactionException.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/FixedPositionAdminMessage.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/LocalNodeUnavailableException.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/PacketHandler.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/PacketQueueRejectedException.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/RadioInterfaceService.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/RadioTransport.ktcore/repository/src/commonTest/kotlin/org/meshtastic/core/repository/AwaitedSendResultTest.ktcore/repository/src/commonTest/kotlin/org/meshtastic/core/repository/ConnectionStateHolderTest.ktcore/repository/src/commonTest/kotlin/org/meshtastic/core/repository/RadioTransportTest.ktcore/resources/src/commonMain/composeResources/values/strings.xmlcore/service/src/commonMain/kotlin/org/meshtastic/core/service/AdminControllerImpl.ktcore/service/src/commonMain/kotlin/org/meshtastic/core/service/MessagingControllerImpl.ktcore/service/src/commonMain/kotlin/org/meshtastic/core/service/NodeControllerImpl.ktcore/service/src/commonMain/kotlin/org/meshtastic/core/service/RadioControllerImpl.ktcore/service/src/commonMain/kotlin/org/meshtastic/core/service/ServiceRepositoryImpl.ktcore/service/src/commonMain/kotlin/org/meshtastic/core/service/SharedRadioInterfaceService.ktcore/service/src/commonTest/kotlin/org/meshtastic/core/service/RadioControllerImplTest.ktcore/service/src/commonTest/kotlin/org/meshtastic/core/service/ServiceRepositoryImplTest.ktcore/service/src/commonTest/kotlin/org/meshtastic/core/service/SharedRadioInterfaceServiceLivenessTest.ktcore/takserver/src/commonTest/kotlin/org/meshtastic/core/takserver/TAKMeshIntegrationTest.ktcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeBle.ktcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeCommandSender.ktcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeRadioController.ktcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeRadioInterfaceService.ktcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeRadioTransport.ktcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeServiceRepository.ktcore/testing/src/commonTest/kotlin/org/meshtastic/core/testing/FakeRadioInterfaceServiceSessionTest.ktcore/testing/src/commonTest/kotlin/org/meshtastic/core/testing/RepositoryFakesTest.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/ProtoExtensions.ktdesktopApp/src/main/kotlin/org/meshtastic/desktop/radio/DesktopRadioTransportFactory.ktdesktopApp/src/main/kotlin/org/meshtastic/desktop/stub/NoopStubs.ktfeature/discovery/src/commonMain/kotlin/org/meshtastic/feature/discovery/DiscoveryHomeRestorer.ktfeature/discovery/src/commonMain/kotlin/org/meshtastic/feature/discovery/DiscoveryInterruptedSessionRecovery.ktfeature/discovery/src/commonMain/kotlin/org/meshtastic/feature/discovery/DiscoveryScanEngine.ktfeature/discovery/src/commonMain/kotlin/org/meshtastic/feature/discovery/DiscoveryTerminalCoordinator.ktfeature/discovery/src/commonTest/kotlin/org/meshtastic/feature/discovery/DiscoveryHistoryBehaviorTest.ktfeature/discovery/src/commonTest/kotlin/org/meshtastic/feature/discovery/DiscoveryHomeRestorerTest.ktfeature/discovery/src/commonTest/kotlin/org/meshtastic/feature/discovery/DiscoveryMapFilterTest.ktfeature/discovery/src/commonTest/kotlin/org/meshtastic/feature/discovery/DiscoveryPacketCollectionTest.ktfeature/discovery/src/commonTest/kotlin/org/meshtastic/feature/discovery/DiscoveryScanEngineTest.ktfeature/discovery/src/commonTest/kotlin/org/meshtastic/feature/discovery/SharedInMemoryDiscoveryDao.ktfeature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/CommonNodeRequestActions.ktfeature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/NodeDetailViewModel.ktfeature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/NodeManagementActions.ktfeature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/NodeRequestRejectionFeedback.ktfeature/node/src/commonMain/kotlin/org/meshtastic/feature/node/list/NodeListViewModel.ktfeature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/MetricsViewModel.ktfeature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/CommonNodeRequestActionsTest.ktfeature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/NodeDetailViewModelTest.ktfeature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/NodeManagementActionsTest.ktfeature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/RecordingSnackbarManager.ktfeature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModel.ktfeature/settings/src/commonTest/kotlin/org/meshtastic/feature/settings/radio/RadioConfigViewModelTest.ktfeature/widget/src/main/kotlin/org/meshtastic/feature/widget/RefreshLocalStatsAction.ktfeature/widget/src/test/kotlin/org/meshtastic/feature/widget/RefreshLocalStatsActionTest.kt
jeremiah-k
marked this pull request as draft
August 8, 2026 23:48
Reserve packet IDs from admission through terminal completion so duplicate sends cannot steal acknowledgements and IDs become reusable only after ownership is released. Serialize dispatch publication with response completion, drain reservations during teardown, and distinguish rejection, timeout, transport departure, and send failure. Publish connection state and epochs from one committed lifecycle snapshot so queued work, acknowledgements, and reconnect handling cannot combine evidence from different sessions. Keep compatibility views correlated under concurrent transitions. Apply the same evidence to edit-settings transactions: await begin and commit boundaries, finalize accepted transactions under NonCancellable cleanup, accept a local commit only after dispatch and a post-baseline departure, and keep remote or pre-dispatch failures fail-closed. Cover queue ownership, cancellation, reconnect, and commit-boundary races across production and test fakes.
Require every transport handoff to report synchronous admission rather than imply delivery. Gate packet and heartbeat scheduling through the active session lease so teardown drains accepted work, rejects late writes, and never publishes dispatch or mesh activity for bytes the transport refused. Enforce the contract across BLE, serial, stream, TCP, replay, mock, and inert transports. Admit serial writes only after the current connection reports ready, reject disconnected TCP and stopped scopes promptly, and isolate replay startup under atomic ownership. Surface queue rejection through command APIs before persisting optimistic side effects, translate routing NAKs into RADIO_REJECTED results, preserve command-stamped reaction status, and present node-request rejection without starting success timers. Keep diagnostics privacy-safe and document the admission boundary. Cover unavailable and opening transports, stopped scopes, teardown races, late heartbeats, realistic fakes, coroutine exception recovery, routing responses, reaction persistence, and rejected request side effects.
Give multi-hop remote-admin metadata refreshes thirty seconds to produce a user-facing result instead of treating valid responses after ten seconds as failures. The request remains asynchronous and late responses still update durable session state. Document the deadline and cover a response arriving after the former timeout so slow mesh paths can establish a session without weakening disconnected or no-response handling.
Linearize transport startup, admission, callback publication, and teardown across BLE, serial, TCP, mock, replay, and stream transports. Bind queued work to the exact transport generation, bound pending sends, drain or cancel owned work before physical teardown, and keep terminal close idempotent for every caller. Preserve packet and admin ownership across queue rejection, persistence, cancellation, and connection departure. Stage local edit projections behind accepted commit boundaries, fail closed when local identity is unavailable, and bind post-handshake passkey and telemetry retries to the connected lifecycle generation that requested them. Serialize discovery terminal persistence and home-radio restoration so aggregate/session state is written before restoration can finalize status. Keep interrupted recovery device-bound, preserve terminal outcomes across cancellation, and refuse overlapping cleanup or unrestorable scans. Add deterministic coverage for queue bounds, replacement generations, close/send races, stale callbacks, recovery ordering, transaction boundaries, and best-effort UI request failures.
Make dispatch evidence and queued channel-batch ownership single-source and cancellation-safe. Keep discovery preparation bound to captured radio identity, preserve terminalization while backing off process-lifetime restore retries, and align direct dependencies, lifecycle fakes, documentation, and regression coverage with production contracts. Preserve routing-error identity and bounded ownership for late remote settings reads without relaxing write, batch, or destructive-action failure semantics.
jeremiah-k
force-pushed
the
bugfix/command-transport-lifecycle
branch
from
August 9, 2026 15:29
6839180 to
0b5307d
Compare
Contributor
Author
|
@coderabbitai I consolidated the follow-up here rather than posting a series of small inline replies. The issues raised in this CodeRabbit review have been addressed across the current five-commit series:
While applying those fixes, I also tightened a few adjacent lifecycle cases for consistency:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
echo "=== CommandSenderImpl.requestNeighborInfo ==="
rg -n "requestNeighborInfo" -A 15 core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/CommandSenderImpl.kt | head -60
echo "=== DiscoverySessionStatus.RECOVERABLE ==="
cat core/database/src/commonMain/kotlin/org/meshtastic/core/database/entity/DiscoverySessionStatus.kt
echo "=== UiText.resolve() ==="
fd UiText.kt | xargs -I{} sh -c 'echo ---{}---; cat {}'Length of output: 6605
These three checks match your description. Given the scope of this consolidated response, I will treat the full list as addressed based on this verification and the consistency of the surrounding changes. I appreciate the detailed write-up; it made review much easier to follow. (=^･ω･^=) ノ You are interacting with an AI system. |
jeremiah-k
marked this pull request as ready for review
August 9, 2026 16:37
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This makes outbound packet admission explicit across the queue, service, and transport layers and keeps admitted work bound to the connection or transport generation that owns it.
Rejected or stale sends no longer create dispatch evidence, response timers, optimistic state, or follow-on work. Accepted sends retain their ownership through acknowledgements, cancellation, reconnects, and teardown. The same lifecycle boundaries are applied to settings transactions, post-handshake requests, remote-admin session refresh, and discovery home restoration.
Review Guide
The five commits are intended to be reviewed in order:
Key Changes
Packet admission and command outcomes
0.Transport lifecycle
Admin and connection lifecycle
Discovery recovery
User-facing rejection handling
Testing
Added or extended coverage for:
On-device F-Droid validation exercised BLE, Wi-Fi/TCP, and USB serial transport paths, including transport switching, handshake, packet admission, node and configuration exchange, telemetry, admin-session refresh, and remote-admin traffic. All three transports completed normal connection setup and handshake, carried admitted outbound traffic, and continued exchanging expected queue, configuration, telemetry, and session data. Transitions between transports also cleared the outgoing session and established a new connection generation without stale work being carried forward.
Scope