MIR-856: Clean orphaned entity index entries

[evanphx]

Summary

• Keep orphan repair in the existing bounded background index GC; foreground lists remain read-only.
• Silence missing backing entities at normal log levels in List, ListPage, and ListDocuments, while retaining ERROR logs for undecodable records.
• Deliver ID-only WatchIndex delete events without ERROR when both current and historical entity reads confirm the orphan is missing.

Verification

• iso run -- bash hack/test.sh './pkg/entity ./servers/entityserver ./controllers/indexgc' — 411 tests, 2 existing MockStore skips.
• make lint — 0 issues.

Linear: https://linear.app/miren/issue/MIR-856/entity-store-clean-up-orphaned-index-entries
Amp thread: https://ampcode.com/threads/T-01a0cbe2-da05-7419-ac2f-9a6eeff692b2

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 28087cd3-dd99-4711-bc41-3e907cd8b7e6

📥 Commits

Reviewing files that changed from the base of the PR and between 26ec4bc and e8cfb37.

📒 Files selected for processing (6)

• pkg/entity/cleanup.go
• pkg/entity/cleanup_test.go
• pkg/entity/store.go
• pkg/entity/store_test.go
• servers/entityserver/entityserver.go
• servers/entityserver/entityserver_test.go

Included review availability: 5 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 6 reviews per hour.

---

📝 Walkthrough

Walkthrough

The change adds EtcdStore.CleanupOrphanedIndexEntry to remove matching index entries when the backing entity is absent and the entry revision is unchanged. Entity listing and page resolution invoke cleanup for missing entities. Missing primary keys now remain nil without a warning from getEntities, while decode failures continue to be logged. Tests cover guarded deletion, listing results, logs, and watch events.

Merge Risk: ⚪ Minimal · up to e8cfb

Orphaned entries can be cleaned during production listing without deleting entries for recreated entities. The change is mergeable after normal checks.

---

_{Comment @coderabbitai help to get the list of available commands.}

[phinze]

🤖 Thanks for digging into this one. The cleanup mechanics are careful: the CAS on CreateRevision == 0 and on each slot's ModRevision holds up. But MIR-856 predates the fix it's asking for, and this PR runs against a design we settled after it was filed.

MIR-1389 / #917 shipped controllers/indexgc in July. It's a bounded background sweep that CAS-deletes orphaned index entries, and it runs a minute after boot and every six hours after that. That ticket made an explicit call to keep foreground reads pure: "explicitly not an inline write-on-read." Read-path deletes tie read availability to write success, and they can set off a write storm right when the store is already under stress. The indexgc package doc says the same thing. This PR puts deletes on exactly the List skip site that decision ruled out, so I'd rather not take it, even with the per-call cap.

What's still missing after #917 is the noise, and two pieces here fix it. Could you carve the PR down to just these?

• The WatchIndex change. It fixes a bug #917 introduced. When indexgc deletes an orphan entry, every watcher gets a delete event where both the current read and the rev-1 read come back NotFound, and main logs that at ERROR. Suppressing it only when both reads are NotFound is the right fix. TestEntityServer_OrphanCleanupWatchDelete currently relies on List to do the delete, so it would need to delete the entry directly, the way the GC does.
• Quieting the WARN+ERROR pair. Keep the warnMissing removal, and lower entity in index but not in store to Debug in both List and resolve. That covers the window between sweeps. The GC owns the repair now, so neither line is something an operator acts on.

That means dropping CleanupOrphanedIndexEntry, cleanupOrphan, the per-list cap, and their tests. With that trimmed, this still closes MIR-856.

[evanphx]

In response to the changes-requested review: agreed that #917 owns repair. I removed write-on-read cleanup and its cap, kept missing-entity logs at Debug for both listing paths, and changed the watcher test to exercise the GC delete path. The new read test confirms listings leave the orphan for GC and emit no WARN/ERROR. The affected-package suite passed (411 tests, 2 existing skips), make lint reported 0 issues, and CI is green on the pushed revision. Please take another look when convenient.

— e + 🤖

[miren-code-agent]

🍪 biscuit: ✅ ready to merge — auto-review, non-blocking

This is ready to merge. The PR now does exactly the carve-down @phinze asked for, and the latest head doesn't change anything of substance.

Scope. The write-on-read cleanup is gone completely. CleanupOrphanedIndexEntry, cleanupOrphan, maxIndexCleanupsPerList and warnMissing don't appear anywhere in pkg/entity or servers/entityserver, and every getEntities caller uses the three-argument signature. Repair is left to controllers/indexgc, as #917 intended.

WatchIndex delete path (entityserver.go:594-601). It logs at ERROR unless both the current read and the rev-1 read are not-found. GetEntity and GetEntityAtRevision both return cond.NotFound for a missing key, so the log is skipped in exactly the GC-removed-orphan case. Every other outcome still reaches the log:

• Corruption errors, where the key exists but won't decode.
• Transport errors.
• Compaction errors on the historical read.

For a normal DeleteEntity, the rev-1 read still recovers the entity. For a session-lease index drop, the current read does.

Log levels. The missing-entity line is now Debug in both List and resolve. A corrupt entity still produces an ERROR from getEntities itself (failed to decode entity from etcd), even on the non-paged List path, where the undecodable map is discarded. So this PR doesn't hide anything an operator needs to act on. The only other GetEntities caller is deploymentattempts.migrateVersion, and it loses only the WARN it used to get for ids it read out of ListIndex. That's the same expected-orphan case.

Tests. TestEntityServer_OrphanCleanupWatchDelete makes its delete through CleanupStaleCollectionEntries, the call the GC makes. It resumes from listed.Header.Revision+1, so it can't race the delete. TestEntityServer_OrphanReadsStayQuietAndPure checks that the index key survives repeated List/ListPage calls and that nothing is logged at WARN or ERROR. It will catch anyone who reintroduces a write on the read path.

The one open item is @phinze's changes-requested review. As far as I can tell from the code, it's addressed, but it's his to clear before this merges.

---

🍪 full review note · reviewed at c0af393 · comment /biscuit review to run biscuit again.

[phinze]

🤖 Thanks for the quick turnaround. This is exactly the carve-down: repair stays in indexgc and reads stay pure. The watch test now goes through CleanupStaleCollectionEntries, so it covers the real path that exposed the ERROR. Ship it, and MIR-856 can close with it.