Conversation
A test actor is a connection-less player which executes commands (walk, attack, skill, say, pickup, warp) and records what the game would have shown to its client as a stream of JSON events with attribution (who hit whom). It exists to test game mechanics - PvP, parties, damage - without a client. - GameLogic/TestActors: ScriptedPlayer (a Player, not an OfflinePlayer, so that bots, mini games and the admin panel treat it as a human), event log, recording view plug-in container, hit recorder, command loop, registry, protocol handler and JSON serialisation. - Startup/TestActors: an NDJSON-over-TCP control endpoint hosted as a BackgroundService. It is off by default and only started when the OPENMU_ACTOR_PORT environment variable names a port; it has no authentication and is meant for loopback use on development servers. Registered in the all-in-one Startup host only. - Network/LoginProbe: a console tool which logs an account in over the real wire (connect server -> game server) and holds the session, for tests which need a genuine client connection. - tests/MUnique.OpenMU.Tests/TestActors: 43 unit tests.
Adds the server-features page for the actors to the docs website, registers it in the sidebar, and gives the login probe a Readme like the other nested tool projects.
|
shortly, without AI slop: this is a great tool for (semi-)automated testing of features, where one can instruct an AI agent via local port to control the bot and issue it commands for testing. |
The Codacy run on the pull request reported findings in the added files; this fixes the ones which are real, without changing behaviour: - public constants of the actor protocol (error codes, object kinds, the environment variable name and the defaults) became public static readonly fields, so they are not inlined into consumers (SonarC# S2339). The two event log defaults were used as optional parameter values, which needs a compile time constant, so those parameters are nullable now; - removed the usings which the enclosing namespace already provides (S1128), and the stray blank line they left behind; - the argument parser of the login probe got one statement per line, an invariant culture for its int.Parse calls and a while loop instead of a for loop whose counter it advanced twice (S122, S4056, S127), and no longer keeps the unused result of the connection info request (S1481); - IGameServerContextLocator.GetContexts() and ActorControlService.GetConfiguredPort() became properties (S4049); - the catch which only rethrew a cancellation is now an exception filter on the general catch (S2737), and the comment which read like commented out code was rewritten (S125); - the documentation page rewraps an over-long line and the tool readme labels its fenced block (markdownlint MD013, MD040). Verified: 43 unit tests pass, the login probe and the Startup image build without a new StyleCop warning in the touched files, the docs website builds, and the image answers spawn/state/stop on the control port while staying silent without OPENMU_ACTOR_PORT.
|
response to Codacy's 9 issues (the rest fixed): What cannot be fixed (9) Codacy runs SonarC# without a compilation, so semantic rules misfire:
Verification (actual outcomes)
|
sven-n
left a comment
sven-n
left a comment
There was a problem hiding this comment.
Thanks for writing this up as an issue first and for the very thorough PR description — that made the review a lot easier. I read through the whole diff; overall the design is sound and it clearly comes from actually using the thing: deriving from Player instead of OfflinePlayer with the reasoning spelled out, running every command through the player's persistence lock, the interruptible long commands, the double-checked account occupancy in ActorRegistry.SpawnAsync (login server and a scan of every context of the process, because bots and offline sessions bypass the login server), and taking hit attribution from IAttackableGotHitPlugIn because IShowHitPlugIn does not carry the attacker. The remarks sections explain the why in the places where it is not obvious, which is what I want in this code base.
I left six inline comments. The first one is the only one I consider blocking; the rest are things I'd like to see addressed or at least answered.
- The endpoint binds
IPAddress.Anywhile both the class remark and the docs page promise loopback. Please bind loopback by default. ActorHitRecorderPlugInis active by default on every installation, including ones which never enable the feature.ActorRegistry.List()/Find()block synchronously on a semaphore which is held across the whole spawn.walkcan answerokwhen the engine refused or truncated the walk.bots onpermanently rewrites persisted bot settings which it does not own.- Unchecked numeric casts in the protocol handler.
On the two design questions from #963:
- Endpoint configuration. I'd keep the environment variable — a plugin configuration would be a nicer fit for the admin panel, but it would also mean the endpoint can be switched on in a UI on a live server, which is the opposite of what I want for an unauthenticated port. Environment variable plus a loopback default is the safer combination. A shared secret is not needed as long as it is loopback-only; if you want one anyway, a simple
{"token":"…"}field checked before dispatch would do, and I would not build anything more than that. - Distributed deployment. "Single-process host only" is fine, and the docs page states it. The Dapr host would need the registry to reach across processes, which is a different feature; let's not carry that complexity for a test tool.
Two smaller things for the PR body rather than the diff:
PosixSignalRegistrationfor SIGTERM inActorControlService. The comment is right that this host's shutdown goes throughAppDomain.ProcessExitand thatStopAsyncoften does not get to run in a container — but that is a shortcoming of the host, not of the actors, and a test feature installing a process-wide signal handler to work around it is the wrong layer. It also blocks the signal thread for up to eight seconds. Could you split that out? If the host's shutdown is fixed properly (so hosted services are stopped on SIGTERM), every player benefits, and the handler here can go away. A separate PR or issue for that is welcome.--passwordon the command line of the login probe is visible inpsand lands in shell history; an environment variable or stdin would be the usual way. Minor, given it targets local test accounts, but easy to do.
Placement in src/GameLogic/TestActors/ is acceptable to me — it needs too much internal access to live elsewhere, and Bots/ and Offline/ already set the precedent. Test count and the docs page are appreciated.
One process note, since the PR says the code was written with AI assistance: I read it as a whole and the reasoning holds up, but please make sure every non-obvious remark still matches the code after the next round — the IPAddress.Any case above is exactly the kind of drift between a confident comment and the implementation that I want to avoid in this repository.
Generated by Claude Code
| // straight from the signal handler makes the shutdown promise hold regardless. | ||
| this._sigTermRegistration = PosixSignalRegistration.Create(PosixSignal.SIGTERM, _ => this.StopActorsOnSignal()); | ||
|
|
||
| var listener = new TcpListener(IPAddress.Any, this._port); |
There was a problem hiding this comment.
This is the one thing I'd like changed before anything else: the listener binds IPAddress.Any, while the class remark above says "the local stack publishes that port on the host's loopback address only" and the docs page says "Bind it to a loopback address, never expose it". Inside Docker that holds because of the port publishing, but the host is also run directly (dotnet run --project src/Startup, which is exactly what the PR description suggests) — there the variable alone opens an unauthenticated endpoint on every interface which can animate any account, walk it, chat as it, and switch the population bots on and off.
Please bind IPAddress.Loopback by default and only widen it when the operator explicitly asks, e.g. by letting the variable take host:port and defaulting the host to loopback. Then the code matches the documentation instead of relying on the deployment to contain it, and the "off by default" promise also covers "not reachable from outside by accident".
Generated by Claude Code
| /// recording view container nor a command result appends one, so a hit is recorded exactly once per | ||
| /// involved actor (once on each side when two actors fight each other). | ||
| /// </remarks> | ||
| [PlugIn] |
There was a problem hiding this comment.
A [PlugIn] which does not implement IDisabledByDefault is created as active by the data initialization (DataInitializationBase: plugInConfiguration.IsActive = !plugInType.IsAssignableTo(typeof(IDisabledByDefault))). So this test-only plugin runs on every single hit of every server, including production installations which never set OPENMU_ACTOR_PORT, and it shows up in the admin panel's plugin list for everyone.
The two type checks per hit are cheap, so this is about intent rather than throughput: please add IDisabledByDefault and mention in the docs page that the plugin has to be activated together with the endpoint, or drop the plugin attribute and register the plugin point only when the endpoint is enabled.
Generated by Claude Code
| this._lock.Wait(); | ||
| try | ||
| { | ||
| return this._actors.Values.ToList(); | ||
| } | ||
| finally | ||
| { | ||
| this._lock.Release(); | ||
| } | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public ScriptedPlayer? Find(string loginName) | ||
| { | ||
| this._lock.Wait(); |
There was a problem hiding this comment.
List() and Find() block synchronously on _lock, and that same semaphore is held across awaits in SpawnAsync — a database load of the account, the login server call and the whole world entry. Every ping, list and every actor command goes through Find, so while one spawn is running, each of those blocks a thread pool thread for its whole duration instead of yielding. With a couple of scripts against one endpoint that is easy to notice, and it is the kind of sync-over-async the rest of the code base avoids.
Since both callers (ActorProtocolHandler) are already async, I'd make them ListAsync/FindAsync and await _lock.WaitAsync().
Generated by Claude Code
| while (this._player.IsWalking) | ||
| { | ||
| if (cancellationToken.IsCancellationRequested) | ||
| { | ||
| return await this.InterruptWalkAsync(path.Count, walked).ConfigureAwait(false); | ||
| } | ||
|
|
||
| try | ||
| { | ||
| await Task.Delay(WalkPollMilliseconds, cancellationToken).ConfigureAwait(false); | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| // Handled by the check at the top of the loop. | ||
| } | ||
| } | ||
|
|
||
| walked += chunk.Count; | ||
| } | ||
|
|
||
| return ActorCommandResult.Success( | ||
| new ActorEventField("steps", path.Count), | ||
| new ActorEventField("walked", walked), | ||
| new ActorEventField("x", this._player.Position.X), | ||
| new ActorEventField("y", this._player.Position.Y)); | ||
| } | ||
|
|
There was a problem hiding this comment.
walk can report ok although the actor never moved, which is exactly the "silent no-op" the rest of the design rules out.
PlayerMovement.WalkToAsync refuses a walk whose first step is blocked (it resynchronizes the client and returns), and truncates the path at the first blocked later step. In the refusal case IsWalking is false immediately, the while loop falls through, walked += chunk.Count counts the chunk as done, and the command answers success with steps == walked == path.Count. In the truncation case the next chunk is built from the actual position but continues with the original path nodes, so its steps are no longer adjacent tiles.
The path finder uses Terrain.AIgrid while the movement check uses the walk map, so the two do disagree in practice (safe zones, temporarily blocked tiles). Suggestion: after each chunk compare this._player.Position with the chunk's last node, and on a mismatch stop and answer no_path (or a partial result) with the position actually reached; the final success can then also assert Position == target.
Generated by Claude Code
| configuration.NumberOfAccounts = Math.Max(0, count ?? DefaultBotCount); | ||
|
|
||
| // One character per account, so the requested number is what stands in the world, and no | ||
| // bot logs out again while a scenario runs. | ||
| configuration.MaxCharactersPerAccount = 1; | ||
| configuration.PresenceRotation = false; | ||
| } |
There was a problem hiding this comment.
bots on overwrites NumberOfAccounts, MaxCharactersPerAccount and PresenceRotation in the persisted plugin configuration and never restores them, so a single test command permanently replaces whatever the operator had configured in the admin panel, surviving the restart. On a dedicated test server that is harmless, but it is a surprising amount of state for a tool which is otherwise careful not to disturb the server.
Either restrict the write to Enabled (+ NumberOfAccounts when a count is given) and leave the character/rotation settings alone, or keep the overwrite but return the previous values in the result and document it on the docs page so a scenario can put them back.
Generated by Claude Code
| "walk" => new WalkCommand((byte)(GetNumber(request, "x") ?? 0), (byte)(GetNumber(request, "y") ?? 0)), | ||
| "attack" => new AttackCommand( | ||
| GetTarget(request) ?? string.Empty, | ||
| (int)(GetNumber(request, "times") ?? 1), | ||
| (int)(GetNumber(request, "interval") ?? 0)), | ||
| "skill" => new SkillCommand((ushort)(GetNumber(request, "skill") ?? 0), GetTarget(request) ?? string.Empty), | ||
| "say" => new SayCommand(GetString(request, "text") ?? string.Empty), | ||
| "pickup" => new PickupCommand((ushort)(GetNumber(request, "id") ?? 0)), |
There was a problem hiding this comment.
Nit, same spirit as "nothing is a silent no-op": these casts are unchecked, so {"cmd":"walk","x":300,"y":132} walks to x=44 and a negative or oversized skill/id/slot wraps just as quietly. A range check answering bad_request would be a few lines in GetNumber (e.g. an overload taking the allowed range) and would keep a typo in a scenario from producing a plausible-looking wrong result.
Generated by Claude Code
Changes requested in the review of the pull request, in its order: - The control endpoint binds the loopback address by default. The variable OPENMU_ACTOR_PORT still takes a bare port, which now means 127.0.0.1:port; an operator who wants another address names it explicitly as host:port (0.0.0.0:55990 inside a container, whose port publishing then contains it). A value which is neither is logged and ignored, so the endpoint stays off. The class remark and the docs page now describe what the code does. - ActorHitRecorderPlugIn is no longer a discoverable [PlugIn]: it gets no persisted configuration, is not listed in the admin panel and does not run on servers which never enable the actors. The control endpoint registers it at the IAttackableGotHitPlugIn point when it starts. - IActorRegistry.List/Find became ListAsync/FindAsync and await the registry's semaphore, which is held across the whole spawn; no caller blocks a thread pool thread on it any more. - A walk verifies its progress: after each chunk the actor's position is compared with the chunk's last node, and a refused or truncated walk answers no_path with the position and steps actually reached instead of ok. The final success also asserts that the actor stands on the target. - bots on/off writes only Enabled, plus NumberOfAccounts when --count is given; MaxCharactersPerAccount and PresenceRotation are left as the operator configured them, and bots status reports all three. - Numeric request fields are range-checked (byte, ushort, int, and a minimum of 1 for attack --times); a value outside its range answers bad_request instead of being wrapped by a cast. - The SIGTERM handler is gone from ActorControlService; the actors are stopped from StopAsync only, like every other hosted service. Making the host stop its hosted services on SIGTERM is a separate change. - The login probe reads the password from LOGINPROBE_PASSWORD instead of a --password argument, so it is neither visible in the process list nor kept in a shell history. Tests: the walk tests cover a refused and a truncated walk, the protocol tests cover out-of-range numbers for every numeric field, and the bots tests assert that the untouched settings survive. 53 tests pass.
|
Thanks for the careful read - all eight points are addressed in
On the two PR-body notes:
And on the process note: I re-read every remark against the code after this Testing: 53 unit tests pass ( Codacy will report one more S1128 ( |
2 participants
Closes #963.
What
Connection-less players which execute commands and report what the game shows
them, for testing game mechanics without a client. Docs page:
docs-website/docs/server-features/test-actors.md.src/GameLogic/TestActors/-ScriptedPlayer(aPlayer, not anOfflinePlayer: bots defend against it, mini games count it, the admin panelshows it as a player), the command loop (runs on the player's own tick,
inside the persistence lock; long commands are interruptible), the event log
(strictly increasing sequence numbers, UTC timestamps), a recording view
plug-in container, a hit recorder on
IAttackableGotHitPlugInforattacker/victim attribution, the registry, the protocol handler and JSON.
src/Startup/TestActors/-ActorControlService, aBackgroundServicespeaking newline-delimited JSON over TCP. Registered in
Program.csonlywhen
OPENMU_ACTOR_PORTnames a port; otherwise nothing is listening. Itbinds
127.0.0.1unlessOPENMU_ACTOR_ADDRESSnames another address. Itcan also switch the population bots on and off through their persisted
plug-in configuration, so an actor has opponents.
src/Network/LoginProbe/- nested console tool (likeNetwork/Analyzerand
ChatServer/ExDbConnector) which logs an account in over the real wireand holds the session; used to test that a connected account cannot be
animated by an actor. The password comes from
LOGINPROBE_PASSWORD(default: the account name).
tests/MUnique.OpenMU.Tests/TestActors/- 53 unit tests.Commands:
spawn,stop,list,state,nearby,walk,attack,skill,say,pickup,warp,halt,events [--since N] [--follow],bots on|off|status. Every command answers with a result or an error code(
out_of_range,safezone,not_in_view,unknown_skill,no_path,interrupted, ...).Try it
Design notes / open points
default. Per the review: environment variable rather than a plug-in
configuration, no shared secret while it is loopback-only.
ActorHitRecorderPlugInis deliberately not a discoverable[PlugIn]; theendpoint registers it at the
IAttackableGotHitPlugInpoint on start, so itneither runs nor shows up on servers which never enable the actors.
Startuphost only; the DaprGameServer.Hostdoes not open it (stated in the docs page).skillcasts on a target only; area/position casting(
AreaSkillAttackAction) is left out on purpose.Startup/Program.cs(registration),Network/MUnique.OpenMU.Network.csproj(excludes the nested tool, likePackets\**),MUnique.OpenMU.sln,docs-website/sidebars.js.Testing
A recording of the feature being exercised live on a running server, driven
by a coding agent on my behalf: https://www.youtube.com/watch?v=lQuFN4KtkuI
dotnet test tests/MUnique.OpenMU.Tests --filter FullyQualifiedName~TestActors: 53 passed.MUnique.OpenMU.GameLogicandNetwork.LoginProbebuild with zero StyleCopwarnings in the added files.
Startup/Dockerfileimage built from this branch; started with and withoutOPENMU_ACTOR_PORT(listener present / absent), with the port alone(bound to
127.0.0.1inside the container) and withOPENMU_ACTOR_ADDRESS=0.0.0.0, and a spawn → walk → attack → events runagainst a population bot on a fresh PostgreSQL database.
it was verified. Codacy: the nine tool artefacts listed in the comments
plus one more S1128 of the same kind (
using System.Net;inActorEndpointOptions.cs, whereIPEndPointandIPAddressare used).Written with AI assistance and tested by hand, per CONTRIBUTING.