Skip to content

Throttle qBittorrent when watching Plex, Jellyfin, or Emby remotely - #1355

Open
davidnewhall wants to merge 6 commits into
mainfrom
dn2_qbit_throttle
Open

davidnewhall wants to merge 6 commits into
mainfrom
dn2_qbit_throttle

Conversation

@davidnewhall

@davidnewhall davidnewhall commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Add pkg/triggers/qbitlimit to turn qBittorrent alternative (turtle) speed limits on while Plex WAN movie/episode sessions are active, or when the website signals Jellyfin or Emby playback.
  • Restore turtle mode only if this client enabled it, after the website cooldown.
  • New trigger: GET /api/trigger/qbitspeed/{enable|disable} (Jellyfin/Emby) or empty to reconcile.
  • Pins golift/qbit#41 for the speed-limit APIs.
  • Fixes #1212 on the client side. Website UI is a separate PR.

Test plan

  • Enable actions.qbitThrottle from the website (or a mocked ClientInfo), with qBit configured.
  • Start a Plex WAN movie/episode session and confirm turtle turns on; stop it and wait cooldown, turtle turns off only if we turned it on.
  • curl .../api/trigger/qbitspeed/enable then /disable with jellyfin or emby enabled.
  • If turtle was already on, stopping playback leaves it on.

The website configures when to enable alternative speed limits; the client owns turtle state so we do not steal a user-enabled limit.

Co-authored-by: Cursor <cursoragent@cursor.com>
@golift-bot
golift-bot requested a balanced review from Copilot September 2, 2026 07:56
@davidnewhall davidnewhall added enhancement New feature or application improvement upstream Requires dependency updates premium This request supports a premium feature project Could be part of a project labels Sep 2, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Failed operations and configuration transitions can strand turtle mode, while the API can report success for ignored requests.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds automatic qBittorrent turtle-mode throttling for remote Plex and Jellyfin playback, addressing issue #1212.

Changes:

  • Adds configurable Plex/Jellyfin throttling with cooldown and instance selection.
  • Introduces the qbitspeed trigger and Plex session callbacks.
  • Updates the qBittorrent dependency and adds unit tests.
File summaries
File Description
pkg/website/clientinfo/clientinfo.go Defines website throttle configuration.
pkg/triggers/triggers.go Wires the throttle action into triggers.
pkg/triggers/qbitlimit/qbitlimit.go Implements reconciliation and ownership logic.
pkg/triggers/qbitlimit/qbitlimit_test.go Tests session detection and planning helpers.
pkg/triggers/plexcron/sessions.go Signals Plex session updates.
pkg/triggers/plexcron/plexcron.go Adds session-update callback registration.
pkg/triggers/handler.go Exposes the qBittorrent speed trigger.
go.mod Updates the qBittorrent dependency.
go.sum Records updated dependency checksums.
Review details

Suppressed comments (2)

pkg/triggers/qbitlimit/qbitlimit.go:200

  • Ownership is updated before the state-changing call succeeds. If a disable attempt fails, weEnabled becomes false; the next reconcile sees turtle mode still on and treats it as user-owned, so it is never restored. Commit the new ownership state only after SetSpeedLimitsModeContext succeeds.
		turtle, weOwn, skip := planTurtle(current, desired, c.weEnabled[instance])
		c.weEnabled[instance] = weOwn

pkg/triggers/qbitlimit/qbitlimit.go:189

  • An instance removed from cfg.Instances is skipped even when weEnabled[instance] is true. A website reconfiguration can therefore strand turtle mode on a formerly selected instance. Reconcile owned-but-unselected instances with a per-instance desired state of false instead of skipping them.
		if !app.Enabled() || (len(cfg.Instances) > 0 && !cfg.Instances.Has(instance)) {
			continue
  • Files reviewed: 8/9 changed files
  • Comments generated: 3
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread pkg/triggers/handler.go
Comment on lines +546 to +563
func (a *Actions) qbitspeed(input *common.ActionInput, content string) (int, string) {
if content != "" {
input.Args = []string{content}
}

if !a.QbitLimit.Send(input) {
return http.StatusNotImplemented, "qBittorrent speed limit is not enabled."
}

switch content {
case "enable":
return http.StatusOK, "qBittorrent alternative speed limits enable requested."
case "disable":
return http.StatusOK, "qBittorrent alternative speed limits disable requested."
default:
return http.StatusOK, "qBittorrent alternative speed limits reconcile requested."
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in e52aa98. Mode is validated first; anything other than empty/enable/disable is 400 and does not queue.

Comment on lines +64 to +66
info := clientinfo.Get()
if info != nil && info.Actions.QbitThrottle.Enabled && c.hasQbit() {
dur = pollInterval

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in e52aa98. If the feature is off but weEnabled still has instances we own, Create keeps the 15s timer and queues a restore; reconcile calls apply(..., false) instead of returning. Owned-but-unselected instances restore the same way.

Comment thread pkg/triggers/qbitlimit/qbitlimit.go Outdated
Comment on lines +91 to +94
// Send queues a reconcile. Optional args: "enable" or "disable" (Jellyfin).
func (a *Action) Send(input *common.ActionInput) bool {
return a.cmd.Exec(input, TrigQbitSpeed)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in e52aa98. Send now checks that the feature is enabled and a qBit instance exists, so the documented 501 is reachable.

Comment thread pkg/triggers/qbitlimit/qbitlimit.go Outdated
}

turtle, weOwn, skip := planTurtle(current, desired, c.weEnabled[instance])
c.weEnabled[instance] = weOwn

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Blocking. This clears the ownership record before the write is confirmed. If SetSpeedLimitsModeContext below fails — qBittorrent restarting behind a reverse proxy, a 502, a failed login — we drop weEnabled[instance] to false while the instance is still in turtle mode, and every later reconcile then takes the !weEnabled skip in planTurtle. The throttle becomes permanent with no retry and no way back short of a manual toggle.

I reproduced it against an in-package harness with a fake qBit WebUI that 500s a single toggle: after playback ends and the toggle fails, turtle stays 1 and two further reconciles issue zero writes.

Suggest confirming before bookkeeping:

turtle, weOwn, skip := planTurtle(current, desired, c.weEnabled[instance])
if skip {
	c.weEnabled[instance] = weOwn // only the no-op paths are safe to record now.
	continue
}

if err := app.SetSpeedLimitsModeContext(ctx, turtle); err != nil {
	mnd.Log.Errorf(...)
	continue // keep ownership so the next tick retries the restore.
}

c.weEnabled[instance] = weOwn

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed — that ordering was a real strand. weEnabled is only written after SetSpeedLimitsModeContext succeeds; a failed restore keeps ownership so the next tick retries.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verified at e52aa98 with the 502-once fault harness again: the failed restore now keeps weEnabled, the next tick retries and clears turtle, and the idle tick after that writes nothing. Same harness still fails at c95b1e8, so the ordering change is what fixed it. Good to go.

Comment thread pkg/triggers/qbitlimit/qbitlimit.go Outdated
defer c.mu.Unlock()

info := clientinfo.Get()
if info == nil || !info.Actions.QbitThrottle.Enabled || !c.hasQbit() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Two more paths end with the box throttled forever, both from the same "we only restore what we enabled" rule:

  • Turning the feature off (website toggle, or qbitThrottle.enabled: false) takes this early return on every tick, including while c.weEnabled says we hold the enable. watchChan re-runs Create() on the existing Action, so the map survives — the info is right here, it's just never acted on. Restoring our own instances before returning costs ~4 lines.
  • A process restart (auto-update counts) during remote playback loses weEnabled entirely, so the next desired=false skips and turtle stays on. Jellyfin is the worst case: jellyfinUntil is also in-memory, so after a restart nothing wants turtle but nothing restores it either.

For the restart case I get why you don't want to steal a user's own turtle. Would it be worth an opt-in in QbitThrottleConfig ("take over turtle mode if it's already on"), or at minimum an INFO line the first time we find turtle on and decide not to own it? Right now the state is invisible in the logs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Disable-while-owned now restores (no cooldown) and keeps the timer until that happens.

No take-over field: the plan is not to steal a turtle we did not enable, and a restart still loses weEnabled / the Jellyfin TTL. We log once the first time we find turtle already on and leave it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Disable-while-owned checks out — ran it against a fake qBit WebUI and the restore write lands, with the timer held at 15s until weEnabled empties.

One gap in the log line: reconcile returns at the lastDesired.IsZero() gate before it ever reaches apply, so a process that restarts into "turtle already on, nothing wants it" never issues the GET and the INFO never prints — 40 ticks in my harness, zero requests to qBit. It only appears once something actually wants turtle. Fine to leave it; flagging it so the log isn't counted as covering the restart case.

Comment thread pkg/triggers/qbitlimit/qbitlimit.go Outdated
instance := idx + 1
app := &c.Apps.Qbit[idx]

if !app.Enabled() || (len(cfg.Instances) > 0 && !cfg.Instances.Has(instance)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Question on the contract, since the website UI lands separately: an empty Instances list throttles every qBit instance here, while the other website-driven instance filter does the opposite — pkg/triggers/cfsync/radarr.go:55 bails out on len(...) < 1, so an empty list means "nothing". If the UI lets someone enable the feature without ticking an instance, this client throttles all of them. Which one is intended?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Empty Instances is every qBit instance here on purpose. The website v1 UI has no instance picker, so the payload is always empty. Comment added on the check; not matching cfsync's empty-means-nothing.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Works for me — empty meaning "all" is fine when v1 has no picker, and the comment at the check settles it for the next reader.

return false
}

return session.Player.State == playing || session.Player.State == paused

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is counting paused deliberate? A remote player that pauses and gets left open holds turtle mode indefinitely — checkSessionDone in pkg/triggers/plexcron/finished.go:68 treats anything that isn't playing as ignoring, paused. If you want paused to count, it may deserve a max-pause age (the Player.StateTime the session tracker already fills in would do it).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Paused is deliberate — the plan counts playing or paused WAN movie/episode sessions. No max-pause age; the viewer is still in the stream. Comment added.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good — that is what the code says now, and the comment answers it for the next reader.

@qwen-pr-bot qwen-pr-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed c95b1e8 against fdf9ac2 — 9 files. The pull request contains five changes: the new pkg/triggers/qbitlimit package, the SetSessionUpdated hook in plexcron, the GET /api/trigger/qbitspeed/{mode} endpoint, QbitThrottleConfig in clientinfo, and the golift.io/qbit bump that carries the speed-limit API.

The design holds up. planTurtle as a pure function with a table test around it is the right shape — "don't steal a turtle we didn't light" is the correct rule for touching a user's download client — and Kick being a non-blocking send is not just tidy, it's required: plexSessionTracker runs both in the shared action loop and in the webhook goroutine while holding the plexcron mutex, so a blocking send there would wedge Plex sessions behind qBittorrent.

What blocks me is one ordering bug and the family of cases where turtle never gets turned back off. qBittorrent keeps that state across restarts, so every one of them ends with a user's downloads capped until they notice and click the turtle themselves — which is the complaint #1212 was meant to fix. Details on qbitlimit.go:200 and :118.

Executed validation. Nothing here is static-only:

  • go build ./... and go vet ./... clean at c95b1e8 (I dropped a placeholder file in frontend/dist/ to satisfy the embed; no frontend code touched or judged).
  • go test -race -count=1 ./... — every package passes, including the new pkg/triggers/qbitlimit tests.
  • Public CI at this head is green: golangci-lint (linux, darwin, freebsd, windows), gotest (ubuntu, macos, windows), codespell, check. Fetched from the API, not run here.
  • Throwaway in-package harness (uncommitted, lived in the disposable checkout) driving cmd.reconcile with real apps.New() clients against a fake qBittorrent WebUI (speedLimitsMode GET, toggleSpeedLimitsMode POST) and a fake Plex /status/sessions, with clientinfo and plexCurrentSessions served through the data cache: WAN playing movie turns turtle on with exactly one toggle write and the next tick writes nothing; LAN-only session writes nothing; restore fires only after the cooldown and clears ownership; Instances: [2] touches only instance 2; an empty list touches both; enable/disable drive the mode and the TTL holds it. Reproduced all three orphan paths from the comments — failed toggle write, feature disabled while we own the enable, and fresh state finding turtle already on — plus a hung qBit instance with timeout unset staying inside reconcile for the whole test window.

Non-blocking, take or leave:

  • The timer is fixed at 15s and unreadable from the website: one mode read per eligible instance per tick, ~4 requests/minute/instance, forever. It also isn't Hide: true, so runEventAction writes an INFO Event Triggered line every 15 seconds — plexcron.go:106 sets Hide on the 1-minute session tracker for exactly this reason.
  • reconcile runs on the action loop's context with no deadline, and QbitConfig.Setup (pkg/apps/integrations.go:259) doesn't default a timeout the way PlexConfig.Setup does (:35). The [[qbit]] block in examples/notifiarr.conf.example ships no timeout, so the http.Client gets Timeout: 0. A qBittorrent WebUI that accepts the connection and stalls then holds the single shared action loop open indefinitely, with dashboard, Plex sessions, and every other trigger queued behind it. A context.WithTimeout around the body of reconcile caps that.
    Copilot's run landed while I was reading and reached the same conclusion on the disable path and on the endpoint's 200/501 wording; my comment on :118 is the same finding with the restart and Jellyfin cases added, so treat those as two independent reviewers agreeing rather than extra work.

One small thing of my own: jellyfinTTL is a fixed 4h while cooldown is configurable. Fine if deliberate — a missed disable call caps the damage at 4h instead of forever.

…isable.

Failed toggles no longer drop weEnabled, so the next tick retries. Turning the feature off restores instances we still own, and the HTTP trigger returns 400/501 instead of silently reconciling.

Co-authored-by: Cursor <cursoragent@cursor.com>
@davidnewhall

Copy link
Copy Markdown
Contributor Author

Pushed e52aa98 for the Copilot/Qwen review notes.

Take-or-leave items from the Qwen review:

  • 15s timer is Hide: true now so it does not spam Event Triggered.
  • reconcile has a one-minute deadline, and QbitConfig.Setup defaults a zero timeout to one minute (same as Plex/Tautulli).
  • jellyfinTTL stays a fixed 4h; a missed disable caps the damage instead of leaving turtle on forever.

#1355

@qwen-pr-bot qwen-pr-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Done — the comment settles it.

Executed validation: ran go test -race ./pkg/triggers/qbitlimit/ at e52aa98 (pass). Read the new apply/planTurtle/instanceDesired ordering and the qbitspeed mode validation; the strand-on-failed-toggle and the disable-while-owned restore are both fixed, so my block is cleared.

@qwen-pr-bot qwen-pr-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approval — the two paths I flagged are closed, and I checked the fix rather than taking the reply on faith.

Executed validation (at e52aa980; go vet clean, go test -race ./pkg/triggers/... ./pkg/website/... passes — the pkg/triggers/* packages need a frontend/dist directory to satisfy the embed, unrelated to this PR):

  • Disable-while-owned: with weEnabled[1]=true, turtle on, and QbitThrottle.Enabled false, Create() keeps the timer at 15s, queues a restore, and the first reconcile issues the toggle — turtle off, ownership cleared, no cooldown in the way. Matches what you described.
  • Failed restore: with the toggle endpoint returning 500, weEnabled[1] survives and the next apply restores it. The strand-on-error from my earlier comment is gone.
  • Unowned turtle: the INFO prints exactly once and no write is issued.

The one thing that doesn't hold is the log covering a restart. reconcile returns at the lastDesired.IsZero() gate before it reaches apply, so a process that comes back into "turtle on, nothing wants it" issues zero requests to qBit and prints nothing — 40 ticks in my harness, not even a GET. Left it as a note in the thread, not a block: not stealing a turtle you didn't enable is your call, and the disable path was the real defect.

The website sends the same enable/disable trigger; the client now treats the Emby flag as another source for that TTL so turtle mode works without stealing user-enabled limits.

Co-authored-by: Cursor <cursoragent@cursor.com>
@davidnewhall davidnewhall changed the title Throttle qBittorrent when watching Plex or Jellyfin remotely Throttle qBittorrent when watching Plex, Jellyfin, or Emby remotely Sep 2, 2026
Comment thread pkg/triggers/qbitlimit/qbitlimit.go Outdated

cfg := info.Actions.QbitThrottle
if !cfg.Enabled {
c.apply(ctx, input, cfg, false)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Non-blocking, but this branch never goes quiet. Once the restore lands, nothing disarms the 15s timer create() armed for the owned case, and Kick() keeps queueing on every Plex sessions fetch — so with the feature off we still pay a GET /api/v2/transfer/speedLimitsMode per instance. Measured: 4 reconciles against a fake WebUI with QbitThrottle.Enabled=false and ownership already cleared produced 4 GETs, and no answer to those GETs can cause a write. Same shape if the instance we own is removed from config: apply never visits that index, so the stale weEnabled entry keeps hasOwned() true, and that timer survives every reconfiguration until the process restarts.

An ownership guard makes the idle path free:

if !cfg.Enabled {
    if c.owned() { // unlocked variant; reconcile already holds c.mu
        c.apply(ctx, input, cfg, false)
    }

    return
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 7408e4f. Disabled reconcile only calls apply while hasOwned() is still true, so idle ticks after restore make no GETs. Kick also stays quiet when the feature is off (and does not take the mutex — GetSessions can call it while reconcile holds it).

Removed or disabled instances drop out of weEnabled so a stale key cannot keep the restore timer armed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Checked at 7408e4f against a fake WebUI: feature off with nothing owned, 4 reconciles produce 0 requests to qBit; owned + off produces exactly one toggle (2 GETs — the second is go-qbit's get-then-toggle read) and then goes quiet; a removed or URL-less instance drops out of weEnabled before hasOwned() is consulted. Kick() returned immediately with c.mu held, so the no-lock claim holds too. go test -race clean on the package.

The 15s ticker create() armed for the owned case does keep ticking after the restore lands, since nothing re-runs create(), but each tick now returns at the hasOwned() guard for free, so I'm fine with it.

@qwen-pr-bot qwen-pr-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Both things I asked for are done, and I checked them by running the code rather than only reading it.

Ownership now survives a failed write: against a fake qBit WebUI that 500s exactly one restore, turtle stays on, weEnabled[1] stays true, and the next reconcile completes the restore and clears the flag. Same harness against c95b1e88 ends with turtle on, ownership gone, and zero further writes — so the bug I flagged is closed, not just papered over.

Disabling the feature also restores what we own, including an instance dropped from Instances mid-flight, and a user's own turtle mode stays untouched with the new INFO line. I had this as "opt-in or at minimum an INFO line" — the INFO line is the right call for now.

One non-blocking note inline on the disabled branch: that path keeps hitting qBittorrent after the restore lands.

Executed validation at e52aa980 (base fdf9ac2f, delta c95b1e88..e52aa980):

  • go build ./... and go vet ./pkg/triggers/... ./pkg/apps/... clean (needed a scratch frontend/dist/ file for the embed pattern).
  • go test -count=1 ./pkg/... passed all packages; go test -race -count=1 ./pkg/triggers/qbitlimit/... passed.
  • Disposable in-package harness driving reconcile and create() against an httptest qBit WebUI: failed-restore retry, restore-on-disable, unowned-untouched, deselect-while-owned restore, plus a create() timer/queue table and an idle-tick HTTP count. Reproduced the failed-restore failure mode on c95b1e88 with the same harness.
  • pkg/triggers harness calling qbitspeed directly: enabel → 400, ENABLE → 400, enable and empty with the feature off → 501.
  • CI at this head is green (gotest ×3, golangci-lint ×4, codespell, check) per the GitHub API. golangci-lint itself I could not run locally — the pinned config wants a newer version than I installed.

Comment thread pkg/triggers/qbitlimit/qbitlimit.go Outdated
}

func (c *cmd) desired(ctx context.Context, cfg clientinfo.QbitThrottleConfig, now time.Time) bool {
if (cfg.Jellyfin || cfg.Emby) && !c.jellyfinUntil.IsZero() && now.Before(c.jellyfinUntil) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

New consequence of this commit: Jellyfin and Emby share one jellyfinUntil, and /trigger/qbitspeed/{mode} carries no source app, so a disable from either one releases the throttle for both. Confirmed with a throwaway harness — both flags on, enable then disable — turtle went back off. It fails toward a buffer stall rather than a stranded throttle, so not a block. If the website can tag the trigger (/qbitspeed/emby-disable) or keep per-source TTLs, this is the line where it lands.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Intentional. The website decides whether Jellyfin or Emby should fire qbitspeed; the client only sees generic remote playback (enable/disable) plus local Plex WAN. One TTL is the right model — we are not tracking per-app sessions, so the last Stop from either app turning turtle off is expected. Tagged modes / per-source TTLs would pretend we have that data.

3d7c34e drops the unused jellyfin/emby client flags and logs “remote playback” instead of a source app.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Makes sense — one TTL for a source-agnostic trigger is the honest model, and dropping the dead client flags in 3d7c34e removes the illusion that we track per-app sessions. Verified the delta builds and the qbitlimit tests pass. Thread resolved from my side.

@qwen-pr-bot qwen-pr-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Delta c95b1e8..c0210f4. Both things I blocked on are closed — covered in the threads on :200 and :118, so nothing to restate.

Emby is wired correctly. Read create, ready, applyJellyfinArg and desired against the new flag and drove them: with Jellyfin off and Emby on, a single enable turns turtle on with exactly one toggle write and records ownership; a plain timer tick holds the throttle with zero extra writes; disable restores and clears ownership; and with both flags off the arg is ignored entirely, zero writes. The two boolean ORs are the whole change, and Emby needs nothing in ready() — it gates nothing there that Enabled doesn't already cover.

Non-blocking note inline on the shared Jellyfin/Emby TTL. The idle-tick note on :169 still stands; still not a block.

Executed validation at c0210f4 (base fdf9ac2, delta c95b1e8..c0210f4):

  • go build ./... and go vet ./pkg/triggers/... ./pkg/apps/... ./pkg/website/... clean. Needed a scratch frontend/dist/index.html to satisfy the embed pattern; no frontend code touched or judged.
  • go test -race -count=1 ./pkg/triggers/... ./pkg/apps/... ./pkg/website/... — all packages pass, including the new TestInstanceDesired.
  • Throwaway in-package harness (uncommitted, in the disposable checkout) driving cmd.reconcile against a fake qBittorrent WebUI: the four Emby cases above, plus both flags on with one disable (un-throttles both), plus a stale jellyfinUntil with both flags off (no writes, so no risk of a TTL outliving its flag). The first disable I ran appeared to do nothing; that was the 30s cooldown gate holding the restore, not a defect — it lands once the cooldown passes.

The disabled path only talks to qBit while we still own turtle, and we drop ownership of instances that were removed so the restore timer cannot stick forever.

Co-authored-by: Cursor <cursoragent@cursor.com>

@qwen-pr-bot qwen-pr-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Both items from my last pass are done. Verified with a throwaway harness (fake qBit WebUI) run against both commits: at c0210f4, 20 idle reconciles with the feature off cost 20 GETs and a disabled instance kept its ownership key; at 7408e4f, the same harness gets 0 GETs and the stale key is dropped. Kick behind ready() is the right gate, and it still takes no lock, so GetSessions → Kick inside a held c.mu can't self-deadlock.

Executed validation at 7408e4f: go build ./..., go vet ./pkg/triggers/..., go test -count=1 ./..., and go test -race ./pkg/triggers/... all pass — including harness cases for owned-restore-then-quiet, a disabled instance dropping out of weEnabled, and Kick hammered concurrently with reconcile. CI: gotest x4, golangci-lint x4, codespell all green.

Jellyfin and Emby checkboxes stay on the site; the client only needs enable/disable plus local Plex WAN.

Co-authored-by: Cursor <cursoragent@cursor.com>

@qwen-pr-bot qwen-pr-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Delta 7408e4f..3d7c34e reads clean — the shared-TTL model you described in the :206 thread is now the whole design instead of a condition, and dropping Jellyfin/Emby from QbitThrottleConfig leaves no dangling references anywhere in the tree.

Executed validation at 3d7c34e: go build ./... and go vet ./pkg/triggers/... ./pkg/website/... clean; go test -race -count=1 ./pkg/triggers/... ./pkg/website/... passes (pkg/triggers/qbitlimit, filewatch, pkg/website ok). Plus a throwaway in-package harness driving applyRemoteArg + desired: with Enabled: true and Plex: false, enable sets remoteUntil and desired returns true through the TTL boundary and false at exactly now+remoteTTL; disable clears it immediately; empty args and an unknown arg leave state untouched. The two new log lines fire with the right [api requested] prefix.

One thing outside the code: the pull request description still says "when the website signals Jellyfin or Emby playback" and the test plan says to fire /trigger/qbitspeed/enable "with jellyfin or emby enabled" — those config fields are gone now, so the wording will mislead whoever picks up the website-side PR. Worth a minute of editing before this merges.

Co-authored-by: Cursor <cursoragent@cursor.com>

@qwen-pr-bot qwen-pr-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Docs-only delta since 3d7c34e9; nothing blocking.

Executed validation (at 418761bf): regenerated both specs with the repo's own commands from frontend/generate.sh (go run github.com/swaggo/swag/v2/cmd/swag@v2.0.0-rc5 i --v3.1 ..., api and ui instances). api_openapi.json came out byte-identical to the committed file after canonical JSON sorting, so the two /trigger/qbitspeed* entries are faithful generator output for the annotations at pkg/triggers/handler.go:535-545. ui_openapi.json matched too except in cnfg.Duration and logs.FileMode, which this commit does not touch (local swag/Go version renders int64 bounds and enums differently). go build ./pkg/... clean; go test ./pkg/... -count=1 green, including the new TestNvidiaConfigHasID.

One note, not a request: pkg/snapshot/nvidia_test.go is unrelated to qBittorrent throttling — it covers the pre-existing NvidiaConfig.HasID. Fine to keep, just out of scope for this branch.

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

Labels

enhancement New feature or application improvement premium This request supports a premium feature project Could be part of a project upstream Requires dependency updates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FR] qbit speedlimit

2 participants