Skip to content

Commit d90590f

Browse files
Retry failed scheduler priority notifications (#122)
## Description Closes #121. Record a priority snapshot as emitted only after its JSON-RPC notification write succeeds. Previously a failed write updated the delivery cache first, so ordinary reconciliation suppressed retries and scheduler status incorrectly showed the failed update as delivered. The change keeps notification I/O outside the state mutex and retains the existing recompute serialization. Each engine remains independent: a successful delivery is not repeated because another engine failed. Ranking policy, wire fields, retry cadence and receiver behavior are unchanged. ## Release intent <!-- pair-release-intent:v1 --> ### Changelog title Retry failed scheduler priority notifications ### Changelog body The scheduler retries changed priorities after a failed local notification write and keeps its status aligned with the last successful delivery. ### Bumps - services: patch - nvpair-cluster-manager: none - nvpair-engine-manager: none - nvpair-errors: none - nvpair-job-scheduler: patch - nvpair-manual-nodes: none - nvpair-node-info: none - nvpair-node-scanner: none - nvpair-node-settings: none - nvpair-proxy: none - nvpair-tui: none - nvpair-ui-broker: none - nvpair-workload-manager: none <!-- /pair-release-intent:v1 --> ## Scope Only the scheduler delivery bookkeeping, its regression tests and its README are changed. The scheduler binary and services aggregate receive patch intent. No version files are edited by hand. The broker, shared wire types, proxies, desktop and terminal interface need no contract changes. Existing cross-process scheduler tests verify the emitted frames and ranking behavior. The open engine-addition PRs modify the engine list in the same source file, not this delivery path. ## Validation - Five new regression scenarios fail before the fix and pass afterward. They cover initial write failure, error and zero-byte retry paths, retention of an older successful snapshot, and failed forced-refresh timestamps. - The complete scheduler package passes with race detection on macOS and Linux arm64: 36 top-level tests and 11 subtests, no failures or skips. - Four existing cross-process scheduler tests pass, driving real built binaries through stdin/stdout. They cover workload updates, telemetry, restart replay and a 50-job synthetic burst. The harness builds the service binaries but the selected tests start only the scheduler. - All 16 shared JSON-RPC tests pass with race detection on macOS. - `go vet`, the scheduler build, `gofmt`, repository SPDX-header checks and `git diff --check` pass. The release-intent description validates locally. - Linux tests run in an isolated container with networking disabled. App-data paths are isolated under temporary directories. ```sh cd services/nvpair-job-scheduler go test -race -count=1 ./... go vet ./... go build . # From services/tests: go test -race -count=1 -run '^TestScheduler' ./... # From services/shared: go test -race -count=1 ./jsonrpc # From the repository root: node scripts/spdx-headers.mjs ``` Tested with Go 1.27.1. The desktop gates, whole cross-process suite, live engines and multi-node integration were not run. No test or runtime guard was disabled. These tests cover failures before any frame bytes are delivered; they do not claim recovery from partial-frame corruption. ## Risk Low, localized state-ordering correction. No new dependency, network listener, persistent data, protocol field or service boundary. The previous snapshot remains authoritative when a new write fails. The issue was reported on 25 September 2026 and rechecked for assignments and competing fixes before submission. ## Checklist - [x] I have read the contributing guidelines. - [x] Every commit is signed off. - [x] Regression and existing tests cover the change. - [x] Relevant documentation is updated. - [x] The diff and commit contents contain no credentials, private data or generated artifacts. - [x] Validation commands and limitations are recorded above. - [x] Version bumps are declared in the release-intent block; automation-owned version files are unchanged. Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
1 parent 603fb0c commit d90590f

3 files changed

Lines changed: 120 additions & 3 deletions

File tree

‎services/nvpair-job-scheduler/README.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,3 +129,11 @@ go test ./...
129129
- [`../nvpair-workload-manager/README.md`](../nvpair-workload-manager/README.md)
130130
— where workload state originates
131131
- [`../VERSIONING.md`](../VERSIONING.md) — SemVer bump rules
132+
133+
## Delivery failures
134+
135+
The last-emitted snapshot and timestamp are updated only after the local
136+
JSON-RPC notification write succeeds. A failed update leaves the previous
137+
successful snapshot intact, so ordinary reconciliation retries changed ranks
138+
without requiring another workload event. Each engine is tracked independently;
139+
a successful delivery is not repeated merely because another engine failed.
Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package main
5+
6+
import (
7+
"io"
8+
"testing"
9+
)
10+
11+
// unavailableWriter is an in-memory test fixture; it opens no files or sockets.
12+
type unavailableWriter struct{}
13+
14+
func (unavailableWriter) Read([]byte) (int, error) { return 0, io.EOF }
15+
func (unavailableWriter) Write([]byte) (int, error) { return 0, io.ErrClosedPipe }
16+
17+
func TestFailedNotificationDoesNotAdvanceStatus(t *testing.T) {
18+
m := mgrWith(unavailableWriter{}, []string{"test-node-a"})
19+
m.recomputeAll(false)
20+
for _, engine := range schedulerEngines {
21+
status := m.status().Engines[engine]
22+
if status.LastEmittedAt != 0 || len(status.Emitted) != 0 {
23+
t.Fatalf("failed notification was recorded as emitted: %+v", status)
24+
}
25+
}
26+
}
27+
28+
// recoveringWriter simulates one failed local codec write, then records frames.
29+
type recoveringWriter struct {
30+
capRW
31+
remaining int
32+
shortWrite bool
33+
}
34+
35+
func (w *recoveringWriter) Write(data []byte) (int, error) {
36+
if w.remaining > 0 {
37+
w.remaining--
38+
if w.shortWrite {
39+
return 0, nil
40+
}
41+
return 0, io.ErrClosedPipe
42+
}
43+
return w.capRW.Write(data)
44+
}
45+
46+
func TestFailedNotificationRetriesWithoutChangingRanks(t *testing.T) {
47+
for _, shortWrite := range []bool{false, true} {
48+
name := "write-error"
49+
if shortWrite {
50+
name = "zero-byte-write"
51+
}
52+
t.Run(name, func(t *testing.T) {
53+
writer := &recoveringWriter{remaining: 1, shortWrite: shortWrite}
54+
m := mgrWith(writer, []string{"test-node-a", "test-node-b"})
55+
m.recomputeAll(false)
56+
m.recomputeAll(false)
57+
m.recomputeAll(false)
58+
for _, engine := range schedulerEngines {
59+
orders := writer.orders(engine)
60+
if len(orders) != 1 {
61+
t.Fatalf("%s received %d frames, want one successful delivery", engine, len(orders))
62+
}
63+
assertStrs(t, orders[0], []string{"test-node-a", "test-node-b"})
64+
if m.status().Engines[engine].LastEmittedAt == 0 {
65+
t.Fatal("successful delivery was not recorded")
66+
}
67+
}
68+
})
69+
}
70+
}
71+
72+
func TestFailedChangedNotificationRetainsDeliveredRanks(t *testing.T) {
73+
writer := &recoveringWriter{}
74+
m := mgrWith(writer, []string{"test-node-a", "test-node-b"})
75+
m.recomputeAll(false)
76+
engine := schedulerEngines[0]
77+
previous := m.status().Engines[engine]
78+
writer.remaining = 1
79+
m.nodes["test-node-c"] = true
80+
m.recomputeAll(false)
81+
current := m.status().Engines[engine]
82+
if !equalRanks(current.Emitted, previous.Emitted) || current.LastEmittedAt != previous.LastEmittedAt {
83+
t.Fatal("failed update replaced the last successfully delivered snapshot")
84+
}
85+
m.recomputeAll(false)
86+
orders := writer.orders(engine)
87+
if len(orders) != 2 {
88+
t.Fatalf("received %d snapshots, want initial and recovered update", len(orders))
89+
}
90+
assertStrs(t, orders[1], []string{"test-node-a", "test-node-b", "test-node-c"})
91+
}
92+
93+
func TestFailedForcedNotificationRetainsDeliveredTimestamp(t *testing.T) {
94+
writer := &recoveringWriter{}
95+
m := mgrWith(writer, []string{"test-node-a"})
96+
m.recomputeAll(false)
97+
engine := schedulerEngines[0]
98+
m.emitted[engine] = engineState{ranks: m.emitted[engine].ranks, lastEmittedAt: 1}
99+
writer.remaining = 1
100+
m.recomputeAll(true)
101+
if got := m.status().Engines[engine].LastEmittedAt; got != 1 {
102+
t.Fatalf("failed forced delivery advanced timestamp to %d", got)
103+
}
104+
m.recomputeAll(false)
105+
if got := len(writer.orders(engine)); got != 1 {
106+
t.Fatalf("previously delivered unchanged ranks were emitted %d times", got)
107+
}
108+
}

‎services/nvpair-job-scheduler/schedule.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -136,9 +136,6 @@ func (m *Manager) emitIfChanged(engine string, order []string, ranks []NodeRank,
136136
m.mu.Lock()
137137
prev := m.emitted[engine]
138138
changed := force || !equalRanks(prev.ranks, ranks)
139-
if changed {
140-
m.emitted[engine] = engineState{ranks: ranks, lastEmittedAt: time.Now().UnixMilli()}
141-
}
142139
m.mu.Unlock()
143140

144141
if !changed {
@@ -149,6 +146,10 @@ func (m *Manager) emitIfChanged(engine string, order []string, ranks []NodeRank,
149146
slog.Warn("emit schedule:priority failed", "engine", engine, "err", err)
150147
return
151148
}
149+
m.mu.Lock()
150+
m.emitted[engine] = engineState{ranks: ranks, lastEmittedAt: time.Now().UnixMilli()}
151+
m.mu.Unlock()
152+
152153
slog.Info("emitted priority", "engine", engine, "nodes", order)
153154
}
154155

0 commit comments

Comments
 (0)