Sample/backfill integration

[edge-marge]

No description provided.

[orcist]

Great work on the first pass, please don't be alarmed by the amount of my comments. 🙏 They are mostly related to hardening the example and reducing surface for human mistake during customization of handlers, plus some comments regarding special edge cases and some nitpicking on codestyle.

Requested modifications and additions for Server Agent:

• I'd like Server Agent to be responsible for counting how many backfills should be created (according to target team size), when to re-create them, and most of the termination logic - handler only calls one method in the Agent to stop backfilling. At this moment it partially lives in the server handler, which is expected to be customized by the developer. This would encapsulate the hard requirements for backfills more safely and reduce surface for human mistakes.
• Could we add a callback here for PlayerConnected(string ticketID): InjectedTicketDTO? Handler would be responsible to expose this method to the netcode-specific caller who receives the ticket ID from the player upon connection. Server Agent can then ensure the backfill is not re-created (since it was filled) and return back the ticket (with team and group assignment) of this new player to the handler, so in future examples we are prepared for multi-team forward compatibility.
  • We should also verify if the connection is amongst the already present assignments, since it could be a player reconnecting to the server. The return of this method should indicate whether its a new player or a reconnection. If the player is neither, it's likely someone rejoining after the allowed rejoin period, or a complete random internet stranger, and should be most likely kicked/banned/rejected through netcode-specific methods (server handler sample should just leave a todo note).
  • If there is a ticket assigned (server agent found it while polling a backfill) but the player does not connect and the PlayerConnected method is not invoked within a configurable amount of time (ConnectionGracePeriodSeconds in the server agent constructor), the assignment should be expired and the backfill recreated with up to date assignments. There's a few suggestions in the code for how to implement parts of this.
• Please modify backfill polling logic so that there's a single coroutine iterating over all active backfills instead of 1 coroutine per backfill, this reduces amount of threads and optimizes usage for typical server environment (usually less than 4 logical cores, typically around 1 core). Having too many threads on system with fewer cores can bottleneck CPU and cause lag because the server spends more time switching between threads rather than executing them. Not an issue with one-off requests, but there's a good chance some games will have many active backfills concurrently and keep polling for a long time.
• Please modify the backfill polling logic so that in the eventuality of errors, we don't drop the backfill right away, but instead continue polling and attempt re-creating the backfill when the ticket expiration period is reached (you will need to define a new agent constructor parameter for this ExpirationPeriodSeconds). The unified loop for re-creating expired ticket described in a previous point will handle re-creating the backfill. The ultimate safety net will be the AdmissionPeriod described in another server handler point below, since we stop all backfills at this point.

Requesting one addition to Server Handler:

• Please add a new optional server handler parameter AdmissionGracePeriodSeconds to configure a time period after which all backfills are deleted, keeping the current assignments so the handler can be extended with custom code for developer to decide if new connections are still accepted (just a note in the handler). Explainer - this is a fairly common design, match stops filling after some time and either aborts deployment (kicks players back to restart matchmaking), or proceeds with partial teams/bots. Defaults to 120 seconds, can be set to -1 to disable the logic, or completely removed by developer since it lives in the handler script example.

One more request for changes, moving logic from client handler to server handler:

• We can't rely on clients to send their ticket ID when disconnecting or abandoning, the operating system may not give enough time for the client to send the request, and in the eventuality of a client crash the disconnection hook will not be called at all. Instead, let's expose a new method PlayerDisconnected(string ticketID) in the server handler which would be invoked by the server netcode when it detects that a client disconnected or simply stopped sending traffic.

[edge-marge]

when you say to "modify the backfill polling logic so that in the eventuality of errors, we continue polling and attempt re-creating the backfill when the ticket expiration period is reached," do you want it to replace the whole logic with the MaxConsecutivePollingErrors parameter, or to have both at the same time?

[orcist]

Lots of great improvements - thank you, we're almost there! 🙏

Regarding the comment on polling logic changes, my intention was not to delete the backfill immediately if we can't fetch it (short term network issue), even including the MaxConsecutivePollingErrors. The idea was that the server assumes the matchmaker can be reachable from client, and a client may be assigned and attempt direct connection to the server during this period.

I want to walk back this design choice, because even if this were implemented - the client who connects and triggers the verification will not be possible to verify without server fetching the updated backfill and the assigned ticket. I think it's best that we treat a backfill like this as abandoned (your original implementation) - attempt a backfill delete request with a few retries from the server, and if that fails simply expire the backfill reference on server and let the backfilling logic attempt to create more backfills to replace the player.

We should still respect the max consecutive retries when trying to read the backfill, since the server could succeed if the retries are spread out a bit (using the standard retry logic).

Let me know if you'd like to clarify over a videochat or in person, this is an opinionated part of the design since there is no clear best option with unreliable network connectivity during incidents... (best effort mode)

[orcist]

Suggested change

	public void AbandonPlayer(string ticketID)
	{
	if (Assignments.Remove(ticketID))
	{
	L.Log($"MM | Backfill - ticket removed [{ticketID}]");
	StartNewBackfill();
	}
	else
	{
	L.Warn($"MM | Backfill - ticket abandon failed [{ticketID}]");
	}
	}
	public bool AbandonPlayer(string ticketID)
	{
	if (Assignments.Remove(ticketID))
	{
	L.Log($"MM | Backfill - ticket removed [{ticketID}]");
	AddBackfills();
	return true;
	}
	L.Warn($"MM | Backfill - ticket abandon failed [{ticketID}]");
	return false;
	}

Two changes:

• Let's pass back the boolean if the developer wants to handle succes/error on abandonment differently.
• I've switched from the internal method to AddBackfills(). This adds a safeguard, ensuring that an outside caller can't bypass the check against the team size limit. (for example, invoking this method from two threads at the exact same time could result in a race condition and add two backfills instead of one, or someone could implement a custom handler manipulating the team size to set it to -1 to stop backfills but your implementation would not check against it when abandoning after the admission period elapsed).

[orcist]

Suggested change

	public DateTime? JoinedAt;
	public DateTime? ConnectedAt;

I like this pattern, just a tweak to make the naming consistent with the Server Agent method Player Connected.

[orcist]

Suggested change

	public BackfillAssignedTicket<A> PlayerConnected(string ticketID)
	{
	if (Assignments.ContainsKey(ticketID))
	{
	if (Assignments[ticketID].JoinedAt is null)
	{
	Assignments[ticketID].JoinedAt = DateTime.Now;
	}
	return Assignments[ticketID];
	}
	else
	{
	return null;
	}
	}
	public BackfillAssignedTicket<A> PlayerConnected(string ticketID)
	{
	if (Assignments.ContainsKey(ticketID))
	{
	if (Assignments[ticketID].JoinedAt is null)
	{
	Assignments[ticketID].JoinedAt = DateTime.Now;
	}
	return Assignments[ticketID];
	}
	return null;
	}

The else statement is not neccessary, since the function returns at the end of your if branch. This is a safer approach in case you added another else-if statement in the future, and guarrantees that if no condition above is met you always return an explicit null.

[orcist]

After the other changes in this PR, this function is now invoked in one place - StartNewBackfill. You can put all of this code directly inside of that StartNewBackfill function as this is not a reusable piece of code and most likely shouldn't be publicly accessible, since it permits a custom handler implementation to bypass the team size limits which should be the Server Agent responsibility now.

If anyone needs more customization with asymmetric teams or doesn't want to use the automated flow for filling teams in the Server Agent, they can always write their own custom agent. Customization at this level means that the developer should fully take responsibility, rather than try to hack around the limits we impose.

[orcist]

This implementation has one vulnerability, a handler could potentially have multiple threads or coroutines invoking this addbackfills method concurrently. This means we could start duplicate backfills, and this scenario should be guarded against by the agent itself.

The simplest fix is to transform this function into a custom mutex (don't look up the C# class it doesn't work well in Unity runtime with outdated mono compiler and poor threading support, I'm just naming the design pattern - we'll implement our own). Here's some pseudocode to give you an idea, the tricky piece is the do-while loop which will execute always at least once, because it checks the condition after running the code block. Happy to elaborate in person, this is some real dark magic here ;)

internal bool BackfillRunning, BackfillPending;

public void AddBackfills()
{
    if (BackfillRunning) { BackfillPending = true; return; }
    BackfillRunning = true;
        try
        {
            do
            {
                BackfillPending = false;
                int plannedBackfills = TargetTeamSize - (Assignments.Count + Backfills.Current.Count);

                for (int i = 0; i < plannedBackfills; ++i)
                {
                    StartNewBackfill();
                }
            }
            while (BackfillPending);
        }
        finally
        {
            BackfillRunning = false;
            BackfillPending = false;
        }
}

[orcist]

Hmm interesting, can you describe the use case where we have a group but no group ID? Is that when we initialize the observable before the first update?

[edge-marge]

yeah exactly; technically there "is" a group but everything in it is null

[orcist]

Nice and clean (no sarcasm intended), but there is a spectacular edge case that will trigger exactly twice a year and would be incredibly difficult to troubleshoot. Changing between daylight savings time and standard time will offset the clock in either direction on the server and suddenly it seems like an hour passed (or we travelled back in time by 1 hour). Additionally, DateTime.Now performs timezone conversion which slows things down a bit (less of an issue but not ideal).

After second thought, instead of checking every frame (on Update), it would be better to move this to a IEnumerator coroutine that you fire whenever enabling backfill, make sure the boolean BackfillRunning can't be set separately from triggering the coroutine. This coroutine could look something like this, utilizing Unity's native awaiter:

private IEnumerator OnEnableBackfillRoutine()
{
    if (AdmissionGracePeriodSeconds <= 0) yield break;
    yield return new WaitForSecondsRealtime(AdmissionGracePeriodSeconds);
    enabled = false;   // Update() stops being invoked
}

This is resilient to daylight savings in general and probably a quite a bit more performant. Sorry to make you jump through the hoops, my original guidance was a miss...

[orcist]

Could you eliminate this method and reuse the DelayMethod with an inlined arrow function to copy the dictionary? Perhaps DelayMethod could be expanded with an optional argument to define how many seconds to delay, which defaults to null, which is captured in DelayMethod and replaced with the default PollingBackoffSeconds + (0.1f * Random.value).