CollectionItemBroker Cleanup + Fixes

[TalZaccai]

Addressing some comments / issues with the existing CollectionItemBroker implementation:

• When are keys removed from keysToObservers?
  • Added logic for removing keys from keysToObservers when the observer queue is empty
  • Added keysToObservers timed cleaning logic in the main loop context (in case the observers died and we never checked that key's queue again)
• HandleBrokerEvent is sync; if there are a large number of blocking sessions, can this be a scalaibility bottleneck?
  • This is by design, as the blocking commands are operating in a first-come-first-serve manner. We can change the logic here but that might be confusing as a different behavior might not be expected by the user.
• Use named (e.g. NotStarted, Started, Disposed) constants for mainLoopTaskStatus. Badrish's checkin added a "2" value for Dispose() so this will improve readability.
  • Done.
• Dispose() loops on Interlocked.CompareExchange but is not testing anything about the value before doing the CompareExchange, so a single Interlocked.Exchange should work
  • Done.
• Verify that all IDisposable members are disposed
  • Done.
• Switch upgradable locks with slim locks
  • Done.
• Switch main queue to use a struct to hold events
  • Done.

Also addressed in this PR -

• Handling WRONGTYPE errors in blocking commands as expected by the protocol (this is in place of PR Blocking functions: Check key type first, and expire empty keys. #1130 which first mentioned the issue)
• Adding tests to check proper WRONGTYPE handling in blocking commands
• Deleting empty collection keys after blocking operations + adding tests
• Handling "special" CollectionItemResult instances in previously missed commands
• Need to have all blocking commands covered in "forced unblock" tests (ClientUnblockBasicTest, MultipleClientsUnblockAndAddTest in RespTests) - will do this in a separate PR

[prvyk]

Another thing I noticed in #1130 was that lists/sets should be deleted when empty -

407a80c

lpush a 1
blpop a 0
exists a
(is 0 on redis, is 1 here. lpop is 0 on both redis and Garnet)

[TalZaccai]

Another thing I noticed in #1130 was that lists/sets should be deleted when empty -

407a80c

lpush a 1 blpop a 0 exists a (is 0 on redis, is 1 here. lpop is 0 on both redis and Garnet)

Ah good to know! Thanks for pointing it out :)

[Copilot]

Pull Request Overview

This PR cleans up the CollectionItemBroker implementation by addressing key removal, error handling (including force unblocked and type mismatches), and improving lock usage and naming for main loop statuses. Key changes include:

• Adding force-unblocked and type mismatch error handling in blocking pop/move commands for SortedSet and List operations.
• Replacing ReaderWriterLockSlim with SingleWriterMultiReaderLock in observers and refining main loop and cleanup logic.
• Refactoring broker event handling to use a union-like struct with explicit field offsets.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
libs/server/Resp/Objects/SortedSetCommands.cs	Added handling for force-unblocked and type mismatch errors in sorted set blocking pop commands.
libs/server/Resp/Objects/ListCommands.cs	Introduced type mismatch error handling in blocking list operations.
libs/server/Objects/ItemBroker/CollectionItemResult.cs	Updated constructor and static instances to support both force-unblocked and type mismatch flags.
libs/server/Objects/ItemBroker/CollectionItemObserver.cs	Switched to SingleWriterMultiReaderLock and added a flag parameter for improved lock management.
libs/server/Objects/ItemBroker/CollectionItemBrokerEvent.cs	Reworked event handling to use a union-like struct with explicit layout and new event types.
libs/server/Objects/ItemBroker/CollectionItemBroker.cs	Refactored main loop status management, observer cleanup, and updated collection item retrieval.

Comments suppressed due to low confidence (1)

libs/server/Objects/ItemBroker/CollectionItemBroker.cs:590

• In the TryGetResult method's ListObject branch, the inner switch cases end with a break without returning a value, which may result in the method not returning the intended result. Consider adding an explicit return (e.g., 'return isSuccessful;') after processing the ListObject commands.

case ListObject listObj: