Feature: Remote participant volume control#47
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an end-to-end remote participant volume control feature: client UI drag handling and throttling, client-side SetRemoteVolume JSON sending with sequencing and debug logging, server-side routing/forwarding with identity mapping and deduplication, and client-side ApplyLocalVolume parsing/deduplication and application, plus supporting schema, registry, tooling, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as MainComponent (Client A)
participant AS as AudioService (Client A)
participant Server as JammerNetzServer
participant Accept as AcceptThread (Server)
participant Forward as SendThread/Mixer (Server)
participant Rx as DataReceiveThread (Client B)
participant Audio as AudioCallback (Client B)
participant Log as RemoteControlDebugLog
UI->>AS: setRemoteParticipantVolume(targetClientId, targetChannelIndex, percent)
AS->>AS: clamp percent, bump sequence
AS->>Server: send JSON SetRemoteVolume {target, channel, volume, sequence}
Server->>Accept: receive control message with source endpoint
Accept->>Accept: parse & validate payload
Accept->>Accept: deduplicate by (source, target, channel, sequence)
alt forwardable
Accept->>Forward: resolve target endpoint via ClientIdentityRegistry
Forward->>Server: sendControlMessageToClient(ApplyLocalVolume JSON)
Server->>Rx: deliver ApplyLocalVolume JSON
Rx->>Rx: parse payload, check routing cache and sequence
alt new sequence & routed
Rx->>Audio: apply volume on UI/audio thread (async)
Rx->>Log: logEvent("apply", details)
else stale/dup or unmapped
Rx->>Log: logEvent("drop", details)
end
else invalid/stale
Accept->>Log: logEvent("drop", details)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Server/Source/AcceptThread.cpp (1)
66-88:⚠️ Potential issue | 🔴 CriticalConstructor member initializer order does not match declaration order in the header.
The initializer list order in
AcceptThread.cpp(lines 66-88) placesbufferConfig_immediately aftersessionControlRevision_, but inAcceptThread.h(line 48),bufferConfig_is declared afterreadbuffer(line 46) andqualityTimer_(line 47). This mismatch will trigger-Wreordercompiler warnings. Reorder the initializer list to match the member declaration order in the header.
🤖 Fix all issues with AI agents
In `@Client/Source/ChannelControllerGroup.cpp`:
- Around line 111-119: In
ChannelControllerGroup::isSessionVolumeSliderBeingDragged, remove the
unconditional jassertfalse that fires when findSessionChannelIndex returns
nullopt or an out-of-range index; instead perform a simple early return false
for that transient/missing case (or, if you want a debug-only check, replace
jassertfalse with jassert containing a descriptive condition like
channelIndex.has_value() && *channelIndex >= 0 && *channelIndex <
channelControllers_.size()). This keeps the existing return false fallback while
avoiding spurious halts during session transitions; update the block that checks
channelIndex (the call to findSessionChannelIndex and the bounds check against
channelControllers_.size()) accordingly.
In `@Client/Source/DataReceiveThread.cpp`:
- Around line 110-146: The lambda passed to MessageManager::callAsync re-reads
Data at async execution time using targetChannelIndex (physical index) which can
become stale; instead compute the logical controller index synchronously before
calling MessageManager::callAsync and capture that computed index in the lambda.
Concretely: perform the loop over channels (using Data::instance().get(),
inputSetup.getChildWithName(VALUE_INPUT_SETUP),
channels.getChildWithName(VALUE_CHANNELS), checking VALUE_CHANNEL_ACTIVE)
outside the MessageManager::callAsync call to derive activeControllerIndex (or
null/optional), verify it is valid, then call MessageManager::callAsync
capturing activeControllerIndex and remoteVolumePercent and inside the lambda
directly update mixer.getOrCreateChildWithName("Input" +
String(*activeControllerIndex)).setProperty(VALUE_VOLUME, remoteVolumePercent,
nullptr).
In `@Client/Source/DataReceiveThread.h`:
- Line 47: The map lastAppliedRemoteVolumeSequence_ in DataReceiveThread retains
per-(clientID,volumeID) sequence state and is never cleared, causing new
connections that reset sequence counters to have their commands treated as
stale; add logic to expire or remove entries from
lastAppliedRemoteVolumeSequence_ when a session setup changes or when a client
disconnects/reconnects (e.g., provide a method on DataReceiveThread such as
ClearRemoteVolumeSequenceForClient(clientId) and another
ClearAllRemoteSequencesOnSessionReset()), and call these from the session
setup/change handler and client-disconnect handling code so entries for missing
or reinitialized client IDs are removed before new sequences arrive.
In `@Client/Source/MainComponent.cpp`:
- Around line 369-378: The call to
allChannels_.isSessionVolumeSliderBeingDragged(...) can hit jassertfalse when
thisSetup (new layout) contains a channel identity that no longer exists in
allChannels_ (old layout); before calling isSessionVolumeSliderBeingDragged in
the loop over thisSetup.channels, first verify the identity exists in the
current session (e.g., call findSessionChannelIndex or a new
hasSessionChannel(uint32, uint16) on ChannelControllerGroup) and only call
isSessionVolumeSliderBeingDragged if that check succeeds; to implement this
either make findSessionChannelIndex public or add a lightweight
hasSessionChannel method to ChannelControllerGroup, or alternately change
isSessionVolumeSliderBeingDragged to return false instead of asserting when the
identity is not found, and update the loop using remoteVolumeCommandState_,
thisSetup, allChannels_, and isSessionVolumeSliderBeingDragged accordingly.
In `@run_test.sh`:
- Around line 21-24: The trap registration is happening after the client
processes are launched, risking orphaned PIDs if the script exits early; move
the trap setup so it runs before spawning clients (or at minimum immediately
after launching the first background process). Specifically, register the trap
for cleanup (trap cleanup EXIT INT TERM) prior to creating
client1_pid/client2_pid or right after the first background launch, and ensure
cleanup() safely guards against unset PIDs (check client1_pid/client2_pid before
kill) so kill calls won't error if a PID is unset.
- Around line 16-26: The clients (JammerNetzClient launched into background and
tracked by client1_pid/client2_pid) are started before the server
(JammerNetzServer), causing a race; reorder the script so JammerNetzServer is
started first (and its stdout captured as before), wait for readiness (simple
sleep or better: poll the server port/health endpoint) before launching the two
JammerNetzClient instances, then start the clients in background and set
client1_pid/client2_pid, and keep the existing cleanup() and trap to kill those
pids on EXIT/INT/TERM; ensure any readiness-wait uses the same stamp/log naming
(JN_REMOTE_LOG_NAME) as the clients.
In `@Server/Source/AcceptThread.cpp`:
- Around line 199-205: Throttling can drop the final slider update; modify the
logic around sessionControlRevision_, lastSessionRevisionBumpMillis_ and
kSessionRevisionMinIntervalMs so that the final SetRemoteVolume is always
reflected: detect when commandSequence (or the incoming command ID) differs from
the last-processed sequence and force a revision bump even if within the 60ms
window, or alternatively schedule a short debounce timeout that fires after the
last slider event to call sessionControlRevision_.fetch_add(1) and update
lastSessionRevisionBumpMillis_; ensure the comparison and forced bump live
alongside the existing timed throttle so downstream consumers always see the
final state.
- Around line 156-170: The sequence deduplication currently uses
lastForwardedControlSequence_[sequenceKey] which default-initializes to 0 and
causes a true duplicate check when the first incoming command has sequence 0;
change the logic to use map::find() (or equivalent) to detect whether
sequenceKey is present before comparing: if find(sequenceKey) == end, treat this
as first-seen and accept the command and insert the incoming sequence into
lastForwardedControlSequence_ (instead of relying on the default 0); otherwise
compare *parsedPayload->commandSequence to the found lastSequence and update the
stored value on accept. Apply the same fix in the mirrored code path in
Client/Source/DataReceiveThread.cpp where the corresponding last-seen map is
accessed.
🧹 Nitpick comments (8)
Server/Source/Main.cpp (1)
59-61: Minor: indentation inconsistency.Lines 59–61 use an extra tab level compared to the surrounding constructor body (e.g., line 63). This appears to be a whitespace issue introduced with the change.
Client/Source/AudioService.cpp (1)
57-63: Indentation is inconsistent with surrounding code.Lines 57–62 have an extra level of indentation (looks like a tab was added). The lines before and after (e.g., line 56 and line 63) use a single tab level within the
forbody.🔧 Fix indentation
- JammerNetzSingleChannelSetup channel((uint8) target); - double volume = controllerData.getProperty(VALUE_VOLUME, 100.0); - channel.volume = (float)volume/100.0f; - channel.sourceClientId = 0; - channel.sourceChannelIndex = static_cast<uint16>(setup->activeChannelIndices[i]); - auto username = Data::instance().get().getProperty(VALUE_USER_NAME).toString().toStdString(); + JammerNetzSingleChannelSetup channel((uint8) target); + double volume = controllerData.getProperty(VALUE_VOLUME, 100.0); + channel.volume = (float)volume/100.0f; + channel.sourceClientId = 0; + channel.sourceChannelIndex = static_cast<uint16>(setup->activeChannelIndices[i]); + auto username = Data::instance().get().getProperty(VALUE_USER_NAME).toString().toStdString();common/RemoteControlDebugLog.cpp (1)
39-49: Dual enable/disable env vars are slightly confusing but benign.
JN_REMOTE_LOG_ENABLEmust be explicitly set to"1"or"true"(logging is off by default), and thenJN_REMOTE_LOG_DISABLEcan override it. Since the enable check already guards entry, the disable check on Lines 45-49 is redundant in practice. Consider documenting the intended precedence (e.g., in the header or a comment) so future maintainers understand why both exist, or simply drop the disable variable.Client/Source/ChannelControllerGroup.h (1)
23-23: Consider naming the handler parameters for documentation purposes.The
SessionVolumeChangedHandleralias uses unnamed parameters(uint32, uint16, float). While valid C++, adding comments or a brief docstring would help callers understand the meaning (client ID, channel index, volume percent).💡 Suggested improvement
- using SessionVolumeChangedHandler = std::function<void(uint32, uint16, float)>; + // Parameters: targetClientId, targetChannelIndex, volumePercent + using SessionVolumeChangedHandler = std::function<void(uint32 targetClientId, uint16 targetChannelIndex, float volumePercent)>;Server/Source/AcceptThread.h (1)
50-50: Consider adding a comment or type alias for the composite map key.
std::map<std::tuple<uint32, uint32, uint16>, uint64_t>is not self-documenting. A brief comment or a named alias would clarify what each element of the tuple represents (e.g., source client ID, target client ID, target channel index).💡 Suggested improvement
+ // Key: (sourceClientId, targetClientId, targetChannelIndex) → last forwarded sequence number std::map<std::tuple<uint32, uint32, uint16>, uint64_t> lastForwardedControlSequence_;Server/Source/MixerThread.cpp (2)
87-93: Indentation is inconsistent with the surrounding code.Lines 87–93 have an extra level of indentation compared to the enclosing scope (the
forloop on line 74 and the code after line 93). This looks like a leftover from a removedifblock or a merge artifact. It doesn't affect correctness but hurts readability.
115-125:getOrAssignClientIdis called redundantly inside thefor_eachlambda.
clientIdentityRegistry_.getOrAssignClientId(client.first)is invoked once per multimap entry for the same client key. Since the result is deterministic for a givenclient.first, hoist it before thefor_eachto avoid repeated lookups.Also, the indentation on line 115 (
if) has an extra tab relative to the surrounding block — appears to be a formatting artifact.Proposed fix
if (client.first != receiver.first) { + auto sourceClientId = clientIdentityRegistry_.getOrAssignClientId(client.first); auto range = allSessionChannels.equal_range(client.first); - for_each(range.first, range.second, [&](std::pair<const std::string, JammerNetzChannelSetup> setup) { - auto sourceClientId = clientIdentityRegistry_.getOrAssignClientId(client.first); + for_each(range.first, range.second, [&, sourceClientId](std::pair<const std::string, JammerNetzChannelSetup> setup) { for (const auto& sourceChannel : setup.second.channels) { auto copiedChannel = sourceChannel; copiedChannel.sourceClientId = sourceClientId; sessionSetup.channels.push_back(copiedChannel); } }); }Server/Source/AcceptThread.cpp (1)
95-134:atoifor port parsing is fragile; considerstd::stoiorjuce::String::getIntValue.
atoisilently returns 0 on non-numeric input and has undefined behavior on overflow. Theport <= 0guard catches the common failure case, butstd::stoi(with exception handling) or JUCE'sString(…).getIntValue()would be more robust. Not a blocking issue since the input comes from a controlledclientNameformat (ip:port), but worth hardening.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…icipant-volume-control
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@Client/Source/DataReceiveThread.cpp`:
- Around line 150-171: The code currently uses targetChannelRoutingCache_ when
cachedRouting exists even if activeControllerIndex is empty; change the logic in
the ApplyLocalVolume handling (the block that reads targetChannelRoutingCache_,
resolvedControllerIndex and uses RemoteControlDebugLog::logEvent) to guard
against stale cache entries by checking if activeControllerIndex has_value()
before trusting cachedRouting; if activeControllerIndex is not present,
remove/invalidate the cache entry (targetChannelRoutingCache_.erase or
equivalent), log a warning via RemoteControlDebugLog::logEvent indicating a
stale cache for targetChannel, and return/drop the command instead of applying
the cached controller index.
- Around line 221-261: The topology-change branch in handleSessionSetupChange
currently clears per-client sequences but fails to invalidate the routing cache;
update handleSessionSetupChange so that when topologyChanged is true it also
clears/invalidates targetChannelRoutingCache_ (same behavior as
clearAllRemoteSequencesOnSessionReset), then proceed to call
clearRemoteVolumeSequenceForClient for currentClientIds; reference
handleSessionSetupChange, targetChannelRoutingCache_,
clearAllRemoteSequencesOnSessionReset, and clearRemoteVolumeSequenceForClient
when making the change.
In `@issues/2020-12-21-branch-createmidiclock-triage.debt.md`:
- Line 14: The phrase "MIDI clock related" in the diff for the createMIDIclock
branch should be hyphenated as a compound modifier; update the text to "MIDI
clock-related" (or "MIDI-clock-related") so the line reads "...adds server-time
transport and MIDI clock-related changes..." referencing the createMIDIclock
branch text in the diff.
In `@issues/2024-10-18-branch-rustserver-prototype-disposition.debt.md`:
- Line 14: In the line describing the Rust prototype (the text containing
`rustServer`), change the phrase "non working" to the hyphenated "non-working"
so the sentence reads that `rustServer` contains early prototype work for a Rust
mixing server with explicit "non-working leftover" status in the latest commit;
update only that phrase to correct the typo.
In `@issues/2026-02-15-packetstreamqueue-quality-divide-by-zero.bug.md`:
- Around line 18-21: In qualityStatement(), avoid dividing by zero by checking
packagesPopped before computing droppedPacketCounter / (float)packagesPopped *
100.0f: if packagesPopped == 0 return a sensible default (e.g., 0% or "N/A") or
handle the case explicitly; update the code around the percentage calculation
that uses droppedPacketCounter and packagesPopped so the division only occurs
when packagesPopped > 0 and ensure the function still returns the expected
type/format.
🧹 Nitpick comments (8)
issues/2026-02-15-mixerthread-incoming-map-reset-race.bug.md (2)
19-25: Consider using more maintainable code references.Hardcoded line numbers will drift as the source files evolve, making it difficult to verify this issue in the future. Consider anchoring references using:
- Git commit SHAs with line numbers (e.g.,
commit abc123, lines 87-90)- Function/method names (e.g.,
MixerThread::handleUnderrun())- Code snippets or surrounding context that's easier to search for
This will improve long-term maintainability of the issue documentation.
3-11: Consider adding additional tracking metadata.The current metadata is well-structured, but you might consider adding:
- Assignee: Who is responsible for addressing this issue
- Milestone/Target: When this should be resolved
- Related Issues: Cross-references to related bugs or features
- Severity: Classification beyond priority (e.g., Critical, High, Medium, Low)
These additions would improve issue tracking and project management, though the current structure is already quite good.
issues/2026-02-15-packetstreamqueue-quality-divide-by-zero.bug.md (2)
30-31: Clarify the preferred fix approach.The next steps propose two alternatives (
n/aor0.00%) without recommending which is preferred. Consider being more prescriptive:n/ais semantically clearer since it indicates no data is available yet, whereas0.00%could be misinterpreted as perfect quality (zero drops).📝 Suggested clarification
1. guard denominator: - - if `packagesPopped == 0`, print `n/a` or `0.00%` + - if `packagesPopped == 0`, print `n/a` (preferred: indicates no data vs. zero drops) 2. add tiny unit test for zero-populated quality summary
34-37: Consider adding regression testing to exit criteria.The exit criteria are clear and testable. Consider explicitly mentioning that the unit test mentioned in "Next steps" should be included in the exit criteria to prevent regression.
📝 Optional enhancement
- no divide-by-zero in quality report path - stable output for empty/initial stream states +- unit test coverage for zero-populated quality scenariosissues/PRIORITY_PLAN.md (1)
131-136: Self-referential PR mention.Line 136 references "merge or close PR
#47" — which is this very PR. If the intent is to track follow-up work after this PR merges, consider rewording to clarify the scope (e.g., "address remaining validation items from PR#47" or link to a separate follow-up issue).Client/Source/DataReceiveThread.cpp (1)
80-116: Deduplication logic looks correct.The per-source/per-channel sequence tracking correctly drops stale or duplicate commands. The negative index guard (line 92) before the
uint16cast (line 99) is good.One minor note:
static_cast<uint16>(payload->targetChannelIndex)could silently truncate values above 65535. In practice channel counts are tiny, but ajassert(payload->targetChannelIndex <= std::numeric_limits<uint16>::max())would make the assumption explicit.log_analyze.py (2)
39-53:STDOUT_ERROR_PATTERNScontains overly broad patterns that may produce false positives.The
r"error"pattern (line 52) will match any line containing "error" case-insensitively, including benign output like"errors: 0","error_count=0", or JUCE info messages. Similarly,r"assert"matches"assertion","asserting", or even benign log contexts.Consider using word boundaries (
\berror\b) or more specific patterns to reduce noise. Alternatively, add an exclusion list for known-benign patterns.Tighter patterns example
STDOUT_ERROR_PATTERNS = [ re.compile(pat, re.IGNORECASE) for pat in [ r"fatal", r"overflow", r"send queue length overflow", r"malloc\(\)", r"unaligned tcache", r"aborted", - r"assert", + r"\bassert(?:ion)?\s*(?:failed|error)", r"network down", r"disconnect", - r"error", + r"\berror\b", ] ]
65-87:summarize_sequencecould produce a very largemissinglist for big gaps.If a sequence jumps from e.g. 1 to 1,000,000,
missing.extend(range(...))materializes ~1M elements. This is unlikely in normal operation but could be triggered by corrupt or misinterpreted log data. Consider capping the gap expansion or only recording the gap range endpoints.
| auto targetChannel = static_cast<uint16>(payload->targetChannelIndex); | ||
| std::optional<int> resolvedControllerIndex; | ||
| auto cachedRouting = targetChannelRoutingCache_.find(targetChannel); | ||
| if (cachedRouting != targetChannelRoutingCache_.end()) { | ||
| resolvedControllerIndex = cachedRouting->second; | ||
| if (activeControllerIndex.has_value() && *activeControllerIndex != cachedRouting->second) { | ||
| RemoteControlDebugLog::logEvent("client.recv", | ||
| "routing drift targetChannel=" + String(payload->targetChannelIndex) | ||
| + " computedController=" + String(*activeControllerIndex) | ||
| + " cachedController=" + String(cachedRouting->second) | ||
| + " action=use-cached"); | ||
| } | ||
| } | ||
| else { | ||
| if (!activeControllerIndex.has_value()) { | ||
| RemoteControlDebugLog::logEvent("client.recv", | ||
| "drop ApplyLocalVolume inactive targetChannel=" + String(payload->targetChannelIndex)); | ||
| return; | ||
| } | ||
| resolvedControllerIndex = *activeControllerIndex; | ||
| targetChannelRoutingCache_.emplace(targetChannel, *resolvedControllerIndex); | ||
| } |
There was a problem hiding this comment.
Cached routing used even when the target channel is no longer active — potential stale volume application.
When cachedRouting exists (line 153) but activeControllerIndex is empty (the target physical channel is no longer active), the code still proceeds with the cached controller index. This could silently apply volume to an unrelated controller or one that no longer maps to the intended channel.
Consider adding a guard: if the channel is confirmed inactive (no activeControllerIndex) and a cached entry exists, either invalidate the cache entry and drop the command, or at least log a warning.
Suggested guard
auto cachedRouting = targetChannelRoutingCache_.find(targetChannel);
if (cachedRouting != targetChannelRoutingCache_.end()) {
resolvedControllerIndex = cachedRouting->second;
+ if (!activeControllerIndex.has_value()) {
+ RemoteControlDebugLog::logEvent("client.recv",
+ "routing stale targetChannel=" + String(payload->targetChannelIndex)
+ + " cachedController=" + String(cachedRouting->second)
+ + " action=channel-inactive-drop");
+ targetChannelRoutingCache_.erase(cachedRouting);
+ return;
+ }
if (activeControllerIndex.has_value() && *activeControllerIndex != cachedRouting->second) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto targetChannel = static_cast<uint16>(payload->targetChannelIndex); | |
| std::optional<int> resolvedControllerIndex; | |
| auto cachedRouting = targetChannelRoutingCache_.find(targetChannel); | |
| if (cachedRouting != targetChannelRoutingCache_.end()) { | |
| resolvedControllerIndex = cachedRouting->second; | |
| if (activeControllerIndex.has_value() && *activeControllerIndex != cachedRouting->second) { | |
| RemoteControlDebugLog::logEvent("client.recv", | |
| "routing drift targetChannel=" + String(payload->targetChannelIndex) | |
| + " computedController=" + String(*activeControllerIndex) | |
| + " cachedController=" + String(cachedRouting->second) | |
| + " action=use-cached"); | |
| } | |
| } | |
| else { | |
| if (!activeControllerIndex.has_value()) { | |
| RemoteControlDebugLog::logEvent("client.recv", | |
| "drop ApplyLocalVolume inactive targetChannel=" + String(payload->targetChannelIndex)); | |
| return; | |
| } | |
| resolvedControllerIndex = *activeControllerIndex; | |
| targetChannelRoutingCache_.emplace(targetChannel, *resolvedControllerIndex); | |
| } | |
| auto targetChannel = static_cast<uint16>(payload->targetChannelIndex); | |
| std::optional<int> resolvedControllerIndex; | |
| auto cachedRouting = targetChannelRoutingCache_.find(targetChannel); | |
| if (cachedRouting != targetChannelRoutingCache_.end()) { | |
| resolvedControllerIndex = cachedRouting->second; | |
| if (!activeControllerIndex.has_value()) { | |
| RemoteControlDebugLog::logEvent("client.recv", | |
| "routing stale targetChannel=" + String(payload->targetChannelIndex) | |
| " cachedController=" + String(cachedRouting->second) | |
| " action=channel-inactive-drop"); | |
| targetChannelRoutingCache_.erase(cachedRouting); | |
| return; | |
| } | |
| if (activeControllerIndex.has_value() && *activeControllerIndex != cachedRouting->second) { | |
| RemoteControlDebugLog::logEvent("client.recv", | |
| "routing drift targetChannel=" + String(payload->targetChannelIndex) | |
| " computedController=" + String(*activeControllerIndex) | |
| " cachedController=" + String(cachedRouting->second) | |
| " action=use-cached"); | |
| } | |
| } | |
| else { | |
| if (!activeControllerIndex.has_value()) { | |
| RemoteControlDebugLog::logEvent("client.recv", | |
| "drop ApplyLocalVolume inactive targetChannel=" + String(payload->targetChannelIndex)); | |
| return; | |
| } | |
| resolvedControllerIndex = *activeControllerIndex; | |
| targetChannelRoutingCache_.emplace(targetChannel, *resolvedControllerIndex); | |
| } |
🤖 Prompt for AI Agents
In `@Client/Source/DataReceiveThread.cpp` around lines 150 - 171, The code
currently uses targetChannelRoutingCache_ when cachedRouting exists even if
activeControllerIndex is empty; change the logic in the ApplyLocalVolume
handling (the block that reads targetChannelRoutingCache_,
resolvedControllerIndex and uses RemoteControlDebugLog::logEvent) to guard
against stale cache entries by checking if activeControllerIndex has_value()
before trusting cachedRouting; if activeControllerIndex is not present,
remove/invalidate the cache entry (targetChannelRoutingCache_.erase or
equivalent), log a warning via RemoteControlDebugLog::logEvent indicating a
stale cache for targetChannel, and return/drop the command instead of applying
the cached controller index.
| void DataReceiveThread::handleSessionSetupChange(const JammerNetzChannelSetup& newSessionSetup) | ||
| { | ||
| std::set<uint32> previousClientIds; | ||
| for (const auto& channel : currentSession_.channels) { | ||
| if (channel.sourceClientId != 0) { | ||
| previousClientIds.insert(channel.sourceClientId); | ||
| } | ||
| } | ||
|
|
||
| std::set<uint32> currentClientIds; | ||
| for (const auto& channel : newSessionSetup.channels) { | ||
| if (channel.sourceClientId != 0) { | ||
| currentClientIds.insert(channel.sourceClientId); | ||
| } | ||
| } | ||
|
|
||
| for (auto clientId : previousClientIds) { | ||
| if (currentClientIds.find(clientId) == currentClientIds.end()) { | ||
| clearRemoteVolumeSequenceForClient(clientId); | ||
| } | ||
| } | ||
|
|
||
| bool topologyChanged = currentSession_.channels.size() != newSessionSetup.channels.size(); | ||
| if (!topologyChanged) { | ||
| for (size_t i = 0; i < currentSession_.channels.size(); i++) { | ||
| const auto& oldChannel = currentSession_.channels[i]; | ||
| const auto& newChannel = newSessionSetup.channels[i]; | ||
| if (oldChannel.sourceClientId != newChannel.sourceClientId | ||
| || oldChannel.sourceChannelIndex != newChannel.sourceChannelIndex) { | ||
| topologyChanged = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (topologyChanged) { | ||
| for (auto clientId : currentClientIds) { | ||
| clearRemoteVolumeSequenceForClient(clientId); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
handleSessionSetupChange does not clear the routing cache on topology change.
When topology changes are detected (line 256), sequences are cleared for all current clients, but targetChannelRoutingCache_ is not invalidated. Topology changes (channel reordering, clients joining/leaving) can make the physical→controller mapping stale. In contrast, clearAllRemoteSequencesOnSessionReset (line 210) does clear the routing cache.
This looks like an oversight — the routing cache should also be cleared when topology changes.
Suggested fix
if (topologyChanged) {
for (auto clientId : currentClientIds) {
clearRemoteVolumeSequenceForClient(clientId);
}
+ clearRemoteVolumeChannelRoutingCache();
}🤖 Prompt for AI Agents
In `@Client/Source/DataReceiveThread.cpp` around lines 221 - 261, The
topology-change branch in handleSessionSetupChange currently clears per-client
sequences but fails to invalidate the routing cache; update
handleSessionSetupChange so that when topologyChanged is true it also
clears/invalidates targetChannelRoutingCache_ (same behavior as
clearAllRemoteSequencesOnSessionReset), then proceed to call
clearRemoteVolumeSequenceForClient for currentClientIds; reference
handleSessionSetupChange, targetChannelRoutingCache_,
clearAllRemoteSequencesOnSessionReset, and clearRemoteVolumeSequenceForClient
when making the change.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests / Docs