Skip to content

chore(srtla-send-rs): set SRTLA_CUTOVER_VERSION to 2026.6.2#14

Closed
andrescera wants to merge 45 commits into
irlserver:mainfrom
CERALIVE:main
Closed

chore(srtla-send-rs): set SRTLA_CUTOVER_VERSION to 2026.6.2#14
andrescera wants to merge 45 commits into
irlserver:mainfrom
CERALIVE:main

Conversation

@andrescera

@andrescera andrescera commented Jun 16, 2026

Copy link
Copy Markdown

IGNORE<< I miss clicked

andrescera and others added 30 commits June 12, 2026 15:32
Bootstrap the CERALIVE fork of irlserver/srtla_send (the Rust SRTLA sender)
as a sibling repo with a reproducible, green baseline.

- Pin rust-toolchain.toml to an exact nightly (nightly-2026-06-12, rustc
  1.98.0-nightly b30f3df3b) with rustfmt/clippy/rust-src + the
  aarch64-unknown-linux-gnu target. Upstream CI floats on `nightly`; the
  device image needs a reproducible build, so the fork pins a date.
- Rewrite AGENTS.md to the sibling-repo pattern: fork role, origin/upstream
  remote topology, MANUAL + COMPAT-GATED upstream-merge policy (no auto-sync),
  the device CLI + telemetry parity contract, the pinned-toolchain bump rule,
  and the no-bindings/ invariant (TS bindings stay in srtla/).
- Green the baseline with a MECHANICAL-ONLY pass: upstream HEAD (80cd0c4) is
  red on its own CI ("Check formatting"), so apply `cargo fmt` + a
  `clippy --fix` collapsible_if -> let-chain rewrite. Behavior is byte-
  identical (5 files, no logic change); full gate now passes on the pin —
  build --release, fmt --check, clippy -D warnings, and 290 tests
  (--all-features / test-internals), 0 failed, 0 ignored.
- gitignore the local orchestration scratch dir (Rule D).
Honor the CeraLive srtla_send positional contract <listen_port> <srtla_host> <srtla_port> <ips_file> exactly as CeraUI's buildSrtlaSendArgs emits it, and add the CeraLive control-plane flags on top of the upstream surface:

- --verbose: raise tracing to debug (parity with the C sender). - --dry-run: parse the IP list and resolve the receiver, print them, and exit 0 without binding any socket; an unusable IP list exits non-zero with a specific error (missing/unreadable, empty, or zero valid IPs). - --stats-file <path>: accepted and stored (telemetry sink lands in a later task). - --stats-file-interval <ms>: telemetry cadence override, default 1000.

Upstream scheduler/control-socket flags stay intact; binary name unchanged (srtla_send). Adds unit tests for positional order, every flag, missing-arg errors, and the dry-run happy/invalid paths.
…elease

Adapt the fork's CI to build on the pinned nightly (rust-toolchain.toml,
nightly-2026-06-12) and produce .debs the device image builder can fetch.

- ci.yml: keep the upstream gate (fmt, clippy -D warnings lib+bin, full test
  fan-out, cargo audit) on the pinned nightly; add a build-deb matrix that
  cross-compiles aarch64-unknown-linux-gnu (device) + x86_64-unknown-linux-gnu
  and packages each .deb. Cross-channel/OS jobs now use `cargo +<channel>` so
  rust-toolchain.toml does not silently force nightly onto them.
- release.yml: on a v* tag, rebuild both arches, verify the fetch glob, and
  attach both .debs (+ sha256) to the GitHub release. Replaces build-debian.yml.
- ci/build-deb.sh: single source of truth for the package. Installs
  /usr/bin/srtla_send, names the artifact srtla-send-rs_<ver>_<arch>.deb to
  match fetch-debs.sh's `*${ARCH}*.deb` glob (re-run as a self-test), and
  declares Conflicts/Replaces: srtla (<< cutover) because srtla still ships the
  C srtla_send.
- Docs: AGENTS.md CI/PACKAGING section + README badges/packaging notes.
Record the new CeraLive control-plane flags in the parity contract (AGENTS.md) and the operator reference (README.md): --verbose, --dry-run, --stats-file, --stats-file-interval. Note that --stats-file is accepted but its telemetry sink is not yet wired, and that scheduler/control-socket flags stay present but unexposed in CeraUI.
Implement the opt-in --stats-file telemetry sink for the Rust sender,
mirroring the C reference (srtla/src/sender_telemetry.h).

What:
- New src/telemetry_file.rs: build_telemetry_json (the single home of the
  mandated bytes/s -> bits/s x8 conversion), an atomic temp -> fsync ->
  rename(2) publish, a conns_from_stats projection, and a TelemetryWriter
  whose Drop unlinks the live file on any graceful exit.
- --stats-file <path> + --stats-file-interval <ms> (default 1000) are now
  consumed by the sender loop: an initial snapshot publishes immediately
  (file appears in ~tens of ms), then a cadence timer republishes; SIGTERM
  and SIGINT trigger a graceful shutdown that unlinks the file.

Why:
- CeraUI's frozen @ceralive/srtla Zod reader consumes this file. The emitted
  JSON parses through it unchanged: schema_version is stripped as unknown,
  and the required window/in_flight fields are present.
- Divergences from the C producer are additive/strictly-better: schema_version
  = 1, Kalman-smoothed rtt_ms (C hardcodes 0), and a normalized weight_percent.

How to verify:
- cargo test (17 telemetry unit tests: schema field/types, x8 math, weight
  normalization, atomic in-place replace, a 1000-read no-torn-read loop,
  opt-in, unlink-on-drop) plus the existing suite stay green.
- Gate: cargo fmt --all -- --check; cargo clippy -- -D warnings (lib+bin).

Risks:
- Fully opt-in: absent --stats-file builds no writer and writes nothing.
…GTERM/SIGINT

# Conflicts:
#	AGENTS.md
#	README.md
#	src/main.rs
#	src/sender/mod.rs
…pipeline .deb release

# Conflicts:
#	AGENTS.md
The SIGHUP IP-list reload guard is only invoked from the #[cfg(unix)] event
loop arm, so on Windows analyze_ip_reload / IpReload / ReloadRefusal were
unused and tripped dead_code in the Windows CI test job (warnings-as-errors).
Gate the module declaration to unix, matching the surrounding signal handling.
- Replace blanket #[allow(dead_code)] with targeted, per-item allows
- Add inline justification comments for each suppression
- SRTLA_TYPE_REG_NAK: reference constant (receiver never sends to sender)
- SRT_TYPE_HANDSHAKE/SHUTDOWN/DATA: used in integration tests for protocol validation
- SRTLA_TYPE_REG3_LEN: used in integration tests for protocol validation
- All constants are either used or explicitly justified as protocol references
- cargo build --release: ✓ PASS
- cargo clippy --lib -- -D warnings: ✓ PASS (no dead_code warnings)
Connection/housekeeping timing already reads tokio::time::Instant, so it
honors tokio::time::pause()/advance(). Add advance_test_clock() in
test_helpers as the explicit paused-clock seam and two start_paused tests
(fake_clock_timeout, clock_frozen_no_timeout) that exercise is_timed_out()
deterministically with no real sleep. WHY comments at is_timed_out and the
all_failed_at timer pin the tokio-Instant invariant against a std::Instant
regression. No production timing values change (CONN_TIMEOUT stays 5s).
T12: the Rust sender's CONN_TIMEOUT diverged from the bonding receiver/C
sender (5s vs 15s). Git archaeology shows the 5s was inherited verbatim from
upstream (4 in the initial rust version, bumped 4->5 in an upstream file-split
refactor a003e06, all predating fork base 80cd0c4) — never a deliberate CeraLive
choice — while CeraLive deliberately tuned the C sender to 15s to match the
receiver. This is accidental drift, reconciled to 15s for parity.

Pin the value and the sender==receiver relationship with conn_timeout_value_pinned
so an upstream merge reverting 15->5 fails the gate; document the decision in the
AGENTS.md PARITY CONTRACT and a WHY comment on the constant. Real failover speed is
unaffected (dead links are caught in ~1s by the send-failure path and quality-score
deselection, not by this timeout). Repair the switch-dampening test to key off
CONN_TIMEOUT instead of a hardcoded 6s, and sync stale netns parity comments.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Add a registry-only pure-TS sender binding under bindings/typescript/
(@ceralive/srtla-send, GitHub Packages, @CERALIVE scope) mirroring
@ceralive/srtla's ./sender + ./telemetry subpaths. Modules are empty
skeletons (filled by T19/T20); the package installs and typechecks.

Retire the 'no bindings/ directory' anti-pattern in AGENTS.md (folded in
here for Rule A self-consistency): bindings are consumed registry-only,
never via a sibling link: or vendored .tgz. Add the binding gate to
BUILD/GATE.
Evidence for T2 wire-conformance ENCODE pins (REG1/REG2/REG3/bare-KA).
The encode test module itself landed in 466fcb9 (swept in by a concurrent
agent's commit in this shared checkout); this records the green test runs.
Replace wall-clock thread::sleep in the registration probing tests with deterministic now_ms()-based deadline control: force check_probing_complete()'s timeout branch via set_pending_timeout_at_ms() instead of sleeping 2.1s, and drop the no-op 50ms sleeps (handle_probe_response is already a no-op once a probe RTT is recorded). Assertions and coverage unchanged; the suite now runs instantly with no flakes.
Round-trip the real Rust-producer golden fixture (T10) through the telemetry
reader, asserting exact field values and the bitrate_bps = bytes/s x8 invariant.
Cover the three watch states (fresh/stale/absent) via watchTelemetry's {data,stale}
contract, plus schema edge cases (schema_version:2 rejected, missing required
window rejected, unknown fields stripped). Add missing buildSrtlaSendArgs coverage
(defaults, verbose, stats-file emit/omit, stats-file-interval).

The golden fixture is copied into the package's own tests/fixtures so the suite
never reads above its repo root (Rule D).
…trusted publishing

Align the sender binding's release path with @ceralive/cerastream: the public
npm registry, CalVer versioning, and npm OIDC trusted publishing in place of
GitHub Packages + token auth.

- package.json: version 1.0.0 -> 2026.6.0 (CalVer); publishConfig registry ->
  registry.npmjs.org, access -> public
- .npmrc: @CERALIVE scope -> registry.npmjs.org. A scoped @scope:registry entry
  overrides publishConfig.registry for scoped packages, so this is what actually
  routes the publish to public npm.
- publish-bindings.yml: OIDC trusted publishing (permissions id-token: write,
  npm publish --access public, no NODE_AUTH_TOKEN); tag scheme bindings/v* ->
  bindings-v*; latest/next dist-tag split on -rc; dist-only tarball guard.
- AGENTS.md, README.md: document the public-npm + bindings-v* release flow.

The trusted publisher is configured on npm (CERALIVE/srtla-send-rs ->
publish-bindings.yml). 2026.6.0 was bootstrap-published once to create the
package; subsequent releases run through the workflow with no token.
build(bindings): publish @ceralive/srtla-send to public npm via OIDC trusted publishing
* test(telemetry): edge cases + golden-fixture parity

* test(bindings): telemetry parse + malformed-input coverage

* docs: triage sendmmsg TODO + sync telemetry/binding test notes
…#3)

* chore(biome): add Biome 2.5 via @ceralive/biome-config (TS binding)

* docs(biome): standardize on Biome 2.5 + @ceralive/biome-config

* chore(biome): switch to published @ceralive/biome-config registry dep

* chore(biome): repin @ceralive/biome-config to ^2026.6.1 (CalVer)

* fix(bindings): restore telemetry golden fixture to byte-frozen single-line form

The Biome adoption in this PR ran a format pass that pretty-printed
bindings/typescript/tests/fixtures/telemetry-golden.json into multi-line form
with a trailing newline. That fixture is a deliberately byte-identical copy of
the Rust producer golden (tests/fixtures/telemetry-golden.json) — the
single-line, newline-free atomic-publish telemetry shape (ADR-001).

Reformatting it broke the cross-language parity test
(tests/telemetry_fixture_parity.rs): both rust_and_ts_goldens_are_byte_identical
and the newline-free assertion, which failed every Rust test job in CI while the
.deb builds stayed green.

Restore the fixture byte-for-byte from the Rust source of truth. Verified
locally: Rust parity 3/3 green, binding gate (tsc + 52 bun tests) green.
… contract (#4)

The Biome adoption (#3) reformatted tests/fixtures/telemetry-golden.json, which
broke the cross-language parity test and every Rust CI job. That was fixed by
restoring the fixture, but the root cause remained: nothing stopped the next
`biome check --write` from re-mangling it.

Exclude the fixtures tree from Biome (`files.includes: ["**", "!**/tests/fixtures/**"]`)
so the byte-frozen, newline-free ADR-001 golden can never be auto-formatted again.
Document the exclude and its rationale in AGENTS.md.

Verified: biome check . clean (fixture explicitly ignored), Rust parity 3/3,
binding tsc + 52 bun tests green.
@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds atomic telemetry file output (ADR-001 JSON schema), a SIGHUP reload guard that refuses zero-valid-IP reloads, 32-byte NAT padding for SRTLA control packets, --dry-run/--verbose/--stats-file CLI flags, connection-pool file-order rebuild, SIGTERM/SIGINT graceful shutdown, a new @ceralive/srtla-send TypeScript binding, and overhauled CI/CD pipelines including a dedicated release workflow and npm OIDC publish workflow. CONN_TIMEOUT is changed from 5 to 15 seconds.

Changes

Rust Sender Core

Layer / File(s) Summary
Toolchain pin, deny.toml, version bump, gitignore
rust-toolchain.toml, deny.toml, Cargo.toml, .gitignore
Pins nightly toolchain to nightly-2026-06-12, adds cargo-deny supply-chain enforcement (RUSTSEC advisory ignore + crates.io-only sources), bumps crate version to 2026.6.1, and ignores .omo//test-results/.
Control-packet 32-byte NAT padding
src/connection/packet_io.rs, src/connection/mod.rs, src/connection/batch_send.rs
Adds MIN_CONTROL_PKT_LEN = 32 and send_control_padded; all keepalive/SRTLA/REG2 sends route through it. DATA frames are explicitly documented as non-padded.
CONN_TIMEOUT bump to 15s
src/protocol/constants.rs
Changes CONN_TIMEOUT from 5 to 15 seconds with a parity-contract comment; adds #[allow(dead_code)] annotations to several protocol constants.
Telemetry file module (ADR-001)
src/lib.rs, src/telemetry_file.rs, tests/fixtures/telemetry-golden.json
Adds TelemetryConn, build_telemetry_json (bytes/s → bits/s ×8 conversion), conns_from_stats (weight normalization), write_atomic (fsync+rename), and TelemetryWriter with RAII Drop cleanup. Golden fixture populated.
SIGHUP reload guard
src/sender/reload.rs
New module defining ReloadRefusal (NotFound, Empty, NoValidIps), IpReload (Apply/Refuse), analyze_ip_reload_text (pure), and analyze_ip_reload (filesystem wrapper).
Connection pool file-order rebuild
src/sender/connections.rs, src/sender/housekeeping.rs
Refactors apply_connection_changes to rebuild the live pool in ips-file order using label-keyed HashMaps, preserving survivors without re-handshake and resetting last_selected_idx on reorder. Empty-pool guard added to handle_housekeeping.
Sender lifecycle integration
src/sender/mod.rs
run_sender_with_config gains telemetry: Option<TelemetryWriter>, periodic telemetry tick, tolerant empty-pool startup, guarded probing, SIGHUP refactored through analyze_ip_reload, and SIGTERM/SIGINT clean-exit branches.
CLI --dry-run, --verbose, --stats-file
src/main.rs
Adds --verbose, --dry-run, --stats-file, --stats-file-interval to Cli; DryRunReport + dry_run_resolve perform DNS resolution without socket binding; main constructs optional TelemetryWriter and removes stats files on shutdown.
EDPF/IoDS selection cleanup
src/sender/selection/edpf.rs, src/sender/selection/iods.rs, src/sender/selection/mod.rs, src/sender/status.rs
Collapses nested if let blocks into single short-circuit conditions; reformats function signatures and test assertions. No behavior change.
Deterministic timing seam
src/test_helpers.rs
Adds advance_test_clock(by: Duration) as the canonical seam for #[tokio::test(start_paused = true)] tests.
Rust unit test suite
src/tests/connection_tests.rs, src/tests/integration_tests.rs, src/tests/protocol_tests.rs, src/tests/registration_tests.rs, src/tests/sender_tests.rs
Replaces wall-clock sleeps with deterministic timing; adds tests for control padding, CONN_TIMEOUT pin, protocol encode/decode/malformed, REG1→REG3 handshake, pool reload ordering, socket-identity parity, and ACK/NAK attribution.
Outer integration test suites
tests/dry_run.rs, tests/signal_parity.rs, tests/telemetry_edge_cases.rs, tests/telemetry_fixture_parity.rs, tests/netns_pr19_parity.rs
Adds CLI dry-run contract tests, Unix signal parity tests (SIGHUP/SIGTERM/SIGINT), ADR-001 telemetry serializer boundary tests, Rust↔TS golden-fixture byte-identity test, and optional Linux netns keepalive/jitter tests.
Documentation
AGENTS.md, README.md
AGENTS.md rewritten as a project-specific contributor guide (parity contract, anti-patterns, hardening tasks). README.md expands CI/CD, CLI options, operational behavior, and protocol transport note.

TypeScript @ceralive/srtla-send Binding

Layer / File(s) Summary
Package manifest and tooling
bindings/typescript/package.json, bindings/typescript/tsconfig.json, bindings/typescript/tsconfig.build.json, bindings/typescript/biome.json, bindings/typescript/.*
Creates the ESM package with subpath exports (., ./sender, ./telemetry), TypeScript ES2022/NodeNext config, Biome linting, and npm publish config.
Sender module
bindings/typescript/src/sender/index.ts, bindings/typescript/src/sender/index.test.ts, bindings/typescript/src/index.ts
Adds Zod schema for srtla_send options, buildSrtlaSendArgs (positional + conditional flags), getSrtlaSendExec (resolution chain), spawnSrtlaSend, sendSrtlaSendHup, isSrtlaSendRunning; plus test suite.
Telemetry module
bindings/typescript/src/telemetry/index.ts, bindings/typescript/src/telemetry/index.test.ts, bindings/typescript/tests/telemetry-reader.test.ts, bindings/typescript/tests/fixtures/telemetry-golden.json
Adds connectionTelemetrySchema, telemetrySchema (literal schema_version: 1), readTelemetry (non-throwing), watchTelemetry (polling + stale flag + stop()), senderTelemetryPath; golden fixture; schema edge-case and round-trip test suites.

CI/CD Pipelines and Deb Packaging

Layer / File(s) Summary
ci/build-deb.sh
ci/build-deb.sh, .github/workflows/build-debian.yml
New deterministic Debian packaging script that reads version from Cargo.toml, writes DEBIAN/control (with Conflicts/Replaces via SRTLA_CUTOVER_VERSION), performs glob self-test, and conditionally builds/validates with dpkg-deb. Old build-debian.yml removed.
ci.yml updates
.github/workflows/ci.yml
Uses rust-toolchain.toml pinning for the main test job, adds cargo-deny supply-chain check, adds build-deb matrix job (arm64/amd64), and fixes test-stable/test-beta/windows/macOS jobs to use explicit cargo +<channel> invocations.
release.yml
.github/workflows/release.yml
New tag-triggered workflow that cross-builds srtla_send for arm64/amd64, packages .deb via ci/build-deb.sh, computes SHA-256 checksums, verifies both arch artifacts with filename-glob gate, and attaches to the GitHub release.
publish-bindings.yml
.github/workflows/publish-bindings.yml
New bindings-v*-triggered workflow that verifies tag/package.json version parity, typechecks/tests/builds, guards against .ts source in tarball, selects latest/next dist-tag, and publishes via npm OIDC with optional dry-run.

Sequence Diagram(s)

sequenceDiagram
    participant CLI as srtla_send CLI
    participant main
    participant dry_run_resolve
    participant run_sender_with_config
    participant TelemetryWriter
    participant analyze_ip_reload
    participant apply_connection_changes

    CLI->>main: --dry-run flag
    main->>dry_run_resolve: ips_file, host, port
    dry_run_resolve-->>main: DryRunReport (no sockets bound)
    main-->>CLI: exit 0

    CLI->>main: normal run + --stats-file
    main->>run_sender_with_config: telemetry: Some(TelemetryWriter)
    run_sender_with_config->>TelemetryWriter: publish initial snapshot
    loop periodic tick
        run_sender_with_config->>TelemetryWriter: publish(shared_stats)
    end
    run_sender_with_config->>analyze_ip_reload: on SIGHUP
    analyze_ip_reload-->>run_sender_with_config: Apply(ips) or Refuse(reason)
    run_sender_with_config->>apply_connection_changes: new_ips (file order)
    apply_connection_changes-->>run_sender_with_config: survivors preserved, new conns added
    run_sender_with_config-->>main: Ok(()) on SIGTERM/SIGINT
    main->>TelemetryWriter: remove (unlink .json + .tmp)
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

  • irlserver/srtla_send#11: Directly related — this PR changes CONN_TIMEOUT in the same src/protocol/constants.rs file where PR #11 first introduced/set that constant.
  • irlserver/srtla_send#3: Directly related — both PRs modify apply_connection_changes in src/sender/connections.rs, with this PR refactoring the pool-rebuild logic PR #3 established.
  • irlserver/srtla_send#7: Directly related — this PR's control-send path updates (writing last_sent after padded sends in send_srtla_packet) continue the last_keepalive_mslast_sent connection-tracking refactor from PR #7.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/main.rs (1)

42-42: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Keep the help usage in the CeraLive positional order.

The CLI parses the right positionals, but the usage string advertises [OPTIONS] first. The contract requires the user-facing form to be srtla_send SRT_LISTEN_PORT SRTLA_HOST SRTLA_PORT BIND_IPS_FILE [OPTIONS].

Proposed fix
-    override_usage = "srtla_send [OPTIONS] SRT_LISTEN_PORT SRTLA_HOST SRTLA_PORT BIND_IPS_FILE"
+    override_usage = "srtla_send SRT_LISTEN_PORT SRTLA_HOST SRTLA_PORT BIND_IPS_FILE [OPTIONS]"

As per coding guidelines, src/main.rs must keep the CLI positional order exactly as srtla_send <SRT_LISTEN_PORT> <SRTLA_HOST> <SRTLA_PORT> <BIND_IPS_FILE> [OPTIONS].

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main.rs` at line 42, The override_usage string in src/main.rs incorrectly
places [OPTIONS] before the positional arguments. Reorder the override_usage
assignment to move the positional arguments (SRT_LISTEN_PORT, SRTLA_HOST,
SRTLA_PORT, BIND_IPS_FILE) before [OPTIONS], and wrap each positional argument
name in angle brackets to match the required format: srtla_send
<SRT_LISTEN_PORT> <SRTLA_HOST> <SRTLA_PORT> <BIND_IPS_FILE> [OPTIONS].

Source: Coding guidelines

🧹 Nitpick comments (2)
bindings/typescript/src/sender/index.ts (1)

44-46: ⚡ Quick win

Emit --stats-file-interval only when statsFile is also set.

--stats-file-interval is emitted whenever statsFileInterval is defined, even if statsFile is not set. Semantically, the interval has no effect without a stats file to write to. While the Rust CLI likely ignores the orphaned flag, the binding should guard the emission:

if (options.statsFile && options.statsFileInterval !== undefined) {
    args.push('--stats-file-interval', String(options.statsFileInterval));
}

This prevents passing meaningless flags and improves clarity.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@bindings/typescript/src/sender/index.ts` around lines 44 - 46, The
conditional check for emitting the --stats-file-interval flag only validates
that statsFileInterval is defined, but this flag is meaningless without a
corresponding stats file. Update the condition that guards the args.push call
for --stats-file-interval to require both options.statsFile to be set AND
options.statsFileInterval to be defined before emitting the flag, preventing
orphaned flags from being passed to the underlying command.
bindings/typescript/.npmignore (1)

1-12: 💤 Low value

Consider removing .npmignore or clarifying the comment.

When package.json has a "files" field, it acts as an allowlist and takes precedence, making .npmignore (a denylist) redundant. The comment on line 2 says "This complement" but .npmignore doesn't actually complement the files field—the files field is the single source of truth for what ships. Having both is harmless but adds maintenance surface.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@bindings/typescript/.npmignore` around lines 1 - 12, The `.npmignore` file is
redundant because the `files` field in package.json acts as an allowlist and
takes precedence over `.npmignore` as the single source of truth for what gets
shipped to npm. Remove the `.npmignore` file entirely to eliminate maintenance
surface and confusion, since the `files` field in package.json already defines
what should be included in the npm tarball. If the intent is to keep the file,
update the misleading comment on line 2 that claims `.npmignore` "complements"
the `files` field—clarify instead that the `files` field is the actual source of
truth and that `.npmignore` is not necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/ci.yml:
- Around line 20-25: GitHub Actions are currently using mutable version tags
(`@v4`, `@v1`, etc.) which pose a supply-chain security risk. In
.github/workflows/ci.yml, pin the actions/checkout@v4 action at line 21 and
actions-rust-lang/setup-rust-toolchain@v1 action at line 24, along with the
remaining actions at lines 85, 91, and 115 to their full immutable commit SHAs.
Similarly, in .github/workflows/release.yml, pin all uses entries at lines 31,
35, 68, 82, and 112 to their full commit SHAs. For each action, replace the
mutable tag (e.g., `@v4`) with the complete commit SHA (a 40-character hash) to
ensure supply-chain security and prevent unexpected changes.

In @.github/workflows/publish-bindings.yml:
- Around line 64-73: The tag/package version equality check in the "Verify tag
matches package version" step does not validate the required CalVer format for
bindings tags. Add validation to ensure the extracted TAG variable matches the
CalVer pattern bindings-vYYYY.M.P with optional -rc.N suffix using a regex
pattern match. This prevents publishing if package.json is accidentally set to
an invalid version format that still matches a malformed tag. Add the format
validation as an additional conditional check in the run script before or
alongside the existing equality check.
- Around line 47-56: Replace the mutable action version tags with pinned full
commit SHAs across all three actions (actions/checkout, oven-sh/setup-bun, and
actions/setup-node) to lock in specific versions. Additionally, add
persist-credentials: false to the actions/checkout step configuration to prevent
persisting checkout credentials and reduce the supply-chain blast radius for
this npm publishing workflow.
- Around line 87-95: The tarball guard in the publish-bindings.yml workflow at
the "Tarball guard — only compiled dist/ ships" step currently only rejects
TypeScript source files (matching `\.ts$` excluding `\.d\.ts$`), but it should
also reject compiled test files like `.test.js` artifacts. Extend the grep
condition to also detect and block compiled test files to prevent them from
being shipped in the tarball, ensuring compliance with the `files: ["dist"]`
publish contract that prohibits test artifacts.
- Around line 107-125: The npm publish step in the "Publish to npm via OIDC
trusted publishing" task lacks a guard to prevent real publishes on non-tag
refs. Add a check before the actual npm publish command that verifies github.ref
matches the refs/tags/bindings-v* pattern when FORCE_DRY_RUN is false. If the
ref does not match this pattern and it's not a dry-run, the step should exit
with an error to prevent unintended publishes from manual workflow dispatch on
arbitrary branches. Only allow the real npm publish (without --dry-run flag) to
proceed when running on a properly tagged ref.

In @.github/workflows/release.yml:
- Around line 12-14: Apply least-privilege token permissions across two workflow
files. In `.github/workflows/release.yml#L12-L14`, remove the workflow-level
`permissions` block containing `contents: write` and instead add job-level
`permissions` blocks: set `contents: read` for the `build-deb` job and
`contents: write` for the `release` job. In `.github/workflows/ci.yml#L9-L17`,
add an explicit `permissions` block at the workflow level with at least
`contents: read` to prevent the CI workflow from inheriting default mutable
repository permissions.

In `@AGENTS.md`:
- Line 3: Remove the Parent reference line that contains the path to the parent
workspace (../AGENTS.md) from line 3 of the AGENTS.md file. This external parent
reference violates Rule D because it references a path above the repository root
that doesn't exist in CI environments, making the link broken when the
repository is built and tested standalone. Delete the entire Parent line to
ensure the document is self-contained within the srtla-send-rs repository.

In `@bindings/typescript/src/telemetry/index.ts`:
- Around line 57-59: The schema definitions for window and in_flight fields are
currently using z.number().int() which allows negative values. Add non-negative
constraints to both the window and in_flight field definitions by appending
.nonnegative() (or .min(0)) to enforce that these values must be greater than or
equal to zero, preventing malformed snapshots with negative values from parsing
as valid telemetry.

In `@Cargo.toml`:
- Line 7: The version field in Cargo.toml has been manually bumped to 2026.6.1,
but according to coding guidelines, version management is handled automatically
by the release process and should not be manually modified. Revert the version
back to 3.0.0 in the Cargo.toml file to comply with the automated version
management workflow and prevent conflicts with the release automation.

In `@src/sender/connections.rs`:
- Around line 42-83: The issue is that stale sequence entries are removed from
seq_tracker before validating whether the new connections can be successfully
created, and the connection pool is rebuilt regardless. If all new connections
fail (resulting in empty fresh HashMap) while the current pool is non-empty,
this causes all working uplinks to be dropped. Move the loop that calls
seq_tracker.remove_connection for each conn_id in removed_conn_ids to execute
only after the fresh HashMap is populated and validated. Additionally, add a
guard condition that returns early without mutating the connection pool when
new_ip_list is non-empty but fresh ends up empty (indicating connection creation
failed) while the current connections pool is non-empty, preventing the
transition from a healthy pool to an empty one due to failed replacement
creation.

In `@src/telemetry_file.rs`:
- Around line 193-206: The write_tmp function creates a temporary file at a
predictable path using File::create(tmp), which follows symlinks and is
vulnerable to symlink-clobbering attacks if the stats directory is
world-writable and the process has elevated privileges. Fix this by modifying
the file creation in write_tmp to use no-follow or create-new semantics, or
alternatively use OpenOptions to create the file with OpenFlags that prevent
symlink following. Ensure the temporary file cannot be hijacked by an
attacker-controlled symlink before the atomic rename in write_atomic completes.
- Around line 249-253: The publish() method performs blocking filesystem I/O
operations (write_atomic and sync_all) directly within the async sender loop,
which can stall UDP packet forwarding on slow storage. Move the blocking I/O off
the Tokio event loop by either wrapping the publish() call with
tokio::task::spawn_blocking in the caller at src/sender/mod.rs, or by creating a
dedicated telemetry worker thread that receives stats snapshots asynchronously
and handles the atomic writes independently. This ensures the async sender loop
remains non-blocking and responsive to UDP packet forwarding.
- Around line 508-524: The spin-wait loop that checks
writes.load(Ordering::Relaxed) > 0 currently executes after the 1000 read
attempts, allowing all reads to potentially complete before the writer thread
has even started. Move the bounded spin-wait loop (the 0..100 iteration that
waits for writes > 0) to execute before the parse_errors loop (the 0..1000
iteration). This ensures the writer thread has performed at least one write
before the concurrent read sampling begins, properly exercising the concurrent
publish/read behavior the test is intended to validate.

---

Outside diff comments:
In `@src/main.rs`:
- Line 42: The override_usage string in src/main.rs incorrectly places [OPTIONS]
before the positional arguments. Reorder the override_usage assignment to move
the positional arguments (SRT_LISTEN_PORT, SRTLA_HOST, SRTLA_PORT,
BIND_IPS_FILE) before [OPTIONS], and wrap each positional argument name in angle
brackets to match the required format: srtla_send <SRT_LISTEN_PORT> <SRTLA_HOST>
<SRTLA_PORT> <BIND_IPS_FILE> [OPTIONS].

---

Nitpick comments:
In `@bindings/typescript/.npmignore`:
- Around line 1-12: The `.npmignore` file is redundant because the `files` field
in package.json acts as an allowlist and takes precedence over `.npmignore` as
the single source of truth for what gets shipped to npm. Remove the `.npmignore`
file entirely to eliminate maintenance surface and confusion, since the `files`
field in package.json already defines what should be included in the npm
tarball. If the intent is to keep the file, update the misleading comment on
line 2 that claims `.npmignore` "complements" the `files` field—clarify instead
that the `files` field is the actual source of truth and that `.npmignore` is
not necessary.

In `@bindings/typescript/src/sender/index.ts`:
- Around line 44-46: The conditional check for emitting the
--stats-file-interval flag only validates that statsFileInterval is defined, but
this flag is meaningless without a corresponding stats file. Update the
condition that guards the args.push call for --stats-file-interval to require
both options.statsFile to be set AND options.statsFileInterval to be defined
before emitting the flag, preventing orphaned flags from being passed to the
underlying command.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: bc342eb0-b93c-418c-bc78-f7f13e852e06

📥 Commits

Reviewing files that changed from the base of the PR and between 80cd0c4 and e5c8090.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • bindings/typescript/bun.lock is excluded by !**/*.lock
📒 Files selected for processing (52)
  • .github/workflows/build-debian.yml
  • .github/workflows/ci.yml
  • .github/workflows/publish-bindings.yml
  • .github/workflows/release.yml
  • .gitignore
  • AGENTS.md
  • Cargo.toml
  • README.md
  • bindings/typescript/.gitignore
  • bindings/typescript/.npmignore
  • bindings/typescript/.npmrc
  • bindings/typescript/biome.json
  • bindings/typescript/package.json
  • bindings/typescript/src/index.ts
  • bindings/typescript/src/sender/index.test.ts
  • bindings/typescript/src/sender/index.ts
  • bindings/typescript/src/telemetry/index.test.ts
  • bindings/typescript/src/telemetry/index.ts
  • bindings/typescript/tests/fixtures/telemetry-golden.json
  • bindings/typescript/tests/telemetry-reader.test.ts
  • bindings/typescript/tsconfig.build.json
  • bindings/typescript/tsconfig.json
  • ci/build-deb.sh
  • deny.toml
  • rust-toolchain.toml
  • src/connection/batch_send.rs
  • src/connection/mod.rs
  • src/connection/packet_io.rs
  • src/lib.rs
  • src/main.rs
  • src/protocol/constants.rs
  • src/sender/connections.rs
  • src/sender/housekeeping.rs
  • src/sender/mod.rs
  • src/sender/reload.rs
  • src/sender/selection/edpf.rs
  • src/sender/selection/iods.rs
  • src/sender/selection/mod.rs
  • src/sender/status.rs
  • src/telemetry_file.rs
  • src/test_helpers.rs
  • src/tests/connection_tests.rs
  • src/tests/integration_tests.rs
  • src/tests/protocol_tests.rs
  • src/tests/registration_tests.rs
  • src/tests/sender_tests.rs
  • tests/dry_run.rs
  • tests/fixtures/telemetry-golden.json
  • tests/netns_pr19_parity.rs
  • tests/signal_parity.rs
  • tests/telemetry_edge_cases.rs
  • tests/telemetry_fixture_parity.rs
💤 Files with no reviewable changes (1)
  • .github/workflows/build-debian.yml

Comment thread .github/workflows/ci.yml
Comment on lines 20 to 25
- name: Checkout code
uses: actions/checkout@v4

- name: Install Rust toolchain
- name: Install Rust toolchain (from rust-toolchain.toml)
uses: actions-rust-lang/setup-rust-toolchain@v1
with:
toolchain: nightly
components: rustfmt, clippy

- name: Cache dependencies
uses: actions/cache@v4
with:
path: |
~/.cargo/registry
~/.cargo/git
target
key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
restore-keys: |
${{ runner.os }}-cargo-

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check both workflow files for action pinning
echo "=== ci.yml ===" 
cat -n .github/workflows/ci.yml | head -130

echo -e "\n=== release.yml ===" 
cat -n .github/workflows/release.yml | head -120

Repository: irlserver/srtla_send

Length of output: 10564


Pin all GitHub Actions to immutable commit SHAs across CI and release workflows. GitHub Actions using mutable version tags (@v4, @v1, @v2) can change unexpectedly and pose a supply-chain risk.

Pin every uses: entry to full commit SHAs:

  • .github/workflows/ci.yml: Lines 21, 24, 85, 91, 115
  • .github/workflows/release.yml: Lines 31, 35, 68, 82, 112
🧰 Tools
🪛 zizmor (1.25.2)

[warning] 20-21: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false

(artipacked)


[error] 21-21: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)


[error] 24-24: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)

📍 Affects 2 files
  • .github/workflows/ci.yml#L20-L25 (this comment)
  • .github/workflows/release.yml#L30-L36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/ci.yml around lines 20 - 25, GitHub Actions are currently
using mutable version tags (`@v4`, `@v1`, etc.) which pose a supply-chain security
risk. In .github/workflows/ci.yml, pin the actions/checkout@v4 action at line 21
and actions-rust-lang/setup-rust-toolchain@v1 action at line 24, along with the
remaining actions at lines 85, 91, and 115 to their full immutable commit SHAs.
Similarly, in .github/workflows/release.yml, pin all uses entries at lines 31,
35, 68, 82, and 112 to their full commit SHAs. For each action, replace the
mutable tag (e.g., `@v4`) with the complete commit SHA (a 40-character hash) to
ensure supply-chain security and prevent unexpected changes.

Source: Linters/SAST tools

Comment on lines +47 to +56
- uses: actions/checkout@v4

- uses: oven-sh/setup-bun@v2
with:
bun-version: latest

# npm (not bun) performs the publish: it speaks OIDC trusted publishing.
- uses: actions/setup-node@v4
with:
node-version: 22 # trusted publishing needs Node >= 22.14

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Pin publish-workflow actions and avoid persisting checkout credentials.

This workflow can publish to npm, so mutable action tags (@v4, @v2) and persisted checkout credentials expand the release supply-chain blast radius. Pin these actions to full commit SHAs and set persist-credentials: false for checkout.

Hardening sketch
-      - uses: actions/checkout@v4
+      - uses: actions/checkout@<full-commit-sha>
+        with:
+          persist-credentials: false
 
-      - uses: oven-sh/setup-bun@v2
+      - uses: oven-sh/setup-bun@<full-commit-sha>
         with:
           bun-version: latest
 
       # npm (not bun) performs the publish: it speaks OIDC trusted publishing.
-      - uses: actions/setup-node@v4
+      - uses: actions/setup-node@<full-commit-sha>
🧰 Tools
🪛 zizmor (1.25.2)

[warning] 47-47: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false

(artipacked)


[error] 47-47: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)


[error] 49-49: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)


[error] 54-54: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)


[error] 49-49: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default

(cache-poisoning)


[error] 54-54: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default

(cache-poisoning)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/publish-bindings.yml around lines 47 - 56, Replace the
mutable action version tags with pinned full commit SHAs across all three
actions (actions/checkout, oven-sh/setup-bun, and actions/setup-node) to lock in
specific versions. Additionally, add persist-credentials: false to the
actions/checkout step configuration to prevent persisting checkout credentials
and reduce the supply-chain blast radius for this npm publishing workflow.

Source: Linters/SAST tools

Comment on lines +64 to +73
- name: Verify tag matches package version
if: startsWith(github.ref, 'refs/tags/bindings-v')
run: |
TAG="${GITHUB_REF#refs/tags/bindings-v}"
PKG="$(node -p "require('./package.json').version")"
echo "tag=$TAG package=$PKG"
if [ "$TAG" != "$PKG" ]; then
echo "::error::tag bindings-v$TAG does not match package.json version $PKG"
exit 1
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate the bindings-vYYYY.M.P[-rc.N] shape before publishing.

The guard checks tag/package equality, but not the required CalVer tag format. If package.json is accidentally bumped to an invalid version shape, a matching malformed bindings-v* tag can still publish.

Suggested guard
           TAG="${GITHUB_REF#refs/tags/bindings-v}"
           PKG="$(node -p "require('./package.json').version")"
           echo "tag=$TAG package=$PKG"
+          if ! echo "$TAG" | grep -Eq '^[0-9]{4}\.[0-9]+\.[0-9]+(-rc\.[0-9]+)?$'; then
+            echo "::error::tag bindings-v$TAG must match bindings-vYYYY.M.P or bindings-vYYYY.M.P-rc.N"
+            exit 1
+          fi
           if [ "$TAG" != "$PKG" ]; then
             echo "::error::tag bindings-v$TAG does not match package.json version $PKG"
             exit 1

As per coding guidelines, “Binding publish workflow publish-bindings.yml tags must use bindings-vYYYY.M.P format (CalVer, distinct from v* release tags)”.

📝 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
- name: Verify tag matches package version
if: startsWith(github.ref, 'refs/tags/bindings-v')
run: |
TAG="${GITHUB_REF#refs/tags/bindings-v}"
PKG="$(node -p "require('./package.json').version")"
echo "tag=$TAG package=$PKG"
if [ "$TAG" != "$PKG" ]; then
echo "::error::tag bindings-v$TAG does not match package.json version $PKG"
exit 1
fi
- name: Verify tag matches package version
if: startsWith(github.ref, 'refs/tags/bindings-v')
run: |
TAG="${GITHUB_REF#refs/tags/bindings-v}"
PKG="$(node -p "require('./package.json').version")"
echo "tag=$TAG package=$PKG"
if ! echo "$TAG" | grep -Eq '^[0-9]{4}\.[0-9]+\.[0-9]+(-rc\.[0-9]+)?$'; then
echo "::error::tag bindings-v$TAG must match bindings-vYYYY.M.P or bindings-vYYYY.M.P-rc.N"
exit 1
fi
if [ "$TAG" != "$PKG" ]; then
echo "::error::tag bindings-v$TAG does not match package.json version $PKG"
exit 1
fi
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/publish-bindings.yml around lines 64 - 73, The tag/package
version equality check in the "Verify tag matches package version" step does not
validate the required CalVer format for bindings tags. Add validation to ensure
the extracted TAG variable matches the CalVer pattern bindings-vYYYY.M.P with
optional -rc.N suffix using a regex pattern match. This prevents publishing if
package.json is accidentally set to an invalid version format that still matches
a malformed tag. Add the format validation as an additional conditional check in
the run script before or alongside the existing equality check.

Source: Coding guidelines

Comment on lines +87 to +95
- name: Tarball guard — only compiled dist/ ships
run: |
npm pack --dry-run --json > /tmp/pack.json
FILES="$(node -e 'const j=require("/tmp/pack.json"); for (const f of j[0].files) console.log(f.path)')"
echo "$FILES"
if echo "$FILES" | grep -E '\.ts$' | grep -vE '\.d\.ts$'; then
echo "::error::tarball contains TypeScript source — only compiled dist/ may ship"
exit 1
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Make the tarball guard reject compiled tests, not just .ts sources.

This only blocks TypeScript source files. If tsconfig.build.json ever emits dist/**/*.test.js or fixture files, they would pass this guard and ship despite the files: ["dist"] publish contract.

Suggested stricter guard
           npm pack --dry-run --json > /tmp/pack.json
           FILES="$(node -e 'const j=require("/tmp/pack.json"); for (const f of j[0].files) console.log(f.path)')"
           echo "$FILES"
           if echo "$FILES" | grep -E '\.ts$' | grep -vE '\.d\.ts$'; then
             echo "::error::tarball contains TypeScript source — only compiled dist/ may ship"
             exit 1
           fi
+          if echo "$FILES" | grep -E '(^|/)tests?/|\.test\.js$|fixtures/'; then
+            echo "::error::tarball contains tests or fixtures — only runtime dist artifacts may ship"
+            exit 1
+          fi

As per coding guidelines, TS binding package.json must list only dist/ so compiled tests never land in the published tarball.

📝 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
- name: Tarball guard — only compiled dist/ ships
run: |
npm pack --dry-run --json > /tmp/pack.json
FILES="$(node -e 'const j=require("/tmp/pack.json"); for (const f of j[0].files) console.log(f.path)')"
echo "$FILES"
if echo "$FILES" | grep -E '\.ts$' | grep -vE '\.d\.ts$'; then
echo "::error::tarball contains TypeScript source — only compiled dist/ may ship"
exit 1
fi
- name: Tarball guard — only compiled dist/ ships
run: |
npm pack --dry-run --json > /tmp/pack.json
FILES="$(node -e 'const j=require("/tmp/pack.json"); for (const f of j[0].files) console.log(f.path)')"
echo "$FILES"
if echo "$FILES" | grep -E '\.ts$' | grep -vE '\.d\.ts$'; then
echo "::error::tarball contains TypeScript source — only compiled dist/ may ship"
exit 1
fi
if echo "$FILES" | grep -E '(^|/)tests?/|\.test\.js$|fixtures/'; then
echo "::error::tarball contains tests or fixtures — only runtime dist artifacts may ship"
exit 1
fi
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/publish-bindings.yml around lines 87 - 95, The tarball
guard in the publish-bindings.yml workflow at the "Tarball guard — only compiled
dist/ ships" step currently only rejects TypeScript source files (matching
`\.ts$` excluding `\.d\.ts$`), but it should also reject compiled test files
like `.test.js` artifacts. Extend the grep condition to also detect and block
compiled test files to prevent them from being shipped in the tarball, ensuring
compliance with the `files: ["dist"]` publish contract that prohibits test
artifacts.

Source: Coding guidelines

Comment on lines +107 to +125
- name: Publish to npm via OIDC trusted publishing (or dry-run)
env:
FORCE_DRY_RUN: ${{ github.event.inputs.dry_run }}
REPO_PRIVATE: ${{ github.event.repository.private }}
run: |
# Auth is the GitHub OIDC token (id-token: write) exchanged against the
# trusted publisher on the npm package — no NPM_TOKEN. npm auto-generates
# provenance for public repos but cannot for a private source repo, so
# disable it explicitly while private (re-enables when the repo is public).
if [ "${REPO_PRIVATE}" = "true" ]; then
export NPM_CONFIG_PROVENANCE=false
echo "Private source repo — provenance disabled."
fi
if [ "${FORCE_DRY_RUN}" = "true" ]; then
echo "Dry-run forced — no publish."
npm publish --access public --dry-run --tag "${{ steps.tag.outputs.dist_tag }}"
else
npm publish --access public --tag "${{ steps.tag.outputs.dist_tag }}"
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Prevent manual dispatch from bypassing the tag/version publish gate.

workflow_dispatch can run on a branch, skips the tag-match guard at Lines 64-73, and will publish if dry_run=false. Refuse real publishes unless github.ref is a refs/tags/bindings-v* ref.

Suggested guard
           if [ "${FORCE_DRY_RUN}" = "true" ]; then
             echo "Dry-run forced — no publish."
             npm publish --access public --dry-run --tag "${{ steps.tag.outputs.dist_tag }}"
           else
+            case "${GITHUB_REF}" in
+              refs/tags/bindings-v*) ;;
+              *)
+                echo "::error::real publish is only allowed from bindings-v* tags"
+                exit 1
+                ;;
+            esac
             npm publish --access public --tag "${{ steps.tag.outputs.dist_tag }}"
           fi

As per coding guidelines, publishing is tied to bindings-vYYYY.M.P binding tags and package-version parity.

📝 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
- name: Publish to npm via OIDC trusted publishing (or dry-run)
env:
FORCE_DRY_RUN: ${{ github.event.inputs.dry_run }}
REPO_PRIVATE: ${{ github.event.repository.private }}
run: |
# Auth is the GitHub OIDC token (id-token: write) exchanged against the
# trusted publisher on the npm package — no NPM_TOKEN. npm auto-generates
# provenance for public repos but cannot for a private source repo, so
# disable it explicitly while private (re-enables when the repo is public).
if [ "${REPO_PRIVATE}" = "true" ]; then
export NPM_CONFIG_PROVENANCE=false
echo "Private source repo — provenance disabled."
fi
if [ "${FORCE_DRY_RUN}" = "true" ]; then
echo "Dry-run forced — no publish."
npm publish --access public --dry-run --tag "${{ steps.tag.outputs.dist_tag }}"
else
npm publish --access public --tag "${{ steps.tag.outputs.dist_tag }}"
fi
- name: Publish to npm via OIDC trusted publishing (or dry-run)
env:
FORCE_DRY_RUN: ${{ github.event.inputs.dry_run }}
REPO_PRIVATE: ${{ github.event.repository.private }}
run: |
# Auth is the GitHub OIDC token (id-token: write) exchanged against the
# trusted publisher on the npm package — no NPM_TOKEN. npm auto-generates
# provenance for public repos but cannot for a private source repo, so
# disable it explicitly while private (re-enables when the repo is public).
if [ "${REPO_PRIVATE}" = "true" ]; then
export NPM_CONFIG_PROVENANCE=false
echo "Private source repo — provenance disabled."
fi
if [ "${FORCE_DRY_RUN}" = "true" ]; then
echo "Dry-run forced — no publish."
npm publish --access public --dry-run --tag "${{ steps.tag.outputs.dist_tag }}"
else
case "${GITHUB_REF}" in
refs/tags/bindings-v*) ;;
*)
echo "::error::real publish is only allowed from bindings-v* tags"
exit 1
;;
esac
npm publish --access public --tag "${{ steps.tag.outputs.dist_tag }}"
fi
🧰 Tools
🪛 zizmor (1.25.2)

[info] 122-122: code injection via template expansion (template-injection): may expand into attacker-controllable code

(template-injection)


[info] 122-122: code injection via template expansion (template-injection): may expand into attacker-controllable code

(template-injection)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/publish-bindings.yml around lines 107 - 125, The npm
publish step in the "Publish to npm via OIDC trusted publishing" task lacks a
guard to prevent real publishes on non-tag refs. Add a check before the actual
npm publish command that verifies github.ref matches the refs/tags/bindings-v*
pattern when FORCE_DRY_RUN is false. If the ref does not match this pattern and
it's not a dry-run, the step should exit with an error to prevent unintended
publishes from manual workflow dispatch on arbitrary branches. Only allow the
real npm publish (without --dry-run flag) to proceed when running on a properly
tagged ref.

Source: Coding guidelines

Comment thread Cargo.toml
resolver = "3"
name = "srtla_send"
version = "3.0.0"
version = "2026.6.1"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Revert the version bump; version management is automated by the release process.

As per coding guidelines, "Never bump the internal package version in Cargo.toml. This is handled automatically by the release process." Manual version changes risk conflicting with release automation and circumvent the governed version bump workflow.

🔄 Revert to 3.0.0
-version = "2026.6.1"
+version = "3.0.0"
📝 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
version = "2026.6.1"
version = "3.0.0"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Cargo.toml` at line 7, The version field in Cargo.toml has been manually
bumped to 2026.6.1, but according to coding guidelines, version management is
handled automatically by the release process and should not be manually
modified. Revert the version back to 3.0.0 in the Cargo.toml file to comply with
the automated version management workflow and prevent conflicts with the release
automation.

Source: Coding guidelines

Comment thread src/sender/connections.rs
Comment on lines +42 to 83
// Drop the leaving links' sequence entries so a stale NAK can't be
// misattributed to whichever uplink later reuses that index.
let removed_conn_ids: SmallVec<u64, 4> = connections
.iter()
.filter(|c| !desired_labels.contains(&c.label))
.filter(|c| !desired_labels.contains(c.label.as_str()))
.map(|c| c.conn_id)
.collect();

connections.retain(|c| desired_labels.contains(&c.label));

// If connections were removed, reset selection state and clean up sequence tracker
if connections.len() != old_len {
info!("removed {} stale connections", old_len - connections.len());
*last_selected_idx = None;

// Remove entries for removed connections from the ring buffer
for conn_id in removed_conn_ids {
seq_tracker.remove_connection(conn_id);
}
let removed = removed_conn_ids.len();
for conn_id in &removed_conn_ids {
seq_tracker.remove_connection(*conn_id);
}

// Add new connections
let mut seen = HashSet::<IpAddr>::new();
let new_ips_needed: SmallVec<IpAddr, 4> = new_ips
let new_ip_list: SmallVec<IpAddr, 4> = desired
.iter()
.copied()
.filter(|ip| seen.insert(*ip))
.filter(|ip| {
let label = format!("{}:{} via {}", receiver_host, receiver_port, ip);
!current_labels.contains(&label)
})
.filter(|(_, label)| !current_labels.contains(label.as_str()))
.map(|(ip, _)| *ip)
.collect();
let attempted_new = new_ip_list.len();
let mut fresh: HashMap<String, SrtlaConnection> = if new_ip_list.is_empty() {
HashMap::new()
} else {
create_connections_from_ips(&new_ip_list, receiver_host, receiver_port)
.await
.into_iter()
.map(|c| (c.label.clone(), c))
.collect()
};
let added = fresh.len();

if !new_ips_needed.is_empty() {
let mut new_connections =
create_connections_from_ips(&new_ips_needed, receiver_host, receiver_port).await;
let added_count = new_connections.len();
connections.append(&mut new_connections);
let previous_order: SmallVec<u64, 4> = connections.iter().map(|c| c.conn_id).collect();
let mut survivors: HashMap<String, SrtlaConnection> = std::mem::take(connections)
.into_iter()
.filter(|c| desired_labels.contains(c.label.as_str()))
.map(|c| (c.label.clone(), c))
.collect();

if added_count > 0 {
info!("added {} new connections", added_count);
} else {
warn!(
"failed to add any new connections (attempted {})",
new_ips_needed.len()
);
for (_, label) in &desired {
if let Some(conn) = survivors.remove(label) {
connections.push(conn);
} else if let Some(conn) = fresh.remove(label) {
connections.push(conn);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Refuse all-replacement reloads when every new uplink fails to connect.

A SIGHUP file with parseable but unusable replacement IPs reaches Apply, then fresh can be empty while all current labels are removed. Because sequence entries are removed before that failure is known and connections is rebuilt anyway, a bad reload can drop every working uplink instead of keeping the existing pool.

Move stale-sequence removal until after the replacement viability check, and return without mutating the pool when an already-running sender would otherwise transition from non-empty to empty due only to failed new connection creation.

Suggested guard shape
-    let removed_conn_ids: SmallVec<u64, 4> = connections
-        .iter()
-        .filter(|c| !desired_labels.contains(c.label.as_str()))
-        .map(|c| c.conn_id)
-        .collect();
-    let removed = removed_conn_ids.len();
-    for conn_id in &removed_conn_ids {
-        seq_tracker.remove_connection(*conn_id);
-    }
+    let survivor_count = connections
+        .iter()
+        .filter(|c| desired_labels.contains(c.label.as_str()))
+        .count();

     let new_ip_list: SmallVec<IpAddr, 4> = desired
         .iter()
         .filter(|(_, label)| !current_labels.contains(label.as_str()))
         .map(|(ip, _)| *ip)
         .collect();
@@
     };
     let added = fresh.len();
+
+    if !connections.is_empty() && survivor_count == 0 && attempted_new > 0 && added == 0 {
+        warn!(
+            "refusing reload: all replacement uplinks failed to connect; keeping existing connections"
+        );
+        return;
+    }
+
+    let removed_conn_ids: SmallVec<u64, 4> = connections
+        .iter()
+        .filter(|c| !desired_labels.contains(c.label.as_str()))
+        .map(|c| c.conn_id)
+        .collect();
+    let removed = removed_conn_ids.len();
+    for conn_id in &removed_conn_ids {
+        seq_tracker.remove_connection(*conn_id);
+    }
📝 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
// Drop the leaving links' sequence entries so a stale NAK can't be
// misattributed to whichever uplink later reuses that index.
let removed_conn_ids: SmallVec<u64, 4> = connections
.iter()
.filter(|c| !desired_labels.contains(&c.label))
.filter(|c| !desired_labels.contains(c.label.as_str()))
.map(|c| c.conn_id)
.collect();
connections.retain(|c| desired_labels.contains(&c.label));
// If connections were removed, reset selection state and clean up sequence tracker
if connections.len() != old_len {
info!("removed {} stale connections", old_len - connections.len());
*last_selected_idx = None;
// Remove entries for removed connections from the ring buffer
for conn_id in removed_conn_ids {
seq_tracker.remove_connection(conn_id);
}
let removed = removed_conn_ids.len();
for conn_id in &removed_conn_ids {
seq_tracker.remove_connection(*conn_id);
}
// Add new connections
let mut seen = HashSet::<IpAddr>::new();
let new_ips_needed: SmallVec<IpAddr, 4> = new_ips
let new_ip_list: SmallVec<IpAddr, 4> = desired
.iter()
.copied()
.filter(|ip| seen.insert(*ip))
.filter(|ip| {
let label = format!("{}:{} via {}", receiver_host, receiver_port, ip);
!current_labels.contains(&label)
})
.filter(|(_, label)| !current_labels.contains(label.as_str()))
.map(|(ip, _)| *ip)
.collect();
let attempted_new = new_ip_list.len();
let mut fresh: HashMap<String, SrtlaConnection> = if new_ip_list.is_empty() {
HashMap::new()
} else {
create_connections_from_ips(&new_ip_list, receiver_host, receiver_port)
.await
.into_iter()
.map(|c| (c.label.clone(), c))
.collect()
};
let added = fresh.len();
if !new_ips_needed.is_empty() {
let mut new_connections =
create_connections_from_ips(&new_ips_needed, receiver_host, receiver_port).await;
let added_count = new_connections.len();
connections.append(&mut new_connections);
let previous_order: SmallVec<u64, 4> = connections.iter().map(|c| c.conn_id).collect();
let mut survivors: HashMap<String, SrtlaConnection> = std::mem::take(connections)
.into_iter()
.filter(|c| desired_labels.contains(c.label.as_str()))
.map(|c| (c.label.clone(), c))
.collect();
if added_count > 0 {
info!("added {} new connections", added_count);
} else {
warn!(
"failed to add any new connections (attempted {})",
new_ips_needed.len()
);
for (_, label) in &desired {
if let Some(conn) = survivors.remove(label) {
connections.push(conn);
} else if let Some(conn) = fresh.remove(label) {
connections.push(conn);
}
let survivor_count = connections
.iter()
.filter(|c| desired_labels.contains(c.label.as_str()))
.count();
let new_ip_list: SmallVec<IpAddr, 4> = desired
.iter()
.filter(|(_, label)| !current_labels.contains(label.as_str()))
.map(|(ip, _)| *ip)
.collect();
let attempted_new = new_ip_list.len();
let mut fresh: HashMap<String, SrtlaConnection> = if new_ip_list.is_empty() {
HashMap::new()
} else {
create_connections_from_ips(&new_ip_list, receiver_host, receiver_port)
.await
.into_iter()
.map(|c| (c.label.clone(), c))
.collect()
};
let added = fresh.len();
if !connections.is_empty() && survivor_count == 0 && attempted_new > 0 && added == 0 {
warn!(
"refusing reload: all replacement uplinks failed to connect; keeping existing connections"
);
return;
}
// Drop the leaving links' sequence entries so a stale NAK can't be
// misattributed to whichever uplink later reuses that index.
let removed_conn_ids: SmallVec<u64, 4> = connections
.iter()
.filter(|c| !desired_labels.contains(c.label.as_str()))
.map(|c| c.conn_id)
.collect();
let removed = removed_conn_ids.len();
for conn_id in &removed_conn_ids {
seq_tracker.remove_connection(*conn_id);
}
let previous_order: SmallVec<u64, 4> = connections.iter().map(|c| c.conn_id).collect();
let mut survivors: HashMap<String, SrtlaConnection> = std::mem::take(connections)
.into_iter()
.filter(|c| desired_labels.contains(c.label.as_str()))
.map(|c| (c.label.clone(), c))
.collect();
for (_, label) in &desired {
if let Some(conn) = survivors.remove(label) {
connections.push(conn);
} else if let Some(conn) = fresh.remove(label) {
connections.push(conn);
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sender/connections.rs` around lines 42 - 83, The issue is that stale
sequence entries are removed from seq_tracker before validating whether the new
connections can be successfully created, and the connection pool is rebuilt
regardless. If all new connections fail (resulting in empty fresh HashMap) while
the current pool is non-empty, this causes all working uplinks to be dropped.
Move the loop that calls seq_tracker.remove_connection for each conn_id in
removed_conn_ids to execute only after the fresh HashMap is populated and
validated. Additionally, add a guard condition that returns early without
mutating the connection pool when new_ip_list is non-empty but fresh ends up
empty (indicating connection creation failed) while the current connections pool
is non-empty, preventing the transition from a healthy pool to an empty one due
to failed replacement creation.

Comment thread src/telemetry_file.rs
Comment on lines +193 to +206
fn write_tmp(tmp: &Path, json: &str) -> io::Result<()> {
let mut file = File::create(tmp)?;
file.write_all(json.as_bytes())?;
file.sync_all()
}

/// Atomically publish `json` to `path`: write a `.tmp` sibling, fsync, then
/// `rename(2)` over the live path. On the same filesystem rename is atomic, so a
/// concurrent reader only ever sees a complete previous-or-next document. On any
/// I/O error the temp sibling is removed and the previous snapshot is left in
/// place (to go stale) rather than vanishing.
pub fn write_atomic(path: &Path, json: &str) -> io::Result<()> {
let tmp = tmp_path(path);
match write_tmp(&tmp, json).and_then(|()| fs::rename(&tmp, path)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Harden the predictable .tmp publish path against symlink clobbering.

File::create(tmp) follows an attacker-controlled symlink at <stats-file>.tmp. If the stats path lives in a world-writable directory and srtla_send runs with elevated privileges, a local user can make the telemetry publish truncate or replace another file. Create the temp file with no-follow/create-new semantics, or use a safely-created same-directory temp file before the final rename.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/telemetry_file.rs` around lines 193 - 206, The write_tmp function creates
a temporary file at a predictable path using File::create(tmp), which follows
symlinks and is vulnerable to symlink-clobbering attacks if the stats directory
is world-writable and the process has elevated privileges. Fix this by modifying
the file creation in write_tmp to use no-follow or create-new semantics, or
alternatively use OpenOptions to create the file with OpenFlags that prevent
symlink following. Ensure the temporary file cannot be hijacked by an
attacker-controlled symlink before the atomic rename in write_atomic completes.

Comment thread src/telemetry_file.rs
Comment on lines +249 to +253
pub fn publish(&self, stats: &StatsSnapshot) {
let json = build_telemetry_json(now_ms(), &conns_from_stats(stats));
if let Err(e) = write_atomic(&self.path, &json) {
let path = self.path.display();
warn!("telemetry stats-file write failed: {path}: {e}");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Move telemetry fsync/rename off the Tokio packet-forwarding loop.

publish() does blocking filesystem I/O plus sync_all(), and src/sender/mod.rs calls it directly from the async sender loop. On slow storage this can pause UDP handling; use a spawn_blocking publish path or a dedicated telemetry worker so atomic stats writes cannot stall live forwarding.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/telemetry_file.rs` around lines 249 - 253, The publish() method performs
blocking filesystem I/O operations (write_atomic and sync_all) directly within
the async sender loop, which can stall UDP packet forwarding on slow storage.
Move the blocking I/O off the Tokio event loop by either wrapping the publish()
call with tokio::task::spawn_blocking in the caller at src/sender/mod.rs, or by
creating a dedicated telemetry worker thread that receives stats snapshots
asynchronously and handles the atomic writes independently. This ensures the
async sender loop remains non-blocking and responsive to UDP packet forwarding.

Comment thread src/telemetry_file.rs
Comment on lines +508 to +524
let mut parse_errors = 0;
for _ in 0..1000 {
if let Ok(content) = fs::read_to_string(&path)
&& serde_json::from_str::<serde_json::Value>(&content).is_err()
{
parse_errors += 1;
}
}

// Ensure the writer thread has actually run before stopping it.
// Bounded spin-wait to prevent writer starvation under parallel load.
for _ in 0..100 {
if writes.load(Ordering::Relaxed) > 0 {
break;
}
thread::sleep(Duration::from_millis(10));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Start the writer before the torn-read assertion loop.

This test can finish the 1000 read attempts before the writer thread performs its first write; the later spin-wait only proves a write happened after the sampling window. Wait for writes > 0 before entering the read loop so the test actually exercises concurrent publish/read behavior.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/telemetry_file.rs` around lines 508 - 524, The spin-wait loop that checks
writes.load(Ordering::Relaxed) > 0 currently executes after the 1000 read
attempts, allowing all reads to potentially complete before the writer thread
has even started. Move the bounded spin-wait loop (the 0..100 iteration that
waits for writes > 0) to execute before the parse_errors loop (the 0..1000
iteration). This ensures the writer thread has performed at least one write
before the concurrent read sampling begins, properly exercising the concurrent
publish/read behavior the test is intended to validate.

@andrescera andrescera closed this Jun 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant