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:
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.
Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1268,
"verdict": "ready",
"event": "approve",
"summary": "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.\n\n**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.\n\n**`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:\n- Corruption errors, where the key exists but won't decode.\n- Transport errors.\n- Compaction errors on the historical read.\n\nFor a normal `DeleteEntity`, the `rev-1` read still recovers the entity. For a session-lease index drop, the current read does.\n\n**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.\n\n**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.\n\nThe 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.",
"posted_to_pr": true,
"head_sha": "c0af393af30a51b8e135fbd6bc7ab2edbb6b94be"
}I'd still merge this. The new commit cuts the PR down to exactly what @phinze asked for, and the smaller version is the better change. His point stands: controllers/indexgc (#917) already does the bounded background repair, and it was designed so that read requests never write. My last two reviews didn't know about that design and only asked for a cap on the write-on-read cleanup. Removing the cleanup entirely is the right answer. CleanupOrphanedIndexEntry, cleanupOrphan, the per-list cap and their tests are all gone, and nothing in pkg/ or servers/ refers to them now.
What's left:
WatchIndex delete handling logs at ERROR unless both the current read and the rev-1 read come back not-found. Both GetEntity and GetEntityAtRevision return cond.NotFound for a missing key, so isNotFound matches exactly the orphan case. Corruption and compaction errors still log. GC also removes mismatched entries, where the entity exists but no longer holds the indexed value. For those the current read succeeds, so the delete event carries the entity and nothing is logged. TestEntityServer_OrphanCleanupWatchDelete now makes the delete through CleanupStaleCollectionEntries, the same call indexgc/gc.go:154 makes, so it covers the real path that exposed the bug. It also resumes the watch from listed.Header.Revision+1, so the event replays even if the watch registers after the GC delete, and the test can't race.entity in index but not in store is now Debug in both List and resolve. Undecodable entities still log at ERROR, which is correct: a corrupt entity is something an operator needs to act on, while a missing one is GC's job. Dropping warnMissing also quiets deploymentattempts.migrateVersion. That's the only other GetEntities caller, and its ids also come from ListIndex, so it's the same expected-orphan case.TestEntityServer_OrphanReadsStayQuietAndPure covers the new contract. After two rounds of List and ListPage over an orphan, the index key still exists and the Debug line appears exactly 4 times. There's no WARN or ERROR, and Total drops to 1. If someone adds a write back into the read path, this test will fail.The minor point I raised earlier, about non-EtcdStore stores dropping orphans silently, no longer applies: that early return has been removed, and every store now logs the same Debug line.
@phinze's review is still marked as requesting changes. As far as I can tell the code now does everything he asked for, but he should take another look and clear it himself.
Verdict: ready
The caveat from my last review is fixed, and I'm comfortable with this merging.
What changed. Each List and ListPage call now cleans up at most maxIndexCleanupsPerList = 16 orphans. Any beyond that are skipped quietly, and later calls pick them up. So a read RPC now does a bounded amount of etcd work, however large the backlog. Concurrent listers can still repeat each other's work, but only up to 16 per call, and the CAS keeps that correct. Deleting entries in the middle of a paged walk can't skip anything, because the page cursor is key-based (resumeKey + "\x00"), not an offset.
Tests. TestEntityServer_ListCleanupIsBounded seeds cap+2 orphans and runs both paths, paged and unpaged. It checks that the first pass leaves exactly 2, the second clears them, and the "cleaned up" log appears exactly cap+2 times. That pins the behaviour I asked for.
The other new pieces also hold up:
cleanupOrphan now skips AttrSession and DBId indexes. That's right: listIds reads session indexes through ListSessionEntities, and DBId has its own path in ListIndexRevision. Neither lives under the collections/<col>/<id> layout that CleanupOrphanedIndexEntry builds keys for.WatchIndex, the error log for a delete event is now suppressed only when both the current read and the rev-1 read return not-found. That's exactly the orphan case, since the entity never existed at the prior revision. A compaction or transport error on the historical read still logs.TestEntityServer_OrphanCleanupWatchDelete checks that a resuming watcher gets the delete with EntityId set and no Entity, and that nothing is logged at ERROR.warnMissing from getEntities is a straight simplification. Every remaining caller passed false except GetEntities, and the server now decides what a miss means.The minor point from my previous review still applies: non-EtcdStore stores now drop orphans silently. That only affects test doubles, so it's not a blocker.
Posted to the PR's comment threads when you submit.
List and resolve (the ListPage path) each stop cleaning up after maxIndexCleanupsPerList (16) orphans, skip the rest without logging, and leave them for later calls. TestEntityServer_ListCleanupIsBounded checks the 16-then-2 drain on both paths. That bound is the one I was looking for.Verdict: ready
The delete logic looks correct to me. The cost of doing it inside the read path is the one thing I'd want you to accept on purpose before merging.
Why I trust the delete. CleanupOrphanedIndexEntry does both safety checks inside the delete transaction. It requires CreateRevision(entityKey) == 0, and each index slot must still have the ModRevision it had when it was read. So a list read at an older revision can't delete an entry for an entity recreated since then, and it can't delete a slot that now holds a different id. The tests cover both races.
The key layout matches addToCollectionOp / addToCollectionSessionOp: collections/<col>/<base58 id>[/<sess>]. Because base58 has no /, the key+"/" prefix scan only finds this id's session variants, and the key+"suffix" test proves a neighbouring slot survives.
Undecodable entities keep their entries because their key still exists. TestCleanupOrphanedIndexEntry_LeavesUnreadableEntity pins that, and it matters for the non-paged List path, where GetEntities drops the undecodable map, so a corrupt entity and a missing one both show up as nil.
On batching, 63 entries per txn gives 64 compares and 63 ops, under the 128 limit. The per-entry retry when a batch txn fails keeps one contended slot from blocking the rest.
The caveat: unbounded synchronous writes on List. List and ListPage are read RPCs that now issue two Gets and at least one Txn to etcd, one after another, for every orphan they meet.
ListPage is bounded by page size.List walks the whole index. On a cluster with many orphans (the situation MIR-856 exists for), the first List after deploy could make thousands of etcd round trips before replying.That's correct and self-healing, since progress persists and later calls get cheap. But it skips the rate limiting that CleanupStaleCollectionEntries deliberately does (cleanupDeleteBatchSize plus pauses). It also means a plain read now produces watch delete events. If you expect orphan counts to be small, this is fine. If not, consider capping how many orphans one List call cleans up, or handing them to a background worker.
A minor point: for non-EtcdStore stores, cleanupOrphan returns early and the old "entity in index but not in store" error is gone, so orphans there are now silent. That probably only affects test doubles.
List loop with the request's context and no cap. Each orphan costs two Gets plus a Txn, run one after another, and List walks the whole index. On a cluster with a large orphan backlog, the first few List calls could take a long time, and concurrent callers repeat the same work (CAS keeps it correct, but not cheap). Consider capping cleanups per call, e.g. the first N orphans, and just skipping the rest, since the next call picks them up. You could also queue them to something rate-limited like the existing sweep. If you've checked that real orphan counts are small, that's fine too, but please make it a deliberate choice.Verdict: caveats