Use transaction lock file to provide details abour currently running transaction by m-blaha · Pull Request #2391 · rpm-software-management/dnf5

m-blaha · 2025-08-18T09:26:50Z

This PR:

Enhances the existing Locker class to be able to read and write the contents of the lock file.
Adds previously missing unit tests for the Locker class.
Introduces a private TransactionLocker class, which is specifically designed for transaction locking and can record essential details about the current transaction, including the initiator, the process PID, and a description of the transaction.
provide a more informative error message to the user. Rather than simply stating, "Failed to obtain rpm transaction lock. Another transaction is in progress," the message should include additional information, such as the PID of the other process, its current status, and the command line used to initiate the transaction.

dcantrell · 2025-09-17T19:35:16Z

+        info.set_pid(toml::find_or<pid_t>(data, "pid", -1));
+        info.set_start_time(toml::find_or<time_t>(data, "start_time", 0));
+    } catch (const toml::syntax_error &) {
+        // Return default/empty ActiveTransactionInfo on TOML syntax errors


What happens here?

Both these exceptions mean that the lock file exists, but contains damaged/incorrect data not digestible by TOML parser. Since data from the lock file are not crucial for dnf5 behavior (only contains details about a concurrently running transaction), instead of throwing an error an empty ActiveTransactionInfo object is returned.

Maybe I'm missing something here, but that feels like it is obscuring what is happening by returning an empty ActiveTransactionInfo object. Damaged or incorrect data in the lock file feels like it warrants a new exception type so that it is more clear what is happening and where. Or at least to help a developer debugging something going wrong.

I guess you are right. I originally thought that the parsing errors are not worth handling properly. But yeah, it definitely makes debugging harder and it's better to do the error handling right away than adding it later.

@dcantrell I've added a commit with parsing errors handling.

dcantrell · 2025-09-17T19:35:24Z

+    } catch (const toml::syntax_error &) {
+        // Return default/empty ActiveTransactionInfo on TOML syntax errors
+    } catch (const toml::type_error &) {
+        // Return default/empty ActiveTransactionInfo on TOML type errors


What happens here?

dcantrell · 2025-09-17T19:35:54Z

+    try {
+        write_content(transaction_info.to_toml());
+    } catch (const SystemError & e) {
+        //TODO(mblaha): log the error (would require base object)


At least in the interim should this just go to stderr?

Hm, we try to keep libdnf5 quiet. It does not print to stderr at all.

If libdnf5 is logging, stderr is not all that different, right? If we are to log here but are not [yet], then stderr feels appropriate. Either that or raise an exception.

If the catch block is going to do nothing here, I suggest just removing the try/catch block entirely and leaving the function as just a call to write_content().

dcantrell

It looks good, I only have a few minor questions and based on your answers or changes I'll mark it as approved.

dcantrell · 2026-01-13T14:23:33Z

+    try {
+        write_content(transaction_info.to_toml());
+    } catch (const SystemError & e) {
+        //TODO(mblaha): log the error (would require base object)


If the catch block is going to do nothing here, I suggest just removing the try/catch block entirely and leaving the function as just a call to write_content().

mfocko · 2026-02-09T11:42:17Z

+    TransactionLocker transaction_locker(lock_file_path, info);
+    if (!transaction_locker.write_lock()) {
        return TransactionRunResult::ERROR_LOCK;
    }
+    transaction_locker.write_transaction_metadata();


NGL it feels quite verbose

At least for naming:

Suggested change

TransactionLocker transaction_locker(lock_file_path, info);

if (!transaction_locker.write_lock()) {

return TransactionRunResult::ERROR_LOCK;

}

transaction_locker.write_transaction_metadata();

TransactionLocker lock(lock_file_path, info);

if (!lock.write_lock()) {

return TransactionRunResult::ERROR_LOCK;

}

lock.write_transaction_metadata();

As for the interface itself, I would say that it can be easily deduced from the type name, so even

write_lock() -> write()

write_transaction_metadata() -> write_metadata()

wouldn't reduce the clarity IMO

Now that I'm looking at it, why don't we merge the write_lock() and write_transaction_metadata() into one method? In this case, I guess it would even make sense to fail with ::ERROR_LOCK when the metadata cannot be stored properly?

mfocko · 2026-02-09T11:43:45Z

    TransactionLocker transaction_locker(lock_file_path, info);
    if (!transaction_locker.write_lock()) {
+        auto content = transaction_locker.read_content();
+        concurrent_transaction = std::make_unique<ActiveTransactionInfo>(ActiveTransactionInfo::from_toml(content));


I guess we can guarantee only one concurrent transaction, because of the shared path?

dcantrell

I like this revision. Apologies for the delay getting back to it!

Extend the Locker class to include the ability to read and write the contents of the lock file. This feature is intended to record information identifying the process that acquired the lock.

Introduce a new ActiveTransactionInfo class for storing transaction metadata. This metadata will be saved in the lock file, allowing other processes to provide the user with information about the currently running transaction.

This class can write details about transaction into transaction lock file in TOML format.

If a transaction run fails because another transaction is running concurrently, record information about that transaction so it can be used later to generate a clearer error message.

Errors are no longer silently ignored, a new ActiveTransactionInfoParseError is thrown instead. The error is caught and logged during the transaction run.

m-blaha changed the title ~~Transaction-info~~ Use transaction lock file to provide details abour currently running transaction Aug 18, 2025

m-blaha force-pushed the transaction-info branch 2 times, most recently from 0738f32 to 43b79e3 Compare September 17, 2025 08:32

m-blaha marked this pull request as ready for review September 17, 2025 09:00

m-blaha requested a review from a team as a code owner September 17, 2025 09:00

m-blaha requested review from dcantrell and removed request for a team September 17, 2025 09:00

dcantrell requested changes Sep 17, 2025

View reviewed changes

m-blaha requested a review from dcantrell October 2, 2025 13:50

m-blaha force-pushed the transaction-info branch from fa9351f to d4bf88e Compare October 10, 2025 07:13

dcantrell requested changes Jan 13, 2026

View reviewed changes

mfocko force-pushed the transaction-info branch from d4bf88e to 3473640 Compare February 17, 2026 09:27

mfocko self-assigned this Feb 17, 2026

mfocko reviewed Feb 17, 2026

View reviewed changes

m-blaha force-pushed the transaction-info branch from 3473640 to 991c706 Compare March 12, 2026 07:19

dcantrell previously approved these changes Mar 23, 2026

View reviewed changes

m-blaha added 7 commits March 24, 2026 10:42

locker: Add methods to read/write lock file content

21a6eca

Extend the Locker class to include the ability to read and write the contents of the lock file. This feature is intended to record information identifying the process that acquired the lock.

Add unit tests for the Locker class

d308e01

Add ActiveTransactionInfo class

db4e278

Introduce a new ActiveTransactionInfo class for storing transaction metadata. This metadata will be saved in the lock file, allowing other processes to provide the user with information about the currently running transaction.

Specialized TransactionLocker class

b8bdf7d

This class can write details about transaction into transaction lock file in TOML format.

transaction: Use TransactionLocker instead of Locker

a44de00

transaction: Store info about concurrently running transaction

70c3f5e

If a transaction run fails because another transaction is running concurrently, record information about that transaction so it can be used later to generate a clearer error message.

dnf5: Print details about concurrent running transaction

4909e45

m-blaha force-pushed the transaction-info branch from 991c706 to 9e06f54 Compare March 24, 2026 09:43

Handle TOML parsing errors for ActiveTransactionInfo

5b8a3cc

Errors are no longer silently ignored, a new ActiveTransactionInfoParseError is thrown instead. The error is caught and logged during the transaction run.

m-blaha force-pushed the transaction-info branch from 9e06f54 to 5b8a3cc Compare March 24, 2026 13:00

m-blaha dismissed dcantrell’s stale review via 5b8a3cc March 24, 2026 14:26

mfocko approved these changes Apr 1, 2026

View reviewed changes

m-blaha added this pull request to the merge queue Apr 8, 2026

Merged via the queue into rpm-software-management:main with commit 9541e3f Apr 8, 2026
18 of 19 checks passed

m-blaha deleted the transaction-info branch April 8, 2026 09:26

Uh oh!

Conversation

m-blaha commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcantrell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dcantrell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

m-blaha commented Aug 18, 2025 •

edited

Loading