fix: give every bot id a discriminator so it identifies one bot in one match - #444
Open
Taure wants to merge 2 commits into
Open
fix: give every bot id a discriminator so it identifies one bot in one match#444Taure wants to merge 2 commits into
Taure wants to merge 2 commits into
Conversation
…e match
Bot names came from a shared list, so the first fill of every match in a
mode produced `bot_Spark`. Every group a bot is reachable through is keyed
on that id and nothing else - `{bot, Id}` for shared state, `{player, Id}`
for everything asobi_presence:send/2 delivers - so two matches holding the
same id shared those groups and each other's traffic.
`bot_id/1` now mints `bot_<name>_<discriminator>` from three bytes of
crypto entropy via `asobi_id:rand_suffix/1`, which the module already
documents as the tool for a handle that must not collide under concurrent
generation. Not `asobi_id:generate/0`: a UUIDv7 leads with a millisecond
timestamp, so two bots minted in the same tick would collide on exactly the
prefix a reader eyeballs.
Queue fill names a bot before a match exists, which is why the match cannot
be the discriminator and entropy has to be.
Three defects fall out of the collision, and out with it:
- `bots_needing_ai/1` asked whether an id had a process anywhere, not in
this match, so a second concurrent match's bot was skipped and never got
an AI at all. It held a roster slot and never acted, for the life of the
match - scan_groups/2 visits a match once (#443).
- `do_remove_bot/2` stopped every process in the id's group, so
`game.bots.remove` reached other matches' bots. It now resolves the name
or id against this match's own roster, which is the only place that can
answer which bot the script meant.
- `send_match_state/3` and `broadcast_match_event/3` fanned out to a
same-named bot in another match.
`add_bot_refusal/3` matches on the name rather than the whole id, because
an id now never repeats: "already in this match" is the question a script
asking for Spark twice is really asking, and without the change a per-tick
`game.bots.add` would seat a new bot every tick up to `?MAX_BOT_FILL`.
`name_part/1` takes the discriminator off by width rather than by splitting
on `_`, because a name may contain `_` itself. An id minted before this
change comes back whole, so a match recovered from a backup written by an
older node still resolves.
Also here, both small and both from the same review:
- the refusal log is rate limited through asobi_script_log_limiter, keyed
on the match. A script calling `game.bots.add` on every tick was an
unbounded log channel the script controlled, which is the case
asobi_match_server:log_dropped_input/3 already limits.
- asobi_world_lobby_server's cache_listing/3 doc said ten keys; it is
eight, two for worlds and six for matches.
Closes #442
🟡 Code Coverage — 75.8%7181 of 9479 lines covered. 🟡 ELP Lint — 176 warnings195 diagnostics found. See job logs for details. |
The first mutation run over the changed lines left survivors that were not all log noise: - the ?MAX_BOT_FILL ceiling was only ever exercised at exactly the ceiling, so 64 -> 63 survived. Now asserted on both sides of the boundary. - name_part/1's non-hex branch had no test, so a trailing run of the discriminator's width that is not hex could have been eaten. `bot_red_zzzzzz` is a name, `bot_red_abcdef` is a name plus a discriminator. - bot_name/2's fallback past the end of the names list was never reached, nor was the clause for a names entry that is not a binary - which a sys.config-declared mode can supply, because only the script path filters. 79/94 on the changed lines, up from 72. The 15 that remain are the ?LOG_INFO call in log_bot_not_added/3, its own metadata constants, and `ok` returns discarded by the caller: no observable effect to assert on.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bot names come from a shared list, so the first fill of every match in a mode produced
bot_Spark. Since #424,game.bots.add(name)also lets a script pick any 32-byte name, so a collision became script-reachable rather than merely likely.Every group a bot is reachable through is keyed on that id and nothing else:
{bot, Id}for shared state,{player, Id}for everythingasobi_presence:send/2delivers. Two matches holding the same id shared those groups and each other's traffic.The fix
bot_id/1mintsbot_<name>_<discriminator>from three bytes of crypto entropy viaasobi_id:rand_suffix/1, which that module already documents as the tool for a handle that must not collide under concurrent generation.Not
asobi_id:generate/0: a UUIDv7 leads with a millisecond timestamp, so two bots minted in the same tick collide on exactly the prefix a reader eyeballs. Not the match id either, because queue fill names a bot before a match exists. Entropy is what is available at the moment the name is drawn.What that fixes
A bot in a second concurrent match never got an AI (#443).
bots_needing_ai/1asks the pg group behindbot_pids/1whether an id already has a process, and that group carries no match. Match A's live Spark answered for match B, so match B's roster entry was skipped. It held a slot and never acted for the life of the match, becausescan_groups/2visits a match once. This is a regression from #424; before it the loop started an AI for every roster bot.game.bots.removereached other matches.do_remove_bot/2stopped every process in the id's group and only consultedMatchPidon the empty-list fallback. It now resolves the name or the id against this match's own roster, which is the only thing that can answer which bot the script meant. Two tests pin it: an identically named bot in another match survives, and an id that is not on this roster is not touched at all.Shared state and match events were cross-delivered.
send_match_state/3resolves bots through?BOT_GROUP(PlayerId)andbroadcast_match_event/3fans out throughsend/2on{player, Id}. Both were reaching a same-named bot in another match. Unique ids close them without touching presence.Two knock-ons worth reading
add_bot_refusal/3now matches on the name rather than the whole id. An id never repeats, solists:member(BotId, Players)could never say "already here" again, and a per-tickgame.bots.addwould have seated a new bot every tick up to?MAX_BOT_FILL. The question a script asking for Spark twice is really asking is whether this match already holds a bot it called Spark.fill_until_loop/6'salready_queuedbranch can no longer be reached by id reuse, because the matchmaker's per-(player, mode) idempotency has nothing to match on. The branch stays as the guard it is, and its test now keys the double on the name the index drew. Fill still converges: it is bounded by the queue depth passed in, not by the idempotency.name_part/1takes the discriminator off by width rather than splitting on_, because a name may contain_itself. An id minted before this change comes back whole, so a match recovered from a backup written by an older node still resolves.Also here
Both from the same review of #424, both small:
asobi_script_log_limiter, keyed on the match. A script callinggame.bots.addon every tick was an unbounded log channel the script controlled, which is exactly the caseasobi_match_server:log_dropped_input/3already limits. Keyed on the match rather than the bot so a script looping over names does not get a fresh bucket per name.asobi_world_lobby_server:cache_listing/3's doc said ten cache keys. It is eight, two for worlds and six for matches.Wire
Roster ids change shape:
bot_Sparkbecomesbot_Spark_a3f91c. Additive in the sense that nothing new appears on the wire, but a client or script matching a bot id exactly rather than on thebot_prefix will stop matching.is_bot/1is unaffected.game.bots.removetakes the bare name or the full id, so no script has to learn about the discriminator.lua-api.mdandlua-bots.mdupdated, including the "Bot ids" section, which now says plainly that an id is an address and the chosen name is its middle section.Verification
fmt, xref, dialyzer and ex_doc clean. 1789 eunit and 360 CT, zero failures. eqwalizer is net zero against main: 322 before, 322 after, the one standing error in this module being the pre-existing
min/2atfill_until/4. elp lint is net zero too, 194 both sides, none in the touched files.Mutation testing on the changed lines: 79/94, 84.0%. Its first run found four real gaps, which is the second commit - the
?MAX_BOT_FILLboundary only ever tested at the ceiling,name_part/1's non-hex branch untested, and both ofbot_name/2's fallbacks unreached. The 15 survivors that remain are the?LOG_INFOinlog_bot_not_added/3, its own metadata constants, andokreturns the caller discards: nothing observable to assert on.Baselines for eqwalizer and lint were measured by stashing this branch rather than taken from a previous PR's numbers. Worth noting CI skips
ELP eqWAlizeandMutation Testingon this repo, so neither tick above comes from CI.Closes #442