fix(propagation): bound /txs chunks by payload bytes, not just tx count - #342
Open
galt-tr wants to merge 1 commit into
Open
fix(propagation): bound /txs chunks by payload bytes, not just tx count#342galt-tr wants to merge 1 commit into
galt-tr wants to merge 1 commit into
Conversation
Teranode's /txs handler caps a request at 1024 transactions and 32 MiB, checked with >= before each read, and answers a bare 400 carrying no per-tx verdict after it has already dispatched everything it read. broadcastInChunks only bounded count, so an oversized chunk was requeued at the same size until the retry budget parked it, invisible to endpoint health.
- planChunks bounds every chunk by teranode_max_batch_size (default 1024 -> 1000; a chunk of exactly 1024 is refused upstream) and the new teranode_max_batch_bytes (default 16 MiB); a lone oversize tx still travels alone.
- teranode.ErrBatchTooLarge marks a 413 or one of the three bare-400 limit bodies; the propagator narrows the txs nobody voted on, requeues a chunk of one quoting the peer, and keeps the circuit breaker neutral.
- arcade_propagation_chunk_bytes histogram, batch_bytes log fields, chunk_total{fallback="size_rejected"}, and a startup warn when a configured cap reaches Teranode's limit.
- PENDING_RETRY park text no longer uses the missing-parent wording for non-parent reasons.
Fixes #271
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core propagation broadcast/narrowing behavior and error classification across multiple paths (dispatcher + reaper + endpoint health accounting), so a final human review is warranted despite the strong test coverage.
Pull request overview
This PR updates the propagation service’s /txs batching so outbound broadcasts are chunked by both transaction count and total payload bytes, aligning Arcade’s behavior with Teranode’s strict request-shape limits and preventing requeue storms caused by oversized batches.
Changes:
- Add byte-aware chunk planning (
planChunks) and wire it into both the dispatcher and reaper rebroadcast paths. - Introduce
propagation.teranode_max_batch_bytes(default 16 MiB) and lower the defaultteranode_max_batch_size(1024 → 1000), with exported defaults used consistently by config and runtime fallback. - Detect Teranode batch-shape refusals (413, and specific bare 400 bodies) as
ErrBatchTooLarge, triggering narrowing/requeue behavior without misclassifying endpoint health or terminalizing transactions.
File summaries
| File | Description |
|---|---|
| tests/smoke/recording_teranode.go | Record request body byte length for smoke assertions. |
| tests/smoke/harness.go | Explicitly set shipped batch caps for smoke tests. |
| tests/smoke/chained_txs_test.go | Assert both count and byte caps for recorded /txs batches. |
| teranode/client.go | Add ErrBatchTooLarge classification for 413 and exact-match 400 limit bodies. |
| teranode/client_test.go | Extend non-parseable 4xx coverage to include 413. |
| teranode/client_batch_limit_test.go | New tests pin exact batch-limit bodies and errors.Is(ErrBatchTooLarge) behavior. |
| services/propagation/propagator.go | Use byte-aware chunking, emit chunk-bytes metrics/log fields, narrow on shape refusals, adjust requeue wording. |
| services/propagation/propagator_test.go | Assert captured batch body sizes are bounded and sum correctly. |
| services/propagation/chunking.go | New: planChunks, span fullness logic, and rawTxsBytes helper. |
| services/propagation/chunking_test.go | New: table + property/invariant tests for byte-aware chunk planning. |
| services/propagation/batch_shape_rejection_test.go | New: end-to-end tests for narrowing/requeue/health neutrality on shape refusals. |
| services/propagation/batch_bytes_test.go | New: tests proving chunking-by-bytes and oversize-tx “alone chunk” behavior. |
| services/propagation/batch_bytes_reaper_test.go | New: reaper rebroadcast path inherits byte chunking. |
| metrics/README.md | Document queries/recipes for chunking signals. |
| metrics/metrics.go | Add arcade_propagation_chunk_bytes histogram + clarify chunk_total semantics. |
| metrics/metrics_test.go | Ensure chunk-bytes histogram is registered/scrapable. |
| docs/teranode-error-surfacing.md | Document batch-shape rejection behavior and updated broadcast-mode caps. |
| config/config.go | Add byte cap config + exported defaults; update defaults wiring and comments. |
| config/config_test.go | Pin defaults and prove env overrides bind for both new/updated keys. |
| config.example.yaml | Document both propagation chunk cap keys and their rationale. |
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
What Changed
services/propagation/chunking.go(new):planChunksslices a batch into contiguous spans bounded by bothteranode_max_batch_sizetransactions and the newteranode_max_batch_bytespayload bytes. A single transaction larger than the byte cap still travels, alone in its own chunk, with a Warn carrying its size.broadcastInChunksnow calls it, so the dispatcher and the reaper rebroadcast path both inherit the cap.config/config.go: newpropagation.teranode_max_batch_bytes(default 16 MiB) and theteranode_max_batch_sizedefault lowered 1024 → 1000. Both defaults are exported (DefaultTeranodeMaxBatchSize/DefaultTeranodeMaxBatchBytes) and reused by the propagator's non-positive fallback, so code and shipped config cannot disagree. Comments state Teranode's real, strict rules.teranode/client.go:SubmitTransactionsreturns an error satisfyingerrors.Is(err, ErrBatchTooLarge)for a 413, or a 400 whose bare body is exactly one of Teranode's three limit early-exits (Invalid request body: too many transactions/too much data/too many submissions). Exact match, not prefix — the same handler answers a bare 400request context cancelled. The existingerrUnexpectedStatusCodewrapping is preserved.services/propagation/propagator.go: a shape-rejecting peer votes for nobody. Any transaction left without a vote setsneedsNarrowing(reusing the existingnarrowChunkhalving); a chunk of one requeues carrying the peer's text as its reason. A sibling's 200 stays sticky, so "peer A too-large, peer B 200" settles in one round trip.recordBroadcastOutcomestreats a shape rejection as neutral for the circuit breaker (today a mixed A-400/B-200 charged A the slow-track breaker, and an all-peer 400 reset counters via the unanimous-reject arm).Newwarns (does not clamp) when a configured cap reaches Teranode's limit, and the startup Info line prints both effective caps.parkExhaustedRequeues: the "parent not yet accepted by the network" wording is now reserved for a missing-parent line (mirroringgiveUpReason); any other retry reason is quoted verbatim. This was already wrong forSTORAGE_ERRORinfra lines.metrics/metrics.go:arcade_propagation_chunk_byteshistogram (dedicated 4 KiB–64 MiB buckets with resolution across 1–32 MiB),batch_byteson the endpoint success/failure logs, andarcade_propagation_chunk_total{fallback="size_rejected"}.config.example.yamldocuments both keys (it previously omittedteranode_max_batch_sizeentirely);docs/teranode-error-surfacing.mdgains a batch-shape paragraph and a response-level table row;metrics/README.mdgains a chunking recipe; the smoke harness records body bytes andassertChunkSizechecks both caps.Why It Was Necessary
Fixes #271.
broadcastInChunkssplit a batch by transaction count only. Teranode's/txshandler (every tag since v0.15.0) enforcesmaxTransactionsPerRequest = 1024andmaxDataPerRequest = 32 MiB, both checked with>=before each read — so the effective rules are strictly fewer than 1024 transactions and strictly under 32 MiB (arcade's shipped default of exactly 1024 was itself refused; production runs 25, so this was latent). On a trip Teranode answers a bare 400 text body after it has already dispatched every transaction it read, and discards the per-tx error list. Arcade could not parse that body, so the whole chunk classified "no per-tx vote → requeue" and was re-sent at the same size untilretry_max_attemptsparked it at PENDING_RETRY, then the reaper repeated it in 200-tx layers. Because an all-peer bare 400 counted as a unanimous reject, the loop was also invisible to endpoint health.Decisions: 16 MiB default is half of Teranode's hard ceiling and matches the Kafka producer cap from #330; local caps are inclusive and the margin lives in the defaults; Teranode stays the oracle for per-tx size policy (no locally invented REJECTED reason); the reactive path exists for peers running a lower
propagation_httpBodyLimitor a future upstream change.Testing Performed
CGO_ENABLED=1 go test ./...— 32 packages ok, 0 failures.go build ./...,go vet ./...,go vet -tags=smoke ./tests/smoke/,gofmt -lclean.golangci-lintv2.13.2 (run viago run …@v2.13.2, since the installed 2.12.2 binary predates the Go 1.27 target in go.mod) onservices/propagation,teranode,config,metrics: 0 issues.TestPlanChunks_Table/_Invariants(pure chunker incl. exact-fit, lone oversize, disabled caps, random contiguity/disjointness),TestProcessBatch_ChunksByBytes,TestProcessBatch_OversizeTxBroadcastAlone,TestNew_BatchCapDefaults,TestNew_WarnsWhenCapsReachTeranodeLimits,TestReapOnce_RebroadcastChunksByBytes,TestSubmitTransactions_413_IsBatchTooLarge/_400BatchLimitBodies_AreBatchTooLarge/_400OtherBodies_AreNotBatchTooLarge,TestNarrowChunk_SizeRejection_AllAcceptedWithinBound(all four rejection shapes, exactly 3 round trips),TestBroadcast_SizeRejection_ChunkOfOne_RequeuesWithReasonNeverRejects,TestBroadcast_SizeRejection_LosesToSiblingAcceptance,TestRecordBroadcastOutcomes_ShapeRejectIsNeutral,TestBatchShapeRejectedLog_HasSizeFields,TestPropagationBatchCapsBind,TestPropagationChunkBytesRegistered. The dormantbatchSizescapture inTestProcessBatch_ChunksOversizedBatchis now asserted.-tags=smoke, needs podman) and e2e; the smoke package compiles, vets and lints.Impact / Risk
16 MiB × endpoints × max_parallel_chunks × max_concurrent_batcheswhere it was unbounded before.ExtraInfofor non-parent reasons no longer claims a missing parent.unanimous_reject.teranode_max_batch_size: 1024explicitly now get a startup Warn (the value is not clamped; narrowing absorbs the refusal at the cost of two extra requests per full chunk). Production sets 25 and is unaffected; the deployment configmap should gainteranode_max_batch_bytesexplicitly.client_batch_limit_test.gowith a pointer to the upstream file.Follow-ups (not in this PR)
>=check refuses an exactly-at-limit body and discards the per-tx list on early exit.unplaceablenarrowing path still narrows unconditionally; the new size path narrows only when a transaction is unvoted. Unifying them changes tested behaviour and was left alone.Notifications