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

Broken redirects prevent proper manifest creation #16

Closed
xKevin04 opened this issue Jan 1, 2022 · 9 comments
Closed

Broken redirects prevent proper manifest creation #16

xKevin04 opened this issue Jan 1, 2022 · 9 comments
Labels
bug Something isn't working

Comments

@xKevin04
Copy link

xKevin04 commented Jan 1, 2022

I can't generate a fresh manifest at the moment, due to a temporary error on GOG's side. When running gogcli.exe manifest generate --lang=english --lang=german --os=windows, I end up with the following manifest.json (this is the entire content of the generated file):

{
  "Errors": [
    "GetDownloadFileInfo(downloadPath=/downloads/imperator_rome/en1patch0) -> Expected response status code of 302, but got 404"
  ]
}

Seems like it aborts after encountering the first problem of this kind, broken HTTP redirects handled by retrieveUrlRedirectLocation() from what I see. It happens for various of the Imperator: Rome files when retrying, the download links for them are pretty unstable right now (also confirmed via manual browser download). No other files are created by gogcli, only this unusable manifest.

I tried running it with debug-level logging, but nothing of interest is logged beyond starting a HTTP GET request. Output stops after the batch of requests containing the broken one was logged and then the program terminates with exitcode 1. Here are the last 10 lines of the log (got lucky the culprit was actually in the very last line, usually it's somewhere else in the batch):

[sdk] GetDownloadFileInfo(downloadPath=/downloads/simulacra_2/en1installer0) -> GET https://www.gog.com/downloads/simulacra_2/en1installer0
[sdk] GetDownloadFileInfo(downloadPath=/downloads/geneforge_3/en1installer0) -> GET https://www.gog.com/downloads/geneforge_3/en1installer0
[sdk] GetDownloadFileInfo(downloadPath=/downloads/cyberpunk_2077_game/en1installer6) -> GET https://www.gog.com/downloads/cyberpunk_2077_game/en1installer6
[sdk] GetDownloadFileInfo(downloadPath=/downloads/pathologic_2/en1patch2) -> GET https://www.gog.com/downloads/pathologic_2/en1patch2
[sdk] GetDownloadFileInfo(downloadPath=/downloads/rebel_galaxy_outlaw/en1installer2) -> GET https://www.gog.com/downloads/rebel_galaxy_outlaw/en1installer2
[sdk] GetDownloadFileInfo(downloadPath=/downloads/not_for_broadcast/en1patch1) -> GET https://www.gog.com/downloads/not_for_broadcast/en1patch1
[sdk] GetDownloadFileInfo(downloadPath=/downloads/layers_of_fear/en1installer1) -> GET https://www.gog.com/downloads/layers_of_fear/en1installer1
[sdk] GetDownloadFileInfo(downloadPath=/downloads/my_time_at_portia/en1patch1) -> GET https://www.gog.com/downloads/my_time_at_portia/en1patch1
[sdk] GetDownloadFileInfo(downloadPath=/downloads/finding_paradise/en1patch2) -> GET https://www.gog.com/downloads/finding_paradise/en1patch2
[sdk] GetDownloadFileInfo(downloadPath=/downloads/imperator_rome/en1patch0) -> GET https://www.gog.com/downloads/imperator_rome/en1patch0

I would have expected the program to create a working manifest and list the error alongside other problems in manifest-warnings.json. The option --tolerate-dangles is implicitly set to true, after all. I've also tried setting it explicitly, just to be absolutely sure.

@Magnitus-
Copy link
Owner

Magnitus- commented Jan 10, 2022

@xKevin04
Apologies for not noticing your issue sooner. Thank you for your input.

I think it will just be a matter of adding the 302 code to the list here: https://github.com/Magnitus-/gogcli/blob/main/sdk/download.go#L142

I've added non-200 codes as I encountered them. I guess I should just add a blanket range for the 3xx codes.

It should be a quick fix. I'll look into it tonight.

@Magnitus-
Copy link
Owner

Your issue should be fixed in this commit: 8b36536

The fix can be found in the 0.17.1 release.

@Magnitus- Magnitus- self-assigned this Jan 11, 2022
@Magnitus- Magnitus- added the bug Something isn't working label Jan 11, 2022
@Magnitus- Magnitus- removed their assignment Jan 11, 2022
@xKevin04
Copy link
Author

xKevin04 commented Jan 11, 2022

@Magnitus-
Thank you, and no worries about the response time. I'm glad you're developing and maintaining the tool at all.

Unfortuntely, I'm afraid the patch didn't fix this issue. I've tried to create a complete manifest twice which failed as before. Here's the manifest when running gogcli.exe manifest generate --title="Imperator: Rome" with v0.17.1:

{
  "Errors": [
    "GetDownloadFileInfo(downloadPath=/downloads/imperator_rome/en1patch1) -> Expected response status code of 302, but got 404",
    "GetDownloadFileInfo(downloadPath=/downloads/imperator_rome/en2installer0) -> Expected response status code of 302, but got 404",
    "GetDownloadFileInfo(downloadPath=/downloads/imperator_rome/en1patch3) -> Expected response status code of 302, but got 404"
  ]
}

@Magnitus-
Copy link
Owner

Magnitus- commented Jan 11, 2022

Ok, wow, that means they are returning something unexpected here now: https://github.com/Magnitus-/gogcli/blob/main/sdk/download.go#L118

That's a completely new misstep I haven't yet seen from GOG in their little download dance. Slightly more involved fix then, but still doable.

I'll take a moment to ponder on this tonight and take another crack at it.

@Magnitus- Magnitus- reopened this Jan 11, 2022
@Magnitus-
Copy link
Owner

Magnitus- commented Jan 12, 2022

@xKevin04

Ok, I think this commit should do the trick: b649ed4

It is in the 0.17.2 release.

@xKevin04
Copy link
Author

xKevin04 commented Jan 12, 2022

@Magnitus-
Great, this has solved the issue. Faulty redirects are now logged in manifest-warnings.json as I would expect. Thanks!

Just for completion sake, here are some other broken manifests that were generated when retrying. However, those all resulted from hard server side errors where requests couldn't be fulfilled, so it's ok to abort in my book. Just in case you want to add output to stderr (nothing on the console seems like everything went fine at first glance) or handle it in some way. Feel free to ignore either way, it's not something I feel is worth making an issue out of.

{
  "Errors": [
    "GetDownloadFileInfo(downloadPath=/downloads/ticket_to_ride/en1installer0) -> retrieval request error: Get \"https://www.gog.com/downloads/ticket_to_ride/en1installer0\": read tcp 192.168.0.4:57797->88.221.61.153:443: wsarecv: An existing connection was forcibly closed by the remote host.",
    "GetDownloadFileInfo(downloadPath=/downloads/batman_arkham_city_goty/en1installer1) -> retrieval request error: Get \"https://www.gog.com/downloads/batman_arkham_city_goty/en1installer1\": read tcp 192.168.0.4:57797->88.221.61.153:443: wsarecv: An existing connection was forcibly closed by the remote host.",
    "GetDownloadFileInfo(downloadPath=/downloads/ultima_1/en1installer0) -> retrieval request error: Get \"https://www.gog.com/downloads/ultima_1/en1installer0\": read tcp 192.168.0.4:57797->88.221.61.153:443: wsarecv: An existing connection was forcibly closed by the remote host.",
    "GetDownloadFileInfo(downloadPath=/downloads/void_bastards/en1patch1) -> retrieval request error: Get \"https://www.gog.com/downloads/void_bastards/en1patch1\": read tcp 192.168.0.4:57797->88.221.61.153:443: wsarecv: An existing connection was forcibly closed by the remote host.",
    "GetDownloadFileInfo(downloadPath=/downloads/mutant_year_zero_road_to_eden/en1installer0) -> retrieval request error: Get \"https://www.gog.com/downloads/mutant_year_zero_road_to_eden/en1installer0\": read tcp 192.168.0.4:57797->88.221.61.153:443: wsarecv: An existing connection was forcibly closed by the remote host.",
    "GetDownloadFileInfo(downloadPath=/downloads/openttd/en1installer0) -> retrieval request error: Get \"https://www.gog.com/downloads/openttd/en1installer0\": read tcp 192.168.0.4:57797->88.221.61.153:443: wsarecv: An existing connection was forcibly closed by the remote host.",
    "GetDownloadFileInfo(downloadPath=/downloads/gods_will_fall/en1patch1) -> retrieval request error: Get \"https://www.gog.com/downloads/gods_will_fall/en1patch1\": read tcp 192.168.0.4:57797->88.221.61.153:443: wsarecv: An existing connection was forcibly closed by the remote host.",
    "GetDownloadFileInfo(downloadPath=/downloads/trine_2_complete_story/en1installer0) -> retrieval request error: Get \"https://www.gog.com/downloads/trine_2_complete_story/en1installer0\": read tcp 192.168.0.4:57797->88.221.61.153:443: wsarecv: An existing connection was forcibly closed by the remote host.",
    "GetDownloadFileInfo(downloadPath=/downloads/xcom_enforcer/en1installer3) -> retrieval request error: Get \"https://www.gog.com/downloads/xcom_enforcer/en1installer3\": read tcp 192.168.0.4:57797->88.221.61.153:443: wsarecv: An existing connection was forcibly closed by the remote host.",
    "GetDownloadFileInfo(downloadPath=/downloads/original_war_v30/de1installer1) -> retrieval request error: Get \"https://www.gog.com/downloads/original_war_v30/de1installer1\": read tcp 192.168.0.4:57797->88.221.61.153:443: wsarecv: An existing connection was forcibly closed by the remote host."
  ]
}
{
  "Errors": [
    "GetOwnedGames(page=13, search=) -> retrieval request error: did not expect status code of 503",
    "GetOwnedGames(page=16, search=) -> retrieval request error: did not expect status code of 503",
    "GetOwnedGames(page=15, search=) -> retrieval request error: did not expect status code of 503",
    "GetOwnedGames(page=21, search=) -> retrieval request error: did not expect status code of 503"
  ]
}
{
  "Errors": [
    "GetGameDetails(gameId=1861202745) -> retrieval request error: Get \"https://embed.gog.com/account/gameDetails/1861202745.json\": unexpected EOF"
  ]
}

Also had one generated for the following console output, but have since overwritten the manifest itself:

[sdk] GetGameDetails(gameId=1207659051) -> GET https://embed.gog.com/account/gameDetails/1207659051.json failed with code 504. Will retry.
[sdk] GetGameDetails(gameId=1207659046) -> GET https://embed.gog.com/account/gameDetails/1207659046.json failed with code 504. Will retry.
[sdk] GetGameDetails(gameId=1207659057) -> GET https://embed.gog.com/account/gameDetails/1207659057.json failed with code 502. Will retry.
[sdk] GetGameDetails(gameId=1207659048) -> GET https://embed.gog.com/account/gameDetails/1207659048.json failed with code 504. Will retry.
[sdk] GetGameDetails(gameId=1207659047) -> GET https://embed.gog.com/account/gameDetails/1207659047.json failed with code 504. Will retry.
[sdk] GetGameDetails(gameId=1207659049) -> GET https://embed.gog.com/account/gameDetails/1207659049.json failed with code 504. Will retry.

@Magnitus-
Copy link
Owner

Magnitus- commented Jan 12, 2022

The "Will retry." warnings you posted are fine. It is a request failing due to 5xx server error and the sdk retrying the request. The lack of follow-up message means that the retry of the request subsequently succeed. I could add a follow-up message to provide additional feedback to the user that the retrying succeeded, though that would involve carefully managing a retry state in a situation where there is concurrent output (not crazy, but not completely trivial either, especially if I want to handle it elegantly) and given the other more important improvements that I could be working on, I consider that a low priority task.

For your other "An existing connection was forcibly closed by the remote host." errors causing an abort, it is because GOG forcibly closed the connection without returning an http response and retrying only occurs when an http response is returned with a 5xx code (indicating a server-side error). I could feasibly flag this situation as a retry, either by detecting the error type if possible, or otherwise as a last resort, looking for that string in the error message.

And lastly, yes, I'm currently lacking proper log feedback for manifest generation. This is something I'll need to address. I think I went a bit far trying to support piping the output of the command and realistically, I'm not sure who will want to use that.

@xKevin04
Copy link
Author

Yeah, it's all fine! As I said, these are hard server-side errors where it doesn't really make sense for the program to ignore them/collect them as warnings and continue, because it's probable that any request to GOG will fail anyway. So aborting is the correct behavior here, even if dangles are tolerated.

I just meant that the first time I had a broken manifest generated, I didn't realize it right away, because there was no output on the console at all. So I thought everything went fine and tried to use it several times. I eventually saw that the JSON was really small and looked inside. It would suffice to write something along the lines of "Error generating manifest, see created file for details" if something happens that prevents creation of a usable manifest/leads to a non-zero exit code. I don't know if that could be a problem for other commands as well, maybe even a generic "An unexpected error occurred, aborting" would be enough to catch a user's attention.

@AstrowareConception
Copy link

I had the same issue. Your fix works perfectly! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants