Skip to content

data plane: keep attached streams through revocation check outages - #795

Merged
NancyKwongPerplexity merged 12 commits into
mainfrom
data-plane-revocation-cache-staleness
Oct 22, 2026
Merged

NancyKwongPerplexity merged 12 commits into
mainfrom
data-plane-revocation-cache-staleness

Conversation

@NancyKwongPerplexity

@NancyKwongPerplexity NancyKwongPerplexity commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

An attached command stream re-checks its session's revocation state every 10 seconds. When that check failed, the stream ended at once with error{backend_stream_error, retryable:true} even though the process kept running. The check fails when the cluster API is unreachable or its database is restarting. A brief control-plane outage therefore cut every long-lived SSE and WebSocket stream that ran a check during it. A check that hung instead froze the stream for up to the cluster API client's 30-second deadline. Error frames also carried raw backend text. The SSE frame included the tonic::Status, which named the internal endpoint (ClusterIP and port), and WebSocket frames forwarded backend status messages.

Separately, accepted data-plane sockets set only TCP_NODELAY. If an idle client's connection died silently, nothing was being sent that could time out, so its stream and the node's per-sandbox permit behind it stayed open indefinitely, and a single-attach Terminal or Process session refused a reconnect with session_already_attached meanwhile.

After this PR, streams survive short revocation-check outages up to a fixed bound, error frames carry fixed messages, and the data-plane listener drops idle dead peers.

Bounded staleness for revocation checks (native.rs, state.rs, lib.rs). Every SSE and WebSocket stream, and port-session upgrade proxying, uses session_is_revoked for this check.

  • Outages: a check that ends in Unavailable, DeadlineExceeded, Cancelled, ResourceExhausted, Internal, or Unknown keeps the session's last confirmed verdict, up to the bound. Cancelled is what a call queued on a cluster API connection that closes during a rolling restart reports, and ResourceExhausted is what an h2 ENHANCE_YOUR_CALM becomes. Internal covers more than outages: tonic reports a response it can't decode as Internal, and the cluster API returns Internal for database errors and for stored rows it can't use, such as an unknown revocation status or a negative epoch. All of those ride the bound too. Other failures fail closed at once:

    • a refusal, such as PermissionDenied;
    • an answer that decodes but can't be used: missing revocation state or an invalid org id.

    A confirmed revocation still ends the stream on the next check.

  • The bound: a failed check keeps the verdict only while the verdict is younger than REVOCATION_MAX_STALENESS minus one check interval minus one check timeout. At defaults that is 47 s (60 − 10 − 3). The verdict's age is read when a check finishes, so the first failed check that finishes after that fails closed. That decision comes 47–60 s after the read that confirmed the verdict began: a check starts at most one interval after the last one that kept the verdict and takes at most its 3 s timeout, so the stream ends within 60 s of that read. A hung check that starts at 45 s, for example, fails closed at 48 s. The bound applies to when checks start, not to delivery: as on main, a blocked sink.send or ingress write_all postpones the stream's next tick.

  • Dating a verdict: each verdict, and the org's next refresh, is dated from when the read that returned it began, not from when the answer arrived. A route resolve can wait up to 15 s on ingress preparation after reading its rows. For the same org epoch the date only moves forward, and the org entry is updated through DashMap::entry, so a late answer cannot overwrite a newer one.

  • Refresh cadence: a confirmed org epoch answers checks for just under one interval (9 s at defaults). A stream alone in its org therefore re-reads the org on every check, not every other one, and its verdict is at most about one interval old when an outage starts. In steady state such a stream rides out at least 37 s of outage at defaults. Right after a slow route resolve it can ride out less, about 30 s: with the resolve's read at 0 s and its answer at 15 s, the stream's checks fall at 25, 35, 45 and 55 s, so an outage that starts at 24.9 s fails closed at the 55 s check.

  • Check timeout: each check, including one that reads the org and then the session, has a single REVOCATION_CHECK_TIMEOUT of 3 s and reports DeadlineExceeded when it runs out. A hung check pauses the stream for at most 3 s and then counts as an outage.

  • Cache bookkeeping: the revocation cache records when each org epoch was last confirmed. A verdict's age is its org entry's. That works because every revocation advances the org epoch, so confirming the epoch unchanged confirms each session read under it. A verdict read under an older org epoch than the cache now holds doesn't count. A refresh that another stream already has in flight answers from the cache only within the same tolerance. Past it, the session is re-read directly.

  • Route-cache hits: an attach answered from the route cache did not ask the cluster API anything. It no longer refreshes the org's confirmation time or postpones the org's next refresh.

  • Interval limit: the tolerance shrinks as --revocation-check-interval-ms grows. Intervals above 19000 ms, a third of what is left of the bound after one timeout, are now refused, which keeps the tolerance at least two intervals. The flag parser rejects them, and run() refuses a DataPlaneConfig that carries one through DataPlaneConfig::validate, before it connects to anything. The service logs the effective tolerance at startup.

  • Metrics: data_plane.revocation.check gains result:kept_last_verdict for a failed check that kept the verdict, and result:timeout for a check that outlived its local timeout. Before, timeouts counted as status_error. A check can record more than one result: a tolerated failed check records status_error or timeout and then kept_last_verdict, and an org read that finds the epoch changed records org_epoch_changed and then the session read's result. Summing the counter over its results therefore does not count checks; the metric's doc says so.

Fixed public error messages (streams.rs, sse.rs, websocket.rs). Stream error frames that report a backend or internal error no longer carry its text, which can name internal endpoints or repeat the guest's own wording. Both transports take those messages from one set of constants in streams.rs:

  • SSE and WebSocket mid-stream failures: process event stream failed or file watch event stream failed.
  • WebSocket stream-open errors: event stream could not be opened.
  • WebSocket refusal before start: this request was refused.
  • Refused terminal resize: terminal resize is not supported by this process.

Frames about the client's own request keep their existing fixed text. The bridges still log the error detail on the server. A refused resize keeps the stream open and now logs the detail at debug.

Dead-peer detection on the data-plane listener (lib.rs). Accepted connections set TCP keepalive: 30 s idle, then 3 probes 10 s apart. This is a data-plane-only layer on top of the shared http_listener::nodelay helper, which is unchanged, so the API listener is unaffected. It applies to every route the data-plane listener serves, ECW's /process.Process/* and filesystem routes included. Keepalive drops an idle dead peer in about a minute, and it never closes a connection whose peer still answers its probes. That peer is the immediate TCP peer, often a proxy rather than the end client. A busy connection is never idle, so keepalive never probes it: a dead peer on an SSE stream, which sends a keep-alive comment every 15 s, waits out the kernel's retransmission timeout, as on main.

No TCP_USER_TIMEOUT is set. The data plane tells native and ECW traffic apart per request, not per connection, so any value would also limit ECW connect streams, which cannot resume once reset. Behind an L7 proxy it would mostly cap how long a client may stall reading, not catch dead peers. On Linux it would also take over from keepalive's probe count, so an idle dead peer would wait out the user timeout instead of about a minute.

Validation

Unit and stream tests:

  • failed_checks_keep_a_verdict_only_while_the_stream_ends_within_the_bound runs on a paused clock with no network, at production timings, for every check phase in 250 ms steps. It reads the verdict's age as each check starts, the earliest a check can, so the decision to fail closed falls 47–57 s after the confirming read. A check that reads it on finishing after its full 3 s timeout decides by 60 s, so the stream ends within the bound.
  • only_unanswered_revocation_checks_keep_the_last_verdict: a table over every tonic::Code, plus MissingRevocationState and RouteState, pinning which failures keep a fresh verdict.
  • revocation_verdicts_are_dated_from_when_their_read_began: against a fake whose answers take 300 ms, the route resolve, internal resolve, org check, and session check each date the confirmation from when the read began.
  • lone_stream_rereads_its_org_on_every_check: on a paused clock, a lone stream's checks re-read the org even when they start up to 500 ms short of one interval after the last read.
  • revocation_check_has_one_timeout_across_its_reads: a 1.2 s org read followed by a hung session read fails with DeadlineExceeded after one 2 s timeout, not two. The org's refresh is made due before the check, so the test does not depend on how fast the route resolve answered.
  • revocation_cache_org_confirmation_only_moves_forward: a slower read that answers last, or an older epoch, does not move the org entry back.
  • revocation_check_interval_above_the_maximum_is_rejected: the service's flag parser accepts 19000 and rejects 19001.
  • run_refuses_a_revocation_check_interval_above_the_maximum: DataPlaneConfig::validate accepts 19000 ms and rejects 19001 ms, and run() returns that error before it connects.
  • Stream tests against a fake cluster API assert on outcomes and check counts, not measured time:
    • process_stream_survives_a_revocation_outage_within_the_staleness_bound: output keeps flowing through failed checks.
    • process_stream_fails_closed_once_its_verdict_is_older_than_the_bound: the stream rides out several failed checks, then ends with the fixed message, and the fake's internal address does not appear.
    • process_stream_keeps_delivering_while_revocation_checks_hang: output sent during hung checks arrives as each check times out.
    • hung_check_after_an_org_revocation_fails_closed_once_it_times_out.
    • refused_revocation_check_fails_closed_at_once: a PermissionDenied answer ends the stream at the first such answer.
    • revoked_session_ends_its_stream_at_once: a revocation confirmed during an outage ends the stream without waiting out the bound.
  • route_cache_hit_does_not_confirm_the_revocation_verdict: a cache-hit attach leaves the verdict's age and the org's next refresh unchanged. Its last probe lands a tolerance past the resolve's refresh and a tolerance short of where one the cache hit set would fall.
  • revocation_cache_keeps_a_confirmed_verdict_only_within_the_staleness_bound: cache-level coverage of the tolerance, in-flight sharing, and newer org epochs, at the production tolerance.
  • websocket_backend_stream_errors_carry_a_fixed_message and websocket_resize_refusal_carries_a_fixed_message: real WebSocket attaches whose backend fails with text that names an internal address. The frames carry the fixed messages, and the address does not appear.
  • accepted_connections_detect_dead_peers: keepalive time, interval and retries on accepted sockets, and no TCP_USER_TIMEOUT.

Each new assertion was mutation-checked: breaking the code it covers makes it fail.

End to end on a local Kind cluster brought up with bootstrap.sh, using scripts that are not committed. Sandboxes were deleted and replicas restored after each run.

Cluster API outage. The script creates one seed:latest sandbox and two commands that print a line per second. It attaches one over SSE (/v1/sessions/{id}/events) and one over WebSocket (/v1/sessions/{id}/attach). It then runs kubectl scale deploy/space-internal-api --replicas=0, holds, scales back to 2, and waits for the rollout.

Build Unreachable SSE WebSocket
main (b6ca1b2) about 31 s ended about 15 s into the outage with backend_stream_error; the message held the raw transport error, including the internal-api ClusterIP:port dropped about 15 s into the outage, with no close frame
this PR (a62db3c) 40 s from scale-down to ready (15 s hold) 40 lines during the outage, 40 after; never ended 40 lines during the outage, 40 after; never ended
this PR (a62db3c) 82 s from scale-down to ready (60 s hold) ended 48 s into the outage with {"type":"error","code":"backend_stream_error","message":"process event stream failed","retryable":true} dropped 47 s into the outage, with no close frame

Reader stall. The script runs a command printing about 40 KB/s and attaches over SSE from a socket with a 4 KB receive buffer, straight to a node's data-plane NodePort. It reads a few events and then stops reading. Every 10 s it polls ss in the data-plane pods' network namespaces, then resumes reading.

Build Stall Server socket After resuming
an earlier head of this PR (a62db3c, 5-minute TCP_USER_TIMEOUT) 330 s zero-window probing until t+296 s; gone by t+306 s ConnectionResetError
this PR (858c6fe) 330 s zero-window probing (timer:(persist,…)) with 31 KB queued, throughout 17.7 MB read in 20 s, stream still open

Idle dead peer. The script runs sleep 900 and attaches over WebSocket from a throwaway container on the Kind network, straight to a node's data-plane NodePort, so the attach carries nothing. A second attach to the same session from the host gets {"type":"error","code":"session_already_attached",…}. The script then runs docker network disconnect on the container, so the client vanishes without a FIN, and polls ss in the data-plane pods' network namespaces every 5 s.

Build Server socket after the client vanished Re-attach after the drop
this PR (858c6fe) first keepalive probe at about t+22 s, 30 s after the attach went idle; three unanswered probes 10 s apart (timer:(keepalive,…,1) to …,3)); gone at t+60 s {"type":"attached",…} then process_started

The run on the earlier head with the 5-minute user timeout was not repeated. The same experiment in a disposable network namespace, scaled down to keepalive 3 s / 1 s / 3 probes, dropped the idle dead peer after 6.2 s with no user timeout and after 15.5 s with a 15 s one, as tcp(7) describes.

The cluster API outage runs above are at a62db3c. The revocation code is unchanged since then: the later commits add the interval check in run(), drop TCP_USER_TIMEOUT from the listener, and change tests and comments.

At 858c6fe, the data-plane pods logged revocation_check_interval_ms=10000 revocation_tolerance_ms=47000 at startup, and scripts/tests/smoke-kind-typed.sh native passed all 26 scenarios on the same cluster, with none failed or inconclusive. The later 8828db9 only removes a unit test, and 4cc7fa4 only declares socket2's all feature, which the build already had through another dependency, so Kind was not redeployed for them. That includes native.m1_generated_sdk_conformance, native_concurrency.slow_reader, native_contract.token_scope, native_recovery.attached_stream_drains_through_data_plane_restart, native_recovery.command_survives_data_plane_restart, and the ECW scenarios that run on the native surface.

Severity and risk

Severity: moderate. A short control-plane outage ended every attached stream that checked during it, and SSE error frames exposed internal endpoint addresses to API clients.

Risk:

  • During an outage, a revoked token can keep streaming for up to the bound: no more than 60 s after the read that confirmed its last verdict began, while the stream's checks run on schedule. As on main, a blocked write to the client postpones the stream's next check. Before, any failed check ended the stream.
  • Answers that tonic or the cluster API report as Internal, including unusable stored rows and database errors, ride the bound instead of failing closed at once.
  • Sustained slowness trades a freeze for a cut. On main, a check that took 5 s succeeded, and the stream paused and lived. Now every check that takes longer than 3 s times out. Nothing confirms, and every attached stream ends after about 50 s, while new attaches still succeed.
  • A hung revocation check now pauses a stream's output for at most 3 s, down from 30 s.
  • An idle data-plane connection whose peer stops answering keepalive probes is closed after about a minute. That includes ECW connections. Keepalive never closes a connection whose peer, or proxy, still answers, so no route gets a new limit on how long a client may stop reading.
  • --revocation-check-interval-ms above 19000 now fails at startup. The default is 10000, and the Kind chart doesn't set it.
  • Port-session upgrade proxying uses the same session_is_revoked check, so it gets the same tolerance.
  • Error frames keep their code and retryable values, including backend_stream_error and resize_not_supported. Only the message text changes, so a client that matched on the old text will see different messages.
  • The /v1 contract and the API listener are unchanged.

To roll back, revert this PR.

Not covered

  • When a WebSocket bridge fails mid-stream, including when it fails closed here, the socket is dropped without an error or close frame. Sending one would let clients tell a failure from a network drop.
  • The revocation check runs inside the stream's select loop, so even a bounded check pauses output while it runs, for up to 3 s. Running the check beside the stream would remove that pause. It would also let a slow answer still confirm.
  • One stream ending removes its siblings' tolerance. When two streams share a session, such as file-watch or port streams, or an SSE process stream and a later attach, forget on one deletes the cache entry. The other fails closed at its next failed check. This fails safe and matches main. Reference-counting entries, or keeping the confirmed epochs on each stream, would fix it.
  • The cluster API returns Internal both for database errors and for invalid stored rows. Returning Unavailable for the first and FailedPrecondition or DataLoss for the second would let the data plane fail closed on bad rows at once.
  • Brownouts: a failed refresh leaves the org's refresh due, and the abandoned RPC keeps running server-side, so retries per org run back to back. A retry-after on failure would spread them.
  • spawn_revocation_fake and spawn_backend_failure_fake rebuild most of spawn_binding_fake. A shared builder with per-RPC hooks would keep them from drifting.
  • A dead peer on a busy connection, such as an SSE stream, still waits out the kernel's retransmission timeout, and an idle native WebSocket attach is only caught by keepalive. A native-only fix would be a write-progress deadline in the native SSE and WebSocket bridges (no successful write for N), plus WebSocket pings with a pong deadline for idle attaches. N should come from measured stall durations, such as a histogram of how long a write to the client stays blocked, per surface.
  • There is no typed local.* smoke scenario for an attached stream across a cluster API outage or a reader stall.

AI assistance: Claude Code drafted the code, tests, and this description; the author reviewed them.

A failed revocation refresh ended every attached stream that checked
during it, so a brief cluster API or database outage cut long-lived
command streams while their processes kept running. The error frame
also carried the raw status text, which can name an internal endpoint.

A failed check now keeps the session's last confirmed verdict while it
is younger than a 60 second bound, then fails closed. A confirmed
revocation still ends the stream at once, and a check shared with
another stream answers from the cache only within the same bound. The
rule lives in session_is_revoked, so SSE and WebSocket streams share it.

Stream error frames now carry a fixed message; the detail stays in the
log. Accepted data-plane connections set TCP keepalive and, on Linux,
TCP_USER_TIMEOUT, so a client whose connection dies silently releases
its stream in about a minute.
The WebSocket bridges still forwarded backend status text in three
frames: a refusal before the stream opened, a process or file-watch
stream that failed mid-stream, and a refused terminal resize. That
text can name internal endpoints or carry the guest's own wording.

Those frames now carry fixed public messages, shared with the SSE
frames from one set of constants. The bridges log the status as
before; a refused resize, which keeps the stream open, now logs it too.
A stream could outlive the 60 second staleness bound: the check that
found the verdict too old could come one interval after the last one
that passed, and a hung check could hold the stream for the cluster API
client's 30 second deadline. A failed check now keeps the verdict only
while it is younger than the bound less one check interval and one
check timeout, so the stream ends within the bound of its last confirmed
check. Each revocation RPC has a 3 second timeout, reported as a
deadline status.

Only failures to answer keep the verdict: Unavailable, DeadlineExceeded,
Internal and Unknown statuses. A malformed answer or a refusal fails
closed at once.

An attach answered from the route cache no longer counts as a confirmed
check, and leaves the org's next refresh where the resolve put it.
@NancyKwongPerplexity
NancyKwongPerplexity marked this pull request as ready for review October 21, 2026 20:13

@ac-pplx ac-pplx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. The core cache logic holds up well: the org-epoch gate in confirmed_active, the in-flight tolerance in probe, and the route-cache-hit change. I could not find an interleaving where a revocation the data plane has already learned of is overridden by the new tolerance. The points below are about the edges of the 60 s bound, a side effect of the TCP change, and a few places where the description is stronger than the code. None of them is a blocker.

Before merge

  1. TCP_USER_TIMEOUT also resets live clients that stop reading for 60 s. crates/libs/data/plane/server/src/lib.rs:473, lib.rs:493

    • On Linux 5.11 and later, TCP_USER_TIMEOUT also limits how long a peer may keep a zero receive window, even while it answers every window probe.
    • Failure scenario: a client, or an ingress proxy passing backpressure upstream, pauses reading for more than 60 s while command output is pending. The kernel resets the connection. On main the stream just waited.
    • The listener wraps the whole router, ECW /process.Process/* routes included (lib.rs:448-449, lib.rs:670-682). "ECW paths are unchanged" holds for the handlers, but not for ECW connections, and an ECW connect stream cannot resume.
    • SSE sends a keep-alive comment every 15 s (native/sse.rs:185), so TCP keepalive never probes an SSE connection. The user timeout is what catches dead SSE peers, so removing it loses that.
    • Suggested fix: choose a deliberate stall limit, for example a few minutes, or keep 60 s and document it as part of the stream contract in Risk. Correct the ECW statement. A Kind run with a reader stall longer than the timeout would show which behaviour you get.
  2. Confirmation time and next refresh are stamped when the answer arrives, not when the cluster API read the row. crates/libs/data/plane/server/src/state.rs:583-589, set via native.rs:84, native.rs:386, native.rs:881, native.rs:899, native.rs:926

    • Route resolves have no 3 s cap; they run under the cluster API client's 30 s deadline. A Port or Protocol resolve reads the session row and then waits for ingress preparation (crates/libs/internal/api/server/src/data_plane_sessions/session_route.rs:56, session_route.rs:133), with a 15 s budget.
    • Failure scenario: a resolve reads org epoch E, a revocation in the org commits, and the response arrives 15 s later. That stamp makes E look fresh, so the bound measured from the actual read becomes about 75 s.
    • Second effect: for a stream that is alone in its org, refresh_after = arrival + interval. The tick one interval later is therefore still Fresh, and the org is re-read only every two intervals. At defaults the verdict can be about 20 s old when an outage starts, so the guaranteed ride-through is about 30 s, not 47 s. At intervals of 19 s or more a lone stream gets none.
    • Suggested fix: capture tokio::time::Instant::now() before each confirming RPC and stamp both confirmed_at and refresh_after from it, moving confirmed_at only forward. This fixes both effects.
  3. One check can make two sequential 3 s RPCs. crates/libs/data/plane/server/src/native.rs:891, native.rs:911

    • Failure scenario: when the org epoch changed, RefreshOrg runs the org RPC and then the session RPC, each under its own timeout. A 2.9 s org answer followed by a hung session read pauses output for about 6 s, and the stream ends about 62.5 s after its last confirmation.
    • No output flows while the check runs and the outcome fails closed, so nothing is exposed. The "at most 3 s" pause and the "within 60 s" end are what is off, along with the paused-clock test, which models one timeout per check (native/tests.rs:1183-1190).
    • Suggested fix: one tokio::time::timeout(check_timeout, check_session_revocation(..)) in session_is_revoked in place of the per-RPC wrappers.
  4. Cancelled is not treated as an outage. crates/libs/data/plane/server/src/native.rs:833-845

    • tonic 0.14.6 reports a canceled hyper request, and an h2 CANCEL reset, as Code::Cancelled.
    • Failure scenario: a revocation RPC is queued on a connection to a cluster API pod that closes during a rolling restart. The check returns Cancelled and the stream ends at once, with most of its budget unused.
    • Scaling to zero replicas gives connection refused, which maps to Unavailable, so the Kind run would not show this.
    • The cluster API never answers Cancelled as a verdict. Suggested fix: add it, and consider ResourceExhausted, which is what h2 ENHANCE_YOUR_CALM becomes.
  5. The classification has gaps in its tests. crates/libs/data/plane/server/src/native.rs:833-845, native/tests.rs:840-860

    • No test pins the Internal or Unknown arms. Deleting them leaves every test green.
    • unusable_revocation_answers_fail_closed_at_once advances the org epoch from 1 to 2 before its invalid-org answer. confirmed_active therefore rejects the old verdict whatever the classifier says, so that case does not exercise the rule. Missing state is covered by the predicate assertion in the phase test.
    • Suggested fix: one pure table test over every tonic::Code, plus MissingRevocationState and RouteState, asserting keep or fail closed. It can also replace the per-phase missing-state assertion at native/tests.rs:1203-1211.
  6. Description and Risk corrections.

    • Sustained slowness: on main, a revocation check that took 5 s succeeded, and the stream paused and lived. Now each such check times out (native.rs:942-950, native.rs:971-978), nothing confirms, and every attached stream ends after about 50 s, while new attaches still succeed. Worth a line in Risk, since it trades the freeze for a cut.
    • The bound applies to check scheduling, not delivery. A blocked sink.send or ingress write_all postpones the tick (native/websocket.rs:243, native/websocket.rs:577), as it did on main. Suggest wording such as "the first check that starts after the tolerance fails closed".
    • The cluster API returns Internal for unusable stored rows, such as an unknown status or a negative epoch (crates/libs/internal/api/server/src/data_plane_sessions/revocation.rs:94, revocation.rs:118). Those ride the bound rather than failing closed at once, as the commit message says they should.
    • The resize message is an inline literal (native/websocket.rs:1112), not one of the shared constants.

Follow-ups

  • One stream ending removes its siblings' tolerance. crates/libs/data/plane/server/src/state.rs:668, native/sse.rs:357-360, native/sse.rs:667-670, native/websocket.rs:69, native/websocket.rs:115
    • Only Terminal and Process sessions are single-attach (state.rs:2127-2129), and an SSE process stream releases its permit once the binding settles (native.rs:1048-1050).
    • Failure scenario: two file-watch or port streams share a session, or an SSE stream and a later attach share a process. One ends during an outage, forget deletes the entry, and the other fails closed at its next check.
    • This fails safe and matches main. Fix: reference-count entries, or keep the confirmed epochs on each stream.
  • Separate bad-row answers from outages on the server. Return Unavailable for database connectivity errors and FailedPrecondition or DataLoss for invalid stored rows in revocation.rs. Internal can then mean only an undecodable answer.
  • Brownout behaviour. A failed refresh leaves refresh_after in the past, and the abandoned RPC keeps running server-side. Retries per org therefore run back to back. Consider a retry-after on failure, or running the refresh beside the stream so a slow answer can still confirm.
  • Interval coupling. The tolerance silently shrinks as --revocation-check-interval-ms grows (native.rs:824-829, crates/apps/data/plane/svc/src/main.rs:79-80). At 28.5 s or more, RefreshInFlight can no longer be reached (state.rs:629, state.rs:644-648), so every ticking stream issues its own session read. At 57 s or more the tolerance is zero. Validate at startup or log the effective tolerance.
  • Observability. No metric separates "kept the last verdict" from other failed checks, and local timeouts count as status_error (native.rs:798-803, native.rs:990-996). A kept_last_verdict label would let operators see how many streams are riding the tolerance.
  • Test fakes. spawn_revocation_fake and spawn_backend_failure_fake rebuild most of spawn_binding_fake (native/tests.rs:779-923, native/tests.rs:1290-1370). A shared builder with per-RPC hooks would keep them from drifting.
  • remember_org_epoch. The get-then-insert is not atomic (state.rs:577-590). A late equal-or-older epoch can replace a newer one with a fresh stamp. The window is narrow and predates this PR; DashMap::entry would close it.

Nits

  • The constants doc says "frames carry only these" (native/streams.rs:201-203), but other WebSocket frames carry other fixed literals.
  • confirmed_active and session_is_revoked say "the session's last confirmed verdict", but the age is the org entry's confirmed_at (state.rs:651-665). That is sound because every revocation bumps the org epoch; worth saying so.
  • revocation_check_unavailable says the cluster API "could not answer", but Internal also covers undecodable answers (native.rs:831-832).
  • Tests pass REVOCATION_MAX_STALENESS as tolerance, while production passes 47 s (src/tests.rs:1858-1919).
  • The phase test's lower bound accepts tolerances between 44 s and 47.25 s (native/tests.rs:1189, native/tests.rs:1198). Asserting on the start time of the failing check would tighten it.
  • The TCP doc block describes four constants but attaches only to the first (lib.rs:464-468). Keepalive detects the immediate TCP peer, which is often a proxy, not the end client.
  • The refused-resize info! fires once per client resize frame (native/websocket.rs:1107); debug may suit better.

On Linux 5.11 and later, TCP_USER_TIMEOUT also limits how long a peer
may keep a zero receive window while it answers every window probe. At
60 seconds, a client or a proxy passing backpressure upstream that
paused reading for a minute with output pending had its connection
reset, on every route the listener serves, ECW's included. Before the
listener set it, such a stream just waited.

The user timeout is now 5 minutes, which is the longest a reader may
stall. It stays set because SSE's 15-second keep-alive comments keep a
stream from ever being idle, so keepalive never probes it, and the user
timeout is what drops a dead SSE peer.
…k once

A verdict and the org's next refresh were stamped when the answer
arrived. A route resolve can wait on ingress preparation for up to 15
seconds after reading its rows, so a revocation committed meanwhile
could be ridden out about 15 seconds past the bound. And a stream alone
in its org found its own refresh not yet due one interval later, so it
re-read the org only every other check and rode out less of an outage.
Each read now records when it began: the cache dates the confirmation
from it, moves it only forward for the same epoch, and makes the next
refresh due just under one interval after it. The org entry is updated
through the map's entry API, so a late answer cannot overwrite a newer
epoch between its check and its write.

A check that read the org and then the session had a timeout per read,
so it could pause output for twice the timeout. The whole check now has
one timeout.

Cancelled and ResourceExhausted statuses, which a cluster API pod
closing its connections in a rolling restart or shedding load produces,
now count as outages like Unavailable, instead of ending the stream at
once.

A check that keeps the last verdict is counted as kept_last_verdict,
and a check that outlived its local timeout as timeout rather than
status_error. The service rejects a check interval above 19 seconds,
past which a failed check could not keep a verdict through even one
more interval, and logs the effective tolerance at startup.
The refused-resize frame was the one backend-error frame whose fixed
message was an inline literal rather than one of the shared constants.
Its detail is now logged at debug, since a client can send a resize
frame many times on one stream.
…e CLI

The 19-second cap on the revocation check interval was enforced only by
the service's flag parser, so a caller that builds DataPlaneConfig
itself could still start with a tolerance shorter than two intervals.
run now validates the config before it connects to anything and
refuses such an interval.

The revocation check metric's doc now says a check can record more
than one result, such as status_error or timeout followed by
kept_last_verdict, so summing its results does not count checks.
@NancyKwongPerplexity

Copy link
Copy Markdown
Collaborator Author

thanks, all addressed in 530517f..fa3fac4:

  1. user timeout is now 5 min, documented as the stall limit in risk, ecw statement fixed. kind: the 60s build reset a 90s stall at ~60s; now 90s survives and 330s resets at ~300s
  2. reads are dated from when they start and only move forward. that alone was still a coin flip for a lone stream in real time (tick jitter), so freshness is now 0.9× interval and lone streams re-read every check
  3. one timeout around the whole check
  4. cancelled + resource_exhausted now count as outages
  5. table test over every code + missing state / route state; dropped the missing-state/invalid-org stream cases and the per-phase assert
  6. pr body updated (slowness, scheduling vs delivery, internal bad rows); resize msg is a constant now

follow-ups: added kept_last_verdict + timeout labels, entry api in remember_org_epoch, interval capped at 19s (enforced in run() via DataPlaneConfig::validate, not just the cli) with the tolerance logged at startup. metric doc notes a tolerated failure counts twice. the rest are listed under not covered

nits: all done

@ac-pplx ac-pplx self-assigned this Oct 22, 2026
@ac-pplx
ac-pplx self-requested a review October 22, 2026 02:24
@ac-pplx ac-pplx removed their assignment Oct 22, 2026

@ac-pplx ac-pplx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough round. I re-checked every item from the last review at fa3fac4. All six before-merge items are addressed, and the revocation fixes hold up in the code:

  • Dating: each verdict and the org's next refresh are dated from when the read began (state.rs:580-603, state.rs:710). Every confirming read starts the clock before its RPC (native.rs:60, native.rs:370, native.rs:911, native.rs:934, native.rs:953), and freshness is 0.9 × interval (native.rs:866).
  • Timeout: one timeout around the whole check (native.rs:805).
  • Outage codes: Cancelled and ResourceExhausted count as outages (native.rs:876-886).
  • Classification test: an exhaustive table over every code (native/tests.rs:1144).
  • Description: updated.

The DashMap::entry change, the interval cap in run(), the startup log of the effective tolerance, and the new metric labels also check out. The follow-ups you listed under "Not covered" match the code.

Two things before merge. The first revisits my own suggestion from last round, now that I've looked at it more closely.

Before merge

  1. Drop TCP_USER_TIMEOUT from the shared listener, and keep keepalive. crates/libs/data/plane/server/src/lib.rs:506-513, lib.rs:520-529, lib.rs:540-543

    • It can't be limited to native traffic. The option is set when a connection is accepted. That listener serves native and ECW traffic alike, and the data plane tells them apart only per request, by path and Host header (lib.rs:720, lib.rs:1538). Whatever value we choose applies to ECW connect streams, which can't resume once reset. The PR's own reader-stall run shows the effect at the new value: a 330 s pause is reset at about 300 s. On main, how long a reader could stall was left to the client and whatever proxy sits in front, and the data plane did not limit it.
    • Behind an L7 proxy it mostly limits stalls, not dead peers. The data plane usually runs as a ClusterIP service behind an ingress, so the TCP peer is the proxy, not the end client. The proxy deals with a client that vanishes. What reaches the data plane is backpressure: the client stops reading, the proxy's buffer fills, and the proxy stops reading upstream. So the option mainly becomes a new limit on how long a client may stall, tighter than whatever idle limit the proxy is configured with.
    • On Linux it also overrides keepalive. tcp(7): when TCP_USER_TIMEOUT is used with SO_KEEPALIVE, it decides when the connection closes, and the probe count is ignored. With 5 minutes, an idle dead peer is dropped after about 5 minutes, not the "about a minute" that lib.rs:504, lib.rs:523-524 and the PR body state. Terminal and Process sessions are single-attach, so in that window a reconnect to the same replica gets session_already_attached. At the earlier 60 s value both came to about a minute, so the doc happened to be right.
    • Without it, keepalive alone is safe for every route. Keepalive never closes a live connection, because the peer, or the proxy, answers its probes. Idle dead peers are then dropped in about a minute, as documented. ECW behaves as it does on main.
    • What drops is the dead peer on a busy connection, such as an SSE stream with its 15 s keep-alive comments. It waits out the kernel's retransmission timeout, as on main.
    • Suggested follow-up, native only: a write-progress deadline in the native SSE and WebSocket bridges (no successful write for N), plus WebSocket pings with a pong deadline for idle attaches. Choose N from measured stall durations rather than a guess, for example a histogram of how long a write to the client stays blocked, per surface.
    • This also means removing the 5-minute stream contract from Risk, and adjusting accepted_connections_detect_dead_peers (src/tests.rs:874).
  2. Two new tests reach their branch only if a localhost RPC takes longer than 0.9 ms. crates/libs/data/plane/server/src/native/tests.rs:1365-1400, native/tests.rs:1405-1440

    • Both set a 1 ms interval, so freshness is 0.9 ms. They resolve with the fast default answer and then call session_is_revoked right away on the real clock.
    • Failure scenario: the resolve finishes within 0.9 ms of when its read began. probe then returns Fresh(true) (state.rs:635), the timeout test's expect_err panics, and the metrics test records nothing.
    • The margins are tight too:
      • the 800 ms org read has to finish inside the 1 s budget, otherwise the verdict is kept and expect_err panics;
      • the metrics test needs the Down answer within its 100 ms timeout.
    • Suggested fix: make the org's refresh due explicitly before the check, or wait one interval, and widen the margins.

Nits

  • "Rides out at least 37 s" holds in steady state. Right after a slow resolve, the stream can ride out about 30 s. Example: read at t=0, answer at t=15, ticks at 25, 35, 45 and 55, and an outage starting at 24.9 s.
  • The keep-or-fail decision uses the verdict's age when the check finishes. A hung check that starts at 45 s fails closed at 48 s, so "starts 47-57 s after" describes when the decision is made, not when the check starts. That is conservative; it only affects the wording. The Risk line "never more than 60 s" could also carry the scheduling caveat that the Bound bullet already has.
  • MAX_REVOCATION_CHECK_INTERVAL_MS: "a third of the staleness bound less one check timeout" (lib.rs:152-153) reads as 17 s. The code computes (60 − 3) / 3 = 19 s.
  • route_cache_hit_does_not_confirm_the_revocation_verdict (native/tests.rs:1493-1503): with 0.9 × interval freshness, a cache hit that wrongly reset refresh_after lands exactly on the probe time, so the last assertion still passes. The earlier !confirmed_active assertion catches the likely regression. Advancing by fresh_for - tolerance (8 s) would restore the check.

The data-plane listener serves native and ECW routes alike and tells
them apart only per request, so a user timeout set on accept applied to
ECW connect streams too, which cannot resume once reset. Behind an L7
proxy the TCP peer is the proxy, so the option mostly capped how long a
client could stall reading, tighter than the proxy's own idle limit. On
Linux it also overrides the keepalive probe count, so at 5 minutes an
idle dead peer was dropped after about 5 minutes rather than the minute
the listener documents.

Accepted connections now set only keepalive, which never closes a
connection whose peer, or proxy, still answers. An idle dead peer is
dropped in about a minute again, and ECW connections see no new stall
limit. A dead peer on a busy connection, such as an SSE stream, waits
out the kernel's retransmission timeout, as on main.

socket2's all feature is now needed only by the listener test, for the
socket option getters it reads.
… branch

Both tests set a 1 ms check interval so that the org would be due for a
refresh by the next check, but freshness is 0.9 of the interval. When the
route resolve answered within 0.9 ms of when its read began, the check
answered from the cache: the timeout test's expect_err panicked and the
metrics test recorded nothing.

The tests now make the org's refresh due explicitly before each check
and keep the default interval. The timeout test's org read leaves 800 ms
of a 2 s budget, and 600 ms separates one timeout from a read plus a
second one; the metrics test's timeout is 1 s instead of 100 ms.
With freshness at 0.9 of the interval, a cache hit that wrongly pushed
the org's next refresh out landed exactly on the time the test probed,
so its last assertion passed either way. The test now advances to a
tolerance past the resolve's refresh, a tolerance short of where one set
by the cache hit would fall.

The interval cap's doc read as a third of the bound less one timeout,
17 s; the code takes a third of what is left after one timeout, 19 s.
The fail-closed doc said the first check that starts past the tolerance
ends the stream, but the verdict's age is read when a check finishes.
The repository's test policy keeps tests off metrics and telemetry, and
revocation_check_metrics_count_kept_verdicts_and_timeouts asserted on
the data_plane.revocation.check results a check records. The metric and
its kept_last_verdict and timeout labels are unchanged. The test-capture
dev-dependency only that test used is removed with it.
TcpKeepalive::with_retries, which the data-plane listener uses to set
the probe count, is gated on socket2's all feature. The crate stopped
declaring it when TCP_USER_TIMEOUT went away and built only because
hyper-util enables it for the whole build. The feature is declared on
the normal dependency again, which also covers the socket option
getters the listener test reads, so the dev-dependency entry is gone.
@NancyKwongPerplexity

Copy link
Copy Markdown
Collaborator Author

thanks for the second pass. all addressed in d9595ce..4cc7fa4:

  1. tcp_user_timeout: agreed, dropped it and kept keepalive. checked the tcp(7) override in a netns first (idle dead peer dropped at 6.2 s with no user timeout vs 15.5 s with a 15 s one, at scaled keepalive timings). on kind a 330 s reader stall now survives, an idle dead peer goes at ~60 s and re-attach then works. the 5 min stream contract is out of risk, and a write-progress deadline + ws ping/pong is listed under not covered. socket2 keeps all in [dependencies] since with_retries needs it. d9595ce, 4cc7fa4
  2. flaky tests: both now make the org refresh due explicitly on the default interval, with wider margins (1.2 s read inside a 2 s budget). e390a8c

nits:

  • 37 s: now says steady state, plus your ~30 s after-slow-resolve case
  • 47–57 s: reworded to when the decision is made (47–60 s, with your 45 → 48 example), and the 60 s risk line carries the scheduling caveat
  • interval cap doc: fixed, says 19 s now
  • route cache test: advances by fresh_for − tolerance. confirmed the old version passed with a refresh-pushing mutation and the new one fails. 858c6fe

also dropped revocation_check_metrics_count_kept_verdicts_and_timeouts, since it asserts on metrics (same test-policy point you raised on #799). the labels themselves are still there. 8828db9

@ac-pplx ac-pplx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem. Every attached command stream asks the cluster API every 10 seconds whether its session was revoked. On main, any failed check ended the stream at once, even though the command kept running. A short cluster API or database outage therefore cut every attached SSE and WebSocket stream that checked during it, and the error frame carried raw backend text.

Example.

t=0     attach; check confirms active
t=10    check OK
t=12    cluster API unreachable
t=20    check fails
          main:    stream ends with backend_stream_error (raw backend text)
          this PR: keeps the 10 s old verdict (limit 47 s); output keeps flowing
t=42    cluster API back
t=50    check OK; the stream never ended

Verdict: approving. I re-checked the previous review at 4cc7fa4, and every item is fixed:

  • TCP_USER_TIMEOUT: removed. The listener sets keepalive only (lib.rs:526-536), its docs match (lib.rs:515-525), and the test asserts that no user timeout is set on Linux (src/tests.rs:897-900). The 5-minute stall contract is gone from Risk.
  • Timing-sensitive test: make_org_refresh_due (native/tests.rs:780) makes the refresh due explicitly. With the wider margins (1.2 s read inside a 2 s budget, ceiling 2.6 s), a return to per-read timeouts would still fail it, at about 3.2 s.
  • Route-cache test: now advances by fresh_for - tolerance (native/tests.rs:1468-1470).
  • Wording: the interval-cap doc (lib.rs:152-158), the "decision when a check finishes" wording (native.rs:794-799), and the steady-state and Risk wording are all fixed.

One optional wording nit, no code change:

  • PR body, "Refresh cadence": "about 30 s" right after a slow resolve assumes the failed checks answer at once. If they hang instead, take the same example: read at 0, answer at 15, checks at 25, 35, 45 and 55, outage from 24.9 s. The 45 s check times out at 48 s with a 48 s old verdict and fails closed. That is about 23 s ridden out. The steady-state "at least 37 s" still holds for hung checks.

@NancyKwongPerplexity
NancyKwongPerplexity added this pull request to the merge queue Oct 22, 2026
Merged via the queue into main with commit 290ee17 Oct 22, 2026
27 checks passed
@NancyKwongPerplexity
NancyKwongPerplexity deleted the data-plane-revocation-cache-staleness branch October 22, 2026 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants