typeorm / typeorm

ORM for TypeScript and JavaScript. Supports MySQL, PostgreSQL, MariaDB, SQLite, MS SQL Server, Oracle, SAP Hana, WebSQL databases. Works in NodeJS, Browser, Ionic, Cordova and Electron platforms.
http://typeorm.io
MIT License
34.83k stars 6.38k forks source link

Regression in transactionDepth handling? #11260

Open aphofstede opened 1 week ago

aphofstede commented 1 week ago

Issue description

transactionDepth went negative

Expected Behavior

transactionDepth is never decremented more often than it was incremented

Actual Behavior

Negative transaction depth

Steps to reproduce

Nest a couple of transactions and cause a DB error (e.g. insert a bad value into a column of the wrong type)

My Environment

Dependency Version
Operating System
Node.js version 22.6.0
Typescript version 5.4.5
TypeORM version 0.3.20

Additional Context

Seen in the wild, in a situation where an unhandled database error occurred deep down in a transaction stack, the transactionDepth appears to be decremented more often than it was incremented. When a new save point is attempted it will cause a syntax error due to the minus sign on the negative number.

Might be related to the changes from https://github.com/typeorm/typeorm/pull/10210

      "err": {
        "driverError": {
          "length": 90,
          "position": "19",
          "stack": "error: syntax error at or near \"-\"\n    at /app/node_modules/pg/lib/client.js:526:17\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n    at async PostgresQueryRunner.query (/app/node_modules/typeorm/driver/postgres/PostgresQueryRunner.js:184:25)\n    at async PostgresQueryRunner.startTransaction (/app/node_modules/typeorm/driver/postgres/PostgresQueryRunner.js:130:13)\n    at async EntityPersistExecutor.execute (/app/node_modules/typeorm/persistence/EntityPersistExecutor.js:134:25)",
          "name": "error",
          "routine": "scanner_yyerror",
          "code": "42601",
          "type": "DatabaseError",
          "line": "1180",
          "severity": "ERROR",
          "message": "syntax error at or near \"-\"",
          "file": "scan.l"
        },
        "file": "scan.l",
        "stack": "QueryFailedError: syntax error at or near \"-\"\n    at PostgresQueryRunner.query (/app/node_modules/typeorm/driver/postgres/PostgresQueryRunner.js:219:19)\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n    at async PostgresQueryRunner.startTransaction (/app/node_modules/typeorm/driver/postgres/PostgresQueryRunner.js:130:13)\n    at async EntityPersistExecutor.execute (/app/node_modules/typeorm/persistence/EntityPersistExecutor.js:134:25)",
        "severity": "ERROR",
        "position": "19",
        "routine": "scanner_yyerror",
        "type": "QueryFailedError",
        "length": 90,
        "code": "42601",
        "message": "syntax error at or near \"-\"",
        "line": "1180",
        "query": "SAVEPOINT typeorm_-7"
      }

Relevant Database Driver(s)

Are you willing to resolve this issue by submitting a Pull Request?

Yes, I have the time, but I don't know how to start. I would need guidance.

alumni commented 1 week ago

I am also investigating the flaky SQLite tests with draft PR #11261, after fixing one issue, it seems second issue is very similar to yours:

  1) github issues > #10209
       should not fail to run multiple nested transactions in parallel:
     QueryFailedError: SQLITE_ERROR: no such savepoint: typeorm_2
      at Statement.handler (src/driver/sqlite/SqliteQueryRunner.ts:17:65)
      at Statement.replacement (node_modules/sqlite3/lib/trace.js:25:27)
      at Statement.replacement (node_modules/sqlite3/lib/trace.js:25:27)

The annoying thing is that I can't replicate it happens randomly at the moment.

alumni commented 1 week ago

The flaky test issue is not the same issue as this, but it was introduced by the same PR.

This issue happens because we decrement before executing the commit/release/rollback query, so if the statement fails, we decrement twice, e.g.:

await queryRunner.startTransaction();
try {
  await queryRunner.query(`INSERT INTO "people"("id", "company_id") VALUES (1, 5)`);
  await queryRunner.commit();
} catch {
  await queryRunner.rollback();
} finally {
  await queryRunner.release();
}

If the people table has a deferrable foreign key constraint to the companies table, then the INSERT statement will not fail, but the COMMIT will because the constraint will be enforced at commit. Since we decrement the counter before we commit, the transaction level is now 0, but because it fails, we try to roll back and the transaction level is decremented again before executing the rollback statement, so it's now -1. This could also happen in a nested transaction (if releasing the save point will fail), which should be easier to replicate.

So I think it's okay the way transactions were ended before the PR was merged.

Looking at how transactions are created, I think it's less relevant since I don't know any situation in which starting a transaction could fail (maybe reaching some db limit?), but if starting a transaction can error, then I think it was okay before the PR as well.

Now, the test: it's been constantly failing for the SQLite driver. It was supposed to test parallel nested transactions that don't make any sense because in a transaction everything happens in sequence.

So I think we should revert PR #10210 and write some tests for this issue, as well as keep some of the changes in #11261.

alumni commented 11 hours ago

I added the test I mentioned in the comment above in PR #11264. I can confirm it fails on the master branch, the transaction depth goes below 0.