Skip to content

Conversation

@longfellowone
Copy link
Contributor

Summary

  • Ensure abort reasons used when closing shared sync client ports are cloneable across Comlink.
  • Add a regression test for both credentials and upload abort paths.

Changes

  • SharedSyncImplementation now aborts with a plain string reason when removing a port.
  • New tests in shared_sync_abort_reason.test.ts cover fetch-credentials and upload abort paths.
  • Changeset for @powersync/web.

Tests

  • pnpm --filter @powersync/common run build
  • pnpm --filter @powersync/web run build:tsc
  • pnpm vitest --run tests/shared_sync_abort_reason.test.ts

@changeset-bot
Copy link

changeset-bot bot commented Jan 4, 2026

🦋 Changeset detected

Latest commit: 4c2ea53

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@powersync/web Patch
@powersync/diagnostics-app Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@stevensJourney
Copy link
Collaborator

Could you please add more context for the issue this PR is trying to solve. Specifically the error you were observing before these changes.

From a quick refresher on the code. The abort reason is used to when aborting the uploadDataController, which will resolve the pending uploadCrud operation - Which means we don't actually pass any error from this flow. Which, from what I can tell, should not cause any clone errors when updating the SyncStatus for connected clients.

I haven't attempted to reproduce the issue this PR is trying to solve, but believe general cloning of unserializable Errors should be handled in #786 which will be merged and released soon.

protected serializeError(error?: Error) {

@longfellowone
Copy link
Contributor Author

Saw DataCloneError while digging into #808. The only value that is directly set in that teardown path is the abort reason
passed from removePort(). Making that reason a plain string ensures signal.reason is cloneable (even if the abort Event itself still isn’t).

Based on my understanding, the path this targets (fetchCredentialsController):

  • SharedSyncImplementation.fetchCredentials creates a Promise and sets abortController.signal.onabort = reject.
  • removePort() aborts the controller, so the Promise rejects with the abort Event (whose target.reason is the
    string).
  • If that rejection happens while the streaming loop is building a request (via AbstractRemote.getCredentials()
    fetchCredentials()), the error can in some cases bubble to the streaming loop and be assigned to dataFlow.downloadError (see
    AbstractStreamingSyncImplementation.ts).
  • SharedSyncImplementation.updateAllStatuses() broadcasts that status via Comlink, and SyncStatus.toJSON() does
    not sanitize errors.

@stevensJourney
Copy link
Collaborator

Saw DataCloneError while digging into #808. The only value that is directly set in that teardown path is the abort reason passed from removePort(). Making that reason a plain string ensures signal.reason is cloneable (even if the abort Event itself still isn’t).

Based on my understanding, the path this targets (fetchCredentialsController):

  • SharedSyncImplementation.fetchCredentials creates a Promise and sets abortController.signal.onabort = reject.
  • removePort() aborts the controller, so the Promise rejects with the abort Event (whose target.reason is the
    string).
  • If that rejection happens while the streaming loop is building a request (via AbstractRemote.getCredentials()
    fetchCredentials()), the error can in some cases bubble to the streaming loop and be assigned to dataFlow.downloadError (see
    AbstractStreamingSyncImplementation.ts).
  • SharedSyncImplementation.updateAllStatuses() broadcasts that status via Comlink, and SyncStatus.toJSON() does
    not sanitize errors.

I see. The serialization in updateAllStatuses should be fixed in PR 786. It seems like the fetchCredentials error is actually caused by calling reject with the Abort event from onabort.

IMO, a nicer fix is to pair sending the AbortOperation as the Error for reject and serialising the error correctly.

          fetchCredentials: async () => {
            const lastPort = await this.getLastWrappedPort();
            if (!lastPort) {
              throw new Error('No client port found to fetch credentials');
            }
            return new Promise(async (resolve, reject) => {
              const abortController = new AbortController();
              this.fetchCredentialsController = {
                controller: abortController,
                activePort: lastPort
              };
-              abortController.signal.onabort = reject;
+              abortController.signal.onabort = () => {
+                reject(abortController.signal.reason);
+              };

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