Skip to content

Fix workload broadcast ordering and record dedup keys only after successful emit - #80

Open
mkalkere wants to merge 3 commits into
NVIDIA:mainfrom
mkalkere:fix/workload-broadcast-reliability
Open

mkalkere wants to merge 3 commits into
NVIDIA:mainfrom
mkalkere:fix/workload-broadcast-reliability

Conversation

@mkalkere

@mkalkere mkalkere commented Sep 14, 2026

Copy link
Copy Markdown

Description

Fixes two reliability bugs in services/nvpair-workload-manager that could corrupt the scheduler's view of cluster load:

  1. Inter-node workload broadcasts are now serialized in origin order. Every frame was fanned out in its own goroutine, so a lifecycle remove could overtake its own upsert on a peer and resurrect a ghost workload -- phantom load in the scheduler. A single ordered worker (broadcastCh + broadcastLoop) now emits frames in order; a full queue drops with a warning and the heartbeat/backfill re-syncs.
  2. Dedup keys are recorded only after the broker emit succeeds. Previously the key was recorded before emit, so a failed emit returned 500 and the retry was swallowed as a duplicate -- the event was lost with no repair path. dedupIndex.seen()/add() are now split and the key is added only on success.

Scope

Included: services/nvpair-workload-manager only, broadcast_order_test.go, dedup_after_emit_test.go, version bump, docs/workload-broadcast-reliability.mdx. Excluded: frozen-node handling (issue #6, separate discussion).

Validation

  • go test -race ./... in services/nvpair-workload-manager: pass.
  • broadcast_order_test.go: 25 frames through the real cluster-mTLS broadcast path arrive in exact origin order.
  • dedup_after_emit_test.go: failed lifecycle/remove emit returns 500 with no dedup recorded, retry emits exactly once, genuine duplicates still deduped.
  • Toolchain: Go 1.26.8 (repo requires Go 1.25+), Linux sandbox. go vet clean, gofmt clean, node scripts/spdx-headers.mjs reports 0 missing headers on every branch.

Risk

  • Ordering change is confined to the outbound broadcast path; no wire-format changes.
  • The dedup change only affects the failed-emit path; the success path is byte-identical in behavior.

Checklist

  • I have read the Contributing Guidelines.
  • Every commit is signed off (git commit -s), certifying the Developer Certificate of Origin.
  • New or existing tests cover the change.
  • Relevant documentation is updated.
  • I checked the diff, changed filenames, and commit messages for credentials, private data, internal URLs, internal issue identifiers, and generated artifacts.
  • I recorded the validation commands and results above.
  • I bumped any affected component in services/versions.json, and described user-visible changes above so they reach the release notes.

Replace the per-frame goroutine fan-out with a single ordered broadcast
worker draining a bounded queue, so a remove can never overtake the
lifecycle upsert it follows and resurrect a ghost workload on peers.

Signed-off-by: mkalkere <14184493+mkalkere@users.noreply.github.com>
Split the dedup check from the record: a failed broker emit answers 500
without recording the key, so the peer retry is emitted instead of being
swallowed as a duplicate. Applies to lifecycle upserts and removals.

Signed-off-by: mkalkere <14184493+mkalkere@users.noreply.github.com>
Ordering diagrams for the serialized broadcast worker and the
dedup-after-emit sequence, plus a reading-order entry in the README.

Signed-off-by: mkalkere <14184493+mkalkere@users.noreply.github.com>
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