Skip to content

fix: release the id increment lock after generation - #87

Merged
joamag merged 1 commit into
masterfrom
bug/id-lock-release
Jul 15, 2026
Merged

joamag merged 1 commit into
masterfrom
bug/id-lock-release

Conversation

@joamag

@joamag joamag commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

DataAdapter._id acquired _inc_lock (an RLock) and released it only on the exception path, leaking the acquisition on every successful call. The owning thread (whichever performs the first insert, typically the main thread at boot) keeps re-entering its own RLock, so single-threaded usage never notices - but any other thread blocks forever on its first insert through an adapter that generates identifiers via _id (e.g. TinyAdapter). In practice, appier Scheduler-driven jobs writing over the tiny adapter deadlock silently: the tick never completes and nothing is logged.

The lock is now released in a finally clause. The new test_id_lock_release regression test asserts the lock is acquirable from another thread after _id runs - it fails on the previous code and passes now. The Mongo path was never affected (identifier generation is delegated to pymongo).

Found while wiring fidelia's scheduler bots (hivesolutions/fidelia#5), whose thread hung on the first Voucher.save().

Closes #86

- the success path of DataAdapter._id leaked the RLock acquisition, deadlocking inserts from background threads (scheduler jobs) on non Mongo adapters
- regression test asserts the lock is acquirable from another thread
@coderabbitai

coderabbitai Bot commented Jul 15, 2026 •

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3f193584-b9e2-4eb1-ad1d-183e5f4ee18b

📥 Commits

Reviewing files that changed from the base of the PR and between cdcfcad and 33dd5db.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • src/appier/data.py
  • src/appier/test/data.py

📝 Walkthrough

Walkthrough

Changes

Identifier lock fix

Layer / File(s) Summary
Reliable increment lock release
src/appier/data.py
DataAdapter._id() releases _inc_lock in a finally block during increment-token construction.
Background-thread regression coverage
src/appier/test/data.py, CHANGELOG.md
A threaded test verifies the lock can be reacquired after _id(), and the unreleased changelog entry documents the fix.

Poem

I’m a rabbit who hops through the code,
Where locks once lingered and slowed the road.
_id() now frees them, neat and bright,
Threads can acquire them without a fight.
Schedulers twitch their noses—“All right!”

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@joamag
joamag marked this pull request as ready for review July 15, 2026 07:57
Copilot AI review requested due to automatic review settings July 15, 2026 07:57
@joamag joamag self-assigned this Jul 15, 2026
@cursor

cursor Bot commented Jul 15, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@joamag joamag added bug Something isn't working p-medium Medium priority issue labels Jul 15, 2026
@joamag
joamag merged commit abca7f3 into master Jul 15, 2026
73 of 74 checks passed
@joamag
joamag deleted the bug/id-lock-release branch July 15, 2026 07:58

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.

Pull request overview

Fixes a concurrency deadlock in DataAdapter._id where the increment lock (_inc_lock, an RLock) was not released on the success path, causing background threads (e.g., scheduler jobs using TinyAdapter) to block indefinitely when generating identifiers.

Changes:

  • Release _inc_lock in a finally block to guarantee unlocking after identifier increment in DataAdapter._id.
  • Add a regression test to ensure _inc_lock can be acquired from another thread after _id completes.
  • Document the fix in CHANGELOG.md under Unreleased > Fixed.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/appier/data.py Ensures _inc_lock is always released after _id updates the increment value.
src/appier/test/data.py Adds a multithreaded regression test validating the lock is not leaked after _id.
CHANGELOG.md Records the deadlock fix in the Unreleased “Fixed” section.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

Labels

bug Something isn't working p-medium Medium priority issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix identifier generation deadlock for background threads

2 participants