CXH-1905: ServiceNow incremental sync (watermark + delete capture) - #53
CXH-1905: ServiceNow incremental sync (watermark + delete capture)#53cvkramer wants to merge 10 commits into
Conversation
…al state store Add the connector-side primitives for incremental sync against ServiceNow's sys_updated_on column: - model.go: carry sys_updated_on on User/Role/Group and the membership rows (GroupMember, UserToRole, GroupToRole). - request.go: include sys_updated_on in the fields lists; add appendUpdatedSince to AND a "sys_updated_on>=<ts>" clause onto a query (empty ts = no-op). - client.go: add *UpdatedSince list variants (originals delegate with "" for a full pull, preserving behavior) plus drainAll + GetAll*UpdatedSince helpers that page through an entire delta in one call. - pkg/incremental: new connector-managed state store. baton-sdk v0.15.5 has no connector-facing delta/merge (per-sync session store; dormant etag replay), so a bare list-filter would drop unchanged rows from the c1z. State keeps a per-deployment JSON snapshot + per-stream watermarks, merges deltas by sys_id, persists inline (the SDK does not forward Close on in-process syncs), and emits the full union so the c1z stays complete. Strictly opt-in. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ental flag Thread the incremental.State through the connector and the user/role/group syncers. When --incremental is set, each List (and group/role Grants) drains only rows changed since the per-stream watermark, merges them over the cached snapshot, and emits the full union as a single page; on error it marks the run failed so no watermark is advanced past unsynced rows. The non-incremental path (default) is unchanged. - connector.go: ServiceNow holds *incremental.State; New takes incrementalEnabled + stateDir; Close best-effort saves state. - user/group/role.go: state.Enabled() incremental branches; shared resource/grant construction helpers. - config.go (+ regenerated conf.gen.go): --incremental (default off) and --state-dir (CLI-only); main.go passes them through. - INCREMENTAL_SYNC_NOTES.md: full design, SDK-constraint research, live-test results, limitations (deletions, TZ), and the on-call-branch follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ental sync Close the deletion gap in incremental sync: sys_updated_on polling detects inserts/updates but not hard deletes, leaving stale rows (and stale grants) in the cached snapshot. Add a sys_audit_delete reconciliation pass that prunes deleted sys_ids out of the snapshot once per run, before any merged union is built. - model.go: AuditDeleteRecord type, audited table-name constants, AuditedTables. - client.go: GetDeletedSince / GetAllDeletedSince drainers over sys_audit_delete. - incremental/state.go: DeleteWatermark on the snapshot (separate from update watermarks); Deleter interface injected at Load; Reconcile(ctx) runs once per run (sync.Once), prunes resources by sys_id and join rows out of the nested GroupMembers/UserRoles/GroupRoles maps (tablename-scoped so a sys_id collision can't prune the wrong record), advances + persists the delete watermark. Graceful degradation: an audit-table error is logged and swallowed (full-sync backstop reconciles), never failing the sync. - connector.go: pass the client as Deleter to Load. - user/group/role.go: call state.Reconcile(ctx) at the top of each incremental branch (idempotent via sync.Once). - state_test.go: prune-resource, prune-join-row, tablename-scoping, reconcile + watermark advance + once-per-run, graceful-degradation, disabled-no-op tests. - stateVersion bumped 1->2 (forces a clean full pull on upgrade). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… deletion capture Two correctness fixes surfaced while live-testing deletion capture on dev289997: 1. Watermark freeze within a run. The per-group/per-role grant fetches share one stream watermark that each Merge* advances inline. Across the many parallel fetches in a single sync, a far-future sys_updated_on (the instance has demo memberships dated 2031) advanced the stored watermark mid-sync, so every group/role processed afterward fetched ZERO rows -- silently dropping most grants on every warm sync (observed 71 grants vs ~4900 correct). State.Watermark now freezes the value at first read per run (readWatermarks); the stored watermark still advances for the NEXT run. 2. Atomic-persist race. persist() used a fixed '<path>.tmp' name; when the sdk drives the connector across multiple goroutines/instances sharing one state path, two writers collided and one rename hit a temp the other had already renamed away (ENOENT), failing the sync. persist now uses a unique os.CreateTemp(dir, base+'.tmp-*') per call. Added TestWatermarkFrozenWithinRun. Documented deletion capture, the three deletion caveats, the periodic-full-sync backstop, both fixes, the live deletion test result, and the cmn_rota_member on-call follow-up in INCREMENTAL_SYNC_NOTES.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tall detection A future-dated sys_updated_on (ServiceNow demo-data date shifting — e.g. the ITSM App-Dev sample group dated 2031 — clock skew, or imports that preserve source timestamps) was seeding the group_members/groups watermarks to 2031, so real 2026 changes fell below the watermark and were missed by warm syncs until a full sync. advance() now ignores ts in the future (and empty ts); the delete watermark gets the same cap. Future-dated rows are still fetched/merged each run (idempotent). Verified on dev289997: group_members watermark now seeds to the real 2026 max instead of 2031. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cut essay-level comment blocks (state.go was 30% comment lines) down toward the repo's sparse ~2-4% norm, keeping concise godoc on exported symbols and the load-bearing why-notes (watermark cap at now(); snapshot+merge because the SDK has no native delta; per-stream watermarks; sys_audit_delete deletion reconcile). No code changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nilerr (intentional full-pull fallback on corrupt snapshot -> //nolint with reason), revive line-length (split long state-dir description), and goconst (use the package field-name consts fieldSysID/fieldName/UpdatedSinceField in request.go slices). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Working/design notes captured in CXH-1905; kept out of the connector tree. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t map doc - appendUpdatedSince now appends ^ORDERBYsys_updated_on when a watermark is set, so every incremental …UpdatedSince drain paginates in a stable ascending order. Without it the Table API can return rows in an unstable order across offset pages, silently skipping rows. Full-pull path (empty ts) is unchanged. (Same shared helper as the event-feed branch.) - Snapshot.UserRoles/GroupRoles doc comments said 'user/group sys_id' for the outer key, but both are keyed by role sys_id (MergeUserRoles/MergeGroupRoles take roleID). Corrected to avoid misleading maintainers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // persist marshals and atomically writes the snapshot. Caller must hold s.mu. | ||
| // A no-op once the run has been marked failed. | ||
| func (s *State) persist() error { |
There was a problem hiding this comment.
🟡 Suggestion: persist() serializes and atomically rewrites the entire snapshot (all users/groups/roles + every join map) on each Merge* call. Since Grants runs per group and per role, a sync triggers roughly G + 2R full-snapshot rewrites — O(N) JSON marshals of the whole dataset per run, which can be costly on large deployments. Consider persisting once at end of run (you already added Save()/Close()) or debouncing, while keeping MarkFailed semantics. (confidence: medium, non-blocking)
| // a future-dated sys_updated_on (clock skew, demo data) would otherwise push the | ||
| // watermark beyond real changes and stall detection until a full sync. Lexical | ||
| // compare is valid for the fixed-width UTC string. Caller must hold s.mu. | ||
| func (s *State) advance(stream Stream, ts string) { |
There was a problem hiding this comment.
🟡 Suggestion: the next-run watermark advances to the global max sys_updated_on across a stream, but join streams (e.g. group_members) share one watermark across all groups/roles drained sequentially. If group A is drained early and group B later yields a higher max, then a change to group A occurring after A was drained but with a timestamp below B's max is skipped on the next run (>= watermark excludes it). The frozen read-watermark prevents intra-run skips, but not this cross-entity end-watermark gap. It's reconciled by the periodic full sync, so this is by-design; worth documenting that reliance, or using sync-start time as the next watermark. (confidence: low, non-blocking)
Connector PR Review: CXH-1905: ServiceNow incremental sync (watermark + delete capture)Blocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryFull PR diff scanned for security and correctness. This change adds opt-in connector-managed incremental sync: a per-deployment on-disk snapshot plus per-stream Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
Per-stream watermarks (especially the join streams group_members/user_roles/ group_roles, which share one watermark across many groups/roles drained sequentially) advanced to the max sys_updated_on observed. That could skip a row changed mid-sync on an entity drained early, since its timestamp can fall below a later entity's max, and the next run reads >= that max. Advance each synced stream to the run's start time instead. Reads always begin at/after run-start with the frozen lower bound, so everything before run-start was captured; anything at/after is re-fetched next run (idempotent upsert by sys_id). This also subsumes the prior future-dated-row cap (run-start <= now). The run-start clock is injectable (nowFn) for deterministic tests. Addresses PR review #53. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
||
| // persist marshals and atomically writes the snapshot. Caller must hold s.mu. | ||
| // A no-op once the run has been marked failed. | ||
| func (s *State) persist() error { |
There was a problem hiding this comment.
🟡 Suggestion: persist() re-marshals and atomically rewrites the entire snapshot on every Merge* call. With one MergeUsers/MergeRoles/MergeGroups plus per-group/per-role member merges, a sync does roughly G + 2R full-file serializations (the whole users/groups/roles maps each time). On large deployments this is significant disk/CPU churn. Since each Merge* already advances only its own stream, consider debouncing or relying on the final Save()/Close() flush instead of persisting on every merge. (low confidence — correctness is fine; this is an efficiency concern.)
| l := ctxzap.Extract(ctx) | ||
|
|
||
| s.mu.Lock() | ||
| since := s.snapshot.DeleteWatermark |
There was a problem hiding this comment.
🟡 Suggestion: On the very first incremental run the stored DeleteWatermark is empty, so GetAllDeletedSince is called with createdSince="" and drains the entire sys_audit_delete history (potentially millions of rows on a busy instance) — yet the snapshot is empty on that run, so nothing is pruned. Consider seeding DeleteWatermark to runStart (or skipping reconcile) when the snapshot was freshly seeded, so the first run doesn't pay an unbounded audit scan for no benefit. (low confidence — efficiency, not correctness.)
2 participants
Implements CXH-1905 — opt-in incremental sync (
--incremental, default off; full behavior unchanged when off).Why connector-managed
baton-sdk v0.15.5 has no working connector-facing delta/merge, so a naive list-filter would permanently drop unchanged resources from each c1z. The connector keeps its own prior snapshot, fetches only rows changed since
sys_updated_on, merges over the snapshot, and emits the full union — the c1z stays whole while the API only pays for changed rows.What this adds
sys_updated_on) +…UpdatedSince/GetAll…UpdatedSinceclient methods.pkg/incrementalstate store: per-deployment JSON snapshot under--state-dir, per-stream watermarks, atomic temp-file+rename persist, version migration. Disabled/missing/corrupt ⇒ safe full pull.sys_audit_delete(tablename-scoped prune, shared delete-watermark, graceful degradation — never fails the sync).appendUpdatedSinceappends^ORDERBYsys_updated_onwhen a watermark is set, so offset pagination walks forward deterministically (full-pull path unchanged).Validation (live, dev289997)
ORDERBYpagination verified complete/dup-free against the live Table API.go build/vet/testgreen; golangci-lint: 0 new findings vs main.Merge-order note
Shares several
pkg/servicenowsymbols (GetUsersUpdatedSince,AuditDeleteRecord/AuditDeleteResponse,appendUpdatedSince,AuditedTables, access-table consts) with the event-feed branch (CXH-1906). Whichever of the two merges second must rebase and drop the duplicates.🤖 Generated with Claude Code