Skip to content

fix(sync): plug outbox hole via seek pagination#475

Merged
Clap404 merged 15 commits into
mainfrom
fix/outbox-fullsync-hole
Jul 20, 2026
Merged

fix(sync): plug outbox hole via seek pagination#475
Clap404 merged 15 commits into
mainfrom
fix/outbox-fullsync-hole

Conversation

@Clap404

@Clap404 Clap404 commented Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR aims to fix two problems at once :

  • a hole exists when data is changed during a full sync : because a full sync can last a long time, items may be synced, then changed before the full sync completes. These items are not added to the outbox and never sent again to cloudsync
  • performance becomes slower for large databases because querying with offset+limit forces db to sort and re-read every row before relevant page.

The solution proposed here is to switch to key-based pagination. During full sync, rows with an id below last synced key are added to the outbox, other rows are ignored and sent via full sync.
Key based pagination is also much more efficient to fetch data anywhere in the table, fixing slow fetch operations on large tables.

@Clap404
Clap404 force-pushed the fix/outbox-fullsync-hole branch 9 times, most recently from 61ac116 to 39a4db5 Compare June 30, 2026 13:42
@Clap404 @claude
fix(sync): plug outbox hole via seek pagination
fe72dc4 
Full sync used LIMIT/OFFSET and gated the outbox on full_sync_finished,
so any mutation to an already-uploaded row during a long full sync was
silently dropped: not in the page that already left, not in the outbox.
CloudSync ended up with a stale snapshot until the next mutation after
the full sync finally finished.

Replace offset pagination with a seek cursor (last_seek_key) stored per
type+lang+shop on eventbus_type_sync. The outbox gate now records a
mid-sync mutation when the row's encoded id is <= the last emitted
cursor, so behind-cursor changes are replayed once full sync completes.
Ahead-of-cursor changes are skipped because the moving cursor will pick
them up naturally.

Schema:
- eventbus_type_sync.last_seek_key VARCHAR(190) NULL
- Upgrade-4.1.0 adds the column and resets in-progress full syncs

Pagination:
- RepositoryInterface: retrieveContentsForFullSeek + countRemainingAfterSeek
- ShopContentServiceInterface: getContentsForFullSeek + encodeOutboxIdAsSeekKey
- ShopContentAbstractService hosts default INT encoding via padInt()
- Composite override: Products, CustomProductCarriers
- Singleton override: Info, Themes
- 33 repositories converted to seek queries (WHERE pk > cursor ORDER BY pk)

SynchronizationService is single-path: every service implements seek;
the legacy offset branch and the Seekable opt-in interface are gone.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Clap404
Clap404 force-pushed the fix/outbox-fullsync-hole branch from 39a4db5 to fe72dc4 Compare June 30, 2026 14:02
Clap404
Clap404 commented Jun 30, 2026
View reviewed changes
Comment thread src/Repository/OrderDetailRepository.php
) {
$seekStartId = (int) $seekStartIdResult[0]['id_order_detail'];
if ($lastSeekKey !== null) {
$this->query->where('od.id_order_detail > ' . (int) $lastSeekKey);

@Clap404 Clap404 Jun 30, 2026

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.

ici, on transformait offset en seek key, donc grosse simplification !

Clap404 and others added 6 commits June 30, 2026 16:45
@Clap404 @claude
refactor(sync): drop seek-key padding, decode at compare time
b886b5c 
Replace zero-padded lex strcmp with in-PHP integer tuple compare.
Storage becomes raw "123" or "10-45"; padInt, SEEK_KEY_PAD constants
and encodeOutboxIdAsSeekKey go away. Outbox gate calls a single
isAtOrBehindSeekKey on the service. Repositories were already
int-casting cursors so no schema or query change needed.

Also:
- upsertTypeSync: lastSeekKey promoted to 2nd param, null-check dropped
- getLastSeekKey: collapse defensive checks now that we write null cleanly
- decodeCompositeSeekKey lifted to AbstractRepository (was duplicated)
- $rawRows rename reverted to $result (pure churn)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Clap404 @claude
refactor(sync): drop pre-filter on outbox insert path
482ef8e 
The hasFullSyncStarted disjunction was a fast-path skip; shouldRecordIntoOutbox
already returns false when last_seek_key is null, so the per-id filter handles
the same case at trivial cost.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Clap404 @claude
refactor(sync): collapse insertContentIntoIncremental branches
f144365 
Multi-lang and single-lang paths were near-duplicates. Unify by deriving
$isoCodes once, then a single loop over lang × type × id with inline filter
via shouldRecordIntoOutbox. Drops two closures and ~30 lines.

The Products id_object "-0" suffix is preserved on the multi-lang path only
(pre-existing behavior — products are multi-lang in practice).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Clap404 @claude
style: php-cs-fixer auto-fixes
67f7c34 
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Clap404 @claude
fix: PHPStan errors
d15828b 
- ShopContentServiceInterface: re-add getFullSyncContentLeft declaration
  (dropped by mistake during interface cleanup; caller still uses it via
  the interface type so missing decl triggered runtime + static failures)
- SynchronizationService: drop redundant isset() on getContentsForFull
  return — shape is now guaranteed by the interface contract
- AbstractRepository::decodeCompositeSeekKey: drop redundant isset on
  $parts[0] (explode always returns >= 1 element)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Clap404 @claude
fix(carrier_details): seek by id_reference, emit one ref per page
84721d0 
Carrier_details rows come from a GROUP BY over (id_reference, id_zone,
id_range), so there is no single column to seek on at row granularity.
Pick the next $limit distinct id_references after the cursor, then emit
every (id_zone, id_range) row for those references in one page. Cursor
stores the last id_reference emitted. Restore the empty-string check in
getLastSeekKey: Db::insert converts PHP null to empty string, not SQL
NULL, so the cursor reads back as '' on first call.

Relax the remaining_objects-decreases-by-limit assertion for
carrier-details — limit applies to reference count, not row count.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Clap404
Clap404 commented Jun 30, 2026
View reviewed changes
Comment thread e2e/src/query-params-validation.spec.ts Outdated
Clap404 and others added 3 commits June 30, 2026 18:17
@Clap404 @claude
fix(e2e): make ps_accounts_mock install idempotent
c64630c 
Without removing the existing dir and forcing unzip overwrite, the
script hangs on an interactive overwrite prompt when the volume already
contains a previous ps_accounts install.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Clap404 @claude
fix(orders): drop duplicate payment_name select
d5f9bff 
Both 'o.payment as payment_name' and 'o.module as payment_name' were
selected, producing a SQL "Duplicate column name 'payment_name'" on PS
8.1+. Keep 'o.module as payment_name' to match fixture data (module
slug like "cheque"/"bankwire"); the actual payment label is already
exposed via payment_mode = o.payment.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Clap404 @claude
fix(cart_products): seek per id_cart, emit one cart per page
98217b3 
cart_product rows are keyed by (id_cart, id_product, id_product_attribute);
a cart can hold many product rows. Seeking on cp.id_cart > cursor with a
row-level LIMIT could end a page mid-cart, after which the next page's
cp.id_cart > cursor would skip that cart's remaining rows — same outbox
hole as carrier_details.

Page by distinct id_cart: emit every product row for the next $limit
carts after the cursor. $limit applies to the cart count, not the row
count. Reflect the relaxation in the remaining_objects e2e assertion.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Clap404 Clap404 added the 🥕 Radis to review Ready to reviewed by 1 peer label Jun 30, 2026
@Clap404 @claude
fix(seek): advance cursor from picked ids, cheap remaining count
4ca699a 
Two related fixes for per-parent seek pagination (carrier_details,
cart_products):

1. retrieveContentsForFull now returns {rows, lastId}. lastId is the max
   parent id picked in the DISTINCT subquery, not the last emitted row.
   When the join produces zero rows for the picked parents (fixture /
   config edge — e.g. PS 1.7.8.10 with only one active country), the
   cursor still advances instead of looping forever on the same
   parents.

2. countFullSyncContentLeft now counts DISTINCT parent ids directly
   against the base table, dropping the heavy GROUP BY subquery wrap.
   Since pagination granularity is per-parent, remaining_objects should
   reflect remaining parents.

Also uninstall ps_eventbus before install in the e2e init script — some
flashlight images (notably v9) ship the module pre-installed with a
stale schema (offset column instead of last_seek_key) and a plain
install is a no-op.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Clap404 Clap404 self-assigned this Jul 1, 2026
Clap404 and others added 4 commits July 1, 2026 10:25
@Clap404 @claude
refactor(upgrade): drop redundant last_seek_key reset
2ede853 
ADD COLUMN ... DEFAULT NULL already NULLs every existing row. The
subsequent UPDATE ... WHERE full_sync_finished = 0 was a no-op — and
misleading, since NULL is not used as a pending/finished marker
(full_sync_finished is). Finished syncs also store NULL.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Clap404
simplify upgrade script further
4f20f26
@Clap404 @claude
refactor(seek): use row offset as cursor for CD/CP
91fb484 
carrier_details and cart_products now paginate with SQL LIMIT/OFFSET
over the base GROUP BY query, cursor = row count emitted so far. This
gives an exact remaining_objects that decrements by limit per page and
removes the per-parent complexity.

Trade-off: numeric-compare between the row-offset cursor and a content
id (id_carrier / id_cart) has no meaning, so isAtOrBehindSeekKey is
overridden to always return true for these two collections. Every
mutation during full sync is recorded in the outbox; incremental sync
re-uploads any rows that were already emitted. Over-recording is safe
— under-recording (the pre-4.1.0 outbox hole) is not.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Clap404
add pending outbox count during fullsync
5f5716c
@sonarqubecloud

sonarqubecloud Bot commented Jul 1, 2026

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
39.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@Clap404
Clap404 merged commit fbc17df into main Jul 20, 2026
21 of 22 checks passed
@Clap404
Clap404 deleted the fix/outbox-fullsync-hole branch July 20, 2026 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@Clap404 Clap404

Labels

🥕 Radis to review Ready to reviewed by 1 peer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Clap404