Repository navigation
Conversation
|
No tests are more than 2x slower than master anymore. |
…tead of throwing
next() does the version check and runs inside the client's connect_allowed
listener when the version is auto-detected, or synchronously from createBot()
when one is passed in. The throws there were never caught, so a server outside
mineflayer's supported range escaped as an uncaught exception and took down the
host process, while bot.on('error') - the way every other mineflayer failure is
reported - never fired.
Wrap next() in try/catch and report the failure with bot.emit('error', err)
followed by bot.end(...), on the next turn so createBot() has returned and the
caller has had a chance to attach its listener. The connection is dropped so the
bot does not sit half-initialised with plugins never injected.
Internal test: a 1.7.10 server, older than oldestSupportedVersion, asserted to
surface through the error event and end the connection rather than throw out of
createBot(). It fails on master with the throw from the issue.
Fixes PrismarineJS#4038
aa58f4d to
1bda3da
Compare
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
The deferred error path lets callers attach a bot error listener before an unsupported explicitly selected version is reported, and terminates the partial connection. The new regression needs two harness repairs, detailed in the existing inline comment: it is excluded by every CI version filter, and its teardown checks a nonexistent server.listening property instead of closing the NMP server.
I executed the focused test with minecraft-protocol 1.68.0 and minecraft-data 3.117.0. Its assertion passes, but the ordinary Mocha process remains alive until a 180-second process timeout. A teardown-only control replacing the guard with server.close() passes and exits normally. Please fix cleanup and CI selection before relying on the green matrix to protect this behavior.
Skills used: prismarine-review checked runner discovery and actual fixture execution; prismarine-lifecycle-action-review traced creation, deferred error notification and connection/server cleanup.
|
|
||
| // The loop above only ever starts servers mineflayer supports, so the path a | ||
| // bot takes when the server it reaches is out of range needs its own block. | ||
| describe('loader: unsupported server version', function () { |
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
This standalone suite has no version label, while every MinecraftServer job runs Mocha with -g "${v}v". Consequently the new unsupported-version regression is excluded from all those jobs; the green CI does not protect the behavior added here. This case deliberately needs an unsupported server, so it need not be forced into each supported-version fixture: give this standalone test an explicit once-per-run CI invocation (or an established selected suite), retaining its real NMP server. Please ensure that invocation is part of the normal test workflow.
Skill used: prismarine-review checked actual suite titles against the candidate workflow, while prismarine-lifecycle-action-review confirmed the unsupported-version fixture exercises a distinct initialization path.
There is also a teardown issue in this newly added suite: server is the minecraft-protocol Server wrapper, whose listening state is on server.socketServer, so if (server.listening) server.close() never closes it. A normal focused Mocha run passed its assertion but remained alive until my 180-second process timeout. With only that guard replaced by server.close(), the same test passed and the process exited normally. Please close the server through the established fixture teardown (and await closure if needed); otherwise enabling this previously skipped suite in CI leaves an open listening socket. The lifecycle skill's cleanup check exposed this independently of the discovery issue.
…operly
The MinecraftServer jobs run Mocha with -g "${v}v", so the standalone
suite had no invocation and the green matrix did not cover it; a new
Standalone job runs it once per workflow run.
minecraft-protocol's Server.close() takes no callback and never fires
server.listening, so the old guard left the socket open and the mocha
process alive. Await the server's close event instead. Both verified
locally: the suite passes and the process exits.
|
Fixed both:
Verified locally: suite passes, process exits cleanly. |
Summary
Fixes #4038.
Connecting to a server outside mineflayer's supported range killed the whole Node
process instead of firing
bot.on('error').next()inlib/loader.jsdoes the version check, and it runs inside the client'sconnect_allowedlistener when the version is auto-detected, or synchronouslyinside
createBot()when a version is passed in. Thethrows there were nevercaught, so they escaped as uncaught exceptions.
Changes
next()intry/catch.bot.emit('error', err)followed bybot.end(...), on the next turnof the event loop. Deferring means
createBot()returns normally and a caller whoattaches
bot.on('error')straight afterwards still gets the event.injected.
The version checks, their messages and the
inject_allowedpath are unchanged.Verification
New test
loader: unsupported server versionstarts a 1.7.10 server (older thanoldestSupportedVersion= 1.8.8) and asserts thatcreateBot()returns, that theerrorevent carries the "Oldest supported version" message, and that theconnection is then ended.
Without the fix the test fails with exactly the error from the issue:
Error: Server version '1.7.10' is not supported. Oldest supported version is '1.8.8'. at Object.createBot.standardandstandard-markdownboth pass.Local runs, one version per process:
Both single failures are
packet before pong is tick_end, the pre-existing Send tick_end at the end of every client tick on 1.21.2+ #4137regression that fails on upstream master too and is fixed by Allow tick_end between the movement packet and the pong in the tick-boundary test #4162. No new
failures on any version.