[bjester]

Summary

This change completes the streaming deserialization migration. It adds the sink, switches all callers to the new pipeline, and removes the legacy _deserialize_from_store path. Deserialization now runs as a stream, like serialization:

StoreModelSource → AppModelDeserialize → AppModelValidate → Buffer(500, by model) → DeserializeSink

• DeserializeSink writes the resolved outcome of each task, one chunk at a time:
  • SAVE: The sink saves each record in its own savepoint, with pre_save and post_save muted.
  • DELETE: The sink deletes the app rows with one Django Collector for each model in the chunk.
  • PROPAGATE_DELETE and PROPAGATE_HARD_DELETE: The sink adds the records to the tracking tables with one bulk_create(ignore_conflicts=True) for each table.
  • ERROR: The sink saves the error on the Store record.
• A clean write sets dirty_bit=False and clears the error fields on a Store record. The sink does the clean writes of a chunk with one QuerySet.update.
• The sink saves each Store error one record at a time. We measured bulk_update as slower than saves of single records.
• deserialize_from_store(profile, sync_filter=None, skip_errored=False) is the entry point. It has the same shape as serialize_into_store, and it logs a count for each outcome.
• deserialize_store_model(store, fk_cache, sync_filter=None) deserializes one Store record outside of a pipeline run. It returns the validated app model, or None for a deleted record.
• The sink and deserialize_store_model use the same write helpers, delete_app_models and record_propagated_deletions.

Benchmark results

I instructed Claude to run the same scenarios to benchmark memory and time performance, before and after the streaming conversion. The results are very promising!

  
  ┌──────────────────────────┬─────────┬─────────┬──────────┬──────────┐
  │           Case           │ Records │ Version │ Peak mem │ Duration │
  ├──────────────────────────┼─────────┼─────────┼──────────┼──────────┤
  │ 1: existing 18k users    │ 108,388 │ Before  │ 287.7 MB │ 93.16 s  │
  ├──────────────────────────┼─────────┼─────────┼──────────┼──────────┤
  │                          │         │ After   │ 100.3 MB │ 77.33 s  │
  ├──────────────────────────┼─────────┼─────────┼──────────┼──────────┤
  │ 2: generated 500 users   │ 184,369 │ Before  │ 754.7 MB │ 156.57 s │
  ├──────────────────────────┼─────────┼─────────┼──────────┼──────────┤
  │                          │         │ After   │ 126.0 MB │ 128.03 s │
  ├──────────────────────────┼─────────┼─────────┼──────────┼──────────┤
  │ 3: trimmed to 1000 users │ 33,678  │ Before  │ 178.1 MB │ 28.19 s  │
  ├──────────────────────────┼─────────┼─────────┼──────────┼──────────┤
  │                          │         │ After   │ 77.6 MB  │ 23.78 s  │
  ├──────────────────────────┼─────────┼─────────┼──────────┼──────────┤
  │ 4: trimmed to 5000 users │ 48,804  │ Before  │ 197.5 MB │ 41.22 s  │
  ├──────────────────────────┼─────────┼─────────┼──────────┼──────────┤
  │                          │         │ After   │ 82.9 MB  │ 34.08 s  │
  └──────────────────────────┴─────────┴─────────┴──────────┴──────────┘

Why the sink tests each save against two sets

The buffer holds up to one chunk of validated records before the sink writes them. FK constraints are DEFERRABLE INITIALLY DEFERRED. As a result, a save that breaks a constraint succeeds in its savepoint, but the transaction fails at commit.

Two writes of the sink can remove an FK target after a record validated against it:

1. The save of a parent fails, for example because of a unique violation, after its children passed validation.
2. A DELETE cascades to app rows that a dirty record references.

The sink records the IDs that failed and the keys of the cascaded rows. If a SAVE references one of them, the sink saves an error on its Store record and does not save the app model. Each record with a problem gets its own error, and the transaction always commits.

Callers and removal

• ReceiverDeserializeOperation and MorangoProfileController.deserialize_from_store call the new entry point. The public signature of the controller does not change.
• This change removes _deserialize_from_store, _save_deserialized_record, _validate_store_foreign_keys, _validate_missing_store_foreign_keys and _handle_deleted_store_foreign_keys. It also removes the imports that only these functions used.
• This change replaces Store._deserialize_store_model with deserialize_store_model. The new function returns the app model or None, not a tuple, and it has no defer_fks option.
• CHANGELOG.md gets a new 0.9.0 section that lists these changes. It names the replacement for each removed function, so that Kolibri can use it for its upgrade.

Partition order in the stream source

MorangoSource streams records one partition prefix of the sync filter at a time. It sorted the prefixes first, which could stream a record before the record that its FK references. The source now keeps the order of the sync filter, and the partition_order option is removed.

The sort was never in a release, so this change needs no changelog entry.

The caller must list the partitions of FK targets first. Kolibri passes Scope.read_filter as its pull filter, and Scope lists the read-write partitions first. In the single-user scope, this puts the logs in user-rw before the FacilityUser in user-ro. Kolibri needs to reorder the read filter, with the read-only partitions first. #398 tracks a change to Scope that lists the read-write partitions last, so that Kolibri does not need this reorder.

Behavior differences from legacy

• A record that propagates the deletion of its FK target now ends with dirty_bit=False. Legacy left it dirty.
• A dirty parent can be left out of the run by skip_errored or by the sync filter. Its children now validate against the older app row of the parent, and the sink saves them. Legacy failed them with MorangoMissingParent. This difference comes from the Part 2 transforms (Streaming deserialization part 2: transformations #390).
• Some models load a related object in clean_fields. For these models, a child in the same chunk as its dirty parent cannot load the parent yet, so it fails with MorangoDirtyParent.
• As a result, a dirty chain of depth d in one chunk needs d runs. We assume that Kolibri models do not load related objects in clean_fields, so this does not affect Kolibri.
• The test model Facility.clean_dereferences_parent now defaults to False to match Kolibri. Tests set it to True to cover the models that load related objects.
• A dirty grandchild of a record that a cascade deleted always gets a "was deleted" error and stays dirty. In legacy, the result depended on the order of the queries.
• If the save of a parent fails after its children passed validation, each child gets its own error. The cross-model path of legacy saved the children, and the transaction failed at commit.
• deserialize_store_model raises the error that the transforms resolved. This error can be MorangoDirtyParent, MorangoMissingParent or IntegrityError.
• The deserialization warnings now go to the logger morango.sync.stream.deserialize, not morango.sync.operations.

Not in scope

• A separate issue covers the record of cascaded deletions in the tracking tables. post_delete stays muted, as in legacy.
• deferred_clean_fields, ForeignKeyReference, TemporaryTable and the backend bulk helpers lose their only callers. Remove the deferred FK validation API and unused backend bulk helpers #396 tracks their removal before 0.9.0 ships.

References

• Umbrella issue: Morango memory performance improvements #279. Related issue: Reduce memory overhead of _deserialize_from_store #210, for the memory use of deserialization.
• Part 1: Streaming deserialize: add source stream and task #318. Part 2: Streaming deserialization part 2: transformations #390.
• Follow-up cleanup: Remove the deferred FK validation API and unused backend bulk helpers #396.

Reviewer guidance

Verification

• On SQLite, the full suite passes, with 643 tests passed and 2 skipped. The PostgreSQL CI job passed at c106140, before the changes from review.
• We installed this branch into Kolibri, and migrated the Morango imports and calls of Kolibri to the new API. With the sort removed, and with Kolibri's read filter reordered to list the read-only partitions first, the Morango integration tests of Kolibri pass.
• Without the reorder, two Kolibri tests failed: test_kolibri_issue_8439 and test_issue_fixed_in_morango_pull_146. In both, a log streamed before its user on a single-user device that did not have the user yet. See Scope filters should list read-write partitions last #398.
• Before the sort was removed, one Kolibri test failed: test_single_user_course_syncing. Its sync filter streamed a CourseSession before the user that it references.
• prek run --all-files passes, and makemigrations --check reports no changes.
• We turned off the sink guards and ran the tests again. All 4 DeserializeFromStoreTestCase tests that need the run to commit failed at commit with FOREIGN KEY constraint failed.
• After running the benchmarks, I used SQLite's diff tool to compare the resulting Kolibri databases before and after. The result showed no functional differences in the deserialized records, confirming it's operating correctly.

Test changes

• Before the caller switch, we replaced the comparisons in the Part 2 parity tests with literal values that we captured from legacy. The tests now run against the new pipeline. Only these expectations change:
  • The 3 rows that propagate a deletion: dirty_bit becomes False.
  • test_dirty_parent_not_in_run: The sink now saves the child and it ends clean. Legacy failed it with MorangoMissingParent.
• In DeserializationTestCases, the expected results do not change. Only the call and its keyword arguments change.
• In test_controller.py, only the logger name in test_unique_violation_logs_warning changes.
• SelfReferentialFKDeserializationTestCase::test_models_created_successfully passes only because of the new Facility default. It deserializes a full dirty chain in one run, which is the multi-run difference above.
• The 3 tests for an ascending or descending order of partitions become one test that the order of the sync filter is kept. test_sync_filter__partition_order covers an FK target in a later partition of the filter.

Areas to read carefully

• DeserializeSink.consume writes in this order: errors, deletes, propagated deletions, saves, then the Store records. The deletes come before the saves, so that the save guard can see the rows that a delete in the same chunk cascades to.
• delete_app_models builds its Collector while post_delete is muted. Django looks for signal receivers when it decides to fast-delete, so the signal must be muted first.
• Each fast-deleted queryset costs one more SELECT, and only in chunks that have deletes.
• The integration tests use shared databases on disk. Do not run two test processes against them at the same time.

AI usage

I used Claude Code (Claude Opus 5.5) for this part, in a workflow of spec, plan, then implementation. I drafted the design spec and the implementation plan with Claude, and I reviewed and approved both.

A second AI review of the spec found three problems before implementation. We added the cascade case, replaced a query-count criterion that SQLite cannot meet, and documented the effect of the new Facility default.

I made the main decisions: the buffered sink, saves of single records instead of bulk_update, and a standalone function for the deserialization of one record. Claude implemented the plan one task at a time, and wrote failing tests first. Claude also captured the legacy parity values. It also turned off the guards and made sure that the new tests fail when a deferred constraint breaks.

[rtibblesbot]

🟡 Waiting for changes

Outcome: reviewed 2c7cc1b

Last updated: 2026-10-20 21:07 UTC

[bjesterbot]

Executive Summary

This PR successfully completes the streaming deserialization migration (#279, #210). The implementation gives deserialization the same streaming pipeline architecture (StoreModelSource → AppModelDeserialize → AppModelValidate → Buffer → DeserializeSink) as serialization, eliminating unbounded memory usage during sync.

The resolution of the "sink lag" challenge with deferred foreign keys (DEFERRABLE INITIALLY DEFERRED) is particularly well designed: tracking both failed saves (failed_ids) and cascade-deleted rows (cascaded_keys) in DeserializeSink ensures that broken foreign key references are caught and recorded as individual Store errors rather than failing the transaction at commit time.

Findings Summary

• 🚨 Blocker (Must-Fix): None.
• ⚠️ Architecture & Design (Should-Fix): None.
• 💡 Tactical Suggestion (Optional):
  • morango/sync/stream/deserialize.py:542: Record error on task.errors in _record_error to keep in-memory DeserializeTask state synchronized.
  • morango/sync/stream/deserialize.py:462: Guard or assert chunk model uniformity in DeserializeSink.consume when calling delete_app_models.
• ✅ Positive Highlights:
  • Pre-save and post-save signals are muted once for the entire batch of saves.
  • Cascade tracking inspects collector.fast_deletes via values_list("pk", flat=True) to track deleted rows without suppressing Django fast-deletes.
  • Comprehensive legacy parity tests with frozen literal expectations guarantee backward compatibility.

Verification Status

• Test Suite: 644 passed, 2 skipped (pytest).
• Pre-commit / Linters: Clean (prek run --all-files passed ruff, ruff format, yaml, and file hygiene checks).
• Django Migrations: makemigrations --check reports no unapplied schema changes.

Comments on Lines Outside Diff

(None)

Generated by Antigravity using model Gemini 3.8 Flash

[rtibblesbot]

PR #392: no correctness bugs found. Chunk ordering, delete-before-save, cascade tracking and FK-failure propagation match the removed legacy path.

CI pending.

• suggestion: deferred_clean_fields now has no production caller (inline).
• nitpicks: one type annotation, rationale comments, duplicated and brittle tests (inline).

After this review: Full code review.

• Confirm the streaming deserialize_from_store path preserves sync semantics for downstream (Kolibri) callers, since the legacy Store._deserialize_store_model and defer_fks are gone.
• Check that the morango.sync.stream.deserialize logger rename in the changelog is acceptable for anyone's log config.

---

Written by rtibblesbot, an LLM-based coding agent.

[bjester]

Follow-up on the two checks in the review summary from @rtibblesbot:

• Kolibri callers: Kolibri pins morango==0.8.15, so nothing changes for it until it upgrades to 0.9.0. When it does, two call sites need updates:

  • kolibri/core/auth/sync_operations.py calls _deserialize_from_store(profile, filter=...).
  • kolibri/core/auth/test/helpers.py calls store._deserialize_store_model({}).

  The 0.9.0 changelog now names the replacement for each, and the renamed filter and skip_erroring arguments (58c3f5c).

• Logger rename: Kolibri does not configure a logger named morango.sync.operations. All matches in Kolibri are imports of the operation classes, so the rename does not affect it.

[rtibblesbot]

PR #392: 4 of 6 prior findings resolved; 2 acknowledged. No new findings. CI pending.

Prior-finding status

ACKNOWLEDGED — morango/models/core.py:546 — deferred_clean_fields has no production caller (tracked in #396)
RESOLVED — morango/sync/stream/deserialize.py:445 — counts annotation looser than its keys
ACKNOWLEDGED — morango/sync/stream/deserialize.py:521 — comments restating rationale
RESOLVED — tests/testapp/tests/sync/stream/test_deserialize.py — duplicate of test_failed_parent_save__cross_model
RESOLVED — tests/testapp/tests/sync/stream/test_deserialize.py:1683 — self-referential one-level-per-run test
RESOLVED — tests/testapp/tests/sync/stream/test_deserialize.py:1725 — "'save': 1" pins dict repr

After this review: Full code review.

• Confirm the streaming pipeline fully replaces the removed legacy deserialization in operations.py and Store, including error and cascade-delete semantics other sync code relies on.
• Check the 0.9.0 changelog entry describes the behaviour change accurately for downstream integrators.

---

Written by rtibblesbot, an LLM-based coding agent.

[rtibblesbot]

PR #392: no new findings. 4 of 6 prior findings resolved. 2 acknowledged: one deferred to #396, one kept by the author. CI pending.

Prior-finding status

ACKNOWLEDGED — morango/models/core.py:546 — Model.deferred_clean_fields has no production caller (tracked in #396)
ACKNOWLEDGED — morango/sync/stream/deserialize.py:521 — rationale-only comments (author kept them)
RESOLVED — morango/sync/stream/deserialize.py — counts annotation looser than its DeserializeOutcome keys
RESOLVED — tests/testapp/tests/sync/stream/test_deserialize.py — sink test_failed_save duplicates pipeline cross-model test
RESOLVED — tests/testapp/tests/sync/stream/test_deserialize.py:1683 — self-referential one-level test framed as contract
RESOLVED — tests/testapp/tests/sync/stream/test_deserialize.py — summary log test pins dict repr

After this review: Full code review.

• Confirm the streaming pipeline fully replaces the removed legacy deserialization in operations.py and Store, so sync behaviour on 0.9.0 is unchanged for downstream apps.
• Confirm the self-referential one-level-per-run limitation is acceptable for the release.

---

Written by rtibblesbot, an LLM-based coding agent.

[rtibblesbot]

PR #392: all 6 prior findings resolved or acknowledged; no new findings.

CI: Read the Docs fails at HEAD but passed at the previous commit. The new commit touches no documented module. The Sphinx build passes locally. Please re-run the Read the Docs check before merge.

Prior-finding status

ACKNOWLEDGED — morango/models/core.py:546 — deferred_clean_fields has no production caller (tracked in #396)
RESOLVED — morango/sync/stream/deserialize.py — counts annotation looser than its keys
ACKNOWLEDGED — morango/sync/stream/deserialize.py:519 — comments give rationale with no external fact
RESOLVED — tests/testapp/tests/sync/stream/test_deserialize.py — sink test_failed_save duplicates pipeline test
RESOLVED — tests/testapp/tests/sync/stream/test_deserialize.py:1668 — one-level-per-run test framed as contract
RESOLVED — tests/testapp/tests/sync/stream/test_deserialize.py — summary log test pins dict repr

After this review: Full code review.

• Confirm every caller's sync filter lists FK-target partitions before the partitions that reference them, since prefixes are no longer sorted.
• Confirm the Read the Docs failure clears on re-run before merge.

---

Written by rtibblesbot, an LLM-based coding agent.

[rtibbles]

The code changes make sense from a read through - the tests cover the scenarios we have previously discussed, the quick swap for the deserialization tests give strong confidence against regressions, and the integration test results give very strong assurance. Good to go!

[rtibbles]

I think the changes in this file are the most compelling - literally just swapping over the implementation, and all existing tests pass.

[bjester]

Indeed!

[rtibblesbot]

Quick check on rtibblesbot's review of this PR: react 👍 helpful · 😕 mixed · 👎 not helpful. Reply with anything specific.