Log duplicate escalated write requests#2681
Conversation
Track the ids of escalated requests in flight on the leader and log when a duplicate arrives while an identical request is still being processed. Expose a cumulative counter via Status and add a cluster test covering it.
Match the convention used across the cluster tests: node 0 is the leader, node 1 a follower, guarded with waitForState, instead of scanning nodes.
rlinoz
changed the title
rlinoz
changed the title
|
@danieldoglas is this what you had in mind? |
![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: ac42ad6461
ℹ️ 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".
| // The escalated request is done, stop tracking it. | ||
| if (fromPrivateCommandPort) { | ||
| lock_guard<mutex> lock(_inFlightEscalatedRequestsMutex); | ||
| _inFlightEscalatedRequests.erase(escalatedID); |
There was a problem hiding this comment.
When a duplicate request hits the insert failure path, it still executes and then reaches this unconditional erase; if that duplicate finishes before the original, it removes the original request's marker even though the original is still in flight. A third copy of the same escalated request arriving during that window will insert successfully and won't increment/log, so the new status counter under-reports the repeated in-flight duplicates it is meant to measure. Track whether this handler actually inserted the id, or use a refcount, before erasing.
Useful? React with 👍 / 👎.
|
@rlinoz do you know where the ID is coming from? Does it come from PHP? |
|
From what I understand It comes from here Line 1574 in ac42ad6 We generate the ID in Bedrock to build the command. |
|
Yeah, so that's kind of a problem. The issue is that PHP is resending the command to followers, which are escalated to leader. So leader would always receive a different ID on those cases |
|
aaah I got it wrong, from the issue I thought followers were duplicating the commands, that makes more sense. I will rethink this, thanks! |
…rites PHP re-sends a timed-out write to a different follower, and each follower mints its own command id, so command->id can't identify the duplicate copy that lands on the leader. Fingerprint the request content instead (excluding the transport headers a follower stamps on while escalating), so two copies of one logical write - same requestID and payload - match. Also track duplicates for writes that reach the leader directly on the public command port (client connected to the leader), not just escalations on the private port, and extract the tracking into trackInFlightWrite/ untrackInFlightWrite.
| canonical += '\n'; | ||
| } | ||
| canonical += request.content; | ||
| return hash<string>{}(canonical); |
There was a problem hiding this comment.
I think hashing the request is better than sending another unique ID from PHP, mostly because it is less moving parts.
I say another because there is the requestID but we forward that to other commands, so not unique enough.
Do you have any thoughts @danieldoglas ?
2 participants
Details
Duplicate escalated write requests may reach the leader and be executed more than once, causing write amplification (identified during fire analysis).
This is a protective, observability-first step: on the leader, we now track the ids of escalated requests currently in flight and detect when the same request arrives again while a previous copy is still being processed. For now duplicates are only logged (via
SWARN) — they are not dropped — so we can first confirm whether/how often this actually happens in production.Fixed Issues
For https://github.com/Expensify/Expensify/issues/649211
Tests
Added
EscalateTest::duplicateEscalatedRequestDetected, which escalates two identical write commands sharing one id from a follower (kept overlapping in flight on the leader via a slow process step) and verifies the leader'sduplicateEscalatedRequestCountincreased.Internal Testing Reminder: when changing bedrock, please compile auth against your new changes