Skip to content

fix(lock): compare the tail check against the stored chained promise - #6

Open
latent-9 wants to merge 1 commit into
Twigpine:mainfrom
latent-9:fix/lock-table-leak
Open

latent-9 wants to merge 1 commit into
Twigpine:mainfrom
latent-9:fix/lock-table-leak

Conversation

@latent-9

Copy link
Copy Markdown

withLock stores prev.then(() => next) in the locks table, but the cleanup compares locks.get(key) against next. The map never holds next (it holds the promise returned by .then), so the tail check is always false and the entry is never deleted. The comment says avoid unbounded growth, but the cleanup is dead: locks grows by one entry per distinct key for the process lifetime. The write path takes a keyed lock per namespace and per owner, and the section can throw before the map is trimmed, so entries leak even for rejected requests. Reachable from PUT/DELETE /api/memory/:ns via upsert. Fix: compare against the chained promise that is actually stored. Adds tests for serialization, drain-to-empty across many keys and under contention, and cleanup on throw; the drain and throw cases fail on the old code and pass with the fix. Full suite green, 61 tests.

@kevincodex1 kevincodex1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks good to me

@kevincodex1

Copy link
Copy Markdown
Member

hello @latent-9 thank you for your contribution some test is failing please fix the formatting.

@latent-9

latent-9 commented Aug 15, 2026 •

Copy link
Copy Markdown
Author

@kevincodex1 Thanks for taking a look. That failure was Biome's import sorting on the new test file: the named imports in tests/lock.test.ts were not alphabetical, so biome ci bailed before the tests ran. I sorted them and re-ran the checks locally, biome ci is clean, type-check passes, and all 61 tests pass. Pushed the fix.

withLock stores prev.then(() => next) in the lock table but the cleanup
compared locks.get(key) against next. The map never holds next (it holds the
promise returned by .then), so the tail check was always false and the entry
was never deleted. The table grew by one entry per distinct key for the
process lifetime, including keys whose critical section threw (the finally
still ran the dead check), so the write path leaked entries even for rejected
requests.

Compare against the chained promise that is actually stored. Adds a test
covering serialization, drain-to-empty across many keys and under contention,
and cleanup on throw.
@latent-9
latent-9 force-pushed the fix/lock-table-leak branch from 133b30a to 369079b Compare August 15, 2026 06:18
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