[LocalDb] FailClosedOnDrift is skipped during snapshot sync — a future-stamped row jumps the clock #39

New Issue

2026-11-09T14:39:22-05:00

dohertj2 commented

2026-11-09 14:39:22 -05:00

From the scadaproj#15 shared-library review (code-reviews/LocalDb/findings.md, LocalDb-004), verified at 91fb2b4. Update the finding's Resolution row when fixed.


Severity	Medium
Priority	P2
Category	Spec & shared-contract adherence
Status	Open
Location	`ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/Internal/SnapshotStreamer.cs:249`

Description

The drift guard (MaxHlcDriftAhead + FailClosedOnDrift) lives only in
SyncSession.ApplyDeltaEntriesAsync (SyncSession.cs:599-628). SnapshotApplier.OnBatchAsync
passes every snapshot row straight to LwwApplier.ApplyBatchAsync. It takes no
ReplicationOptions, so it cannot check drift. The applier then calls
db.Clock.Observe(maxHlc) (LwwApplier.cs:155), and OnCompleteAsync persists last_seen_hlc.
That value feeds clock recovery on every later open (SqliteLocalDb.cs:69-75).

This contradicts README §Semantics ("With FailClosedOnDrift, each over-drift entry is
dead-lettered") and design §Error handling ("configurable max-drift guard (warn, optionally
fail-closed)").

Failure scenario

Reproduced in a scratch test with FailClosedOnDrift = true. A peer whose clock was a year ahead
when it wrote a row serves a snapshot after a cap prune or rebuild. The receiving node writes 0
dead-letters, applies the row, and its HLC jumps about 365 days ahead. Every later local write on
that node is stamped a year in the future, and the stamp survives restarts through last_seen_hlc.
From then on those writes beat every genuine write on the peer under LWW until wall-clock time
catches up. The operator enabled FailClosedOnDrift specifically to prevent this.

Recommendation

Give SnapshotApplier the options and the clock, and run the same per-row drift check as the delta
path: dead-letter over-drift rows when FailClosedOnDrift, otherwise warn once. Better, move the
check into LwwApplier so both paths share it. Add a snapshot-drift test.

Resolution

Unresolved.

From the scadaproj#15 shared-library review (`code-reviews/LocalDb/findings.md`, **LocalDb-004**), verified at 91fb2b4. Update the finding's Resolution row when fixed. | | | |--|--| | Severity | Medium | | Priority | P2 | | Category | Spec & shared-contract adherence | | Status | Open | | Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/Internal/SnapshotStreamer.cs:249` | **Description** The drift guard (`MaxHlcDriftAhead` + `FailClosedOnDrift`) lives only in `SyncSession.ApplyDeltaEntriesAsync` (`SyncSession.cs:599-628`). `SnapshotApplier.OnBatchAsync` passes every snapshot row straight to `LwwApplier.ApplyBatchAsync`. It takes no `ReplicationOptions`, so it cannot check drift. The applier then calls `db.Clock.Observe(maxHlc)` (`LwwApplier.cs:155`), and `OnCompleteAsync` persists `last_seen_hlc`. That value feeds clock recovery on every later open (`SqliteLocalDb.cs:69-75`). This contradicts README §Semantics ("With `FailClosedOnDrift`, each over-drift entry is dead-lettered") and design §Error handling ("configurable max-drift guard (warn, optionally fail-closed)"). **Failure scenario** Reproduced in a scratch test with `FailClosedOnDrift = true`. A peer whose clock was a year ahead when it wrote a row serves a snapshot after a cap prune or rebuild. The receiving node writes 0 dead-letters, applies the row, and its HLC jumps about 365 days ahead. Every later local write on that node is stamped a year in the future, and the stamp survives restarts through `last_seen_hlc`. From then on those writes beat every genuine write on the peer under LWW until wall-clock time catches up. The operator enabled `FailClosedOnDrift` specifically to prevent this. **Recommendation** Give `SnapshotApplier` the options and the clock, and run the same per-row drift check as the delta path: dead-letter over-drift rows when `FailClosedOnDrift`, otherwise warn once. Better, move the check into `LwwApplier` so both paths share it. Add a snapshot-drift test. **Resolution** _Unresolved._

dohertj2 added the type:review-finding priority:p2 labels 2026-11-09 14:39:22 -05:00

dohertj2 commented

2026-11-09 15:03:20 -05:00

Fixed on main c207573 (LocalDb-004 → Resolved): 2-node test reproduced a year-ahead row applied with no dead-letter. The snapshot path now runs the same drift check as the delta path, and under FailClosedOnDrift a rejected row's timestamp is no longer saved as last_seen_hlc. Fail-closed test fails without the fix; warn-only still applies. Evidence: LocalDb 236/236; Auth 344 passed / 78 skipped (live SQL/LDAP gated) after rebase; code-reviews --check consistent.

Fixed on main c207573 (LocalDb-004 → Resolved): 2-node test reproduced a year-ahead row applied with no dead-letter. The snapshot path now runs the same drift check as the delta path, and under FailClosedOnDrift a rejected row's timestamp is no longer saved as last_seen_hlc. Fail-closed test fails without the fix; warn-only still applies. Evidence: LocalDb 236/236; Auth 344 passed / 78 skipped (live SQL/LDAP gated) after rebase; code-reviews --check consistent.

dohertj2 closed this issue

2026-11-09 15:03:20 -05:00

dohertj2 referenced this issue

2026-11-09 15:03:20 -05:00

[LocalDb/Auth] Follow-ups: snapshot dead-letters uncounted, unbounded held-back rows, duplicate app sync interceptors, GC-forest sAMAccountName match #50

dohertj2 referenced this issue

2026-11-12 14:08:40 -05:00

[LocalDb] Publish 0.3.7 and adopt in ScadaBridge + OtOpcUa (tombstone fix #16) #53

Sign in to join this conversation.