[countersyncd]: Harden and optimize HFT IPFIX processing

[Pterosaur]

Summary

Optimize the SONiC HFT IPFIX decoder and keep lifecycle handling local to one
IPFIX actor. This PR uses the clarified best-effort exporter contract, not a
lossless collector or cross-channel generation protocol.

Decoder and batching

• Replace ipfixrw and its thread-local cache with actor-owned compiled templates.
• Compile field offsets, widths, SAI IDs and shared object names once.
• Support unsigned counters of every width from 1 through 8 bytes in network byte order, with a small local 8-byte fast path.
• Scope templates by (observation_domain_id, template_id).
• Validate complete input framing and known Set layouts before emitting records or switching templates.
• Batch flat SAI stats and preserve decoded record order. 8,192 counters is only a batching target, never an input or record limit; larger records are sent intact.
• Retain defensive raw-input/metadata validation, bounded channel capacities, per-dequeue communication statistics and sink-error propagation. No artificial installed-template count/byte admission quotas.

Local best-effort lifecycle

• The deployed exporter allocates increasing template IDs and reuses them only after eventual wraparound, after old traffic has drained.
• Unknown-template Sets are immediately dropped and counted with rate-limited diagnostics; known Sets continue without a domain barrier.
• Compile and validate the complete new snapshot before registration. Keep at most active plus latest pending for each session, regardless of counter additions/removals/reorder/width changes.
• First validated nonempty data on a genuinely new pending ID promotes the entire session snapshot and removes old-only IDs. Unchanged shared IDs alone do not trigger cutover. A removal-only snapshot lacking a new ID cannot supply this signal and remains pending until a distinguishing update or local Delete/disable.
• No semantic matcher or per-template cutover map. Pending cancellation resends active; supersession removes only superseded pending-only entries.
• Delete/disable remove only the named session. Malformed templates remove that owner's decoder where necessary; later valid configuration resumes processing.
• A new registration colliding with an in-use (domain,id) logs ERROR and rejects without changing either owner's existing active/pending state. Shared unchanged entries remain valid; different domains do not collide; reuse after local removal is allowed.
• Runtime Redis HGETALL WRONGTYPE produces one complete reconciliation envelope. IPFIX arbitrates it atomically using current accepted ownership: still-claiming incumbents and unchanged pending sessions retain priority; unowned ambiguous claims are rejected, never chosen by order. Rejected snapshots retain previous active/pending state and propagate retained-key conflicts. Accepted changed snapshots recover legitimate Delete/recreate and pending releases. No synthetic reset gap. See crates/countersyncd/RECONCILIATION.md for the full policy and regression matrix. Transport errors remain fatal; Redis scan itself is non-atomic and startup-constructor WRONGTYPE is outside this path.

Removed scope

No HFT-triggered self-exec, restart notification/backoff/environment state, exec-only
FD sweep/watchdog, process-lifetime used-ID history, tombstones, owner/domain fences,
deferred queue, unknown-domain barriers, TTL replay, or sequence floors remain.
Earlier review requirements for these mechanisms were explicitly superseded.
Normal critical-actor supervision and the independently required OTel root-failure
exit-code classification remain.

Short startup/transition losses (unknown data before installation and late old-ID
data after cutover) are accepted. Sustained loss, crashes, unbounded growth and
incorrect steady-state decoding are not.

Validation

Validated in sonicdev-microsoft.azurecr.io:443/sonic-slave-bookworm:master-amd64
with CI common-lib/swss-common packages and Redis configured following
.azure-pipelines/build-template.yml. No host SONiC package installation.

RUSTFLAGS=-Dwarnings cargo check --locked -p countersyncd --all-targets
RUSTFLAGS=-Dwarnings cargo test --locked -p countersyncd
RUSTFLAGS=-Dwarnings cargo test --locked -p countersyncd -- --test-threads=1
RUSTFLAGS=-Dwarnings cargo bench --locked -p countersyncd --no-run
RUSTFLAGS=-Dwarnings cargo build --release --locked -p countersyncd
RUSTFLAGS=-Dwarnings cargo bench --locked -p countersyncd --bench end_to_end -- --test

Latest result: 350 test executions passed per parallel/serial run (161 library,
172 binary, 2 end-to-end integration, 11 IPFIX helper integration, 4 standalone
helper tests; some tests run in multiple targets). All-target check, bench
compilation, release build, and all five end-to-end consumption/DB-smoke scenarios
passed. Superseded replay/fencing/re-exec tests were removed; new tests cover
unknown-drop recovery, local delete/reinstall, whole-snapshot handover across changed counters,
shared-key and malformed non-promotion, pending cancellation/supersession, collisions
preserving active state, over 9,000 coexisting keys and over 64MiB storage without
quota rejection, WRONGTYPE recovery ordering, all widths and intact large records.

Historical Performance

The checked-in crates/countersyncd/benches/ipfix_actor_perf_results.md and baseline
patch retain historical measurements: candidate bff4dc87, baseline 0b4057cd,
unified-helper revision 2aa5c7ae. These are not measurements of this simplified
head, and the revision comparison is not an isolated helper-only A/B. Exact
historical CI artifact build IDs are unavailable. This scope change did not rerun
the full throughput comparison. End-to-end smoke success verifies consumption
and a DB smoke key, not all final persisted counter values.

Profile and Boundaries

• SONiC HFT Template Set ID 2, one 8-byte raw observationTimeNanoseconds, and 1-8-byte enterprise counters only.
• Existing private enterprise layout remains type_id << 16 | stat_id; the contrary HLD example needs a separate correction.
• No Options Templates, on-wire withdrawals, variable-length IEs or generic RFC NTP timestamp conversion.
• No guarantee for arbitrary delayed data surviving an entire template-ID reuse cycle, or global ordering across templates.
• OTel's existing full-width u64 to i64 conversion and encoded-request byte limits are separate work.

Latest recovery regressions cover current incumbents against previously rejected claims, unchanged pending snapshots, owner-order permutations, repeated recovery, transitive collision fallback, unowned ambiguity, same-owner recreation and pending release. Normal collision rejection remains unchanged. No throughput rerun for this exceptional recovery path.
SWSS key identity: SubscriberStateTable and Table::get_keys return table-relative keys; all notification/recovery owners are passed through unchanged. Regression coverage includes table-name-equal and prefix-sharing profiles, startup/runtime/reconciliation/delete, disabled auxiliary rows, active peer isolation and exact fixture namespace. Duplicate-owner validation remains strict.

Readability Refactor Validation

Commit 5794633 addresses the six nonblocking readability suggestions with nonrecursive typed envelopes, same-file read-only reconciliation planning, paired validated decoder/layout state, shared input constants, exhaustive scenario enums and local fixture reuse. 350 test executions plus 2 compile-fail doctests passed per parallel/serial run; all-target check, bench/release compilation and five end-to-end smoke scenarios passed.

Matched before/after (parent d614a46 ->57946334), CPU2, same container and workload: 2 counters 6.0918->6.0467 M metrics/s (-0.74%); 8000 counters62.302->62.308 (+0.01%); five single large60.152->60.678 (+0.87%); five four-large58.819->58.431 (-0.66%); mixed31.165->30.803 (-1.16%). Reverse-order mixed follow-up30.989->30.620 (-1.19%). Counts and batch counts match exactly. The small repeated mixed slowdown is disclosed, not called unchanged. See crates/countersyncd/benches/ipfix_readability_results.md for CIs, scope, toolchain, source blobs and raw-log checksums. This is a parent-to-refactor comparison, not a fresh upstream speedup claim.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

🔵 Needs a closer look

The change set is a large, cross-cutting refactor of the hot-path ingestion and actor messaging model (decoder + batching + lifecycle), which warrants final human review despite strong test/bench updates.

Pull request overview

This PR refactors countersyncd’s HFT IPFIX ingestion path to use a stricter, fixed-width decoder and to reduce per-record allocations/copies by introducing flat SAIStatsBatch delivery, tighter validation, bounded buffering/backpressure, and more explicit template/session lifecycle handling.

Changes:

• Replace per-record SAIStats messaging with flat SAIStatsBatch records and update downstream actors (StatsReporter/CounterDB/Otel) and tests/benches accordingly.
• Harden SWSS session handling by actively deactivating disabled/non-IPFIX sessions and validating object metadata (names/IDs) before forwarding templates.
• Optimize operational helpers (hex formatting and comm-stats tracking) and tighten CLI capacity parsing/bounds for batch channels.

File summaries

File	Description
crates/countersyncd/tests/ipfix_test_helpers.rs	Adds object-metadata generation helpers and hardens record generation against invalid declared lengths.
crates/countersyncd/tests/ipfix_helpers_integration.rs	Updates integration tests to consume SAIStatsBatch records and adds readiness/barrier probes to avoid sleep-based timing.
crates/countersyncd/tests/integration_test.rs	Adjusts end-to-end tests for batched stats delivery and more deterministic shutdown/join behavior.
crates/countersyncd/src/utilities/mod.rs	Optimizes hex formatting and replaces comm-stats mutex/map with fixed atomic slots.
crates/countersyncd/src/message/saistats.rs	Introduces SAIStatsBatch and SAIStatsRef, switches object names to Arc<str>, and centralizes enterprise-number decoding.
crates/countersyncd/src/message/otel.rs	Adapts OTEL conversion to borrowed SAIStatsRef and Arc<str> object names.
crates/countersyncd/src/main.rs	Updates actor supervision for IpfixActor result handling and adds bounded capacity parsing for batch channels.
crates/countersyncd/src/actor/swss.rs	Sends deactivation deletes for disabled/non-IPFIX sessions and validates object metadata strictly before forwarding templates.
crates/countersyncd/src/actor/stats_reporter.rs	Consumes batched stats records, uses Arc<str> keys, and updates aggregation/reporting accordingly.
crates/countersyncd/src/actor/otel.rs	Consumes batched stats records while preserving record order and updating buffering/flush behavior.
crates/countersyncd/src/actor/counter_db.rs	Consumes batched stats records, uses Arc<str> keys, and updates DB write path/tests accordingly.
crates/countersyncd/Cargo.toml	Removes unused dependencies (e.g., ipfixrw, rand, binrw) after decoder refactor.
crates/countersyncd/benches/otel_actor_perf.rs	Updates benchmarks to generate and send SAIStatsBatch inputs.
crates/countersyncd/benches/ipfix_bench_data.rs	Adds object metadata generation and readiness probe inputs; removes randomization helpers.
crates/countersyncd/benches/ipfix_actor_perf.rs	Updates IPFIX benchmark harness for batched output and readiness probe, and removes per-iteration input rebuild in timed region.
crates/countersyncd/benches/end_to_end.rs	Updates end-to-end benchmark pipeline for batched delivery, readiness synchronization, and stronger completion assertions.
crates/countersyncd/benches/counter_db_actor_perf.rs	Updates CounterDB benchmark to send batches and reduce channel/message overhead.
Cargo.toml	Removes unused workspace dependencies aligned with decoder refactor.
Cargo.lock	Lockfile updates reflecting removed dependencies and dependency graph simplification.

Review details

• Files reviewed: 18/20 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[50n1c-rnsft]

I found several correctness regressions in the new lifecycle/deferred-delivery paths. The fixed-width decoder and batching optimizations are promising, but these issues should be addressed before merging. I also recommend separating the decoder/batch refactor, lifecycle protocol, and benchmark/comm-stat changes so each contract and performance claim can be reviewed independently.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

🔵 Needs a closer look

It’s a large, performance-critical refactor that changes message shapes and lifecycle behavior across multiple actors/sinks, so it warrants final human verification despite strong test/bench updates.

Review details

• Files reviewed: 19/21 changed files
• Comments generated: 1
• Review effort level: Lite

[50n1c-rnsft]

Most of the first-round findings are fixed: deferred ordering is domain-scoped, expiry is timer-driven, Data Set padding and closed-first fanout are corrected, comm-stat deadlines are exact again, normal data is validated once, and the current benchmark harness uses pre-generated inputs. I reran the focused unit/integration suites successfully. However, three current-head regressions remain reproducible, including one stale-schema correctness issue, so I am keeping the review at request changes.

[50n1c-rnsft]

One additional lifecycle consequence remains separate from reversible disable/re-enable.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[50n1c-rnsft]

The previous round is substantially improved: stale live decoders are quarantined, Template Set padding is accepted, lifecycle marker memory is bounded, the false queue-length fence is removed, the benchmark uses an outer watchdog with a reproducible baseline patch, and historical IDs no longer consume the installed-key count. The existing 46 IPFIX unit tests and 4 integration tests pass in the CI-like Bookworm environment. However, the new exact-schema/history mechanism introduces reproducible generation-boundary and capacity failures, so this is not ready to approve yet.

[50n1c-rnsft]

Two SWSS control-path risks also remain; one predates this refactor but becomes safety-relevant now that invalid/stale control is expected to quarantine decoders.

[50n1c-rnsft]

One compatibility edge in the bounded exact-schema cache also needs clarification.

[50n1c-rnsft]

Full base-to-head review, consolidated as requested rather than stopping at the latest fix. The cancellation/supersession cleanup from the previous review is fixed. I reviewed parser, generation/deferred state, SWSS/restart, consumers, all benchmarks and tests, and checked all existing review threads. Existing all-targets checking and 405 test executions pass in the Bookworm CI-like container (187 library, 203 binary, 2 end-to-end, 9 integration, 4 helper). Five temporary deterministic tests expose four deferred/lifecycle behaviors and the supervisor exit-classification issue below. Other comments explicitly identify static evidence or nonblocking hardening/coverage concerns. These issues are in the full PR, not necessarily regressions caused by 8288650. No claim is made that green tests prove absence of all other bugs. Accepted soft batching target, fixed widths 1-8, and controlled destructive self-exec are not reopened.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[50n1c-rnsft]

Scope/requirements clarification after discussion with the requester: the deployed exporter allocates increasing template IDs and reuses them only on eventual wraparound; under the stated operating contract, old data has drained well before reuse. This telemetry pipeline is best-effort: short sample loss during startup/template changes is acceptable; sustained loss, permanent stalls, crashes/unbounded memory, and incorrect decoding of otherwise valid steady-state data are not. We want template switching serialized within the single IPFIX actor, not lossless cross-channel generation coordination.

I am explicitly withdrawing my earlier requirements to preserve every transition sample, establish a process-wide generation fence for distant ID reuse, or repair self-exec/deferred-replay machinery solely to meet those requirements. Those requests pushed this parser optimization toward unnecessary complexity. Please simplify/remove the mechanisms instead of continuing to patch their edge cases. Related historical comments about dropped transition samples, exact rejoin fencing, deferred replay and restart/FD/backoff support should be treated as superseded where their code is removed; they are not independent requirements to reimplement elsewhere. Retain genuine input validation, bounded resource use, steady-state correctness/recovery, and honest benchmark evidence.

The inline comments below describe one consistent reduced-scope design, not five unrelated features. Keep the compiled parser, fixed 1-8-byte unsigned counters, batching and shared metadata. No source edits or tests were performed for this requirements-only review; this is not an approval of the current large state machine.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[50n1c-rnsft]

The large scope reduction is appropriate and meets the best-effort requirements: self-exec/history/fences/replay are gone, Delete/Disable and rejection are local, unsigned widths1-8 and batching are retained. Full all-targets checking and 310 existing test executions pass in the Bookworm CI-like container (142 library,153 binary,2 end-to-end,9 integration,4 helper).

The remaining handover requirement has now been explicitly clarified by the requester: retain active and pending snapshots for the session/key regardless of counter-list changes, and promote the entire pending snapshot on its first validated new-template data, removing old-only entries. My earlier per-template/independent-pair wording was too restrictive and is superseded; I edited the original comment directly: #4860 (comment) .

Current lines290-335 instead pair ordered counter identities and immediately remove unmatched old templates. Lines383-414 promote only individual pairs. Two focused requirement tests fail: changing stat1 to stat2 under a new ID leaves only400 instead of retaining300+400; with old300/301 and pending400/401, the first400 record leaves old301 instead of completing the snapshot switch. These are one clarified contract mismatch, not a request for more lifecycle machinery. Remove the semantic matcher/cutover map and replace the independent-pair tests with session-snapshot tests (including additions/removals/reorder, malformed non-promotion, shared unchanged keys, pending supersession/cancel). No further high-confidence parser/consumer correctness finding was established in this review under the clarified best-effort assumptions. The same-ID wrap boundary should follow the documented drained-data exporter contract, not reintroduce permanent fences. Historical benchmark results remain explicitly historical; final-head throughput was not rerun. Required CI and real-device validation remain separate from these checks.

[50n1c-rnsft]

Additional full base-to-head pass, excluding the already-reported session-snapshot handover mismatch. One additional reproduced isolation defect: a runtime Redis WRONGTYPE row terminates the whole SWSS actor. A separate capacity/recovery limitation is documented below as nonblocking policy clarification, not a demand for replay/self-exec/retry machinery. In an isolated Bookworm CI-like container, focused tests reproduced both behaviors; temporary tests were removed. Existing all-targets and 310-test results on this unchanged head remain as reported in the previous review. No additional line-provable parser decoding, batch ownership/order, or consumer correctness defect was established under the final best-effort contract.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[50n1c-rnsft]

Reviewed all 23 changed files from base0b4057cd to72c143b1 against the final best-effort contract. Snapshot handover, collision preservation, removal of live-template quotas and removal of self-exec/history/barriers match the requirements. No additional high-confidence parser/batch/consumer blocker found. All-targets checking and 322 existing test executions pass in the Bookworm CI-like environment. Real Redis+IPFIX focused testing nevertheless reproduces the two stable recovery failures below; these need a simple removal/reinstall reconciliation boundary, not lossless replay or a dependency state machine. Final-head throughput was not remeasured. Required CI is still pending; temporary tests were removed.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[50n1c-rnsft]

The previous lost-Delete/recreate and pending-release findings are fixed by inspection and the new tests. Full current tests and all-targets checking pass in the Bookworm CI-like container (151 library,162 binary,2 end-to-end,11 integration,4 helper executions). One exceptional recovery/collision interaction remains reproducible with real Redis and the IPFIX actor. My previous recommendation to reset all observed owners did not account for a previously rejected colliding row still present in STATE_DB; that recommendation needs this qualification. This is not a request for lossless transition handling, self-exec, permanent ID history or template quotas. Ordinary snapshot handover and best-effort behavior remain consistent with the clarified requirements. Required CI is pending and temporary tests were removed.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[50n1c-rnsft]

The previous incumbent/collider finding is fixed: reconciliation now arbitrates in the IPFIX actor, preserves incumbent reservations and complete rejected snapshots, and commits synchronously. Prior delete/recreate and pending-release tests remain passing. Global review under the final best-effort contract found no other high-confidence parser/batch/consumer blocker. All-targets checking and 342 existing test executions pass in the Bookworm CI-like container. One real Redis+IPFIX regression remains: lossy owner normalization can create duplicate reconciliation owners from distinct valid profile names, aborting unrelated recovery. The helper predates this change, but rejection of the entire authoritative envelope amplifies its effect in this commit. Fixing the key identity boundary should reduce code, not add another state machine. Required CI remains pending; temporary tests were removed.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[50n1c-rnsft]

Reviewed the latest identity fix and rechecked the full PR against the final best-effort requirements. Table-relative SWSS keys now remain unchanged through startup, SET/DEL, reconciliation and fixture helpers; the prior alias/duplicate-owner recovery failure is fixed without weakening envelope validation. The new real-Redis tests cover table-looking names, active-profile isolation, unrelated-owner recovery and exact namespace cleanup. No new actionable blocker found. Incumbent preservation and prior Delete/recreate/pending-release recovery remain covered.

The final design retains session-level active/latest-pending handover on first valid genuinely new-key data, local removal/recovery, unsigned widths1-8, complete-record batching, and collision rejection preserving existing decoders. It does not restore self-exec, permanent ID history, deferred data barriers/replay or artificial live-template quotas. Earlier demands superseded by the agreed best-effort contract are not being reimposed.

Validation on d614a46 in sonic-slave-bookworm:master-amd64 with the cached common-lib/swss-common CI artifacts: RUSTFLAGS=-Dwarnings cargo check --locked -p countersyncd --all-targets passed; complete serial and default-parallel cargo test --locked -p countersyncd runs each passed 350 test executions (161 library,172 binary,2 end-to-end,11 integration,4 helper). Checkout remains clean.

This is code-review approval, not a claim that all platform validation is complete. Azure build/ASAN and C++ CodeQL are still pending at submission; merge must remain gated on required CI. I did not run physical-device testing or remeasure final-head throughput. Redis snapshot collection remains non-atomic and runtime WRONGTYPE classification depends on the documented native-library error format; these are documented verification limits, not newly demonstrated blockers.

[50n1c-rnsft]

Architecture/readability-only review of this PR's changes. These are nonblocking maintainability suggestions, not new functional defects and not a withdrawal of my approval. The current ownership boundaries are sensible: one IPFIX actor owns live template state, SWSS translates database input, and downstream actors share record-preserving flat batches. I do not recommend new frameworks, locks, replay state, or splitting files merely because their tests make them long. Prioritize small changes that express existing invariants and remove duplication; any chosen refactor should preserve the approved behavior and rerun existing tests. No source edits or new test runs were performed for this static review.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).