feat(stats): hand transactions to the tracker over a channel, not a lock - #80
Merged
Conversation
The collector counts what the sender submitted. The inclusion tracker counts what became of those submissions. Neither holds both halves, so neither can say what fraction of offered work took effect. This adds the vocabulary and the ledger, and changes no behaviour. The tracker does not call it yet. Result names six terminal states. Two distinctions carry the point. Committed and Failed separate a transaction that did what the workload asked from one that burned its gas doing nothing, which an inclusion count cannot tell apart. Unknown separates "the run did not see" from "the chain did not take it", which decides whether a low goodput ratio is a finding about the chain or about the run. Failed names what a receipt reports rather than a cause. A receipt carries one status bit, and separating a revert from an out-of-gas needs a trace call per transaction that the per-block read budget forbids. RecordResult keys on the OperationKey the send path already labels its metrics with, and adds rather than overwrites. Callers reach it from more than one goroutine, because a block match and a reap sweep both report results. The result strings are a one-way door: a dashboard query and a saved report both carry them, so a test pins them. The strings are unchanged by the type's name. Every guard was checked by breaking what it covers. Overwriting instead of adding fails two tests. Folding Failed into Committed fails three. A drifted name fails the string test. Dropping the lock reports a data race. Requirements: TOT-001, TOT-004, TOT-005, TOT-006, TOT-016. Tasks T003 to T006. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The collector counts what the sender submitted. The inclusion tracker counts what became of those submissions. Neither holds both halves, so neither can say what fraction of offered work took effect. This adds the vocabulary and the ledger. Nothing reports an outcome yet. Outcome names six terminal states. Committed and Failed separate a transaction that did what the workload asked from one that burned its gas doing nothing. StatusUnavailable separates "the run did not see" from "the chain did not take it", which decides whether a low goodput ratio is a finding about the chain or about the run. The zero value is a sentinel, not a state. Committed at index 0 would mean an unassigned variable, a switch matching no case, or an early return counts as a commit, silently, which is the failure the type exists to remove. An unset or out-of-range value counts as Unrecorded instead: it has no legitimate producer, so a non-zero count means sei-load has a bug and nothing else explains it. The first one logs, once per run, because a systematic bug would otherwise write a line per transaction. A run never fails over a counting bug. recordOutcome now takes an Outcome rather than a string. The metric label was already fed by bare literals while Outcome.String() produced the same values, so one wire contract had two independent sources and the test pinned the one nothing used. A literal still compiles, so this makes re-splitting unnatural rather than impossible. status_unavailable rather than unknown: a reader seeing unknown beside expired cannot tell a chain finding from a measurement finding, which is the confusion the state exists to prevent. dropped_at_handoff stays, because both alternatives collided with the dispatcher's own load shed, which RunSummary.Dropped already counts and which means the transaction never reached the chain at all. stats/doc.go carries the type map, the three sentinel rules, and the three lock domains. Lifecycle and ownership are marked absent rather than invented, because both describe the tracker loop this change does not add. Guards proven by breaking what they cover: Committed back at index 0, the out-of-range value vanishing, a drifted frozen string. Requirements: TOT-001, TOT-004, TOT-005, TOT-006, TOT-016. Tasks T003 to T006. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tracker derived inclusion from the transaction hashes in a block, and a hash carries no execution status. One state therefore covered a transaction that committed and one that failed and burned its gas. A run could report a million accepted, near-perfect inclusion and a healthy p99 while every transaction failed, and nothing in the output said so. blockSource becomes receiptSource, backed by ethclient.BlockReceipts. One call per block either way, so the request count does not move: an earlier design fetched a receipt per transaction and its cost grew with the load the run offered, which is the constraint that shaped this one. blockReceipt is this package's own type rather than a go-ethereum receipt. A receipt carries eleven more fields the tracker has no business reading, and a test supplies a hash and a status without constructing one. matchBlock resolves each matched transaction to Committed or Failed and reports it, and the two existing outcome sites now route through the same reporter, so the metric and the collector stay in step. The tracker holds a collector. The reference runs one way: the tracker may take the collector's lock, the collector must never take the tracker's state lock. Nothing takes both, and the field comment is where that is written down. Reports land outside the registry lock. The sender blocks on it at every send completion, so work held under it lands in the latency this package reports. Guards proven by breaking what they cover: every receipt treated as committed, the operation label dropped, a per-transaction fetch reintroduced, reaped transactions no longer reported. Requirements: TOT-001, TOT-002, TOT-009, TOT-015, TOT-016, TOT-017. Tasks T007, T009, T010, T011, T012. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four independent reviewers read the receipts change. Every one of them found the same hole: OutcomeStatusUnavailable was defined, documented as the state that keeps a measurement problem from being reported as a chain problem, and had no producer. A receipt read that failed left its block's transactions to age out as expired, which is a claim about the chain that the run had no grounds to make. The registry now records a watermark. Each entry stamps the count of unreadable blocks at registration; a reap compares it against the count now. A higher count means a block that could have carried this transaction was never read, so the transaction reaches status_unavailable instead of expired. A transaction registered after the hole still expires normally. An empty array is one of those unreadable answers and used to arrive silently. A node that holds a block but has lost its receipt bodies returns an empty list with no error, so a whole block of transactions aged out with no log line and no metric. It now takes the same path as an error, and block_fetch_errors carries a reason so an operator reads the cause off a dashboard rather than the pod log. A receipt carrying a post-state root instead of a status says the transaction executed and does not say how it ended. Reading that as a failure would invent a chain result, so blockReceipt carries whether the status was there. Run proves the endpoint answers before the run starts. A node in validator or seed mode serves no EVM HTTP at all, and without the probe such a run completes, reports every transaction un-included and exits zero, which reads as a chain that accepted nothing. The tracker can now read receipts from a node other than the one it loads. receiptEndpoint defaults to Endpoints[0], so a single-node run is unchanged, and a run that names a second node keeps the read work off the box under load. That matters more than the request count: a receipts read costs the serving node work that grows with the block's transaction count. An endpoint decides what it puts in a receipts array. A null element would have panicked the head loop, and nothing recovers there, so the run would have died and lost every result it had gathered. sender/doc.go states the conservation identity over the terminal states rather than the older three-term one, and corrects the reorg boundary: first observation now fixes an execution status, not only a time. stats/doc.go gains the Lifecycle and Ownership sections it deferred to this change, and the partition claim it documented now has a test. Also: context threads through the report path instead of being dropped; recordOutcome becomes meterOutcome, which is what it does; the two metric descriptions that contradicted their own series are rewritten; the collector is required rather than nil-checked, since no caller passed nil; and the comments that narrated this change rather than stating the present are gone. Guards proven by breaking what they cover: the reap attribution, the registration watermark, the empty-array branch, the status-presence branch, the preflight, the nil guard, the cap-drop report, and a double report. Requirements: TOT-004, TOT-013, TOT-020, TOT-021. Tasks T008, T013. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…a read costs
Two decisions the review surfaced and could not make for itself.
An operator reading seiload_inclusion_outcome_total{outcome="failed"} beside
seiload_run_txs_failed_total sees one word for two layers of the same run. One
is a transaction the chain took and ran to no effect, the other is a send that
returned an error. reverted is what the EVM calls the first, and it does not
collide. succeeded was not available as the other half of the pair, because
OperationStats already carries Successes for the send path.
The label values are declared frozen, and this is the last moment the rename is
free: nothing in the platform repo binds them yet.
The second decision stands rather than changes the code. eth_getBlockReceipts
costs the node answering it work that grows with the square of the block's
transaction count, because it resolves each receipt's index by walking the
whole block and recovers every sender again while doing so. That is a property
of the node, not of this change, and every caller of the method pays it.
The run keeps that cost off the box it is loading by pointing the tracker at a
node that takes no send load, which is what receiptEndpoint is for. The cost
does not disappear: it bounds what the tracking node can keep up with. Both
config.ReceiptEndpoint and receiptSource now say so, and say to measure against
the target chain before turning receipt tracking on.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 2 found that the previous commit's own fix was worse than the bug it removed. Three lenses reached it separately and one measured it: 80 of 100 consecutive Sei mainnet blocks return an empty receipts array. Treating that as a hole marked almost every block. Because the marker only ever grows and a transaction inherits it for its whole time in flight, one idle block in a 30-second window converted the entire registry, and expired stopped being reachable at all. The case the tool exists to detect makes it worse rather than better: a chain that stopped accepting work produces nothing but idle blocks, so the run would have reported "I could not see" for the one run where "the chain took nothing" is the answer. The premise behind that branch was wrong. sei-chain already separates the three answers on the wire. A block it cannot see returns null, which arrives as ethereum.NotFound. Pruned receipts return an error. An empty array means the block carried no EVM transaction, and it is the truthful answer. So the branch is gone rather than made conditional, and an array of nothing but nulls, which is a node answering nothing at all, becomes an error where it is read. The registry counters now split the way the outcomes do. They reached the operator through the closing log line while the outcome ledger reached the same operator through the metric, so one transaction was expired on the surface read first and status_unavailable on the surface read second. The preflight failed on the one case a Sei node never produces and passed on every case it does. A node serving no EVM HTTP refuses the connection, which classified as other and let the run start blind, and that is the case the doc comment named first. A gateway that filters methods answers with an HTTP status carrying the JSON-RPC code in a body the decoder never reads. Both refuse the run now. Classification leads with the typed checks that hold across servers, and the two substring tests that cannot be typed are ordered so pruning is tested before availability, because Sei's two messages differ by one word. The abort message named --receipt-endpoint. There is no such flag: the setting is receiptEndpoint in the profile. The one error allowed to end a run told the operator to use a control that does not exist, and a test now fails on the flag spelling. Register takes the context its caller already holds. The comment saying the caller had none was false; the signature declined it. A second registration of a hash already in flight overwrote the first, so two accepted transactions shared one terminal state. It is counted now. The partition guard covered three of seven states and passed with the whole reap report loop deleted. It exercises every reachable state, asserts each leg, and checks that the registry counters and the outcome ledger describe the same transactions. sender/doc.go is the file stats/doc.go nominates as owning the conservation identity, and it still stated it with the retired word and with registered on the left where dropped_at_cap sits on the right. The two shorter restatements elsewhere are replaced by a pointer to it. HasStatus detects a post-state-root receipt and no other shape. go-ethereum makes both fields optional, so a receipt carrying neither is indistinguishable from a failure after decoding, and the comment says so rather than implying a guarantee. Also: narrow becomes a plain function, matching every other helper in the package; the cost claim names seid rather than the method, because upstream go-ethereum is linear and the quadratic belongs to the implementation being measured; the nolint directive naming a linter this repo does not run is gone while its explanation stays; the fallback to the load endpoint warns; and the README documents receiptEndpoint. Guards proven by breaking what they cover: the idle block, the split counters, the duplicate registration, the unreachable endpoint, and the all-null array. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Shipping receiptEndpoint is what makes this reachable, so it belongs with it. Two reviewers named the same sequence: heads arrive from the load node, the receipt node has not committed that height yet, and it answers null. That arrives as ethereum.NotFound, which counted as a hole with no retry, so a receipt node trailing by one block produced a hole every block and expired became unreachable again. The defect this PR just removed, reintroduced through the topology the PR recommends. A height the node has not reached is now re-read on the next head. One re-read is the bound: a node still behind a block interval later is behind rather than busy, and that is a hole worth counting. This is TOT-020 pulled forward from phase 3c, for the same reason the unreadable block attribution came forward from 3b. Leaving it out means merging a change whose recommended configuration breaks it. Guards proven by breaking what they cover: a lagging node written off on first sight, and a node behind forever retried forever. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…shutdown Round 3 reviewed the re-read that round 2's fix needed, and found four defects in it. Both lenses reached the first two independently. The re-read stamped the transaction with the arrival of the head that triggered it, not the head the block actually arrived on. That arrival becomes the inclusion latency sample, and in the two-node topology this tracker recommends every height is deferred at least once, so the error landed on every sample rather than a few. One block interval against a histogram whose first bucket is half a second. The pending queue carries its own arrival now. A height still waiting when the heads stopped was dropped, and its transactions reaped as expired: a claim about the chain for a block nothing ever read. That is the defect round 2 existed to remove, through a new path. The queue drains whatever ends the head loop. A skipped head was never counted either. sender/doc.go called that an undercount rather than a miscount, which was true when the tracker only counted inclusions and stopped being true when expired became a claim about the chain. Every height in a gap is counted now. The retry bookkeeping was a map that only deleted on the failure path, so a node that caught up left an entry per block for the life of the run. One queue carrying a try count replaces it, bounded in both directions, which also raises the budget past one block: a node two behind used to produce no inclusion data at all. The hardened classifier refused a healthy run three ways. A connection reset is what a busy node does to a caller and read as unreachable. A rate-limited response whose request id happened to contain those six digits read as the method being absent, because the body was searched without regard to the status. A bare 404 from an ingress mid-reconcile read as nothing listening. Refusing a healthy run is worse than the blind run the refusal exists to prevent, so unreachable now means a refused dial or a name that does not resolve, the body is read only under a status that means refusal, and the preflight tries three times before it speaks for the whole run. An endpoint that answers nothing at all across all three is also a refusal, which is what a dropped route looks like. The typed checks the last commit added had no test. Every one could be deleted with the suite still green, because the tests drove error strings the substring fallback caught anyway. They are covered now, by construction rather than by text. Two more from the same review. The empty-array premise was wrong a second time, in the other direction. sei-chain swallows a per-hash receipt lookup that comes back not-found and compacts the slot out, so an empty array can also mean the block's transactions existed and their receipts were gone. Restoring the branch is not the answer: reading an empty array as a hole is what made expired unreachable on 80% of real blocks. The head's gas comes from the consensus result rather than from any receipt, so gas burned with no receipt returned is the one witness that the two cases differ, and it is counted rather than acted on. Gas covers Cosmos transactions too, so treating it as a hole would invent one on any chain carrying non-EVM traffic. Any null element in the array is an error now, not only an array of nothing but nulls. The run cannot see what that element was going to say either way. A re-read gets a shorter budget than a first read. Head processing is serial and the node drops a subscription whose head buffer fills, which ends the run, so two full-length reads in one head cost more than that affords. The head channel is buffered for the same reason. Guards proven by breaking what they cover: the arrival stamp, the queue drain on shutdown, the skipped head, the duplicate leg on both surfaces, the queue draining after a read, a reset read as unreachable, and a rate-limit status allowed to speak for the method. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by reading my own round-3 fix rather than by review. Treating any null element as a failed read threw away the receipts the endpoint did send, so a transaction that committed and whose receipt arrived got nothing and later reaped as unattributable. That trades a known outcome for an unknown one. narrowReceipts reports how many elements were null instead of refusing the array, and receiptSource says so in its signature, because a read that partly succeeded is a real answer and the interface should be able to express it. The block is a hole for the transactions the missing part would have named and not for the ones it named. Guards proven by breaking what they cover, in both directions: discarding what arrived, and hiding the loss. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The worst defect in this series, and it was mine from the last commit. The preflight's refusal wrapped the probe's own error with %w, and main treats a context.Canceled or a context.DeadlineExceeded as the run ending normally. So a run refused for an endpoint that never answered exited zero: a Complete run that carried no load, which the nightly harness then reports as a chain that accepted work and included none of it. The exact false accusation this whole tracker exists to remove, reintroduced through the path meant to prevent it. Refusal now carries its own sentinel, and a test asserts that no refusal reads as the run's context ending. The rest of what round 4 found, in the order it matters. The two read budgets were backwards. In the two-node topology this tracker recommends, the first read of a height returns not-found cheaply and the re-read is the one that carries the receipts, so the short budget landed on the only read that mattered. Measured against pacific-1, two of five idle-block reads already exceeded three seconds. One budget for both. The deferral bound counted heads, which made the failure a cliff: at four heads of lag every block read, at five every block became a hole and expired went unreachable for the whole run. It is a duration now, which is the quantity that actually matters, and the wait is recorded as a histogram so the drift is visible before it is crossed rather than after. The preflight kept only the last attempt's verdict, so two timeouts could erase an earlier answer that proved the endpoint was there, and refuse the run on evidence contradicting its own message. A refusal now needs every attempt to agree on one cause. Any DNS error refused the run. A resolver answering SERVFAIL is temporary, and only a name that does not exist is permanent. An endpoint answering prose rather than JSON stopped being refused when I narrowed the classifier last round. That is the common operator typo: the metrics port, the Cosmos RPC port, an ingress default backend. It is as durable a failure as a refused dial, and it refuses again. The summary's terms overlapped. A receipt whose status could not be read counted in both included and status_unavailable, so adding up the closing log line gave more than the run accepted. included now means the readable ones, and the terms are disjoint. A gap logged one line per height, so a fifty-height gap pushed the run's own summary out of the fifty-line log tail that is the only diagnostic a failed nightly carries. One record per gap, which changes nothing about attribution because the reap only asks whether the count rose. The shutdown sweep marked holes it could not justify. The head loop and the reap loop end on the same signal, so no reap follows it and those transactions are already counted as in flight at shutdown. Marking a hole put a failure on the series that answers "was this run blind?" at the end of every healthy run. Four causes shared one reason label. A node behind the head, a head never seen, a height out of budget, and a receipt the node could not produce now have their own, because the operator's next move differs for each. One reviewer finding I did not take. It measured that Sei's block gas is EVM-only, concluded block_empty_with_gas is clean signal, and asked me to act on it. That measurement was of eth_getBlockByNumber, which sums receipt.GasUsed and would be circular here. The newHeads header this code reads sums every transaction's consensus result, Cosmos included, so the counter stays observed rather than acted on. The comment now names which header, and names sei-chain's own TODO to change it, because the ambiguity misled a careful reader. Guards proven by breaking what they cover: the refusal's sentinel, the non-JSON refusal, agreement across attempts, a resolver blip, the disjoint summary terms, one record per gap, and a healthy shutdown marking no hole. Method note: two mutations in this round reported as surviving when they were really vet failures, and one survived because the fake fell through to success. The battery checks vet and drives every attempt now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by reading my own last commit. Making the deferral bound a duration raised how many heights can be waiting at once from four to about a dozen in steady state, and the sweep gave each one a full read budget. Head processing is serial, so one hanging node could spend minutes inside a single head while the chain moved on. The sweep shares one budget now. A height it does not reach stays queued for the next head, which costs a head of delay rather than a hole, and the requeue puts the oldest first so nothing is starved. Guard proven by lifting the budget: the sweep then ran to the full length of every queued read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An idiom review of the file as it now stands, rather than of the last delta. Five rounds of defect-driven fixes is exactly how a file accumulates incoherence, and it had. The retry count was dead. Round 5 replaced a count-based retirement with a duration and left the field written, passed along, and never compared, plus four comments still describing the mechanism it belonged to. An editor tuning re-read depth would have changed a constant that moves nothing. Removing it made a second fix obvious. matchBlockAttempt and deferHeight took four and five positional parameters carrying most of deferredRead's fields, two of them adjacent same-typed timestamps. Transposing arrival and deferredAt compiles, and it would have measured the inclusion latency and the retry budget from each other's instant. They take the struct now, so a first read passes no hand-written zero values at all. The take-and-clear critical section was duplicated verbatim at two sites that must change together. takePending owns it again. blindFetches counted heads that never arrived as well as reads that failed, so neither its name nor its doc was true. It is blindHeights. deferred_read_wait labelled its values outcome, which inclusion_outcome already uses for a disjoint set. One label name meaning two things across two instruments is the wire hazard Outcome's own type exists to prevent, and the file states that rule thirty lines above where it broke it. The label is disposition, and its three values have constants. requeue dropped a height past the cap silently. deferHeight already holds the queue at the cap so nothing reaches that branch, but a height vanishing from it would leave blindHeights unmoved and let a transaction from that block reap as a verdict about the chain. It records instead. block_gaps described the arithmetic of a different counter: it adds one per missed height, while the once-per-gap record lands on block_fetch_errors. Comment discipline, with the line drawn where the reviewer drew it. A present fact about the deployment shape stays, because it is the constraint that makes a shorter re-read budget wrong. The argument with the version that had it backwards goes, because that is the commit's job and not the code's. Same treatment for the conservation identity's history and for the empty-array incident. Two blocks that explain a non-obvious external API stay untouched. Also: flushDeferred promised a flush and performed an abandon, so it is recordUnreadAtShutdown, and the call site that still argued the behaviour it no longer has is gone. refusesTheRun reads as though the reason refuses; it is isPermanent. reasonNotJSON sat under a comment saying it was not a call failure. The DNS rationale had been appended to the paragraph about connection resets. Two nolint directives named a linter this repo does not enable, and their reasoning survives as plain comments. stats/doc.go gains the fourth sentinel, the queue's bounds under the lock it lives behind, the lifecycle branch where a preflight refuses a run, and the two tests that guard the queue. sender/doc.go gains the sweep bound and the shutdown boundary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The commit titled "bound what one head spends re-reading" did not. The check ran before a read and never bounded the read in progress, so a read starting at 2.999s ran a further ten. Measured 12.8s against a stated 3s, and a worst case of 23s where the shape it replaced was 18s. The bound moved the wrong way in the commit named for fixing it. That is not only a cost. The head loop stamps arrival at dequeue, and that value becomes the inclusion latency sample, so a sweep that overruns writes the tracker's own backlog into the number the run exists to report. A re-read now gets whatever is left of the sweep. Which exposed the next thing: a read this process cut short is not evidence about the node, so that height goes back in the queue rather than being called a hole. Blaming the serving node for a deadline sei-load imposed on itself is the same error as blaming the chain for a block the run never read. requeue put the unreached tail in front of what the sweep had already re-deferred and its comment claimed the opposite. A sweep walks oldest first, so what it re-deferred is older; prepending served the newest first and let the oldest age out against a budget they were never given a turn under. The refusal reasons were two lists that had to agree: one deciding whether a reason ends the run, one turning it into a message. Adding a reason to one and not the other gave either a silent stall through the retry loop or a refusal nothing could reach, with no signal from the compiler or a test. One table now. block_fetch_errors counted a gap once while every other reason counted per height, so an operator summing it undercounted by the length of every gap. The watermark still rises once, because the reap only asks whether it rose. Six guards were missing and one was worse than missing: deleting requeue outright left the suite green while heights vanished, because the assertion only asked whether the queue was non-empty and the heights the sweep did read had refilled it. The whole duration-budget mechanism had no test at all, so it could be disabled, set to five hundred hours, or restarted on every read without a failure. Each now has a guard, proven by breaking what it covers. Also: the latency histogram's count is no longer the included count, since a matched receipt with an unreadable status still samples, and the comment said otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The strongest finding of the review, and the one every earlier round walked past. It was measured: 565 of 1000 transactions reported expired on a chain that included every one of them, with every hole counter reading zero. No read fails. No head is missed. No receipt is null. The tracker's head loop is serial, so when reads take longer than the block interval a backlog builds, and the block carrying a transaction is opened after the reap already evicted it. Five rounds hardened every path where a read fails; this is the path where every read succeeds and the tracker is late. sender/doc.go says expired is a claim about the chain and the run has no grounds for one about a block it did not read. At reap time the run had that block in hand and had not opened it. The registry now knows the highest head taken off the wire and the highest whose block has been read. A reap that finds them apart cannot say expired, because the transaction may be sitting in a height the run is holding. A caught-up run still says expired, which is the point of keeping the two states apart, and that direction has its own test. The same backlog corrupted the number this tool exists to report. Arrival was stamped after the head came off the channel, so every latency sample carried the queue. Measured at 8.2 seconds of error after 16 seconds of chain at 1.5x block time, and 43 seconds at 3.75x, against a histogram whose top bucket is 120. The stamp is taken where the head arrives now, by a step that exists to keep it there, and head_lag reports the gap. That is the number that separates a chain taking nothing from a run that could not keep up: both show un-included transactions, and only this one says which. A head stream that ends mid-run no longer fails the run. Any error from the tracker cancelled the whole scope and exited non-zero, so a dropped WebSocket on a read-only observer turned a good run red and killed the senders, the generator and the report with it. That is the mirror of the refused-run-exits-zero defect fixed earlier: this one is the false fail. Tracking stops, every later height is unread so nothing after it reaps as a chain verdict, and the run finishes. Also: the WebSocket client was never closed. Guards proven by breaking what they cover: the backlog rule in both directions, a dead head stream, a head never counted as received, a head counted only after its block was read, and a stamp taken late. The stamping needed the pump extracted to be testable at all. A guard that drives processHead directly cannot see where Run takes its timestamp, and that is the third time in this series a guard has named something it did not check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three correctness findings, all mine from the last two commits, all measured by the reviewer and reproduced here. The sweep budget was three seconds and one read's budget was ten, so capping a re-read at whatever the sweep had left capped every re-read at three. A node answering in four seconds, well inside its own budget, never finished one: cut short, requeued, cut short again, then retired as though the node were behind. In the topology where every height is deferred that is every height in the run, and expired becomes unreachable. A sweep budget is now not smaller than a read's, and a test pins that relationship rather than the two numbers. The same class one layer up, found by a test I wrote for something else. The wait budget was shorter than a sweep, so a sweep could outlive it and retire its own tail: heights called receipt_node_behind for time this process spent on the heights ahead of them. The wait budget is now twice the sweep, and that is pinned too. Ordering the requeue by age starved the queue. A sweep walks front to back, so what it re-deferred got a turn and the tail did not, and putting the tail behind them meant the same front entries consumed every sweep. Measured: three queued heights, a node holding only the third, and the third retired without the run ever completing a read of it. Order here is service fairness, and the earlier change to age order was a mistake I took from a review without testing what it cost. A reap could still claim the chain while a height sat queued. The previous commit compared heads received against heads read, which a queued height passes: it was received and read once, the node did not have it, and the run is waiting to ask again. A transaction may be in it. Two guards were vacuous. One asserted a reap outcome on a tracker whose reap window was a minute, so nothing was ever old enough to reap; making it real showed the defect above. One asserted a sweep's bound with a source whose delay scaled with the constant under test, so shrinking that constant kept the test green. The shutdown sweep now raises the attribution watermark without emitting a failure. Those heights genuinely went unread, so nothing reaped afterwards may speak for the chain whatever order shutdown runs in; but a healthy run draining a queue it was always going to drain is not the run going blind, and the operator-facing counter should not say it was. Also: a single missed head went unrecorded, since the gap tests used two and forty-nine. And the guarantee that the first entry of a sweep gets a full read was dead code once the budgets were ordered correctly. Eleven guards proven by breaking what they cover. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A traceability review, the first anyone has run on this work, and it found what six rounds of correctness review could not: the code is careful and it does not match its specification. TOT-022 is a MUST. The run takes its head signal from the same node it reads status from, because a head carries the raw committed height while a status read resolves through a watermark behind it. The two disagree even on one node, and taking them from separate nodes adds peer lag on top, which a node inside its readiness threshold carries for minutes without reporting unhealthy. The requirement's own verification row names the failure as "the head signal and the status read come from different nodes". That is what this code did, with a comment justifying it on cost grounds, answering a question the requirement did not ask. The cost of that violation is most of the machinery built since. The deferred read queue exists because heights arrived before the reading node held them, which is the condition TOT-022 forbids creating. It stays for now, because one node still disagrees with itself across the watermark, but it should be rare rather than constant. And it had made expired unreachable a second time. The reap refuses a chain verdict while a height sits queued, which is right, and it is only safe because the queue is normally empty. Split the nodes and the queue never empties, so a run against a healthy chain that took nothing reports "I could not see". That is the defect of two rounds ago, reintroduced through the topology this change recommended. A test now drives the single-node steady state and asserts expired is reachable. TOT-023 says the run must not read a block older than the deadline it gives a transaction to reach one. The read budget was a fixed twenty seconds while the deadline is operator-configurable, so a run reaping at five seconds re-read heights four times past its own bound. The budget answers to the deadline now. The traceability mechanism was absent rather than incomplete. One requirement ID appeared in the whole test suite, in a comment, from the previous PR, while the repo already does this properly for another feature. Forty-two guards now name what they cover. Eight requirements are cited by nothing, and that is the point of doing it: TOT-005, 007, 012 and 019 are the report, TOT-010, 014 and 018 are the hand-off channel, and all seven belong to later phases. TOT-013 is contradicted rather than deferred, and it is called out below. Also: sender/doc.go stated the identity with six terms where the data model has seven; the README advertised a report carrying committed and reverted, which it does not; and the duplicate-registration collision is filed under status_unavailable, which is the closest state the spec defines and not a clean fit, now said out loud where it happens. Open against the spec and not fixed here: the preflight refuses a run, and the spec's design section says nothing in this feature fails a run. Refusing is probably better than a silently blind run, but it is an unrecorded amendment, and it makes TOT-013 and SC-008 unreachable by construction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Roughly 750 lines out, 110 in. The tracker drops from 1219 to 884 lines, its tests from 1224 to 784, and the suite from 20 seconds to 11. The deferred-read queue, its sweep budget, its requeue, its starvation rule, its three interacting constants, its shutdown drain and its wait budget derived from reapAfter all existed for one reason: heads arrived from a node that was not the one being read, so a height was routinely announced before the reader held it. Fixing that violation removed the cause and left the apparatus. The cause is not entirely gone, and that is why a retry stays. seid publishes a head from Commit while the receipt store's writer is still asynchronous, so a height answers null for the gap between the two. Our nodes run the pebbledb receipt store with an async write buffer of a hundred, set in the fleet's own defaults, so the window is real and bounded by that queue rather than by the chain. It is one write, not one block, so waiting in place resolves it in milliseconds where the queue waited for the next head. Deleting the retry as well was the tempting move and it is wrong. A hole raises a watermark that only grows, so one hole anywhere inside a transaction's reap window converts it. At a null rate of five percent, every transaction in a run reports status_unavailable and expired never fires: the tool loses the one verdict it exists to deliver, and a run that can never say the chain dropped anything is not a load test. The preflight loses its refusal table and most of its classifier. The probe asks for height 0, which sei-chain answers from a constant ahead of its watermark and its receipt store, so a healthy EVM RPC cannot fail it and the reason for a failure cannot change the verdict. That also closes a hole the larger version had: an endpoint answering not-found to genesis is not a Sei EVM RPC at all, and it used to be admitted. deferred_read_wait becomes block_read_wait and loses its disposition label. It now measures one thing, which is how far a node's watermark trails its own head, and that is the number nobody has and the one that says whether even the retry earns its place. Nothing in the platform repo reads either name. Fourteen tests lost their subject with the code they covered. Three guards replace them: the gap is waited out rather than written off, the retry gives up rather than holding the head loop, and expired stays reachable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d it
Run had no coverage. Every ordering fact seven rounds of fixes depend on — the
pump stamping a head's arrival, the head loop reading its block, the reap
deciding what to call a transaction, a dead subscription ending the tracking —
was exercised only by helpers called by hand. That is why the two worst defects
in this change survived seven rounds of review, and why four of the guards
written along the way turned out to assert nothing.
The harness is an in-process WebSocket server speaking eth_subscribe("newHeads"),
so a test drives the real subscription, the real client and the real loop. It
earned its keep immediately: the first version sent a header the go-ethereum
decoder rejects, and the run reported the subscription dying rather than the
header being wrong. Coverage of Run goes from nothing to 77%.
The first defect it exposes fires on every run. The senders and the tracker start
together, and the tracker dials and probes before it reads a block, so every run
accepts transactions during a window in which it is observing nothing. Those
transactions land in blocks the run never opened, and reaped as expired: a claim
about the chain drawn from a period the run did not watch. The registry records
when the run first read a block, and a reap will not speak for the chain about
anything accepted before that.
The second is what a dead subscription does. The reap loop ends with the head
loop, so nothing reaps afterwards, and the registry kept filling from senders
that were still working until everything reported dropped_at_cap. An operator
reads that as a cap to raise rather than a subscription that died. Tracking
stopping now settles everything in flight, and a later registration is answered
rather than stored.
That made the reap's own trackingStopped arm unreachable, which two reviews had
already called dead for a different reason. It is gone; Register is the only
reader now, and the field says so.
The registry's conservation identity omitted status_unavailable, so it could not
see a transaction migrating into that term. It has five terms now.
Guards proven by breaking what they cover: the first-read watermark and its
placement, the drain, the registration path after tracking stops, and the reap
arms in both directions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round eight found that both could be disabled with the suite green, and that is the over-reporting direction: every un-matched transaction in every run reports status_unavailable, and nothing says so. One transaction cannot catch it. The first-read watermark and marking a head resolved both move a single transaction's verdict the same way, so a test with one subject cannot tell a working watermark from a dead one. The guard drives Run with two: one accepted before the run read anything, which must not be blamed on the chain, and one accepted after and never included, which must be. A third covers a subscription that is up and has delivered no head, where every other watermark reads as healthy because the run is trivially caught up with the nothing it has seen. The drain reached the metric ledger and not the closing log line, and no test compared them. That is one transaction counted under two names, which the reap path has been guarded against for several rounds and the drain path had not. An ingress error page was retried three hundred and eighty-five times over ten seconds. rpc.HTTPError renders as "404 Not Found: 404 page not found", so the substring check read it as a height the node had not reached yet. Deleting the classifier's typed checks left that substring first to match. The typed checks are back above it, and a table pins which reasons the read retries: only the one that means the node will answer differently in a moment. The package doc still described the deleted queue, named four tests that no longer exist, and said two goroutines drive the run where there are three. It also claimed everything in flight at shutdown is inflight_at_shutdown, which stopped being true when a dead stream started settling them. Guards proven by breaking what they cover: the watermark unset, its arm removed, heads never resolved, the drain's counter, and the error page. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eipts-replace-hashes
TOT-023. A transaction past reapAfter counts as expired whatever a later block says, so reading that block answers a question the run has closed and can only reopen a settled outcome. Each height's read now gets what is left of its own deadline, capped by readTimeout, and a height with nothing left costs no request. Budgeting the remainder rather than a fresh window is what makes the oldest height the run ever asks for exactly reapAfter: a window measured from now would let a read starting just inside the deadline finish outside it, and the retention floor this puts under the tracking node would then move with a constant this file owns. It also drains a backlog instead of deepening one. A tracker falling behind gives each later head a smaller budget, and once a head has none left it costs no request at all. readTimeout stops being the bound. Its own comment said it "should stay below the run's reap deadline", which held at the 30s default and not at --inclusion-reap-after=5s. An assumption a flag can break is not a bound. Test fixtures move off a fixed Unix second, because matchBlock now measures a head's age against the real clock and a head stamped in 1970 is one the tracker is decades behind on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The run report named no outcome at all. Every count existed in the collector and none of it reached the operator, so a run whose transactions all reverted printed the same send rate and the same latency distribution as a run that did everything it claimed. The new section prints three layers and never totals across them. Committed and reverted are what the chain did. Expired and the two drop counts are what this run did to its own transactions. StatusUnavailable and InflightAtShutdown are what it did not observe (TOT-012, TOT-019). Three arms, and the difference between two of them is the point. A run that tried to measure and failed prints its zeros under a warning, because an absent number reads as zero (TOT-013). A run with --track-receipts off prints no counts at all, because "committed 0" for a run that never asked manufactures a chain finding out of a flag setting. The counts come from the collector, not from InclusionSummary.Included, which adds committed and reverted together. That conflation is the defect this feature removes. main reads the inclusion summary before printing rather than after. The report could not name an outcome while it ran first. The old closing log line goes with it: it printed the conflated included= and would now contradict the report on the same stdout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every guard broken on purpose first: an all-expired run mislabelled as a measurement failure, an untracked run printing counts, the revert share taken over accepted rather than executed, and the status_unavailable row dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
readTimeout stopped being the bound in the commit before this one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lt-1078-outcome-report
TOT-005 through TOT-008. Reporting one ratio hides whichever layer it divides away, so the run states both: committed over every send it attempted, and committed over the sends an endpoint accepted. The first answers what the profile asked for. A run whose endpoint refuses half shows a halved number rather than a healthy one measured over the survivors. The second isolates chain execution, which is the number to read when the RPC layer is not under test. The gap between them is the rejection share, so the report names that too. An indeterminate status counts in the denominator and never in the numerator. Counting it in the numerator reports a success nobody saw; dropping it from the denominator reports a healthy ratio over the survivors, which is the defect this feature removes one layer up. Every operation carries its own ledger. The run-level total cannot separate a revert rate spread evenly across operations from one concentrated in a single call, and those point at different things: the chain, or the workload. A run reporting 46% goodput where one operation sits at 0% and another at 98% now says so. It prints as a second line under each operation rather than widening the first, so anything parsing the existing format keeps working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TOT-010, TOT-014, TOT-018. The sender called Register on the goroutine that had just completed a send, and Register took the registry lock. The reap loop holds that same lock while it walks every in-flight transaction, so a sweep landed in the latency this package reports. Submit is a non-blocking send into a channel. A dedicated goroutine drains it and does the admission. A full channel drops the transaction and counts the drop: a hand-off that blocks brings back the stall this removes, and one that drops in silence leaves an accepted transaction with no terminal state. The drain performs no chain read, which is what makes the depth derivable rather than guessed. A drain sharing the head loop would inherit that loop's block read, and no depth absorbs a multi-second stall at a few thousand transactions per second. Admission alone waits only as long as the registry lock is held. The depth follows from the configured send rate at one second of headroom, with a floor for a profile that sets no rate. That is four orders of magnitude above what the drain actually stalls for, and shallow enough that a drain which cannot keep up says so through the drop count rather than hiding in a deep queue. Tests call admit directly where they want deterministic admission, which is what they were written to check. The hand-off has its own tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR SummaryMedium Risk Overview Senders call Shutdown / dead head stream: closing admission routes late handoffs to Extensive Reviewed by Cursor Bugbot for commit a23da27. Bugbot is set up for automated code reviews on this repo. Configure here. |
golangci-lint runs staticcheck with the QF checks on, which bare staticcheck does not, so these three only appeared under make lint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…come-report # Conflicts: # config/settings.go # config/settings_reap_test.go # stats/inclusion_outcome_test.go # stats/inclusion_tracker.go
…0-handoff-channel
Two reviews disagreed about the skip, and both were right about their half. Skipping at reapAfter discards receipts for a transaction the reaper has not swept yet, turning a committed into status_unavailable. Reading out to twice that reads blocks nothing is waiting on, which TOT-023 forbids. The deadline was never the right test. A block whose head arrived at some moment can only carry transactions registered before it, so the exact question is whether any live entry is that old. Asking the registry answers it without depending on the reaper's tick, so neither failure is reachable and the comment is true without qualification. evictionBound goes with it, and the retention floor is reapAfter again rather than double. The skip warning names the bound that actually fires. The scan runs only once a height is already past its deadline, which happens only while the tracker is behind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…0-handoff-channel
Register becomes admit here, and the test wants deterministic admission. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…esh window The gate added a moment ago handed a past-deadline read the full readTimeout, measured from now. That is the thing readBudget was changed to stop: the read outlives the deadline it belongs to, and a backlog pays the ceiling per height rather than draining. staleReadBudget measures the remainder the same way readBudget does, floored so the attempt is worth making. The test for that gate never reached it. It stamped the head two seconds in the future, so the budget stayed positive and the ordinary path ran. Both times are in the past and ordered now, with an assertion that the budget really is under the floor before the read, so the branch cannot go unexercised again. The mutation pass missed it for the same reason: removing the whole gate broke the ordinary path too, which is what the test was actually covering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An idiom review found eight, and the doc-attribution ones are real defects rather than style: godoc gave skipPastDeadline's comment to the function above it and left skipPastDeadline undocumented, and placed carried unaccounted's. Both verified with go doc rather than by reading. readBudget still claimed the oldest height the run asks for is exactly reapAfter. matchBlock reopens a height past its deadline while something waits on it, so the bound is near twice that. The claim names an operational contract an operator sizes a node against, so a stale one is worse than none. The cross-package assertion moves to the toolchain's own static-assert form. The array-length form ties its failure to the constants' magnitude and the target word size as well as to the ordering: verified that it overflows int on a 386 build at a thirty-second floor, where the conversion form does not, and that the conversion form still fails on an inversion. MinInclusionReapAfter's first sentence called it the shortest accepted deadline while Validate and its test both exclude it. It is the longest rejected one. Three comments narrated edits rather than stating the present, one of them in the first person. ExecutionOutcomes was missing from the package doc's type list, and its own documented invariant, that Tracked is derived and never set by hand, had nothing holding it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…0-handoff-channel # Conflicts: # stats/inclusion_outcome_test.go
… one try Two defects in the past-deadline skip, one of them mine and one from review. skipPastDeadline incremented blindHeights. A hole says the run could not see a height, and reap files every transaction in flight across one as status_unavailable. But the gate above it has already proved nothing live could be in that block, so the only transactions the hole reached were ones that provably could not have been in it. Measured: a transaction registered after the skipped head reported status_unavailable when the run had watched it correctly the whole time. It reports expired now. staleReadBudget measured a remainder that is provably already spent. The branch runs only once the head's own deadline has passed, and the transaction waiting on it is older still, so the max always won and every reopened height got exactly minReadBudget. The code says that value directly, with the reason a remainder cannot help. The gate stays. Removing it was measured too: a transaction registered before the skipped head then reports expired for a block that provably contained it, which is a false claim about the chain and worse than what it replaces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…0-handoff-channel
Register becomes admit here, and the test wants deterministic admission. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ne match Three from review, and the first is a real defect. The reopen branch fell through to clearPastDeadline, which logs that the tracker caught up and clears the flag. A reopened height is past its deadline by construction, so that states the opposite of what happened. Because the flag is edge-triggered it also re-armed the warning, so a sustained backlog alternated between the two every sweep: entries age past the deadline between sweeps, a head reopens and clears, the sweep drains them, and the next head warns again. oldestLiveBefore computed a minimum nobody read. The caller wants the answer, not the time, so it returns on the first match and is named for the question it asks. It runs on the head loop holding the registry lock, which Register and reap both wait on, and the registry holds tens of thousands of entries at load, so the full scan was work the question does not need. One paragraph in matchBlock survived the rewrite above it and still said the skip records a hole, which is the opposite of what skipPastDeadline now documents about itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…0-handoff-channel
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dput-ratios # Conflicts: # stats/execution_outcomes.go # stats/execution_outcomes_test.go # stats/logger.go
…0-handoff-channel
…doff-channel # Conflicts: # stats/execution_outcomes.go # stats/execution_outcomes_test.go # stats/logger.go
…eans Four from review. The run-level DroppedAtHandoff overwrote the per-operation sum with the tracker's tally, on a comment claiming the tracker is the only place holding it. Submit's drop leg calls report, so the collector holds it too. The two move in lockstep, which is why overwriting is the loss: a future divergence gets absorbed instead of surfacing through the residual. Resolved by taking main's side of the merge, which never had the overwrite. inclusionState.droppedAtHandoff was declared, read once, and never written. Two homes for one count under a plus sign reads as deliberate, and the next contributor to increment it would do so under the registry lock, which is the stall this change removes. Summary added the queue depth to its own tally and nowhere else, so the report ledger was short by the buffer depth and the residual read the gap as a counting bug. Those transactions reached no terminal state anywhere, which is what unaccounted means, so the residual is where they belong. StatusUnavailable now carries hand-offs that were never in flight and never had a receipt read, but its doc and the closing log line still said a receipt read failed. On a mid-run head-stream death that names the receipt endpoint for a dead subscription, which is the wrong-cause substitution Submit's own closed branch exists to avoid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
All four addressed. The overwrite. You are right that my justification was factually wrong — The dead field. Confirmed: The ledger split. The wording. Taken, including the log line. On a mid-run head-stream death the count is dominated by hand-offs, so
Merge conflict cleared too. Main had #79 and #82 since this branched, and this time main was the newer side rather than the branch, so I took main throughout and verified every
|
bdchatham
enabled auto-merge (squash)
September 1, 2026 22:39
Contributor
Author
|
@seidroid review |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
All four unresolved findings from the previous review are genuinely fixed: the logger.go overwrite is gone, Summary no longer counts the queue depth into the tracker-only tally, the dead inclusionState.droppedAtHandoff field is removed, and both the StatusUnavailable doc and the closing log line now name the wider cause. What remains is doc/code drift: closeAdmission's own comment still claims Summary counts the buffer, Submit's "never blocks" still overstates what the fallback legs do, and the hand-off depth's derivation reads backwards for the default unthrottled profile.
Findings: 0 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
RunSummary(stats/run_summary.go) still has noDroppedAtHandofffield, somain.go:436-440populates every other term of the conservation identity fromInclusionSummaryand omits the one this PR makes producible; itsStatusUnavailabledoc (line ~40) also still says "registered txs that were in flight while a receipt read failed", which is no longer the whole story now that post-drain hand-offs land there. Onlyinflight_at_shutdownis emitted as a gauge today so no metric is wrong yet, but the struct that mirrors the identity no longer closes and the gap surfaces when the remaining gauges land (TODO(PLT-467), stats/run_summary.go:33). Codex flags the same thing. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| // The order matters. Closing first means a sender racing this call takes the | ||
| // unavailable path instead of adding to a queue nobody will read. Settling | ||
| // second empties what was already there. A send that wins the race anyway is | ||
| // still accounted for: Summary counts whatever remains in the buffer, so the |
There was a problem hiding this comment.
[suggestion] This doc no longer matches Summary. The + queued term was removed this revision (line 1240 is now s.statusUnavailable + s.duplicates + t.handoffUnavailable.Load()), and TestQueuedAtShutdownReadsAsUnaccounted asserts exactly the opposite of what the comment claims: two transactions sitting in submit, Summary().StatusUnavailable == 0. So a sender that wins the race described here is not accounted for and the identity does not close — it surfaces as a non-zero unaccounted() residual, which execution_outcomes.go:114 calls "the only figure in the report that catches a counting bug".
Dropping the tracker-only increment was the right call (it was the ledger split flagged last round), but the comment should now say what the code does: the residual is where such a transaction shows up, deliberately.
The race itself is also closable rather than documented away — a re-check after the successful enqueue would do it:
case t.submit <- tx:
select {
case <-t.admissionClosed:
t.closeAdmission(ctx) // idempotent; re-drains what we may have just added
default:
}closeOnce already makes closeAdmission safe to call from Submit, and the second drain pass costs nothing on the happy path because the outer select never fires. Codex rates this a blocker; the window is narrow and the outcome is an honestly-labelled residual rather than a wrong number, so a suggestion seems right — but the comment asserting the identity closes should not stay.
| // Submit hands ownership of tx's InclusionTime to the tracker. Call it only for | ||
| // successful sends, at send-completion, so registered is a subset of succeeded. | ||
| // | ||
| // It never blocks. The sender calls this on the goroutine that just completed a |
There was a problem hiding this comment.
[suggestion] Raised as a file-level note last round and still present, and Codex flags it too: "It never blocks" is true of the registry lock but not of the collector lock. Both fallback legs call t.report (lines 319 and 328), which takes c.mu via Collector.RecordOutcome (line 889-893). The admissionClosed leg is the one that matters — after the head stream dies it fires for every send for the rest of the run, not once.
The code is defensible as-is: the send path already takes the collector mutex unconditionally on every send (sender/eth_client.go:152 → RecordTransaction), so this is not the stall the PR set out to remove. It is the wording that overpromises. Neither TestSubmitDoesNotBlockWhenTheQueueIsFull nor TestSubmitDoesNotBlockBehindTheRegistryLock holds the collector lock, so nothing pins the claim either. "It never waits on the registry lock" would be accurate and would still say what the hand-off exists for.
| // small enough that a drain which genuinely cannot keep up says so through the | ||
| // drop count instead of hiding in a deep queue. | ||
| // | ||
| // The floor covers a profile with no rate set, where TPS arrives as zero. |
There was a problem hiding this comment.
[suggestion] The floor's rationale reads backwards for the default profile. TPS: 0.0 is the config default (config/settings.go:133), and main.go:254 treats zero as no rate limiter at all — an unthrottled run, i.e. the highest send rate the process can produce. So "a profile with no rate set" is not a low-rate fallback; it is precisely the case where one second of send is largest, and it gets the smallest queue (1024) plus a depth that is a constant rather than something derived, which is what the function doc says it exists to avoid.
In practice 1024 is probably ample — admission stalls only for the registry lock — but two things follow. TestTheHandoffDepthFollowsTheSendRate documents handoffDepth(0) == 1024 as "a profile with no rate set", which reinforces the wrong reading, and an unthrottled run is exactly where dropped_at_handoff first fires outside a deliberate overload test, which is the stated un-defer condition for D-2. Worth either deriving the unthrottled floor from something real (inclusionRegistryCap already reaches for maxInFlight * 4 when TPS is zero) or saying plainly that zero means unthrottled and 1024 is a judgement call for that case.
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
1 participant
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.
TOT-010, TOT-014, TOT-018. Seventh in the outcome-tracking stack. Closes User Story 3.
The stall
The sender called
Registeron the goroutine that had just completed a send, andRegistertook the registry lock. The reap loop holds that same lock while it walks every in-flight transaction. So a reap sweep landed directly in the latency this package reports, and the measurement slowed the thing it measures.The hand-off
Submitis a non-blocking send into a channel. A dedicated goroutine drains it and does the admission.A full channel drops and counts. Both halves are required: a hand-off that blocks brings back the stall this removes, and one that drops in silence leaves an accepted transaction with no terminal state, which breaks the conservation identity.
Why the drain does no chain read
This is the requirement that makes the depth derivable rather than guessed (TOT-018).
A drain sharing the head loop inherits that loop's block read. At a few thousand transactions per second no depth absorbs a multi-second stall, so the number would be a guess defended by nothing. A drain that only admits waits as long as the registry lock is held, which a reap sweep bounds at microseconds.
The depth then follows from the configured send rate: one second of headroom, floored for a profile that sets no rate. That is four orders of magnitude above what the drain actually stalls for, and shallow enough that a drain which genuinely cannot keep up says so through
dropped_at_handoffrather than hiding in a deep queue.dropped_at_handoffstops being the count with no producer that #78 flagged.D-1 and D-2
D-1, the queue depth, is answered here: the clarification said the depth follows once the drain's goroutine is settled, and TOT-018 settles it.
D-2, whether a drop voids the run, stays deferred. Its own condition is un-defer when a run first drops a hand-off outside a deliberate overload test, and that has not happened.
Tests
Existing tests call
admitdirectly where they want deterministic admission, which is what they were written to check. Assertions are unchanged. The hand-off gets its own tests, including a conservation check that drivesSubmitand the drain concurrently against a deliberately shallow channel.gofmt,go vet, full suite clean.-raceclean onstatsandsender.