Skip to content

fix(cluster-manager): announce only persisted endorsement changes - #62

Open
RecursiveIntell wants to merge 3 commits into
NVIDIA:mainfrom
RecursiveIntell:ares/pair-trust-fix-20260911
Open

RecursiveIntell wants to merge 3 commits into
NVIDIA:mainfrom
RecursiveIntell:ares/pair-trust-fix-20260911

Conversation

@RecursiveIntell

@RecursiveIntell RecursiveIntell commented Sep 11, 2026

Copy link
Copy Markdown

Summary

  • Emit truststore change notifications only after a newly received endorsement has been successfully persisted.
  • Keep duplicate endorsements, identical re-pins, empty merges, and other tested no-op operations silent.
  • Keep failed endorsement writes silent and leave the in-memory truststore unchanged.
  • Add focused regression coverage for direct endorsement merges, identical-pin endorsement merges, duplicate suppression, and persistence failures.
  • Bump nvpair-cluster-manager from 1.1.4 to 1.1.5.

This is a notification/persistence-correctness fix in the truststore. It does not claim to strengthen cryptographic trust, pairing-PIN security, authorization, certificate validation, endorsement verification, or mTLS configuration.

Scope

Changed paths are limited to:

  • services/nvpair-cluster-manager/truststore.go
  • services/nvpair-cluster-manager/truststore_announce_test.go
  • services/versions.json

Validation

Passed on commit be34de2:

go test ./... -run 'TestTrustStore(AnnouncesEveryMutation|AnnouncesNewEndorsementOnce|AnnouncesNewEndorsementOnIdenticalPinOnce|StaysSilentWhenEndorsementWriteFails|StaysSilentWhenNothingChanged)' -count=1
go test ./... -race -run 'TestTrustStore(AnnouncesEveryMutation|AnnouncesNewEndorsementOnce|AnnouncesNewEndorsementOnIdenticalPinOnce|StaysSilentWhenEndorsementWriteFails|StaysSilentWhenNothingChanged)' -count=1
go test ./... -count=1
go test ./... -race -count=1
go vet ./...
git diff origin/main...HEAD --check

The full nvpair-cluster-manager normal suite and independently rerun race suite pass. The race run validates this module under Go's race instrumentation; the added tests are focused regression tests, not a claim of universal race-freedom or a separate concurrency-specific regression fixture.

Broader validation status

The repository-wide make test gate is not green:

  • ordinary make test stops in services/lmstudio-proxy because the shallow candidate cannot obtain Go VCS status (error obtaining VCS status: exit status 128);
  • GOFLAGS=-buildvcs=false make test reaches the LM Studio tests but fails TestHandleHTTP_RealSocketFlushDeadline with workload never terminated after the client stopped reading (flush path not deadline-aware);
  • the same failing test was reproduced on a clean origin/main control worktree, so it is currently preserved as a baseline/unrelated failure rather than attributed to this change.

This PR must not be described as repository-wide green, fully validated, release-ready, or merge-ready until CI and maintainer review complete.

Security and privacy notes

This touches persisted certificate endorsements and membership-propagation notifications, so it remains security-sensitive code. The change does not intentionally modify certificate validation, endorsement verification, admission authorization, PIN handling, revocation, or network exposure. Tests use synthetic peer identifiers, signatures, certificates, and temporary directories. No credentials, private keys, real cluster identities, customer data, or personal data were added.

Existing PAIR security caveats remain applicable, including the low-entropy temporary pairing PIN and the local-network trust boundary.

Review status

  • Public-evidence review completed against exact HEAD be34de2.
  • Review outcome: revise wording before treating the candidate as fully publication-ready; draft-PR publication remains bounded by the qualifiers above.
  • The earlier commit subject's phrase “race coverage” refers only to module-suite execution under race instrumentation; this PR does not claim that the added fixtures are concurrency-specific or universally race-free.
  • Both commits contain matching Signed-off-by DCO trailers.
  • Merge is not requested by this PR creation step; maintainers must review the broader baseline failure and required CI results.

Emit truststore change announcements only after an endorsement merge adds and persists a new endorsement. Keep idempotent re-pins, empty or duplicate merges, and failed writes silent, with component-level regression and race coverage.

Signed-off-by: Josh Stevenson <j.stevenson.cs@gmail.com>
Signed-off-by: Josh Stevenson <j.stevenson.cs@gmail.com>
Verify callback read-back and disk reload for direct and identical-pin merges, including mixed and duplicate batches. Exercise replace failures through both entry points, preserve old disk and live state, and verify recovery without temporary residue. Add concurrent identical submissions and missing-target regression coverage.

Signed-off-by: Josh Stevenson <j.stevenson.cs@gmail.com>
@RecursiveIntell

Copy link
Copy Markdown
Author

Reconciliation and validation update

Rechecked this PR against NVIDIA main at 13b68115fa2c9c1d94f1ead1358f8d5a527cfecf. That commit is already an ancestor of the branch, so no rebase, merge commit, or history rewrite was necessary. Updated head: da7df0a48060d7e729519fb5d6884778fb604c29.

What changed in this follow-up

The new commit is test-only; it preserves the existing runtime fix and the nvpair-cluster-manager patch bump from 1.1.4 to 1.1.5. The full PR remains limited to truststore.go, truststore_announce_test.go, and services/versions.json.

The tests now check the notification/persistence boundary rather than treating an in-memory read as disk-persistence evidence:

  • Both AddEndorsements and identical-certificate, same-admission-epoch Pin merge an existing/new/duplicate batch with one notification. Repeating the batch or submitting an empty batch stays silent and leaves the stored contents unchanged.
  • The notification callback reads the live pin and opens a fresh truststore from disk. The callback successfully read the updated live pin and reloaded it from disk; its read-lock acquisition provides evidence that the mutation lock was released before the callback ran.
  • An injected failure at the final file-replacement step returns an error through both APIs, emits no notification, preserves the old live and on-disk endorsements, and leaves no temporary file. Retrying after storage recovery persists the update and announces once.
  • In the focused race run, each of 50 repetitions exercised 16 concurrent identical submissions, split between the two APIs; each repetition produced one stored endorsement and one notification. The observer uses an atomic counter.
  • Adding endorsements for a missing peer stays silent without creating live or on-disk state.

As a regression control, I ran the same test file against otherwise unchanged main at the SHA above. It fails on duplicate/no-op notifications, the missing-peer case, and the concurrent case (16 notifications instead of 1). Those cases pass with this PR's implementation. The failed-write cases already pass on main; they protect existing behavior rather than claiming a newly repaired write-atomicity defect.

Fresh local validation

Validation was executed on the pre-commit worktree whose three changed-path SHA-256 values match the blobs committed at da7df0a; those receipts therefore record parent HEAD be34de2, not the candidate commit itself. The focused/module checks passed against those matching candidate blobs. Repository-wide make test remains blocked by failures reproduced on clean main. After committing, I also reran go test ./... -race -run TestTrustStore -count=1 -timeout=60s -json from the cluster-manager module at da7df0a; it passed with no failed or skipped test events.

Environment: Linux/amd64, Go 1.25.6, Node 26.0.0, npm 11.12.1; Go checks used GOMAXPROCS=2.

From services/nvpair-cluster-manager:

go test ./... -count=1 -timeout=180s -json
go test ./... -race -count=1 -timeout=180s -json
go test ./... -race -run TestTrustStore -count=50 -timeout=120s -json
go vet ./...

All passed. The full module normal and race runs each executed 106 top-level tests with no failed or skipped test events. The repeated run includes the new concurrent regression; it is not a claim of universal race-freedom.

Also passed:

  • Repository make check: SPDX headers, build-script verification, lint, type checks, service contracts, and 208 desktop unit tests.
  • npm run dead-code:check from desktop.
  • gofmt verification and git diff --check.
  • A Linux cluster-manager build with VCS stamping disabled and the version read from services/versions.json; --version returned 1.1.5.
  • Real-process integration from services/tests, using temporary cluster configurations and synthetic peers:
GOFLAGS=-buildvcs=false go test ./... \
  -run '^TestCluster(ManagerPairing|Fanout|Leave)$' \
  -count=1 -timeout=180s -json

All three selected integration tests passed without skips. This covers pairing/removal, transitive membership propagation, and leave behavior; it is not a full cross-process-suite result.

Broader gate remains open

Repository-wide make test is not green:

  1. The ordinary command stops while LM Studio's TestMain builds its binary: error obtaining VCS status: exit status 128. The same build error occurs in the clean exact-main control.
  2. GOFLAGS=-buildvcs=false make test reaches the LM Studio suite but fails TestHandleHTTP_RealSocketFlushDeadline: workload never terminated after the client stopped reading (flush path not deadline-aware).
  3. That exact test fails with the same message on clean main under the same workaround. No LM Studio source is changed by this PR, and I have not hidden or skipped that failure to claim a full pass.

These are local results bound to the stated source snapshot, not hosted-CI results. No hosted checks are currently reported for this PR, and maintainer review is still required. Windows/macOS execution, power-loss durability, live-engine testing, and deployment are not claimed. The change does not modify certificate/endorsement verification, admission authorization, pairing-PIN security, or mTLS policy.

The original commits are retained, and the follow-up uses the same author with a matching DCO sign-off. This update does not merge the PR or change its draft state. The next gate is maintainer review of this focused fix and disposition of the broader baseline failure before treating the repository-wide validation gate as satisfied.

@RecursiveIntell
RecursiveIntell marked this pull request as ready for review September 13, 2026 04:28
@jlacroix82

Copy link
Copy Markdown

Independent confirmation from a 9-node fleet (8× DGX Spark + 1 gateway box, all on PAIR 0.1.1 services): every node's nvpair-node.service journal shows cluster:trust-changed at a constant ~5.3/s — exactly 3,200 events per 10-minute bucket on every Spark, all day, with no pairing activity and trusted/*.json mtimes untouched for a week.

Tracing it landed on the same line this PR fixes: in TrustStore.Pin, the identical-re-pin branch sets changed = err == nil after mergeEndorsementsLocked, so a gossip re-pin that merges nothing still fires announceannounceTrustChangedcluster:trust-changed. With nine peers gossiping, that is the storm. Each event also makes the broker re-derive scanner trust and push node-info, so it is not just log noise.

The fix here (announce only when an endorsement was actually persisted) matches what I would have written. +1 to landing it as a bug fix.

RecursiveIntell added a commit to RecursiveIntell/Personal-AI-Router that referenced this pull request Sep 15, 2026
Merge the reviewed three-path PAIR truststore correction into fork main. NVIDIA PR NVIDIA#62 remains untouched.
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.

2 participants