CXP-897 Incremental sync support

[JavierCarnelli-ConductorOne]

No description provided.

[linear-code]

CXP-897

[github-actions]

Connector PR Review: CXP-897 Incremental sync support

Blocking Issues: 1 | Suggestions: 3 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 3cc087ecd030.
Review mode: full
View review run

Review Summary

Scanned the full PR diff (17 files: new audit-log event feed, SQL Statement Execution API client, Get methods for targeted sync, config/docs/capabilities) for security and correctness. Most prior feedback is addressed in the current tree: resolveSQLWorkspaces and workspaceBuilder.Get now apply the --workspaces allowlist (exclusions come for free via ListWorkspaces), cancelStatement uses /cancel instead of DELETE, Validate() bounds the audit probe at 90s, auditLogActionNames is sorted, parseAuditLogRows bounds-checks row width, and the first-poll window no longer narrows an explicit earliestEvent. The trailing-lag fix, however, introduced a new cursor-advance defect that can stall the feed in a loop.

Security Issues

None found. The audit SQL statement interpolates only hardcoded constants from auditLogActions; all cursor-derived values go through named StatementParameter bindings.

Correctness Issues

• pkg/connector/audit_event_feed.go:333-342 — the hasMore clamp to now-auditLogTrailingLag has no never-regress guard, so a full first page moves the cursor backwards from now-1h to now-4h, and with 1000+ matching events inside the 4h window the cursor never advances while HasMore stays true — a non-terminating paging loop that re-runs the SQL query and re-emits the same page.

Suggestions

• pkg/connector/audit_event_feed.go:310 — hasMore is computed from post-parse rows, so one malformed row makes a genuinely full page look partial.
• pkg/connector/audit_event_feed.go:474 — the single-workspace short-circuit skips the WarehouseExists probe, degrading a bad --sql-warehouse-id into an opaque statement error.
• pkg/connector/workspaces.go:289-299 — the token-auth Get branch omits the IsWorkspaceNameExcluded check that List applies at line 89.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

Correctness Issues

In `pkg/connector/audit_event_feed.go`:
- Around line 333-342: In the `hasMore` branch of advanceEventCursor, the clamp
  to `now.Add(-auditLogTrailingLag)` can move the cursor backwards, because
  there is no never-regress guard on this path (the `target.Before(cursor.StartAt)`
  guard only exists on the drained path below). Two concrete failures: (1) the
  first poll starts at `now - auditLogLookback` (1h); a full first page clamps
  StartAt to `now - auditLogTrailingLag` (4h), regressing the cursor by 3 hours.
  (2) Once clamped to `now-4h`, if 1000 or more matching audit rows fall inside
  the trailing 4h window, every subsequent page is also full, `latest` is again
  after `now-4h`, and StartAt is clamped straight back to `now-4h` while
  StreamState.HasMore stays true, so the SDK re-invokes ListEvents in a tight
  loop that re-executes the SQL statement and re-emits the same ~1000 events
  indefinitely, advancing only at wall-clock rate. Fix by never letting the
  clamped value fall below the incoming cursor, e.g. clamp to the later of
  `cursor.StartAt` and `now-auditLogTrailingLag`, and preserve
  `cursor.StartAfterEventID` when the clamp lands exactly on `cursor.StartAt`;
  alternatively drop the intra-page clamp entirely and apply the trailing lag
  only on the drained (`hasMore == false`) path. Add a regression test that
  drives advanceEventCursor with `hasMore=true` from a cursor newer than
  `now-auditLogTrailingLag` and asserts StartAt never regresses, plus one that
  feeds two consecutive full pages inside the lag window and asserts StartAt
  strictly advances.

Suggestions

In `pkg/connector/audit_event_feed.go`:
- Around line 310: `hasMore := len(rows) >= auditLogPageLimit` uses the
  post-parse slice, but parseAuditLogRows silently drops malformed rows. A page
  that really returned 1000 rows with one unparseable row yields len(rows)==999,
  so hasMore is false and the feed prematurely switches to the drained path
  while more data is pending. Have parseAuditLogRows also return the raw row
  count (or read len(result.Rows) directly) and base hasMore on that.
- Around line 474: `resolveWarehouseWorkspace` returns `workspaces[0]` without
  probing when there is exactly one candidate. A mistyped --sql-warehouse-id in
  a single-workspace account then surfaces as an opaque statement-execution
  failure from ValidateAuditLogAccess instead of the clear "was not found in any
  of the N available workspaces" error. Run the WarehouseExists probe even for a
  single candidate, or return the clear not-found error when it fails.

In `pkg/connector/workspaces.go`:
- Around line 289-299: the token-auth branch of `workspaceBuilder.Get` checks
  membership in `w.workspaces` but does not call
  `w.client.IsWorkspaceNameExcluded(resourceId.Resource)`, which `List` applies
  at line 89. The OAuth branch inherits exclusions via ListWorkspaces, so only
  this path can return a workspace that a full sync skips. Add the
  IsWorkspaceNameExcluded check alongside the existing configured-set check and
  return the same "is not configured" style error.

[github-actions]

Blocking issues found — see review comments.

Requires another review

[github-actions]

No blocking issues found.

[github-actions]

Blocking issues found — see review comments.

[mateoHernandez123]

This is still keyed only by action_name, so the filter drops documented IAM events: addPrincipalsToGroup / removePrincipalsFromGroup, user lifecycle add/delete (not createUser/deleteUser — that last one is the PII purge), removeGroup, and setAccountAdmin / removeAccountAdmin. removeAdmin is workspace-admin revoke but the map attaches AccountAdminRole.

Key the map by (service_name, action_name) and select service_name in the query so generic names like add stay unambiguous. Source: https://docs.databricks.com/aws/en/admin/account-settings/audit-logs

[FeliLucero1]

round 3 fixed the (service, action) keying + plural membership / removeGroup / add+delete — nice. one gap still open though: account-level admin events are setAccountAdmin / removeAccountAdmin (still under accounts), and those aren't in the map. setAdmin alone won't catch the real account-admin grant/revoke.

[mateoHernandez123]

This is still only partially fixed at ec6bc18. The (service_name, action_name) keying and plural group membership events are now correct, but the map still contains only setAdmin / removeAdmin; the documented account-level setAccountAdmin / removeAccountAdmin events are absent. Please add those account-admin mappings before resolving this thread. Source: https://docs.databricks.com/aws/en/admin/account-settings/audit-logs

[mateoHernandez123]

These are skip-and-continue on a corrupt cursor (self-heal to lookback). Please log at Debug, not Warn — Warn on this path shows up as an incident in C1.

Same for the matching Warn on the unmarshal failure below.

[mateoHernandez123]

The base64/JSON corruption paths were moved to Debug, but the same skip-and-continue flow still adds Warn at audit_event_feed.go:158 (stale cursor) and :579 (malformed row). This PR also adds two more at pkg/databricks/sql.go:157 and :194. Please downgrade all four to Debug; connector code must not introduce Warn logs because C1 surfaces them as incidents. Team precedent: https://github.com/ConductorOne/baton-google-cloud-platform/pull/75

[luisina-santos]

Bootstrap lookback (1h) is narrower than the steady-state trailing lag (auditLogTrailingLag, 4h below). Steady-state deliberately holds the watermark 4h behind the newest event to protect against slow-indexing/late-arriving rows, but a brand-new install only looks back 1h — so a fresh connector could miss events that are 1-4h old, which is exactly the class of event the trailing lag exists to protect against once running. Worth reconciling the two constants (or documenting why bootstrap intentionally gets a narrower window than ongoing sync).

[FeliLucero1]

fair point — bootstrap at 1h vs steady-state 4h lag is a real gap for late-indexed events on a fresh install. either bump lookback to match the lag or call it out in the README.

[luisina-santos]

pollStatement blocks synchronously in-band (up to statementPollMaxWait = 5 min) inside ExecuteStatement, which ListEvents/Validate call directly. None of the peer connectors surveyed have this shape since their APIs return results directly rather than requiring async statement submission + polling — this one ties up a sync worker for up to 5 minutes per statement on a cold warehouse. Probably fine at current scale, but worth keeping in mind if this connector ever needs to poll many workspaces/accounts concurrently — there's no async/job-queue model to prevent worker starvation under load. The hard cap + best-effort cancel on give-up is a good safeguard regardless.

[FeliLucero1]

agreed as a scale note — 5m in-band poll is fine for one warehouse today; just something to watch if this ever fans out. the hard cap + /cancel on give-up is the important bit and that's in place.

needs a new review

[github-actions]

🟠 Bug: the hasMore branch has no never-regress guard, so this clamp can move StartAt backwards and stall paging. The first poll starts at now-1h (auditLogLookback); a full first page clamps StartAt to now-4h (auditLogTrailingLag) — 3h earlier than where it started. Worse, once clamped there, if ≥1000 matching events fall inside the trailing-4h window every subsequent page is also full, latest is again after now-4h, and the cursor is clamped straight back to now-4h with HasMore: true. The feed then re-runs the SQL statement and re-emits the same ~1000 events in a tight loop, only creeping forward at wall-clock rate. Consider clamping to max(cursor.StartAt, now-auditLogTrailingLag) (i.e. never below the incoming cursor), or dropping the clamp and applying the trailing lag only on the drained path.

[FeliLucero1]

+1 still broken — clamp to now-4h with no floor at cursor.StartAt means a full first page from now-1h jumps backwards, and with ≥1000 rows in the lag window you just loop on the same page. TestAdvanceEventCursorFullPageClampsToLagWindow currently expects that regress. clamp to max(cursor.StartAt, now-lag) (keep StartAfterEventID when you land on the old StartAt).

[github-actions]

🟡 Suggestion: rows is the post-parse slice, and parseAuditLogRows drops malformed rows. A genuinely full page (1000 rows returned by LIMIT 1000) with even one unparseable row yields len(rows) == 999, so hasMore is false and the feed switches to the drained/trailing-lag path while more data is waiting. Track the raw row count from result.Rows for the hasMore decision instead.

[FeliLucero1]

agreed — one skipped malformed row turns a real full page into len==999 and we prematurely drain. hasMore should use the raw statement row count, not the post-parse slice.

[github-actions]

🟡 Suggestion: the single-workspace short-circuit skips the WarehouseExists probe entirely, so a typo'd --sql-warehouse-id in a single-workspace account is never caught here. Validate() still fails, but with an opaque statement-execution error rather than the clear "not found in any of the N available workspaces" message this function produces for multi-workspace accounts. Probing even when there's one candidate costs one call at startup.

[FeliLucero1]

yep — single-workspace short-circuit skips the nice not-found error. probing once is cheap (and we cache anyway), worth doing for the clearer Validate failure.

[github-actions]

🟡 Suggestion: the token-auth branch checks membership in w.workspaces but not w.client.IsWorkspaceNameExcluded(...), which List applies at line 89. The OAuth branch below gets exclusions for free (they're applied inside ListWorkspaces), so only this path can hand back a workspace a full sync deliberately skips. Now that CAPABILITY_TARGETED_SYNC is declared for every resource type, Get can be called independently of the event feed.

[FeliLucero1]

+1 — List filters excludes at L89, token Get doesn't. incremental is OAuth-only so the feed won't hit this, but targeted sync still can. mirror the exclude check.

[github-actions]

Blocking issues found — see review comments.

[mateoHernandez123]

This branch currently has merge conflicts in README.md and pkg/config/config.go. Please rebase it onto the current main and resolve those conflicts before the next review round; checks are currently awaiting conflict resolution.