Reset cp_sync_status on update of artifacts from gateway#2505
Conversation
📝 WalkthroughWalkthroughThe gateway config upsert now overwrites CP sync fields on update, and platform-api adds shared unique-violation detection plus retry handling for handle-based artifact import conflicts, with new regression and contract tests. ChangesUpsertConfig CP Sync Semantics
Shared Unique-Violation Handling and Import Retries
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/storage/sqlite_test.go (1)
1365-1397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't exercise
cp_sync_inforeset with a non-empty prior value.
UpdateCPSyncStatus(..., reason="")(Line 1379) leavescp_sync_infoatNULL, and the secondUpsertConfigcall never setscfg.CPSyncInfo, so it staysNULLthroughout. The test therefore only validatescp_sync_statusreset, not thecp_sync_inforeset the updated doc comment insql_store.goalso claims. Consider setting a non-emptycp_sync_inforeason before the update-triggering call and asserting it's cleared/overwritten to strengthen coverage of the actual behavior change.Suggested addition
// Simulate a successful DP->CP push: status success, CP UUID recorded. - assert.NilError(t, storage.UpdateCPSyncStatus(cfg.UUID, "cp-uuid-123", models.CPSyncStatusSuccess, "")) + assert.NilError(t, storage.UpdateCPSyncStatus(cfg.UUID, "cp-uuid-123", models.CPSyncStatusSuccess, "prior sync info"))assert.Equal(t, models.CPSyncStatusPending, retrieved.CPSyncStatus) // The CP UUID from the prior successful sync is preserved (not wiped by the update). assert.Equal(t, "cp-uuid-123", retrieved.CPArtifactID) + // cp_sync_info should be reset (not carried over from the prior success state). + assert.Equal(t, "", retrieved.CPSyncInfo)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/gateway-controller/pkg/storage/sqlite_test.go` around lines 1365 - 1397, The test in sqlite_test.go only verifies that UpsertConfig resets cp_sync_status for gateway-origin configs, but it never exercises cp_sync_info because UpdateCPSyncStatus is called with an empty reason and cfg.CPSyncInfo is never set. Update the test around UpdateCPSyncStatus and the second UpsertConfig call to seed a non-empty cp_sync_info value before the overwrite path, then assert that GetConfig returns the expected cleared or overwritten cp_sync_info after the update; keep the existing checks for CPSyncStatus and CPArtifactID in the same test case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@gateway/gateway-controller/pkg/storage/sqlite_test.go`:
- Around line 1365-1397: The test in sqlite_test.go only verifies that
UpsertConfig resets cp_sync_status for gateway-origin configs, but it never
exercises cp_sync_info because UpdateCPSyncStatus is called with an empty reason
and cfg.CPSyncInfo is never set. Update the test around UpdateCPSyncStatus and
the second UpsertConfig call to seed a non-empty cp_sync_info value before the
overwrite path, then assert that GetConfig returns the expected cleared or
overwritten cp_sync_info after the update; keep the existing checks for
CPSyncStatus and CPArtifactID in the same test case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 133fe693-ab51-459b-b261-9f623095e92a
📒 Files selected for processing (2)
gateway/gateway-controller/pkg/storage/sql_store.gogateway/gateway-controller/pkg/storage/sqlite_test.go
cb33be6 to
dd119a3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
platform-api/internal/service/artifact_import_test.go (2)
380-453: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test covering retry exhaustion.
Current tests only cover a single conflict resolved on the first retry. Add a case where
failOnceImporter(or a variant) fails on every call soimportConflictMaxRetriesis exhausted, assertingImportultimately returns the original error rather than looping incorrectly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/service/artifact_import_test.go` around lines 380 - 453, Add a retry-exhaustion test for ArtifactImport import conflicts: the current tests around TestArtifactImport_RetriesAsUpdateOnHandleConflict and TestArtifactImport_RetryPreservesLastInWinsForStalePush only cover a single conflict that succeeds on retry. Introduce a failing importer variant (or extend failOnceImporter) that returns the unique-violation on every call so the import path in svc.Import exhausts importConflictMaxRetries, then assert Import returns the original error and does not keep retrying indefinitely.
455-541: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider asserting no orphan
artifactsrow remains after the duplicate-handle violation.This test proves duplicate handles surface a detectable unique violation, but doesn't assert that the losing Create left no dangling parent
artifactsrow. This is downstream of the transactionality concern flagged inartifact_import.go(retry loop /resolveAndImport) — see that comment for the root-cause discussion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@platform-api/internal/service/artifact_import_test.go`:
- Around line 380-453: Add a retry-exhaustion test for ArtifactImport import
conflicts: the current tests around
TestArtifactImport_RetriesAsUpdateOnHandleConflict and
TestArtifactImport_RetryPreservesLastInWinsForStalePush only cover a single
conflict that succeeds on retry. Introduce a failing importer variant (or extend
failOnceImporter) that returns the unique-violation on every call so the import
path in svc.Import exhausts importConflictMaxRetries, then assert Import returns
the original error and does not keep retrying indefinitely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fe40f2fe-b8f3-4117-8b69-d9f4e9badb3c
📒 Files selected for processing (5)
platform-api/internal/repository/errors.goplatform-api/internal/repository/errors_test.goplatform-api/internal/repository/user_identity_mapping.goplatform-api/internal/service/artifact_import.goplatform-api/internal/service/artifact_import_test.go
| "display_name = ?", "version = ?", "data_version = ?", "kind = ?", "handle = ?", | ||
| "desired_state = ?", "deployment_id = ?", "origin = ?", | ||
| "updated_at = ?", "deployed_at = ?", | ||
| "cp_sync_status = ?", "cp_sync_info = ?", |
There was a problem hiding this comment.
So in the existing code, any updates to an Artifact are not reflected in the CP in general?
I mean, as defined in this issue #2475 the GW has to restart and switch on/off sync. Or is just a PUT /rest-apis also not working?
There was a problem hiding this comment.
Updates to artifacts using PUT calls when the sync was off were not reflected in AI Workspace when sync was turned on and the gateway connects. Updates to an artifact is reflected in the CP in real time when sync is on. The issue only arises when the update is done when sync is off.
3 participants
This pull request updates the config upsert logic to ensure that
cp_sync_statusandcp_sync_infoare always reset on update, not just on insert. This fixes an issue where these fields could become stale, especially for gateway-origin artifacts, and adds regression tests to prevent future breakage.Fixes -
Bug fix: Upsert logic for sync status fields
sql_store.goto clarify thatcp_sync_statusandcp_sync_infoare reset on update, whilecp_artifact_idandcreated_atare only set on insert.sql_store.goto explicitly setcp_sync_statusandcp_sync_infoon updates, ensuring their values are refreshed appropriately.Testing improvements
sqlite_test.goto verify that:cp_sync_statusto pending but preservescp_artifact_id.cp_sync_statusas NULL.