feat(instance): a shared instance that repairs what it inherits - #40
Merged
Conversation
Contributor
Test results 285 files 285 suites 8m 57s ⏱️ Results for commit 58523a4. ♻️ This comment has been updated with latest results. |
TheMeinerLP
marked this pull request as ready for review
August 3, 2026 14:47
TheMeinerLP
force-pushed
the
feat/shared-instance
branch
from
August 3, 2026 16:18
032dd1d to
139936f
Compare
This comment has been minimized.
This comment has been minimized.
…rits Eight tasks. FalcoSharedInstance extends SharedInstance, because areLinked compares getInstanceContainer() rather than testing a class, so a subclass keeps the fast path that avoids a 765 ms resend. It then replaces the four delegating methods that alias the container. Two corrections to the spec came out of writing it, both verified in the pinned sources. UNSAFE_setBlock is called from four places, not five - the fifth was its own declaration, counted by a grep. And NFR-006 contradicted the non-goals: it demanded a chunk lock instead of an instance monitor without qualification, while section 3 lists removing that monitor from a shared container as out of scope. It now binds the instances this project implements, which is the only reading under which stage 4 is buildable. The plan also avoids a test that would have proven nothing. Counting ChunkDataPacket on an instance switch reads zero on both paths, because resetChunkQueue does not reset chunkBatchLead and a TestConnection never sends the acknowledgement that would release the next batch. It asserts UpdateViewPositionPacket and UnloadChunkPacket instead, with an unlinked control so that zero cannot mean "attached to nothing". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It stores per instance and returns what was stored, which repairs the aliasing. It does not make the view generate - the container does that, and asks its own generator. Both halves belong in the javadoc, because a setter that silently has no effect is the trap, and saying so is the difference between a documented limitation and a defect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…till recognises Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… traffic Task 1 proved areLinked answers true; that is the mechanism, not the outcome. This asserts what US-4.01 asks for on the wire, so a change to Player#setInstance is caught here instead of costing a full resend per transfer in production. The markers are UpdateViewPositionPacket and UnloadChunkPacket, both sent unconditionally by the slow path. ChunkDataPacket is asserted too but carries no weight alone: after the first spawn the chunk queue waits for a batch acknowledgement a test connection never sends. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The class doc claimed ChunkDataPacket reads zero on both paths and carries no weight, so the unlinked control never tracked it. That claim was wrong, and it talked the strongest assertion in the file down to decoration. Cyano installs TestPlayerImpl as the player provider of every test connection, and it overrides Player#sendChunk(Chunk) to send the full data packet at once instead of queueing it. Player#chunkAdder dispatches virtually, so the chunkBatchLead/maxChunkBatchLead gate the doc relied on is never reached. Measured: the slow path sends 25 chunk data packets, one per chunk of the 5x5 view of viewDistance(1). The control now tracks them and asserts that 25, which is what gives chunks.assertEmpty() on the fast path its meaning. Correcting the earlier commit message of eaa354c, which repeated the same wrong claim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… the container SharedInstance forwards setGenerator to its container, so configuring one view reconfigures the world and every other view of it, and clearing one view empties the world for everybody. FalcoSharedInstance now stores the generator per instance, seeded once from the container. The javadoc of both accessors states the limit the repair creates: the stored value has no reader, because chunks are created by the container and the container asks its own generator. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… into the container A shared instance forwards setChunkSupplier to its container, so configuring one view decides what type of chunk the whole world is made of and reconfigures every sibling view along with it. The value is now stored per instance, seeded from the container once at construction, and null is refused rather than stored. The stored supplier has no reader: chunks are created by the container, which asks its own supplier, and a chunk loader is handed the container rather than the view. Both accessors say so, because a setter that is silently inert is the trap this repair would otherwise introduce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Minestom's SharedInstance forwards enableAutoChunkLoad to its container, so configuring one view reconfigured the world and every sibling view of it. The flag now lives on the view, seeded once from the container. Unlike the generator and the chunk supplier this value is not inert: the view overrides loadOptionalChunk and consults its own flag there, which is the method Player#chunkAdder calls, so a player of this view observes the setting. A write through setBlock still reaches the container and still asks the container's flag - both halves are stated in the Javadoc of the setter and the getter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ontainer Moving the auto chunk load flag onto the view means a view whose flag is on loads a chunk where the container's own flag is off. That is a deliberate act - a fresh view is seeded from the container, so someone has to turn it back on at the view - but it was neither stated nor pinned. The Javadoc of loadOptionalChunk now states it and a test holds it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Javadoc claimed Player#chunkAdder was the only observer of the flag and that nothing else saw the effect. Four further Minestom call sites route through loadOptionalChunk, and two of them do not survive the null it hands back: chunkAdder itself parks a NullPointerException in an unobserved future because sendChunk dereferences its argument at once, and Entity#setInstance refuses to register or spawn the entity at all. That second one is a per-view failure mode stock SharedInstance could only produce world-wide, so it is exactly the consequence the reader needs told. All five call sites are now enumerated on loadOptionalChunk, and a test pins the entity case with two views over one container: the view with the flag off never registers the entity, the sibling does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ntainer's Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tance save Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…'s data saveInstance() on a view hands the loader this instance, so the container is no longer saved by that call, and a view constructed without tags hands over an empty compound which an AnvilLoader drops without touching the file. Both halves are now on the method, with the redirect to getInstanceContainer().saveInstance() that every other override here carries. A test pins the empty compound so the claim cannot rot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… monitor The block owner is an InstanceContainer, its UNSAFE_setBlock is private synchronized, and it is reached from four places of which setBlock is only one. Overriding setBlock would take over one and leave the other three on the private path, so the limitation is documented instead of worked around. FalcoSharedInstanceWriteTest pins the two observable consequences: the chunk is the container's object for every view, and the per-view auto chunk load flag does not reach the write path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Test counts per module against the stage 2 result with the divergence accounted for, the mutation injected for each of the seven tasks and what it turned red, the packet count that may be quoted, and the four things this stage did not reach. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…iner The plan's wording put tags next to the three setters as a fourth value the stock shared instance aliases. It is not one: Instance holds a TagHandler per instance (Instance.java:127) and SharedInstance does not override tagHandler(). What was broken is that saveInstance() handed the loader the container, so the view's own tags were never written - a different defect with a different repair. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ments The load-bearing sentence of US-4.04 is a claim about Minestom's bytecode: UNSAFE_setBlock is private synchronized and is reached from four places, so a shared world pays the container's instance monitor per block and an override of setBlock would leave three routes behind. FalcoSharedInstanceWriteTest observes blocks and chunks, so all three of its cases stay green through every way that claim could stop being true - including an override of setBlock, which was measured rather than assumed. ForeignWritePathTest reads the bytecode instead: the two modifiers, the caller set as an exact set, the forward from SharedInstance to the container, and that FalcoSharedInstance overrides none of the three write entry points. Each rule was proved to bite on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The stage 4 result reported the tree at e9c5cce, before Task 7's review follow-up added ForeignWritePathTest. Its test table therefore recorded 42 archunit rules while the module runs 46, and its build verdict came from an up-to-date check rather than from javadoc having run. Re-run in full at 9271642: six modules green, 182 / 205 / 217 / 166 / 42 / 46, one skip that predates this stage, and `build -x test --rerun-tasks` green with javadoc genuinely executed and silent. Two mutations re-injected by a session that did not write them - setGenerator delegating to super, and a super-only setBlock override - both bit exactly where the table claims. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The result said stage 4 touched exactly one module and named a commit range as the check. Neither survives 9271642: ForeignWritePathTest lives in falco-archunit, and the range starts at a mid-branch merge, so it also misses the two test classes tasks 1 and 2 committed before it. Ten files, named, with a check that works per commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n sum The paragraph the previous self-review wrote to correct this claim said "ten files" and then enumerated nine: six in falco-instance with "four test classes", when the union of `git show --name-only` over the stage's own commits holds seven there, five of them test classes. The two it dropped are FalcoSharedInstanceTest and FalcoSharedInstanceResendTest -- the same pair the same paragraph names as the ones a range check loses, and the same five classes it counts by test case two sentences later (7+3+12+5+3 = 30). The files are now named one by one and the sum is written out, so the count can be checked against the list instead of believed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… exists loadOptionalChunk claimed a divergence between a view's auto-load flag and its container's took "a deliberate act" - the view being turned back on after the container refused. That is only true when the container's flag is off before the view is constructed, which is the ordering every test used. The runtime ordering is the other one: the flag is a snapshot taken in the constructor and the load is delegated to InstanceContainer#loadChunk, which that flag never governed, so a container that turns auto load off after its views exist stops none of them and each of them pulls chunks into the container that refused to load one. Stock SharedInstance asked the container on every call, so its off switch was authoritative for every view at once. The paragraph now says that, enableAutoChunkLoad names the same ordering, and three cases pin what three Javadoc paragraphs assert about the snapshot - generator, chunk supplier and the auto-load flag changed on the container after the view exists, the last one carried as far as the chunk landing in the container. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
createSharedInstance refuses an unregistered container with a state check; registerSharedInstance, the only route that can register this type, does not. The class documents that route change and inherited none of its checking, and what it lets through is quiet: an unregistered container is in no InstanceManager, so ServerProcess never ticks it, so InstanceContainer#tick never clears currentlyChangingBlocks - every repeat write of the same block value at a position stays suppressed and the map grows without bound, while the view ticks normally and looks healthy. The constructor now performs the check the abandoned route performed, and the class documentation says why it has to. The new case asserts both halves of the asymmetry, not only the throw: createSharedInstance still refuses the same container and registerSharedInstance still takes a view over it. The README picks up the ordering, and the auto-load sentence the previous commit left it owing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The method states both halves of everything else it repairs and stated only one half of the failure path. InstanceContainer#optionalAsync hands a parallel failure to the ExceptionManager as well as to the future and throws a synchronous one at the call site; this override does neither - it completes the returned future exceptionally on both branches and tells nobody else. That was pinned by testAFailureIsReturnedOnceOnBothBranches and described in the test class alone, which is not documentation for a consumer. A paragraph now names both deviations and what they cost: firing the call without observing the future is a save that failed in silence where stock would have logged it, and a try/catch around it catches nothing because the synchronous branch no longer throws either. The @return tag says the future is the only report. FalcoInstance#runSave makes the same choice, and the paragraph says so, so a code base using both is told once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The result described the tree at 9271642 and three of its statements stopped being true there: falco-instance runs 186 tests rather than 182, US-4.02 is carried by three cases more than it names, and the mutation table stops one review short. It now reports the falco-instance row from the third run at 3cf4771, says which run each column comes from, counts the stage's own 34 cases, and carries a section for the three findings of the final review with the commit that closed each. The ten-file list is unchanged and says so - the review touched five of the ten and added none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TheMeinerLP
force-pushed
the
feat/shared-instance
branch
from
August 3, 2026 17:38
139936f to
58523a4
Compare
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
1 participant
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.
Stage 4 of four. Stacked on #39 — the base is
feat/block-storage, notmain,so this diff shows only what stage 4 added. Against
mainit would repeat all of#39 a second time.
Complete: all eight tasks, acceptance recorded under
## Stage 4 resultin the plan.A final whole-branch review is running.
Why a subclass and not a replacement
SharedInstanceforwardssetGenerator,setChunkSupplier,enableAutoChunkLoadand
saveInstanceto its container. Configuring one view therefore reconfigures theworld and every other view of it, and a view's save writes the container's tags
instead of its own.
The obvious repair — a
BlockStoreboth instances hold, no instance pointing atanother — was measured and rejected: it loses
areLinked, and a full chunk resendat view distance 10 costs 765 ms and 86.5 MB while the fast path costs nothing.
areLinkedturned out not to test for a class at all; it comparesgetInstanceContainer(). Nothing inSharedInstanceisfinal. A subclass thereforekeeps the fast path and may still replace every delegating method — which is what this
does.
What each repair does and does not do
Every one states both halves in its javadoc, because a setter that is silently
without effect is a trap rather than a feature:
setGeneratorkeeps per-view statesetChunkSupplierlikewisenullis refused rather than storedenableAutoChunkLoadlikewiseloadOptionalChunksaveInstancewrites this view's tagslevel.datper world: container and views still overwrite one another, last save winsThe wall, documented and guarded
The block owner must be an
InstanceContainer, and its monitor cannot be removed.UNSAFE_setBlockisprivate synchronizedand reached from four places; overridingsetBlockbypasses one and leaves three on the private path — two write paths overthe same data, one synchronised and one not.
US-4.04asks for that to be written down. It is, andForeignWritePathTestinfalco-archunitmakes the claim fail if Minestom ever stops carrying it — thesentence had been documented in four places and guarded nowhere.
Not achieved, and named as such
InstanceContainer,which this stage requires, and
EntityTrackerissealed.saveInstancecannot be made separable — onelevel.datper world.Test count
:falco-instance:182,:falco-light:205,:falco-anvil:217,:falco-demo:166,:falco-benchmarks:42,:falco-archunit:46 — green, none skipped. Of the 39 addedto
falco-instance, 30 belong to this stage; the rest arrived with two mid-stage merges.No timing figure here is citable — the acceptance run measured a load average of
33.75 before the suite and 44.81 after it.
🤖 Generated with Claude Code
https://claude.ai/code/session_01NGpJqdmh7ZNH487GLqPJmK