Problem
We have a failing e2e test "POST /orders is protected from oversell" which is pretty basic and most likely not enough to cover a somewhat realistic scenario.
Also, there is a draft of extended test, but since this basic test is failing it should be fixed first.
The basic test is flaky, locally I have not encountered any failures, but on CI I got one right away:
https://github.com/ForeverProglamer/e-commerce-platform/actions/runs/24860976515/job/72786083638
Proposal
- Enable the basic test, fix it, and ensure it passes consistently.
- Extend the basic test to ensure that:
- oversell protection is working for scenarios with greater concurrency
- all user requests get responses with either 201 Created or 409 Conflict (not enough items in stock)
- if possible, there are no deadlocks, otherwise they are handled with retrying operation/transaction
Alternatively, it might be more helpful to use extended test from the start, since the problem we got on CI and problems produced by extended test seem to be of a similar nature.
Extended test draft
Initially, the test was meant to spot row ordering problem when Postgres acquires locks for rows of the products table (tx1 locks products [A, B], tx2 locks [B, A] => mutual exclusion, deadlock).
But, when requests concurrency was increased from 20 to 150, we started facing problems even earlier (concurrent insertion into orders table).
it("POST /orders is protected from oversell", async () => {
const stock = 20;
const products = await Promise.all([
createProduct({ title: "Hot Product 1", stock }),
createProduct({ title: "Hot Product 2", stock }),
]);
const items = products.map((p) => ({ id: p.id, qty: 1 }));
const dto1: CreateOrderDto = { items };
const dto2: CreateOrderDto = { items: items.reverse() };
const randomDto = () => (Math.random() > 0.5 ? dto1 : dto2);
const productsResponse1 = await Promise.all(
products.map((p) => request(server).get(`/products/${p.id}`)),
);
productsResponse1.forEach((resp, i) => {
expect(resp.status).toBe(200);
expect(resp.body.stock).toBe(products[i].stock);
});
const submitOrder = async (dto: CreateOrderDto) => {
return await request(server)
.post("/orders")
.send(dto)
.set("Idempotency-Key", randomUUID())
.auth(accessToken, { type: "bearer" });
};
const requestsCount = 150;
const responses = await Promise.all(
Array.from({ length: requestsCount }, () => submitOrder(randomDto())),
);
const successful = responses.filter((r) => r.status === 201);
const conflict = responses.filter((r) => r.status === 409);
expect(successful.length).toBe(stock);
expect(conflict.length).toBe(requestsCount - stock);
const productsResponse2 = await Promise.all(
products.map((p) => request(server).get(`/products/${p.id}`)),
);
productsResponse2.forEach((resp) => {
expect(resp.status).toBe(200);
expect(resp.body.stock).toBe(0);
});
}
Related thoughts
Currently, our OrdersService.createOrder flow is the following (and wrapped into a transaction):
- Create order with deduplication
- If order already exists => it is a duplicate request => return existing order to user
- Select requested products from DB by their IDs and acquire locks for each row
- Verify all products exist, otherwise fail
- Verify products have enough of stock to fulfill the reuqest, and decrease stock, otherwise fail
- Prepare orderItems, and save them (order.id is required for DB write)
- Save updated products with decreased stock
What if we move order creation after all other verifications are done? This way, we defer insertion operation until its totally necessary.
Perhaps that might impact our deadlock problem with early order insertion attempt.
Problem
We have a failing e2e test
"POST /orders is protected from oversell"which is pretty basic and most likely not enough to cover a somewhat realistic scenario.Also, there is a draft of extended test, but since this basic test is failing it should be fixed first.
The basic test is flaky, locally I have not encountered any failures, but on CI I got one right away:
https://github.com/ForeverProglamer/e-commerce-platform/actions/runs/24860976515/job/72786083638
Proposal
Alternatively, it might be more helpful to use extended test from the start, since the problem we got on CI and problems produced by extended test seem to be of a similar nature.
Extended test draft
Initially, the test was meant to spot row ordering problem when Postgres acquires locks for rows of the
productstable (tx1 locks products [A, B], tx2 locks [B, A] => mutual exclusion, deadlock).But, when requests concurrency was increased from 20 to 150, we started facing problems even earlier (concurrent insertion into
orderstable).Related thoughts
Currently, our
OrdersService.createOrderflow is the following (and wrapped into a transaction):What if we move order creation after all other verifications are done? This way, we defer insertion operation until its totally necessary.
Perhaps that might impact our deadlock problem with early order insertion attempt.