Stabilize rapid presence snapshot ordering - #1366
Conversation
|
Exact-head local gate for
Two independent adversarial reviewers are starting with separate concurrency-correctness and false-green/test-contract lenses. Please review this exact head as well. @codex review |
There was a problem hiding this comment.
Code Review
This pull request enhances the SignalR presence concurrency stress tests by introducing a causal barrier (notBefore timestamp) to filter out stale snapshots from previous phases, and by sorting snapshots by their server occurrence time (OccurredAt) to handle out-of-order delivery. One important issue was identified in the error-handling path of WaitForPresenceStabilizationAsync, where calling MaxBy on an empty list of eligible snapshots will throw an unhandled InvalidOperationException instead of the intended TimeoutException.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Taskdeck/backend/tests/Taskdeck.Api.Tests/ConcurrencyRaceConditionStressTests.cs
Lines 1058 to 1062 in 5da5db8
When several JoinBoard calls return successfully but the tracker/barrier snapshot only contains one joined user, this relaxed lower-bound still passes. Because JoinBoard returns only after _presenceTracker.Join runs and the owner barrier publishes the current tracker state after Task.WhenAll(joinTasks), delayed or coalesced join broadcasts are no longer a reason to accept a partial member count; please require the barrier snapshot to include all successful joins so the stress test cannot false-green lost presence entries.
ℹ️ 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".
Chris0Jeky
left a comment
There was a problem hiding this comment.
Independent adversarial concurrency/causality review
Reviewed exact commit: 5da5db86ea2c9e4973cf7bc805bcd130576fd2c2
Verdict: changes required; do not merge this head.
I completed the source/causal analysis before the mandatory final discussion refresh. That refresh exposed two public false-green findings matching conclusions I independently reached below; I did not use another reviewer’s analysis to derive them.
MEDIUM
-
OccurredAtis not a total causal order, and the claimed growth negative control bypasses the selector. InConcurrencyRaceConditionStressTests.cs:884,MaxBy(snapshot => snapshot.OccurredAt)resolves only strictly greater timestamps. The tracker creates each timestamp withDateTimeOffset.UtcNow, which is neither unique nor monotonic, so equal values retain the first callback-delivered snapshot—the exact order this repair correctly treats as unreliable. The>= notBeforecutoff also admits a preceding-phase snapshot tied with the client cutoff. A barrier/current snapshot and a genuinely later growth snapshot can therefore share a timestamp and leave the earlier-delivered barrier selected, false-greening the invariant. The reordered-delivery test uses deliberately distinct +1/+2 ms values, whilePresenceStabilization_StillRejectsNewerGrowthAfterLeavesat line 942 calls only the assertion helper and never exercises filtering orMaxBy. Add a helper-level negative control with equal/cutoff timestamps and reordered delivery, then use a causal total-order signal (for example a tracker sequence/version generated under the existing lock, or an explicitly identifiable barrier plus defined tie handling) rather than wall-clock time alone. The .NET 8 implementation’s strict> 0update is visible here: https://github.com/dotnet/runtime/blob/v8.0.24/src/libraries/System.Linq/src/System/Linq/Max.cs#L768-L859. -
The post-join barrier is current state, but the assertion still permits successful joins to disappear. At lines 1058-1062, the test accepts any count from 2 through
actualJoined + 1. Every successfulJoinBoardinvocation has already executed the locked tracker mutation before returning, all connections remain alive, and the owner barrier runs afterTask.WhenAll; delivery coalescing no longer justifies a partial count. A lost tracker entry can therefore false-green. Require the barrier snapshot to contain the owner plus every successfully joined connection/user (or exactactualJoined + 1for these unique users). This is also present in the current public Codex review and remains unresolved. -
The post-leave assertion permits every successful leave to be a no-op.
AssertPresenceDidNotGrowAfterLeaves(lines 902-909), used at line 1108, accepts equality withsettledJoinCount. SinceactualLeft > 0and the owner barrier is invoked after all successfulLeaveBoardcalls, a tracker regression that removes nobody still passes. Require the successful leaving users to be absent or prove the corresponding count decrease. Existing thread: #1366 (comment).
Existing-comment disposition
- The Gemini empty-sequence finding at line 894 is not valid for this type.
BoardPresenceSnapshotis a reference type; .NET 8MaxBy<TSource,TKey>returnsdefaultfor an empty sequence whendefault(TSource) is null, so the intendedTimeoutExceptionstill executes. Framework source: https://github.com/dotnet/runtime/blob/v8.0.24/src/libraries/System.Linq/src/System/Linq/Max.cs#L689-L708. - Copilot produced no review because of quota; no action exists beyond recording that state.
- Both current substantive public findings are accounted for above; both live threads remain unresolved at this snapshot.
Evidence
- Guarded exact-head worktree and full one-file diff inspected; production hub/tracker semantics traced.
dotnet test ... --artifacts-path C:\Users\Public\codex-shell-home\taskdeck-pr1366-5da5-review --filter "FullyQualifiedName~PresenceStabilization_|FullyQualifiedName~Presence_RapidJoinLeave_EventuallyConsistent"— 3 passed, 0 failed, 0 skipped in 47 s.git diff --check d46adf48..5da5db86— passed.- Exact-head CI snapshot: 22 passing, 3 pending, 11 path-gated/skipped.
- Worktree remained clean; no edits or merge performed.
Chris0Jeky
left a comment
There was a problem hiding this comment.
Independent adversarial test-contract/operations review
Reviewed exact head: 5da5db86ea2c9e4973cf7bc805bcd130576fd2c2
Verdict: changes required; do not merge this head.
I fixed the source conclusions below before the mandatory final discussion refresh.
MEDIUM
-
Wall-clock
OccurredAtis not a total causal order, and the negative control bypasses that seam.WaitForPresenceStabilizationAsyncselectsMaxBy(snapshot => snapshot.OccurredAt)and admitsOccurredAt >= notBefore, butInMemoryBoardPresenceTrackercreates those values withDateTimeOffset.UtcNow. Equal timestamps therefore cannot distinguish causal order; tie selection can retain the first callback-delivered snapshot even though delivery order is the behavior this PR correctly treats as unreliable. The reordered-delivery regression assigns deliberately distinct+1 ms/+2 msvalues, whilePresenceStabilization_StillRejectsNewerGrowthAfterLeavescalls onlyAssertPresenceDidNotGrowAfterLeavesand never exercisesnotBeforeorMaxBy. A barrier snapshot and later growth snapshot with a tied timestamp can thus leave the barrier selected and false-green the composed path. This is not theoretical clock uniqueness: an independent Windows probe observed 999,912 equal adjacent values in 1,000,000UtcNowreads (minimum positive step 32 ticks). Add an integrated helper-level negative control covering equal/cutoff timestamps plus reordered delivery, and use a causal total-order signal or explicitly identifiable barrier rather than wall time alone. -
The current-state join barrier still permits successful joins to disappear. At lines 1058-1062 the test accepts any member count from 2 through
actualJoined + 1. Every successfulJoinBoardhas already executed the locked tracker mutation before returning, all joined connections remain alive, and the owner barrier runs afterTask.WhenAll; delayed/coalesced callbacks no longer justify accepting a partial current-state snapshot. A tracker regression that loses four of five successful joins can pass. Require the barrier snapshot to contain the owner plus every successfully joined unique user (an exactactualJoined + 1count here, preferably identity assertions). -
The current-state leave barrier permits every successful leave to be a no-op.
actualLeft > 0is required, butAssertPresenceDidNotGrowAfterLeavesaccepts equality withsettledJoinCount. Because each successfulLeaveBoardinvocation has completed before the barrier, a tracker regression that removes nobody still passes. Require the successful leaving users to be absent, or require the exact corresponding count decrease. This accounts for the live leave false-green thread.
Existing discussion disposition
- The public join-loss and leave-no-op findings are valid and covered above.
- The Gemini empty-sequence claim at line 894 is not valid for this generic instantiation:
BoardPresenceSnapshotis a reference type, so .NET 8MaxBy<TSource,TKey>returnsdefaultfor an empty sequence and the intendedTimeoutExceptionpath still executes. Add a timeout-path test if the diagnostic contract should be locked, but this is not the claimed runtime exception. - Copilot produced only a quota notice; there is no actionable content.
Evidence
- Guarded clean worktree; exact base/head and complete one-file diff inspected; production hub/tracker causality traced.
- Old-helper red proof is sound by construction: the synthetic test delivers the newer one-member snapshot first and the older two-member snapshot last, so the old
Last()selector fails deterministically. - Exact-head focused suite (ordering control, growth assertion, real rapid join/leave): 3 passed, 0 failed in 46.7 s using isolated output.
- Independent second real rapid join/leave run: 1 passed, 0 failed in 46.4 s.
git diff --check d46adf48..5da5db86passed; worktree remained clean.- Exact-head dual-OS API Integration checks are green. At the final snapshot, Backend Solution Regression and E2E Smoke were still pending; one older CI Extended OpenAPI job was cancelled by a superseding run.
No files were edited and no merge was performed.
Review repair evidence ? exact head
|
|
Exact-head local gate completed on
All initial-head findings are mapped to |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Exact-head CI blocker ? not caused by this diffPR #1366 exact
The load run otherwise completed 3,467 iterations / 17,419 checks with 0 failed HTTP checks; the job exits 99 on the stale write threshold and also lacks a writable summary artifact. This one-file PR touches only PR #1359 is currently open, mergeable/clean, exact Per repository law, do not merge #1366 while this check is red. Resume condition: maintainer merges #1359, then update/reconcile #1366 with |
Chris0Jeky
left a comment
There was a problem hiding this comment.
Fresh exact-head concurrency / causal-correctness re-review
Reviewed exact head 1f4bfd6d022d4f883299607ccebe8035b3dd73a9 independently from source and focused execution before the final public-discussion refresh.
Code verdict: no findings in the assigned concurrency / causal-correctness scope.
What I tried to refute:
- Unique editing-marker barrier. Each phase creates a fresh random marker on the owner after the phase's join/leave invocations have settled. The selector requires that exact owner/marker pair, so delayed earlier snapshots cannot satisfy the barrier. The tied-timestamp/reordered-delivery test exercises that same selector contract.
- Exact successful-join identity. Successful joins retain
(connection, userId)identity, and the barrier snapshot is checked against the exact owner-plus-successful-join ID set; count-only masking is gone. - Exact successful-leave subtraction. Only user IDs from completed successful leaves are subtracted, and the post-leave barrier snapshot is checked against the resulting exact set.
- Timeout proof and growth negative control. Missing-marker timeout behavior is deterministic and diagnostic. The integrated growth test routes an unexpected-member snapshot through the real barrier selector and proves exact membership rejects it even when a delayed stale snapshot arrives later.
I refreshed all public comments, reviews, and threads after deriving that conclusion. Both original review threads now have direct repair evidence and are resolved/outdated; I found no remaining unaddressed code-review comment.
Verification:
dotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1 --artifacts-path C:\Users\Public\codex-shell-home\taskdeck-pr1366-1f4-review --filter "FullyQualifiedName~PresenceBarrier_|FullyQualifiedName~Presence_RapidJoinLeave_EventuallyConsistent"— 4 passed, 0 failed, 0 skipped.git diff --check d46adf48d9a35a3e8d34f401c5f24eaf68ca1b26..1f4bfd6d022d4f883299607ccebe8035b3dd73a9— passed.- Review worktree remained clean; no edits or merge were performed.
CI blocker: exact-head CI is not green. Load and Concurrency Harness failed: its k6 functional checks completed successfully, but the board-write threshold was crossed (p95 reported as 2s) and k6 also hit permission denied writing frontend/taskdeck-web/test-results/load/k6-summary.json; two E2E jobs were still in progress at this refresh. This test-only PR does not change the exercised API runtime or k6 output path, but the failure is still a real red gate and must be investigated/recovered before merge. Therefore this is a no-code-findings review, not merge readiness.
Chris0Jeky
left a comment
There was a problem hiding this comment.
Fresh exact-head test-contract/operations re-review
Reviewed exact head: 1f4bfd6d022d4f883299607ccebe8035b3dd73a9
Verdict: one MEDIUM remains; do not merge this head.
MEDIUM
-
Caught hub failures are still treated as definite no-mutations, so exact membership can reintroduce a false-red race. In the join task (lines 1045-1055), a connection is added to
joinedConnectionsonly whenInvokeAsync(JoinBoard)returns successfully; the analogous leave path (lines 1091-1100) records a user only whenInvokeAsync(LeaveBoard)returns successfully. Both catchHubExceptionandHttpRequestExceptionand otherwise discard the outcome. But the production hub mutates_presenceTrackerbefore awaiting the final group broadcast (BoardsHub.cs:37-38for join and:53-54for leave), and a client transport failure can also lose the invocation result after the server mutation. Therefore a caught join can still appear in the marker snapshot, or a caught leave can already be absent, while the expected ID sets assume the opposite. Depending on disconnect cleanup timing, the same partial failure can pass or fail, which undermines the stabilization goal.Do not infer tracker outcome from a broad client exception. Either fail the stress scenario immediately on ambiguous invocation failures, reconcile each caught connection against an explicit authoritative mutation outcome before building the expected sets, or dispose/settle ambiguous connections through a causally observed cleanup path. Add deterministic post-mutation failure controls for both join and leave so the exact-membership bookkeeping cannot drift from server state.
Repaired seams verified
- A fresh owner editing marker is an effective causal identity; stale/tied/reordered snapshots cannot accidentally contain it.
- Exact user-ID equivalence closes the lost-successful-join and successful-no-op-leave false greens when invocation outcomes are unambiguous.
- The integrated growth control exercises marker selection plus the production assertion helper.
- The zero-match test locks the intended diagnostic
TimeoutExceptionpath. - The current PR description and exact-head evidence now describe the marker-based four-test design accurately.
Public discussion and CI state
- Every public conversation comment, bot review, and inline thread was refreshed. Both original inline threads have evidence-backed replies and are resolved; exact-head Codex automation reported no major issue.
- Exact-head CI finished with 33 successful, 5 path-gated/skipped, 1 failed. The sole failure is the pre-existing k6 summary-permission / 2.00 s board-write gate defect owned by #1358 and repaired in open T4 PR #1359; this one-file test diff does not cause it. Repository policy still requires #1366 to remain unmerged while that exact-head check is red.
Independent evidence
- Isolated-output exact-head barrier/negative/timeout + real rapid stress slice: 4 passed, 0 failed in 42.8 s.
- Exact-head dual-OS API Integration and serialized Backend Solution Regression are green; the posted local full-backend proof is 7,225 passed / 0 failed / 1 known skip.
git diff --check d46adf48..1f4bfd6dpassed; worktree remained clean.
No files were edited and no merge was performed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 067eac3d07
ℹ️ 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".
Address issue #1371 findings on this PR's head (test-only, one file): - Coverage narrowing: the marker barrier snapshot is republished by a separate SetEditingCard broadcast, so the stress test previously passed even if every JoinBoard/LeaveBoard broadcast was dropped. Add SelectPhaseBroadcasts + AssertJoinBroadcastsObserved/AssertLeaveBroadcastsObserved and wire them into Presence_RapidJoinLeave so each join is proven delivered as its own non-marker delta and each leave as a shrink delta, keeping the marker barrier for the exact final-state assertion. Six helper-level controls fail closed when broadcasts are suppressed or an identity is missing. - Fail-closed vs tolerance: keep hard-require; document that the test host now mirrors production SQLite busy_timeout/WAL via UseTaskdeckSqlite (#1373). - Near-tautological control: strengthen PresenceMutation post-mutation test to drive the real InMemoryBoardPresenceTracker to committed post-mutation state and prove the fail-closed helper propagates the ambiguity instead of recording a diverging success.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7b5d7538a
ℹ️ 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".
Self-review found the leave delivery check only asserted absence, which a co-leaver's full-state snapshot satisfies even when a given leaver's own LeaveBoard broadcast was dropped (false-green under one lock order). Add a per-phase count floor to AssertJoinBroadcastsObserved/AssertLeaveBroadcastsObserved so each successful join/leave must produce its own observed broadcast (the marker fence keeps stragglers out of the phase window). Add two controls that fail closed on partial suppression, and rename the post-mutation control so it no longer overclaims divergence on the leave branch.
Fresh Codex review flagged two ordering gaps: the observer's later SetEditingCard marker is not a delivery fence for the OTHER connections' join/leave broadcasts, so (a) a delayed join delta could arrive just after the barrier resolved and false-red the delivery assertion, and (b) a straggler join delta could leak past the leave-phase clear and masquerade as a leave delta, false-greening a dropped-leave regression. Add DrainPhaseBroadcastsAsync: since each successful invocation produces exactly one broadcast, wait until all N expected phase deltas have been observed (bounded 15s) before asserting and before clearing. This awaits a slow delta (no false-red) and guarantees every join broadcast is consumed before the clear, so none can leak into the leave phase (no false-green). A genuinely dropped broadcast still fails closed on the short count via the existing per-phase count floor.
Rework: merged main, resolved #1371, addressed fresh review — head
|
Coordinator merge-gate adjudication (2026-07-17 overnight run)Same classification as PR #1362 (comment 4998506565 there): this PR will be merged with the Load and Concurrency Harness check red, because that failure is the tracked #1358 mainline pair — raw log verified on this head ( This branch is a test-only C# change ( Review state: the 07-14 independent reviews (3 MEDIUM fixed) plus tonight's independent adversarial pass (1 MEDIUM + 3 LOW fixed) and two Codex rounds (2 P2 fixed) — all threads resolved, red-before-green discrimination proof posted for the #1371 delivery-coverage restoration. Reversible by revert if the maintainer disagrees; flagged in the morning report. |
Summary
Root cause
Concurrent SignalR broadcasts can reach the observer in a different order than the in-memory tracker created them. The original helper returned the last callback; the first repair selected
MaxBy(OccurredAt)behind a client wall-clock cutoff. Two independent reviews showed thatDateTimeOffset.UtcNowties are routine and cannot establish causal order, while the relaxed count assertions could false-green both lost successful joins and successful no-op leaves.The repaired test invokes
SetEditingCardwith a fresh marker only after each concurrent phase settles. No pre-barrier snapshot can contain that marker, so callback order and tied timestamps cannot select stale state. The marker snapshot is then checked against exact user identities rather than a count range.An earlier event-count attempt was also disproved locally: two successful leave invocations produced only one eligible observer callback. The final contract therefore does not assume one broadcast callback per invocation.
Review resolution
1f4bfd6dMaxByreport was invalid under .NET 8; the helper no longer usesMaxBy, and an explicit diagnostic-timeout regression locks the intended contractVerification — exact
1f4bfd6d022d4f883299607ccebe8035b3dd73a9Scope
Test/harness only. Production hub, tracker, frontend, API contracts, and canonical product docs are unchanged.
Closes #1365