Release the check-in anchor when the check-in never reached the backend - #71
Conversation
…e backend The gate claims its anchor before the upstream call, which is what makes the burst race safe to close. The cost was that a check-in which never landed still held the window: on an upstream timeout or 5xx, another attempt for that account inside the window was absorbed with a 201 even though nothing had been recorded. Same shape as the bug this gate was just fixed for, with a narrower trigger. Pipe turns a transport failure into 504/500, so a 5xx on the response is exactly the "not delivered" set. An upstream 4xx is a deliberate rejection that a retry would not change, so it keeps the window. Release names the exact anchor it claimed and removes only that one, under the same stripe lock, so a stale release arriving after the account has checked in again cannot discard the live anchor. Not awaiting the upstream result to stamp only on success instead: that means hand-rolling the Express-compatible response path Pipe owns (invariant 3), and a 2xx from the backend does not mean the check-in was credited anyway, since its verifier decides that later. Closes #70
Code Review by Qodo
1.
|
|
Warning Review limit reached
Next review available in: 27 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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. Comment |
PR Summary by QodoRelease check-in gate anchor on upstream non-delivery (5xx)
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Greptile SummaryThe PR adds exact-stamp anchor release for check-ins that fail before reaching the backend and invokes it from a finally block around upstream forwarding. However, the handler currently conflates locally generated transport-error statuses with 5xx responses returned by a backend that received the request.
Confidence Score: 4/5The PR is not yet safe to merge because a backend-originated 5xx can release an anchor for a request that was delivered and allow a duplicate retry. Upstream.Pipe propagates ordinary backend 5xx statuses into the response, while the new finally block interprets every such status as non-delivery and removes the reservation. Files Needing Attention: dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs
|
| Filename | Overview |
|---|---|
| dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs | Adds finally-based reservation cleanup, but its status-only condition cannot distinguish transport non-delivery from a backend-originated 5xx after delivery. |
| dotnet/EcencyApi/Infrastructure/CheckinGate.cs | Adds an exact-stamp, stripe-locked release operation that safely avoids deleting a newer anchor. |
| dotnet/EcencyApi.Tests/CheckinGateTests.cs | Covers release and stale-release behavior in the gate, but does not exercise the handler’s classification of upstream outcomes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Reserve account anchor] --> B[Send check-in upstream]
B --> C{Outcome}
C -->|Transport exception| D[Pipe synthesizes 500 or 504]
C -->|Backend responds 5xx| E[Pipe propagates backend status]
D --> F{Response status at least 500}
E --> F
F --> G[Release anchor]
G --> H[Immediate retry is forwarded]
E --> I[Backend may already have processed check-in]
I --> H
H --> J[Possible duplicate check-in]
Reviews (2): Last reviewed commit: "fix(checkin): release the anchor from a ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…started case Review found two ways the release could be skipped, both leaving a failed check-in holding the window, which is the failure this gate is being fixed for. Pipe can throw out of the write itself, so a release that sits after the await never runs when a client disconnects mid-response. It moves into a finally. ApiRequest builds the auth headers eagerly and throws on a misconfigured deployment, so the request can fail before Pipe is entered at all. The status code cannot report that, since nothing set it, so an explicit flag marks whether the upstream call ever started. A backend answer that only failed on the way back to a client that went away still keeps the anchor: the check-in landed, and SendLikeExpress sets the upstream status before it writes, so the status still reports that in the finally. Release is now silent as well as swallowing. It runs after the response is written, so an escaping exception would raise an error the client can no longer be told about, and a request handler should not be adding logging.
96ca9d5 to
5a877cb
Compare
| // landed, and SendLikeExpress sets the upstream status before it | ||
| // writes, so the status still reports that here. The release has to | ||
| // sit in a finally, because Pipe can throw out of the write itself. | ||
| if (reservedAnchor != null && (!upstreamStarted || ctx.Response.StatusCode >= 500)) |
There was a problem hiding this comment.
If the backend receives and processes a check-in but returns a 5xx response, Upstream.Pipe propagates that status and this condition treats it as non-delivery. The anchor is removed, so an immediate retry is forwarded and can duplicate a check-in the backend already received or credited.
There was a problem hiding this comment.
Accurate, and intentional. A backend that recorded the check-in and then answered 5xx does lose its anchor here.
The cost of that is one extra upstream call: the next check-in forwards early, and the backend's own per-account spacing refuses it as too close. No double credit, because this gate is not what decides credit.
The cost of the other choice is an account losing a check-in and its streak, silently, because the gate answers 201. That asymmetry is the whole reason this endpoint is being fixed, so every boundary case here resolves toward forwarding. It is written up as an invariant at the top of CheckinGate rather than left as an accident.
There is also no signal that would let the gate do better. A 5xx cannot be split into "recorded then failed" and "never recorded" from this side, and even a clean 2xx does not mean credited: the backend's verifier decides that asynchronously, after the response.
1 participant
Closes #70. Follow-up to #69, from a Qodo finding that landed on that PR after it merged.
The gate claims its anchor before the upstream call, which is what makes the burst race safe to close. The cost was that a check-in which never reached the backend still held the window: on an upstream timeout or 5xx, another attempt for that account inside the window was absorbed with a 201 even though nothing had been recorded. That is the same shape as the bug #69 fixed, with a narrower trigger.
Change
CheckinGate.Release(username, stamp)gives the anchor back, and the handler calls it when the piped response came back 5xx.Upstream.Pipeturns a transport failure into 504/500, so a 5xx is exactly the "not delivered" set. An upstream 4xx is a deliberate rejection that a retry would not change, so it keeps the window.The release names the exact anchor it claimed and removes only that one, under the same stripe lock as the reservation, so a stale release arriving after the account has checked in again cannot discard the live anchor. Both properties are pinned by tests.
Alternative considered and rejected
Awaiting the upstream result and stamping only on success. Two problems: it means hand-rolling the Express-compatible response path that
Pipeowns, which invariant 3 protects, and a 2xx from the backend does not mean the check-in was credited anyway, since its verifier decides that asynchronously. Releasing on a known non-delivery gets the part that is actually knowable here.Tests
AnUndeliveredCheckinGivesTheAnchorBackandAReleaseCannotDiscardALaterAccountsAnchor. Full suite green: 130 passed, build clean with no warnings.