Repository navigation
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
8ec386b to
126c8ff
Compare
miparnisari
left a comment
There was a problem hiding this comment.
Used Postman and tried all the APIs against CRDB and Spanner. Notes:
- we're missing this change for Spanner. You can use
docker-compose -f docker-compose.spanner.yaml up --buildto test. E.g.- call to WriteSchema I get
"unable to list caveats: spanner: code = \"NotFound\", desc = \"Table not found: caveat\", requestID = \"1.23ff9b5081a896f8.1.1.70.1\"",. - call to DeleteRelationships I get
"object definition doc not found". This line has to be updated to account for when the error description says "table"spicedb/internal/datastore/spanner/reader.go
Line 300 in f41f641
- call to WriteSchema I get
- in many cases, the errors returned to the user are prefixed with
error encoding zedtoken: unable to read unique IDwhich makes no sense to me, and probably not to the end user 😆 It comes from. i'm very tempted to remove thatspicedb/pkg/zedtoken/zedtoken.go
Lines 71 to 74 in 01d72a1
error encoding zedtokenand to clarify what the "unique ID" is - in spanner, if you didn't run migrations, calling Watch api gives a HTTP 400. But against CRDB, the watch service is completely disabled:
unknown service authzed.api.v1.WatchService, which means that you need to re-boot the server after you ran the migrations. I wonder if the server is supposed to be hable to handle migrations running at the same time ?
Anyway. Could you update
spicedb/internal/datastore/crdb/crdb.go
Line 576 in 3370198
- when you call ImportBulk, you get this
{
"code": 2,
"message": "statement description failed: ERROR: relation \"relation_tuple\" does not exist (SQLSTATE 42P01)",
"details": []
}, seems like we missed an error handling here:
| err, | ||
| codes.FailedPrecondition, | ||
| spiceerrors.ForReason( | ||
| v1.ErrorReason_ERROR_REASON_UNSPECIFIED, |
There was a problem hiding this comment.
nit: please create the right error code
| // instructing the user to run migrations. | ||
| func NewSchemaNotInitializedError(underlying error) error { | ||
| return SchemaNotInitializedError{ | ||
| fmt.Errorf("datastore error: the database schema has not been initialized; please run \"spicedb datastore migrate\": %w", underlying), |
There was a problem hiding this comment.
| fmt.Errorf("datastore error: the database schema has not been initialized; please run \"spicedb datastore migrate\": %w", underlying), | |
| fmt.Errorf("%w. please run \"spicedb datastore migrate\", underlying), |
| if wrappedErr := pgxcommon.WrapMissingTableError(err); wrappedErr != nil { | ||
| return nil, datastore.NoRevision, wrappedErr |
There was a problem hiding this comment.
| if wrappedErr := pgxcommon.WrapMissingTableError(err); wrappedErr != nil { | |
| return nil, datastore.NoRevision, wrappedErr | |
| if pgxcommon.IsMissingTableError(err) { | |
| err = common.NewSchemaNotInitializedError(err) | |
| } |
to keep the same pattern as the lines before
| pgReadOnlyTransaction = "25006" | ||
| pgQueryCanceled = "57014" | ||
| pgInvalidArgument = "22023" | ||
| pgMissingTable = "42P01" |
There was a problem hiding this comment.
there are two constants with the same value defined elsewhere: postgresMissingTableErrorCode
please remove them in favor of this one
| cterr := tx.QueryRow(ctx, sql, args...).Scan(&newXID, &newSnapshot, ×tamp) | ||
| if cterr != nil { | ||
| if wrappedErr := common.WrapMissingTableError(cterr); wrappedErr != nil { | ||
| return newXID, newSnapshot, timestamp, wrappedErr |
There was a problem hiding this comment.
| return newXID, newSnapshot, timestamp, wrappedErr | |
| cterr = .... |
| }) | ||
| if err != nil { | ||
| if wrappedErr := pgxcommon.WrapMissingTableError(err); wrappedErr != nil { | ||
| return nil, wrappedErr |
There was a problem hiding this comment.
| return nil, wrappedErr | |
| err = wrappedErr |
otherwise you lose the errUnableToListNamespaces below
|
I also wonder if instead of littering all the datastore implementations with error handling, we could just prevent the server from coming up online when you have never ran E.g. here spicedb/internal/services/health/health.go Lines 112 to 115 in 70cd757 if I'm sure there are more changes required than this, perhaps an update to |
- Add TODO comment for future ERROR_REASON_DATASTORE_NOT_MIGRATED - Simplify error message format to: "%w. please run migrate" - Use IsMissingTableError + reassignment pattern (not early return) - Export PgMissingTable constant, remove duplicates from migration drivers - Add Spanner IsMissingTableError support in reader.go and caveat.go - Differentiate CRDB watch service error message for missing tables - Add ImportBulk error handling in bulk.go - Update tests for new error message format 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add TODO comment for future ERROR_REASON_DATASTORE_NOT_MIGRATED - Simplify error message format to: "%w. please run migrate" - Use IsMissingTableError + reassignment pattern (not early return) - Export PgMissingTable constant, remove duplicates from migration drivers - Add Spanner IsMissingTableError support in reader.go and caveat.go - Differentiate CRDB watch service error message for missing tables - Add ImportBulk error handling in bulk.go - Update tests for new error message format
ee34e7f to
7c53e96
Compare
|
I replaced scattered Before: 15 files with scattered IsMissingTableError checks (easy to miss spots) |
ab8dd24 to
33b2e94
Compare
3d37e2d to
d873dd4
Compare
131f022 to
7176e82
Compare
- Add TODO comment for future ERROR_REASON_DATASTORE_NOT_MIGRATED - Simplify error message format to: "%w. please run migrate" - Use IsMissingTableError + reassignment pattern (not early return) - Export PgMissingTable constant, remove duplicates from migration drivers - Add Spanner IsMissingTableError support in reader.go and caveat.go - Differentiate CRDB watch service error message for missing tables - Add ImportBulk error handling in bulk.go - Update tests for new error message format
a03d096 to
10f604e
Compare
- Add TODO comment for future ERROR_REASON_DATASTORE_NOT_MIGRATED - Simplify error message format to: "%w. please run migrate" - Use IsMissingTableError + reassignment pattern (not early return) - Export PgMissingTable constant, remove duplicates from migration drivers - Add Spanner IsMissingTableError support in reader.go and caveat.go - Differentiate CRDB watch service error message for missing tables - Add ImportBulk error handling in bulk.go - Update tests for new error message format
10f604e to
c3ce28a
Compare
- Add TODO comment for future ERROR_REASON_DATASTORE_NOT_MIGRATED - Simplify error message format to: "%w. please run migrate" - Use IsMissingTableError + reassignment pattern (not early return) - Export PgMissingTable constant, remove duplicates from migration drivers - Add Spanner IsMissingTableError support in reader.go and caveat.go - Differentiate CRDB watch service error message for missing tables - Add ImportBulk error handling in bulk.go - Update tests for new error message format
acd0018 to
2eefa26
Compare
- Add TODO comment for future ERROR_REASON_DATASTORE_NOT_MIGRATED - Simplify error message format to: "%w. please run migrate" - Use IsMissingTableError + reassignment pattern (not early return) - Export PgMissingTable constant, remove duplicates from migration drivers - Add Spanner IsMissingTableError support in reader.go and caveat.go - Differentiate CRDB watch service error message for missing tables - Add ImportBulk error handling in bulk.go - Update tests for new error message format
f918ab5 to
09f257a
Compare
- Add TODO comment for future ERROR_REASON_DATASTORE_NOT_MIGRATED - Simplify error message format to: "%w. please run migrate" - Use IsMissingTableError + reassignment pattern (not early return) - Export PgMissingTable constant, remove duplicates from migration drivers - Add Spanner IsMissingTableError support in reader.go and caveat.go - Differentiate CRDB watch service error message for missing tables - Add ImportBulk error handling in bulk.go - Update tests for new error message format
5135842 to
4a1ecdc
Compare
|
Squashed to 2 commits and addressed all feedback:
Zedtoken prefix and Watch API behavior seem like separate concerns - happy to tackle in follow-up PRs. Unit test failure ( |
ea16385 to
cdcc99d
Compare
a7ddcc6 to
ef7c4d9
Compare
77ca4fc to
056ba22
Compare
Add SchemaNotInitializedError type that wraps missing-table errors with a helpful message instructing users to run migrations. Uses IsMissingTableError checks across all datastores (CRDB, Postgres, MySQL, Spanner) with consistent error handling that preserves caller context.
Add readiness gate that blocks gRPC requests until the datastore is ready. Uses singleflight to deduplicate concurrent checks and caches results briefly. Only blocks for migration-related issues; transient states like connection pool warmup are allowed through.
056ba22 to
64db87b
Compare
Bump authzed-go to v1.8.0 which includes the new error reason from authzed/api#159. Replace ERROR_REASON_UNSPECIFIED with the proper structured error code.
Satisfies the usetesting linter rule introduced in the latest golangci-lint version.
The readiness gate re-checked the datastore every 500ms for the life of the process. Once a newer SpiceDB migrated the datastore, older servers would report not migrated and reject every request. Keep the gate open once the datastore has been ready, matching the health checker, and return ERROR_REASON_DATASTORE_NOT_MIGRATED from the gate. Remove the unused WrapMissingTableError helpers and the unused MySQL error file, and add tests against unmigrated postgres, crdb and spanner databases.
4e942e4 to
07de1f0
Compare
When users start SpiceDB without running migrations first, they get a confusing error like:
This doesn't tell them what went wrong or how to fix it.
Now the error message is clear and actionable:
This applies to all SQL datastores (postgres, mysql, crdb) and covers read operations, write operations, and statistics queries.
Closes #2749
Readiness gate latches once ready
The readiness middleware now acts as a startup migration gate: after the datastore first reports ready, the gate remains open for the lifetime of the process. This matches the existing health manager, which marks services serving and stops checking after the datastore and dispatcher become ready. Previously, the gate rechecked readiness every 500ms, so an older running pod could begin rejecting requests after a newer SpiceDB version advanced the datastore to a revision the older binary did not recognize. Latching prevents that additional rejection during rolling upgrades and removes readiness queries from the normal request path after startup. Before the gate opens, migration-related failures still return
FailedPreconditionwithERROR_REASON_DATASTORE_NOT_MIGRATED.