Skip to content

Serialize concurrent item replaces per user - #63

Open
tobiashochguertel wants to merge 1 commit into
taskbook-sh:masterfrom
tobiashochguertel:fix/serialize-replace-items
Open

tobiashochguertel wants to merge 1 commit into
taskbook-sh:masterfrom
tobiashochguertel:fix/serialize-replace-items

Conversation

@tobiashochguertel

Copy link
Copy Markdown

Summary

PUT /api/v1/items and PUT /api/v1/items/archive call replace_items. It runs DELETE FROM items WHERE user_id = $1 AND archived = $2 and then one INSERT per item, all in one transaction.

When two of these requests overlap for the same user (two synced clients, or one client saving twice in quick succession), PostgreSQL's default READ COMMITTED isolation lets both transactions run the DELETE. The second DELETE waits for the first transaction, then skips the rows it already deleted. It does not see the rows that transaction just inserted. Its INSERTs then conflict with UNIQUE(user_id, item_key, archived). The request fails with duplicate key value violates unique constraint "items_user_id_item_key_archived_key", which the server returns as a 500, and that client's sync write is lost.

Fix

Inside the transaction, replace_items first takes a per-user transaction-level advisory lock:

SELECT pg_advisory_xact_lock(hashtextextended($1::text, 0))

This serializes replaces for the same user. The lock is released automatically on commit or rollback. Different users still run in parallel. The delete-then-insert logic and the API stay the same. There is no schema change.

Tests

  • Added replace_items_concurrent_same_user in handlers/items.rs. It runs 8 concurrent replace_items calls with the same 50 keys for one user, repeated 5 times, and checks that all of them succeed and 50 rows remain. It needs a real PostgreSQL, so it is #[ignore]d by default and reads TB_TEST_DATABASE_URL. That keeps cargo test --workspace in CI unchanged:

    TB_TEST_DATABASE_URL=postgres://postgres@localhost:5432/postgres cargo test -p taskbook-server -- --ignored
    
  • Against PostgreSQL 18:

    • Without the fix, the test fails with duplicate key value violates unique constraint "items_user_id_item_key_archived_key".
    • With the fix, it passes on repeated runs.

    A standalone script running the same delete-then-insert transactions shows the same thing: 35 of 40 concurrent replaces failed without the lock, and none failed with it.

  • rustfmt --check passes on the changed file. The only Rust toolchain available to me was 1.83, which cannot build the workspace: rand 0.10 needs edition 2024. So I compiled replace_items and the new test in a small standalone crate with the same sqlx 0.8, ran clippy -D warnings there, and leave the full-workspace cargo clippy / cargo test to CI.

This change was prepared with AI assistance (Devin).

Two overlapping PUT /items (or /items/archive) requests for the same user could both run the DELETE and then insert the same keys. Under READ COMMITTED the second transaction does not see the rows the first one inserted, so it failed with a unique-constraint violation and returned a 500.

replace_items now takes a per-user transaction-level advisory lock before the DELETE, so replaces for the same user run one after another.

Adds an ignored regression test that needs PostgreSQL (TB_TEST_DATABASE_URL).

Co-Authored-By: Tobias Hochgürtel <[email protected]>
Copilot AI balanced review requested due to automatic review settings October 13, 2026 05:55

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The lock correctly scopes serialization per user and the regression test exercises the reported race.

0 open findings

What changed in this PR

Adds per-user PostgreSQL advisory locking to prevent concurrent item replacements from causing duplicate-key failures.

Changes:

  • Serializes active and archived item replacements per user.
  • Adds an ignored PostgreSQL concurrency regression test.
File Description
crates/​taskbook-server/​src/​handlers/​items.rs Adds transactional advisory locking and concurrency coverage.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@tobiashochguertel

Copy link
Copy Markdown
Author

Friendly ping — this serializes concurrent replace_items calls per user via an advisory lock (with a regression test). The branch is still current with master; the CI workflow is waiting on maintainer approval ("Approve and run") as this is a first-time contribution. Happy to split or rework the change if that helps review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants