pdf-converter: stop conversion when Cancel is clicked in GUI#37
Conversation
|
Could you please take a look when you have a moment? |
I'll run them, no worries :) But, can you try to add one for cancel too? For example copy test_002_500_pages, but call it as |
|
Maybe also check if dispvm got killed, for example collect list of qubes before the test (based on |
| /usr/bin/qvm-convert-pdf "$@" >"$fifo" & | ||
| converter_pid="$!" | ||
|
|
||
| zenity --progress --text="Converting PDF using Disposable VM..." --auto-close <"$fifo" |
There was a problem hiding this comment.
Zenity is not declared as a dependency for all supported distros: https://github.com/search?q=repo%3AQubesOS%2Fqubes-app-linux-pdf-converter%20zenity&type=code
t would be nice to have it in a separate commit.
There was a problem hiding this comment.
Actually, "recommends" instead of "depends", to not require zenity on minimal system that wants to be a client of pdf conversion.
| rm -f "$fifo" | ||
|
|
||
| if [ "$zenity_rc" -ne 0 ]; then | ||
| kill -TERM "$converter_pid" 2>/dev/null |
There was a problem hiding this comment.
TERM is the default.
Why hide stderr of this kill?
|
|
||
| if [ "$zenity_rc" -ne 0 ]; then | ||
| kill -TERM "$converter_pid" 2>/dev/null | ||
| wait "$converter_pid" 2>/dev/null |
There was a problem hiding this comment.
Why hide stderr of this wait?
| zenity --progress --text="Converting PDF using Disposable VM..." --auto-close <"$fifo" | ||
| zenity_rc="$?" | ||
|
|
||
| rm -f "$fifo" |
There was a problem hiding this comment.
If playing with rm and variables, use -- and nounset:
set -u
...
rm -f -- "$fifo"
Jayant-kernel
force-pushed
the
fix-pdf-convert-cancel-10274
branch
from
0318219 to
bbb99ab
Compare
|
@ben-grande |
| python3-tqdm, | ||
| ${misc:Depends} | ||
| Recommends: | ||
| zenity |
There was a problem hiding this comment.
added Recommends: zenity to rpm_spec/qpdf-converter.spec.in.
There was a problem hiding this comment.
There are two specs in that directory.
| @@ -0,0 +1,73 @@ | |||
| #!/usr/bin/python3 | |||
There was a problem hiding this comment.
This file should be inside /tests/
There was a problem hiding this comment.
Moved the test_client.py is now at qubespdfconverter/tests/test_client.py.
|
Can you test this on Qubes OS? I don't mean running the tests, but interacting with the tool via GUI and CLI. |
marmarek
left a comment
marmarek
left a comment
There was a problem hiding this comment.
Manual interaction with this works fine now (both canceling in CLI and GUI seems to work). But unfortunately automated tests fails:
| self.vm.run('test -r test.trusted.pdf', wait=True), 0, | ||
| 'trusted pdf should not exist after cancel') | ||
| domains_after = set(self.app.domains) | ||
| self.assertEqual(domains_before, domains_after, |
There was a problem hiding this comment.
Unfortunately it fails right now, I think dispvm needs a bit of time to be cleaned up. When I add self.loop.run_until_complete(asyncio.sleep(5)) (and also import asyncio) just before this line, it works, but it isn't nice...
The trick here is to balance:
- detecting if dispvm really got killed, instead of simply completing conversion in the background
- not failing if dispvm cleanup takes some more time
5s works on my system (according to logs, even 1s might be enough), but it isn't always guaranteed, especially on slower systems. On the other hand, the earlier test for converting 500 pages fully takes just above 1m30s on the same system, and 20s of that time is already waited before cancelling conversion here, so can't wait too long, to detect the first point above.
Hm, if I read the logs correctly, it doesn't even need to wait for the dispvm cleanup to complete - if dispvm cleanup just starts, the VM is not listed anymore. So, maybe 5s isn't really that bad? It isn't long enough to guarantee complete dispvm cleanup, but should be long enough to for cleanup process to start. And surely 5s is not long enough to complete conversion of 500 pages.
@ben-grande do you have some better ideas?
Otherwise, @Jayant-kernel can you add the proposed sleep here? I'd run it a few times through our CI to see if it passes reliably there.
|
On core we wait 20s:
https://github.com/QubesOS/qubes-core-admin/blob/53ca30fc3257ac8174ae6a6d37dce70f72e788d5/qubes/tests/integ/dispvm.py#L431
So if you are worried of completing cleanup in the background, you can
lower it to 10.
If you consider it too much, you can also (I never tried) check for a
DispVM with auto_cleanup enabled, that is trying to shutdown/get killed,
but that will eventually lead to QubesVMPropertyAccessError (or some other
similar one) or QubesVMNotFoundError, which should be gracefully handled.
You can get the qube object early if you await for it in a loop with
asyncio.sleep while running the conversion command also asynchronously
(asyncio.create_subprocess_exec, or some similar name).
…On Sat, Mar 14, 2026, 5:08 PM Marek Marczykowski-Górecki < ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Manual interaction with this works fine now (both canceling in CLI and GUI
seems to work). But unfortunately automated tests fails:
------------------------------
In qubespdfconverter/tests/__init__.py
<#37 (comment)>
:
> @@ -135,6 +135,24 @@ def test_003_filename_with_spaces(self):
self.assertCorrectlyTransformed('orig.pdf',
'test with spaces.trusted.pdf')
+ def test_004_cancel_stops_conversion(self):
+ self.create_pdf('test.pdf', ['This is test'] * 500)
+ domains_before = set(self.app.domains)
+ p = self.vm.run(
+ 'cp test.pdf orig.pdf; '
+ 'timeout --signal=INT 20 qvm-convert-pdf test.pdf 2>&1',
+ passio_popen=True)
+ (stdout, _) = p.communicate()
+ self.assertNotEqual(p.returncode, 0,
+ 'Expected non-zero exit from interrupted conversion: {}'.format(stdout))
+ self.assertNotEqual(
+ self.vm.run('test -r test.trusted.pdf', wait=True), 0,
+ 'trusted pdf should not exist after cancel')
+ domains_after = set(self.app.domains)
+ self.assertEqual(domains_before, domains_after,
Unfortunately it fails right now, I think dispvm needs a bit of time to be
cleaned up. When I add self.loop.run_until_complete(asyncio.sleep(5))
(and also import asyncio) just before this line, it works, but it isn't
nice...
The trick here is to balance:
- detecting if dispvm really got killed, instead of simply completing
conversion in the background
- not failing if dispvm cleanup takes some more time
5s works on my system (according to logs, even 1s might be enough), but it
isn't always guaranteed, especially on slower systems. On the other hand,
the earlier test for converting 500 pages fully takes just above 1m30s on
the same system, and 20s of that time is already waited before cancelling
conversion here, so can't wait too long, to detect the first point above.
Hm, if I read the logs correctly, it doesn't even need to wait for the
dispvm cleanup to complete - if dispvm cleanup just starts, the VM is not
listed anymore. So, maybe 5s isn't really that bad? It isn't long enough to
guarantee complete dispvm cleanup, but should be long enough to for cleanup
process to start. And surely 5s is not long enough to complete conversion
of 500 pages.
@ben-grande <https://github.com/ben-grande> do you have some better ideas?
Otherwise, @Jayant-kernel <https://github.com/Jayant-kernel> can you add
the proposed sleep here? I'd run it a few times through our CI to see if it
passes reliably there.
—
Reply to this email directly, view it on GitHub
<#37 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BCE2O4I6UOZBNLTWS6WBAAL4QV7X5AVCNFSM6AAAAACWFPM3LGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTSNBYHE4DMMRVHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
|
Jayant-kernel
force-pushed
the
fix-pdf-convert-cancel-10274
branch
from
d95c3af to
3fa0dcb
Compare
|
@ben-grande @marmarek |
|
Please cleanup commit history. |
Replace the pipeline in qvm-convert-pdf.gnome with explicit process tracking via a FIFO. Store the converter PID and send SIGTERM when zenity exits non-zero (user clicked Cancel), then wait for it to exit. Handle both SIGINT and SIGTERM in the async signal handler so the converter responds to either signal. Fix the subprocess cleanup condition (returncode is None means still running) to ensure qrexec subprocesses are always terminated on CancelledError or failure. Fixes QubesOS/qubes-issues#10274
zenity is needed by qvm-convert-pdf.gnome for the progress dialog. Use Recommends (not Depends) so minimal client-only installs are not forced to pull in a GUI toolkit. Added to both debian/control and rpm_spec/qpdf-converter.spec.in.
Jayant-kernel
force-pushed
the
fix-pdf-convert-cancel-10274
branch
from
3fa0dcb to
1a05c11
Compare
|
@marmarek The main fix (gnome script FIFO + signal handling) Could you re-run CI to verify the test passes reliably? |
Add test_004_cancel_stops_conversion: sends SIGINT after 20 seconds into a 500-page conversion, verifies the output file was not created, and polls up to 10s for the DispVM to disappear from self.app.domains. DispVM cleanup in dom0 is asynchronous, so a polling loop is used instead of an immediate check (mirrors qubes-core-admin pattern). Add unit tests for the SIGINT/SIGTERM handling in client.py.
Jayant-kernel
force-pushed
the
fix-pdf-convert-cancel-10274
branch
from
1a05c11 to
befc310
Compare
|
@marmarek @ben-grande |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026032905-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026032404-devel&flavor=update
Failed tests50 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/170766#dependencies 30 fixed
Unstable testsDetailsPerformance TestsPerformance degradation:16 performance degradations
Remaining performance tests:88 tests
|
|
@marmarek @ben-grande |
|
Still happening for Fedora: https://openqa.qubes-os.org/tests/173065#step/TC_00_PDFConverter_fedora-43-xfce/5 It's probably because the template shutdown is too slow. It is skipped on Whonix, but probably would require more time. Since there is no PR, I will make one that extends the time to twenty seconds. |
OpenQA tests passes on Debian but not on Fedora, therefore, increase the timeout by a bit. Fixes: QubesOS/qubes-issues#10274 For: QubesOS#37
4 participants
Summary
Fix
qvm-convert-pdf.gnome/ client cancellation flow so clicking Cancel in the GUI progress dialog actually stops the active conversion job.Problem
In
QubesOS/qubes-issues#10274, canceling the Zenity dialog did not reliably stop the running conversion in the DispVM. The user-facing dialog closed, but backend conversion could continue in the background.Changes
qvm-convert-pdf.gnomeSIGTERMtoqvm-convert-pdfwhen Zenity exits non-zero (Cancel).qubespdfconverter/client.pySIGINTandSIGTERMin async signal handlers.qrexec-client-vmsubprocess is terminated onCancelledError.returncode is None) so running subprocesses are actually terminated.qubespdfconverter/test_client.pyWhy this should fix the bug
GUI cancel now explicitly signals the converter process, and converter-side cancellation cleanup reliably tears down active qrexec subprocesses, which prevents lingering background conversion.
Validation
python -m unittest qubespdfconverter.test_clientFixes QubesOS/qubes-issues#10274.