Detail Bug Report for mirendev/runtime

View in Detail

Introduced in #232 by @evanphx on Oct 13, 2025

Summary

• Context: EntityStorage is the durable, etcd-backed implementation of the saga Storage interface; its Save writes each execution as an entity and ListIncomplete/ListTerminal discover executions by querying the entity store's per-status index. The Executor first persists every saga as pending, then Saves it again on every status transition (running/undoing/completed/failed), and on restart Recover resumes everything ListIncomplete returns.
• Bug: On every Save after the first, the prior status index key is never removed, so a saga's ID leaks into every status index it ever passed through; and EntityStorage.ListIncomplete trusts that index without re-verifying the decoded Status (unlike MemoryStorage, which filters by actual status). The two combine so terminal sagas are returned by ListIncomplete.
• Actual vs. expected: A completed/failed saga is reported by ListIncomplete after a normal pending→running→completed (or →failed) lifecycle; the contract (asserted by the conformance suite) is that "a completed saga must NOT appear in ListIncomplete".
• Impact: (1) On each restart Recover re-claims and re-decodes every terminal saga; failed sagas contribute a saga failed: … error per leaked entry, so Recover's recovery completed with N errors grows with the historical failure count. (2) RunRetention uses ListIncomplete to determine which terminal children are still shielded by a live parent; a terminal parent leaking into ListIncomplete makes its expired terminal children Skipped forever, so they never get deleted (a retention stall). (3) The leaked pending/running/undoing collection keys are never collected — neither DeleteEntity, nor the background CleanupStaleCollectionEntries sweeper, nor Reindex removes entity-present-but-stale-attribute collection keys — so every saga that ever ran permanently leaves orphan status-index keys in etcd.

Code with Bug

pkg/entity/store.go — ReplaceEntity computes “original” indexed attributes from the replacement

// Keep track of original indexed attributes for removal
originalIndexedAttrs, err := s.collectIndexedAttributes(ctx, repl.attrs) // <-- BUG 🔴 reads replacement attrs, so old status index keys are never removed

pkg/saga/storage.go — ListIncomplete trusts the index and appends without re-checking status

exec, err := entityToExecution(sagaEntity)
if err != nil {
	s.log.Warn("failed to convert saga entity, skipping", "id", ent.Id(), "error", err)
	continue
}
executions = append(executions, exec) // <-- BUG 🔴 terminal sagas leaked into the non-terminal index are returned as "incomplete"

Explanation

• In etcd, the per-status index is maintained as separate collection keys. When a saga transitions status, EntityStorage.Save updates the entity via ReplaceEntity.
• ReplaceEntity mistakenly collects the “original” indexed attributes from repl (the replacement entity), so buildCollectionOps compares new attrs against themselves and emits no delete operations for the prior status’s collection key.
• As a result, a saga ID remains in every status collection it has ever had (e.g., pending and running even after it becomes completed).
• EntityStorage.ListIncomplete then uses those index results without filtering by decoded exec.Status, so terminal executions can be returned as incomplete.

Codebase Inconsistency

UpdateEntity/PatchEntity source indexed attributes from the existing entity (the intended behavior), unlike ReplaceEntity:

originalIndexedAttrs, err := s.collectIndexedAttributes(ctx, entity.attrs)

Recommended Fix

• Fix root cause in pkg/entity/store.go ReplaceEntity: source “original indexed attributes” from the fetched original entity being replaced (matching UpdateEntity/PatchEntity), so old status index keys are removed.
• Defense-in-depth in pkg/saga/storage.go: have ListIncomplete filter out decoded StatusCompleted/StatusFailed (mirroring MemoryStorage). Consider adding the symmetric guard in ListTerminal (drop entries whose decoded status is not terminal).
• Add a test against the real EtcdStore that ReplaceEntitys an entity to a different indexed attribute value and asserts ListIndex no longer returns the old value.

History

This bug was introduced in commit 783455ce. The change refactored the entity system to move ID/Revision/timestamps from struct fields to attributes and split ReplaceEntity to build a separate repl entity from the replacement attributes (via NewEntity(attributes)) alongside the fetched original entity. In doing so it flipped the "original indexed attributes" source from entity.Attrs (the existing entity, correct) to repl.Attrs (the replacement), so buildCollectionOps now compares the new attrs against themselves and never emits a delete for the prior status's collection key. The original 2c7324ebe (RFD 0017 CRUD) implementation was correct (collectIndexedAttributes(ctx, entity.Attrs) before entity.Attrs = attributes), and the later a4e3effc8 encapsulation refactor only mechanically renamed repl.Attrs → repl.attrs, carrying the bug forward without altering the logic.

Production confirmation: Selkie, September 6, 2026

This bug amplified a startup failure while upgrading a 2-core, 4 GB host from v0.11.1 to v0.14.0. Sandbox recovery loaded a large backlog of unrelated PostgreSQL teardown sagas. During a guarded retry, Miren RSS reached about 2.14 GB despite GOMEMLIMIT=768MiB; a host memory guard stopped it. No OOM kill was observed.

The pre-upgrade snapshot contains 94,837 teardown executions for one addon association: 74,873 failed and 19,964 pending, plus three other incomplete sagas. A later live pending index query reported 94,842 matches and returned entities whose actual status was failed.

We independently reproduced the writer bug against real etcd on main 34beeec5d054. Create an entity with indexed status pending, replace it with failed, then check the stored status and both indexes. The stored status and new index are correct, but the old index still returns the entity. Changing only repl.attrs to originalEntity.attrs makes the test pass. The defect is also present in v0.11.1. This was a local experiment; the correction has not been deployed.

Include repair of existing wrong-value index entries in the fix, with a regression test covering an entity that still exists. MIR-1389's missing-entity cleanup does not cover this case. Repair must be bounded, resumable, and safe against concurrent entity updates. Keep the decoded-status guards and real-etcd replacement regression already proposed above.

The companion recovery/retention issue covers bounded reads. Correcting indexes alone still leaves 19,967 actually incomplete executions in this snapshot.