Skip to content

Fix concurrent local KV increments and maximum updates 馃馃馃 - #28194

Open
jashkarangiya wants to merge 2 commits into
directus:mainfrom
jashkarangiya:fix/kv-local-concurrent-updates
Open

jashkarangiya wants to merge 2 commits into
directus:mainfrom
jashkarangiya:fix/kv-local-concurrent-updates

Conversation

@jashkarangiya

@jashkarangiya jashkarangiya commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

What's Changed

  • Fixed the shared local KV read-modify-write race in increment() and setMax() by reading synchronously before starting the existing write.
  • Updated the Kv interface to allow synchronous or asynchronous implementations through MaybePromise<T> return types.
  • Made all KvLocal methods synchronous while preserving serialization, numeric validation, equal-value rejection, and TTL behavior. The Redis implementation remains asynchronous; no new dependencies were added.
  • Added 23 real-store regression/compatibility tests across Map, size-limited LRU, and TTL-only LRU, and a patch changeset for @directus/memory.

Tested Scenarios

  • Concurrent increments retain all updates and return distinct counts, including positive, negative, and zero amounts.
  • Concurrent smaller/equal maxima cannot overwrite a larger value; mixed setMax() / increment() calls see the latest value.
  • Sequential behavior, non-number rejection, TTL renewal and expiration, and no TTL renewal for rejected maxima remain intact.
  • Before the fix, 12 of the new tests failed and 11 compatibility checks passed. After the fix, all 23 passed.
  • Full memory package suite: 24 files, 171 tests passed.
  • Memory package build and built createKv API reproduction passed.
  • Repository-wide pnpm lint, pnpm lint:style, pnpm format, and git diff --check passed. ESLint reported 81 existing warnings outside the changed files, zero errors.

Review Notes / Questions / Concerns

  • Following maintainer review, the local implementation is synchronous end-to-end. Its read-modify-write operations therefore complete without yielding, while the shared interface continues to support the asynchronous Redis backend.
  • This is separate from Fix the local key-value store treating equal setMax values as updates聽#28172's equality-boundary fix. Both operations previously read stale values when their calls overlapped.
  • Schema/extension election impacts in Local KV operations lose concurrent increments and can lower the stored maximum 馃馃馃聽#28193 are inferred from their increment-based coordination; this PR verifies the underlying package behavior, not a full-server reproduction.
  • Supplemental tsc --noEmit -p packages/memory/tsconfig.json reports four existing errors: three overload errors in src/bus/lib/create.test.ts and one Buffer type error in src/bus/lib/redis.test.ts. Comparing compiler diagnostics with the unchanged base sources produced the identical four errors and no new diagnostics. These unrelated files are unchanged.

Checklist

Leave unchecked where not applicable

  • Tests added/updated
  • Documentation PR created in directus/docs
  • OpenAPI updated
  • SDK (@directus/sdk) updated to reflect the changes
  • Types (@directus/types) updated to reflect the changes
  • GraphQL schema updated to reflect the changes
  • System data (@directus/system-data) updated for changes to system collections/fields/relations
  • Database migration added for schema/system changes
  • Environment variables documented for new/changed config
  • App translations added for new user-facing strings
  • Security implications apply

Fixes #28193

@Nitwel Nitwel 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.

LGTM!

@Nitwel Nitwel 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.

After discussing it with @ComfortablyCoding and @br41nslug, we should update the Kv interface to support MaybePromises via type MaybePromise<T> = Promise<T> | T. And then make all the async functions sync in the local implementation.

@jashkarangiya

jashkarangiya commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @Nitwel! Updated this in c4d8077. The Kv interface now uses MaybePromise<T> for its return types, and I removed async from the local implementation. I also updated the tests to account for errors being thrown synchronously.

The full memory suite (171 tests), package build, lint, Stylelint, and formatting all pass. Could you take another look when you have a chance?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Local KV operations lose concurrent increments and can lower the stored maximum 馃馃馃

3 participants