## Problem The incremental pull watermark is stored as an integer Unix second. The record values it is compared against can carry fractional seconds. `QueueHandler::enqueueResultSet()` parses each record's `pull_trigger_date` with `strtotime()`, which returns whole seconds. `ApiSyncMapping::getPullQuery()` renders the lower bound with `gmdate('Y-m-d\TH:i:s\Z', $start)`. Both steps discard the fractional part. A remote that emits sub-second `Edm.DateTimeOffset` values therefore has records the watermark can never pass. Business Central returns three fractional digits. A record whose trigger value is `2026-04-13T12:00:06.123Z` advances the watermark only to `2026-04-13T12:00:06Z`. The next run filters on `gt 2026-04-13T12:00:06Z`, which still matches that record, so it is fetched and queued again. This repeats on every cron run. `PullBase` normally decides that no entity update is needed, so data is not corrupted. The effects are a pull queue that does not drain, repeated fetches of the same records, and genuine changes that are harder to see among them. Three further defects sit in the same path. **The watermark can move backwards.** `$maxTime` is reset to `0` at the top of `enqueueResultSet()`. That method runs once per page, because `enqueueAllResults()` loops it over `@odata.nextLink`. The first record of each page after the first satisfies `0 < $recordTime` and writes state. A run therefore ends with the last page's maximum rather than the maximum across the run. The pull query sets no `$orderby`, so the pages are not guaranteed to arrive in ascending order. **`enqueueResultSet()` cannot report failure.** Its `try` block wraps the whole record loop. On a throw it dispatches an error event and returns `FALSE`. The caller is `while (!$this->enqueueResultSet(…))`, where `FALSE` means "there are more pages". A failure part-way through a page therefore looks the same as an ordinary page, the remaining records of that page are never enqueued, and the loop continues to its normal end. **State is written inside the record loop.** `setLastPullTime()` serialises and writes the whole multi-mapping state array once per improving record. `setPullInfo()` also writes back a copy of `pull_info` taken once in the entity constructor. If something else has written the sibling delete key since then, that value is reverted. The order used in cron happens to avoid this. The Drush commands and the mapping form, which holds the entity across a form submit, do not. ## Proposed resolution ### Store the watermark as a string `apisync.mapping_pull_info` gains `last_pull_watermark` and `last_delete_watermark`, and loses `last_pull_timestamp` and `last_delete_timestamp`. The keys are renamed rather than reused so that a site which has not run the update yet fails visibly instead of behaving as though it has no watermark. The stored value is treated as opaque: only one class parses it. The reason is that a future trigger need not be a timestamp. For Business Central the realistic alternatives are `SystemModifiedAt`, which is a timestamp, and `SystemRowVersion`, which is a monotonic counter. Delta links are not an option, as they were removed from the Business Central server in 2024 release wave 1. An integer or datetime type could not hold a row version, so a string is used and the meaning is left to the strategy class below. The state array also gains `last_pull_attempt`, a local Unix integer. See *Separate the two clocks*. ### Add one strategy class ```php interface PullWatermarkInterface { public function fromRecord(ODataObjectInterface $record, string $triggerField): ?string; public function fromLocalTime(int $timestamp): string; public function toFilterLiteral(string $watermark): string; public function compare(string $a, string $b): int; public function isValid(string $watermark): bool; } ``` `DateTimePullWatermark` is the single implementation. It normalises to RFC 3339 in UTC at a fixed width — always six fractional digits, always `Z` — and parses and compares through `DateTimeImmutable`. Four rules come with it, each covered by a test. - `isValid()` rejects any value outside the canonical form, and any value containing `=`, `<`, `>` or `!`. `SelectQuery::__toString()` applies `str_replace` to the whole condition array, including the value. Current literals contain none of those characters, so this works today; the check makes it a guaranteed property rather than a coincidence. - `compare()` is the only comparison. A test asserts that `strcmp` agrees with it for canonical values, but raw watermarks are never compared with `<` or `>`. `ApiSyncPullCommands::pullQuery()` currently has `if ($start > $stop)`, which would become a string comparison and give wrong results as soon as two different precisions or a non-`Z` offset are involved. - `toFilterLiteral()` rounds down. Rounding down is always safe under a strict `gt`, and it removes any dependence on how a given remote handles fractional digits beyond its own precision. - `fromRecord()` and `fromLocalTime()` are separate methods because they produce different kinds of value: a position in the remote's change stream, and a reading of the local clock. `fromLocalTime()` is documented as unsafe to use directly as a filter bound. Treating the two as interchangeable is what makes the current delete watermark wrong. It is registered as a service. Turning it into a plugin type chosen per mapping is a reasonable follow-up once a second implementation exists. Because every caller goes through the interface, that change would be mechanical. ### Advance the watermark once per run Correct the error handling first. `enqueueResultSet()` catches, dispatches and then rethrows instead of swallowing. The `catch` around `queryMore()`, which carries a `@todo do we really want to eat this exception here?`, returns before anything is written. `enqueueAllResults()` then takes ownership. It seeds the run maximum from the stored watermark, carries it across pages, and writes state once, after the loop, on the success path. Not in a `finally`, because that also runs when an exception is thrown. Seeding from the stored value means the watermark cannot decrease. Writing only after a completed loop means an interrupted run leaves the watermark unchanged and fetches the same range again, rather than advancing past pages it never received. ### Make `getPullQuery()` a pure function of its arguments It currently falls back to `getLastPullTime()` internally, and three call sites pass `$start = 1` to mean "ignore the stored watermark and fetch everything": a site module's own OData provider, `DeleteHandler::queryRemoteContactsFlaggedForDeletion()`, and `ApiSyncPullCommands::pullQuery()` under `--force-pull`. Move the fallback to the caller. A `$start` of `NULL` then means one thing only — no lower bound, fetch everything — and those three sites pass `NULL` instead of `1`. This removes the sentinel value, removes a hidden `\Drupal::state()` dependency from a config entity, and makes the method straightforward to test. ### Keep full pulls working, and detect a missing update A full pull is a normal operation and must keep working. `drush as-pull-reset <mapping>` clears the watermark and flags every mapped object with `force_pull`, and the next run is expected to fetch the whole collection. The same applies to a mapping that has never pulled, and to `--force-pull`. What should not happen is a full pull caused by an update that has not run. Between deploying the new code and running `updatedb`, the state array holds `last_pull_timestamp` and not `last_pull_watermark`. A cron run in that window would otherwise read "no watermark", fetch the entire collection, and queue it. The three cases are told apart by the shape of the state array, tested with `array_key_exists()` rather than `isset()`: | State | Meaning | Behaviour | | --- | --- | --- | | `last_pull_watermark` holds a canonical string | ordinary incremental pull | filter on `gt <literal>` | | `last_pull_watermark` is present and `NULL` | reset, or never pulled | no lower bound; full pull | | `last_pull_watermark` is absent and `last_pull_timestamp` is present | the update has not run | throw `ApiException` | `setLastPullWatermark(NULL)` writes the key with a `NULL` value, so `as-pull-reset` produces the second row and behaves exactly as it does today. The update writes the key for every mapping, so the third row cannot occur once it has run. `QueueHandler::doApiSyncObjectQuery()` already catches `ApiException` and dispatches an `ApiSyncErrorEvent`, so that case is logged and the run is skipped instead of fetching everything. ### Separate the two clocks `getNextPullTime()` is `last_pull_timestamp + pull_frequency`, compared against the local request time. The left side comes from the remote's clock, by way of record data; the right side is local. Two things follow: a mapping whose remote has not changed recently never reaches its next pull time, and a remote clock that runs ahead causes pulls to be skipped. This is not visible today because `pull_frequency` defaults to `0`. Drive `getNextPullTime()` from the new `last_pull_attempt` instead, written at the start of each run. The watermark then has no local-time meaning at all. ### Add a `pull_overlap` setting, default `0` The current second truncation replays part of the boundary second on each run, and the per-page reset causes extra re-fetches. Both are removed by the changes above, which narrows the queried range. That matters if a remote makes records visible in a different order from their change timestamps: a record written at `.100` that only becomes visible after one written at `.900` is missed permanently once the watermark reaches `.900`. This is a known risk for any change capture based on timestamps. `pull_overlap` is a per-mapping value in seconds, subtracted when the bound is emitted rather than when the watermark is stored. It defaults to `0`. Enabling it by default would bring back the behaviour this issue describes: with a watermark of `…06.123` and a bound of `…05.123`, the record that set the watermark matches again, and on a mapping where nothing else changes it matches on every run. The setting is documented as the first thing to increase if records are found to be missing. ### Update path A `hook_update_N` in `apisync_mapping` converts existing values. It belongs there rather than in `apisync_pull` because `ApiSyncMapping` performs both the read and the write, and a site running `apisync_mapping` without `apisync_pull` would otherwise get the new accessors without the migration. The conversion is exact. The stored integer is already a whole second, because it came from `strtotime()` of a value that may have had a fraction. The integer and the same instant written with `.000000` are the same point in time, so no record can be skipped. The hook is idempotent and accepts a stored value of `0`, `NULL`, a missing key, or a string that has already been converted. The same change resolves a contradiction between `apisync.install`, which states that `apisync.mapping_pull_info` is deliberately kept on uninstall so that a reinstall retains its state, and `apisync_pull_uninstall()`, which deletes it. ## Remaining tasks - Review the merge request. - Decide whether to move to per-mapping state keys (`apisync.mapping_pull_info.<id>`) in the same update. That would remove the read-modify-write of the whole array and the cross-mapping overwrite it allows. It costs nothing extra now and needs a second migration later. ## User interface changes The mapping form shows the stored watermark in its canonical UTC form instead of passing it through `dateFormatter`. The value is a position in the remote system's clock, so rendering it in the site's timezone is misleading when diagnosing a sync gap. A `pull_overlap` field is added. The "Reset Last Pull Date" and "Reset Last Delete Date" buttons are unchanged. ## API changes Breaking, within an alpha release. `ApiSyncMappingInterface`: `getLastPullTime()`, `setLastPullTime()`, `getLastDeleteTime()` and `setLastDeleteTime()` are replaced by `getLastPullWatermark()`, `setLastPullWatermark()`, `getLastDeleteWatermark()` and `setLastDeleteWatermark()`. `getPullQuery()` takes `?string $start` and `?string $stop` and no longer reads state itself; callers resolve the bound and pass an explicit value for a deliberate full pull. New: `Drupal\apisync_mapping\PullWatermarkInterface`, implemented by `DateTimePullWatermark` and registered as `apisync_mapping.pull_watermark`. ## Data model changes `apisync.mapping_pull_info` changes shape, migrated by a `hook_update_N` in `apisync_mapping`. `apisync_mapping.schema.yml` gains `pull_overlap`.