CXH-1390 CXP-481 Support for Incremental Sync

[JavierCarnelli-ConductorOne]

Description

• Bug fix
• New feature

Support for Incremental Sync included

Useful links:

• Baton SDK coding guidelines
• New contributor guide

[linear]

CXP-481 Incremental Sync for Baton SQL

[github-actions]

Claude finished @JavierCarnelli-ConductorOne's task in 4m 19s —— View job

---

PR Review: Incremental Sync Support

• Gather context and understand changes
• Review core implementation (event_feed.go, event_pagination.go)
• Review config and validation changes
• Review connector integration
• Review example configurations
• Post detailed review

---

Overview

This PR adds incremental sync (event feed) support to baton-sql, enabling efficient change detection via timestamp-based queries instead of full re-syncs. The implementation supports three event types:

• Resource changes — detects new/modified resources via updated_at-style columns
• Grant changes — detects new grants
• Grant revokes — detects soft-deleted grants (optional)

The architecture is well-designed: each "source" (resource type or grant query) maintains its own independent cursor, and a round-robin scheduler processes one page per source per ListEvents call. The cursor design correctly separates committed state (SourceCursors) from in-flight state (CurrentSince, CurrentMaxSeen), which ensures crash safety.

What works well

• Cursor design — The two-tier cursor (SourceCursors for committed state, CurrentSince/CurrentMaxSeen for in-flight) is clean. The since timestamp stays constant across all pages of a scan cycle, preventing missed or duplicated events.
• Deterministic source ordering — getSources sorts resource type IDs lexicographically, ensuring stable cursor semantics across restarts.
• Validation is thorough — Required placeholders (?<since>, ?<id>) are checked at startup, reserved var names are blocked, and get is required when incremental_sync is configured on a resource type.
• Reuse of existing runQuery — The incremental sync queries reuse the same query execution and pagination infrastructure as full sync, avoiding code duplication.
• grantRowKey for event ID uniqueness — Good addition in the fix commit to handle cases where multiple grant rows share the same (resource, principal, timestamp) triple.
• SDK interface compliance — SQLEventFeed correctly implements connectorbuilder.EventFeed, and userSyncer inherits Get() via embedding.
• Comprehensive examples — All database-specific example files were updated with working incremental sync configurations.

Issues found

Must-fix

1. No tests for new code — event_feed.go (429 lines) and event_pagination.go (127 lines) have zero test coverage. The cursor state machine (commitAndAdvance), source enumeration (getSources), and time parsing (toTime) are all testable in isolation without a database. The validation additions in validate.go also lack test cases. Given the complexity of the multi-source pagination state machine, unit tests are essential. Add tests →

Should-fix

2. %v in grantRowKey risks scientific notation (event_feed.go:405) — Per CLAUDE.md, %v with large numeric IDs can produce 1.23e+15. Use a type switch for numeric types. See inline comment.

3. Implicit coupling between incremental sync query and list mapping (event_feed.go:215) — processResourceChangePage evaluates s.config.List.Map.Id against rows from the incremental sync query. If the user's incremental query doesn't return the columns referenced by the list mapping's id expression, they get a runtime CEL error with no startup validation. See inline comment.

4. earliestEvent parameter is silently ignored (event_feed.go:56) — The design decision is sound (per-source cursors shouldn't be overridden), but the unused parameter needs a comment explaining why. See inline comment.

Nits / suggestions

5. EventFeeds re-creates syncers on each call (connector.go:49-60) — GetSQLSyncers allocates new instances each time. The SDK calls this during init, so it's not a hot path, but caching would be cleaner. See inline comment.

6. getSources rebuilds on every ListEvents call (event_feed.go:64) — The source list is derived from static config and doesn't change. Could be precomputed in NewSQLEventFeed.

7. SAP HANA example uses unusual indentation — sap-hana-test.yml has all top-level keys indented 2 spaces under ---. This is valid YAML but inconsistent with all other example files. This was a pre-existing issue, not introduced by this PR.

Summary

The core design is solid — the multi-source cursor state machine, the separation of committed vs in-flight state, and the reuse of existing query infrastructure are all well-thought-out. The main gap is test coverage: the cursor state machine and validation logic have enough complexity to warrant dedicated unit tests before merging. The %v scientific notation issue and the implicit list-mapping coupling are worth addressing as well.

[github-actions]

Claude finished @JavierCarnelli-ConductorOne's task in 2m 8s —— View job

---

PR Review: Incremental Sync Support

• Gather context and understand changes
• Review core implementation (event_feed.go, event_pagination.go)
• Review configuration and validation
• Review connector integration and examples
• Post detailed review

---

Overview

This PR adds incremental sync (event feed) support to baton-sql. Instead of full re-syncs, the connector can now detect changes via timestamp-based SQL queries and emit ResourceChangeEvent, CreateGrantEvent, and CreateRevokeEvent. The implementation supports three source kinds per resource type — resource changes, grant changes, and grant revokes — each with its own independent cursor.

Architecture Assessment

The design is well-structured:

• Two-tier cursor model (event_pagination.go:17-25) — SourceCursors holds committed state; CurrentSince/CurrentMaxSeen hold in-flight state. This correctly prevents skipping events on crash/restart.
• Round-robin scheduling — One page per source per ListEvents call (event_feed.go:73-77), advancing to the next source after exhaustion (event_feed.go:158-170).
• Deterministic source ordering (event_pagination.go:72-77) — Sorting resource type IDs lexicographically ensures cursor semantics are stable across restarts.
• Reuse of runQuery — The incremental queries share pagination and query infrastructure with full sync, reducing code duplication.
• Thorough startup validation — ?<since> placeholder required, reserved var names blocked, get required when incremental_sync is configured, CEL expressions compiled at startup.

Issues

1. No tests for new code (must-fix)

event_feed.go (445 lines) and event_pagination.go (127 lines) have zero test coverage. The cursor state machine, source enumeration, and time parsing are testable without a database. Given the existing test files in pkg/bsql/ (validate_test.go, query_test.go, helpers_test.go), this codebase clearly values tests — these new files should follow suit.

Key areas to test:

• marshalCursor/unmarshalCursor round-trip and edge cases (empty string, nil SourceCursors)
• getSources deterministic ordering with multiple resource types
• commitAndAdvance state transitions (mid-page vs. source exhaustion vs. full cycle)
• toTime with time.Time, string, []byte, and unsupported types
• defaultLookback with valid duration, invalid duration, and nil config
• grantRowKey with int64, float64, missing key, nil pagination

The validation additions in validate.go (lines 177-270) also lack test cases — the existing validate_test.go only covers ListQuery.

Add tests →

2. Potential data loss: toTime silently falls back to since on parse failure (event_feed.go:229-237, 293-301)

When the cursor column value can't be parsed by toTime, the code silently falls back to the since timestamp as the rowTimestamp:

rowTimestamp := since
if ts, ok := rowMap[rc.CursorColumn]; ok {
    if t, parseErr := toTime(ts); parseErr == nil {
        rowTimestamp = t
        ...
    }
}

If toTime fails (e.g., unexpected format from a database driver), maxSeen never advances, so the next cycle will re-query from the same since — creating an infinite loop of duplicate events. This is subtle and hard to debug. Consider logging a warning on parse failure, or returning an error.

3. defaultLookback silently swallows parse errors (event_pagination.go:119-127)

func defaultLookback(config *IncrementalSyncConfig) time.Duration {
    if config != nil && config.DefaultLookback != "" {
        d, err := time.ParseDuration(config.DefaultLookback)
        if err == nil {
            return d
        }
    }
    return defaultLookbackDuration
}

Startup validation in IncrementalSyncConfig.staticValidate catches bad durations, so this path should only fire with valid values. But if it does fail at runtime (e.g., config modified post-validation), silently defaulting to 3h could be surprising. A debug log would help.

4. getSources rebuilds on every ListEvents call (event_feed.go:68)

The source list is derived from static config and can't change at runtime. Building it in NewSQLEventFeed and storing it on the struct would avoid repeated allocation on every call. Not a correctness issue, but it's a simple optimization.

5. Event ID format asymmetry between resource and grant events

Resource event IDs use the format resource:type:id:timestamp (event_feed.go:240) while grant/revoke event IDs include an extra rowKey disambiguator: grant:type:resourceID:principalID:timestamp:rowKey (event_feed.go:329).

Resource events rely on (resourceType, resourceID, timestamp) being unique. If a bulk update sets the same updated_at for multiple distinct resources, the event IDs would technically still be unique (different resourceID), but if two rows with different data map to the same resource ID (unlikely but possible with complex CEL expressions), they'd collide. This is more of a documentation point than a bug.

6. Connector.EventFeeds swallows syncer creation errors (connector.go:57-60)

syncers, err := c.config.GetSQLSyncers(ctx, c.db, c.dbEngine, c.celEnv)
if err != nil {
    l.Debug("baton-sql: failed to create syncers for event feed...", zap.Error(err))
    return nil
}

A debug-level log for a potentially significant failure (incremental sync silently becomes unavailable) is easy to miss. Consider l.Warn(...) so it's visible in normal operation.

Validation review

The validation changes are solid:

• GetQuery.staticValidate (validate.go:177-194) — Correctly requires ?<id> placeholder and blocks reserved var names.
• ResourceIncrementalSync.staticValidate (validate.go:196-221) — Requires cursor_column, validates ?<since> placeholder, blocks reserved vars, and compiles optional resource_id CEL expression at startup.
• GrantsIncrementalSync.staticValidate (validate.go:223-261) — Requires resource_id and changes_cursor_column, validates ?<since> in both changes and revokes queries, enforces revokes_cursor_column when revokes_query is set.
• IncrementalSyncConfig.staticValidate (validate.go:263-270) — Validates default_lookback as a Go duration.
• Cross-check in sql_syncer.go:183-185 — Correctly requires get when incremental_sync is configured.

One gap: as noted in the previous review, there's no startup validation that the incremental sync query returns the columns referenced by List.Map.Id (when ResourceId is not explicitly set). The resource_id field on ResourceIncrementalSync (config.go:495) was added to address this — users can explicitly set it to decouple the incremental query from the list mapping. The fallback to List.Map.Id (event_feed.go:217-220) is documented but could catch users off-guard.

Config and examples

• The ResourceIncrementalSync.ResourceId field (config.go:492-495) is a good addition — it cleanly solves the coupling issue between incremental queries and list mappings.
• example.yml is well-documented with inline comments explaining each section.
• All database-specific examples updated consistently.

Minor observations

• The earliestEvent parameter comment (event_feed.go:55-57) is clear and explains the design decision well. Good improvement from the earlier commit.
• grantRowKey (event_feed.go:406-423) correctly handles int64 and float64 to avoid scientific notation, addressing the CLAUDE.md guideline. The %v fallback for other types is fine since non-numeric types won't produce scientific notation.
• The sinceKey and idKey constants (query.go:29-32) with their doc comment properly establish the reservation.

Summary

The architecture is sound — the cursor state machine, the source scheduling, and the config validation are all well-designed. The main gap is test coverage: the new code has enough state machine complexity to warrant dedicated unit tests before merging. The silent toTime fallback (issue #2) is worth a second look since it could mask cursor advancement failures. The rest are low-severity improvements.

[github-actions]

Claude finished @JavierCarnelli-ConductorOne's task in 2m 59s —— View job

---

PR Review: Incremental Sync Support

• Gather context and understand changes
• Review core implementation files
• Review configuration and validation
• Review connector integration and examples
• Post detailed review

---

Overview

This PR adds incremental sync (event feed) support to baton-sql. Instead of full re-syncs, the connector can now detect changes via timestamp-based SQL queries and emit ResourceChangeEvent, CreateGrantEvent, and CreateRevokeEvent. The implementation supports three source kinds per resource type — resource changes, grant changes, and grant revokes — each with its own independent cursor.

Also adds Get() (targeted resource fetch by ID), required when incremental sync is configured.

Architecture

The design is solid:

• Two-tier cursor model (event_pagination.go:17-25) — SourceCursors holds committed state; CurrentSince/CurrentMaxSeen hold in-flight state. This prevents skipping events on crash/restart.
• Round-robin scheduling — One page per source per ListEvents call, advancing to the next source after exhaustion.
• Deterministic source ordering (event_pagination.go:72-77) — Sorts resource type IDs lexicographically, ensuring stable cursor semantics across restarts.
• Reuse of runQuery — Incremental queries share pagination and query infrastructure with full sync.
• Thorough startup validation — ?<since> placeholder required, reserved var names blocked, get required when incremental_sync is configured, optional CEL expressions compiled at startup.

The latest commit (28c9e4e) correctly addressed the toTime silent fallback — parse failures are now propagated as errors rather than silently falling back to since, which prevents the infinite-reprocessing loop flagged in earlier reviews.

Issues

1. GrantsIncrementalSync.resource_id CEL expression not compiled at startup (bug)

ResourceIncrementalSync.staticValidate compiles the optional resource_id CEL expression at startup (validate.go:215-218), catching typos early. But GrantsIncrementalSync.staticValidate (validate.go:223-261) only checks that resource_id is non-empty — it never calls s.env.Compile(). A bad CEL expression in grant incremental sync's resource_id would only fail at runtime during processGrantPage (event_feed.go:309).

// validate.go:223 — missing Compile call for GrantsIncrementalSync.ResourceId
func (l *GrantsIncrementalSync) staticValidate(ctx context.Context, s *SQLSyncer) error {
    if l.ResourceId == "" {
        return errors.New("incremental_sync.resource_id is required")
    }
    // Should add: if err := s.env.Compile(l.ResourceId); err != nil { ... }

Fix this →

2. No tests for new code (must-fix, carried over)

event_feed.go (450 lines) and event_pagination.go (127 lines) have zero test coverage. This was flagged in both previous reviews and remains unaddressed. The cursor state machine has enough complexity that unit tests are essential.

Testable in isolation (no database needed):

• marshalCursor/unmarshalCursor round-trip and edge cases
• getSources deterministic ordering with multiple resource types
• commitAndAdvance state transitions (mid-page, source exhaustion, full cycle wrap)
• toTime with time.Time, string, []byte, and unsupported types
• defaultLookback with valid/invalid/nil config
• grantRowKey with int64, float64, missing key, nil pagination

The validation additions (validate.go:177-270) also lack test cases — the existing validate_test.go only covers ListQuery.

Add tests →

3. EventFeeds error logging level too low (connector.go:59)

If GetSQLSyncers fails inside EventFeeds, incremental sync silently becomes unavailable with only a Debug-level log. This is a user-facing degradation that would be very hard to troubleshoot. Consider Warn level.

// connector.go:59 — Debug level hides a significant failure
l.Debug("baton-sql: failed to create syncers for event feed...")
// Should be:
l.Warn("baton-sql: failed to create syncers for event feed...")

4. getSources rebuilds on every ListEvents call (event_feed.go:68)

The source list is derived from static config and can't change at runtime. It could be precomputed in NewSQLEventFeed and stored on the struct. Not a correctness issue but a simple optimization that removes repeated allocation and sorting on every call.

Good changes since earlier reviews

• toTime parse failures now propagated as errors (28c9e4e) — Fixes the silent fallback that could cause infinite reprocessing loops. This was the most impactful issue from prior reviews.
• grantRowKey handles numeric types — Correctly uses strconv.FormatInt for int64/float64 to avoid scientific notation per CLAUDE.md guidelines.
• ResourceId field on ResourceIncrementalSync (config.go:492-495) — Decouples incremental queries from list mappings, addressing the implicit coupling concern.
• earliestEvent comment (event_feed.go:54-57) — Clear explanation of the intentional design decision.
• EventFeeds statelessness comment (connector.go:48-51) — Documents why re-creating syncers is safe.

Validation review

The validation is thorough and well-structured:

• GetQuery.staticValidate — Requires ?<id> placeholder, blocks reserved var names
• ResourceIncrementalSync.staticValidate — Requires cursor_column, validates ?<since>, blocks reserved vars, compiles optional resource_id CEL
• GrantsIncrementalSync.staticValidate — Requires resource_id and changes_cursor_column, validates ?<since> in both queries, enforces revokes_cursor_column when revokes_query is set
• Cross-check in sql_syncer.go:182-184 — Correctly requires get when incremental_sync is configured

The only gap is issue #1 above (missing CEL compile for grants resource_id).

Summary

The core architecture is well-designed — the cursor state machine, crash safety, deterministic ordering, and reuse of existing infrastructure are all solid. The latest commit addressed the most critical prior review issue (silent toTime fallback). Two items remain before this is merge-ready: add the missing CEL compile check for grants resource_id (issue #1) and add unit tests (issue #2). The log level and getSources caching (issues #3-4) are nice-to-haves.

[luisina-santos]

I know you've done a lot of testing against a real sql database, but just mentioning: I would like to have test coverage for new event feed code

pkg/bsql/event_feed.go and event_pagination.go add ~500 lines of logic with no corresponding _test.go. The cursor advancement, source ordering, commitAndAdvance, and sinceForSource are all untested. At minimum, unit tests for getSources, commitAndAdvance, and unmarshalCursor would catch regressions.

[luisina-santos]

should we keep some comments as notes next to the fields/definitions?

[github-actions]

Claude finished @JavierCarnelli-ConductorOne's task in 2m 8s —— View job

---

PR Review: Incremental Sync Support

• Gather context and understand changes
• Review core implementation (event_feed.go, event_pagination.go)
• Review configuration and validation
• Review connector integration, tests, and examples
• Post detailed review

---

Overview

This PR adds incremental sync (event feed) support to baton-sql — a significant feature that enables efficient change detection via timestamp-based SQL queries instead of full re-syncs. The implementation supports three event source kinds: resource changes, grant changes, and grant revokes, each with independent cursors. Also adds Get() (targeted resource fetch by ID), required when incremental sync is configured.

The new test file (event_feed_test.go, 878 lines) addresses the test coverage gap flagged in all three prior reviews — this is a welcome and thorough addition.

Architecture Assessment

The design is clean and well-thought-out:

• Two-tier cursor model (event_pagination.go:17-25) — SourceCursors for committed state, CurrentSince/CurrentMaxSeen for in-flight state. This correctly prevents skipping events on crash/restart.
• Round-robin scheduling — One page per source per ListEvents call, advancing to the next source on exhaustion. The index-wrapping logic (event_feed.go:73-75, 167-169) is correct.
• Deterministic source ordering (event_pagination.go:72-77) — Lexicographic sort on resource type IDs ensures stable cursor semantics across restarts.
• Reuse of runQuery — Incremental queries share the existing query execution and pagination infrastructure, avoiding duplication.
• Crash safety — CurrentSince is pinned on the first page of each cycle (event_feed.go:93-95) and the committed cursor only advances on source exhaustion (event_feed.go:160-161). Well-designed.

Issues

1. GrantsIncrementalSync.resource_id CEL expression not compiled at startup (bug)

ResourceIncrementalSync.staticValidate compiles the optional resource_id CEL expression at startup (validate.go:215-218), catching typos early. But GrantsIncrementalSync.staticValidate (validate.go:223-261) only checks that resource_id is non-empty — it never calls s.env.Compile(). A bad CEL expression in grant incremental sync's resource_id would only fail at runtime during processGrantPage (event_feed.go:309).

This was flagged in the third review and remains unaddressed.

// validate.go:226 — should add after the empty check:
if err := s.env.Compile(l.ResourceId); err != nil {
    return fmt.Errorf("incremental_sync.resource_id: invalid CEL expression: %w", err)
}

Fix this →

2. EventFeeds error logging level too low (connector.go:59)

If GetSQLSyncers fails inside EventFeeds, incremental sync silently becomes unavailable with only a Debug-level log. This is a user-facing degradation that would be very difficult to troubleshoot in production.

// connector.go:59 — should be Warn, not Debug
l.Warn("baton-sql: failed to create syncers for event feed; incremental sync will be unavailable", zap.Error(err))

3. Grant incremental sync GrantMap field validation gap

In getSources (event_pagination.go:90-113), when a GrantsQuery has IncrementalSync configured, the source is created with GrantMap: gq.Map. However, there's no validation that gq.Map is non-nil or non-empty when IncrementalSync is configured. If a user configures incremental_sync on a grants query without map entries, processGrantPage would silently produce no events per row (the for _, mapping := range source.GrantMap loop at event_feed.go:318 iterates zero times), which could be confusing. A startup validation check would help.

4. getSources rebuilds on every ListEvents call (event_feed.go:68)

The source list is derived from static config and can't change at runtime. It could be precomputed in NewSQLEventFeed and stored on the struct. This was flagged in all three prior reviews. Not a correctness issue, but a simple optimization that avoids repeated allocation + sorting on every call.

Test Coverage Assessment

The new event_feed_test.go is comprehensive and well-structured:

• Unit tests for grantRowKey, getSources, commitAndAdvance, sinceForSource, marshalCursor/unmarshalCursor, toTime, defaultLookback, and mapGrantFromRow — all exercising edge cases and branch coverage.
• SQLite integration tests for processResourceChangePage, processGrantPage, and ListEvents end-to-end, including multi-page cursor pinning and out-of-bounds index clamping.
• Tests properly verify error propagation for unparseable cursor column values (both resource and grant paths).

One gap: validation tests for the new staticValidate methods in validate.go (lines 177-270) — GetQuery, ResourceIncrementalSync, GrantsIncrementalSync, and IncrementalSyncConfig — are still missing. The existing validate_test.go only covers ListQuery. These validators have enough branching (required fields, placeholder checks, reserved var names, CEL compilation) that dedicated test cases would catch regressions.

Add validation tests →

Good improvements since earlier reviews

• Test coverage added (event_feed_test.go, 878 lines) — Addresses the "must-fix" from all three prior reviews. Thorough coverage of the cursor state machine, source enumeration, time parsing, and end-to-end ListEvents.
• toTime parse failures now propagated as errors (event_feed.go:232-234, 299-301) — Fixes the silent fallback that could cause infinite reprocessing loops.
• grantRowKey handles numeric types (event_feed.go:420-427) — Correctly uses strconv.FormatInt for int64/float64 to avoid scientific notation.
• ResourceId field on ResourceIncrementalSync (config.go:492-495) — Decouples incremental queries from list mappings.
• earliestEvent comment (event_feed.go:55-57) — Clear explanation of the intentional design decision.
• EventFeeds statelessness comment (connector.go:48-51) — Documents why re-creating syncers is safe.

Minor observations

• The Compile method added to bcel.Env (bcel.go:47-53) is a clean addition that enables startup validation without evaluation.
• The parseTime function (referenced by toTime) is reused from the existing codebase — good consistency.
• Example files are well-documented with inline comments. @luisina-santos's question about keeping comments in example.yml is worth considering — the comments are helpful for users configuring incremental sync.

Summary

The architecture is solid and the test coverage gap has been meaningfully addressed. Two items remain:

1. Bug: Add CEL compile check for GrantsIncrementalSync.resource_id in validate.go (issue init sql mapper ideas #1) — this is a real validation gap that could cause runtime failures.
2. Should-fix: Promote the EventFeeds error log from Debug to Warn (issue Bump github.com/conductorone/baton-sdk from 0.2.27 to 0.2.31 #2) — important for production debuggability.

Everything else is low-severity or nice-to-have.

[ggreer]

Noticed a few things. I'm testing the branch out now and might add some more comments after that.

[ggreer]

Technically, event feeds and partial syncs are separate features, though the ability to do partial syncs is only useful if you can get a change log to determine what should be partially synced. I'd call a lot of these IncrementalSyncConfig & IncrementalSync attributes something like "EventFeedConfig" & "EventFeed" to make it clearer as to what's going on.

[ggreer]

Also I think this section should contain the query needed by the event feed, since event feeds might not be a 1:1 mapping to resource types. For example, there could be a table called audit_log that contains rows for when both users and groups were added/deleted. I've seen this in a decent number of APIs and I expect to see it in database schemas too.

[JavierCarnelli-ConductorOne]

I get your idea in both comments.
About the first one, the naming of the properties. I pictured this as a separate feature from any other Event Feed type implementation (such as LastLogIn or other similar).
Since we aim for this to be really loose and untied to any specific DB schemas, I think that having independent fields for each is convenient, cause we cannot be sure if there will be actually something like a "Log table", "Audit registry" or similar. If we name the properties like "Event Feed", we will be implying that it's used in more than just the incremental sync process, when we may need to develop some other specific configs for stuff like the LastLogIn or similar event dependent data.

About the second comment, I will relay on the same idea of we aiming for this to be really flexible for different DB schemas. We are likely to have just something like a date type column on each relational (or even entity) table that will be used to tell when something was modified (such as a permission grant or an update or some resource property). Keeping that in mind, it's not convenient to "join" or put in one config the Query to get the updated data, since this could be different for each resource.
I think that having it separated it's more dynamic and will adapt better for any type of arrangement in different customers DBs.

I get your point tho, I'm open to discuss further about this. Your approach may be convenient in some situations, so let me know if you don't agree!

[github-actions]

Claude finished @JavierCarnelli-ConductorOne's task in 3m 4s —— View job

---

PR Review: Incremental Sync Support

• Gather context and understand changes
• Review core implementation (event_feed.go, event_pagination.go)
• Review configuration and validation
• Review connector integration, tests, and examples
• Review outstanding issues from prior reviews
• Post detailed review

---

Overview

This PR adds incremental sync (event feed) support to baton-sql — a major feature enabling efficient change detection via timestamp-based SQL queries instead of full re-syncs. It also adds Get() (targeted resource fetch by ID), required when incremental sync is configured.

New files: event_feed.go (450 lines), event_pagination.go (127 lines), event_feed_test.go (878 lines)
Modified: config.go, validate.go, sql_syncer.go, connector.go, query.go, bcel.go, plus all example YAMLs

The implementation supports three event source kinds per resource type:

• Resource changes — detects new/modified resources via a timestamp column
• Grant changes — detects new grants
• Grant revokes — detects soft-deleted grants (optional)

Architecture Assessment

The design is well-structured and shows careful thought about crash safety:

• Two-tier cursor model (event_pagination.go:17-25) — SourceCursors holds committed state; CurrentSince/CurrentMaxSeen hold in-flight state. The committed cursor only advances when a source is fully exhausted (event_feed.go:160-161), which correctly prevents skipping events on crash/restart.
• Round-robin scheduling — One page per source per ListEvents call, with correct index-wrapping (event_feed.go:73-75, 167-169).
• Deterministic source ordering (event_pagination.go:72-77) — Lexicographic sort on resource type IDs ensures stable cursor semantics across restarts.
• Reuse of runQuery — Incremental queries share the existing query execution and pagination infrastructure, avoiding code duplication.
• Get() with gRPC NotFound (sql_syncer.go:73) — Correctly returns codes.NotFound so the SDK can handle deleted-between-event-and-sync gracefully.
• Type assertion guard (sql_syncer.go:25) — var _ connectorbuilder.ResourceTargetedSyncer = (*SQLSyncer)(nil) catches interface regressions at compile time. Good addition per ggreer's suggestion.

Issues

1. GrantsIncrementalSync.resource_id CEL expression not compiled at startup (bug)

ResourceIncrementalSync.staticValidate compiles the optional resource_id CEL expression at startup (validate.go:215-218), catching typos and bad expressions early. But GrantsIncrementalSync.staticValidate (validate.go:223-261) only checks that resource_id is non-empty — it never calls s.env.Compile(). A bad CEL expression in grant incremental sync's resource_id would only fail at runtime during processGrantPage (event_feed.go:309).

This was flagged in the third and fourth reviews and remains unaddressed.

// validate.go:226 — should add after the empty check:
if err := s.env.Compile(l.ResourceId); err != nil {
    return fmt.Errorf("incremental_sync.resource_id: invalid CEL expression: %w", err)
}

Fix this →

2. EventFeeds error logging level too low (connector.go:59)

If GetSQLSyncers fails inside EventFeeds, incremental sync silently becomes unavailable with only a Debug-level log. This is a user-facing degradation that would be very difficult to diagnose in production.

// connector.go:59 — should be Warn, not Debug
l.Warn("baton-sql: failed to create syncers for event feed; incremental sync will be unavailable", zap.Error(err))

This was flagged in the third and fourth reviews and remains unaddressed.

3. Naming: IncrementalSync vs EventFeed (per ggreer's comment)

ggreer noted (config.go:469) that event feeds and partial syncs are separate features, and suggested renaming IncrementalSyncConfig / IncrementalSync fields to EventFeedConfig / EventFeed to better communicate what they do. This is a naming/clarity concern worth considering, though it would be a larger refactor. The current naming is consistent internally but may confuse users who think of "incremental sync" as the SDK-level partial sync rather than the event feed that drives it.

4. Missing validation test coverage for new staticValidate methods

The existing validate_test.go only covers ListQuery. The new validation methods — GetQuery.staticValidate, ResourceIncrementalSync.staticValidate, GrantsIncrementalSync.staticValidate, and IncrementalSyncConfig.staticValidate — have enough branching (required fields, placeholder checks, reserved var names, CEL compilation) that dedicated test cases would catch regressions.

Add validation tests →

5. getSources rebuilds on every ListEvents call (event_feed.go:68)

The source list is derived from static config and can't change at runtime. It could be precomputed in NewSQLEventFeed and stored on the struct. This has been flagged in all four prior reviews. It's a simple optimization: compute in constructor, store on the struct, avoid repeated allocation + sort on every call.

Good improvements since earlier reviews

• Test coverage added (event_feed_test.go, 878 lines) — Comprehensive unit tests for grantRowKey, getSources, commitAndAdvance, sinceForSource, marshalCursor/unmarshalCursor, toTime, defaultLookback, and mapGrantFromRow. Integration tests for processResourceChangePage, processGrantPage, and end-to-end ListEvents.
• toTime parse failures propagated as errors (event_feed.go:232-234, 299-301) — Fixes the silent fallback that could cause infinite reprocessing loops.
• grantRowKey handles numeric types (event_feed.go:420-427) — Uses strconv.FormatInt for int64/float64 to avoid scientific notation per CLAUDE.md guidelines.
• ResourceId field on ResourceIncrementalSync (config.go:492-495) — Decouples incremental queries from list mappings.
• earliestEvent comment (event_feed.go:55-57) — Clear explanation of the intentional design decision.
• EventFeeds statelessness comment (connector.go:48-51) — Documents why re-creating syncers is safe.
• Type assertion guard (sql_syncer.go:25) — Per ggreer's suggestion.
• gRPC NotFound on Get (sql_syncer.go:73) — Per johnallers's and ggreer's comments.

Summary

The architecture is solid — the cursor state machine, crash safety guarantees, deterministic ordering, and reuse of existing infrastructure are all well-designed. The test coverage gap has been meaningfully addressed with 878 lines of tests.

Two items should be addressed before merging:

1. Bug: Add CEL compile check for GrantsIncrementalSync.resource_id in validate.go (issue init sql mapper ideas #1) — real validation gap that allows runtime failures from typos in config.
2. Logging: Promote EventFeeds error log from Debug to Warn (issue Bump github.com/conductorone/baton-sdk from 0.2.27 to 0.2.31 #2) — important for production debuggability.

The validation test coverage (issue #4) and getSources caching (issue #5) are worth doing but lower priority. The naming discussion (issue #3) is a larger design decision for the team.

[FeliLucero1]

This always says "grant changes" even when isRevoke=true

[JavierCarnelli-ConductorOne]

I think it's ok to say "grant changes" since that implies any modification on grants. Being a Grant or a Revoke

[FeliLucero1]

Why not an err? A user who configured incremental sync would have no indication why it isn't working

[JavierCarnelli-ConductorOne]

You mean why log a debug instead of an error?
We cannot return errors in this function since it's not supported and logging them would make the problem visible. Since we normally don't do that in connectors (for example, the common resource syncer in this connector returns nil without any logs) I thought on adding the debug log just in case we need some visibility here

[ggreer]

I'd like for the config to be tweaked so that we have fewer event feeds, but if that's too much trouble I think this can be approved. Feel free to slack me or whatever if you think that will result in this getting merged faster.

[ggreer]

Nitpick: I would call this section event_feeds or something similar, as it's possible to have event feeds unrelated to incremental sync, such as keeping track of when users logged in. We don't support that in baton-sql yet, but I'm sure we'll want it in the future.

[JavierCarnelli-ConductorOne]

I get your point. However this particular section is to config the look back value for the Incremental Sync process if it's convenient to have a specific value for some reason.
I think it's ok to name it like this, since we may have situations where we need different look back values for different event feed processes, right?
Maybe for a process that tracks when users logged in we want some other value for a particular reason. In that we would need a specific field for it.
This type of configs can be useful in some specific situation, it's a handy tool but I guess it won't be commonly used.
If this doesn't make sense to you, we can discuss it further I could be wrong

[ggreer]

Also I think this section should contain the query needed by the event feed, since event feeds might not be a 1:1 mapping to resource types. For example, there could be a table called audit_log that contains rows for when both users and groups were added/deleted. I've seen this in a decent number of APIs and I expect to see it in database schemas too.

[ggreer]

I think this config should live in a separate section for event feeds, though the query to get a specific resource by id should remain in each resource type. That would allow us to specify fewer event feeds to keep track of changes. We have to emit a list_events task for every event feed, so the fewer event feeds we have in a connector, the better it should perform.

[btipling]

I think Geoff's comments about event feed make sense, there's a disconnect between the event_feed implementation and the incremental sync name in the customer facing config. However a search for incremental sync across our connector shows we have a lot of config options that refer to incremental sync so it is consistent. I don't have any other feedback for this connector. I think I would make a case directly with Geoff with the name. I'm not going override his objection. Whatever this PR ships will become a permanent feature on its config and we should get it right.