Skip to content

Return non-timeout read errors#87

Closed
nickvanw wants to merge 1 commit into
fix/delete-update-error-cursorfrom
fix/propagate-read-errors
Closed

Return non-timeout read errors#87
nickvanw wants to merge 1 commit into
fix/delete-update-error-cursorfrom
fix/propagate-read-errors

Conversation

@nickvanw

Copy link
Copy Markdown
Contributor

Summary

  • return non-timeout gRPC read errors from connectClient.Read instead of treating them as successful syncs
  • keep the existing binlog-expiration reset path as the explicit non-error exception
  • add regression coverage for callback serialization errors from row processing

Evidence

The live bugbash BIT test against PlanetScale hit a local serializer error: "unable to serialize row: failed to serialize DataType_LONG". connectClient.Read logged the Internal error but returned a nil error with a cursor, so Sync.Handle treated the failed row sync as successful and only emitted the truncate/checkpoint path. That can hide data serialization failures and checkpoint past rows that were not emitted.

Relationship to #78

#78 fixes which cursor is returned when callbacks fail. This PR fixes whether the callback error is returned at all. The two changes merge cleanly.

Validation

  • GOCACHE=/tmp/fivetran-bugbash.qcjUz1/gocache go test ./lib -run TestRead_ReturnsNonTimeoutCallbackErrors -count=1 -v
  • GOCACHE=/tmp/fivetran-bugbash.qcjUz1/gocache go test ./lib
  • GOCACHE=/tmp/fivetran-bugbash.qcjUz1/gocache go test ./...
  • git diff --check

@nickvanw nickvanw force-pushed the fix/propagate-read-errors branch 2 times, most recently from b0dcff0 to f8c670b Compare June 15, 2026 23:37
@nickvanw nickvanw force-pushed the fix/propagate-read-errors branch from f8c670b to de845ed Compare June 15, 2026 23:40
@nickvanw nickvanw changed the base branch from main to fix/delete-update-error-cursor June 15, 2026 23:40
@nickvanw

Copy link
Copy Markdown
Contributor Author

Closing in favor of #89, which folds the non-timeout read-error propagation into a single non-stacked PR with the cursor-safety fix and VStream schema guidance.

@nickvanw nickvanw closed this Jun 16, 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