fix(tak): gate V2 packets on known firmware - #6600
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesTAK protocol selection
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/takserver/src/jvmAndroidMain/kotlin/org/meshtastic/core/takserver/TAKServerJvm.kt (1)
166-185: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPrevent connection admission after
stop().
stop()can clearconnectionswhile this coroutine is inauthenticateTlsClient(). The coroutine can then add and start aTAKClientConnectionafter shutdown completes.Track pending authentication sockets or jobs. Serialize the running-state check and connection registration with shutdown. Close a socket when shutdown has started.
🤖 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/takserver/src/jvmAndroidMain/kotlin/org/meshtastic/core/takserver/TAKServerJvm.kt` around lines 166 - 185, Update the connection-launch flow around authenticateTlsClient and the connectionsLock registration to track pending authentication jobs or sockets and serialize admission with stop(). Recheck the running state while holding the same synchronization used by shutdown before inserting or starting TAKClientConnection; if shutdown has begun, close clientSocket and do not register it. Ensure stop() marks shutdown before clearing active connections and also closes or cancels pending authentication work.
🤖 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/takserver/src/commonTest/kotlin/org/meshtastic/core/takserver/TAKMeshIntegrationTest.kt`:
- Line 435: Update the test setup around FakeNodeRepository in
TAKMeshIntegrationTest so it supplies a present MyNodeInfo whose firmwareVersion
is null, rather than passing null to the repository constructor. Preserve the
existing outbound packet assertions and ensure the test exercises the production
path for a present node with unknown firmware.
In
`@core/takserver/src/jvmTest/kotlin/org/meshtastic/core/takserver/TAKServerJvmInteropTest.kt`:
- Around line 115-117: Update the handshake test around client.startHandshake()
to wrap the call in assertFailsWith<SSLException>, reflecting the expected mTLS
failure; keep the subsequent timeout and server connection-count assertions
unchanged.
---
Outside diff comments:
In
`@core/takserver/src/jvmAndroidMain/kotlin/org/meshtastic/core/takserver/TAKServerJvm.kt`:
- Around line 166-185: Update the connection-launch flow around
authenticateTlsClient and the connectionsLock registration to track pending
authentication jobs or sockets and serialize admission with stop(). Recheck the
running state while holding the same synchronization used by shutdown before
inserting or starting TAKClientConnection; if shutdown has begun, close
clientSocket and do not register it. Ensure stop() marks shutdown before
clearing active connections and also closes or cancels pending authentication
work.
🪄 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: 2e85e75e-55b3-4ee9-bad6-541b75a081f2
📒 Files selected for processing (4)
core/takserver/src/commonMain/kotlin/org/meshtastic/core/takserver/TAKMeshIntegration.ktcore/takserver/src/commonTest/kotlin/org/meshtastic/core/takserver/TAKMeshIntegrationTest.ktcore/takserver/src/jvmAndroidMain/kotlin/org/meshtastic/core/takserver/TAKServerJvm.ktcore/takserver/src/jvmTest/kotlin/org/meshtastic/core/takserver/TAKServerJvmInteropTest.kt
a0e96b9 to
6b8db4d
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
Testing
:core:takserver:spotlessCheck:core:takserver:jvmTest --tests org.meshtastic.core.takserver.TAKMeshIntegrationTestSummary by CodeRabbit