Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TransmissionBt client implementation #26

Merged

Conversation

zivkovic
Copy link
Contributor

Implementation of TransmissionBt client

@zivkovic zivkovic force-pushed the add_transmissionbt_client branch from a4a8a15 to 7f09b2f Compare October 12, 2024 15:57
@zivkovic zivkovic force-pushed the add_transmissionbt_client branch from 7f09b2f to fac78f3 Compare October 12, 2024 19:50
@moleculekayak
Copy link
Owner

Thank you for this!

I was busy over this weekend for holidays and now I'll be out of cell service for a week starting today. These changes look broadly good but I'll give them a proper review and see the extent to which I can test them once I'm back.

In the meantime, could you please keep using this new branch you've created and confirm that it works as expected over the next ~week?

@zivkovic
Copy link
Contributor Author

zivkovic commented Oct 14, 2024

Il try to set it up and test it for a while. Thanks.

Edit: quickly tested a couple (5 from RED, 2 from OPS) and all injected without any issues. Gonna have to build docker container and set it up on the server, since matching the file paths on my pc/server is a pita.

Edit: Docker container now running.

@zivkovic zivkovic force-pushed the add_transmissionbt_client branch from fac78f3 to 4e107d2 Compare October 17, 2024 19:29
@zivkovic
Copy link
Contributor Author

I have been testing this for about 5 days now (and will leave running, until this is released) and aside from one bug (session going stale, re-login, fixed already) everything works perfectly.

A small optimization for the library would be to not store "generated" torrent files if direct injection is enabled, since they are not used at all after injection.

@zakkarry
Copy link
Contributor

I have been testing this for about 5 days now (and will leave running, until this is released) and aside from one bug (session going stale, re-login, fixed already) everything works perfectly.

A small optimization for the library would be to not store "generated" torrent files if direct injection is enabled, since they are not used at all after injection.

Is this "session going stale" due to cookie expiration or a transient error? It's possible that if it's expiration, the expiration time is reported in the header if it is anything like other clients, in which case this can be anticipated and automatically handled. This is how authentication headers are generally handled anyway, and most cookies in general. Most only expire after a period of non-usage though and the cookie's expiration is continually pushed back the expiration period from last usage. Not sure how tm handles this, but figured I'd note that here, if nothing else for posterity.

I see you just relogin on an unauthenticated response (409), which should handle things fine. Weird status code for that though lol

I'm not sure if we faced similar with tm in cross-seed, but our user base is relatively small and that client implementation does go relatively untouched for the most part.

@zivkovic
Copy link
Contributor Author

I'm also using cross-seed and this implementation is based on that Yeah, the 409 status code for unauthenticated is weird. I was testing this though out the day and the first request that was sent to fertilizer worked, afterwards others did not. Since the error was clearly logged, I saw that the "X-Transmission-Session-Id" and "Authorization" headers were sent, but I still received 409. Since I clearly wasn't logged in anymore, I simply re-login.

Also I just checked, there are no expiration headers sent from transmission. Verified it by checking cross-seed source.

Another question for you @zakkarry, since you mentioned cross-seed.
Fertilizer currently only checks for possible cross-seed torrent after it was completed (onDoneScript). Which means if the torrent was transplanted/uploaded to red/ops at a later date, we would not have it added automatically. Now there are two options:

  • Use autobrr to notify cross-seed, which would then in turn notify fertilizer and check if newly uploaded torrent can be cross-seeded
  • Use autobrr to notify fertilizer and check if newly uploaded torrent can be cross-seeded

Both options would require changes to fertilizer, although I'm leaning more on option #2, as it seems simpler + less dependencies on other projects.

@zakkarry
Copy link
Contributor

zakkarry commented Oct 22, 2024

I'm not sure what the purpose of using cross-seed as an intermediary would provide, and I don't think we'd really be inclined to add a "forwarding" of API calls to cross-seed anyway. I also think you could achieve this with autobrr without cross-seed regardless. Not sure where cross-seed would need to be involved.

It would be relatively easy to implement either an RSS or API to do the same thing we do in cross-seed, however, the issue would be the matching algorithms, as names can vary slightly on RSS and announce, although this wouldn't really be game-breaking given that you only have one media type.

I'm not sure if that's something that @moleculekayak would be inclined to do over just having something that periodically searches, especially given the relative parity that red and ops have with uploads. I wouldn't be surprised if simply doing one search after download completes and then maybe one a day later caught the majority of the "misses".

@moleculekayak
Copy link
Owner

Hey there! I'm back and I'm just getting caught up on everything.

@zivkovic regarding option 2, I think the changes required would be quite small. Autobrr already has the ability to ping a webhook when a torrent is found and I think you could simply use their macros to send up {{.TorrentHash}} to your fertilizer instance. As far as I can tell the only change would be to ensure fertilizer can accept the JSON provided by autobrr.

Do you see any flaws in that logic? I'm working on a proof-of-concept as time permits but please let me know if I'm missing anything

@zivkovic
Copy link
Contributor Author

zivkovic commented Oct 22, 2024

I don't believe it would be that simple, would it?

The infohash sent from autobrr would not match the one you already have present in your torrent client. How would you then go matching this infohash to the actual torrent present in the client, since the infohashes do not match?
I don't think there is a simple way to do so, since you cannot decode infohash to its actual building blocks.

AFAIK cross-seed receives the below data as json from autobrr, which it then uses to search for torrent on other sites:

{
  "name": "{{ .TorrentName }}",
  "guid": "{{ .TorrentUrl }}",
  "link": "{{ .TorrentUrl }}",
  "tracker": "{{ .Indexer | js}}"
}

Just by using the new infohash would not be possible.

I might be wrong, this is just my understanding on how this works. If only infohash would be required, then cross-seed would use that, but as seen in above example it does not even use it.

Maybe @zakkarry could explain a bit more?

And to answer @zakkarry about using the search a day later. Yes, this would catch most of the releases, but you would miss the initial "boom" and you would miss the cross-seedable torrents which are transplanted at a later date (maybe even a year later). If the functionality to react to IRC (autobrr) is added, this would always catch all of them (unless IRC/autobrr/torrent client is offline). The last problem could easily be solved by running search periodically for all non cross-seeded torrents.

@zakkarry
Copy link
Contributor

zakkarry commented Oct 22, 2024

Gimme a bit just got out of the shower, but also the infohash would result in snatching the torrent to get the infohash, so that would be a non-starter as far as RED and OPS go anyway.

You'd need to do something off name matching via the announce or RSS feed if you're doing it from autobrr, there are no trackers that announce infohashes as far as i know.

I'll read and respond here in like 15.


Created an issue for this, as its not relevant to this specific PR. See mention below.

@moleculekayak
Copy link
Owner

@zivkovic coming back to this PR, can you confirm your testing has been successful and that the reauth-on-409 logic has worked as you've expected? I'm not normally a Transmission user, but if this is ready for testing then I'll complete a code review and see about setting up a Transmission instance locally for testing!

A small optimization for the library would be to not store "generated" torrent files if direct injection is enabled, since they are not used at all after injection.

This is personal preference, but I like to keep the torrent file regardless. If people don't want the files, they can set the output to something like /tmp or /dev/shm (or their OS' equivalent)

@zivkovic
Copy link
Contributor Author

Yes, everything is working, I think I'm up to about 85+ cross seeds so far.

@moleculekayak
Copy link
Owner

@zivkovic I've made some changes in these commits - namely running linting, writing some tests, and minor code updates. Would you mind reviewing them to ensure you're okay with these?

@zivkovic
Copy link
Contributor Author

Looks good to me.

@moleculekayak moleculekayak merged commit ca94825 into moleculekayak:master Oct 25, 2024
1 check passed
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.

3 participants