userlog/activitylog: cross-instance lost updates on event-list writes (fix direction?)
- Dominant language
- Go
- Stars
- 2.1k
- Forks
- 274
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 103
Description
## Summary
`userlog` and `activitylog` both keep a per-entity list in the persistent store and update it with a non-atomic **read-list → append → write-list**. With more than one service instance this races and silently loses notifications/activities. PRs #12415 (userlog) and #12417 (activitylog) added a local `sync.Mutex`, but as @kobergj correctly pointed out that only serializes a single process and does nothing across instances. I'd like to agree a direction before reworking, since the real fix lives one layer below these two services.
## Root cause
1. **Events are group-load-balanced.** Both services call `events.Consume(stream, "", …)`, so with N instances a given user's/resource's events are spread across different instances.
2. **The write is a non-atomic read-modify-write**, and the go-micro `store.Store` interface exposes only `Read`/`Write`/`Delete` — `WriteOptions` carries no revision / if-match / CAS. So two instances can each `Read` the list, append, and `Write`, and the later write wins (lost update). A per-process mutex cannot prevent this.
For #12417 specifically, the debounce makes it slightly worse: buffering widens the read-modify-write window, so the cross-instance lost-update probability *increases* versus writing immediately.
## Options
**A. Partition the event stream by entity key** (userid / resourceID) so each entity's events always land on one instance (subject partitioning + per-partition durable consumers). Then a per-process lock is sufficient and reads stay a single fast `Read`.
- *Pro:* architecturally correct for an event-sourced system; benefits every event consumer; no store change.
- *Con:* touches the shared `reva` events layer (publisher + consumer wiring).
**B. Add optimistic concurrency (CAS) to the store.** nats-js-kv's underlying `nats.KeyValue` already supports revisioned `Update(key,val,rev)` + `Get().Revision()`; the go-micro plugin just hides it. Expose a conditional-store interface (memory: in-proc version map; redis: `WATCH`/Lua; noop: passthrough) and do a `read-rev → modify → write-if-rev → retry` loop.
- *Pro:* most reusable — fixes read-modify-write for any oCIS service; keeps single-key reads.
- *Con:* store-layer change (ideally upstreamed to go-micro-plugins, or oCIS opens its own nats-KV handle for these writes).
**C. Append-only, one key per event** (`/`), read with `ReadPrefix()`.
- *Pro:* self-contained in the two services, no upstream change, removes contention by construction.
- *Con:* nats-js-kv's prefix read does `bucket.Keys()` over the whole bucket then filters, so reads scale with total keys, not per-entity — fine for bounded notification lists, risky for high-volume activitylog.
## Recommendation
I lean toward **A** as the correct long-term fix and **B** as the highest-leverage one (it removes this whole class of bug across oCIS). I'd keep the per-process mutex out of the final solution rather than ship a single-binary-only guard.
## Decision needed
@kobergj @DeepDiver1975 — which direction do you prefer? I'm happy to implement A, B, or C (or split: C for `userlog`, A/B for `activitylog`). I'll rebase #12415/#12417 onto whatever we settle on.
Contributor guide
Research direction
Read the event consumers in userlog and activitylog alongside PRs #12415 and #12417; then inspect the shared events and store interfaces described in the issue. Done means the maintainers choose A, B, or C and the selected approach demonstrates that concurrent writes no longer lose events.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, redis
- Domain
- backend, distributed-systems
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100