Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix server sync reading and wrong disconnection from API Server #3304

Merged
merged 2 commits into from
Jul 30, 2024

Conversation

guilherme-gm
Copy link
Member

@guilherme-gm guilherme-gm commented Jul 1, 2024

Pull Request Prelude

Changes Proposed

This fixes how packet 0x272D (CharServer -> LoginServer online accounts sync) is read by Login Server. Thus, preventing accounts from being wrongly marked as offline.

When login-server marks an account as offline, it will eventually tell API Server to disconnect them, thus also invalidating users' API Tokens while they are in game.

Huge thanks to AcidMarco in Herc's discord that provided a quite good explanation of what he has observed and this helped me a lot in finding the root cause (or at least I think it did...)

Issues addressed:
None, I think.

… and login-server

Char-Server writes the account ids starting at offset 8, while login-server was reading from offset 6
This caused account ids to be messed up when syncing both servers,
thus, many players (if not all) would be considered as logged off by login-server at this point
and this would them invalidate API Server token
src/login/login.c Outdated Show resolved Hide resolved
@dastgirp
Copy link
Member

dastgirp commented Jul 1, 2024

Probably unrelated to this PR, but there's one more issue wherein API server is called for autotraders on restart, and we get bunch of Account not online errors

@guilherme-gm guilherme-gm marked this pull request as ready for review July 28, 2024 15:46
@guilherme-gm
Copy link
Member Author

Probably unrelated to this PR, but there's one more issue wherein API server is called for autotraders on restart, and we get bunch of Account not online errors

Yeah, I think this is a separate issue. I will take a look into it and open a new PR if I find something (maybe we should create an issue about it?)

@MishimaHaruna MishimaHaruna merged commit 216c752 into HerculesWS:master Jul 30, 2024
338 of 346 checks passed
@MishimaHaruna MishimaHaruna added this to the Release v2024.07 milestone Jul 30, 2024
@guilherme-gm guilherme-gm deleted the fix-account-sync branch July 31, 2024 00:16
@vBrenth
Copy link

vBrenth commented Aug 9, 2024

Hello, I applied this on my server and it fixes some of recent problem but after few hours of server being online the character-login gets disconnected.

I don't know how it happen or reproduce its just gets disconnected. Then i had to restart the Login and Character server to be able to login again.

image

@dastgirp , @guilherme-gm any updates?

@guilherme-gm
Copy link
Member Author

@vBrenth can you try using the first commit only? this one: 29d858c

I am wondering if I may have made a mistake when converting to struct.

@vBrenth
Copy link

vBrenth commented Aug 12, 2024

@guilherme-gm I've been running the first commit only and its still having some saving problem but less and no disconnection between char and login so far.

@guilherme-gm
Copy link
Member Author

@guilherme-gm I've been running the first commit only and its still having some saving problem but less and no disconnection between char and login so far.

Thanks for the update, I made a fix on #3312 so the 2nd commit properly works.

Are the saving problems giving any logs? is there any reproduction steps for it? I think we need to figure out whether this is a server issue or a client one, since I noticed that there are some cases where the client doesn't send the changes to be saved

@vBrenth
Copy link

vBrenth commented Aug 18, 2024

I cant find a way to reproduce it. Its just happen after some time with players are online.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants