lance.blue / sds
bayes for days
Star
Fork 0
Watch
sds docs UPSTREAM.md
18 kB
Markdown
at claude/declaration-order

Upstream notes #

Defects and sharp edges found in MegaMek 0.51.0 while building this harness, with what we do about each. Line numbers are from the 0.51.0 source tree.

Two reasons this file exists. A workaround with no recorded cause gets removed by the next person who thinks it looks superstitious. And several of these are worth reporting upstream, which needs the analysis written down somewhere other than a commit message.

Nothing here is a criticism of MegaMek. Almost all of it only bites a headless, bot-only, many-matches-in-parallel workload, which is not what it was built for.

1. AutosaveService spins forever on String.format #

Severity: fatal. Not headless-specific. This is a real, unconditional bug.

server/AutosaveService.java:103-119, getAutosaveFilename():

String localDateTime = LocalDateTime.now().format(...);   // computed once
while (repeatedName) {
    fileName = String.format(FILENAME_FORMAT,
          gameManager.getGame().getCurrentRound(),          // loop-invariant
          localDateTime);                                   // loop-invariant
    repeatedName = false;
    for (final File file : autosaveFiles) {
        if (file.getName().compareToIgnoreCase(fileName) == 0) {
            repeatedName = true;                            // nothing ever changes
            break;
        }
    }
}

Both format arguments are loop-invariant, so the generated name is identical on every pass. If a file of that name already exists, the loop never terminates.

It runs on the packet pump thread, holding serverLock and GAME_LOCK, from TWPhasePreparationManager (case INITIATIVE) — after AbstractGameManager.changePhase has already set the phase. So the game freezes at INITIATIVE with every other thread healthy, the sockets fine, and no exception anywhere.

Why it fires: MMConstants.SAVEGAME_DIR is "savegames" relative to the process working directory, so every match launched from the same directory shares one folder. The default stamp format has one-second resolution, and the default max_rotating_round_saves = 3 leaves the two newest files in place. Two matches reaching their first initiative in the same wall-clock second collide.

What we do: set OptionsConstants.BASE_MAX_NUMBER_ROUND_SAVES to 0 in SdsHost, which short-circuits performRollingAutosave() before the loop. It also removes a full XStream serialisation of the Game to gzipped XML from the packet pump once per round, which a bot-only benchmark has no use for.

Upstream fix is two lines — hoist the format out of the loop and vary something per attempt, or bound the loop:

for (int attempt = 0; ; attempt++) {
    fileName = String.format(FILENAME_FORMAT, round,
          localDateTime + (attempt == 0 ? "" : "-" + attempt));
    ...
}

2. Lost wakeup in Server.PacketPump.run #

Severity: hangs a quiet game. Observed 2026-08-19.

server/Server.java:151-163. The packetQueue.isEmpty() check is outside the monitor; the producer at :299-302 does synchronized (packetQueue) { add; notifyAll; }. An add landing between the check and the wait() at :161 loses the notify, and the pump sleeps with a non-empty queue until the next packet.

A GUI game always has more traffic. A headless bot-only game sitting in a report phase waiting on exactly two Done packets does not, so the hang is permanent.

Confirmed on 2v2-clan-invasion-s101020, which stalled at round 3. The dump:

Packet Pump                    WAITING
    at java.lang.Object.wait(Object.java:339)
    at megamek.server.Server$PacketPump.run(Server.java:161)
Client Connection, Player North RUNNABLE   (blocked reading the socket)
Client Connection, Player South RUNNABLE

Both clients waiting on a server whose pump is asleep, which is the predicted signature. No frame of ours appears except the dump writer.

Intermittent: the same scenario and seed ran to completion five times either side of it. Timing decides, so a rate cannot be read off one observation.

What we do: record it and move on. bridge/ compiles against the stock jar, and the only fix inside that constraint is to wake the pump from outside, which means reflecting into a private field.

SdsHost has always taken --stall-seconds, but nothing passed it, so a hang cost the 300 s default. MatchSpec.stall_seconds and the CLI's --stall-seconds now reach it; the default is still 300, and a run that expects to meet this can lower it. Still recorded rather than retried: a harness that silently reruns stalls hides the bug that causes them.

Not to be confused with the autosave hang (#1), which also parks the pump but does it by spinning inside handle rather than by sleeping in wait. The dump tells them apart: spinning shows String.format, this shows Object.wait.

3. A dead packet pump is silent #

Server.PacketPump.run (server/Server.java:152-157) has no try/catch, and TWGameManager.handlePacket:731 calls the whole phase engine outside its own try, catching only InvalidPacketDataException. Any RuntimeException in the phase engine propagates out of run() and kills the pump; the server then queues packets forever and processes none.

MegaMek installs a default uncaught-exception handler in MegaMek.main (MegaMek.java:114), which a program embedding Server never runs — so the failure is completely invisible and looks exactly like a phase that will not advance.

What we do: SdsHost installs its own handler first thing in main, and dumps every thread's state on a stall.

4. Tactical Genius deadlocks a bot-only game #

TWGameManager.checkReady:1456-1465: on an initiative reroll request it calls resetActivePlayersDone(), re-rolls, and sends SENDING_REPORTS_TACTICAL_GENIUS — with no PHASE_CHANGE.

BotClient only ever sends Done from changePhase (BotClient.java:578-585), and Client.java:1109-1122 handles that packet by firing a GameReportEvent, which re-enables a human's Done button and does nothing in a bot. Both bots have just been set back to not-done, and nothing will ever set them done again.

So a pilot with the tactical_genius SPA permanently deadlocks INITIATIVE_REPORT in a game with no humans.

What we do: nothing. Our generated scenarios use plain regular pilots. If a scenario ever grants the SPA, this is what it will do.

5. CLIENT_NAME is handled off the packet pump #

server/Server.java:266-274 dispatches CLIENT_NAME straight to handle() on the connection reader thread, with no lock. receivePlayerName → sendCurrentInfo → TWGameManager.sendCurrentInfo:694-699 calls endCurrentPhase() when the phase does not use turns.

A client connecting or reconnecting outside the lounge therefore advances the phase from a second thread, concurrently with the pump.

What we do: attach all bots in the lounge and never reconnect one mid-match.

6. WeaponType.getDamage() returns a sentinel for cluster weapons #

Not a bug — a trap. WeaponType.DAMAGE_BY_CLUSTER_TABLE = -2 and DAMAGE_VARIABLE = -3. Every missile weapon returns -2, so any consumer that treats getDamage() as a number gets negative damage for every LRM and SRM in the game.

We shipped exactly that bug for a while: the bot was told its missiles did minus two damage and duly valued them below nothing.

What we do: Observation.java uses getBattleForceDamage(range, fcs) * 10, which is MegaMek's own averaging and which the subclasses correct where they must (SRMWeapon overrides it with a factor of two). There is deliberately no plain damage field on the wire.

7. Precognition does not know two packet commands #

client/bot/princess/Precognition.java mirrors Client's packet switch but is missing UPDATE_CUT_HEXES and SYNC_TEMPORARY_ECM_FIELDS, both of which the server sends once per round. Its default: arm logs at ERROR and breaks — no retry, no re-queue, no feedback loop.

Purely log noise, and only on the Princess side, but it is several lines per round in every match log.

What we do: nothing. Raising the log level for that class would hide real Precognition errors too.

8. MMLogger sharp edges #

  • parametrizedStringAnyway (logging/MMLogger.java:355-357) falls back to String.format for any message that does not contain {}, so a message with a stray literal % throws UnknownFormatConversionException from inside a log call.
  • warn(Throwable, ...), error(Throwable, ...) and debug(Throwable, ...) call Sentry.captureException outside the level check.

What we do: -Dsentry.dsn= on every match JVM.

9. SanityInputFilter is expensive per deserialised class #

common/net/marshalling/SanityInputFilter.java runs ~40 regex matches for every class in an incoming object stream, plus a fallback doing className.contains(pattern.toString()) — a substring search over the regex source text, which is not what that was meant to do.

On a loopback socket with two bots this is measurable but not fatal.

What we do: nothing. Noted in case packet handling ever shows up in a profile.

10. VictoryHelper freezes the victory conditions at construction #

Severity: silent wrong results. Not headless-specific. Any caller that sets a victory game option after the Game exists is affected.

server/victory/VictoryHelper.java. javap -p on the 0.51.0 jar:

private final boolean checkForVictory;
private final java.util.List<VictoryCondition> victoryConditions;
public VictoryHelper(megamek.common.game.Game);
private void buildVCList(megamek.common.game.Game);

Both are final and both are filled from the game options in the constructor, by buildVCList. BVDestroyedVictoryCondition is built there from VICTORY_BV_DESTROYED_PERCENT. Nothing re-reads the option afterwards.

Game.createVictoryConditions() makes a fresh helper, and scenario.createGame() has already called it by the time a host gets the game back. So the ordinary-looking sequence - load the scenario, then configure the options - sets values that nothing will ever read.

Nothing fails. There is no exception, no warning, and the matches still play; they simply run to elimination or the round limit, because the condition that would have stopped them was never in the list. In 8v8 matches across 26 player records, no loser finished between 20% and 40% BV, where a 70%-destroyed stop lands, and eleven finished at exactly zero.

What we do: call game.createVictoryConditions() in SdsHost after setting the options, which rebuilds the helper against them.

Upstream fix is a documentation change at least: createVictoryConditions is public and does the right thing, but nothing says it must be called again after a victory option changes. Rebuilding the helper lazily on first checkForVictory after an option write would remove the ordering requirement altogether.

11. FormationType.qualifies divides by zero on the Order Lance #

Severity: fatal to the caller, and one formation only.

client/ratgenerator/FormationType.java, the grouping half of qualifies:

int groupSize = Math.min(gc.getGroupSize(), matching.size());
int numGroups = Math.min(gc.getNumGroups(), matching.size() / groupSize);

createOrderLance builds its grouping constraint through the three-argument GroupingConstraint constructor, which leaves groupSize at its default of 0. Every other formation with a grouping constraint uses the six-argument one and passes 2. So for the Order Lance the second line divides by zero, and qualifies throws ArithmeticException rather than answering.

It is invisible in the RAT generator because nothing calls qualifies on the Order Lance with a list to check: generateFormation builds a formation from the tables instead, and the Analyze Formation dialog is driven by a formation the user has already picked. Any caller that walks all 41 formations and asks each one hits it on the first Order Lance.

What we do: crates/sds-core/src/formation.rs reads the definitions rather than calling qualifies, so this cannot throw here; the case is recorded because it is the reason the Order Lance's grouping constraint reads size=0 groups=1 in crates/sds-core/tests/corpus/formations.txt, which looks like a dump defect and is not.

Upstream fix is one word: createOrderLance should use the constructor that sets a group size, or qualifies should guard groupSize == 0 by treating the constraint as unsatisfiable rather than dividing by it.

12. The Anti-Air Lance's description names a quirk it never tests #

Severity: cosmetic, and expensive to a reader.

client/ratgenerator/FormationType.java, createAntiAirLance(). The constraint's description is "Standard AC, LBX, Artillery weapon, Anti-Air targeting quirk" and its report-metric key is "AC/LBX/Artillery/AA Quirk". The predicate behind both is:

ms -> getMissionRoles(ms).contains(MissionRole.ANTI_AIRCRAFT)
      || ms.getEquipmentNames().stream().map(EquipmentType::get)
            .anyMatch(eq -> eq instanceof ACWeapon
                  || eq instanceof LBXACWeapon
                  || eq instanceof ArtilleryWeapon);

There is no quirk test. FormationType never calls MekSummary.getQuirkNames() or getWeaponQuirkNames() anywhere in the class. The fourth alternative is the RAT generator's own MissionRole.ANTI_AIRCRAFT, which ModelRecord parses out of data/forcegenerator, and which a unit can carry with no anti-air quirk at all.

The description is the only documentation these constraints have — the qualificationReport prints it to a player, and a port reads it — so a reader who takes it at its word looks for a quirk that is not consulted and misses a data-file role that is.

What we do: read the three weapon classes, which is the half that is a fact about a machine on the board, and record the mission-role half as a stated narrowing in docs/FORMATIONS.md. The two are an or, so the narrowing can only refuse a force MegaMek would have accepted.

Upstream fix is the description string: "Standard AC, LB-X, artillery weapon, or the anti-aircraft mission role".

13. A combat vehicle's front hit table never rolls the right side #

Reported upstream: https://github.com/MegaMek/megamek/issues/8850 (filed 2026-08-29 against 0.51.0). Check it before assuming this entry is still live.

Severity: play-affecting, and not headless-specific. It changes where damage lands in any game MegaMek resolves.

Tank.rollHitLocation(int table, int side) with side = SIDE_FRONT returns LOC_LEFT on a 5 and on a 9. Every other arc separates the pair:

arc 5 9
front LS LS
left FR RR
right FR RR
rear LS RS

So a vehicle shot from in front never takes a right-side hit at all, and takes left-side hits at twice the rate the printed table gives. Rolls 5 and 9 are 4 ways of 36 each, so this is 8 rolls in 36 landing in the wrong place - not a corner case.

Confirmed by live fire. The table read above is a dice-pinned probe, which is a fine way to read a table and a poor way to be believed. So it was fired instead: bridge/sds/SdsVehicleLiveFire.java puts a real Gürteltier MBT on a real board, deploys four Atlases around it, and resolves 200,000 hit locations per arc with real dice, taking the arc from Entity.sideTable (re-checked every iteration, drift 0) and the labels from MegaMek's own getLocationAbbreviations(). No damage is applied, so no location can be destroyed and bias what follows; the run asserts none was.

arc FR RS LS RR TU
front 0.6121 0.0000 0.2213 0.0000 0.1666
rear 0.0000 0.1115 0.1111 0.6116 0.1657
left 0.1123 0.0000 0.6103 0.1111 0.1664
right 0.1115 0.6111 0.0000 0.1112 0.1663

Zero right-side hits in 200,000 from the front, and the other three arcs are the control that makes it mean something: RS is plainly reportable, since the rear arc returns 22,309 of them and the right arc 122,214. Three arcs split their two adjacent locations evenly - rear 0.1111/0.1115, left 0.1123/0.1111, right 0.1115/0.1112 - and the front does not, giving one side 0.2213 and the other nothing. One arc of four failing to split, on the same entity, the same dice and the same counter, is a single-cell defect and not an instrument.

The static probe agrees, and it eliminated its own alternatives first:

  • both rows consume exactly two randomInt(6) calls, so neither is reaching for a third die the pinned source would be answering wrongly;
  • forcing a third die to each of its six values moves neither row;
  • Tank.SIDE_LOC_MAPPING is {FRONT->FR, LEFT->LS, RIGHT->RS, REAR->RR}, so the arc-to-location mapping is not the cause;
  • VTOL.rollHitLocation, which is a different method, does separate them - front 5 gives RS and front 9 gives LS.

What we do: model it. crates/sds-core/src/hitloc.rs holds MegaMek's table rather than the printed one, because MegaMek is what resolves the shots, and a_vehicles_front_table_hits_one_side_twice_and_that_is_megameks fails the day it changes.

Upstream fix is one cell. Worth reporting: it is silent, it is not bot-specific, and a player would have to roll a few hundred front hits on a tank to notice their right side was never getting touched.