Skip to content

test(connections): cover the delete persist-before-notify sync invariant#1506

Merged
datlechin merged 1 commit into
mainfrom
test/sync-delete-ordering-invariant
May 30, 2026
Merged

test(connections): cover the delete persist-before-notify sync invariant#1506
datlechin merged 1 commit into
mainfrom
test/sync-delete-ordering-invariant

Conversation

@datlechin

Copy link
Copy Markdown
Member

Summary

Phase 8 / R-008: make the documented sync invariant executable. CLAUDE.md records that SyncChangeTracker.markDeleted() must run after saveConnections() — persist before notify, or a sync can re-upload a just-deleted record. ConnectionStorage.deleteConnection implements this (persist behind a guard, then markDeleted for non-local/non-sample connections), but it had no test.

Adds two regression tests against a real ConnectionStorage wired to a test-backed SyncChangeTracker/SyncMetadataStorage and a temp file:

  • Deleting a connection records a sync tombstone after it is persisted.
  • A delete that fails to persist (storage directory removed to force the saveConnections write to fail) records no tombstone — pinning the persist-before-notify guard.

Risk Addressed

  • R-008: sync invariants were documented but not enforced by tests; a regression in the delete ordering could silently re-upload deleted records across devices.

Verification

  • xcodebuild test -only-testing:TableProTests/ConnectionStorageSyncDeleteTests — both tests pass.
  • Test target builds.

Notes

  • Test-only change; no production code touched. No CHANGELOG / ABI / docs.
  • UserDefaults(suiteName:)! matches the existing storage-test convention (force_unwrapping is warning-level).
  • Scope note: the broader R-008 sync consolidation (shared record/conflict types in TableProSync, desktop adopting them) is blocked behind the same PluginKit single-module work as the desktop DatabaseType adoption, and is tracked separately.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 12436eabbf

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

func failedPersistenceRecordsNoTombstone() {
let connection = TestFixtures.makeConnection()
storage.addConnection(connection)
try? FileManager.default.removeItem(at: storageDirectory)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use a per-test temp directory before deleting it

When the Swift Testing runner executes storage tests concurrently, this removes the shared temporaryDirectory/tablepro-tests directory rather than only this test's fixture, because storageDirectory is just fileURL.deletingLastPathComponent(). Other tests in the repo create their own temp databases/files under the same parent (for example QueryHistoryStorageTests and SQLFavoriteStorageTests), so this test can nondeterministically delete another test's live file and cause unrelated failures; make the UUID a directory component and delete only that unique directory.

Useful? React with 👍 / 👎.

@datlechin datlechin merged commit f5d6853 into main May 30, 2026
4 checks passed
@datlechin datlechin deleted the test/sync-delete-ordering-invariant branch May 30, 2026 11:02
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.

1 participant