Conversation
Implements scheduler.ScheduleProvider by querying tenant_schedule tables across all org_* schemas. Includes GormTenantScheduleRepository for schema discovery and per-schema querying, with Schedule.ID formatted as "tenant_id:schedule_name". Unit tests cover provider mapping, ID format, empty results, and error propagation. Integration tests verify multi-tenant queries against CockroachDB testcontainers.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds tenant-backed schedule discovery: a TenantScheduleRepository and TenantSchedule model, a TenantScheduleProvider that converts tenant schedules into scheduler.Schedule (ID formatted as "<tenant_id>:<schedule_name>"), and a GORM-backed repository that enumerates tenant schemas via information_schema and queries per-schema Changes
Sequence DiagramsequenceDiagram
participant Caller
participant TenantScheduleProvider
participant TenantScheduleRepository
participant Database as information_schema / <schema>.tenant_schedule
Caller->>TenantScheduleProvider: ListSchedules(ctx)
TenantScheduleProvider->>TenantScheduleRepository: ListEnabledSchedules(ctx)
TenantScheduleRepository->>Database: Query information_schema for tenant schemas
Database-->>TenantScheduleRepository: tenant schema list
loop per tenant schema
TenantScheduleRepository->>Database: Query `<schema>.tenant_schedule` WHERE enabled = true
Database-->>TenantScheduleRepository: enabled rows
alt query error
TenantScheduleRepository->>TenantScheduleRepository: log error / increment failure counter
end
end
TenantScheduleRepository-->>TenantScheduleProvider: []TenantSchedule (or ErrAllSchemaQueriesFailed if all queries failed)
TenantScheduleProvider->>TenantScheduleProvider: map to []scheduler.Schedule (ID = tenantID:scheduleName)
TenantScheduleProvider-->>Caller: []scheduler.Schedule
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Claude Code ReviewCommit: SummaryClean implementation of The latest commit ( One domain-level observation remains: Risk Assessment
Findings
Bot Review Notes
Previously Flagged
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
services/control-plane/internal/scheduling/provider_test.go (4)
86-88: Minor redundancy with compile-time check inprovider.go.This duplicates the interface assertion at
provider.go:76. Having it in tests is a valid documentation pattern, but note the redundancy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/control-plane/internal/scheduling/provider_test.go` around lines 86 - 88, Remove the redundant compile-time interface assertion test: delete the TestTenantScheduleProvider_ImplementsScheduleProvider function from provider_test.go since provider.go already contains the same assertion (var _ scheduler.ScheduleProvider = (*TenantScheduleProvider)(nil)); ensure TenantScheduleProvider still has the single assertion in provider.go and run tests.
166-188: Test assertions could be strengthened.
- Lines 175-176: Missing error checks on
Exec(same issue as the multi-tenant test).- Lines 183-187: The loop only checks schedules where
TenantID == "alpha"but doesn't verify the total count or fail if unexpected schedules appear.♻️ Suggested improvements
// Insert one enabled and one disabled - tc1.DB.Exec(fmt.Sprintf(`INSERT INTO %q.tenant_schedule (schedule_name, saga_name, cron_expr, enabled) VALUES ('enabled-job', 'job.run', '0 0 * * *', true)`, tc1.Tenant.SchemaName())) - tc1.DB.Exec(fmt.Sprintf(`INSERT INTO %q.tenant_schedule (schedule_name, saga_name, cron_expr, enabled) VALUES ('disabled-job', 'noop.run', '0 0 * * *', false)`, tc1.Tenant.SchemaName())) + require.NoError(t, tc1.DB.Exec(fmt.Sprintf(`INSERT INTO %q.tenant_schedule (schedule_name, saga_name, cron_expr, enabled) VALUES ('enabled-job', 'job.run', '0 0 * * *', true)`, tc1.Tenant.SchemaName())).Error) + require.NoError(t, tc1.DB.Exec(fmt.Sprintf(`INSERT INTO %q.tenant_schedule (schedule_name, saga_name, cron_expr, enabled) VALUES ('disabled-job', 'noop.run', '0 0 * * *', false)`, tc1.Tenant.SchemaName())).Error) repo := NewGormTenantScheduleRepository(tc1.DB, nil) schedules, err := repo.ListEnabledSchedules(context.Background()) require.NoError(t, err) + + // Filter to alpha schedules and verify only enabled-job is returned + var alphaSchedules []TenantSchedule + for _, s := range schedules { + if s.TenantID == "alpha" { + alphaSchedules = append(alphaSchedules, s) + } + } + require.Len(t, alphaSchedules, 1, "should return exactly one enabled schedule from alpha") + assert.Equal(t, "enabled-job", alphaSchedules[0].ScheduleName) - for _, s := range schedules { - if s.TenantID == "alpha" { - assert.Equal(t, "enabled-job", s.ScheduleName, "only enabled schedules should be returned") - } - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/control-plane/internal/scheduling/provider_test.go` around lines 166 - 188, In TestGormTenantScheduleRepository_ListEnabledSchedules_OnlyEnabled strengthen the test by checking errors returned from the two tc1.DB.Exec(...) calls (capture and require.NoError on each Exec) and assert the results of repo.ListEnabledSchedules more strictly: require that err is nil, require.Len(schedules, 1) (or assert the exact expected count) and assert that the single returned schedule has TenantID "alpha" and ScheduleName "enabled-job" and that no schedule with enabled==false is present; this uses the existing test function TestGormTenantScheduleRepository_ListEnabledSchedules_OnlyEnabled and the repository method ListEnabledSchedules to locate where to add the checks.
190-207: Test setup creates unused resources; test name may be misleading.
setupTestDB(t)at line 195 creates tenant schemas that are immediately discarded. The test then uses a fresh database connection without tenant schemas. This tests "no tenant schemas exist" rather than "tenant schemas exist but have no schedules."Consider either:
- Removing the unused
setupTestDBcall, or- Using the existing tenant contexts without inserting schedules to test truly empty tenant tables
♻️ Option 1: Remove unused setup
func TestGormTenantScheduleRepository_ListEnabledSchedules_EmptyTenants(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") } - _, _, cleanup := setupTestDB(t) - defer cleanup() - - // No schedules inserted - both tenants have empty tables db, dbCleanup := testdb.SetupCockroachDB(t, nil) defer dbCleanup() repo := NewGormTenantScheduleRepository(db, nil) schedules, err := repo.ListEnabledSchedules(context.Background()) require.NoError(t, err) assert.Empty(t, schedules) }♻️ Option 2: Test empty tenant tables
func TestGormTenantScheduleRepository_ListEnabledSchedules_EmptyTenants(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") } tc1, _, cleanup := setupTestDB(t) defer cleanup() // No schedules inserted - tenant tables exist but are empty - db, dbCleanup := testdb.SetupCockroachDB(t, nil) - defer dbCleanup() - repo := NewGormTenantScheduleRepository(db, nil) + repo := NewGormTenantScheduleRepository(tc1.DB, nil) schedules, err := repo.ListEnabledSchedules(context.Background()) require.NoError(t, err) assert.Empty(t, schedules) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/control-plane/internal/scheduling/provider_test.go` around lines 190 - 207, The test TestGormTenantScheduleRepository_ListEnabledSchedules_EmptyTenants creates tenant schemas via setupTestDB(t) but then ignores them and uses a fresh DB from testdb.SetupCockroachDB; remove the unused setupTestDB call (and its deferred cleanup) so the test only uses the db returned by testdb.SetupCockroachDB, or alternatively switch to using the tenant DB returned by setupTestDB and drop the extra testdb.SetupCockroachDB call; update the test to call NewGormTenantScheduleRepository against the single intended DB and keep the assertion that ListEnabledSchedules returns no schedules.
111-126: Simplify DDL formatting to avoid double substitution.Lines 117 and 120 use a confusing double-formatting pattern:
fmt.Sprintf(tenantScheduleDDL, "%s", "alpha")leaves a literal%sin the DDL thattestdb.CreateTablelater fills via its ownfmt.Sprintf. While this works becauseCreateTablehandles the remaining substitution on line 89, it's unnecessarily complex.Simplify by letting
CreateTableperform all formatting:testdb.CreateTable(t, tc1.DB, tc1.Tenant, tenantScheduleDDL)Then update
tenantScheduleDDLconstraint to use only one%splaceholder for the schema name, and name constraints consistently per schema. Or, if constraint names must differ, pre-format only the constraint suffix and pass the pre-formatted DDL toCreateTable—but document why the pattern exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/control-plane/internal/scheduling/provider_test.go` around lines 111 - 126, The DDL is being double-formatted which leaves a literal `%s` for testdb.CreateTable to fill; update setupTestDB so it passes tenantScheduleDDL directly to testdb.CreateTable (remove fmt.Sprintf around tenantScheduleDDL in setupTestDB), and then adjust tenantScheduleDDL to contain exactly one `%s` for the schema name (and make constraint names include the schema or a consistent suffix so they are unique per tenant) so CreateTable's internal fmt.Sprintf can substitute the schema once; references: setupTestDB, testdb.CreateTable, tenantScheduleDDL.services/control-plane/internal/scheduling/provider.go (1)
107-130: Silent partial failures may mask systemic issues.The current design logs per-schema query errors and continues, which is resilient. However, if all tenant queries fail, the caller receives an empty slice with no indication of widespread failure. Consider tracking failed schemas and returning an error (or at least a warning log) if all or most schemas fail.
♻️ Optional: Track and surface partial failures
var allSchedules []TenantSchedule + var failedSchemas int for _, schema := range schemas { tenantID := schemaToTenantID(schema) var entities []TenantScheduleEntity err := r.db.WithContext(ctx). Table(fmt.Sprintf("%q.tenant_schedule", schema)). Where("enabled = ?", true). Find(&entities).Error if err != nil { r.logger.Error("failed to query tenant schedules", "schema", schema, "error", err) + failedSchemas++ continue } for _, e := range entities { allSchedules = append(allSchedules, TenantSchedule{ TenantID: tenantID, ScheduleName: e.ScheduleName, SagaName: e.SagaName, CronExpr: e.CronExpr, Metadata: e.Metadata, }) } } + if failedSchemas > 0 && failedSchemas == len(schemas) { + return nil, fmt.Errorf("all %d tenant schema queries failed", failedSchemas) + } + if failedSchemas > 0 { + r.logger.Warn("some tenant schema queries failed", + "failed", failedSchemas, "total", len(schemas)) + } + return allSchedules, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/control-plane/internal/scheduling/provider.go` around lines 107 - 130, The loop in the method that gathers schedules silently continues on per-schema DB errors (the block using r.db.WithContext(...).Table(...).Where(...).Find(&entities).Error and logging via r.logger.Error), which can hide systemic failures; modify this routine to track failedSchemas (or a failure counter) while iterating schemas and after the loop return an error (or at minimum a warning log) when all schemas failed or when failures exceed a configurable threshold, otherwise return the collected allSchedules as before; ensure you reference the TenantScheduleEntity parsing and preserve existing per-schema logging but also surface an aggregated error via the function's error return (or a clear r.logger.Warn if the function cannot return errors).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/control-plane/internal/scheduling/provider_test.go`:
- Around line 136-142: The DB.Exec calls in provider_test.go (the tc1.DB.Exec
and tc2.DB.Exec statements inserting into tenant_schedule) ignore returned
errors; update each Exec invocation to capture the result and error and fail the
test on error (e.g., check err and call t.Fatalf / require.NoError) so failed
INSERTs don't go unnoticed. Ensure you change all three Exec calls (the one for
tc1 and both for tc2) to validate the error return and surface it in the test.
---
Nitpick comments:
In `@services/control-plane/internal/scheduling/provider_test.go`:
- Around line 86-88: Remove the redundant compile-time interface assertion test:
delete the TestTenantScheduleProvider_ImplementsScheduleProvider function from
provider_test.go since provider.go already contains the same assertion (var _
scheduler.ScheduleProvider = (*TenantScheduleProvider)(nil)); ensure
TenantScheduleProvider still has the single assertion in provider.go and run
tests.
- Around line 166-188: In
TestGormTenantScheduleRepository_ListEnabledSchedules_OnlyEnabled strengthen the
test by checking errors returned from the two tc1.DB.Exec(...) calls (capture
and require.NoError on each Exec) and assert the results of
repo.ListEnabledSchedules more strictly: require that err is nil,
require.Len(schedules, 1) (or assert the exact expected count) and assert that
the single returned schedule has TenantID "alpha" and ScheduleName "enabled-job"
and that no schedule with enabled==false is present; this uses the existing test
function TestGormTenantScheduleRepository_ListEnabledSchedules_OnlyEnabled and
the repository method ListEnabledSchedules to locate where to add the checks.
- Around line 190-207: The test
TestGormTenantScheduleRepository_ListEnabledSchedules_EmptyTenants creates
tenant schemas via setupTestDB(t) but then ignores them and uses a fresh DB from
testdb.SetupCockroachDB; remove the unused setupTestDB call (and its deferred
cleanup) so the test only uses the db returned by testdb.SetupCockroachDB, or
alternatively switch to using the tenant DB returned by setupTestDB and drop the
extra testdb.SetupCockroachDB call; update the test to call
NewGormTenantScheduleRepository against the single intended DB and keep the
assertion that ListEnabledSchedules returns no schedules.
- Around line 111-126: The DDL is being double-formatted which leaves a literal
`%s` for testdb.CreateTable to fill; update setupTestDB so it passes
tenantScheduleDDL directly to testdb.CreateTable (remove fmt.Sprintf around
tenantScheduleDDL in setupTestDB), and then adjust tenantScheduleDDL to contain
exactly one `%s` for the schema name (and make constraint names include the
schema or a consistent suffix so they are unique per tenant) so CreateTable's
internal fmt.Sprintf can substitute the schema once; references: setupTestDB,
testdb.CreateTable, tenantScheduleDDL.
In `@services/control-plane/internal/scheduling/provider.go`:
- Around line 107-130: The loop in the method that gathers schedules silently
continues on per-schema DB errors (the block using
r.db.WithContext(...).Table(...).Where(...).Find(&entities).Error and logging
via r.logger.Error), which can hide systemic failures; modify this routine to
track failedSchemas (or a failure counter) while iterating schemas and after the
loop return an error (or at minimum a warning log) when all schemas failed or
when failures exceed a configurable threshold, otherwise return the collected
allSchedules as before; ensure you reference the TenantScheduleEntity parsing
and preserve existing per-schema logging but also surface an aggregated error
via the function's error return (or a clear r.logger.Warn if the function cannot
return errors).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e70ffe13-4aad-4d43-90e1-6f68bb6b6d33
📒 Files selected for processing (2)
services/control-plane/internal/scheduling/provider.goservices/control-plane/internal/scheduling/provider_test.go
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Add error checks on all DB.Exec calls in tests - Track failed schemas and return error when all queries fail - Simplify DDL to single %s placeholder for CreateTable - Fix EmptyTenants test to use setupTestDB's DB (tests empty tables) - Strengthen OnlyEnabled test with explicit count assertion - Remove redundant interface assertion test (already in provider.go)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
services/control-plane/internal/scheduling/provider_test.go (1)
189-203: Add a regression test for full-failure/cancellation paths.Current integration coverage misses the repository behavior when schema queries all fail (and when context is canceled). Please add one test asserting
errors.Is(err, ErrAllSchemaQueriesFailed)and one asserting cancellation is surfaced.🧪 Suggested test additions
+func TestGormTenantScheduleRepository_ListEnabledSchedules_AllSchemaQueriesFail(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + + tc1, _, cleanup := setupTestDB(t) + defer cleanup() + + // Break every schema query by dropping tenant_schedule from all discovered schemas. + require.NoError(t, tc1.DB.Exec(fmt.Sprintf(`DROP TABLE %q.tenant_schedule`, tc1.Tenant.SchemaName())).Error) + require.NoError(t, tc1.DB.Exec(`DROP TABLE IF EXISTS "org_beta".tenant_schedule`).Error) + + repo := NewGormTenantScheduleRepository(tc1.DB, nil) + _, err := repo.ListEnabledSchedules(context.Background()) + require.Error(t, err) + assert.ErrorIs(t, err, ErrAllSchemaQueriesFailed) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/control-plane/internal/scheduling/provider_test.go` around lines 189 - 203, Add two regression tests for GormTenantScheduleRepository's ListEnabledSchedules: one that simulates all schema queries failing and asserts errors.Is(err, ErrAllSchemaQueriesFailed), and one that cancels the context before calling ListEnabledSchedules and asserts the cancellation error is returned; locate tests near TestGormTenantScheduleRepository_ListEnabledSchedules_EmptyTenants, use NewGormTenantScheduleRepository(tc.DB, ...) and call repo.ListEnabledSchedules(ctx) with a crafted DB/driver or transaction that forces schema query failures for the first test and with a context canceled via context.WithCancel for the second test, and use require/ assert to check the specific error conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/control-plane/internal/scheduling/provider.go`:
- Around line 120-125: The loop that logs query errors (using r.logger.Error and
incrementing failedSchemas) must not swallow context cancellation/timeouts: if
the error is context.Canceled or context.DeadlineExceeded (or ctx.Err() != nil)
you should immediately propagate the context error rather than treating it as a
schema-level failure and continuing; update the error handling in the
QueryTenantSchedules (or the provider method containing failedSchemas and that
return at Line 146) to check errors.Is(err, context.Canceled) || errors.Is(err,
context.DeadlineExceeded) (or check ctx.Err()) and return nil, ctx.Err() (or
wrap and return that context error) instead of incrementing
failedSchemas/continuing; apply the same change to the similar block around
Lines 138–146 so cancellation/timeouts are not hidden when returning partial
schedules.
---
Nitpick comments:
In `@services/control-plane/internal/scheduling/provider_test.go`:
- Around line 189-203: Add two regression tests for
GormTenantScheduleRepository's ListEnabledSchedules: one that simulates all
schema queries failing and asserts errors.Is(err, ErrAllSchemaQueriesFailed),
and one that cancels the context before calling ListEnabledSchedules and asserts
the cancellation error is returned; locate tests near
TestGormTenantScheduleRepository_ListEnabledSchedules_EmptyTenants, use
NewGormTenantScheduleRepository(tc.DB, ...) and call
repo.ListEnabledSchedules(ctx) with a crafted DB/driver or transaction that
forces schema query failures for the first test and with a context canceled via
context.WithCancel for the second test, and use require/ assert to check the
specific error conditions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f7daf58-a1b9-47f8-8c48-472c33fae8fb
📒 Files selected for processing (2)
services/control-plane/internal/scheduling/provider.goservices/control-plane/internal/scheduling/provider_test.go
Return immediately with the context error when a per-schema query fails due to context.Canceled or context.DeadlineExceeded, rather than silently treating it as a schema-level failure.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
bjcoombs
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
Stale: superseded by latest approval
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
scheduler.ScheduleProviderinterface viaTenantScheduleProviderthat queriestenant_scheduletables across allorg_*tenant schemasGormTenantScheduleRepositorydiscovers tenant schemas usinginformation_schemaand queries each for enabled schedulestenant_id:schedule_namefor unique identification across tenantsChanges
services/control-plane/internal/scheduling/provider.go- Provider, repository interface, GORM implementationservices/control-plane/internal/scheduling/provider_test.go- Unit tests (provider mapping, ID format, error propagation) and integration tests (multi-tenant CockroachDB)Test plan
var _ scheduler.ScheduleProvider = ...)