Skip to content

fix(gui): change app.lock location#310

Merged
cachebag merged 4 commits intocachebag:masterfrom
tristanmsct:fix-app-lock
Mar 27, 2026
Merged

fix(gui): change app.lock location#310
cachebag merged 4 commits intocachebag:masterfrom
tristanmsct:fix-app-lock

Conversation

@tristanmsct
Copy link
Copy Markdown
Contributor

Hi

I did not find an issue for this small problem so I created a standalone PR
I'm trying to follow the contributing guidelines
This PR is really simple but this file has been bugging me, I'm sorry if I missed anything in terms of formating or type of commit, etc. Let me know if I need to correct anything

Motivation

The app creates a $XDG_DATA_DIR/my_app.lock when running to know if an instance is already running.
I think the name and place of the file can be improved.

The name of the file does not hint at which program creates it.
The file itself would be better placed in the XDG_RUNTIME_DIR within a nmrs subfolder. Or at least if it stays in $XDG_DATA_DIR, then it can be in an nmrs subfolder.

Description

This PR simply tries to create the lock file in $XDG_RUNTIME_DIR instead of $XDG_DATA_DIR, and if it fails then it falls back to the old directory.
Also the lock file is now nmrs/app.lock instead of just my_app.lock which is clearer for the user.
The changes are only in this file nmrs-gui/src/file_lock.rs.

Testing

  • cargo test --all-features : all tests were ok except 15 tests that were ignored.
  • cargo test --test integration_test --all-features : 43 tests, all ok

I build the app and tested it, the lock file is created at /run/usr/1000/nmrs/app.lock.
I could not get the creation at this specific place to fail, to confirm that it would indeed be created at ~/.local/share/nmrs/app.lock but I see no reason why it would not work.

AI Disclosure

I am not a rust developer and used a LLM to check my syntaxe and be sure to use the right functions for the job.

Copy link
Copy Markdown
Owner

@cachebag cachebag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tristanmsct
Thanks for this! Always appreciate someone cleaning up my code lol.

Just make sure you run fmt and clippy and whatnot and amend your changes to your commit. Appreciate the patch!

also, let's add a test. at minimum one that calls acquire_app_lock() and asserts it succeeds, and a second call that asserts it fails with the "already running" error while the first lock is held. the existing tests in the project should give you a feel for the style.

for the commit message, please just write fix(gui): ...

@cachebag cachebag added bug Something isn't working nmrs-gui Changes to nmrs-gui labels Mar 26, 2026
@tristanmsct tristanmsct changed the title refactor: change app.lock location fix(gui): change app.lock location Mar 26, 2026
Tries to write app.lock to $XDG_RUNTIME_DIR/nmrs/app.lock
Falls back to $XDG_DATA_DIR/nmrs/app.lock
Copy link
Copy Markdown
Owner

@cachebag cachebag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

@tristanmsct
Copy link
Copy Markdown
Contributor Author

Hello again

I've tried to add two unit tests, one that call the acquire_app_lock function, checks if the lock file i correctly created and another test that call the function twice to see it it fails "properly".

The problem is, based on my very limited understanding of rust unittest, they are run in parallel, so I hit a race condition where the second test can cause the first test to fail.

One solution would be to run all tests sequentially which slows down the tests by a lot.
Do you know of a way to specify that only two specific tests should not be run in parallel ?
The solution of running all tests on a single threat is not super satisfactory

Adds two tests
 - A test to validate that the lock file is properly created
 - A test to see if trying to acquire the lock twice fails
@tristanmsct
Copy link
Copy Markdown
Contributor Author

I added the two tests mentioned above, in a nmrs-gui/tests/applock_test.rs file
I added the test function to a module as per the documentation here https://doc.rust-lang.org/rust-by-example/testing/unit_testing.html, but I saw you did not use this convention so I'm not sure if the test should be removed from the mod for better integration in the codebase

I solved the race condition using this method : rust-lang/rust#43155

Hope this helps

Copy link
Copy Markdown
Owner

@cachebag cachebag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just one final note

Copy link
Copy Markdown
Owner

@cachebag cachebag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@cachebag cachebag merged commit e661568 into cachebag:master Mar 27, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working nmrs-gui Changes to nmrs-gui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants