Conversation
jaskfla
force-pushed
the
tam-7120-1-persist-pulled-deletes-with-sentinel
branch
from
September 10, 2026 04:18
d56886c to
bdcd99a
Compare
|
🦸 Review Hero Summary (round 1) No issues found. Looks good! Below consensus threshold (1 unique issue not confirmed by majority)
|
jaskfla
force-pushed
the
tam-7120-1-persist-pulled-deletes-with-sentinel
branch
2 times, most recently
from
September 10, 2026 05:40
abf13d2 to
d57fbed
Compare
jaskfla
changed the title
jaskfla
changed the title
jaskfla
force-pushed
the
tam-7120-1-persist-pulled-deletes-with-sentinel
branch
from
September 10, 2026 21:57
88f0733 to
f7b198a
Compare
jaskfla
marked this pull request as ready for review
September 10, 2026 22:35
jaskfla
requested review from
chris-bes,
dannash100,
edmofro,
passcod and
rohan-bes
as code owners
September 10, 2026 22:35
jaskfla
force-pushed
the
tam-7120-1-persist-pulled-deletes-with-sentinel
branch
2 times, most recently
from
September 11, 2026 04:58
0a3db27 to
75df0ab
Compare
…tinel Records pulled from central are marked with the INCOMING_FROM_CENTRAL_SERVER sentinel (-1) so the set_updated_at_sync_tick trigger stores them as LAST_UPDATED_ELSEWHERE (-999) and the facility never pushes them back. saveDeletes, saveRestores and the soft-delete branch of saveCreates used paranoid destroy()/restore(), which leave updated_at_sync_tick out of the statement, so the trigger stamped the current tick instead and every facility echoed central's own deletes straight back to it (a ~800k-row bulk delete on central produced an equal-sized push from the facility that pulled it). Write deleted_at and the tick in the same statement. On central the incoming records carry no tick, so behaviour there is unchanged.
Every existing record in a sync payload already goes through saveUpdates, so saveDeletes/saveRestores were a second statement per batch that only flipped deleted_at. Carrying the delete/restore decision on the update payload writes deleted_at, the field changes and updated_at_sync_tick in a single statement per record, and removes setDeletedAt, saveDeletes and saveRestores. On central the incoming record is merged field by field using updated_at_by_field, which does not track deleted_at, so the merge would keep the existing value and drop the delete. deleted_at is kept out of the merge and the caller's decision applied afterwards.
jaskfla
force-pushed
the
tam-7120-1-persist-pulled-deletes-with-sentinel
branch
from
September 15, 2026 01:59
75df0ab to
56a516f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Records pulled from central are marked with the INCOMING_FROM_CENTRAL_SERVER sentinel (-1) so the set_updated_at_sync_tick trigger stores them as LAST_UPDATED_ELSEWHERE (-999) and the facility never pushes them back. saveDeletes, saveRestores and the soft-delete branch of saveCreates used paranoid destroy()/restore(), which leave updated_at_sync_tick out of the statement, so the trigger stamped the current tick instead and every facility echoed central's own deletes straight back to it (a ~800k-row bulk delete on central produced an equal-sized push from the facility that pulled it).
Write deleted_at and the tick in the same statement. On central the incoming records carry no tick, so behaviour there is unchanged.
Changes
Add a brief description of the changes in this PR to help give the reviewer context.
Auto-Deploy
Options
Tests
Review Hero
.github/review-hero/suppressions.yml. Also runs automatically at the end of any auto-fix run.Remember to...