Player attachments not syncing to client properly on respawn

[bejker123]

While developing a mod using the v1 attachments API I've encountered a bug.
Where the server after respawing a player doesn't properly sync attachments to the client.
I believe it only affects client side, generating a stack trace for each attachment, linked below.
I suspect that the packet is sent during the wrong client phase.
I'm happy to provide more relevant information if needed.

Below I include the versions I use, logs, and a workaround I made, that doesn't address the root of the problem, and is only a band-aid solution.

Versions:
Minecraft - 1.21.4
Yarn - 1.21.4+build.8
Fabric Loader - 0.16.10
Fabric API - 0.118.0+1.21.4

A very basic workaround:
I set a constant delay of 100ms which fixes the issue
https://gist.github.com/bejker123/ff2daeeef96fe824d24e4c87b6a2004e

Relevant Logs:
https://gist.github.com/bejker123/a38d08dd622ff4fafd0076616696563d

[bejker123]

After doing some digging I believe the ClientPlayNetworkHandlerMixin.copyAttachmentsOnClientRespawn should mixin'ed after the clientPlayerEntity2.setId(...) call. My assumption is that that causes the issue. Because when setting a break point and inspecting the AttachmentChange.apply function I was able to see that targetInfo.getTarget(...) returns null. This implies that the network id for the client player entity is mismatched with the server.

[...]
               //onPlayerRespawn snippet
		this.startWorldLoading(clientPlayerEntity2, this.world, worldEntryReason);
		clientPlayerEntity2.setId(clientPlayerEntity.getId());
		this.client.player = clientPlayerEntity2; // I suggest mixing in into here
		if (bl) {
			this.client.getMusicTracker().stop();
		}
[...]

[Syst3ms]

I had a look at that code but I think it is? The mixin targets that very field set already: see here.

A minimal code example to reproduce the issue would help.

[bejker123]

After giving it a second look it seems like it should work, but doesn't.
I think my IDE pointed me to the wrong line.
Here is the minimal code example:
https://github.com/bejker123/attch-bug-repro/
It still produces the same logs, with the most basic setup.
I still think the target should be moved to a later line.

Edit 3:
I finally understand the issue. Transferring attachments happened before the player is completely respawned on the client. After changing the transfer to be triggered on ServerPlayerEvents.COPY_FROM to ServerPlayerEvents.AFTER_RESPAWN the stack trace is no longer present.

package net.fabricmc.fabric.impl.attachment;
[...]
public class AttachmentEntrypoint implements ModInitializer {
        [...]
	@Override
	public void onInitialize() {
                [...]
                // Current
 		ServerPlayerEvents.COPY_FROM.register((oldPlayer, newPlayer, alive) ->
				AttachmentTargetImpl.transfer(oldPlayer, newPlayer, !alive)
		);
                // Suggested change
		ServerPlayerEvents.AFTER_RESPAWN.register((oldPlayer, newPlayer, alive) ->
				AttachmentTargetImpl.transfer(oldPlayer, newPlayer, !alive)
		);
               [...]
       }

Edit 2:
I've tried the change suggested above and it didn't help. At this point my only idea is to add a check to AttachmentChange.apply, if target is null. This wouldn't fix the underlying issue but at least wouldn't produce the stack traces.

Edit:
I'm pretty sure it should be moved after this.world.addEntity(...) call. Since AttachmentChange accesses the entity from the world.

[bejker123]

I was clearly wrong as the problem had nothing to do with the mixin I mentioned.
(See comment above)
I'm sorry for the confusion.
Could that change be implemented?
I tried understanding the code structure better, but it's possible that I'm missing something.