fix(runtime): read indexed servers before active ones in orphan cleanup

[Sriram-PR]

Description

Fixes item 2 of #1465 (TestRetrieveTools_ProfileWidenedByRawApplyIsSearchableImmediately returning nil hits on slow runners).

cleanupOrphanedIndexEntries read the active server names first and the indexed names second. The startup backgroundToolIndexing goroutine runs it, so on a loaded runner it could take the active list before the test fixture's LoadConfiguredServers registered a and b, then read the indexed list after the fixture seeded their tools, and delete them as orphans. The profile rebuild then found nothing for b. Caught with a stack trace on the delete:

DeleteServerTools("b") <- cleanupOrphanedIndexEntries <- backgroundToolIndexing <- backgroundInitialization

The same window exists outside tests: a server added while the startup cleanup runs could lose its freshly indexed tools until the next discovery pass.

Fix: read the indexed names first. A server is registered before its tools are indexed, so a server added mid-cleanup is either not in the indexed list yet or already in the active list read after it. A server that was really removed is still cleaned up. The two reads and the comparison moved into findOrphanedIndexServers so the ordering can be tested without timing.

Testing

• I have tested these changes locally

• I have added/updated tests that prove my fix is effective or my feature works

• All existing tests pass

• TestFindOrphanedIndexServers_ServerAddedDuringCleanupIsKept adds a server right after the first read, whichever it is. It fails with the old order ([b] reported as orphan) and passes now. TestFindOrphanedIndexServers_RemovedServerIsOrphan covers the normal case.

• The original flake only shows up under load (the CI seed alone didn't reproduce it). With 16 parallel copies of the test and the CPU saturated it failed about 2% of runs before; with this branch 0 failures across 300 runs of the three ProfileWidened tests under the same load.

• go test -race ./internal/runtime/... passes, and golangci-lint with .github/.golangci.yml reports nothing in the changed files.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 56.00000% with 11 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/runtime/lifecycle.go	56.00%	9 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[Dumbris]

Thank you @Sriram-PR, this is a great fix and an unusually well-investigated one.

What we liked:

• The diagnosis. You noticed that cleanupOrphanedIndexEntries read the active servers before the indexed ones. That leaves a window where a server registered and indexed between the two reads gets deleted as an "orphan". The stack trace of the actual delete and the stress run under load made the cause very convincing. It explains the flaky TestRetrieveTools_ProfileWidenedByRawApplyIsSearchableImmediately, and the same window exists in production for servers added while startup cleanup is running.
• The fix. Reading indexed names first works because a server is always registered as an upstream client before its tools are indexed. So anything in the indexed snapshot is also in the later active snapshot, unless it really was removed.
• The test design. Moving the comparison into findOrphanedIndexServers(listIndexed, listActive) makes the ordering itself unit-testable. The deferred addServer() trick is neat: it checks the read order directly instead of depending on timing. We confirmed the test catches the bug: swapping the reads back to active-first makes TestFindOrphanedIndexServers_ServerAddedDuringCleanupIsKept fail with Should be empty, but was [b].

We verified the change on top of current main:

• Both editions build.
• go test -race ./internal/runtime/... passes.
• golangci-lint is clean on your files.
• CI was green on your commits (it re-runs now for the comment edit; auto-merge is armed).

It also got an independent cross-model review, which approved it.

One small change we pushed to your branch. The reviewer pointed out that the new ordering comment slightly overstates the guarantee. It holds for servers newly added while cleanup runs. At startup, though, index entries persisted from a previous run can exist before the server's async re-registration finishes, so those can still be pruned and are then re-indexed by discovery (the same as before your change). We added a sentence to the comment to say exactly that. It's a comment-only edit, and your commits and authorship are untouched.

The reviewer also noted a separate, older race: a server removed and re-added between the snapshot and DeleteServerTools can still lose its tools. That race predates this PR, and your change makes the window narrower, so we'll track it on its own.

Since this PR resolves only item 2, #1465 stays open for the remaining items. Thanks again for the careful work. Contributions like this are very welcome here, and we'd be glad to see more from you!