Skip to content

fix(test): stop tests borrowing each other's player sessions - #568

Open
Taure wants to merge 5 commits into
mainfrom
fix/zone-integration-test-isolation
Open

Taure wants to merge 5 commits into
mainfrom
fix/zone-integration-test-isolation

Conversation

@Taure

@Taure Taure commented Aug 25, 2026 •

Copy link
Copy Markdown
Contributor

world_input_crossing_touches_only_ring_delta/0 passed in a full eunit run and failed whenever the module was run on its own. That is the worse way round of the two: it was green for a reason that had nothing to do with the ring diff it exists to prove.

Cause

subscribe_interest_zones/4 resolves a player through find_player_pid/1, which reads pg:get_members(nova_scope, {player, PlayerId}). The test joined p1 without registering anything under that key, so the pid came back undefined, asobi_world_server skipped the subscribe entirely (asobi_world_server.erl:1056-1059), and every subscriber count in the test was 0.

In a full run it found a live process another module had left registered under {player, <<"p1">>} in the shared nova_scope, and passed on that.

Fix

The module already has fake_session/2 for exactly this, and its own documentation says so - "a live, pg-registered session for a player, so find_player_pid/1 ... resolve to a real process instead of silently falling through". Six other tests in the module use it. This one now does too, with a distinctive player id, since p1 is what made it borrowable in the first place.

Verification

By substitution rather than by the test simply going green: swapping the pg-registered session for a plain unregistered process reproduces the original failure exactly (expected: 1, got: 0). The assertion is now carried by the subscribe path rather than by whatever else happens to be running.

Also narrows two sys:get_state/1 reads in the same module - they are term(), and touching the module brings it into CI's EQW_SCOPE=diff gate, which requires zero errors in every module a PR touches. eqwalize-all: 195, from 197.

Checks

fmt --check, xref, dialyzer, elp lint (no new findings), eunit 2,398/0, CT 390/0, and the module now passes standalone.

Found during the review gate on #566.


Second commit: fix the mechanism, not just this test

nova_scope is shared by every module in a run and {player, Id} is not namespaced, so a registration that outlives its test is lent to any later test using the same id. That has now cost two silent wrong-reason passes, so the second commit fixes the mechanism.

fake_session/1,2 was hand-copied into four modules in two variants that differed only in whether they forwarded messages. It now lives in asobi_test_helpers, alongside:

  • unique_session/1, deriving the id from unique_id/1. A distinctive id works only until someone else picks it - the same guarantee p1 had.
  • assert_no_session/1, for tests that want the session-less path. Without it a leftover registration silently turns such a test into a different one, which is how this class stayed hidden.

The donor. asobi_world_chat_tests registers self() - the eunit runner, alive for the whole run - under {player, <<"p1">>} in seven tests, with the pg:leave as a trailing call rather than an after. Those tests assert in the middle, so a failing assertion skipped the leave and stranded the runner in the group for the rest of the run. with_registered_player/1 makes the pairing unconditional.

asobi_match_server_tests had two sessions spawned and never killed at all; both now released in an after.

Also narrows two sys:get_state/1 reads in asobi_world_server_tests, since CI eqwalizes every module a PR touches. eqwalize-all: 193, from 212 on main.

eunit 2,398/0, CT 390/0.

Gate

Architecture guardian: ACCEPT, and it caught a real flaw in the first commit - the session and world were released only from a block the failing assertion jumped over, recreating the leak class on the failure path. Fixed in 80b1229.

Security reviewer: the diff is clean; it found the production defect underneath, now fixed separately in #569.

A correction to an earlier version of this description. It said the donor was asobi_world_chat_tests and that the guardian had misattributed it. That is backwards. Every unregister_player() in the chat module was paired, so a green run left nothing behind - what that module had was the failure path. The donor was asobi_match_server_tests, which had two _-prefixed sessions on p1 and killed neither, permanently registered, in a module that sorts first. The guardian had it right; the security review did not, and this description repeated the security review.

Code reviewer: found that the helper five modules now depend on had no tests - and that who/1 had already regressed inside this PR, caught by review rather than by a test. Also that release_session/2 had no callers outside with_session/3, and that prop_reconnect_lifecycle was accumulating up to ~125 registered sessions per run. All worked in the final commit.

@github-actions

github-actions Bot commented Aug 25, 2026 •

Copy link
Copy Markdown

🟡 Code Coverage — 76.9%

9396 of 12223 lines covered.


🟡 ELP Lint — 190 warnings

210 diagnostics found. See job logs for details.


✅ ELP eqWAlize

No diagnostics.

@Taure
Taure force-pushed the fix/zone-integration-test-isolation branch from c9d4d8d to 80b1229 Compare August 26, 2026 11:28
@Taure Taure changed the title fix(test): stop the ring-diff test borrowing another module's session fix(test): stop tests borrowing each other's player sessions Aug 26, 2026
Taure added 5 commits August 26, 2026 18:18
`world_input_crossing_touches_only_ring_delta/0` passed in a full eunit run and
failed whenever the module was run on its own - which is the worse way round of
the two, because it was green for a reason that had nothing to do with the ring
diff it exists to prove.

`subscribe_interest_zones/4` resolves a player through `find_player_pid/1`,
which reads `pg:get_members(nova_scope, {player, PlayerId})`. The test joined
`p1` without registering anything under that key, so the pid was `undefined`,
asobi_world_server skipped the subscribe entirely, and every subscriber count in
the test was 0. In a full run it found a live process ANOTHER module had left
registered under `{player, <<"p1">>}` in the shared scope, and passed on that.

The module already has `fake_session/2` for exactly this, and its documentation
says so; six other tests use it. This one now does too, with a distinctive
player id - `p1` is what made it borrowable.

Verified by substitution: swapping the pg-registered session for a plain
process reproduces the original failure exactly, so the assertion is now
carried by the subscribe path rather than by whatever else is running.

Also narrows two `sys:get_state/1` reads in the same module, which are
`term()`; touching the module brings it into CI's `EQW_SCOPE=diff` gate, which
requires zero errors in every module a PR touches. eqwalize-all: 195, from 197.

The session and the world are released from a `try ... after` that opens
before the join, not after it: the first assertion in this test is the one it
used to fail on, and releasing them from a block that failure jumps over would
leak a pg-registered session and a ticking world into the rest of the run -
the class of contamination this commit exists to remove. Found by the
architecture guardian.
…scope

`nova_scope` is shared by every module in a run and `{player, Id}` is not
namespaced, so a registration that outlives its test is lent to any later test
using the same id. That has now cost two silent wrong-reason passes, so fix the
mechanism rather than the symptoms.

`fake_session/1,2` was hand-copied into four test modules in two variants that
differed only in whether they forwarded messages. It now lives in
`asobi_test_helpers` alongside:

- `unique_session/1`, which derives the id from `unique_id/1`. A distinctive id
  works only until someone else picks it, which is exactly the guarantee `p1`
  had.
- `assert_no_session/1`, for the tests that want the SESSION-LESS path. Without
  it a leftover registration silently turns such a test into a different one,
  which is how this class stayed hidden.

The donor: `asobi_world_chat_tests` registers `self()` - the eunit runner,
alive for the whole run - under `{player, <<"p1">>}` in seven tests, with the
`pg:leave` as a trailing call rather than an `after`. These tests assert in the
middle, so a failing assertion skipped the leave and stranded the runner in the
group for the rest of the run. `with_registered_player/1` makes the pairing
unconditional.

`asobi_match_server_tests` had two sessions that were spawned and never killed
at all; both are now released in an `after`.

Also narrows two `sys:get_state/1` reads in asobi_world_server_tests, since CI
eqwalizes every module a PR touches. eqwalize-all: 193, from 212 on main.

eunit 2398/0, CT 390/0.
Working the architecture guardian's findings on the previous commit, which
claimed to fix the mechanism and left five leaks standing.

`fake_session/1,2` hands back a pid and leaves the pairing to discipline. That
is how three of them ended up bound to `_`-prefixed variables and registered in
the shared `nova_scope` for the rest of the run: the `_` says the author is
discarding the pid, the pg group is not. `with_session/2,3` and
`with_unique_session/2` release in an `after`, so the pairing is structural.

Converted, all previously leaked:

- asobi_match_server_tests: `retry_after_no_session_280`, `p285_reconnect_paused`
- asobi_world_server_tests: `retry_after_no_session`

`assert_no_session/1` now has the two callers it was written for. Both
`join_with_no_live_session_does_not_crash/0` tests assert the SESSION-LESS path
and both used the literal `~"no_session"` - the same unnamespaced key, in two
modules, each depending on it being empty. They now use distinct ids and say so
before relying on it.

`prop_chat_zone_routing:ensure_listener/2` was a fifth copy under another name;
the previous commit's docstring claimed five modules had been consolidated when
four had.

`with_registered_player/1` now unwinds the `{chat, _}` groups the bodies join
as a side effect of `player_joined/3`, not just the player key. Four of the
seven never left them, and `global_chat_joined_and_left/0` joins
`global:general` - the global tier deliberately carries no world id
(#299), so it is the single most shared key in the system and a
failing assertion stranded the runner in it.

`assert_no_session/1` names the leaker rather than the victim: in a full run the
module that fails is the one that did nothing wrong, and a bare pid does not say
who registered it.

Not taken: the guardian asked to narrow `zone_config/1`'s spec to `pid()`. Its
caller is `asobi_world_instance:get_child/2`, which answers `pid() | undefined`,
so that spec would claim an input the call site cannot guarantee. The guard
already makes `undefined` a loud function_clause.

eunit 2398/0, CT 390/0, eqwalize-all 193.
…aves

Working the security review. Two of the previous commit's own additions had
stopped doing their job.

`who/1` exists to name the leaker, and the only identifying field it read was
`current_function` - which names the module the fun is WRITTEN in. Extracting
`fake_session/2` into this module made every leaker report
`asobi_test_helpers`, the one answer `assert_no_session/1` cannot use.
(`initial_call` never helped: `{erlang,apply,2}` for a plain spawn,
`{proc_lib,init_p,5}` for a real session.) The process is now labelled at spawn
with the CALLER's frame, read from `self()` - which also removes the
foreign-process introspection entirely.

`leave_chat_channels/0` under-cleaned in exactly the case that motivates it.
`pg:join` twice registers twice and one `pg:leave` removes one, and
`global_chat_joined_and_left/0` deliberately joins `global:general` through two
worlds. It drains now.

`release_session/2` replaces a bare `exit(Pid, kill)`: `pg:leave/3` is
synchronous, where a killed pid lingers in the group for tens of milliseconds -
long enough for an immediately following `assert_no_session/1` to fail naming a
dead pid, and long enough for production code reading that group to get one. It
also drains the owner's mailbox, because the owner is the SAME process for
every test in a module and undrained forwards outlive the test that caused
them.

The four sites that register `~"p1"` now use per-test ids. Each kills its
session mid-body on purpose, so an assertion failing before the kill leaves it
registered; under `p1` that survivor is borrowable by any later test, under a
per-test id it is inert. That is the donor this branch was opened for.

`no_live_session_does_not_join_as_caller/0` gained the `assert_no_session/1`
precondition its two siblings got - same class, same premise, and
`find_player_pid/1` takes the head of the group, so a leaked session would be
joined instead and the assertion would still pass.

`who/1` also guards `node(Pid) =/= node()`: `process_info/2` RAISES on a remote
pid and pg is a distributed registry, so the day asobi is not single-node an
unguarded call turns the leak report into a badarg that hides the leak. Still
no `dictionary`, `messages` or `backtrace` - asobi_lua_world keeps the whole
zone state, Luerl VM included, in its process dictionary.

A comment corrected: the strand lasts for the rest of the MODULE, not the run.
The eunit runner is per module group and pg reaps its memberships when it dies.
The cross-module vector is a spawned session outliving its creator.

Not taken here: `spawn_link` for the session. It is the structural answer to an
eunit timeout skipping the `after`, but a linked session killed with `kill`
takes the test process with it, so it forces converting all 48 `exit(_, kill)`
sites at once. The per-test ids defang the residual - a survivor nobody can
name is inert.

eunit 2398/0, CT 390/0 (serial), eqwalize-all clean on every touched module.
…e helpers

Working the code review. Three of its findings are that the previous commits
stopped short of their own thesis.

**The helper five modules depend on had no tests**, and `who/1` had already
regressed inside this PR - caught by review, not by a test. `with_session/2`
releasing on a RAISING body, `release_session/2`'s synchronous leave and its
drain, both `assert_no_session/1` branches, and `who/1` naming the calling
module rather than `asobi_test_helpers` are all pinned now.

**`release_session/2` had no callers outside `with_session/3`.** The argument
for it - a killed pid lingers in the group, and `find_player_pid/1` takes the
head with no liveness filter - applied equally to every teardown that was still
`exit(Pid, kill)`, including the one in the test this branch was opened for.
The `after`-block teardowns are converted. The mid-body kills are NOT: those
simulate an abrupt disconnect to trigger a DOWN, and a clean stop would test
something else.

**`prop_reconnect_lifecycle` was the largest remaining producer.** Nothing ever
released the sessions in its model, so up to ~125 pg-registered processes
accumulated per run - and from the second iteration onward a fresh session
joined an id the previous iteration still held, where `find_player_pid/1` takes
the head. The world could be monitoring a session the test no longer
controlled, and `check/2` would not notice: it compares player_count against
the model, and a disconnected player stays joined during grace.
`prop_chat_zone_routing` killed and re-joined the same unnamespaced ids each
iteration; both now release.

`unique_session/1` and `with_unique_session/2` are deleted. Both were dead, and
the first was the hazardous shape - caller-chosen release - while the
`fake_session/1` docstring told readers to prefer it. That docstring now points
at `with_session/2`.

`proc_lib:set_label/1` moved above the receive loop; inside it, the label was
re-applied on every forwarded message, which for a zone session is once per
world.tick. `release_session/2` now waits for the session to die before
draining, rather than draining `after 0` and hoping.

**A correction.** The previous commit and the PR body blamed
`asobi_world_chat_tests` for the zone-integration false pass. That is wrong.
Every `unregister_player()` there was paired, so a green run left nothing
behind; what that module had was the FAILURE path. The donor was
`asobi_match_server_tests`, which had two `_`-prefixed sessions on `p1` and
killed neither - permanently registered, in a module that sorts first. The
architecture guardian had this right and the security review did not.

eunit 2408/0, CT 390/0 (serial), eqwalize-all 191 from 212 on main, and zero in
every module this PR touches.
@Taure
Taure force-pushed the fix/zone-integration-test-isolation branch from b081cce to 6f638bd Compare August 26, 2026 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant