Skip to content

feat: Save unflushed state on Close unless the sync was cancelled - #2631

Closed
disq wants to merge 1 commit into
mainfrom
fix/state-flush-on-close
Closed

disq wants to merge 1 commit into
mainfrom
fix/state-flush-on-close

Conversation

@disq

@disq disq commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

Summary

state.Client.Close now saves changes that were not flushed, so a source plugin that forgets Flush no longer loses its incremental cursors without an error.

  • Close drops the changes instead when the context passed to the state client was cancelled for a reason other than quota. The sync stopped early, and moving the cursors forward would skip records that were never written. scheduler.Sync returns an error only for cancellation, no tables, or max depth, so this matches the "flush unless the sync failed for a reason other than quota" rule that source plugins already follow.
  • Quota is matched with errors.As, because the monitor cancels with ErrNoQuota{team: ...}. The errors.Is(cause, premium.ErrNoQuota{}) check in plugins only matches an empty team name.
  • Flush with no pending changes no longer sends an empty insert, so Flush followed by Close writes once.
  • The Client interface does not change. Plugins need no code change.

Part of https://linear.app/env-zero/issue/ATL-340/fix-source-plugins-that-never-flush-the-state-client-silently-lose


  • Read the contribution guidelines 🧑‍🎓
  • Run go fmt to format your code 🖊
  • Lint your changes via golangci-lint run 🚨
  • Update or add tests 🧪: three new TestStateClose* tests run against memdb. Each one fails when its behavior is removed: no flush on Close (got ""), flush without the context check (got "dropped"), errors.Is in place of errors.As (got ""). make test passes.
  • Ensure the status checks below are successful ✅

@disq
disq marked this pull request as ready for review October 13, 2026 11:32
@disq
disq requested a review from a team as a code owner October 13, 2026 11:32
@disq
disq requested a review from alonnoga October 13, 2026 11:32
@disq

disq commented Oct 13, 2026

Copy link
Copy Markdown
Member Author

Closing this. Close cannot tell whether the sync succeeded, so saving there is not safe:

  • Plugins with their own sync loop (clickhouse, bigquery) run tables in an errgroup on the outer context. A failed table returns an error, but the outer context is not cancelled, so Close would save the cursors of the other tables, including partial ones.
  • A panic in Sync runs the deferred Close, which would save cursors before the process crashes.
  • The save in Close has no timeout, so Close could hang.
  • Most callers use a bare defer stateClient.Close(), so a failed save in Close would still be silent.

An explicit Flush is the only reliable success signal. A CI check in the plugins repo will catch plugins that never call it.

@disq disq closed this Oct 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants