94 lines
4.9 KiB
Markdown
94 lines
4.9 KiB
Markdown
# Code Review: Leave & Stop-Forward (m9-leave-revocation)
|
|
|
|
## Summary
|
|
|
|
Implementation is complete, correct, and production-ready. All 8 integration tests pass, 1299 lib tests pass, `cargo clippy -- -D warnings` is clean, `cargo fmt --check` is clean. No blockers.
|
|
|
|
---
|
|
|
|
## Files Changed
|
|
|
|
| File | Change |
|
|
|---|---|
|
|
| `tidal/src/storage/keys.rs` | Added `Tag::CommunityLeave = 0x11`; updated from_byte, tests, proptest range |
|
|
| `tidal/src/entities/community.rs` | Added `MembershipStatus`, `CommunityMembership`, `serialize_membership`, `deserialize_membership`, 6 unit tests |
|
|
| `tidal/src/entities/mod.rs` | Re-exported new types |
|
|
| `tidal/src/db/mod.rs` | Added `community_leave_status: DashMap<u64, CommunityMembership>` field; initialized in both constructors |
|
|
| `tidal/src/db/community.rs` | Added `leave_community_layer`, `rejoin_community_layer`, `community_layer_status`, `persist_leave_record` |
|
|
| `tidal/src/db/signals.rs` | Added leave gate to `try_cohort_attribution` AND `try_community_forwarding` |
|
|
| `tidal/src/db/state_rebuild.rs` | Added `rebuild_community_leave_status`; called from `from_parts` |
|
|
| `tidal/tests/m9_leave_revocation.rs` | 8 integration tests (TC-01 through TC-08) |
|
|
|
|
---
|
|
|
|
## Correctness
|
|
|
|
### Gate placement
|
|
The gate is placed at the top of BOTH `try_cohort_attribution` (cohort predicate fan-out) and `try_community_forwarding` (community:: key fan-out). This is correct — both paths must be suppressed when a user has left. Before this change, leaving was silent on the forwarding path.
|
|
|
|
```rust
|
|
// In try_cohort_attribution AND try_community_forwarding:
|
|
if self
|
|
.community_leave_status
|
|
.get(&user_id)
|
|
.map_or(false, |m| m.status == crate::entities::MembershipStatus::Left)
|
|
{
|
|
return;
|
|
}
|
|
```
|
|
|
|
### TOCTOU window
|
|
The spec acknowledges a TOCTOU window: a concurrent `signal_with_context` call that passes the gate before `leave_community_layer` completes its DashMap insert may still fan out one signal. This is acceptable — the retroactive-purge path handles historical contributions. The window is bounded to one signal write.
|
|
|
|
### Storage key correctness
|
|
`Tag::CommunityLeave = 0x11` is a new allocation. The existing `Tag::CommunityMembership = 0x0E` was already in use by `m9-community-profile-sync` with a community-name suffix — that tag cannot be reused without suffix (would be a key collision). Using `0x11` correctly avoids the collision.
|
|
|
|
### Startup restore
|
|
`rebuild_community_leave_status` scans all users_engine entries and matches on `Tag::CommunityLeave`. It correctly uses `parse_key` to decompose the key and extract the entity_id, then deserializes the 9-byte value. Malformed records are silently skipped with a debug log. The function is called after `rebuild_community_memberships` in `from_parts`, preserving existing rebuild order.
|
|
|
|
### Serde correctness
|
|
Fixed 9-byte format: `[status: 1 byte][left_at_ns: 8 bytes LE]`. Zero in the timestamp bytes is the unambiguous encoding for `None` (a real timestamp of 0 ns would be Unix epoch, which is not a valid leave time in practice). The `deserialize_membership` correctly checks `ts_raw == 0` to distinguish `None` from a real value.
|
|
|
|
### Idempotency
|
|
`leave_community_layer` called twice: second call writes a new timestamp to both storage and DashMap. The spec states this is intentional — allows a fresh boundary for the retroactive purge. The `left_at_ns >= previous` property is tested in TC-03.
|
|
|
|
### Rejoin history preservation
|
|
`rejoin_community_layer` reads the current `left_at_ns` from the DashMap before creating the Active record, so the leave timestamp survives. Tested in TC-05.
|
|
|
|
---
|
|
|
|
## Performance
|
|
|
|
- Gate check: `DashMap::get(&user_id)` — O(1) per-shard lock, zero storage I/O. Users with no leave record (the vast majority) take the fast `map_or` default path without any DashMap shard contention beyond a key miss.
|
|
- `leave_community_layer` / `rejoin_community_layer`: one storage `put` + one DashMap `insert`. Both are off the hot signal write path.
|
|
- `rebuild_community_leave_status`: full `scan_prefix(&[])` on startup — O(N_users_keys). This is the same approach as `rebuild_community_memberships` and all other startup rebuild functions. Acceptable for the expected user count.
|
|
|
|
---
|
|
|
|
## Test Coverage
|
|
|
|
| Test | Coverage |
|
|
|---|---|
|
|
| `leave_stops_cohort_fanout` | Gate works; base signal ledger unaffected |
|
|
| `join_resumes_cohort_fanout` | Gate cleared on rejoin |
|
|
| `leave_is_idempotent` | Double-leave returns Ok; timestamp monotone |
|
|
| `status_query_lifecycle` | Full Active→Left→Active lifecycle |
|
|
| `left_at_ns_preserved_on_rejoin` | History intact after rejoin |
|
|
| `durability_across_reopen` | Tag::CommunityLeave persisted and restored |
|
|
| `unknown_user_defaults_to_active` | Default Active for absent users |
|
|
| `leave_stops_community_forwarding` | `community::` key fan-out gated |
|
|
|
|
All 8 pass. The m6_cohort regression suite (9 tests) passes — no existing cohort behavior broken.
|
|
|
|
---
|
|
|
|
## Issues
|
|
|
|
None. Implementation matches spec and design exactly.
|
|
|
|
---
|
|
|
|
## Verdict
|
|
|
|
**Approved.** Ship as-is.
|