Skip to content

Conversation

sandhose
Copy link
Member

@sandhose sandhose commented May 7, 2025

  • Introduce a RepositoryFactory
  • Move the pool acquisition metric logic to the PgRepositoryFactory
  • Use the new RepositoryFactory everywhere
  • Don't hold db conns when creating a device on the compat login API

Copy link

cloudflare-workers-and-pages bot commented May 7, 2025

Deploying matrix-authentication-service-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8d7be72
Status: ✅  Deploy successful!
Preview URL: https://904c698e.matrix-authentication-service-docs.pages.dev
Branch Preview URL: https://quenting-dont-hold-db-conns.matrix-authentication-service-docs.pages.dev

View logs

@sandhose sandhose force-pushed the quenting/dont-hold-db-conns branch from 17e3732 to 8d7be72 Compare May 7, 2025 16:33
@sandhose sandhose requested a review from reivilibre May 7, 2025 16:35
@sandhose sandhose added the Z-Build-Workflow Add this label to trigger a build workflow for this pull request label May 7, 2025
{
// Something went wrong, let's end this session and schedule a device sync
let mut repo = repository_factory.create().await?;
let session = repo.compat_session().finish(&clock, session).await?;
Copy link
Member Author

Choose a reason for hiding this comment

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

There is one slight difference in behaviour now: if the device creation failed on Synapse, before we'd just rollback the transaction, meaning that the client could retry the request with the same login token and it would work. This isn't the case anymore

It may also end another session, since we now end sessions with the same device ID

To be fair, this request was never really idempotent, so if the client lost the response it would have had the same effect

@github-actions github-actions bot removed the Z-Build-Workflow Add this label to trigger a build workflow for this pull request label May 7, 2025
Copy link
Contributor

github-actions bot commented May 7, 2025

A build for this PR at commit 1091b51 has been created through the Z-Build-Workflow label by sandhose.

Docker image is available at:

  • ghcr.io/element-hq/matrix-authentication-service:pr-4527
  • ghcr.io/element-hq/matrix-authentication-service:sha-1091b51

Pre-built binaries are available through the workflow run artifacts.

@sandhose sandhose added the Z-Build-Workflow Add this label to trigger a build workflow for this pull request label May 7, 2025
@github-actions github-actions bot removed the Z-Build-Workflow Add this label to trigger a build workflow for this pull request label May 7, 2025
Copy link
Contributor

github-actions bot commented May 7, 2025

A build for this PR at commit 79a4673 has been created through the Z-Build-Workflow label by sandhose.

Docker image is available at:

  • ghcr.io/element-hq/matrix-authentication-service:pr-4527
  • ghcr.io/element-hq/matrix-authentication-service:sha-79a4673

Pre-built binaries are available through the workflow run artifacts.

};

/// A [`RepositoryFactory`] is a factory that can create a [`BoxRepository`]
// XXX(quenting): this could be generic over the repository type, but it's annoying to make it dyn-safe
Copy link
Contributor

Choose a reason for hiding this comment

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

meh, don't overcomplicate it any more than it needs to be. For this, I don't think we should worry about the boxing

@sandhose sandhose marked this pull request as ready for review May 9, 2025 07:13
@sandhose sandhose merged commit e9589ae into main May 9, 2025
38 checks passed
@sandhose sandhose deleted the quenting/dont-hold-db-conns branch May 9, 2025 07:13
@sandhose sandhose added A-Homeserver-Integration Integration with the homeserver T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. labels Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Homeserver-Integration Integration with the homeserver T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants