fix: bound domain timeouts, wire limits, and RPC stream delivery - #214
Open
smiggleworth wants to merge 12 commits into
Open
fix: bound domain timeouts, wire limits, and RPC stream delivery#214smiggleworth wants to merge 12 commits into
smiggleworth wants to merge 12 commits into
Conversation
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.
Summary
This branch grew well past its original scope. It now contains five largely independent
units; the last section suggests how to review (or split) them.
1. Session isolation — a slow domain no longer closes the connection
DeliveryError::Timeoutas a fatal route error and answeredIngressDecision::Close, so one saturated Queue actor tore down the whole multiplexedWebSocket along with unrelated KV/Stream/Schedule/RPC traffic. A domain that does not
answer in time now returns an error frame on that channel and the session survives.
Verified for all seven domains.
Timeout) from astopped one (
ActorStopped). RPC previously collapsed both intoActorStopped.src/runtime/reply_wait.rs.deadline cannot close an already-authenticated session.
count. The counter assumed compounding backoff, but the delay is worker-global and reset
whenever any other ticket succeeded, so under normal churn a ticket was abandoned in
~80 ms instead of the documented ~2.3 s, leaking its subscriptions and inflight leases.
New
fitz_session_cleanup_permanent_failures_total.send_unit_actor_commandreturns its outcome; callers previously could not tell acompleted command from one that timed out.
2. Wire limits — no response can panic the broker
encode_single_tlv_frameno longer asserts. A TLV value carries au16length, andasserting on it turned any aggregate-overflow bug in any domain into a broker panic. It
now returns
DeliveryError::InvalidPayload.limit = 0means "all remaining" and nothingbounded bytes, so 250 schedules with 1 KiB payloads produced a 270 KB response and
panicked the outbound sink. Both
list_entriesandlist_entries_v2are now bounded bya wire budget;
list_entries_v2capped entry count but never bytes.batch-create. A CREATE arrives as one TLV value, so its payload can be ~140 bytes larger
than the same definition costs as a list entry — accepting one left a schedule that fired
normally but could never be listed.
wildcard reserves, and a message deliverable by a concrete reserve is no longer
dead-lettered by a wildcard one (the budgets differed by the routing envelope).
record is charged the cheap
Filteredmarker rather than its full Event cost, so a bodythe client never receives can no longer stop or fail a page. A single record that alone
exceeds the ceiling still fails loudly with
ERR_READ_RESPONSE_TOO_LARGE(2013) ratherthan being skipped — Stream guarantees exact replay, so silently dropping a committed
event from a rebuilt aggregate is worse than a classifiable error.
3. RPC streaming — never drop a frame and continue the sequence
Under outbound backpressure the broker advanced the response sequence before delivery,
dropped the frame, and kept forwarding later ones, so callers saw sequence gaps
(
124 != 123,300 != 299) across a 100,000-frame / 6.1 GiB run while diagnosticsreported zero failures.
pending_for_response_in_familyno longer mutates on lookup — it neither advancesnext_expected_seqnor drops the request onstream_end.put so the worker can resend the same sequence; after
MAX_RESPONSE_DELIVERY_ATTEMPTSthe RPC terminates with an error to both caller and worker (the worker cancel is what
stops late-response amplification).
stream_endpreviously left the callerhanging until timeout, because the request was removed before the forward was attempted.
yield_nowcalls — microseconds, not waiting. Now 8yields then bounded escalating sleeps.
4. Telemetry — health can no longer read green through a fault
transport could not deliver was logged and counted as a success. Added
fitz_queue_response_route_failures_total.inline dispatch path and admission control incremented two others. Converged.
transport_pressurewas computed and used only in a hint string. It now drives a newTransportBackpressurediagnosis and counts towardfailure_count.and a JSON parse of an attacker-controlled payload for an unauthenticated peer.
5. Schedule delivery distribution
round_robin_cursorsis keyed per route, so a first-time route hitunwrap_or(0)and everyone-shot Single schedule chose the same subscriber, concentrating a fleet's load on one
client. Unseen routes now seed from the route hash.
Client-visible contract changes
schedule::ERR_BACKEND_ERROR(7010) — Schedule was the only domainwithout a generic backend code, and borrowing
ERR_PARSE_ERRORwould tell a client itscron was malformed when the broker was merely busy.
schedule::ERR_TIMEOUT(7011) —deliberately not retryable; see below.
already enqueued and may still execute, and only queue ACK is deduplicated — so an
automatic retry of a SEND double-enqueues.
REQ-PROTO-012classifies codes as retryableor fatal, with no third state for "outcome unknown", and
REQ-ERR-006pushes SDKs to aboolean
IsRetryablehelper that erases any prose caveat. Timeouts therefore use codesoutside the retryable set: 1009 / 4007 / 3005 / 2012 / 5006 / 6010 / 7011. Notably RPC
uses its backend code rather than
ERR_RPC_TIMEOUT(6001), which is documented retryable.DeliveryError::InvalidPayloadvariant.DeliveryErrorispuband re-exportedfrom
crate::runtime, so this is technically breaking for external consumers.states this, and clients will come to depend on it — it should be written down.
Dependency
cntryl-midgemoved from49442e6toe04ecb4(lockfile only;branch = "main"unchanged). Of the code commits in that range,
#255adds bounded late-responsediagnostics — request kind, configured timeout, abandonment age, response variant, pending
depth — which is what makes unmatched-response warnings attributable at all, and
#256bounds synchronous runtime waits.
Two consequences absorbed here:
MidgeError::Timeoutnow arrives where midge previously blocked indefinitely. The variantalready existed, so nothing forced handling it; KV classified midge errors by message text
and dropped it into a permanent
BackendError. Typed variants are now matched first.runtime_response_timeout(default 60 s, floored atstorage_io_timeout + 30 s)is set explicitly at Engine open, so the relationship between the two budgets is visible
in code. Fitz's 1 s domain deadlines sit far below it and always fire first — which is
only safe because a timeout is now a retryable-free error frame rather than a session
close.
Validation
cargo fmt --all -- --checkcargo test --workspace— 2,082 tests across 32 binaries, 0 failurescargo clippy --locked --workspace --all-targets --all-features -- -D warnings -D clippy::pedanticreproduced as
[0]where[0, 1]was expected; the cleanup ticket abandoned at 40 msagainst a 2.3 s window; the session close reproduced as
Close("route delivery failed: ... Delivery timed out")for all seven domains.REQ-PROTO-012retryable set ratherthan a hand-picked list, because an earlier hand-picked version missed both 6001 and 7010.
Known gaps
Stated plainly rather than left for a reviewer to find:
the headline fix and it does not exist.
benches/tier4_queue_concurrency.rsalready runs8 clients over real transports but is iteration-driven and discards results.
nonzero — the exact regression that produced the false green.
queue/actor/mod.rs,rpc/sink/state_model/constants.rs) rather than configurable, and are now load-bearing.outbound backoff schedule,
STORAGE_RUNTIME_RESPONSE_TIMEOUT, and the two 1 s replydeadlines. Together they decide how long a saturated caller gets to drain.
make a retried SEND safe. Dedup or cancellation on the enqueue path is the real cure.
ERR_QUEUE_FULL(4005) is still emitted by nothing while remaining documented as a queuecode. Pre-existing; this branch touched the area and left it.
response writer that advances the sequence before encoding succeeds). Separate repo.
Review guidance
82 files, +4,012 / −340 is too large to review well as one unit. The five sections above are
close to independently reviewable, and two are low-risk enough to split off and land first:
Section 1 deserves its own PR — it is the one with live protocol blast radius (a
behavior change plus the retry-taxonomy question above), and it is the one that should not
merge without the 60 s acceptance test.