[erlendjones]

About the Contributor

This pull request is posted on behalf of Bitfocus (Bitfocus Buttons / Companion, which uses atem-connection through the bmd-atem module). Contact: @erlendjones.

Type of Contribution

This is a: Bug fix

Current Behavior

AtemSocketChild._receivePacket only accepts an AckRequest packet if it is exactly lastReceivedPacketId + 1. Any packet that arrives ahead of sequence is silently dropped.

Over VPN the responsiveness gets bogged down. A single lost or reordered packet is enough to push the connection into a backlog it never recovers from:

• The ATEM resends its unacked window once per frame, and each burst contains only one packet we can accept.
• The ATEM also sends a new Time packet every frame, even without timecode being requested.
• We therefore catch up by one packet per frame, which is exactly the arrival rate, so the backlog never drains.
• Every state change (for example PrgI/PrvI/tally after a cut) adds to the backlog permanently.

We confirmed this on a live ATEM reached over a VPN:

• Our acks trailed the ATEM's newest packet id by ~22 packets and advanced by exactly one every ~20 ms.
• Every packet was resent 7–8 times.
• The time from a command to the resulting state update grew from ~150 ms to 400 ms+ within a minute of cutting. After running a while the backlog reached ~180 packets (~3.6 s of lag).
• Only a reconnect (new session) cleared it.

New Behavior

• AckRequest packets up to 256 ids ahead of sequence are buffered (wrap-safe distance; duplicates ignored).
• When the missing packet arrives, the buffered packets that follow it are delivered in order, and the ack for the new last id is sent immediately. This stops the ATEM retransmitting the window.
• The buffer is cleared on a new session, restartConnection and disconnect.
• Packets further ahead than the window are still dropped, as before. Retransmits of already-acked packets are re-acked, as before.
• 256 comfortably covers the unacked window seen in practice (~20–30 packets after one loss) and bounds memory to < 512 KB.

Testing Instructions

• yarn unit. There are 6 new tests in socket-child.spec.ts, and all of them fail on main:
  • a gap filled later, also across the packet id wrap;
  • retransmits of buffered packets are not delivered twice;
  • packets too far ahead are not buffered;
  • the buffer is cleared by a new session;
  • a replay of the per-frame retransmit pattern from the capture. On main the backlog stays at exactly 22 packets for every frame; with this change it drains to 0.
• Manual: connect to an ATEM over a lossy link or VPN and switch preview/program repeatedly. Before this change, packet capture shows the acks trailing the ATEM's packet ids by a stable, growing offset. With it, the backlog stays at 0–1 and retransmits disappear.

Other Information

We tested the same logic against a live ATEM over VPN, patched into the atem-connection bundled in bmd-atem 3.20.1. The backlog stayed at ~0 while cutting, where before it grew with every cut.

Status

• PR is ready to be reviewed.
• The functionality has been tested by the author.
• Relevant unit tests has been added / updated.
• Relevant documentation (code comments, system documentation) has been added / updated.

🤖 Generated with Claude Code

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 284e1310-6b3b-4268-864a-d53dc6a792af

📥 Commits

Reviewing files that changed from the base of the PR and between 8d5ac99 and d916c05.

📒 Files selected for processing (2)

• src/lib/__tests__/socket-child.spec.ts
• src/lib/atemSocketChild.ts

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.

---

Walkthrough

AtemSocketChild buffers eligible out-of-order inbound AckRequest packets and delivers them in sequence when gaps close. It reconnects when a gap remains stalled for the connection timeout, even if other packets arrive. It clears buffered packets on session and connection changes.

Changes

Inbound packet sequencing

Layer / File(s)	Summary
Bounded packet sequencing and acknowledgments src/lib/atemSocketChild.ts, src/lib/__tests__/socket-child.spec.ts	AtemSocketChild buffers packets up to 256 positions ahead and delivers consecutive buffered packets after a gap closes. It sends an immediate acknowledgment after draining buffered packets. Tests cover ordering, wraparound, duplicates, the buffer limit, and burst delivery.
Gap-stall reconnect and buffer reset src/lib/atemSocketChild.ts, src/lib/__tests__/socket-child.spec.ts	AtemSocketChild reconnects when a packet gap remains stalled for the connection timeout, even if other packets arrive. It clears buffered packets and the stall timestamp on disconnect, reconnect, and a new session. Tests cover stall timing and buffer resets.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to d916c

The change buffers eligible out-of-order packets and reconnects on stalled gaps. No actionable merge-blocking issue is established; merge after normal checks pass.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to d916c

The buffer improves recovery from reordered traffic, but it also lets an unvalidated sender place packets into a future delivery window. Those packets could later replace legitimate device updates. Memory bounds and session cleanup limit the impact, while actual network exposure remains unknown.

Retained concerns

• Medium · security · inferred: Buffering broadens the opportunity for unauthenticated state injection. A sender able to reach the listener can prepopulate an eligible forward packet ID; the first payload is retained, duplicates are ignored, and closing the gap delivers that payload through the existing command/state path. Base discarded such early packets. Peer and session validation were already absent, but delayed retention now allows an injected packet to win before its legitimate counterpart arrives. Clearing the buffer at transitions prevents retained entries from crossing a reset, but does not reject mismatched-session packets arriving afterward.

Security review details

Security Blast Radius

• inferred — The demonstrated scope is one reachable socket instance and its associated device-state model, events, and data-transfer command ingress. Exploitation requires UDP reachability and a packet ID in the accepted window; the code does not require possession of a validated active-session identity. Cross-instance, tenant, infrastructure, or credential effects are not established.

Security Findings and Attack Paths

• inferred — A reachable sender can submit a forged command packet ahead of the current sequence. Head retains its payload until legitimate traffic closes the gap, then delivers and acknowledges it; a later legitimate packet with the same ID is suppressed as a duplicate or already received. This widens the injection timing window compared with base, which required the forged packet to arrive exactly next in sequence.

Trust Boundaries and Controls

• observed — Established-state gating, length checks, 15-bit packet IDs, sequence ordering, and transition cleanup constrain delivery. They do not establish sender identity: the receive path neither compares rinfo address/port nor checks the packet session against an established session before retaining payloads.

Resilience and Maintainability Implications

• observed — Retention is bounded by the forward window and the validated datagram length, keeping packet storage below approximately 512 KiB per socket. Duplicate IDs do not grow storage, and connection/session resets release retained payloads. Consumer rejection is logged without replay or rollback, as in base; ordered invocation should not be interpreted as atomic application-state delivery.

Hardening Proposals

• proposed — Bind inbound acceptance to the configured peer and established session before updating sequencing state or storing payloads, subject to protocol compatibility. Validate mismatched-peer/session packets arriving both before a gap closes and after a session reset. Endpoint checks would reduce exposure but would not provide cryptographic authentication.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: buffering out-of-order inbound packets instead of dropping them.
Description check	✅ Passed	The description directly explains the packet-ordering bug, the buffering behavior, the reconnect handling, and the related tests.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!