Fix stale position packets and remember minigame arenas

CHANGELOG.md

@@ -12,8 +12,15 @@
 - Tests that play ViaFabricPlus (ViaLegacy) and the vanilla Classic 0.30 client and check they only get
   packets they understand.
 - `staffChat` event, and `createConsoleActor` can take a rank.
+- Minigame arenas are remembered: levels enabled with `/tntwars enable`, `/ctf enable` or `/zombie enable` are
+  loaded at startup, so `/<game> join` works from any level. `/<game> join <level>` loads the arena if needed.
 
 ### Fixed
+- `/restart` under a supervisor (systemd, pm2, a start script loop) started a second copy of the server.
+  With `MCSCRIPT_SUPERVISED=1` set it now just exits and lets the supervisor restart it.
+- Right after a teleport or a level change, movement packets the client had sent from its old position
+  were taken as its new position. That could fire portals, message blocks or parkour checkpoints in the
+  wrong place (walking into a portal could bounce you straight back). Those late packets are now ignored.
 - Several commands skipped rank checks for anything that was not a player (for example "only give ranks
   lower than your own"). They now compare permission levels, so actions from Discord or other relays follow
   the rank rules. The real console is unaffected.

README.md

@@ -108,7 +108,10 @@ journalctl -u mcscript -f     # live log
 ```
 
 `KillSignal=SIGINT` gives the server time to save the levels when the service is stopped. Don't run the
-server as root.
+server as root. When something else restarts the server (systemd, pm2 or a start script with a loop), set the
+environment variable `MCSCRIPT_SUPERVISED=1` (for systemd: `Environment=MCSCRIPT_SUPERVISED=1` under
+`[Service]`). `/restart` then just exits and lets the supervisor start it again, instead of starting a second
+copy itself.
 
 **Any system with pm2:**
 

lib/commands/core.js

@@ -276,6 +276,8 @@ module.exports = function registerCoreCommands (server) {
       server.broadcast(`&e${reason}...`)
       await server.stop(reason)
       if (server.config.exitOnStop === false) return
+      // under systemd, pm2 or a start script loop the supervisor starts us again
+      if (process.env.MCSCRIPT_SUPERVISED) process.exit(0)
       const { spawn } = require('child_process')
       spawn(process.argv[0], process.argv.slice(1), { stdio: 'inherit', cwd: process.cwd(), detached: true }).unref()
       process.exit(0)

lib/player.js

@@ -355,6 +355,7 @@ class Player extends EventEmitter {
 
     if (from && !opts.keepPosition) this.rememberPosition()
     const at = opts.keepPosition ? { ...this.feetPos, yaw: this.yaw, pitch: this.pitch } : level.spawn
+    if (!opts.keepPosition) this._movedByServer()
     this.spawned = false
     this.level = level
     if (this.selection && from !== level) this.cancelSelection()
@@ -569,6 +570,7 @@ class Player extends EventEmitter {
 
   teleport (x, y, z, yaw = this.yaw, pitch = this.pitch) {
     this.rememberPosition()
+    this._movedByServer()
     this.pos = { x: toUnits(x), y: toUnits(y) + EYE, z: toUnits(z) }
     this.yaw = yaw & 255
     this.pitch = pitch & 255
@@ -773,8 +775,32 @@ class Player extends EventEmitter {
 
   // incoming packets
 
+  // The client keeps sending where it was until it gets the teleport (or the new level), so for a
+  // moment packets from the old spot are still arriving. Remember that spot to drop them.
+  _movedByServer () {
+    if (!this.pos || !this.spawned) return
+    const now = Date.now()
+    // two moves in a row (a level change, then a teleport): the client is still at the first spot
+    if (this._staleFrom && now < this._staleFrom.until) this._staleFrom.until = now + 2000
+    else this._staleFrom = { pos: this.pos, until: now + 2000 }
+  }
+
+  _isStale (p) {
+    const stale = this._staleFrom
+    if (!stale) return false
+    const dist = (a, b) => Math.max(Math.abs(a.x - b.x), Math.abs(a.y - b.y), Math.abs(a.z - b.z))
+    const toNew = dist(p, this.pos)
+    if (Date.now() > stale.until || toNew <= 64) {
+      this._staleFrom = null // the client caught up (or it's been long enough)
+      return false
+    }
+    const toOld = dist(p, stale.pos)
+    return toOld <= 128 && toOld < toNew
+  }
+
   _onPosition (p) {
     if (!this.spawned || this.loadingLevel) return
+    if (this._isStale(p)) return
     this.lastActivity = Date.now()
     if (this.supports('HeldBlock')) this.heldBlock = p.heldBlock
 

plugins/minigames/game-command.js

@@ -8,6 +8,18 @@ module.exports = function gameCommand (ctx, { name, aliases = [], title, metaKey
   const rounds = new Map() // level name -> Round
 
   const configured = () => [...server.levels.loaded.values()].filter(l => l.meta[metaKey])
+
+  // Arenas are remembered, so they are loaded at startup and /<game> join finds them from any level
+  const arenaData = ctx.loadData('arenas.json', {})
+  const arenas = new Set(arenaData[metaKey] || [])
+  const saveArenas = () => { arenaData[metaKey] = [...arenas].sort(); ctx.saveData('arenas.json', arenaData) }
+  const loadArena = (levelName) => {
+    const level = server.levels.get(levelName)
+    if (level) return level
+    if (!server.levels.exists(levelName)) return null
+    try { return server.levels.load(levelName) } catch (err) { ctx.log.warn(`Could not load ${title} arena ${levelName}: ${err.message}`); return null }
+  }
+  for (const levelName of arenas) loadArena(levelName)
   const roundFor = (level) => {
     let r = rounds.get(level.name)
     if (!r) { r = createRound(level); rounds.set(level.name, r) }
@@ -36,7 +48,7 @@ module.exports = function gameCommand (ctx, { name, aliases = [], title, metaKey
       switch (sub) {
         case 'join': {
           if (roundOf(player)) throw new CommandError(`You are already playing. Use /${name} leave`)
-          let level = args[1] ? server.levels.get(args[1]) : null
+          let level = args[1] ? loadArena(args[1]) : null
           if (!level) level = player.level.meta[metaKey] ? player.level : configured()[0]
           if (!level || !level.meta[metaKey]) throw new CommandError(`There is no ${title} arena. An operator can create one with /${name} enable`)
           return roundFor(level).join(player)
@@ -59,12 +71,16 @@ module.exports = function gameCommand (ctx, { name, aliases = [], title, metaKey
           needOp()
           player.level.meta[metaKey] = player.level.meta[metaKey] || {}
           player.level.dirty = true
+          arenas.add(player.level.name)
+          saveArenas()
           return player.message(`&a${player.level.name} is now a ${title} arena. Players join with &f/${name} join`)
         case 'disable':
           needOp()
           if (rounds.get(player.level.name)) rounds.get(player.level.name).end(null, 'Arena disabled')
           delete player.level.meta[metaKey]
           player.level.dirty = true
+          arenas.delete(player.level.name)
+          saveArenas()
           return player.message(`&a${player.level.name} is no longer a ${title} arena.`)
         case 'start': {
           needOp()

test/server.test.js

@@ -623,6 +623,8 @@ test('minigames: capture the flag', async (t) => {
   await op.waitForMessage(/has started/)
   const me = server.findPlayerExact('Op')
   const [enemyFlag, ownFlag] = me.data.team === 'red' ? [[40, 40], [10, 10]] : [[10, 10], [40, 40]]
+  // like a real client, first confirm the team spawn the round teleported us to
+  moveTo(op, me.feetPos.x, me.feetPos.y, me.feetPos.z); await sleep(50)
   moveTo(op, enemyFlag[0] + 0.5, 16, enemyFlag[1] + 0.5)
   await op.waitForMessage(/took the/)
   assert.equal(server.levels.main.getBlock(enemyFlag[0], 16, enemyFlag[1]), 0)
@@ -865,3 +867,65 @@ test('plugins can be turned off and back on, and it survives a restart', async (
   assert.ok(current.plugins.get('warps'))
   assert.deepEqual(current.config.disabledPlugins, [])
 })
+
+test('position packets sent before a teleport or level change arrived are ignored', async (t) => {
+  const { server } = await startServer({ owners: ['Runner'] })
+  t.after(() => server.stop())
+  await server.commands.execute(server.console, '/newlvl other 64 32 64 flat')
+  const c = await join(server, 'Runner')
+  const p = server.findPlayerExact('Runner')
+  const units = (x, y, z) => ({ id: -1, x: Math.round(x * 32), y: Math.round(y * 32) + 51, z: Math.round(z * 32), yaw: 0, pitch: 0 })
+
+  // walking around at (10, 17, 10)
+  c.send('position', units(10.5, 17, 10.5))
+  await sleep(100)
+  assert.equal(p.blockPos.x, 10)
+
+  // the server teleports the player, but a packet from the old spot is still on its way
+  p.teleport(40.5, 17, 40.5)
+  c.send('position', units(11, 17, 10.5))
+  await sleep(100)
+  assert.equal(p.blockPos.x, 40, 'the late packet did not pull the player back')
+
+  // once the client moves from the new spot everything is normal again
+  c.send('position', units(41.5, 17, 40.5))
+  c.send('position', units(12.5, 17, 12.5))
+  await sleep(100)
+  assert.equal(p.blockPos.x, 12)
+
+  // same after changing level: old coordinates don't leak into the new level
+  let moves = 0
+  server.events.on('playerMove', () => { moves++ })
+  p.changeLevel(server.levels.get('other'))
+  await c.waitFor(pk => pk.name === 'levelFinalize')
+  c.send('position', units(12.5, 17, 13))
+  await sleep(100)
+  assert.equal(moves, 0)
+  const spawn = server.levels.get('other').spawn
+  assert.equal(p.blockPos.x, Math.floor(spawn.x))
+
+  // a level change followed by a teleport (what /warp does): normal moves near the new spot still count
+  p.changeLevel(server.levels.main)
+  p.teleport(30.5, 17, 30.5)
+  await c.waitFor(pk => pk.name === 'levelFinalize')
+  c.send('position', units(34.5, 17, 30.5))
+  await sleep(100)
+  assert.equal(p.blockPos.x, 34)
+})
+
+test('minigame arenas are remembered and loaded at startup', async (t) => {
+  const first = await startServer({ owners: ['Op'] })
+  await first.server.commands.execute(first.server.console, '/newlvl arena 64 32 64 flat')
+  const op = await join(first.server, 'Op')
+  await command(op, '/goto arena', 400)
+  assert.match(await command(op, '/zombie enable'), /now a Zombie Survival arena/)
+  op.close()
+  await first.server.stop()
+
+  const { server } = await startServer({}, first.root)
+  t.after(() => server.stop())
+  assert.ok(server.levels.get('arena'), 'the arena is loaded without anyone visiting it')
+  const guest = await join(server, 'Guest')
+  assert.doesNotMatch(await command(guest, '/zombie join', 400), /There is no/)
+  assert.equal(server.findPlayerExact('Guest').level.name, 'arena')
+})