Implement transfer job models - #290
aaronjae22 wants to merge 9 commits into
Conversation
| copied and what remains to be fetched." | ||
| """ | ||
|
|
||
| class Mode(models.TextChoices): |
There was a problem hiding this comment.
I don't think you're going to need Mode. How would a TransferJob behave any differently if it's loopback?
There was a problem hiding this comment.
The idea of it was a bit silly but I wanted to store what mode trigger the data transfer.
And yes, there won't be any differences between the loop and external. The loopback idea is two independent servers interacting to each other and that would be the same regardless of the mode.
If we ever want to label a loopback run we could do a simple check like source_base_ur == settings.BASE_URL
| AUTHORIZING = "authorizing", "Awaiting authorization" | ||
| FETCHING = "fetching", "Fetching collections" | ||
| PAUSED = "paused", "Paused (rate limited)" | ||
| DRY_RUN_COMPLETE = "dry_run_complete", "Dry run complete" |
There was a problem hiding this comment.
This is mixing two kinds of things: status with write policy. Maybe COMPLETE is COMPLETE (one status instead of two) and the fact of dry run is a write policy.
There was a problem hiding this comment.
I completely agree with this
| entry = collections.setdefault(collection, {}) | ||
| entry.update(values) | ||
|
|
||
| job.progress = progress |
There was a problem hiding this comment.
This doesn't do anything because progress hasn't been changed yet
There was a problem hiding this comment.
You're right and while working on it I found that because the merge lands in place, job.progress changes before the save. If the save then fails, the object holds a resume position the row doesn't have so the process thinks a page is recorded while the database doesn't, so the next request starts that collection over and reimports what it already imported.
test_a_failed_save_leaves_memory_and_row_in_agreement asserts job.progress == row.progress and job.updated_at == row.updated_at, then checks the collection reads empty from both.
| return dict(entry) | ||
|
|
||
|
|
||
| def clear_progress(job): |
There was a problem hiding this comment.
This is probably a lot harder to manage than simply making a new TransferJob when you want to do the equivalent of clearing progress. if you have a clear_progress function, then there are all kinds of clear_progress cases to test that can mess with status and saved state. but you don't need it.
There was a problem hiding this comment.
I agreed and I deleted it. Restarting is a new TransferJob now. Clearing left progress beside a stale token, artifacts and state and every combination of those was a thing to reason about
| super().save(*args, **kwargs) | ||
|
|
||
|
|
||
| class TransferredItem(models.Model): |
There was a problem hiding this comment.
This is a lot of extra code for a "dry run" feature that could be just a flag and a check before save
There was a problem hiding this comment.
dry_run now became a flag and a check before save now, and and Outcome.PENDING also was deleted with it.
I would like to keep the table for two things without a flag-shaped alternative, §7.1.1 (MUST) wants a new object ID per copy, and source_id beside destination_id is the only evidence we did it; §8.1 wants a per-object report.
There was a problem hiding this comment.
This is just a comment:
What you're reading into section 8.1 is much more than what's there; not even a production-quality LOLA implementation needs to save a separate record of every object transfered status in ADDITION to saving every object transfered itself.
This may be a YAGNI situation, code that you haven't used yet (I don't believe there's logic yet to create TransferredItem objects automatically within the transfer job progress) and when you do try to use it find that there are easier options than what you've put in the db here. For example, it occurs to me that each object successfully created in a LOLA transfer already has its source_id, it's supposed to save that for use in in "previously"
A full extra reference list of things copied may be useful for debugging so this is just a FYI comment.
There was a problem hiding this comment.
I thought about this and its alternatives and even though TransferredItem could be useful for detecting failures and skips I opted for dropping it. It could be added later on but I think the best and most efficient way to handle the transfer would be directly from just TransferJob. A successfully copied object should carries the mapping. Its own id is the new one and previously holds the old one.
a663420 to
353d85f
Compare
| _PROGRESS_UPDATE_FIELDS = ["progress", "updated_at"] | ||
|
|
||
|
|
||
| def get_collection_progress(job, collection): |
There was a problem hiding this comment.
This looks like it should be an instance method on a job so you could call
job.get_collection_progress(collection)
Is collection a string?
| return dict(entry) | ||
|
|
||
|
|
||
| def set_collection_progress(job, collection, **values): |
There was a problem hiding this comment.
again could be an instance method
There was a problem hiding this comment.
I agreed with both but I Ieft the implementation on jobs.py and TransferJob has small wrappers that call them.
| try: | ||
| job.save(update_fields=_PROGRESS_UPDATE_FIELDS) | ||
| except Exception: | ||
| job.progress = previous_progress |
There was a problem hiding this comment.
Why is reverting the job.progress or job.updated_at needed if the job is not saved? If the save fails then the next time this job is loaded from the DB it will be a new python instance and will not have the progress/updated changes.
If there's a save going on elsewhere, we should look at that.
If a revert is really needed, there's functionality for that that doesn't require saving "previous_progress" or "previous_updated_at": https://docs.djangoproject.com/en/6.1/ref/models/instances/#refreshing-objects-from-database. (probably just the 'del' field approach which isn't even a db hit)
There was a problem hiding this comment.
On if there's a save going on elsewhere, there will be one which is advance()'s failure handler that will catch the error and save the job to record it, and a simple save() there would write the progress that just failed along with it.
| model = TransferJob | ||
|
|
||
| destination_actor = factory.LazyFunction( | ||
| lambda: UserWithActorsFactory().actors.get(role=Actor.ROLE_DESTINATION) |
There was a problem hiding this comment.
Is this doing a query of actors that have the role? That's not what I'd expect from in a factory - I would think that instead the TransferJobFactory would call UserWithActorsFactory(role=Actor.ROLE_DESTINATION) to set the role on the new user, or set the role after calling the factory, or something lie that.
There was a problem hiding this comment.
The factory queries because the post_save signal on User already created both actors by the time the factory gets control. This is something I'll work around later on
| return obj.user | ||
|
|
||
|
|
||
| @admin.register(TransferredItem) |
There was a problem hiding this comment.
Did you mean to expose this in the admin both as a top-level item and as a Inline inside TransferJob?
There was a problem hiding this comment.
Yes, but I'll just keep TransferJob for now.
| super().save(*args, **kwargs) | ||
|
|
||
|
|
||
| class TransferredItem(models.Model): |
There was a problem hiding this comment.
This is just a comment:
What you're reading into section 8.1 is much more than what's there; not even a production-quality LOLA implementation needs to save a separate record of every object transfered status in ADDITION to saving every object transfered itself.
This may be a YAGNI situation, code that you haven't used yet (I don't believe there's logic yet to create TransferredItem objects automatically within the transfer job progress) and when you do try to use it find that there are easier options than what you've put in the db here. For example, it occurs to me that each object successfully created in a LOLA transfer already has its source_id, it's supposed to save that for use in in "previously"
A full extra reference list of things copied may be useful for debugging so this is just a FYI comment.
Closes #289
Original approach
This PR sets the foundation for the data transfer implementation. It adds two models that provides a transfer persisted and state that we can inspect.
As mentioned in the issue, a LOLA transfer cannot be completed inside one HTTP request, for two reasons:
The OAuth approval depends on a human click. The user leaves for the source's consent screen and comes back on a different request, which carries
code,state, andactivitypub_actor. We need to know which transfer is this, who it belongs to, into which actor, etc.A single request shouldn't walk a whole collection. Dockerfile runs
-workers 1 --threads 8. In Mode C one transfer occupies two threads at once, the advancing request plus the collection request it is waiting on and degrades into a hang rather than an error.A transfer that runs as one long request gets killed when it exceeds the timeout, during mid-copy, with no clean resume due to Cloud Run's request timeout.
So a transfer is many requests, and the database is the only thing that carries state between them. LOLA §6.7: "The destination server has more state to maintain to keep track of what has been already copied and what remains to be fetched."
Without these rows there is no dry-run, no resumption after a 429, no §8.1 result report, and no §7.1.1 old→new ID trail.
--
TransferJobstores one row per attempted account copy. The Owner (user), the destination Actor it writes into, the source it pulls from (source_base_url, source_actor_url), the actor the source actually authorized (authorized_actor_url from §5.3's activitypub_actor), mode (loopback | external), status across the nine target states, and four state-carrying columns: policy, progress, artifacts, error.TrnasferredItemstores one row per object the job touched: collection, source_id, destination_id, object_type, outcome, detail. This is both the §8.1 result report and the old to new ID mapping trail.A transfer becomes a row, resumable across requests, and reportable afterwards. Everything that survives between requests has to live somewhere.
TransferJobis the run essentially, what we are copying, from where, under what policy, how far we got.TransferredItemis the ledger, one row per object, what its old ID was, what its new ID is, and what happened to it.TransferJobacts as the record therefore it won't write anything, the implementation ontransfer/storage.pywill. PortabilityOutbox already exists for every destination Actor:Actor.save()callsinitialize_actor()on create and storage adds to an existing outbox rather than making one.A dry-run is not a simulation that throws its results away. It runs the entire transfer for real, up to but not including the writes, it fetches every page, transform every object, decide every new ID, decide every outcome — and then records all of that in TransferredItem rows instead of in Note, CreateActivity, Following and Blocked rows.
TransferJobis one row that mutates through the run.TransferredItemis a growing set of rows, each of which appears at a specific moment and then changes at most twice.I know that it could seems like there are a couple of things that we are overdoing but I believe this is the shape that we should follow. We can iterate over a few things along the way but this tries to follow the LOLA spec in the best possible way and I think its easier if we just get rid of a few thing as we move forward than trying to redo a bunch of stuff that we ommit.
States and their meaning
I decided to go with
TextChoicesinstead of the codebase existing pattern. I think is easier to just update the previous one but more complicated to follow/carry the current pattern into this state/status definition.Both models, request by request
Request 1 - POST /transfer/ - start
Request 2 - advance() - discovery
Two HTTP calls out: RFC8414 metadata, then the public Actor.
Discovery produces artifacts, not items. Nothing has been copied yet, so there is nothing to record an outcome for.
Request 3 - GET /transfer/callback?code=…&state=…&activitypub_actor=…
Request 4 - advance() - authenticated Actor re-fetch
The Actor is fetched again, this time with the bearer token, so it now carries migration.*.
Both Actor fetches are kept as separate artifacts. The difference between them is the evidence that the scope gate works.
Request 5 - advance() - content page 1
And so on until we reach to the commit
advance()which will persist into the destination actor one collection per call.In
transfer/jobs.pyI added three functions that are responsable for making oneadvance()call able to continue what the previous one started.getandsetare two parts of one cycle and are always used together.clearexists for when the user wants to start the job all over from the beginning. This is an user decision, not an error path.advance() #5 after discovery, etc:
For now, we're doing the transfer core with no user-facing UI so the admin models registration are the primary observation tools for now.
These admin classes make a transfer visible while it runs:
TransferJobAdmin: the list of transfers, and one transfer's detail page. Shows who started it, which server it is pulling from, and what state it is in.TransferredItemInline: the per-object results table, shown inside a job's page. One row per object the transfer touched.TransferredItemAdmin: the same rows as a standalone list, for questions that span jobs ("show me everything that failed").Reworking the transfer models agains the review
This branch has been rewritten. I am keeping the same two models with some changes on them. We are not storing a fact that we can not derive from the data we are already storing. That’s why
TransferJobnow we removed a column, the six status values, a FK, a validator and asave()overrides.This is how
TransferJobnow looks:Ownership is a single link. The job points at a destination
Actor, anduseris a property reading through it. There's nousercolumn, so a job whose owner disagrees with its actor's owner can't be expressed — which removed the validator that used to check for exactly that, and thesave()override that existed only to run it. The admin follows the same path (destination_actor__userinsearch_fieldsandlist_select_related), since a property can't appear in an ORM lookup.Lifecycle is three values, not nine. The working phase — discovering, waiting on a human, fetching — is derived from the job's own data instead of stored. No endpoints recorded means discovery hasn't run. Endpoints but no token means the user is still on the source's consent screen. Collections remaining in
progressmeans fetch the next one. All of those describe a job that is stillactive; they only say where inside it we are. The one that ends it is having no collections left.The three that survive are there because a computed property can't be indexed or filtered, and both the admin list and the resume query need that. They're a cache:
advance()recomputes the phase from the data and refreshes them, so if the two ever disagree, the data wins.Pausing is a timestamp, not a state.
retry_whenis a real column set from aRetry-After, and a job is paused precisely while that time is in the future. So "which jobs can resume now?" is one queryset —filter(state="active", retry_when__lte=now())— served by an index on(state, retry_when), equality column before range column. Buried in the progress JSON it would have been a table scan. It also fixes something a storedpausedvalue couldn't: a paused job whose delay has elapsed reads as ready immediately, with nothing needing to run to change it.Dry-run is policy, not a state. There's one terminal success state; whether a run wrote anything is
policy["dry_run"], chosen when the job is created. The column defaults through a named callable returning{"dry_run": True}, so a job created without an explicit policy is a dry run — a missing or unreadable policy resolves to the safe answer rather than the destructive one.One error, and the database insists on it.
errorholds the single terminal reason the run stopped: source unreachable, authorization revoked, retry cap exceeded. A job hits one fatal thing and stops, so that genuinely is one value. A check constraint refuses afailedrow with an empty reason, so a failure can't be recorded without saying why. Per-object failures don't live here — they belong to the ledger, which can hold as many as a run produces.TransferredItembarely changed. One row per object the run touched, carrying the object's ID on the source, the new ID created for it here, what happened to it, and why.Two obligations justify it and neither has a flag-shaped alternative. §7.1.1 requires a new object ID for every copy, and
source_idbesidedestination_idis the only evidence we obeyed it. §8.1 makes per-object success and failure reporting the destination's responsibility so it has to survive the request somewhere the UI can reach.Second Iteration
TransferredItemwas dropped. A successful copied object carries the id mapping itself which are its ownid(new one) andpreviouslywhich holds the old one.Actually, LOLA has an example on this (§7.1.8) that shows exactly that on a copied Like.
TransferredItemwas useful for skipped or failed object which produces no destination row, so there is no breadcrumb and nothing records that we saw it. Maybe we could find a workaround along the way.The progress accessors are now reached through the job. They are thin wrappers in
TransferJobbut the implementation stays injobs.py. Probably I'll go back to this onceadvance()is implemented.Fix collection approach. A typo was pretty much able to write progress under a key nothing reads. I moved the Collection vocabulary from
TransferredItemtojobs.py. Both accessors validate against it, reads included.The failed-save revert is removed.