I reviewed the updated state of this draft PR. All three concerns from my earlier review are resolved.
Session-scoped comment (cleanup.go:191): The comment now explicitly says "This only covers a live entity; a session-scoped entry whose entity is gone falls to the orphan case and is deleted like any other." That's exactly what I asked for — the intent is now on the page rather than carried by case ordering alone.
Stop() idempotency (gc.go:100): The author's pushback persuaded me. The two properties it relies on — that calling a context CancelFunc more than once is safe, and that reading a closed channel returns immediately — are both Go language guarantees, not accidents. And their counterargument was right: nilling the fields after the first stop would introduce a real race where a concurrent second Stop() could dereference a nil stopped. I'd rather have the current code, which holds for documented reasons, than the "tidy" version that introduces a subtle data race.
Undecodable entity fix (the CodeRabbit finding that landed in this push): This was a genuine correctness gap. When decoder.Unmarshal failed, the old path left nil in the result and resolveJustified read that as "entity is absent → orphan". Now getEntities returns a second undecodable map[Id]bool, decode failures log and populate that map, and resolveJustified routes those into unverifiable so their index entries are left alone. The test TestCleanup_LeavesUndecodableEntityAlone corrupts a live entity's payload in place and asserts the sweep leaves the index entry untouched; it would fail without the fix.
The core ReplaceEntity bug fix and the mismatch-detection machinery look solid. The code is well-structured, the comments are honest about what the invariants are, and the test coverage is meaningfully extended. Ready to graduate to human review.
Posted to the PR's comment threads when you submit.
CancelFunc again is safe and that reading a closed channel returns immediately — are documented Go language guarantees, not accidents of the current implementation. And the race you point out (a second Stop() concurrent with the first could see a nil stopped if we'd zeroed the field) is a real failure mode that would be worse than what we have. I'm closing this thread.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1166,
"verdict": "ready",
"event": "comment",
"summary": "I reviewed the updated state of this draft PR. All three concerns from my earlier review are resolved.\n\n**Session-scoped comment (cleanup.go:191):** The comment now explicitly says \"This only covers a live entity; a session-scoped entry whose entity is gone falls to the orphan case and is deleted like any other.\" That's exactly what I asked for — the intent is now on the page rather than carried by case ordering alone.\n\n**`Stop()` idempotency (gc.go:100):** The author's pushback persuaded me. The two properties it relies on — that calling a context `CancelFunc` more than once is safe, and that reading a closed channel returns immediately — are both Go language guarantees, not accidents. And their counterargument was right: nilling the fields after the first stop would introduce a real race where a concurrent second `Stop()` could dereference a nil `stopped`. I'd rather have the current code, which holds for documented reasons, than the \"tidy\" version that introduces a subtle data race.\n\n**Undecodable entity fix (the CodeRabbit finding that landed in this push):** This was a genuine correctness gap. When `decoder.Unmarshal` failed, the old path left `nil` in the result and `resolveJustified` read that as \"entity is absent → orphan\". Now `getEntities` returns a second `undecodable map[Id]bool`, decode failures log and populate that map, and `resolveJustified` routes those into `unverifiable` so their index entries are left alone. The test `TestCleanup_LeavesUndecodableEntityAlone` corrupts a live entity's payload in place and asserts the sweep leaves the index entry untouched; it would fail without the fix.\n\nThe core `ReplaceEntity` bug fix and the mismatch-detection machinery look solid. The code is well-structured, the comments are honest about what the invariants are, and the test coverage is meaningfully extended. Ready to graduate to human review.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6f7dY9",
"body": "The updated comment is exactly right — it names both cases explicitly rather than letting the ordering carry the meaning. Thank you for the quick turnaround on this.",
"resolve": true,
"resolve_reason": "addressed"
},
{
"thread_id": "PRRT_kwDONZ__ss6f7dZB",
"body": "You've persuaded me. Both properties the idempotence rests on — that calling a `CancelFunc` again is safe and that reading a closed channel returns immediately — are documented Go language guarantees, not accidents of the current implementation. And the race you point out (a second `Stop()` concurrent with the first could see a nil `stopped` if we'd zeroed the field) is a real failure mode that would be worse than what we have. I'm closing this thread.",
"resolve": true,
"resolve_reason": "persuaded"
}
],
"posted_to_pr": true,
"draft": true
}This is a draft PR, so I'm judging whether it's ready to graduate to human review. The scope is substantial and well-motivated: it fixes a real bug in ReplaceEntity that leaked index entries, extends the GC sweep to also detect and drain "mismatched" entries (entity exists but value changed), and adds defensive filtering to both saga storage implementations so stale index entries don't cause phantom recoveries or phantom retention deletes. The code is structured carefully and the test coverage is meaningfully extended.
That said, I have one concrete correctness concern, and one design question worth an answer before this graduates.
Correctness concern: Stop() is not safe to call more than once after Start().
The comment on Stop says it is "safe to call more than once," and TestGCController_StopIsSafeFromAnyState asserts this. But the implementation doesn't hold up. After the first Stop() call returns, c.cancel is non-nil and c.stopped is a closed channel. A second call will invoke c.cancel() again (fine for a context cancel func) and then block on <-c.stopped — but a closed channel reads immediately, so it actually works in the common case. However, there's a subtler issue: the test calls c.Stop() twice after a single Start(). The second call sees c.cancel != nil, calls cancel again, and reads from the already-closed c.stopped. That's indeed a no-panic, so the test passes — but the doc says "safe" and the implementation accidentally relies on closed-channel reads being immediate. This is fine as written, but the doc claim could lead a future reader to add state that breaks it. Not a blocker, but worth a note.
Actual correctness concern: the judge function mis-classifies session-scoped mismatches.
Look at the switch in judge (cleanup.go, around line 188–197):
switch {
case present && values[collection]:
return nil // justified — fine
case present && e.sessionScoped:
return nil // leased, etcd will GC it — fine
case present:
p.stats.MismatchedEntriesFound++ // entity exists, value changed
default:
p.stats.OrphanedEntriesFound++ // entity gone
}
A session-scoped entry for an entity that exists but no longer carries the indexed value falls into case present && e.sessionScoped and is silently left alone. That's safe (etcd will expire it), but sessionScopedKey is also checked before the orphan case, so a session-scoped entry for an entity that is gone (!present) falls through to default and is counted as an orphan — and then queued for deletion. The comment says session-scoped entries are "leased, so etcd collects it with the session," but that protection only fires when the entity is present. When the entity is absent, the deletion fires regardless. This is probably the intended behaviour for orphaned session-scoped entries (the session is presumably gone too), but the comment and the code are in tension in a way that could confuse a future maintainer or mask a real issue. The comment inside case present && e.sessionScoped should clarify that this only applies when the entity is present.
Design question: resolveJustified issues one getEntities call per batch page, but the batch size (200) and the etcd transaction op limit (128 ops / 2 = 64 entities per txn) mean each resolve triggers up to 4 internal etcd round trips. The comment on cleanupResolveBatchSize says it "stays modest because every resolved entity arrives with its full payload." That's right, but the interaction with maxEntitiesPerBatch (64) means a 200-entry resolve page is always 4 txns minimum. This is probably fine for a background sweep — just worth confirming the math is intentional.
Neither of these is a merge blocker individually, but the session-scoped orphan deletion behaviour is underdocumented enough that I'd want it clarified before this lands. The ReplaceEntity fix itself (diffing originalEntity.attrs instead of repl.attrs) is correct and the conformance test covers it well.
present is true here). A session-scoped entry whose entity is absent (!present, the default arm) still falls through to deletion, which is probably correct (the session is likely gone too), but the comment doesn't acknowledge that case. The case ordering means session-scoped orphans are deleted, not skipped. Please add a brief note here clarifying that this guard is specifically for the live-entity case, and that absent-entity session entries are intentionally handled by the default arm.Stop is "safe to call more than once," and the test covers it. After a started+stopped controller, c.cancel is non-nil and c.stopped is a closed channel, so a second call will re-invoke cancel() and then read from the closed channel — both are fine, but only accidentally so. If c.stopped ever becomes a buffered channel or gets reset on restart, this breaks silently. Consider setting c.cancel = nil and c.stopped = nil after the first successful stop, or documenting explicitly that the idempotence relies on closed-channel semantics.Verdict: not_ready