State isn't automatically saved during system shutdown

[ThomasFrans]

Describe the bug ncspot doesn't save its playback state during system shutdown or reboot when not manually closed by the user.

To Reproduce Steps to reproduce the behavior:

1. Play any song
2. With ncspot running and playing the song, power off the pc
3. Power on the pc and open ncspot
4. The playback state is different than it was before shutdown (it is the state that was saved when properly closing ncspot last time)

Expected behavior When ncspot is playing songs, the user should be able to power off their system with ncspot open in the background, and have ncspot save its playback state automatically.

System (please complete the following information):

• OS: Arch Linux (latest updates)
• Terminal: Alacritty
• Version: 0.13.0
• Installed from: Arch repository

[hrkfdn]

So we do handle SIGTERM and SIGHUP to save the state when ncspot is being terminated. I wonder if there's another signal involved for system shutdown, ~maybe SIGKILL~?

Edit: Forgot thatSIGKILL can not be handled by applications.

[ThomasFrans]

I don't really know what signals the kernel sends at which time as I haven't used them before. I do think however that a cleaner approach might be to use the initsystem for this (I could be wrong). I think I've read that init systems allow you to inhibit certain events (like lid closed, shutdown, reboot, sleep...). Maybe that would be a simpler, easier way to implement this? I haven't used it myself so I'm not 100% sure that is how it works.

https://www.freedesktop.org/wiki/Software/systemd/inhibit/ This appears to use D-Bus which ncspot already uses for its MPRIS interface so no extra dependencies would be needed :slightly_smiling_face:

[ThomasFrans]

I tried to do an initial implementation of this just to see how it would work with systemd-logind. The code is hacked together just for testing purposes, but I can't get it to work correctly. The state is saved, which I validated both by playing songs, skipping a few and shutting down with ncspot running, as well as by writing a file after receiving the PrepareForShutdown signal, to make sure it was received well. The only thing I can't get working at all is releasing the lock by dropping the file descriptor. I put the drop of the OwnedFd both inside the other thread as well as after the event loop (where it is in this commit). The lock just isn't released. If anyone knows what is wrong with this code, I could use some help :slightly_smiling_face:

https://github.com/ThomasFrans/ncspot/blob/68c4c0f4a078dbdeee71e17073c5592c0885f6f4/src/main.rs#L350-L360

[hrkfdn]

I have to say I still don't understand why this is necessary :thinking: Normally this shouldn't be needed, as I'd assume that ncspot would either receive a SIGTERM or SIGHUP on shutdown..

[ThomasFrans]

Currently I have v0.13.2 installed from the Arch Community repository, and I quickly tested again. It really doesn't save on shutdown. I find this very weird as well as various sources online confirm that Systemd is supposed to send SIGTERM to processes before shutting down. One possibility I've considered is that maybe ncspot takes such a long time to shut down that Systemd sends SIGKILL because it thinks ncspot isn't responding. One Stack Overflow post mentions that Systemd waits 10 seconds between SIGTERM and SIGKILL. This would make it highly unlikely that ncspot receives the SIGKILL signal as it wouldn't take 10 seconds to properly save and exit. I also closed ncspot manually countless times, in which case it does properly save state, indicating that the SIGTERM handling works. I don't have any idea what is going wrong with shutdown specifically.

[bfahrenfort]

I'm looking to get involved with contribution through this. @ interested parties, would a callback that saves state after each song played be wanted here? That way you lose at most one song should the system shut off before SIGTERM or SIGHUP.

Unsure of the performance implications since I'm still unfamiliar with the codebase.

[ThomasFrans]

For me, the main reason why I would want this is to save the exact position in a song before the system shuts down. It's a nice experience to turn your PC on a bit later, still remembering the song that played last time, and having it play from the exact same position again. I remember writing a test with the signals where a file would be written after shutting down the system. The file would actually be written on disk. Therefore I'm still not sure why the more complex example of ncspot saving the exact state doesn't work properly...

[bfahrenfort]

Curious if it's just an OS file issue. I'm messing with how serde_cbor handles serialization to test this. If I do find something, then the fix will be to migrate to a maintained CBOR crate.

[ThomasFrans]

I created a PR with a fix for an unrelated issue a few days ago and I'm currently dogfooding that one. I noticed that when I close my terminal emulator, it hangs for a little bit before it closes. The PR adds a fix to prevent a panic when closing ncspot. I wonder whether this issue might also be related to a similar problem (#1282). What if shutting down the PC causes issue #1282 which in term means that ncspot doesn't get to run the saving logic? I'm not a 100% sure that this is related, but it could very well be. Might be worth it to just wait until #1282 is closed before looking further into this one...