fix: Timeout on concurrent doc-sync for the same doc - #5021
Conversation
Concurrent SyncDocuments calls for the same docID collided on the pubsub-rpc response-channel key, which is a hash of the request bytes. The request carried nothing unique, so identical requests shared one in-flight slot: one call received the response and the rest blocked until their deadline and returned "timeout while syncing doc", even though the document actually synced. Add a random RequestID nonce to docSyncRequest so each request marshals to distinct bytes and gets its own response channel, mirroring the KMS pubsub request.
islamaliev
changed the title
📝 WalkthroughWalkthroughThis PR adds a random nonce to doc sync requests, updates integration helpers for concurrent same-document calls, and adds an integration test that verifies four simultaneous requests succeed. ChangesNonce fix and concurrency test support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Test as Integration test
participant syncDocs
participant Node
participant syncDocuments
participant Pubsub
Test->>syncDocs: SyncDocs(Concurrency=4, same DocID)
loop 4 concurrent calls
syncDocs->>Node: SyncDocuments(docIDs)
Node->>syncDocuments: Build docSyncRequest
syncDocuments->>syncDocuments: Generate RequestID
syncDocuments->>Pubsub: Publish unique request
Pubsub-->>syncDocuments: Route response
syncDocuments-->>Node: Return result
Node-->>syncDocs: Return result
end
syncDocs->>syncDocs: Assert each result
Assessment against linked issues
Out-of-scope changes
Suggested labels: Suggested reviewers: 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/p2p.go`:
- Around line 288-294: The per-call failure message in the concurrent
SyncDocuments loop is unreachable because AssertError already calls
require.NoError when action.ExpectedError is empty, so the later require.NoError
never runs with context. Update the loop in the p2p integration test to avoid
double fatal checks: let AssertError handle expected-error validation, and use a
non-fatal assertion such as assert.NoError for the contextual "concurrent
SyncDocuments call %d/%d failed" message so all errs entries are evaluated
before failing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d08e9ba-d5e1-4cd3-a9a0-2d3d1775b64d
📒 Files selected for processing (4)
internal/db/p2p/sync_doc.gotests/integration/net/sync/documents_test.gotests/integration/p2p.gotests/integration/test_case.go
📜 Review details
⏰ Context from checks skipped due to timeout. (51)
- GitHub Check: Test Limited Resource job
- GitHub Check: Build and test Linux C shared library
- GitHub Check: Start binary job
- GitHub Check: Test coverage job (c, file, collection-named)
- GitHub Check: Test coverage job (cli, file, collection-save)
- GitHub Check: Test coverage job (c, memory, collection-save)
- GitHub Check: Test coverage job (cli, file, collection-named)
- GitHub Check: Test coverage job (c, file, gql)
- GitHub Check: Test coverage job (c, memory, collection-named)
- GitHub Check: Test coverage job (c, file, collection-save)
- GitHub Check: Test coverage job (cli, memory, collection-named)
- GitHub Check: Test coverage job (c, memory, gql)
- GitHub Check: Test coverage job (cli, file, gql)
- GitHub Check: Test coverage job (http, file, collection-named)
- GitHub Check: Test coverage job (cli, memory, gql)
- GitHub Check: Test coverage job (http, memory, collection-save)
- GitHub Check: Test macos job
- GitHub Check: Test coverage job (cli, memory, collection-save)
- GitHub Check: Test coverage job (http, file, gql)
- GitHub Check: Test coverage job (go, file, gql)
- GitHub Check: Test coverage job (go, file, collection-save)
- GitHub Check: Test coverage job (http, memory, collection-named)
- GitHub Check: Test coverage job (http, memory, gql)
- GitHub Check: Test coverage job (http, file, collection-save)
- GitHub Check: Test coverage job (go, memory, gql)
- GitHub Check: Test coverage job (go, memory, collection-named)
- GitHub Check: Test coverage job (go, memory, collection-save)
- GitHub Check: Test coverage job (go, file, collection-named)
- GitHub Check: Check wizard health job
- GitHub Check: Build, install, and test Debian package
- GitHub Check: Check data format changes job
- GitHub Check: Test coverage document acp job (go, source-hub)
- GitHub Check: Validate containerfile job
- GitHub Check: Test coverage secondary index job
- GitHub Check: Test coverage document acp job (c, source-hub)
- GitHub Check: Test coverage document acp job (http, source-hub)
- GitHub Check: Test coverage signed docs job
- GitHub Check: Test coverage JS job
- GitHub Check: Test coverage lens job (wazero)
- GitHub Check: Test coverage leveldb job
- GitHub Check: Test coverage encryption job
- GitHub Check: Test coverage document acp job (cli, source-hub)
- GitHub Check: Test coverage telemetry job
- GitHub Check: Test coverage view job
- GitHub Check: Check mocks job
- GitHub Check: Test NPX/JS build job
- GitHub Check: Check vulnerabilities job
- GitHub Check: Build dependencies job
- GitHub Check: Check http documentation job
- GitHub Check: Check cli documentation job
- GitHub Check: Lint GoLang job
⚠️ CI failures not shown inline (2)
GitHub Actions: Validate Title Workflow / 0_Validate title job.txt: fix(p2p): concurrent SyncDocuments for the same doc no longer time out
Conclusion: failure
##[group]Run ./tools/scripts/validate-conventional-style.sh "fix(p2p): concurrent SyncDocuments for the same doc no longer time out"
�[36;1m./tools/scripts/validate-conventional-style.sh "fix(p2p): concurrent SyncDocuments for the same doc no longer time out"�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
Error: The length of the title is too long (should be 60 or less).
##[error]Process completed with exit code 3.
GitHub Actions: Validate Title Workflow / Validate title job: fix(p2p): concurrent SyncDocuments for the same doc no longer time out
Conclusion: failure
##[group]Run ./tools/scripts/validate-conventional-style.sh "fix(p2p): concurrent SyncDocuments for the same doc no longer time out"
�[36;1m./tools/scripts/validate-conventional-style.sh "fix(p2p): concurrent SyncDocuments for the same doc no longer time out"�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
Error: The length of the title is too long (should be 60 or less).
##[error]Process completed with exit code 3.
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-03-27T08:24:16.083Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4661
File: http/middleware.go:93-93
Timestamp: 2026-03-27T08:24:16.083Z
Learning: When handling `client.ErrNotAuthorizedToPerformOperation` in the defradb repository, treat it as an unauthenticated/missing-identity condition (not insufficient permissions). Map it to HTTP 401 Unauthorized rather than 403 Forbidden; this mapping is intentional and consistent with prior behavior (pre-PR `#4661`).
Applied to files:
tests/integration/net/sync/documents_test.gotests/integration/test_case.gotests/integration/p2p.gointernal/db/p2p/sync_doc.go
📚 Learning: 2026-06-18T16:47:47.339Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4942
File: internal/db/db.go:227-231
Timestamp: 2026-06-18T16:47:47.339Z
Learning: When reviewing this repository’s Go code, do not flag `sync.WaitGroup.Go()` (e.g., `wg.Go(fn)`) as a compilation error. This method exists starting in Go 1.25; it should be treated as valid as long as the repo’s configured/minimum Go toolchain version is 1.25+ (e.g., per `go.mod` and/or CI). If the project is building with an older Go version, then it may be appropriate to flag it.
Applied to files:
tests/integration/net/sync/documents_test.gotests/integration/test_case.gotests/integration/p2p.gointernal/db/p2p/sync_doc.go
📚 Learning: 2026-06-18T16:47:47.339Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4942
File: internal/db/db.go:227-231
Timestamp: 2026-06-18T16:47:47.339Z
Learning: In sourcenetwork/defradb code reviews, do not raise findings that are likely compiler/type-check errors (e.g., code that would fail `go test`/`go build`) because CI pipelines will catch compilation failures. Only flag compilation-related problems if there is a clear, verifiable issue beyond what CI will already surface (e.g., a mismatched build tag or missing generated artifact that CI would not cover).
Applied to files:
tests/integration/net/sync/documents_test.gotests/integration/test_case.gotests/integration/p2p.gointernal/db/p2p/sync_doc.go
🔇 Additional comments (4)
internal/db/p2p/sync_doc.go (1)
15-15: LGTM!Also applies to: 44-48, 119-125
tests/integration/test_case.go (1)
716-724: LGTM!tests/integration/p2p.go (1)
16-16: LGTM!Also applies to: 249-287, 296-315
tests/integration/net/sync/documents_test.go (1)
396-434: LGTM!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5021 +/- ##
===========================================
+ Coverage 76.28% 77.21% +0.93%
===========================================
Files 624 607 -17
Lines 49519 47541 -1978
===========================================
- Hits 37774 36706 -1068
+ Misses 8616 7810 -806
+ Partials 3129 3025 -104
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 78 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
The concurrent SyncDocs path shared a single options builder across all goroutines. The builder is a stateful, single-use enumerable, so the calls raced on and exhausted its cursor, failing the race detector in CI. Build a fresh builder inside each call instead.
islamaliev
changed the title
| // pubsub-rpc layer keys its response channel by a hash of the request bytes, so without this | ||
| // two concurrent syncs for the same docIDs would collide on that key and one would time out | ||
| // while the other succeeds. Mirrors the KMS pubsub fix (fetchEncryptionKeyRequest). | ||
| RequestID []byte `json:"requestID"` |
There was a problem hiding this comment.
question: Why split the requests like this, instead of combining them?
What is the benefit to users in keeping them seperate?
There was a problem hiding this comment.
combining them sounds interesting. If two callers ask for the same documents at the same time, sending two identical requests is wasteful. We can do that with single-flight at the DB level: the first caller does the real request, and any concurrent caller asking for the same documents waits on that result.
And to be clear: the routing collision only ever happens for byte-identical requests, i.e. the same docIDs. Different or even partially-overlapping docID sets already have different keys and never collide.
I can proceed with single-flight solution unless you had another solution in mind.
There was a problem hiding this comment.
Another caveat that we should consider if we decide with combining them is that we also have to include an identity to the serialized key. So in total we will serialize key on collection name on doc ids and on identity. We need to do this because for different users the results should be different. That kind of adds additional overhead. To every request! What do you think?
There was a problem hiding this comment.
sending two identical requests is wasteful
This is a very small amount of wastage though no?
Another caveat that we should consider if we decide with combining them
Sorry, my original comment was ambiguous. I wasn't really talking about combining the request messages, only their results.
So if two sync requests were requested by users, two would be sent, and their timeouts/results would merge - the second request to be handled by the client-node would essentially extend the timeout begun by the first request. The first request would timeout if/when it's timeout its timeout is reached, the second if/when its timeout is reached.
The server-nodes would need to re-check upon receipt of the second request in case their are new heads, but there is no need to keep the requests or responses seperate via a nonce - either results are recieved by the client-node within the relevant timeout, or they are not - it does not matter which request triggered the response, they are all equally valid?
AndrewSisley
left a comment
AndrewSisley
left a comment
There was a problem hiding this comment.
LGTM, sorry - I probably should have approved this when I asked the pending question - it is not a blocker - is just me exploring the solution and wondering if we can make additional improvements (especially as dropping the new prop is a breaking change).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/index/list_collection_name_test.go (1)
60-105: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise the multi-collection query that this test claims to cover.
The test makes two separate
action.ListIndexescalls. Each call sets oneCollectionID, so neither call returns results from both collections. A bug that labels every result with one collection name can pass this test. Use the actual multi-collection listing path in one call. If no such path exists, rename the test and comments to describe two independent collection-scoped calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/index/list_collection_name_test.go` around lines 60 - 105, The test currently makes two collection-scoped ListIndexes calls, so it does not exercise a multi-collection query. Update TestIndexList_WithMultipleCollections_EachResultNamesItsOwnCollection to use the actual multi-collection listing path in a single action and assert both collections’ indexes and names; if that API does not exist, rename the test to describe independent collection-scoped calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/integration/index/list_collection_name_test.go`:
- Around line 60-105: The test currently makes two collection-scoped ListIndexes
calls, so it does not exercise a multi-collection query. Update
TestIndexList_WithMultipleCollections_EachResultNamesItsOwnCollection to use the
actual multi-collection listing path in a single action and assert both
collections’ indexes and names; if that API does not exist, rename the test to
describe independent collection-scoped calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ce3af0c2-702b-45e3-a5db-5ae7a87588d7
📒 Files selected for processing (1)
tests/integration/index/list_collection_name_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (25)
- GitHub Check: Test NPX/JS build job
- GitHub Check: Check http documentation job
- GitHub Check: Check cli documentation job
- GitHub Check: Lint GoLang job
- GitHub Check: Build, install, and test Debian package
- GitHub Check: Test coverage job (http, memory, gql)
- GitHub Check: Test Limited Resource job
- GitHub Check: Check wizard health job
- GitHub Check: Validate containerfile job
- GitHub Check: Test coverage document acp job (cli, source-hub)
- GitHub Check: Test coverage document acp job (c, source-hub)
- GitHub Check: Test coverage document acp job (go, source-hub)
- GitHub Check: Test coverage document acp job (http, source-hub)
- GitHub Check: Test coverage JS job
- GitHub Check: Test coverage encryption job
- GitHub Check: Build and test Linux C shared library
- GitHub Check: Test coverage view job
- GitHub Check: Test coverage signed docs job
- GitHub Check: Test coverage lens job (wazero)
- GitHub Check: Start binary job
- GitHub Check: Check mocks job
- GitHub Check: Build dependencies job
- GitHub Check: Check vulnerabilities job
- GitHub Check: Check mod tidy job
- GitHub Check: Test macos job
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-03-27T08:24:16.083Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4661
File: http/middleware.go:93-93
Timestamp: 2026-03-27T08:24:16.083Z
Learning: When handling `client.ErrNotAuthorizedToPerformOperation` in the defradb repository, treat it as an unauthenticated/missing-identity condition (not insufficient permissions). Map it to HTTP 401 Unauthorized rather than 403 Forbidden; this mapping is intentional and consistent with prior behavior (pre-PR `#4661`).
Applied to files:
tests/integration/index/list_collection_name_test.go
📚 Learning: 2026-06-18T16:47:47.339Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4942
File: internal/db/db.go:227-231
Timestamp: 2026-06-18T16:47:47.339Z
Learning: When reviewing this repository’s Go code, do not flag `sync.WaitGroup.Go()` (e.g., `wg.Go(fn)`) as a compilation error. This method exists starting in Go 1.25; it should be treated as valid as long as the repo’s configured/minimum Go toolchain version is 1.25+ (e.g., per `go.mod` and/or CI). If the project is building with an older Go version, then it may be appropriate to flag it.
Applied to files:
tests/integration/index/list_collection_name_test.go
📚 Learning: 2026-06-18T16:47:47.339Z
Learnt from: islamaliev
Repo: sourcenetwork/defradb PR: 4942
File: internal/db/db.go:227-231
Timestamp: 2026-06-18T16:47:47.339Z
Learning: In sourcenetwork/defradb code reviews, do not raise findings that are likely compiler/type-check errors (e.g., code that would fail `go test`/`go build`) because CI pipelines will catch compilation failures. Only flag compilation-related problems if there is a clear, verifiable issue beyond what CI will already surface (e.g., a mismatched build tag or missing generated artifact that CI would not cover).
Applied to files:
tests/integration/index/list_collection_name_test.go
🔇 Additional comments (1)
tests/integration/index/list_collection_name_test.go (1)
1-56: LGTM!
2 participants
Relevant issue(s)
Resolves #5020
Description
Calling SyncDocuments more than once at the same time for the same document made all but one of the calls block until their deadline and return "timeout while syncing doc", even though the document actually synced. The concurrent requests were indistinguishable from each other, so they shared a single in-flight response slot: one call received the peer's reply and the rest were left waiting. Each request now carries a random nonce so concurrent syncs for the same document stay independent, matching what the encryption-key sync path already does.
How has this been tested?
Added an integration test that fires several concurrent SyncDocuments calls for the same document and asserts they all succeed (it fails without the fix). Existing P2P sync tests continue to pass.