Skip to content

fix(userTransfer): honour .rtorrentDisable opt-out on post-transfer restart (Refs #869) - #929

Open
Shabbir7890 wants to merge 2 commits into
MagnaCapax:mainfrom
Shabbir7890:fix/869-rtorrent-disable-opt-out
Open

Shabbir7890 wants to merge 2 commits into
MagnaCapax:mainfrom
Shabbir7890:fix/869-rtorrent-disable-opt-out

Conversation

@Shabbir7890

Copy link
Copy Markdown
Contributor

Honours the per-user .rtorrentDisable opt-out on the post-transfer restart path, matching what checkRtorrent already does on every cycle.

pmssUserTransferRequestRtorrentRestart() unconditionally touched the restart
marker and ran the restart after a transfer, ignoring the per-user
.rtorrentDisable opt-out that checkRtorrent honours on every cycle (GH#470).
A user who had deliberately disabled rTorrent got it restarted (and their
session reloaded) after any migration, until the next watchdog pass stopped it
again. Skip the restart when the marker is present, matching checkRtorrent.php.


Authored from the private disclosure thread, moved here as a PR at the maintainer's suggestion so the diff arrives apply-able rather than as a ticket attachment.

Not linted or test-run: there is no PHP toolchain or Debian box on the authoring machine. bash -n/sh -n pass on every shell file touched, and the patch applies cleanly to main @ 8598305. Please php -l and run the dev suite before merge.

…estart (Refs MagnaCapax#869)

pmssUserTransferRequestRtorrentRestart() unconditionally touched the restart
marker and ran the restart after a transfer, ignoring the per-user
`.rtorrentDisable` opt-out that checkRtorrent honours on every cycle (GH#470).
A user who had deliberately disabled rTorrent got it restarted (and their
session reloaded) after any migration, until the next watchdog pass stopped it
again. Skip the restart when the marker is present, matching checkRtorrent.php.
@Shabbir7890

Copy link
Copy Markdown
Contributor Author

Validation update — I can now run the repository's own development container, so this is no longer "reviewed but not run".

php -l                on every changed PHP file: clean
development suite     Tests: 3075, Failures: 25, Skipped: 18
main @ 8598305        Tests: 3075, Failures: 25, Skipped: 18
new failures          none

No test count change.

Those 25 baseline failures are not this branch: they are present on a clean main in the documented container and come from the image lacking php-xml and curl, plus docker run landing at uid 0 — measured and filed separately as #936, with a fix in #938. Once that lands the baseline drops to 6.

Run with docker build -t pmss-dev . and the suite against a clean LF clone. Full CI parity (PHP 7.3 lint, bash checks, doctrine/docblock lint) is still yours to run; I have no Debian host, only the container.

@MagnaCapax

Copy link
Copy Markdown
Owner

Thank you for this. The fix is right, and it sits in the right place.

The gap started on 2026-08-17. The .rtorrentDisable opt-out (3d2afe9, #470) was added to checkRtorrent.php, but the post-transfer restart in postSetup.php was written before the opt-out existed and never learned about it. Your guard closes exactly that gap. Here is what I checked:

  • Placement. The guard is inside pmssUserTransferRequestRtorrentRestart() and not in pmssUserTransferPostSetup(). The session-path rewrite (postSetup.php:26) and the completeness check (:47) still run for opted-out users, so re-enabling later loads correct paths. An early return one level up would have skipped both. You avoided that.
  • The marker arrives in time. .rtorrentDisable is not in the rsync exclude list, so it is on the target before post-setup runs, and the check sees it.
  • Parity. The predicate matches checkRtorrent.php:125. The claim in your code comment holds on current main: with the checkRtorrent.php killall cleanup never matches rtorrent's true comm ('rtorrent main') — .rtorrentDisable and suspended-user cleanup silently no-op #836 fix (208dfaf), the watchdog does stop an opted-out rTorrent.
  • Merge. The branch is 77 commits behind main but merges cleanly. main changed postSetup.php once since your base (82a8c49, the restart-script guard in pmssUserTransferRunRtorrentRestart()), and it does not overlap your hunk.

I reviewed the code statically against current main. I have not run the suite, and CI has not run on this PR yet.

Verdict: merge after one fix.

  1. Add a behaviour test (required; AGENTS.md:160 and CONTRIBUTING.md:13 ask for code and tests together). In scripts/lib/tests/development/userTransferTest.php:

    • Build a home with pmssMakeUserWebHome() and write .rtorrentDisable into it.
    • Call pmssUserTransferRequestRtorrentRestart() inside pmssCaptureStdout() with PMSS_DRY_RUN=1 and a temporary PMSS_LOG_DIR.
    • Assert that the skip line is present and Requesting rTorrent restart marker is absent.
    • Add a control case without the marker, where the request line does appear.

    Under dry run, runStep() logs the step without executing it, so the test stays hermetic. testRewriteRtorrentSessionPathsReportsWhenNothingNeedsRewrite (around :411) is the pattern to copy. I'd prefer a test that exercises the behaviour over one that only checks source-string order.

  2. Optional, recommended: replace the two inline checks (checkRtorrent.php:125 and postSetup.php:74) with one pmssRtorrentUserOptedOut(string $home): bool under scripts/lib/rtorrent/. userTransfer: post-transfer rTorrent restart ignores the .rtorrentDisable per-user opt-out marker #869 names this as the preferred variant. If you take it, checkRtorrentContractTest.php:126-140 asserts the literal inline is_file(...) string and its order, so update that contract in the same commit.

  3. Nit: the four-line comment at postSetup.php:70-73 would read just as well in two.

Out of scope for this PR: other rTorrent start paths also ignore the opt-out, namely the ruTorrent panel restart via etc/skel/.rtorrentRestart.php, the @reboot line in etc/seedbox/config/user.crontab.default, and scripts/startRtorrent. Those are separate triggers and belong in their own issue.

(Off means off, even mid-voyage.)

…ansfer restart (Refs MagnaCapax#869)

Adds the behaviour test requested on the PR: pmssUserTransferRequestRtorrentRestart()
skips and logs the opt-out when ~/.rtorrentDisable is present, and requests the restart
marker when it is absent. Runs under PMSS_DRY_RUN so it is hermetic (runStep logs the
step without executing touch/chown), matching the existing session-rewrite tests.
@Shabbir7890

Copy link
Copy Markdown
Contributor Author

Added the behaviour test you asked for (b53e814).

testRequestRtorrentRestartSkipsWhenUserOptedOut writes ~/.rtorrentDisable and asserts the skip is logged and no restart marker is requested; testRequestRtorrentRestartRequestsMarkerWithoutOptOut is the control without the marker. Both run under PMSS_DRY_RUN so runStep logs the step without touching anything, matching the existing session-rewrite tests. I confirmed it's a real guard — flipping the opt-out check off makes the skip test fail, and it passes again with the check in place.

The branch is 77 commits behind main but still merges cleanly; happy to rebase whenever you'd like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants