State key NID mappings can be missing

[kegsay]

`error="found 33449 users but only have state key nids for 33447 of them"``

Caused by roomserver code in storage: JoinedUsersSetInRooms:

func (d *Database) JoinedUsersSetInRooms(ctx context.Context, roomIDs []string) (map[string]int, error) {
	roomNIDs, err := d.RoomsTable.BulkSelectRoomNIDs(ctx, roomIDs)
	if err != nil {
		return nil, err
	}
	userNIDToCount, err := d.MembershipTable.SelectJoinedUsersSetForRooms(ctx, roomNIDs)
	if err != nil {
		return nil, err
	}
	stateKeyNIDs := make([]types.EventStateKeyNID, len(userNIDToCount))
	i := 0
	for nid := range userNIDToCount {
		stateKeyNIDs[i] = nid
		i++
	}
	nidToUserID, err := d.EventStateKeysTable.BulkSelectEventStateKey(ctx, stateKeyNIDs)
	if err != nil {
		return nil, err
	}
	if len(nidToUserID) != len(userNIDToCount) {
		return nil, fmt.Errorf("found %d users but only have state key nids for %d of them", len(userNIDToCount), len(nidToUserID))
	}

It shouldn't be possible to have NIDs in the membership table but not in the state keys table, but apparently it is.

[kegsay]

So the place where InsertMembership is called is in membershipUpdaterTxn which is called just after targetUserNID, err = d.assignStateKeyNID(ctx, txn, targetUserID) when NewMembershipUpdater is made. However, assignStateKeyNID hits a cache first:

func (d *Database) assignStateKeyNID(
	ctx context.Context, txn *sql.Tx, eventStateKey string,
) (types.EventStateKeyNID, error) {
	if eventStateKeyNID, ok := d.Cache.GetRoomServerStateKeyNID(eventStateKey); ok {
		return eventStateKeyNID, nil
	}
	// Check if we already have a numeric ID in the database.
	eventStateKeyNID, err := d.EventStateKeysTable.SelectEventStateKeyNID(ctx, txn, eventStateKey)
	if err == sql.ErrNoRows {
		// We don't have a numeric ID so insert one into the database.
		eventStateKeyNID, err = d.EventStateKeysTable.InsertEventStateKeyNID(ctx, txn, eventStateKey)
		if err == sql.ErrNoRows {
			// We raced with another insert so run the select again.
			eventStateKeyNID, err = d.EventStateKeysTable.SelectEventStateKeyNID(ctx, txn, eventStateKey)
		}
	}
	if err == nil {
		d.Cache.StoreRoomServerStateKeyNID(eventStateKey, eventStateKeyNID)
	}
	return eventStateKeyNID, err
}

If it was possible for it to be in the cache but not the database, this would cause this error. Given this is a rare condition to hit, I wouldn't be surprised if the race logic here doesn't work as intended. If that were the case then the flow could be:

• Insert state key, get back NID 7.
• Insert state key, get back NID 8.
• Cache state key -> 7.
• Cache state key -> 8.
• The control flow for 8 bails out (possibly due to conflicts from 7 or some other reason)
• The control flow for 7 continues to success.
• Subsequent calls for this state key will return a state key 8 which isn't in the database.