Repository navigation
Document the ledger order for sent writes - #3346
Conversation
The gate told the browser to block a write, or let a sent write's answer through, before it kept the event. A reader that saw the outcome in the page, such as the step after an operation returned, could read the events before the gate kept that one, and under load it did: CI saw the last blocked write missing. The gate now keeps the event first, so the outcome a page sees is always already readable.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Ledger and continuation ordering internal/browserhost/writegate/gate.go |
Blocked requests and response-error, unreadable-response, and body-retrieval-failure events are recorded before continuation. For readable responses, the callback continues the response before recording the event. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Possibly related PRs
- dagucloud/dagu#3342: Describes the same ledger-recording and continuation ordering changes in
gate.go. - dagucloud/dagu#3337: Added response-stage pauses and response event recording in
gate.go, which this change reorders. - dagucloud/dagu#3336: Added the
pausedandansweredhandlers that this change updates.
Merge Risk: 
54a6a
-
No actionable merge-blocking risk remains in the reviewed change.
Pre-merge checks: 
-
Check name Status Explanation Description check The description follows the template's main sections. It has a Summary that explains the ordering bug and the fix. It has a Changes-style explanation of the gate reordering, but it lacks a dedicated C… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly identifies the main change: ledger ordering for sent writes. The implementation records events before the browser sees the outcome. Full details: Description check
-
Explanation
The description follows the template's main sections. It has a Summary that explains the ordering bug and the fix. It has a Changes-style explanation of the gate reordering, but it lacks a dedicated Changes list with one item per line. It has no Related Issues section, and it does not reference a closing issue number. It lacks the Checklist, so the template's checklist items are not addressed. It includes a Test plan and Evidence, which go beyond the template. The required structure is partly present, and the missing parts are non-critical for a small code change, so the check passes.
-
Finishing Touches: 💡 1
-
⚔️ Resolve merge conflicts: 💡
-
- Resolve merge conflict in branch
fix-write-gate-ledger-order
- Resolve merge conflict in branch
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
…-order # Conflicts: # internal/browserhost/writegate/gate.go
yohamta0
changed the title
Summary
Main CI fails in
internal/browserhost/writegateafter the write gate merges. On #3337's and #3338's squash commits,TestGateRecordSendsWhatTheOperationMayandTestGateSendsSignInAndWhatAPersonDoeseach lost their last event: the page saw its write blocked, but the next read of the gate's events did not contain it.The cause is a real ordering bug in the gate, not in the tests. The gate told the browser what it decided, using
Fetch.failRequest,Fetch.fulfillRequestorFetch.continueResponse, and only then kept the event in its ledger. A reader that sees the outcome in the page and then reads the events can arrive between those two lines. This is what the step runtime does after an operation returns, and what the tests do after afetchsettles. Under load on a CI runner, it did.The gate now keeps each event before it carries out the decision, for blocked writes and for every sent-write path. So an outcome the page sees is always readable.
Evidence
Test plan
go test -race -count=20 ./internal/browserhost/writegate/, both idle and with 12 CPU-bound processes runninggolangci-lintfor darwin and windowsSummary by CodeRabbit