feat: Configurable P2P block-sync timeout - #5035
Open
islamaliev wants to merge 9 commits into
Open
Conversation
Make the per-block DAG-sync fetch timeout operator-configurable via a new --p2p-block-sync-timeout flag and DEFRA_NET_P2PBLOCKSYNCTIMEOUT env var, and raise its default from 5s to 30s so deployments talking to a real SourceHub chain do not time out on the per-block access checks. Also add a per-request BlockSyncTimeout option to SyncDocuments (threaded through the HTTP and CLI clients) so an individual sync can override the node default, surface a block-fetch timeout as a distinct error rather than a generic load failure, and log when a receiver drops a pushed block it has no access to (previously silent). Refs sourcenetwork#4837
Add an integration test that drives SyncDocuments with a per-request BlockSyncTimeout too small for any block fetch to complete (the document does not sync) and again with a generous one (it syncs), proving the option is honoured through to the per-block fetch. Also correct a stale comment about the default block-sync timeout now that it is 30s.
The sender re-announces a document's head block on every update, so logging each access-denied drop would spam a node running under document ACP once per update for every not-yet-granted document. The drop is an expected, self-healing transient (the block re-arrives once the grant propagates), so leave it unlogged and document why.
📝 WalkthroughWalkthroughAdds configurable and per-request P2P block-fetch timeouts. The value flows through CLI, sync options, HTTP payloads, database context, and DAG loading, with updated defaults, timeout errors, OpenAPI schemas, documentation, unit tests, and integration coverage. ChangesP2P block synchronization timeout
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPHandler
participant Database
participant P2P
participant LinkSystem
Client->>HTTPHandler: POST sync_documents_params with blockSyncTimeout
HTTPHandler->>Database: SyncDocuments with timeout option
Database->>P2P: Add timeout to sync context
P2P->>LinkSystem: Load each block with resolved timeout
LinkSystem-->>P2P: Block or deadline error
P2P-->>Client: Sync result or block-sync timeout error
Possibly related PRs
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/net/sync/documents_block_sync_timeout_test.go`:
- Around line 55-61: Add the appropriate block-sync timeout error expectation to
the first testUtils.SyncDocs action by setting its ExpectedError field, so the
intentional timeout is treated as expected rather than a test failure.
🪄 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: f20ff08c-a8cf-4694-989f-759e44e924b2
📒 Files selected for processing (21)
cli/config/config.gocli/p2p_document_sync.gocli/start.goclient/options/p2p.godocs/website/references/cli/defradb_client_p2p_document_sync.mddocs/website/references/cli/defradb_start.mddocs/website/references/http/openapi.jsonhttp/client_p2p.gohttp/handler_p2p.gohttp/openapi.gointernal/db/config.gointernal/db/p2p.gointernal/db/p2p/errors.gointernal/db/p2p/sync_dag.gointernal/db/p2p/sync_dag_test.gonode/node.gotests/clients/cli/wrapper.gotests/integration/db.gotests/integration/net/sync/documents_block_sync_timeout_test.gotests/integration/p2p.gotests/integration/test_case.go
📜 Review details
⏰ Context from checks skipped due to timeout. (51)
- GitHub Check: Test coverage job (c, file, gql)
- GitHub Check: Test coverage job (cli, file, gql)
- GitHub Check: Test coverage job (c, memory, collection-named)
- GitHub Check: Test coverage job (go, file, gql)
- GitHub Check: Test coverage job (c, file, collection-named)
- GitHub Check: Test coverage job (c, memory, gql)
- GitHub Check: Test coverage job (cli, file, collection-named)
- GitHub Check: Test coverage job (cli, memory, collection-named)
- GitHub Check: Test coverage job (go, file, collection-save)
- GitHub Check: Test coverage job (go, file, collection-named)
- GitHub Check: Test coverage job (go, memory, collection-named)
- GitHub Check: Test coverage job (http, file, gql)
- GitHub Check: Test coverage job (c, memory, collection-save)
- GitHub Check: Test coverage job (cli, memory, gql)
- GitHub Check: Test coverage job (http, memory, collection-named)
- GitHub Check: Test coverage job (http, memory, gql)
- GitHub Check: Test coverage job (http, memory, collection-save)
- GitHub Check: Test coverage job (c, file, collection-save)
- GitHub Check: Test coverage job (http, file, collection-save)
- GitHub Check: Test coverage job (cli, file, collection-save)
- GitHub Check: Test coverage job (go, memory, gql)
- GitHub Check: Test coverage job (go, memory, collection-save)
- GitHub Check: Test coverage job (http, file, collection-named)
- GitHub Check: Test coverage job (cli, memory, collection-save)
- GitHub Check: Test coverage document acp job (http, source-hub)
- GitHub Check: Test coverage document acp job (c, source-hub)
- GitHub Check: Test coverage leveldb job
- GitHub Check: Test coverage document acp job (cli, source-hub)
- GitHub Check: Test coverage document acp job (go, source-hub)
- GitHub Check: Test coverage telemetry job
- GitHub Check: Test coverage secondary index job
- GitHub Check: Test coverage JS job
- GitHub Check: Test coverage view job
- GitHub Check: Test coverage signed docs job
- GitHub Check: Test coverage encryption job
- GitHub Check: Test coverage lens job (wazero)
- GitHub Check: Build dependencies job
- GitHub Check: Check data format changes job
- GitHub Check: Test Limited Resource job
- GitHub Check: Check mocks job
- GitHub Check: Validate containerfile job
- GitHub Check: Test macos job
- GitHub Check: Start binary job
- GitHub Check: Check wizard health job
- GitHub Check: Test NPX/JS build job
- GitHub Check: Check http documentation job
- GitHub Check: Check cli documentation job
- GitHub Check: Build and test Linux C shared library
- GitHub Check: Build, install, and test Debian package
- GitHub Check: Check vulnerabilities job
- GitHub Check: Lint GoLang 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:
cli/p2p_document_sync.gohttp/openapi.gotests/integration/net/sync/documents_block_sync_timeout_test.gointernal/db/p2p/sync_dag_test.gointernal/db/config.gocli/start.gonode/node.gointernal/db/p2p.gotests/integration/test_case.gotests/integration/p2p.goclient/options/p2p.gotests/clients/cli/wrapper.gocli/config/config.gohttp/client_p2p.gointernal/db/p2p/errors.gotests/integration/db.gohttp/handler_p2p.gointernal/db/p2p/sync_dag.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:
cli/p2p_document_sync.gohttp/openapi.gotests/integration/net/sync/documents_block_sync_timeout_test.gointernal/db/p2p/sync_dag_test.gointernal/db/config.gocli/start.gonode/node.gointernal/db/p2p.gotests/integration/test_case.gotests/integration/p2p.goclient/options/p2p.gotests/clients/cli/wrapper.gocli/config/config.gohttp/client_p2p.gointernal/db/p2p/errors.gotests/integration/db.gohttp/handler_p2p.gointernal/db/p2p/sync_dag.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:
cli/p2p_document_sync.gohttp/openapi.gotests/integration/net/sync/documents_block_sync_timeout_test.gointernal/db/p2p/sync_dag_test.gointernal/db/config.gocli/start.gonode/node.gointernal/db/p2p.gotests/integration/test_case.gotests/integration/p2p.goclient/options/p2p.gotests/clients/cli/wrapper.gocli/config/config.gohttp/client_p2p.gointernal/db/p2p/errors.gotests/integration/db.gohttp/handler_p2p.gointernal/db/p2p/sync_dag.go
🔇 Additional comments (25)
internal/db/config.go (1)
42-42: LGTM!node/node.go (1)
120-120: LGTM!internal/db/p2p/sync_dag.go (2)
15-47: LGTM!
123-134: LGTM!internal/db/p2p/errors.go (2)
75-75: LGTM!
166-172: LGTM!internal/db/p2p/sync_dag_test.go (1)
24-37: LGTM!docs/website/references/http/openapi.json (2)
630-649: LGTM!
2534-2534: LGTM!client/options/p2p.go (1)
14-15: LGTM!Also applies to: 487-505, 523-530
http/client_p2p.go (1)
44-56: LGTM!Also applies to: 347-360
http/handler_p2p.go (1)
259-259: LGTM!Also applies to: 279-286, 546-551
internal/db/p2p.go (1)
20-20: LGTM!Also applies to: 373-376
http/openapi.go (1)
44-44: LGTM!cli/config/config.go (1)
77-77: LGTM!Flag name, config key mapping, and default value (30 seconds) are consistent with the upstream consumers in
cli/start.goandinternal/db/config.go.Also applies to: 108-108
cli/start.go (1)
102-104: LGTM!The
> 0guard correctly falls back to the internal default when the config value is zero, and the flag definition matches the config wiring and documentation.Also applies to: 320-324
cli/p2p_document_sync.go (1)
44-46: LGTM!The per-request
--block-sync-timeoutduration flag, the> 0guard, and the updated--timeouthelp text are all correct and consistent with the upstreamSetBlockSyncTimeoutoption and the CLI wrapper.Also applies to: 57-59
tests/clients/cli/wrapper.go (1)
317-319: LGTM!The conditional
--block-sync-timeoutargument propagation correctly usesGetBlockSyncTimeout().HasValue()and serializes the duration via.String(), whichpflag.Durationparses on the receiving end.docs/website/references/cli/defradb_start.md (1)
36-36: LGTM!The documented flag name, type, help text, and default value match the implementation in
cli/start.goandcli/config/config.go.tests/integration/test_case.go (2)
15-15: LGTM!
720-722: LGTM!tests/integration/p2p.go (1)
266-268: LGTM!tests/integration/net/sync/documents_block_sync_timeout_test.go (1)
29-99: Test design looks solid overall.The two-phase approach (insufficient timeout → empty results, sufficient timeout → synced document) effectively exercises the
BlockSyncTimeoutoption end-to-end. The generous-timeout control case correctly rules out unrelated sync failures.tests/integration/db.go (1)
94-97: LGTM!docs/website/references/cli/defradb_client_p2p_document_sync.md (1)
29-31: LGTM!
Comment on lines
+55
to
+61
| testUtils.SyncDocs{ | ||
| NodeID: 1, | ||
| CollectionID: 0, | ||
| DocIDs: []int{0}, | ||
| SourceNodes: []int{0}, | ||
| BlockSyncTimeout: immutable.Some(time.Nanosecond), | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and inspect AssertError and assertExpectedErrorRaised implementations
rg -n 'func AssertError' --type go -A 20
rg -n 'func assertExpectedErrorRaised' --type go -A 15Repository: sourcenetwork/defradb
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the surrounding code.
git ls-files | rg '(^|/)(p2p\.go|sync_dag\.go|documents_block_sync_timeout_test\.go|.*test.*\.go)$'
# Find the helper implementations and the sync path.
rg -n 'func AssertError|func assertExpectedErrorRaised|ExpectedError|SyncDocuments|BlockSyncTimeout|NewErrBlockSyncTimeout' --type go
# If the files exist, print the relevant sections with line numbers.
for f in $(git ls-files | rg 'p2p\.go|sync_dag\.go|documents_block_sync_timeout_test\.go'); do
echo "===== $f ====="
wc -l "$f"
doneRepository: sourcenetwork/defradb
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- documents_block_sync_timeout_test.go ---'
sed -n '1,140p' tests/integration/net/sync/documents_block_sync_timeout_test.go
echo
echo '--- tests/action/sync_collection_versions.go ---'
sed -n '1,220p' tests/action/sync_collection_versions.go
echo
echo '--- tests/action/sync_branchable_collection.go (if present) ---'
if [ -f tests/action/sync_branchable_collection.go ]; then
sed -n '1,220p' tests/action/sync_branchable_collection.go
fi
echo
echo '--- assert helpers ---'
rg -n 'func (AssertError|assertExpectedErrorRaised)|ExpectedError' tests -g '*.go' -A 12 -B 4Repository: sourcenetwork/defradb
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- documents_block_sync_timeout_test.go ---'
sed -n '1,120p' tests/integration/net/sync/documents_block_sync_timeout_test.go
echo
echo '--- sync action definitions ---'
rg -n 'type SyncDocs|type SyncCollectionVersions|func \(a \*SyncDocs\) Execute|func \(a \*SyncCollectionVersions\) Execute|ExpectedError string' tests/action -g '*.go' -A 40 -B 8
echo
echo '--- error helpers ---'
rg -n 'func assertError|func assertExpectedErrorRaised|func assertErrors' tests/action -g '*.go' -A 40 -B 8Repository: sourcenetwork/defradb
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- SyncDocs definition and Execute ---'
rg -n 'type SyncDocs|func \(.*SyncDocs.*\) Execute|ExpectedError string' tests -g '*.go' -A 80 -B 12 --max-count 20
echo
echo '--- assertError / assertExpectedErrorRaised / assertErrors ---'
rg -n 'func assertError|func assertExpectedErrorRaised|func assertErrors' tests -g '*.go' -A 40 -B 8 --max-count 20
echo
echo '--- p2p sync path references ---'
rg -n 'SyncDocuments|NewErrBlockSyncTimeout|BlockSyncTimeout' internal tests -g '*.go' -A 20 -B 10 --max-count 50Repository: sourcenetwork/defradb
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate SyncDocs ---'
fd -a 'sync_.*\.go$|.*sync.*\.go$' tests/action tests/integration | rg 'sync_docs|sync.*doc|SyncDocs' -n
echo
echo '--- SyncDocs implementation ---'
rg -n 'type SyncDocs|func \(a \*SyncDocs\) Execute|ExpectedError string' tests -g '*.go' -A 80 -B 12 --max-count 10 | sed -n '1,220p'
echo
echo '--- error helper implementations ---'
rg -n 'func assertError|func assertExpectedErrorRaised|func assertErrors' tests/action tests/integration -g '*.go' -A 60 -B 10 --max-count 20 | sed -n '1,220p'Repository: sourcenetwork/defradb
Length of output: 29416
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tests/integration/p2p.go: SyncDocs ---'
rg -n 'type SyncDocs|func \(.*SyncDocs.*\) Execute|SyncDocs' tests/integration/p2p.go -A 80 -B 20
echo
echo '--- timeout error path ---'
rg -n 'NewErrBlockSyncTimeout|BlockSyncTimeout|SyncDocuments\(' internal/db/p2p/sync_dag.go internal/db/p2p -A 40 -B 20Repository: sourcenetwork/defradb
Length of output: 39169
Add ExpectedError to the first SyncDocs action
tests/integration/net/sync/documents_block_sync_timeout_test.go:55
This call is expected to hit the block-sync timeout, and the test helper treats any non-nil error as a failure when ExpectedError is empty. Set the timeout error expectation on this action.
🤖 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/net/sync/documents_block_sync_timeout_test.go` around lines
55 - 61, Add the appropriate block-sync timeout error expectation to the first
testUtils.SyncDocs action by setting its ExpectedError field, so the intentional
timeout is treated as expected rather than a test failure.
islamaliev
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #5035 +/- ##
===========================================
- Coverage 76.32% 76.28% -0.04%
===========================================
Files 620 620
Lines 49049 49082 +33
===========================================
+ Hits 37433 37438 +5
- Misses 8544 8562 +18
- Partials 3072 3082 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
The test assumed a tiny per-block timeout would stop a document from syncing, but the per-block timeout only applies to a document's linked blocks; a freshly created single-block document has none, so it synced regardless and the negative assertion failed on the C-bindings client. The per-request timeout resolution is already covered by a deterministic unit test, so drop the integration test and the SyncDocs framework field that existed only for it.
nasdf
approved these changes
Jul 21, 2026
nasdf
left a comment
nasdf
left a comment
Member
There was a problem hiding this comment.
Changes look good! I left two comments I think are worth looking into.
| "net.peers": []string{}, | ||
| "net.pubSubEnabled": true, | ||
| "net.relay": false, | ||
| "net.p2pblocksynctimeout": 30, |
Member
There was a problem hiding this comment.
thought: It could be nicer to use a duration here (https://pkg.go.dev/time#ParseDuration).
| --no-signing Disable signing of commits. | ||
| --no-telemetry Disables telemetry reporting. Telemetry is only enabled in builds that use the telemetry flag. | ||
| --node-acp-enable Enable the node access control system. | ||
| --p2p-block-sync-timeout int Timeout in seconds for fetching each block during P2P DAG sync (default 30) |
Member
There was a problem hiding this comment.
suggestion: It would be nice if this was a duration like the other flags.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relevant issue(s)
Refs #4837
Description
Fetching each block during a P2P document sync was bounded by a fixed five-second timeout that operators could not change. When a node talks to a real access-control chain, the per-block authorization check can take longer than that, so syncs would fail with no way to give them more room. The timeout is now configurable via a flag and environment variable, its default is raised to thirty seconds, and an individual sync request can override it for that call alone. A block fetch that runs out of time now reports a distinct timeout error instead of a generic load failure.
This is one half of the work on the linked issue; a separate change fixes the underlying access-control cost that made the timeout matter.
Tasks
How has this been tested?
Unit tests for the timeout resolution and the distinct error, and an integration test that drives a sync with a per-request timeout too small to complete (the document does not sync) and again with a generous one (it does), confirming the override reaches the per-block fetch. Existing P2P sync suites continue to pass.
Tested on: