Hi team,
I opened two backend fixes last week and would appreciate a review when someone has time.
main ← OmBhandwaldar:fix/folder-commit-update-tx
opened 07:32PM - 15 Oct 26 UTC
## Context
Fixes #8507.
Updating secrets concurrently through the API fails fo… r almost all of them: the requests hang for about 60 seconds and return 500 "Something went wrong", with `KnexTimeoutError: Timeout acquiring a connection. The pool is probably full.` underneath. Same kind of problem as #8401, but on the update path and in any folder.
On update, `createCommit` runs inside the update's transaction and passes `tx` to `filterIgnoredChanges`, which skips updates that didn't change anything. That calls `compareSecretVersions`, which fetched the project's data key without the transaction:
```ts
// before
const { decryptor: secretManagerDecryptor } = await kmsService.createCipherPairWithDataKey({
type: KmsDataKey.SecretManager,
projectId
});
// after
const { decryptor: secretManagerDecryptor } = await kmsService.createCipherPairWithDataKey(
{
type: KmsDataKey.SecretManager,
projectId
},
tx
);
```
Without `tx`, `decryptWithKmsKey` reads the KMS key row through `kmsDAL.findByIdWithAssociatedKms` on `db.replicaNode()`, which is the primary pool when no read replica is configured, and that read isn't cached. Each update holds its transaction's connection while it waits for a second one, so once about as many updates run at once as the pool has connections (`DB_POOL_MAX`, 10 by default), nothing progresses until knex's 60s acquire timeout fails them.
This passes `tx` through `compareSecretVersions` to `createCipherPairWithDataKey`. `filterIgnoredChanges` is its only caller.
`compareFolderStates` has the same pattern on the rollback/revert path, as mentioned in the issue. I left it out since I haven't seen it cause failures; happy to add it here or in a follow-up.
## Steps to verify the change
New e2e spec `backend/e2e-test/routes/v4/secrets-concurrent-update.spec.ts` creates 30 secrets in a subfolder, updates all of them concurrently, and checks that every new value is stored.
```sh
make test-api-e2e SPEC=e2e-test/routes/v4/secrets-concurrent-update.spec.ts
```
- On `main` (`9d67438`): fails, the updates return 500 after ~60s.
- With this change: passes in ~0.5s.
Also ran locally on the same base:
- Related e2e specs (secrets v3/v4, folders, imports, replication, secret sync, approvals, rotations, folder access, insights): 28 files, 353 tests passing, including the new spec. The only failures are 5 tests in `v3/secret-rotations.spec.ts`, which fail the same way on `main` since they need the external databases from `docker-compose.e2e-dbs.yml`.
- `folder-commit` unit tests: passing. `kms-fns.test.ts` fails to load in the FIPS test image (`digital envelope routines::unsupported`) on `main` as well.
- ESLint on the changed files and `npm run type:check`: clean.
## Type
- [x] Fix
- [ ] Feature
- [ ] Improvement
- [ ] Breaking
- [ ] Docs
- [ ] Chore
## Checklist
- [x] Title follows the [conventional commit](https://www.conventionalcommits.org/en/v1.0.0/#summary) format: `type(scope): short description` (scope is optional, e.g., `fix: prevent crash on sync` or `fix(api): handle null response`).
- [x] Tested locally
- [ ] Updated docs (if needed)
- [ ] Updated CLAUDE.md files (if needed)
- [x] Read the [contributing guide](https://infisical.com/docs/contributing/getting-started/overview)
Fixes #8507 . On instances without a read replica and with the default pool size, around 10 or more concurrent secret updates use up the DB connection pool, hang for about 60 seconds and then fail with 500. Same kind of problem as #8401 (fixed in #8496 ), but on the update path: the data key lookup during the commit doesn’t use the transaction, so each update needs a second connection.
main ← OmBhandwaldar:fix/identity-auth-update-ttl-compare
opened 06:06PM - 15 Oct 26 UTC
## Context
Fixes #8537.
Updating the AliCloud, LDAP, OCI, TLS certificate or S… PIFFE auth method of a machine identity through the API fails with 400 "Access token TTL cannot be greater than max TTL" when the request leaves out `accessTokenTTL` and `accessTokenMaxTTL`, even though the stored values are valid.
For omitted fields, the update flows fall back to the stored values. `accessTokenTTL` and `accessTokenMaxTTL` are `bigint` columns and `findOne` returns the raw row, so those values are strings. When the request has neither field, both sides of the check are strings and get compared as text: with a stored TTL of 3600 and max TTL of 2592000, `"3600" > "2592000"` is `true` and the update is rejected.
This converts both sides with `Number()` in all five update flows:
```ts
// before
(accessTokenMaxTTL || identityOciAuth.accessTokenMaxTTL) > 0 &&
(accessTokenTTL || identityOciAuth.accessTokenTTL) > (accessTokenMaxTTL || identityOciAuth.accessTokenMaxTTL)
// after
Number(accessTokenMaxTTL || identityOciAuth.accessTokenMaxTTL) > 0 &&
Number(accessTokenTTL || identityOciAuth.accessTokenTTL) >
Number(accessTokenMaxTTL || identityOciAuth.accessTokenMaxTTL)
```
An update that sets the max TTL below the stored TTL is still rejected.
## Steps to verify the change
New e2e spec `backend/e2e-test/routes/v1/identity-auth-update-ttl.spec.ts` attaches AliCloud, OCI, TLS certificate and SPIFFE auth with a 1h TTL and 30d max TTL, then checks that:
- an update without TTL fields returns 200
- an update with a max TTL below the stored TTL returns 400
```sh
make test-api-e2e SPEC=e2e-test/routes/v1/identity-auth-update-ttl.spec.ts
```
- On `main` (`9d7aa45`): fails, the update without TTL fields returns 400.
- With this change: 8/8 pass.
LDAP isn't in the spec because attaching it needs a reachable LDAP server. I checked it separately by inserting the auth row directly: without the change the update is rejected by this check, and with it the request gets past the check.
Also ran locally on the same base:
- Identity-related e2e specs (identity, auth boundary, access tokens, TLS cert auth, folder access, lockout, memberships): 14 files, 135 tests passing, including the new spec.
- Unit tests for the five services: all pass except 5 Ed25519 tests in `identity-tls-cert-auth-fns.test.ts`, which fail the same way on `main` in the FIPS test image and aren't touched here.
- ESLint on the changed files and `npm run type:check`: clean.
## Type
- [x] Fix
- [ ] Feature
- [ ] Improvement
- [ ] Breaking
- [ ] Docs
- [ ] Chore
## Checklist
- [x] Title follows the [conventional commit](https://www.conventionalcommits.org/en/v1.0.0/#summary) format: `type(scope): short description` (scope is optional, e.g., `fix: prevent crash on sync` or `fix(api): handle null response`).
- [x] Tested locally
- [ ] Updated docs (if needed)
- [ ] Updated CLAUDE.md files (if needed)
- [x] Read the [contributing guide](https://infisical.com/docs/contributing/getting-started/overview)
Fixes #8537 . Updating AliCloud, LDAP, OCI, TLS certificate or SPIFFE auth through the API without the TTL fields can fail with “Access token TTL cannot be greater than max TTL”, for example with a 1h TTL and 30d max TTL. The stored TTLs come back from the database as strings, so they get compared as text (“3600” > “2592000”).
Both have e2e tests that fail on main and pass with the fix, and both merge cleanly with main.
Thanks!
Hey OmBhandwaldar!
Appreciate you reaching out! I will flag this to the team and get back to you. Thanks!
Jake
1 Like