Confirm creative slot sets with a stats round trip instead of a fixed 400ms wait - #4031
Conversation
|
Pushed a second commit: making slot sets fast exposed a latent one-tick race in |
|
Third commit: the strict reset check caught a second latent leak, this one a client/server desync. The sign test fired |
|
CI red was mine: the sign fix waited for an editor-reopen ack that only exists on 1.20+ (editable signs) — every group containing a pre-1.20 version timed out on it, which is also exactly why 1.21.3–1.21.6 was the one green group. Amended: the test just awaits the interact, and the reset now self-heals any client/server block desync (stale past 1.5s → two forced /setblock changes make the server rebroadcast, whichever side is wrong). Verified this round on 1.12.2, 1.19.4, and 26.1×2 including the desync-exercising sequence. The 1.21.8-group failure was an unrelated one-off login timeout (three of its four versions fully passed); the digAndBuild pickup race seen once locally is pre-existing and fixed in #4034. |
fc08c64 to
4e7f371
Compare
| // statistics packet settles the oldest waiter, and everything written | ||
| // before that waiter's request is processed by then. | ||
| const statsConfirms = [] | ||
| bot._client.on('statistics', () => statsConfirms.shift()?.()) |
There was a problem hiding this comment.
statsConfirms.shift()?.()
What does this do?
There was a problem hiding this comment.
Agreed let's do something less weird here
There was a problem hiding this comment.
I rewrote it, should make more sense now
4e7f371 to
0fe1fcc
Compare
… wait On noAckOnCreateSetSlotPacket versions (1.21.9+) setInventorySlot resolved success by sleeping a flat 400ms and treating silence as acceptance. That wait is pure latency on every successful call: the 26.1 test suite alone spends 24.8s in it (62 calls), and any creative bot pays it per slot write. A rejection is a set_slot correction the server sends while processing our packet on its main thread, in receive order. A client_command request_stats round trip on the same connection is therefore proof the rejection window has passed: the statistics reply is generated after our set_creative_slot was processed, and any correction was already delivered before it. Statistics replies carry no transaction id, so in-flight confirms resolve FIFO, which matches the server answering requests in packet order. The old waitTimeout now caps how long we wait for the reply before assuming success, preserving the previous worst-case behavior on a server that never answers. Measured on 26.1: ping_request is answered on the netty thread in 0-2ms before earlier queued packets (10/10 runs), so it proves nothing about processing; request_stats consistently arrives after a just-sent chat echo (10/10) in 7-53ms. The creative test drops from 6.5s to 1.4s.
The marker echo after resetBlocksToSuperflat's fills only proves the commands executed: the server sends command feedback immediately but flushes block changes at tick end, so the client can still hold pre-fill blocks when the next test starts. The fixed 400ms setInventorySlot waits at the top of most tests were absorbing that tick; with server-confirmed slot sets the race surfaces as tests reading stale blocks (placeBlock seeing a previous test's dirt). After the echo, wait until every block in the reset region matches its superflat layer, driven by blockUpdate/chunkColumnLoad events (past 64 changed blocks per section the correction arrives as a chunk resend, not block updates), with a hard 5s deadline that fails loudly naming the stale coordinate.
…syncs The sign test fired bot.activateBlock unawaited from a setTimeout and resolved immediately, so the use_item_on could land in the same server tick as the next reset's fills. On 1.20+ (editable signs) destroying a sign in the tick of its own editor interact makes the editor's out-of-band sign re-broadcast race the fill's air change: the client keeps the sign, the server has air, and no correction ever comes - packet traces show the sign re-asserted after the fills with no air update for that position. The strict reset check then fails the next test with 'world not reset ... warped_sign'. Await the interact, and teach the reset to recover from any such client/server desync: when a block stays stale past 1.5s, two forced /setblock changes (bedrock, then the expected block) make the server rebroadcast it regardless of which side is wrong. Waiting for an editor-reopen ack instead is not an option - only 1.20+ reopens the editor on interact, so that wait times out on every older version.
0fe1fcc to
fcc0ead
Compare
|
Tests more than 1.5x slower than master (durations are noisy, so this is informational): |
3 participants
Problem
On
noAckOnCreateSetSlotPacketversions (1.21.9+),setInventorySlotresolves success by sleeping a flat 400ms and treating silence as acceptance. Every successful call pays the full wait. Measured on the 26.1 external suite with a counter: 62 calls × 400ms = 24.8s per run, ~19% of the suite's wall time — and every creative-mode bot pays the same per slot write.Change
A rejection is a
set_slotcorrection the server sends while processing our packet on its main thread, in receive order. So after writingset_creative_slot, sendclient_command(request_stats)and wait for thestatisticsreply: it is generated after our packet was processed, and any correction was already delivered before it on the same ordered connection. Success now resolves in ~1 tick instead of 400ms.waitTimeout(400ms) now caps the wait for the reply: a server that never answers stats degrades to exactly the old behavior, never a hang.request_statsis sent as{ payload: 1 }onrespawnIsPayloadversions,{ actionId: 1 }otherwise (same gate as health.js respawn).Primitive choice was verified empirically against a vanilla 26.1 server: play-state
ping_requestis answered on the netty thread in 0–2ms, before earlier queued packets (10/10 runs — it proves nothing about processing), while astatisticsreply consistently arrived after a just-sent chat echo (10/10 runs, 7–53ms), confirming main-thread FIFO handling.Tests
Added an assertion to the creative external test (gated on
noAckOnCreateSetSlotPacket): three sequential slot sets must finish under 1s. It fails on master's lib at exactly 1204ms (3×400ms) and passes with this change (~100ms). The creative test drops from ~6.5s to ~1.4s overall; the 26.1 suite loses ~25s.Ran the creative external test locally on 1.12.2, 1.19.4 and 26.1 — all passing. Old versions are unaffected: the acked path is untouched and
confirmServerProcessedis only called from the no-ack branch.