Rich Presence fails if PID is less than 10

[Lachee]

Describe the bug Rich Presence will ignore any activity that sets its PID to 10 or less. This seems like an arbitrary limitation, as other than 0, in general there are no reserved process id.

Steps to reproduce Steps to reproduce the behavior:

1. Send a Rich Presence packet
2. Give it a PID of 8
3. See Rich Presence not being sent
4. See error in console

Expected behavior The presence should be sent, despite what the PID is.

Screenshots

Implementation specifics

• Language: C#
• Library: lachee/discord-rpc-csharp

Additional context

While there is a 9 in a 4,294,967,292 (or a 1 : 477,218,588 ) chance of getting this error (since 0 is generally system), I have manged to get it during testing.... lucky me?

Should also mention I have only tested this using the RPC and sending Rich Presence.

[msciotti]

This is working as intended according to our code:

    pid: joi.number().min(10).required(),

But I do not know why the minimum is 10

[sylveon]

This assumption doesn't hold very well, PIDs can and do rollover so at one point you might have a user mode process with a PID lower than 10. I guess the intent here was "make sure the user doesn't pass a garbage value for fun".

It's also possible to completely remove the requirement to pass a PID by using an API like GetNamedPipeClientProcessId

[Lachee]

It's also possible to completely remove the requirement to pass a PID by using an API like GetNamedPipeClientProcessId

I should mention that you could originally set the Rich Presence via internal http server too.

[msciotti]

Asked some folks internally and got the answer that pids less than 10 are generally reserved for system stuff. Even if not technically always true, I don't think there is reasonably a pid that is going to roll over that way? It seems the max PID is anywhere from like 32k to 4million depending on the operating system so it seems not necessary to take that into account.

Gonna mark this as closed because while maybe somewhat arbitrary, I don't think it actually causes any issues.

[sylveon]

The original issue was created because it happened to cause an issue.

[Lachee]

Asked some folks internally and got the answer that pids less than 10 are generally reserved for system stuff. Even if not technically always true, I don't think there is reasonably a pid that is going to roll over that way? It seems the max PID is anywhere from like 32k to 4million depending on the operating system so it seems not necessary to take that into account.

Gonna mark this as closed because while maybe somewhat arbitrary, I don't think it actually causes any issues.

I mean it literally caused an issue, I couldn't set the presence and had to restart the application to get a new process id. Imagine calling new RichPresenceClient or what ever, and it throwing a Not Supported because your process is 8

This limitation is arbitrary as it is generally but not actually reserved. Modern Windows tend to do PIDs divisible by 4 now for system stuff.

My main point is that I dont see the point of this validation. It would cause no issues if it was less than 10. Even if it was to prevent system pids, just look at your windows instance now and you will see plenty of more that wont even fit in 10.

Its not like you are spawning a process with that PID.

[msciotti]

I will raise it to the native team. I apologize for dismissing this. I assume that you guys had all rolled custom libraries and were arbitrarily setting PIDs for presence to show, not tying it to actual PIDs.

[msciotti]

Talked to some native folks and y'all are right. Will need to update with slightly different validation (I imagine PID > 0 should do the trick). It seems like there's technically a max PID on systems as well, but it seems so high that it doesn't really matter to validate against (and different per architecture)

[Lachee]

Awesome thanks. I assume you will close this PR when done?

[msciotti]

Yup! It's merged--just needs to be deployed.

[tpcstld]

This change should be deployed on all clients 68381 and higher now.