feat: Various stack code improvements and cleanup #305
Conversation
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
DatCaptainHorse
added
scope/front-end
scope/relay
scope/systems
WalkthroughMigration from JSON/ SafeStream to protobuf-based ProtoMessage with a oneof payload; introduced session-aware batched controller state, P2PMessageStream, varint framing, and widespread proto/type renames. Removed CUDA steps from the runner Containerfile and updated dependency versions and build scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Input Client
participant Input as Input Module
participant WebRTC as WebRTC Stream
participant Relay as Relay Server
participant Room as Room/Participants
Client->>Input: input events (button/axis)
Note over Input: collect deltas, update state
alt need full
Input->>Input: build FULL ProtoControllerStateBatch
else delta
Input->>Input: build DELTA ProtoControllerStateBatch
end
Input->>WebRTC: createMessage(ProtoControllerStateBatch) -> send ProtoMessage
WebRTC->>Relay: ProtoMessage on data channel
Relay->>Relay: decode payload by message_base.payload_type
Relay->>Room: dispatch controller state / rumble / attach as appropriate
Room->>Room: BroadcastPacketRetimed() -> participants
sequenceDiagram
participant Old as Old (per-event JSON)
participant New as New (batched protobuf)
Old->>Old: build per-event JSON message
Old->>Old: send immediately
New->>New: collect events -> compute delta
New->>New: batch -> create ProtoMessage
New->>New: send batched protobuf message
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Specific attention areas:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/input/src/mouse.ts (1)
107-145: Tear down the aggregation interval when disposing
startProcessinginstalls asetInterval, but we never keep its handle or clear it. Afterdispose()the timer keeps the instance alive and wakes up everysendInterval, which is a leak and unnecessary CPU churn. Please store the interval ID and clear it when stopping/disposing the mouse.private sendInterval = 10; // 100 updates per second + private sendIntervalId?: number; @@ - setInterval(() => { + if (this.sendIntervalId !== undefined) { + clearInterval(this.sendIntervalId); + } + this.sendIntervalId = window.setInterval(() => { if (this.connected && (this.movementX !== 0 || this.movementY !== 0)) { this.sendAggregatedMouseMove(); this.movementX = 0; this.movementY = 0; } }, this.sendInterval); } @@ public dispose() { document.exitPointerLock(); this.stop(); this.connected = false; + if (this.sendIntervalId !== undefined) { + clearInterval(this.sendIntervalId); + this.sendIntervalId = undefined; + } }
🧹 Nitpick comments (4)
packages/scripts/entrypoint.sh (1)
18-32: Function rename and permissions handling look solid, but consider a more precise name.The implementation correctly follows prior learnings (non-recursive chown to avoid mounted volume issues). However, the function name
handle_user_permissions()is somewhat misleading—the function actually manages ownership (chown), not file permissions (chmod). The comment on Line 18 is more accurate. Consider renaming toensure_user_ownership()or similar for clarity.Also verify that no remaining references to the old
chown_user_directory()function name exist in sourced scripts or other parts of the codebase.packages/play-standalone/package.json (1)
11-15: Consider documenting the shift to fixed versions.The move from semver ranges (e.g.,
^9.4.2) to pinned versions (9.5.0) improves reproducibility but reduces patch-level flexibility. If this is a deliberate pattern across the monorepo, consider documenting the rationale in a shared dependency management guide or ADR.packages/relay/internal/core/room.go (1)
48-48: Consider using== 0for clarity.Since
len()returns a non-negative value, the condition<= 0is equivalent to== 0but less idiomatic. The more conventional check would belen(room.Participants) == 0.- if len(room.Participants) <= 0 && r.LocalRooms.Has(room.ID) { + if len(room.Participants) == 0 && r.LocalRooms.Has(room.ID) {packages/relay/internal/core/state.go (1)
153-164: Consider removing or documenting the commented-out auto-connection logic.The auto-connection logic for new remote rooms is commented out, representing a behavioral change where new remote room states no longer trigger automatic stream requests. While this aligns with the PR's goal to defer broader relay protocol refactoring, leaving large blocks of commented code can lead to maintenance issues.
Consider either:
- Removing the commented code if this change is permanent
- Adding a TODO/FIXME comment explaining why it's disabled and tracking restoration
- Creating an issue to revisit this logic in the future refactoring
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
packages/relay/go.sumis excluded by!**/*.sumpackages/relay/internal/proto/messages.pb.gois excluded by!**/*.pb.gopackages/relay/internal/proto/types.pb.gois excluded by!**/*.pb.gopackages/server/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
containerfiles/runner-builder.Containerfile(2 hunks)packages/input/package.json(1 hunks)packages/input/src/controller.ts(5 hunks)packages/input/src/keyboard.ts(4 hunks)packages/input/src/messages.ts(0 hunks)packages/input/src/mouse.ts(6 hunks)packages/input/src/proto/latency_tracker_pb.ts(1 hunks)packages/input/src/proto/messages_pb.ts(3 hunks)packages/input/src/proto/types_pb.ts(11 hunks)packages/input/src/streamwrapper.ts(1 hunks)packages/input/src/utils.ts(1 hunks)packages/input/src/webrtc-stream.ts(9 hunks)packages/play-standalone/package.json(1 hunks)packages/play-standalone/src/pages/[room].astro(0 hunks)packages/relay/go.mod(4 hunks)packages/relay/internal/common/common.go(1 hunks)packages/relay/internal/common/safebufio.go(4 hunks)packages/relay/internal/connections/datachannel.go(1 hunks)packages/relay/internal/connections/messages.go(0 hunks)packages/relay/internal/core/protocol_stream.go(6 hunks)packages/relay/internal/core/room.go(1 hunks)packages/relay/internal/core/state.go(2 hunks)packages/relay/internal/shared/participant.go(1 hunks)packages/relay/internal/shared/room.go(2 hunks)packages/scripts/entrypoint.sh(3 hunks)packages/scripts/entrypoint_nestri.sh(1 hunks)packages/server/Cargo.toml(2 hunks)packages/server/src/args.rs(1 hunks)packages/server/src/args/app_args.rs(3 hunks)packages/server/src/enc_helper.rs(3 hunks)packages/server/src/input/controller.rs(3 hunks)packages/server/src/main.rs(6 hunks)packages/server/src/messages.rs(0 hunks)packages/server/src/nestrisink/imp.rs(13 hunks)packages/server/src/nestrisink/mod.rs(2 hunks)packages/server/src/p2p/p2p_protocol_stream.rs(4 hunks)packages/server/src/p2p/p2p_safestream.rs(2 hunks)packages/server/src/proto.rs(1 hunks)packages/server/src/proto/gen.rs(0 hunks)packages/server/src/proto/proto.rs(2 hunks)protobufs/messages.proto(1 hunks)protobufs/types.proto(1 hunks)
💤 Files with no reviewable changes (5)
- packages/server/src/messages.rs
- packages/play-standalone/src/pages/[room].astro
- packages/input/src/messages.ts
- packages/relay/internal/connections/messages.go
- packages/server/src/proto/gen.rs
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 286
File: packages/relay/internal/core/protocol_stream.go:64-251
Timestamp: 2025-05-28T11:30:04.000Z
Learning: The user DatCaptainHorse requested to be reminded later about refactoring the ICE candidate buffering logic in packages/relay/internal/core/protocol_stream.go into a reusable utility method. This was acknowledged as valid but deferred as readability/cleanup work. The specific logic is around lines 219-233 in the handleStreamRequest function and is duplicated across multiple handlers.
📚 Learning: 2025-10-18T11:48:40.772Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 304
File: containerfiles/runner-builder.Containerfile:114-126
Timestamp: 2025-10-18T11:48:40.772Z
Learning: In containerfiles/runner-builder.Containerfile (and similar NVIDIA artifact downloads), remind DatCaptainHorse to add SHA256 checksum verification after wget commands for CUDA runtime and driver tarballs to prevent supply-chain attacks—this was deferred from PR #304 to reduce scope.
Applied to files:
containerfiles/runner-builder.Containerfile
📚 Learning: 2025-09-14T22:18:21.939Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 303
File: packages/server/src/main.rs:252-255
Timestamp: 2025-09-14T22:18:21.939Z
Learning: In packages/server/src/main.rs, DatCaptainHorse confirmed they are only targeting CachyOS (Arch-based) containers, not multi-distro deployments, so cross-distribution compatibility concerns like guarding GStreamer properties don't apply to their use case.
Applied to files:
containerfiles/runner-builder.Containerfile
📚 Learning: 2025-07-02T01:17:32.082Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 294
File: packages/scripts/entrypoint_nestri.sh:151-157
Timestamp: 2025-07-02T01:17:32.082Z
Learning: In packages/scripts/entrypoint_nestri.sh, the variable checking pattern `${NESTRI_LAUNCH_CMD+x}` and `${NESTRI_LAUNCH_COMPOSITOR+x}` is intentionally used to allow empty-but-set variables to disable compositor or application launching, providing a three-state system: unset (defaults), empty (disabled), non-empty (custom).
Applied to files:
packages/scripts/entrypoint_nestri.sh
📚 Learning: 2025-06-16T14:33:49.121Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 294
File: packages/scripts/entrypoint_nestri.sh:70-97
Timestamp: 2025-06-16T14:33:49.121Z
Learning: In packages/scripts/entrypoint_nestri.sh, the custom _v2-entry-point file used for Steam patching is only 10 bytes in size, while Steam's original _v2-entry-point files are typically thousands of bytes. This makes truncation during the padding process a non-issue since the custom file is always much smaller than the original.
Applied to files:
packages/scripts/entrypoint_nestri.sh
📚 Learning: 2025-06-16T14:24:05.678Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 294
File: packages/scripts/_v2-entry-point:3-10
Timestamp: 2025-06-16T14:24:05.678Z
Learning: The _v2-entry-point script in packages/scripts/ is a modified entrypoint for Steam patching that intentionally replicates the behavior of the original Steam _v2-entry-point script, including its argument handling approach.
Applied to files:
packages/scripts/entrypoint_nestri.shpackages/scripts/entrypoint.sh
📚 Learning: 2025-05-28T11:23:56.529Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 286
File: packages/relay/internal/core/protocol_stream.go:349-439
Timestamp: 2025-05-28T11:23:56.529Z
Learning: The user prefers to defer non-critical improvements like explicit context cancellation when safeBRW.Receive already provides implicit error handling, especially when managing large PR scope in packages/relay/internal/core/protocol_stream.go.
Applied to files:
packages/relay/internal/common/safebufio.gopackages/relay/internal/core/protocol_stream.go
📚 Learning: 2025-07-02T00:41:53.074Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 294
File: packages/scripts/entrypoint.sh:17-20
Timestamp: 2025-07-02T00:41:53.074Z
Learning: In packages/scripts/entrypoint.sh, the chown_user_directory() function was changed to return 0 (success) even when chown fails because mounted volumes in /home (needed for Steam compatibility) will cause recursive chown operations to fail due to permission restrictions. The solution is to make chown non-recursive to only change ownership of the home directory itself, not its contents.
Applied to files:
packages/scripts/entrypoint.sh
📚 Learning: 2025-07-02T00:57:52.991Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 294
File: packages/scripts/entrypoint.sh:15-18
Timestamp: 2025-07-02T00:57:52.991Z
Learning: In packages/scripts/entrypoint.sh, the GID environment variable is properly defined in the Dockerfile (containers/runner.Containerfile) as ENV GID=1000, so the chown command using "${USER}:${GID}" works correctly and does not cause unbound variable errors.
Applied to files:
packages/scripts/entrypoint.sh
📚 Learning: 2025-06-06T13:46:50.622Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 286
File: packages/input/src/webrtc-stream.ts:0-0
Timestamp: 2025-06-06T13:46:50.622Z
Learning: In packages/input/src/webrtc-stream.ts, the memory leak issue with libp2p event listeners for "peer:connect" and "peer:disconnect" (lines 80-86) should be addressed in Part 2 PR. The fix involves storing the event handler functions as class properties and removing them in the disconnect method to prevent memory leaks.
Applied to files:
packages/input/src/streamwrapper.tspackages/input/src/webrtc-stream.tspackages/relay/internal/core/state.gopackages/input/src/keyboard.tspackages/input/src/mouse.ts
📚 Learning: 2025-08-21T13:59:03.924Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 298
File: packages/play-standalone/package.json:12-14
Timestamp: 2025-08-21T13:59:03.924Z
Learning: In the Nestri monorepo, workspace dependencies like "nestri/input" use "*" instead of "workspace:*" as a consistent pattern across packages, including in apps/www.
Applied to files:
packages/play-standalone/package.json
📚 Learning: 2025-10-18T02:51:27.203Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 304
File: packages/server/src/nestrisink/imp.rs:391-423
Timestamp: 2025-10-18T02:51:27.203Z
Learning: In packages/server/src/nestrisink/imp.rs, the rumble forwarding task (around lines 391-423) captures a cloned data channel which will break when WebRTC renegotiation is implemented. The fix is deferred for a future stability/cleanup PR. The recommended approach is to spawn a single long-lived task when the rumble receiver is set that calls get_data_channel() for each send instead of capturing the channel clone, ensuring rumble messages always target the current active channel.
Applied to files:
packages/input/src/webrtc-stream.tspackages/server/src/nestrisink/mod.rspackages/server/src/input/controller.rspackages/server/src/main.rspackages/server/src/nestrisink/imp.rs
📚 Learning: 2025-10-18T19:28:44.522Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 304
File: packages/relay/go.mod:3-3
Timestamp: 2025-10-18T19:28:44.522Z
Learning: Avoid raising critical concerns about version updates in go.mod or similar dependency files when versions are recent (within a few months of the current date). Maintainers are keeping dependencies up-to-date, and unless there's a clear security or compatibility issue visible in the codebase itself, trust their judgment on version bumps.
Applied to files:
packages/relay/go.mod
📚 Learning: 2025-10-18T04:54:30.108Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 304
File: packages/relay/internal/core/room.go:60-71
Timestamp: 2025-10-18T04:54:30.108Z
Learning: In packages/relay/internal/core/state.go, the onPeerDisconnected function has a memory leak bug: it attempts to delete rooms using `r.Rooms.Delete(peerID.String())` at lines 138-140, but rooms are keyed by room ID (ulid), not peer ID. The correct approach is to iterate through r.Rooms using Range() and delete all rooms where room.OwnerID equals the disconnected peerID. This issue is deferred for a future relay PR per DatCaptainHorse's request.
Applied to files:
packages/relay/internal/core/room.gopackages/relay/internal/shared/room.gopackages/relay/internal/core/protocol_stream.gopackages/relay/internal/core/state.go
📚 Learning: 2025-05-28T10:44:40.671Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 286
File: packages/relay/internal/core/protocol_stream.go:293-310
Timestamp: 2025-05-28T10:44:40.671Z
Learning: In WebRTC applications, RTP forwarding goroutines that call track.ReadRTP() will naturally terminate when the PeerConnection is closed or tracks are removed, as the read operation will error out and break the loop. This follows the same standard pattern as RTCP read goroutines and does not require additional cancellation mechanisms.
Applied to files:
packages/relay/internal/shared/participant.gopackages/relay/internal/shared/room.gopackages/relay/internal/core/protocol_stream.go
📚 Learning: 2025-05-28T06:08:25.177Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 286
File: packages/relay/internal/shared/participant.go:28-44
Timestamp: 2025-05-28T06:08:25.177Z
Learning: In WebRTC applications, RTCP read goroutines that call rtpSender.Read() will naturally terminate when the PeerConnection is closed or tracks are removed, as the read operation will error out and break the loop. This is a standard pattern and does not require additional cancellation mechanisms.
Applied to files:
packages/relay/internal/shared/participant.gopackages/relay/internal/shared/room.gopackages/relay/internal/core/protocol_stream.go
📚 Learning: 2025-05-28T06:19:14.732Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 286
File: packages/relay/internal/core/protocol_stream.go:507-507
Timestamp: 2025-05-28T06:19:14.732Z
Learning: In the relay stream protocol, servedConns map uses peer ID as the key (not room name) to allow serving the same room's stream to multiple viewers/receivers, enabling better scalability for multi-viewer scenarios.
Applied to files:
packages/relay/internal/shared/room.gopackages/relay/internal/core/protocol_stream.gopackages/relay/internal/core/state.go
📚 Learning: 2025-05-28T11:30:04.000Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 286
File: packages/relay/internal/core/protocol_stream.go:64-251
Timestamp: 2025-05-28T11:30:04.000Z
Learning: The user DatCaptainHorse requested to be reminded later about refactoring the ICE candidate buffering logic in packages/relay/internal/core/protocol_stream.go into a reusable utility method. This was acknowledged as valid but deferred as readability/cleanup work. The specific logic is around lines 219-233 in the handleStreamRequest function and is duplicated across multiple handlers.
Applied to files:
packages/relay/internal/core/protocol_stream.go
🧬 Code graph analysis (23)
packages/server/src/proto.rs (3)
packages/relay/internal/common/safebufio.go (1)
CreateMessageOptions(80-83)packages/relay/internal/proto/latency_tracker.pb.go (6)
ProtoLatencyTracker(77-83)ProtoLatencyTracker(96-96)ProtoLatencyTracker(111-113)ProtoTimestampEntry(25-31)ProtoTimestampEntry(44-44)ProtoTimestampEntry(59-61)packages/relay/internal/proto/messages.pb.go (6)
ProtoMessage(76-101)ProtoMessage(114-114)ProtoMessage(129-131)ProtoMessageBase(24-30)ProtoMessageBase(43-43)ProtoMessageBase(58-60)
packages/relay/internal/common/safebufio.go (2)
packages/input/src/proto/messages_pb.ts (2)
ProtoMessage(44-162)ProtoMessageBase(22-32)packages/relay/internal/proto/messages.pb.go (6)
ProtoMessage(76-101)ProtoMessage(114-114)ProtoMessage(129-131)ProtoMessageBase(24-30)ProtoMessageBase(43-43)ProtoMessageBase(58-60)
packages/scripts/entrypoint.sh (1)
packages/scripts/common.sh (1)
log(3-5)
packages/input/src/streamwrapper.ts (2)
packages/input/src/proto/messages_pb.ts (2)
ProtoMessageBase(22-32)ProtoMessage(44-162)packages/input/src/utils.ts (1)
bufbuildAdapter(17-28)
packages/input/src/controller.ts (4)
packages/input/src/proto/messages_pb.ts (1)
ProtoMessageSchema(168-169)packages/input/src/utils.ts (1)
createMessage(76-95)packages/input/src/proto/types_pb.ts (3)
ProtoControllerStateBatch(280-379)ProtoControllerStateBatchSchema(385-386)ProtoControllerRumble(231-266)packages/input/src/codes.ts (1)
controllerButtonToLinuxEventCode(115-133)
packages/input/src/webrtc-stream.ts (3)
packages/input/src/streamwrapper.ts (1)
P2PMessageStream(15-81)packages/input/src/proto/types_pb.ts (8)
RTCIceCandidateInit(416-436)ProtoICE(472-477)ProtoClientRequestRoomStream(529-539)ProtoSDP(491-496)ProtoSDPSchema(502-503)ProtoRaw(510-515)ProtoClientRequestRoomStreamSchema(545-546)ProtoICESchema(483-484)packages/input/src/utils.ts (1)
createMessage(76-95)
packages/server/src/nestrisink/mod.rs (2)
packages/input/src/proto/types_pb.ts (1)
ProtoControllerAttach(168-189)packages/relay/internal/proto/types.pb.go (3)
ProtoControllerAttach(410-417)ProtoControllerAttach(430-430)ProtoControllerAttach(445-447)
packages/input/src/utils.ts (3)
packages/input/src/proto/latency_tracker_pb.ts (2)
ProtoLatencyTrackerSchema(58-59)ProtoTimestampEntrySchema(36-37)packages/relay/internal/common/safebufio.go (1)
CreateMessageOptions(80-83)packages/input/src/proto/messages_pb.ts (3)
ProtoMessage(44-162)ProtoMessageSchema(168-169)ProtoMessageBaseSchema(38-39)
packages/server/src/p2p/p2p_protocol_stream.rs (4)
packages/server/src/nestrisink/imp.rs (1)
mpsc(391-391)packages/input/src/proto/messages_pb.ts (1)
ProtoMessage(44-162)packages/relay/internal/proto/messages.pb.go (3)
ProtoMessage(76-101)ProtoMessage(114-114)ProtoMessage(129-131)packages/server/src/nestrisink/mod.rs (1)
new(16-38)
packages/relay/internal/common/common.go (1)
packages/relay/internal/common/extensions.go (1)
RegisterExtensions(12-36)
packages/relay/internal/connections/datachannel.go (2)
packages/input/src/proto/messages_pb.ts (1)
ProtoMessage(44-162)packages/relay/internal/proto/messages.pb.go (3)
ProtoMessage(76-101)ProtoMessage(114-114)ProtoMessage(129-131)
packages/input/src/proto/messages_pb.ts (1)
packages/input/src/proto/types_pb.ts (17)
ProtoMouseMove(20-30)ProtoMouseMoveAbs(44-54)ProtoMouseWheel(68-78)ProtoMouseKeyDown(92-97)ProtoMouseKeyUp(111-116)ProtoKeyDown(130-135)ProtoKeyUp(149-154)ProtoControllerAttach(168-189)ProtoControllerDetach(203-217)ProtoControllerRumble(231-266)ProtoControllerStateBatch(280-379)ProtoICE(472-477)ProtoSDP(491-496)ProtoRaw(510-515)ProtoClientRequestRoomStream(529-539)ProtoClientDisconnected(553-563)ProtoServerPushStream(577-582)
packages/relay/internal/shared/participant.go (2)
packages/relay/internal/connections/datachannel.go (1)
NestriDataChannel(14-17)packages/relay/internal/common/crypto.go (1)
NewULID(14-16)
packages/relay/internal/shared/room.go (2)
packages/relay/internal/connections/datachannel.go (1)
NestriDataChannel(14-17)packages/relay/internal/shared/participant.go (1)
Participant(17-36)
packages/relay/internal/core/protocol_stream.go (10)
packages/input/src/proto/messages_pb.ts (1)
ProtoMessage(44-162)packages/relay/internal/proto/messages.pb.go (3)
ProtoMessage(76-101)ProtoMessage(114-114)ProtoMessage(129-131)packages/relay/internal/common/crypto.go (1)
NewULID(14-16)packages/relay/internal/common/safebufio.go (1)
CreateMessage(85-127)packages/input/src/proto/types_pb.ts (7)
ProtoClientRequestRoomStream(529-539)ProtoRaw(510-515)ProtoICE(472-477)RTCIceCandidateInit(416-436)ProtoSDP(491-496)RTCSessionDescriptionInit(448-458)ProtoServerPushStream(577-582)packages/relay/internal/proto/types.pb.go (21)
ProtoClientRequestRoomStream(1010-1016)ProtoClientRequestRoomStream(1029-1029)ProtoClientRequestRoomStream(1044-1046)ProtoRaw(965-970)ProtoRaw(983-983)ProtoRaw(998-1000)ProtoICE(875-880)ProtoICE(893-893)ProtoICE(908-910)RTCIceCandidateInit(754-762)RTCIceCandidateInit(775-775)RTCIceCandidateInit(790-792)ProtoSDP(920-925)ProtoSDP(938-938)ProtoSDP(953-955)RTCSessionDescriptionInit(822-828)RTCSessionDescriptionInit(841-841)RTCSessionDescriptionInit(856-858)ProtoServerPushStream(1116-1121)ProtoServerPushStream(1134-1134)ProtoServerPushStream(1149-1151)packages/relay/internal/common/common.go (1)
CreatePeerConnection(94-115)packages/relay/internal/shared/participant.go (1)
NewParticipant(38-57)packages/relay/internal/connections/datachannel.go (1)
NewNestriDataChannel(20-49)packages/relay/internal/common/extensions.go (2)
GetExtension(38-45)ExtensionPlayoutDelay(6-6)
packages/server/src/input/controller.rs (3)
packages/input/src/proto/types_pb.ts (1)
ProtoControllerAttach(168-189)packages/relay/internal/proto/types.pb.go (3)
ProtoControllerAttach(410-417)ProtoControllerAttach(430-430)ProtoControllerAttach(445-447)packages/server/src/nestrisink/mod.rs (1)
new(16-38)
packages/server/src/p2p/p2p_safestream.rs (1)
packages/server/src/p2p/p2p_protocol_stream.rs (2)
new(14-19)new(48-76)
packages/server/src/main.rs (4)
packages/server/src/input/controller.rs (2)
new(27-34)new(52-78)packages/server/src/args.rs (1)
new(18-237)packages/server/src/enc_helper.rs (1)
new(121-135)packages/server/src/nestrisink/mod.rs (1)
new(16-38)
packages/input/src/keyboard.ts (3)
packages/input/src/proto/types_pb.ts (2)
ProtoKeyDownSchema(141-142)ProtoKeyUpSchema(160-161)packages/input/src/utils.ts (1)
createMessage(76-95)packages/input/src/proto/messages_pb.ts (1)
ProtoMessageSchema(168-169)
packages/input/src/mouse.ts (3)
packages/input/src/proto/types_pb.ts (4)
ProtoMouseKeyDownSchema(103-104)ProtoMouseKeyUpSchema(122-123)ProtoMouseWheelSchema(84-85)ProtoMouseMoveSchema(36-37)packages/input/src/utils.ts (1)
createMessage(76-95)packages/input/src/proto/messages_pb.ts (1)
ProtoMessageSchema(168-169)
packages/input/src/proto/types_pb.ts (1)
packages/relay/internal/proto/types.pb.go (34)
ProtoControllerRumble(524-533)ProtoControllerRumble(546-546)ProtoControllerRumble(561-563)ProtoControllerStateBatch(601-624)ProtoControllerStateBatch(637-637)ProtoControllerStateBatch(652-654)ProtoControllerStateBatch_UpdateType(24-24)ProtoControllerStateBatch_UpdateType(53-55)ProtoControllerStateBatch_UpdateType(57-59)ProtoControllerStateBatch_UpdateType(66-68)RTCIceCandidateInit(754-762)RTCIceCandidateInit(775-775)RTCIceCandidateInit(790-792)RTCSessionDescriptionInit(822-828)RTCSessionDescriptionInit(841-841)RTCSessionDescriptionInit(856-858)ProtoICE(875-880)ProtoICE(893-893)ProtoICE(908-910)ProtoSDP(920-925)ProtoSDP(938-938)ProtoSDP(953-955)ProtoRaw(965-970)ProtoRaw(983-983)ProtoRaw(998-1000)ProtoClientRequestRoomStream(1010-1016)ProtoClientRequestRoomStream(1029-1029)ProtoClientRequestRoomStream(1044-1046)ProtoClientDisconnected(1063-1069)ProtoClientDisconnected(1082-1082)ProtoClientDisconnected(1097-1099)ProtoServerPushStream(1116-1121)ProtoServerPushStream(1134-1134)ProtoServerPushStream(1149-1151)
packages/server/src/proto/proto.rs (4)
packages/input/src/proto/types_pb.ts (15)
ProtoMouseMove(20-30)ProtoMouseMoveAbs(44-54)ProtoMouseWheel(68-78)ProtoMouseKeyDown(92-97)ProtoMouseKeyUp(111-116)ProtoKeyDown(130-135)ProtoKeyUp(149-154)ProtoControllerDetach(203-217)ProtoControllerRumble(231-266)ProtoControllerStateBatch(280-379)ProtoRaw(510-515)ProtoClientRequestRoomStream(529-539)ProtoClientDisconnected(553-563)ProtoServerPushStream(577-582)ProtoControllerAttach(168-189)packages/input/src/proto/messages_pb.ts (2)
ProtoMessageBase(22-32)ProtoMessage(44-162)packages/relay/internal/proto/messages.pb.go (6)
ProtoMessageBase(24-30)ProtoMessageBase(43-43)ProtoMessageBase(58-60)ProtoMessage(76-101)ProtoMessage(114-114)ProtoMessage(129-131)packages/input/src/proto/latency_tracker_pb.ts (1)
ProtoLatencyTracker(42-52)
packages/server/src/nestrisink/imp.rs (6)
packages/input/src/proto/types_pb.ts (3)
ProtoControllerAttach(168-189)ProtoControllerRumble(231-266)ProtoServerPushStream(577-582)packages/input/src/proto/messages_pb.ts (1)
ProtoMessage(44-162)packages/server/src/input/controller.rs (2)
new(27-34)new(52-78)packages/server/src/nestrisink/mod.rs (1)
new(16-38)packages/server/src/p2p/p2p_protocol_stream.rs (2)
new(14-19)new(48-76)packages/server/src/proto.rs (1)
create_message(8-35)
🪛 Buf (1.59.0)
protobufs/types.proto
79-79: Enum value name "FULL_STATE" should be prefixed with "UPDATE_TYPE_".
(ENUM_VALUE_PREFIX)
79-79: Enum zero value name "FULL_STATE" should be suffixed with "_UNSPECIFIED".
(ENUM_ZERO_VALUE_SUFFIX)
80-80: Enum value name "DELTA" should be prefixed with "UPDATE_TYPE_".
(ENUM_VALUE_PREFIX)
109-109: Field name "sdpMLineIndex" should be lower_snake_case, such as "sdp_m_line_index".
(FIELD_LOWER_SNAKE_CASE)
110-110: Field name "sdpMid" should be lower_snake_case, such as "sdp_mid".
(FIELD_LOWER_SNAKE_CASE)
111-111: Field name "usernameFragment" should be lower_snake_case, such as "username_fragment".
(FIELD_LOWER_SNAKE_CASE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build images on PR
🔇 Additional comments (21)
packages/input/package.json (2)
10-11: Minor version updates to protobuf build tools.The @bufbuild/buf and @bufbuild/protoc-gen-es versions are being incremented within the v1/v2 release lines respectively (1.57.2→1.59.0 and 2.9.0→2.10.0). These appear to be compatible maintenance updates.
14-26: Verify LibP2P v3 ecosystem compatibility and ensure code updates align with breaking changes.This represents a major version migration of the LibP2P ecosystem from v2 to v3 (e.g., libp2p v1→v3, @libp2p/identify v3→v4, @libp2p/ping v2→v3, @libp2p/websockets v9→v10, @libp2p/webtransport v5→v6). While the versions appear internally consistent within the v3 ecosystem and the PR objectives acknowledge "Updates dependencies (LibP2P v3)", major version bumps typically introduce breaking API changes.
Please verify:
- All code that depends on LibP2P v2 APIs has been updated to use the v3 equivalents (particularly stream handling, protocol interfaces, and message types).
- The protobuf messaging migration (noted in the PR objectives) is fully compatible with the new P2PMessageStream abstraction mentioned in the enriched summary.
- The caret (
^) version ranges are appropriate for production usage in this context (they allow minor updates within each major version, which should be stable within the v3 ecosystem).If you'd like, I can run a verification script to check for usage patterns of these LibP2P packages and flag any v2-specific APIs that may need updating.
packages/scripts/entrypoint.sh (4)
327-339: X11 socket directory setup is correct and well‑guarded.Mode 1777 (sticky bit + world-writable) is the correct permission for /tmp/.X11-unix, and the error handling for both mkdir and chmod is appropriate. Placement before user permission handling makes logical sense.
341-343: User permissions call is properly guarded and well‑placed.Error handling via
|| exit 1is consistent with the established pattern, and placement after directory creation but before user context switch is logical.
352-362: udev directory and control file setup is properly logged and guarded.The explicit creation of both /run/udev directory and /run/udev/control file with separate error handling is defensive and correct. The updated logging message on Line 353 clearly indicates both steps are being taken.
18-375: Overall integration and error handling are solid.The refactored entrypoint properly sequences directory creation, ownership management, and permission setup. All changes follow the established pattern: log, execute, error-exit on failure. The non-recursive chown respects prior learnings about mounted volume compatibility. The only item for improvement is the optional naming clarification on
handle_user_permissions().packages/play-standalone/package.json (1)
12-12: @astrojs/node version 9.5.0 does not exist on npm.The npm registry shows @astrojs/node's latest published version is 9.4.3. This package.json pins line 12 to 9.5.0, which will fail during installation. Update to 9.4.3 or verify the intended version. ([email protected] on line 14 is valid and was published Oct 23, 2025.)
⛔ Skipped due to learnings
Learnt from: DatCaptainHorse Repo: nestrilabs/nestri PR: 298 File: packages/play-standalone/package.json:12-14 Timestamp: 2025-08-21T13:59:03.924Z Learning: In the Nestri monorepo, the established convention is to use "*" (not "workspace:*") for nestri/ workspace dependencies across all packages. This pattern is consistently used in apps/www, packages/www, packages/ui, packages/zero, and other packages. Only packages/functions uses "workspace:" which appears to be an exception.Learnt from: DatCaptainHorse Repo: nestrilabs/nestri PR: 298 File: packages/play-standalone/package.json:12-14 Timestamp: 2025-08-21T13:59:03.924Z Learning: In the Nestri monorepo, workspace dependencies like "nestri/input" use "*" instead of "workspace:*" as a consistent pattern across packages, including in apps/www.containerfiles/runner-builder.Containerfile (3)
133-133: Critical: Invalid git clone syntax will cause build failure.The
--revflag is not a valid option forgit clone. This command will fail during the container build.Apply this diff to fix the syntax:
-RUN git clone --depth 1 --rev "a4abcfe2cffe2d33b564d1308b58504a5e3012b1" https://github.com/games-on-whales/gst-wayland-display.git +RUN git clone --depth 1 https://github.com/games-on-whales/gst-wayland-display.git && \ + cd gst-wayland-display && \ + git checkout a4abcfe2cffe2d33b564d1308b58504a5e3012b1⛔ Skipped due to learnings
Learnt from: DatCaptainHorse Repo: nestrilabs/nestri PR: 303 File: containerfiles/runner.Containerfile:89-89 Timestamp: 2025-09-11T18:12:08.158Z Learning: Git clone supports a `--rev` parameter that allows specifying a commit SHA, branch, or tag directly in the clone command, eliminating the need for separate checkout operations. This is more efficient in Dockerfiles than cloning then doing cd + git checkout.
44-44: Both pinned commits are verified as legitimate.The vimputti commit is from the project owner (DatCaptainHorse) with a recent fix, and the gst-wayland-display commit is a verified merge from the project's collaborators. No concerns.
44-44: Critical: Invalid git clone syntax will cause build failure.The
--revflag is not a valid option forgit clone. This command will fail during the container build.Apply this diff to fix the syntax:
-RUN git clone --depth 1 --rev "2fde5376b6b9a38cdbd94ccc6a80c9d29a81a417" https://github.com/DatCaptainHorse/vimputti.git +RUN git clone --depth 1 https://github.com/DatCaptainHorse/vimputti.git && \ + cd vimputti && \ + git checkout 2fde5376b6b9a38cdbd94ccc6a80c9d29a81a417⛔ Skipped due to learnings
Learnt from: DatCaptainHorse Repo: nestrilabs/nestri PR: 304 File: containerfiles/runner-builder.Containerfile:114-126 Timestamp: 2025-10-18T11:48:40.772Z Learning: In containerfiles/runner-builder.Containerfile (and similar NVIDIA artifact downloads), remind DatCaptainHorse to add SHA256 checksum verification after wget commands for CUDA runtime and driver tarballs to prevent supply-chain attacks—this was deferred from PR #304 to reduce scope.Learnt from: DatCaptainHorse Repo: nestrilabs/nestri PR: 303 File: containerfiles/runner.Containerfile:89-89 Timestamp: 2025-09-11T18:12:08.158Z Learning: Git clone supports a `--rev` parameter that allows specifying a commit SHA, branch, or tag directly in the clone command, eliminating the need for separate checkout operations. This is more efficient in Dockerfiles than cloning then doing cd + git checkout.Learnt from: DatCaptainHorse Repo: nestrilabs/nestri PR: 303 File: containerfiles/runner.Containerfile:136-136 Timestamp: 2025-09-11T18:02:39.225Z Learning: In containerfiles/runner.Containerfile SSH configuration, DatCaptainHorse confirms that key-only SSH authentication is intentionally configured (PasswordAuthentication no, PermitRootLogin no) and openssh was already present in the container - this is the intended security policy, not an issue to flag.packages/scripts/entrypoint_nestri.sh (1)
190-190: I need to examine the script context to understand whether the inconsistency matters:
WAYLAND_DISPLAY supports absolute paths as the Wayland socket path since Wayland 1.15. The change from
wayland-0(relative) to${XDG_RUNTIME_DIR}/wayland-0(absolute) is valid and will work correctly. When WAYLAND_DISPLAY contains an absolute path, it is used as-is rather than concatenated with XDG_RUNTIME_DIR.The inconsistency between lines 180/185 using relative names and line 190 using an absolute path poses no functional issue—both formats resolve to the same socket and are compatible with modern Wayland clients.
packages/server/src/args.rs (1)
214-221: LGTM! The new software-render flag follows existing patterns.The flag definition is consistent with other boolean flags in the codebase and properly integrated into the argument parsing flow.
packages/server/src/args/app_args.rs (1)
18-19: LGTM! The software_render field is properly integrated.The field declaration, initialization, and debug logging follow the established patterns in the codebase, mirroring the implementation of similar flags like
zero_copy.Also applies to: 57-60, 83-83
packages/relay/internal/common/common.go (1)
29-29: LGTM! Removes unnecessary variable shadowing.The change to reuse the outer
errvariable instead of declaring a new one is a minor cleanup that improves code clarity.packages/relay/internal/core/state.go (1)
132-132: LGTM! Comment simplification.The comment update is clear and doesn't affect functionality.
packages/server/src/p2p/p2p_safestream.rs (3)
10-10: LGTM! Separate write stream field enables better concurrency.Extracting the write half as a separate field allows independent read/write operations with finer-grained locking.
29-44: LGTM! Varint encoding implementation is correct.The migration to varint-based length prefixes follows standard protobuf conventions and is properly implemented using the
unsigned_varintcrate.
46-77: Protocol compatibility verified—varint implementation is correct and centralized.Verification confirms the symmetric encoding/decoding implementation using the
unsigned_varintcrate. All protocol traffic flows through the centralizedSafeStream(accessed at lines 99 and 150 inp2p_protocol_stream.rs), ensuring consistent varint-based framing across the codebase. No legacy encoding schemes detected.packages/server/src/nestrisink/mod.rs (2)
22-22: LGTM! The attach_rx parameter is properly integrated.The new
attach_rxparameter follows the same pattern as the existingrumble_rxhandling and aligns with the broader protobuf-based controller signaling changes in the PR.Also applies to: 34-36
21-21: Type change is correctly propagated throughout the codebase.The
rumble_rxparameter type change from 4-tuple to 5-tuple with addedStringfield has been consistently applied:
- Senders (controller.rs): Both
rumble_tx.try_send()calls correctly construct 5-tuples with(slot, strong, weak, duration_ms, session_id)wheresession_idis a clonedString- Type signatures: All declarations match
(u32, u16, u16, u16, String)—inControllerManager,NestriSignaller, and internal structures- Receiver (imp.rs): Properly destructures all 5 elements from the channel
- Caller (main.rs): Single call to
NestriSignaller::new()correctly forwards the receiver without modificationAll compilation points have been updated; no breaking changes remain unaddressed.
protobufs/messages.proto (1)
15-41: Verification successful: ProtoMessage migration is complete across all components.All message producers and consumers have been updated:
- ✅ Producers: keyboard, mouse, controller, and server all creating new
ProtoMessageformat withmessageBaseandoneof payload- ✅ Consumers: p2p_protocol_stream and nestrisink properly decoding messages
- ✅ Bindings: Generated TypeScript and Rust protobuf code correctly reflects the new structure
- ✅ Payload derivation: Dynamic case derivation via
$typeNameeliminates hardcoded message type string brittleness- ✅ No legacy references: Zero
ProtoMessageInputreferences found; old type successfully removedThe breaking protocol change has been coordinated across server, relay, and client components as intended.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/input/src/controller.ts(5 hunks)packages/input/src/webrtc-stream.ts(9 hunks)packages/relay/internal/common/ice_helper.go(1 hunks)packages/relay/internal/core/protocol_stream.go(6 hunks)packages/relay/internal/shared/participant.go(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 286
File: packages/relay/internal/core/protocol_stream.go:64-251
Timestamp: 2025-05-28T11:30:04.000Z
Learning: The user DatCaptainHorse requested to be reminded later about refactoring the ICE candidate buffering logic in packages/relay/internal/core/protocol_stream.go into a reusable utility method. This was acknowledged as valid but deferred as readability/cleanup work. The specific logic is around lines 219-233 in the handleStreamRequest function and is duplicated across multiple handlers.
📚 Learning: 2025-05-28T11:23:56.529Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 286
File: packages/relay/internal/core/protocol_stream.go:349-439
Timestamp: 2025-05-28T11:23:56.529Z
Learning: The user prefers to defer non-critical improvements like explicit context cancellation when safeBRW.Receive already provides implicit error handling, especially when managing large PR scope in packages/relay/internal/core/protocol_stream.go.
Applied to files:
packages/relay/internal/core/protocol_stream.gopackages/relay/internal/shared/participant.gopackages/input/src/webrtc-stream.tspackages/input/src/controller.ts
📚 Learning: 2025-05-28T11:30:04.000Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 286
File: packages/relay/internal/core/protocol_stream.go:64-251
Timestamp: 2025-05-28T11:30:04.000Z
Learning: The user DatCaptainHorse requested to be reminded later about refactoring the ICE candidate buffering logic in packages/relay/internal/core/protocol_stream.go into a reusable utility method. This was acknowledged as valid but deferred as readability/cleanup work. The specific logic is around lines 219-233 in the handleStreamRequest function and is duplicated across multiple handlers.
Applied to files:
packages/relay/internal/core/protocol_stream.gopackages/relay/internal/shared/participant.gopackages/input/src/webrtc-stream.tspackages/relay/internal/common/ice_helper.gopackages/input/src/controller.ts
📚 Learning: 2025-05-28T06:19:14.732Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 286
File: packages/relay/internal/core/protocol_stream.go:507-507
Timestamp: 2025-05-28T06:19:14.732Z
Learning: In the relay stream protocol, servedConns map uses peer ID as the key (not room name) to allow serving the same room's stream to multiple viewers/receivers, enabling better scalability for multi-viewer scenarios.
Applied to files:
packages/relay/internal/core/protocol_stream.go
📚 Learning: 2025-06-06T13:44:32.988Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 286
File: packages/relay/internal/common/flags.go:20-21
Timestamp: 2025-06-06T13:44:32.988Z
Learning: In packages/relay/internal/common/flags.go, the WebRTCUDPStart and WebRTCUDPEnd defaults were intentionally changed from 10000/20000 to 0/0 as part of the libp2p migration. These are optional flags that can be configured when specific firewall or network constraints require fixed UDP port ranges, but the default behavior now uses system-allocated ephemeral ports.
Applied to files:
packages/relay/internal/core/protocol_stream.go
📚 Learning: 2025-10-18T04:54:30.108Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 304
File: packages/relay/internal/core/room.go:60-71
Timestamp: 2025-10-18T04:54:30.108Z
Learning: In packages/relay/internal/core/state.go, the onPeerDisconnected function has a memory leak bug: it attempts to delete rooms using `r.Rooms.Delete(peerID.String())` at lines 138-140, but rooms are keyed by room ID (ulid), not peer ID. The correct approach is to iterate through r.Rooms using Range() and delete all rooms where room.OwnerID equals the disconnected peerID. This issue is deferred for a future relay PR per DatCaptainHorse's request.
Applied to files:
packages/relay/internal/core/protocol_stream.gopackages/relay/internal/shared/participant.gopackages/input/src/webrtc-stream.tspackages/input/src/controller.ts
📚 Learning: 2025-06-06T13:46:50.622Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 286
File: packages/input/src/webrtc-stream.ts:0-0
Timestamp: 2025-06-06T13:46:50.622Z
Learning: In packages/input/src/webrtc-stream.ts, the memory leak issue with libp2p event listeners for "peer:connect" and "peer:disconnect" (lines 80-86) should be addressed in Part 2 PR. The fix involves storing the event handler functions as class properties and removing them in the disconnect method to prevent memory leaks.
Applied to files:
packages/relay/internal/core/protocol_stream.gopackages/relay/internal/shared/participant.gopackages/input/src/webrtc-stream.tspackages/input/src/controller.ts
📚 Learning: 2025-10-18T02:51:27.203Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 304
File: packages/server/src/nestrisink/imp.rs:391-423
Timestamp: 2025-10-18T02:51:27.203Z
Learning: In packages/server/src/nestrisink/imp.rs, the rumble forwarding task (around lines 391-423) captures a cloned data channel which will break when WebRTC renegotiation is implemented. The fix is deferred for a future stability/cleanup PR. The recommended approach is to spawn a single long-lived task when the rumble receiver is set that calls get_data_channel() for each send instead of capturing the channel clone, ensuring rumble messages always target the current active channel.
Applied to files:
packages/relay/internal/core/protocol_stream.gopackages/relay/internal/shared/participant.gopackages/input/src/webrtc-stream.tspackages/input/src/controller.ts
📚 Learning: 2025-05-28T06:08:25.177Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 286
File: packages/relay/internal/shared/participant.go:28-44
Timestamp: 2025-05-28T06:08:25.177Z
Learning: In WebRTC applications, RTCP read goroutines that call rtpSender.Read() will naturally terminate when the PeerConnection is closed or tracks are removed, as the read operation will error out and break the loop. This is a standard pattern and does not require additional cancellation mechanisms.
Applied to files:
packages/relay/internal/core/protocol_stream.gopackages/relay/internal/shared/participant.go
📚 Learning: 2025-05-28T10:44:40.671Z
Learnt from: DatCaptainHorse
Repo: nestrilabs/nestri PR: 286
File: packages/relay/internal/core/protocol_stream.go:293-310
Timestamp: 2025-05-28T10:44:40.671Z
Learning: In WebRTC applications, RTP forwarding goroutines that call track.ReadRTP() will naturally terminate when the PeerConnection is closed or tracks are removed, as the read operation will error out and break the loop. This follows the same standard pattern as RTCP read goroutines and does not require additional cancellation mechanisms.
Applied to files:
packages/relay/internal/core/protocol_stream.gopackages/relay/internal/shared/participant.go
🧬 Code graph analysis (4)
packages/relay/internal/core/protocol_stream.go (9)
packages/relay/internal/common/ice_helper.go (1)
NewICEHelper(16-21)packages/relay/internal/proto/messages.pb.go (3)
ProtoMessage(76-101)ProtoMessage(114-114)ProtoMessage(129-131)packages/relay/internal/common/crypto.go (1)
NewULID(14-16)packages/relay/internal/common/safebufio.go (1)
CreateMessage(85-127)packages/relay/internal/proto/types.pb.go (21)
ProtoClientRequestRoomStream(1010-1016)ProtoClientRequestRoomStream(1029-1029)ProtoClientRequestRoomStream(1044-1046)ProtoRaw(965-970)ProtoRaw(983-983)ProtoRaw(998-1000)ProtoICE(875-880)ProtoICE(893-893)ProtoICE(908-910)RTCIceCandidateInit(754-762)RTCIceCandidateInit(775-775)RTCIceCandidateInit(790-792)ProtoSDP(920-925)ProtoSDP(938-938)ProtoSDP(953-955)RTCSessionDescriptionInit(822-828)RTCSessionDescriptionInit(841-841)RTCSessionDescriptionInit(856-858)ProtoServerPushStream(1116-1121)ProtoServerPushStream(1134-1134)ProtoServerPushStream(1149-1151)packages/relay/internal/common/common.go (1)
CreatePeerConnection(94-115)packages/relay/internal/shared/participant.go (1)
NewParticipant(38-57)packages/relay/internal/connections/datachannel.go (1)
NewNestriDataChannel(20-49)packages/relay/internal/common/extensions.go (1)
GetExtension(38-45)
packages/relay/internal/shared/participant.go (2)
packages/relay/internal/connections/datachannel.go (1)
NestriDataChannel(14-17)packages/relay/internal/common/crypto.go (1)
NewULID(14-16)
packages/input/src/webrtc-stream.ts (3)
packages/input/src/streamwrapper.ts (1)
P2PMessageStream(15-81)packages/input/src/proto/types_pb.ts (8)
RTCIceCandidateInit(416-436)ProtoICE(472-477)ProtoClientRequestRoomStream(529-539)ProtoSDP(491-496)ProtoSDPSchema(502-503)ProtoRaw(510-515)ProtoClientRequestRoomStreamSchema(545-546)ProtoICESchema(483-484)packages/input/src/utils.ts (1)
createMessage(76-95)
packages/input/src/controller.ts (5)
packages/input/src/webrtc-stream.ts (1)
WebRTCStream(24-454)packages/input/src/proto/messages_pb.ts (1)
ProtoMessageSchema(168-169)packages/input/src/utils.ts (1)
createMessage(76-95)packages/input/src/proto/types_pb.ts (5)
ProtoControllerAttachSchema(195-196)ProtoControllerStateBatch(280-379)ProtoControllerStateBatchSchema(385-386)ProtoControllerDetachSchema(223-224)ProtoControllerRumble(231-266)packages/input/src/codes.ts (1)
controllerButtonToLinuxEventCode(115-133)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build images on PR
Comment on lines
+142
to
159
| await this._pc!.setRemoteDescription({ | ||
| sdp: data.sdp.sdp, | ||
| type: data.sdp.type as RTCSdpType, | ||
| }); | ||
| // Create our answer | ||
| const answer = await this._pc!.createAnswer(); | ||
| // Force stereo in Chromium browsers | ||
| answer.sdp = this.forceOpusStereo(answer.sdp!); | ||
| await this._pc!.setLocalDescription(answer); | ||
| // Send answer back | ||
| const answerMsg = NewMessageSDP("answer", answer); | ||
| await this._p2pSafeStream?.writeMessage(answerMsg); | ||
| const answerMsg = createMessage( | ||
| create(ProtoSDPSchema, { | ||
| sdp: answer, | ||
| }), | ||
| "answer", | ||
| ); | ||
| await this._msgStream?.write(answerMsg); | ||
| }); |
There was a problem hiding this comment.
Flush buffered ICE immediately after setting the remote description
We buffer candidates in iceHolder until the offer lands, but we never replay them once setRemoteDescription succeeds. If all remote candidates arrived before the SDP, the list stays unflushed and ICE never completes. Please drain iceHolder right after setRemoteDescription (same error logging pattern is fine).
- await this._pc!.setRemoteDescription({
- sdp: data.sdp.sdp,
- type: data.sdp.type as RTCSdpType,
- });
+ await this._pc!.setRemoteDescription({
+ sdp: data.sdp.sdp,
+ type: data.sdp.type as RTCSdpType,
+ });
+ if (iceHolder.length > 0) {
+ for (const candidate of iceHolder) {
+ this._pc!
+ .addIceCandidate(candidate)
+ .catch((err) => console.error("Error adding held ICE candidate:", err));
+ }
+ iceHolder = [];
+ }📝 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.
Suggested change
| await this._pc!.setRemoteDescription({ | |
| sdp: data.sdp.sdp, | |
| type: data.sdp.type as RTCSdpType, | |
| }); | |
| // Create our answer | |
| const answer = await this._pc!.createAnswer(); | |
| // Force stereo in Chromium browsers | |
| answer.sdp = this.forceOpusStereo(answer.sdp!); | |
| await this._pc!.setLocalDescription(answer); | |
| // Send answer back | |
| const answerMsg = NewMessageSDP("answer", answer); | |
| await this._p2pSafeStream?.writeMessage(answerMsg); | |
| const answerMsg = createMessage( | |
| create(ProtoSDPSchema, { | |
| sdp: answer, | |
| }), | |
| "answer", | |
| ); | |
| await this._msgStream?.write(answerMsg); | |
| }); | |
| await this._pc!.setRemoteDescription({ | |
| sdp: data.sdp.sdp, | |
| type: data.sdp.type as RTCSdpType, | |
| }); | |
| if (iceHolder.length > 0) { | |
| for (const candidate of iceHolder) { | |
| this._pc! | |
| .addIceCandidate(candidate) | |
| .catch((err) => console.error("Error adding held ICE candidate:", err)); | |
| } | |
| iceHolder = []; | |
| } | |
| // Create our answer | |
| const answer = await this._pc!.createAnswer(); | |
| // Force stereo in Chromium browsers | |
| answer.sdp = this.forceOpusStereo(answer.sdp!); | |
| await this._pc!.setLocalDescription(answer); | |
| // Send answer back | |
| const answerMsg = createMessage( | |
| create(ProtoSDPSchema, { | |
| sdp: answer, | |
| }), | |
| "answer", | |
| ); | |
| await this._msgStream?.write(answerMsg); | |
| }); |
🤖 Prompt for AI Agents
In packages/input/src/webrtc-stream.ts around lines 142 to 159, after
successfully calling await this._pc!.setRemoteDescription({...}) you must
immediately flush any buffered ICE candidates stored in iceHolder by iterating
over iceHolder, adding each candidate to this._pc via
this._pc!.addIceCandidate(candidate) (awaiting each or Promise.all as
appropriate), catching and logging errors using the same processLogger.error
pattern, and then clear iceHolder so the buffered candidates are not replayed
later.
Comment on lines
+273
to
+285
| candInit := candidate.ToJSON() | ||
| biggified := uint32(*candInit.SDPMLineIndex) | ||
| iceMsg, err := common.CreateMessage( | ||
| &gen.ProtoICE{ | ||
| Candidate: &gen.RTCIceCandidateInit{ | ||
| Candidate: candInit.Candidate, | ||
| SdpMLineIndex: &biggified, | ||
| SdpMid: candInit.SDPMid, | ||
| }, | ||
| }, | ||
| "ice-candidate", nil, | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
Guard SDPMLineIndex before dereferencing
candidate.ToJSON() can legitimately return a nil SDPMLineIndex (Chrome does this for end-of-candidates and some ICE restarts). Dereferencing it panics the relay, killing the session. Please lift the value behind a nil-check, and only populate the proto field when present (same fix needed in the push-side handler below).
- candInit := candidate.ToJSON()
- biggified := uint32(*candInit.SDPMLineIndex)
- iceMsg, err := common.CreateMessage(
- &gen.ProtoICE{
- Candidate: &gen.RTCIceCandidateInit{
- Candidate: candInit.Candidate,
- SdpMLineIndex: &biggified,
- SdpMid: candInit.SDPMid,
- },
- },
+ candInit := candidate.ToJSON()
+ var sdpMLineIndex *uint32
+ if candInit.SDPMLineIndex != nil {
+ idx := uint32(*candInit.SDPMLineIndex)
+ sdpMLineIndex = &idx
+ }
+ iceMsg, err := common.CreateMessage(
+ &gen.ProtoICE{
+ Candidate: &gen.RTCIceCandidateInit{
+ Candidate: candInit.Candidate,
+ SdpMLineIndex: sdpMLineIndex,
+ SdpMid: candInit.SDPMid,
+ },
+ },Also applies to: 546-556
🤖 Prompt for AI Agents
In packages/relay/internal/core/protocol_stream.go around lines 273-285 (and
similarly for lines 546-556), candidate.ToJSON() can return a nil SDPMLineIndex
which is currently being dereferenced and converted to uint32 causing a panic;
update the code to check if candInit.SDPMLineIndex != nil before dereferencing,
only populate the ProtoICE.RTCIceCandidateInit.SdpMLineIndex field when the
pointer is non-nil (convert the value to uint32 when present), and leave
SdpMLineIndex unset (nil) otherwise; apply the same nil-guard and conditional
population to the push-side handler at the referenced lines.
Comment on lines
+339
to
+347
| iceMsg := msgWrapper.GetIce() | ||
| if iceMsg != nil { | ||
| smollified := uint16(*iceMsg.Candidate.SdpMLineIndex) | ||
| cand := webrtc.ICECandidateInit{ | ||
| Candidate: iceMsg.Candidate.Candidate, | ||
| SDPMid: iceMsg.Candidate.SdpMid, | ||
| SDPMLineIndex: &smollified, | ||
| UsernameFragment: iceMsg.Candidate.UsernameFragment, | ||
| } |
There was a problem hiding this comment.
Handle missing SdpMLineIndex on incoming candidates
Remote peers are allowed to omit sdpMLineIndex. The current uint16(*iceMsg.Candidate.SdpMLineIndex) dereference will panic when that happens, so the relay drops the whole stream. Please populate the Pion candidate only when the field is present (and keep the rest of the logic unchanged). Apply the same guard in the push path below.
- smollified := uint16(*iceMsg.Candidate.SdpMLineIndex)
- cand := webrtc.ICECandidateInit{
- Candidate: iceMsg.Candidate.Candidate,
- SDPMid: iceMsg.Candidate.SdpMid,
- SDPMLineIndex: &smollified,
- UsernameFragment: iceMsg.Candidate.UsernameFragment,
- }
+ cand := webrtc.ICECandidateInit{
+ Candidate: iceMsg.Candidate.Candidate,
+ SDPMid: iceMsg.Candidate.SdpMid,
+ UsernameFragment: iceMsg.Candidate.UsernameFragment,
+ }
+ if iceMsg.Candidate.SdpMLineIndex != nil {
+ smollified := uint16(*iceMsg.Candidate.SdpMLineIndex)
+ cand.SDPMLineIndex = &smollified
+ }Also applies to: 458-466
🤖 Prompt for AI Agents
In packages/relay/internal/core/protocol_stream.go around lines 339-347 (and
similarly for the push-path around lines 458-466), the code dereferences
iceMsg.Candidate.SdpMLineIndex unguarded which will panic when the remote peer
omits sdpMLineIndex; update both places to check that
iceMsg.Candidate.SdpMLineIndex != nil before converting and assigning it (i.e.,
only set SDPMLineIndex on the webrtc.ICECandidateInit when the pointer is
non-nil, converting to uint16 as currently done), leaving all other candidate
fields and logic unchanged.
Comment on lines
+84
to
+96
| if p.DataChannel != nil { | ||
| err := p.DataChannel.Close() | ||
| if err != nil { | ||
| slog.Error("Failed to close Participant DataChannel", err) | ||
| } | ||
| p.DataChannel = nil | ||
| } | ||
| if p.PeerConnection != nil { | ||
| err := p.PeerConnection.Close() | ||
| if err != nil { | ||
| slog.Error("Failed to close Participant PeerConnection", err) | ||
| } | ||
| p.PeerConnection = nil |
There was a problem hiding this comment.
Fix structured slog.Error calls in Close
slog.Error needs key/value pairs. Passing the bare err argument yields a !BADKEY record and drops the actual error, so we lose the failure details for both the data channel and the peer connection. Please log with explicit keys (and keep the participant ID for context).
- if p.DataChannel != nil {
- err := p.DataChannel.Close()
- if err != nil {
- slog.Error("Failed to close Participant DataChannel", err)
- }
+ if p.DataChannel != nil {
+ if err := p.DataChannel.Close(); err != nil {
+ slog.Error("Failed to close Participant DataChannel", "participant", p.ID, "err", err)
+ }
p.DataChannel = nil
}
- if p.PeerConnection != nil {
- err := p.PeerConnection.Close()
- if err != nil {
- slog.Error("Failed to close Participant PeerConnection", err)
- }
+ if p.PeerConnection != nil {
+ if err := p.PeerConnection.Close(); err != nil {
+ slog.Error("Failed to close Participant PeerConnection", "participant", p.ID, "err", err)
+ }
p.PeerConnection = nil
}🤖 Prompt for AI Agents
In packages/relay/internal/shared/participant.go around lines 84 to 96, the
slog.Error calls pass the raw err value which produces !BADKEY and drops the
error; update both error logs to use structured key/value pairs (include the
participant ID for context) — e.g. call slog.Error with a message and keys such
as "participant_id" or "participant", and an error key like "err" or use
slog.Any("err", err) (or slog.String/Err helpers if available) so the actual
error is recorded, then keep clearing the fields as before.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
First commit
Restructured protobuf schemas to make them easier to use across languages, switched to using them in-place of JSON for signaling as well, so there's no 2 different message formats flying about. Few new message types to deal with clients and nestri-servers better (not final format, may see changes still).
General cleanup of dead/unused code along some bug squashing and package updates.
TODO for future commits:
Restructure relay protocols code a bit, to reduce bloatiness of the currently single file for them, more code re-use.Try to fix issue where with multiple clients, static stream content causes video to freeze until there's some movement.throughput-performance, causing CPU latency to be too high.Second + third commit
Redid the controller polling handling and fixed multi-controller handling in vimputti and nestri code sides. Remove some dead relay code as well to clean up the protocol source file, we'll revisit the meshing functionality later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores