Include entity data in delete watch events

[evanphx]

Summary

• When a watched entity is deleted, watchers now receive the full entity data from before deletion, eliminating the need for watchers to cache entities on their side
• WatchEntity uses etcd's WithPrevKV() to get the previous entity value directly from the watch event — no extra round-trip
• WatchIndex fetches the entity via GetEntity (entity still exists since index entries are deleted first), with a fallback to the new GetEntityAtRevision method for robustness
• Adds GetEntityAtRevision to the Store interface for revision-based reads from etcd

Test plan

• Store-level TestWatchEntity_DeleteIncludesEntity against real etcd
• Server-level TestEntityServer_WatchEntity_DeleteIncludesEntity against mock
• Server-level TestEntityServer_WatchIndex_DeleteIncludesEntity against mock
• Server-level TestEntityServer_WatchEntity_DeleteIncludesEntity_Etcd against real etcd
• Server-level TestEntityServer_WatchIndex_DeleteIncludesEntity_Etcd against real etcd
• Full pkg/entity test suite (229 tests passing)
• Full servers/entityserver test suite (23 tests passing)

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds revision-aware reads and preserves pre-deletion state for watches. MockStore gains a deletedEntities map, GetEntityAtRevision and WaitForEntityWatcher. The Store interface and EtcdStore implement GetEntityAtRevision. Etcd watch requests PrevKV and delete events emit the previous entity when available. Server watch logic fetches the full entity for delete events (attempting GetEntity, then GetEntityAtRevision). Controller filtering no longer skips delete events with recent revisions. Tests added to assert delete events include the full entity payload.

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

pkg/entity/mock.go (1)

59-69: Mock simplification is acceptable but worth documenting.

GetEntityAtRevision ignores the rev parameter and simply returns the entity from either current or deleted maps. This is a reasonable mock simplification since:

1. MockStore doesn't track actual etcd revisions
2. Tests primarily care about whether the entity can be retrieved, not revision semantics

Consider adding a brief comment noting this simplification for future maintainers.

📝 Suggested documentation

 func (m *MockStore) GetEntityAtRevision(ctx context.Context, id Id, rev int64) (*Entity, error) {
+	// Note: Mock ignores rev parameter - returns entity if it exists in current or deleted maps
 	m.mu.RLock()
 	defer m.mu.RUnlock()

servers/entityserver/entityserver_test.go (1)

469-471: Consider using a more robust synchronization mechanism for etcd watch establishment.

The time.Sleep(100 * time.Millisecond) is a common pattern but can be flaky under load. While etcd watches are typically established quickly, this could cause intermittent test failures in CI environments.

Consider implementing a similar WaitFor* helper for EtcdStore or using a slightly longer sleep with a comment explaining why.

📝 Alternative: Add comment explaining the timing

 	// Give the watch time to establish
-	time.Sleep(100 * time.Millisecond)
+	// Note: etcd watch establishment is typically fast (<10ms), but we use 100ms
+	// to account for CI environment variability. Unlike MockStore, EtcdStore
+	// doesn't expose internal watcher tracking for synchronization.
+	time.Sleep(100 * time.Millisecond)

servers/entityserver/entityserver.go (1)

486-509: Robust fallback strategy for fetching entity on delete events.

The two-stage fetch approach is well-designed:

1. Try GetEntity first since index entries are deleted before the entity key
2. Fall back to GetEntityAtRevision if the entity was already deleted

One minor observation: the error from the first GetEntity call is silently discarded. While this is intentional (it's expected to fail sometimes), consider logging at debug level to aid troubleshooting.

📝 Optional: Add debug logging for first fetch failure

 				// Try to fetch the entity data for the delete event.
 				// Index entries are deleted before the entity key, so the entity
 				// may still exist. Fall back to a revision-based read if not.
 				en, err := e.Store.GetEntity(ctx, entityId)
 				if err != nil {
+					e.Log.Debug("entity not found at current revision, trying historical read", "id", entityId, "error", err)
 					en, err = e.Store.GetEntityAtRevision(ctx, entityId, event.Kv.ModRevision)
 				}

---

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

[coderabbitai]

Actionable comments posted: 1