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

libdnf5/base/active_transaction_info.cpp

+        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

libdnf5/base/active_transaction_info.cpp

+    } 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

libdnf5/base/transaction.cpp

+    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().

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.

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

libdnf5/base/transaction.cpp

+    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().

dcantrell · 2026-01-13T14:26:37Z

libdnf5/utils/locker.cpp

+        throw SystemError(errno, M_("Failed to write to lock file \"{}\""), path);
+    }
+
+    // Ensure data is written to disk


The single call to fsync() does not technically ensure data is written to the file. Annoyingly you have to fsync() both the fd for the file and a separate fd for the directory containing the file.

This may be splitting hairs because we're also at the mercy of the kernel VFS layer and ultimately the filesystem driver and the storage device driver, but at least in userspace the control given to guarantee data is on the device we have to do two fsync() calls.

dcantrell · 2026-01-13T14:28:34Z

libdnf5/utils/locker.cpp

+
+    std::string content;
+    content.reserve(1024);
+    char buffer[1024];


Style preference, I like using BUFSIZ for arbitrary buffers rather than hardcoding a value. In theory it keeps the code using platform-preferred buffer sizes as defined by the C library at build time.

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 added 8 commits October 10, 2025 09:12

locker: Add methods to read/write lock file content

144a5eb

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

7d703c1

Add ActiveTransactionInfo class

2a870ac

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

2b0276b

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

transaction: Use TransactionLocker instead of Locker

ad6d5f5

transaction: Store info about concurrently running transaction

1ccd277

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

6b96e14

Handle TOML parsing errors for ActiveTransactionInfo

d4bf88e

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 fa9351f to d4bf88e Compare October 10, 2025 07:13

dcantrell requested changes Jan 13, 2026

View reviewed changes

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

m-blaha commented Aug 18, 2025 •

edited

Loading