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.

[orcist]

is this constructor necessary? could you remove it and declare new DeploymentDTO like this using the implicit constructor?

var newDeployment = new DeploymentDTO() {
  Fqdn = "string value",
  PublicIP = "another string",
  Ports = new Dictionary<string, PortmappingDTO>() { ... },
  Location = new LocationDTO() { ... },
}

[orcist]

The responsibility to parse the required subset of parameters for backfills out of the DeploymentEnvironmentDTO object belongs to the new backfill sample script, adding helper like this in a generic data transfer class makes the class unnecessarily heavier on learning cognitive load for anyone who is not using Backfill.

Please can you revert the changes to this file and instead add code to the backfill sample, to read the required fields from the DeploymentEnvironmentDTO class and pass them to the constructor of BackfillAttributes ?

[orcist]

The assignment property expects you to include a request_id, which the DeploymentDTO class currently doesn't have. I would expect the matchmaker to respond with a 400 Bad Request but it's possible we don't validate this particular field, which would be a bug rather than a feature.

Can you please confirm if you tested with this code and received an OK response?

Also, I suggest to define a new class to use instead of DeploymentDTO, which will include the missing request ID in this file, something like this:

    public class BackfillAssignment : DeploymentDTO
    {
        [JsonProperty("request_id")]
        public string RequestID;
    }

[edge-marge]

can confirm I haven't gotten any 400 Bad Request responses when I tested with the code here

[orcist]

Could we rename this class to ServerAgent? It's a little more specific, and follows the server browser naming convention. I would've renamed the Client class earlier as well but I wanted to minimize breaking changes so it was kept as it was...

[orcist]

Probably can drop the comment, we don't need to explain Unity features.

Suggested change

// Update is called once per frame

[orcist]

This would make testing the client handler script in editor a bit more convenient, since you can just disable the server handler script and this hook won't be invoked.

Suggested change

	if (!DeleteBackfillOnQuit)
	if (!enabled)

[orcist]

This should be the only behavior (always delete), unless you see some scenario where we want to disable the feature. To me it's just a risk of someone accidentally flipping the switch and breaking their integration.

Suggested change

	[Header("Expiration and Cleanup")]
	public bool DeleteBackfillOnQuit = true;

[orcist]

As outlined in the general comment on this PR, this is not very reliable - the hook may not fire if the game client crashed, or the operating system may not give enough grace period to the shutdown process to actually send the request. Detecting disconnects on server side would be more reliable.

Suggested change

// todo if connected to server => Disconnect();

[orcist]

This can probably be removed in line with the general comment on this PR regarding server-side detection of disconnects.

Suggested change

	public void Disconnect()
	{
	// todo notify server with player's ticket ID, then disconnect player once processed
	L.Log($"Player {TicketID} leaving game");
	}