fix: locking query shape coverage & AsyncLocal timing bug#11
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ing queries Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…l timing bug) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lidOperationException with LockingConfigurationException Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…distributed lock key validation Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…d lock extensions; add Limitations section to README Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mnbuhl
force-pushed
the
fix/locking-query-shape-coverage
branch
from
fff6019 to
9c30a25
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fff6019076
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var lockTag = selectExpression.Tags.FirstOrDefault(t => | ||
| t.StartsWith(LockTagConstants.Prefix, StringComparison.Ordinal) | ||
| ); |
There was a problem hiding this comment.
Using FirstOrDefault here makes lock resolution depend on tag ordering when multiple locking tags exist on the same query (for example, chaining lock extensions or composing helpers that apply them twice). That can apply a stale mode/timeout instead of the latest lock request, whereas the previous LockContext.Current behavior followed the most recent call. This can emit the wrong lock clause in production queries (e.g., FOR UPDATE instead of an intended later FOR SHARE/timeout).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 39ef779. Changed to in all three provider SQL generators so the most-recently-applied locking tag wins — matching the previous last-write-wins behavior.
| parts[2].Length > 0 | ||
| && double.TryParse( | ||
| parts[2], | ||
| System.Globalization.NumberStyles.Any, | ||
| System.Globalization.CultureInfo.InvariantCulture, | ||
| out var ms | ||
| ) | ||
| ) | ||
| timeout = TimeSpan.FromMilliseconds(ms); |
There was a problem hiding this comment.
If the timeout segment is present but invalid, this path still returns success and leaves Timeout as null, silently changing behavior to "wait indefinitely" instead of preserving the requested timeout. In addition, double.TryParse accepts values like NaN/Infinity, which can make TimeSpan.FromMilliseconds throw even though TryParse is expected to be non-throwing. A malformed or corrupted __efcore_locking tag can therefore cause hangs or unexpected runtime exceptions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 39ef779. TryParse now returns false when the timeout segment is non-empty but unparseable (instead of silently defaulting to null). Also rejects NaN, Infinity, and negative values via !double.IsFinite(ms) || ms < 0 — all of which would cause TimeSpan.FromMilliseconds to throw or produce a nonsensical timeout. Four new unit tests cover these cases.
… and chained Where shapes Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ushes GroupBy into subqueries GroupBy with entity-typed results never appears on the outer SelectExpression that carries the lock tag, so the guard was dead code that produced false test failures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e, join shapes, compiled queries, and FromSql composition Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…query tests; fix SQL Server container on ARM64 - UnsafeShapeDetector: walk expression tree recursively to find aggregate SqlFunctionExpression wrapped in CAST or COALESCE (fixes CountAsync/SumAsync/etc.) - QueryShapeTestsBase: use List<int> instead of int[] for Contains (fixes net10 span constraint) - Remove compiled query tests: EF.CompileAsyncQuery cannot translate ForUpdate() extension - SqlServerFixture: switch to azure-sql-edge (ARM64 native) + custom TCP/login wait strategy Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…formed/NaN timeout in TryParse; update README with discovered limitations - SQL generators: use LastOrDefault instead of FirstOrDefault when scanning locking tags, so the most-recently-applied lock options win (matches LockContext.Current last-write-wins semantics) - LockTagConstants.TryParse: reject non-empty timeout segments that fail double.TryParse, and reject NaN/Infinity/negative values; previously these silently fell back to null (wait indefinitely) - README: correct EF.CompileAsyncQuery entry (throws at compile time, not just missing pre-statement SQL), add Concat/aggregate/GroupBy shape docs Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…reports for interceptor and SQL generation
1 participant
Summary
LockingValidationInterceptornow parseLockOptionsfrom the SQL tag instead of readingLockContext.Current. This eliminates silent missing-lock behaviour when a query is built in one async context and executed in another (deferred execution, stored queryables).UnsafeShapeDetectornow rejectsDISTINCTandGROUP BYqueries with a typedLockingConfigurationExceptioninstead of letting them reach the database and produce cryptic driver errors.InvalidOperationException(no transaction) andArgumentException(invalid distributed lock key) replaced withLockingConfigurationExceptionso callers can catchLockingExceptionas a unified base.ForUpdate/ForShareand all distributed lock extension methods now lists every thrown exception; README gains an "Unsupported query shapes" update and a new "Limitations" section forFromSqlRaw, compiled queries, and bulk operations.