Inintended caching of EntityManager by EntityRepository from within a transactional context

[hesalx]

Describe the bug

According to the docs (https://mikro-orm.io/docs/identity-map#how-does-requestcontext-helper-work) it should be pefectly safe to use a more "global" EM instance from within a transactional or request context (i.e. not having to pass down the EM instance passed to the transaction callback).

However, this expectation breaks in at least one specific scenario that is very easy to replicate.

When the first time a repository is accessed is within a transaction but from the "outside" EM instance (the one on which .transactional was called) then the repository will get "stuck" with the transaction-specific EM instance (can be verified by comapring em.id) which in the best case leads to DriverException: Transaction query already complete and in the worst case can cause data corruption due to improper isolation.

This problem can occur naturally when using MikroORM with NestJS and having services that call one another while each relies primarily on the (global) injected EM instance.

It is worth noting that .getContext() on the "stuck" EM instance is a no-op and returns self, thus the repository is corrupted for the entire lifetime of the application process.

Reproduction

Reproduction:

const orm = await MikroORM.init({});
// EM like the one injected in NestJS
const em = orm.em.fork({ useContext: true });
await em.transactional(async _ => {
    // this could happen in a nested service without access to tx-specific EM
    await em.getRepository(Post).find({id: 1});
});
// some time later, even in a different request
await em.getRepository(Post).find({id: 1}); 
// ^ DriverException: Transaction query already complete, run with DEBUG=knex:tx for more info

// the following does not work anymore, the EM of this repo is forever "stuck"
await em.getRepository(Post).getEntityManager().getContext().find(Post, {id: 1}); 

// em.id !== em.getRepository(Post).getEntityManager().id 

If the repository happened to be accessed prior to a transaction, the issue does not happen:

const orm = await MikroORM.init({});
const em = orm.em.fork({ useContext: true });
// First repo access is outside a transaction
await em.getRepository(Post).find({id: 1});
await em.transactional(async _ => {
    await em.getRepository(Post).find({id: 1});
});
await em.getRepository(Post).find({id: 1}); // works!
// em.id === em.getRepository(Post).getEntityManager().id 

Using tx-specific instance or explicitly calling .getContext() inside the transaction also resolves the issue:

const orm = await MikroORM.init({});
const em = orm.em.fork({ useContext: true });
await em.transactional(async emtx => {
    await emtx.getRepository(Post).find({id: 1});
    await em.getContext().getRepository(Post).find({id: 1});
});
await em.getRepository(Post).find({id: 1}); // works!
// em.id === em.getRepository(Post).getEntityManager().id 

What driver are you using?

@mikro-orm/postgresql

MikroORM version

6.1.12

Node.js version

18.16.0

Operating system

MacOS

Validations

• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Slack.
• The provided reproduction is a minimal reproducible example of the bug.

[B4nan]

That's funny, the commit that broke this (af87693) was trying to fix the exact same error (#3074). Will need to reread that thread, as reverting this would probably break some other case...

[hesalx]

It is worth noting that the aforementioned af87693 explicitly states "do not use context in em.getRepository()", however it is still used there (v6.1.12) and was added by b5b03a6#diff-a62cd78c400d99253a3c0fb5a9bf26e1e63e0deb511059d28ba51014b58242ddR146 but the change does not seem related to the commit's intention.

[ashleyww93]

Could this be causing other unintended consequences?

I’m currently trying to track down an issue(so I can make a reproduction) where entities are deleted and recreated with the same ID.

I thought it was an issue our side (and might be), but it’s seemingly random, like the identity map is incorrect on some occasions.

[hesalx]

@ashleyww93 You can override .getRepository() with a custom EM to be able to get visibility into any occurence of this specific issue. Feel free to just log instead of a hard error.

// replace `PostgreSqlDriver` with you respective driver
export class ExtendedEntityManager extends EntityManager<PostgreSqlDriver> {
  public getRepository<Entity extends object>(entityName: EntityName<Entity>) {
    const repository = super.getRepository(entityName);
    const currentContext = this.id;
    const repoContext = repository.getEntityManager().id;
    if (currentContext !== repoContext) {
      throw new Error(
        `EntityRepository<${repository.getEntityName()}> context (${repoContext}) did not match the current EntityManager context (${currentContext})!`,
      );
    }
    return repository as any;
  }
}

defineConfig({
  // ...
  entityManager: ExtendedEntityManager,
});

Docs: https://mikro-orm.io/docs/entity-manager#extending-entitymanager

[hesalx]

@ashleyww93 If this issue is the culprit of your problem, it may appear random because it requires the first .getRepository() call in the app process to be inside a transaction. After a server restart it may be completely up to chance whether it happens this way.

[B4nan]

It is worth noting that the aforementioned af87693 explicitly states "do not use context in em.getRepository()", however it is still used there (v6.1.12) and was added by b5b03a6#diff-a62cd78c400d99253a3c0fb5a9bf26e1e63e0deb511059d28ba51014b58242ddR146 but the change does not seem related to the commit's intention.

Yeah correct, and reverting this bit is not making any of the existing tests fail, I guess I will go with that, worst case we will revert it, but I agree it looks unrelated to the commit. That change also makes the e2e test suite of the real-world app pass, so it feels safe.

[hesalx]

@B4nan Awesome! Thanks.

Any chance the test suite can be amended with a case like this?

await em.transactional(async _ => {
    // first even call to a repository, em is not tx-specific
    await em.getRepository(Post).find({id: 1});
});
assert.equal(em.id, em.getRepository(Post).getEntityManager().id)

[B4nan]

Yeah, for sure, I will include the repro from OP.