Repository navigation
data plane: resume process SSE streams from the guest output journal - #799
Conversation
ac-pplx
left a comment
There was a problem hiding this comment.
Thanks. This is a solid change. I traced the id encoding through every load, first_after, next and restart path. Ids stay strictly increasing and above Last-Event-ID, data-plane positions match the guest journal's sequences, and space-error-reason survives the node hop. I found no blockers. There are three small fixes and two design questions I'd like answered before merge. The rest can be follow-ups.
Before merge
-
A backend failure before the first start event drops the response's only
process_started.- Code:
native/sse.rs:456presetsprocess_idfor a bound process. The reconnect arm setsreopened = true(:782), so the reopened stream's firstConnectedis filtered (:610,:643-647). - Scenario: a bound attach whose first backend stream fails with
Unavailable, or a refreshable route rejection, after open but before itsConnectedmessage. The client getsattached,backend_reconnecting,backend_reconnectedand output, but neverprocess_started. Onmainthe start was repeated, not lost. Clients that wait for it, such asspace_smoke/m1_conformance.py:701-703, never run their process control. - Fix: record "this response already sent
process_started" when the frame is pushed, and drop only later ones. That also covers the window-miss and cursor-ahead reopens. A test that fails the stream before its firstConnectedwould pin it.
- Code:
-
Some public text is now wrong or stale.
crates/libs/public/api/src/lib.rs:1879-1880: theDataPlaneAttachInfo.events_urldoc, which is published schema text, still says streams "do not replay missed output".docs/space/diagrams/data-plane-sigterm-drain.mmd:19and its.svgstill say "missed output not replayed". That contradicts the rewrittendocs/architecture.md:243.crates/libs/api/src/routes/public_docs.rs:68says an attach withoutLast-Event-ID"starts from the oldest retained output". It actually starts at position 0 (sse.rs:408-413). Once position 0 is evicted, the guest answers lagged and the stream sendscursor_out_of_window, then the live tail (sse.rs:688-699). For a completed process that means no stdout at all, even with output still retained. Suggested wording: "starts from the beginning of the output; if that is no longer retained,cursor_out_of_windowand then live output".- The same text says ids "exceed the integers a double represents exactly". Every id's set bits lie in bits 16 to 62, so a double holds each one exactly;
src/tests.rs:3654-3656asserts this. What breaks is decimal printing above 2^53. Suggested wording: "exceed 2^53, so a JavaScriptNumbermay print them back differently".
-
A test and some comments still describe the dropped contracts.
src/tests.rs:3662-3687still asserts thelast + 1resume for"process", andnative/auth.rs:97-101still justifiesMAX_LAST_EVENT_IDby it. Both are file-watch only now.src/tests.rs:3534-3541talks about "decimal boundaries" (999_999,1_000_000) from the first commit's decimal encoding, which this PR no longer uses. Probing the real layout edges would be better, for examplePROCESS_EVENT_ID_TAG - 1and the last id below the first no-position block.sse.rs:277-284says a+1-counted id never decodes, without the k × 65,536 exception that the Rollout section names.
Questions before merge
Neither is a correctness problem. I'd still like an answer before merge, because the SSE id layout and the guest change are both costly to revise once they ship.
-
Should process ids fit under 2^53?
- The
Last-Event-IDschema is stillinteger/int64(schema_contract.rs:117-126), while the text calls ids opaque strings. - Generators and JS clients that map int64 to
numberprint a different decimal. Every resume then replays the retained output behindcursor_replay_unavailable, and later ids stay in no-position blocks (sse.rs:384-394), so such a client never resumes exactly. The generated-SDK smoke uses Python, so CI cannot see this. - Earlier data planes issue small counters, so a tag at bit 52 would separate them just as well. That leaves 16 guard bits and 36 bits for frame plus field, and the
+1guard keeps working. - If you prefer the current layout, please list this as an accepted limitation in the PR and the docs.
- The
-
Could the guest report its oldest retained sequence in this same change?
- After the first eviction the stream sets its cursor to
None(sse.rs:694-699), andUnknownids never return to positions. A fresh attach to a process that has written more than 4 MiB lands in the same state, because guestcursor(Some(0))withfirst_sequence > 0returns lagged (process.rs:1228). - On long, chatty processes, where reattach matters most, each later short disconnect then loses output the guest still retains. It is flagged by a resync and is no worse than
main. - Adding
first_sequenceas metadata on the lag status would let the data plane reopen there and keep positional ids. This guest change already needs a template rollout, so doing it later costs another one. A follow-up is fine if you'd rather keep the scope.
- After the first eviction the stream sets its cursor to
Follow-ups
- The window-miss reopen bypasses route refresh.
- Code:
sse.rs:700-716reconnects withstate.clientand the stream's existing route token, which has anexpires_at(state.rs:97-101). - Scenario: a long-lived stream whose token has expired falls behind the journal. The reopen is refused as a refreshable rejection, and the stream ends with a retryable
errorinstead of continuing. - Fix: send a refreshable failure through the existing refresh path at
:717, and add that case to the eviction test.
- Code:
- Eviction is detected by a bare status code.
is_output_window_miss(sse.rs:812-820) treats any mid-streamRESOURCE_EXHAUSTEDother than the limiter's as eviction.- That also matches spaced-host's oversized-chunk refusal (
crates/libs/spaced/host/server/src/process.rs:28-36), the guest's "process replay state was evicted", and an h2ENHANCE_YOUR_CALMreset. Each is reported ascursor_out_of_windowplus a jump to the tail. - There is also no cap on reopens that make no progress.
- Fix: tag the guest's lag status with a reason, as
process-output-cursor-aheadis tagged, and keep the bare match only for older guests. Cap reopens without progress, as the ECW stream does.
- The mixed-version exception.
- An old replica that counts exactly k × 65,536 frames on from a new id produces an id that decodes as a position (
sse.rs:343-345). Resuming from it duplicates output silently, or skips some if the count carries into the field. - A two-phase rollout would remove the case entirely: ship decoding first, then turn on the new ids once every replica decodes them.
- An old replica that counts exactly k × 65,536 frames on from a new id produces an id that decodes as a position (
- Older guests.
- With an older guest, a positional cursor past the output waits instead of failing, so output is skipped without a resync. As the PR says, only a forged or foreign id gets there.
- Separately,
ecw_process/stream.rs:232-234notes that older guests replay a retriedStartfrom the output tail. On the native lost-binding path (streams.rs:23-50), positions would then be counted from 0 on a stream that begins at the tail, and a later positional resume would duplicate output silently. - Fix: run the old-guest release check from
docs/versioning.mdagainst a retained 0.1.x template before relying on exact positions there.
- Ids carry no session.
load(sse.rs:415-423) accepts any decodable position, so a client that sends another session's id to a bound process resumes silently at that position. Either document that an id is only valid on the stream that issued it, or derive the guard bits from the session id. - Operability.
cursor_out_of_windownow covers eviction, cursor-ahead, a foreign id on the starting attach, and misclassified transport errors, and nothing tells them apart. The window-miss branch logs nothing.- The new 400 records no
attachmetric (sse.rs:98-107). loadnow runs before the 409 check, soreplay_misscounts frames that are never sent (native.rs:756-761).
- Backpressure. A reattach now connects at position 0, so it takes the guest's primary-reader slot there (
process.rs:1056-1066). A slow reattached client can pause a process with a full buffer until the replay drains. Worth a line in the docs. - Simulator and proto drift.
crates/libs/node/api/sim/src/processes.rs:10-13says it follows the guest journal contract, but:250-253clamps a cursor past the output to the tail.proto/spaced/operations/v1/operations.proto:227-231does not document the newOUT_OF_RANGEor the lag status.
- Smoke coverage. Neither smoke checks that output written while disconnected is replayed. M1 prints its marker before the disconnect (
m1_conformance.py:774), and the REST smoke resumes after exit. A command that keeps printing across the disconnect would cover it.
Nits
- Cursor-ahead followed by an evicted start sends
cursor_out_of_windowtwice: once atsse.rs:487-494, then again at:693-699. ProcessEventId::encodedoes not check its fields, andnextfrom the last block wraps backwards (sse.rs:332-340,:390-393). Neither is reachable today. Adebug_assert!and a const layout check would make the invariant local. The layout constants arepub(crate)only for tests, andPROCESS_EVENT_ID_MAX_RESUME_BLOCKcould be derived from the field width.is_output_cursor_ahead's doc (sse.rs:822) talks about older guests. It should first say what the function matches.JournalFake(native/tests.rs:721-950) repeats most ofBindingFake(:418-521).- This changes documented native-API behavior, so it would help to say which release line it targets.
|
thanks for the careful trace. all addressed in b55f2d9..25561e4:
|
ac-pplx
left a comment
There was a problem hiding this comment.
Thanks for the thorough round. I re-checked every round-1 item at 25561e4. Everything is either fixed or deferred in the PR body the way we discussed. The bit-52 layout, started_sent, gap_reported, the tagged lag status and the route-refresh reopen all hold up, and ids stay increasing and above Last-Event-ID on every path I traced.
One new problem comes from the reopen-at-oldest-retained path, and I'd like it fixed before merge. The rest are small.
Round-1 items
| Item | Status |
|---|---|
1. process_started lost when the backend fails before the first start |
Fixed (native/sse.rs:559-566; test native/tests.rs:1444) |
| 2. Wrong or stale public text | Fixed. One other bullet went stale this round; see below |
| 3. Tests and comments describing dropped contracts | Fixed |
| 4. Ids under 2^53 | Fixed (bit 52, const check, 400 at or above 2^53; the example ids decode correctly) |
| 5. Guest reports its oldest retained sequence | Fixed in the guest. The data plane's use of it has the issue below |
| Route refresh on window-miss reopen | Fixed (native/sse.rs:638-641) |
Bare RESOURCE_EXHAUSTED match; reopen cap |
Fixed (native/sse.rs:925-952, :604-607) |
| Ids carry no session | Fixed through the documented option |
| Operability | Fixed. The single wire reason is deferred |
| Simulator and proto | Fixed |
| Nits: double gap report, classifier doc, merged fakes, release line | Fixed |
Nit: ProcessEventId invariants |
Mostly fixed. See the last nit below |
| Two-phase rollout, older-guest check, backpressure docs, disconnect smoke coverage | Deferred in the PR body. Fine with me |
Before merge
-
Reattaching to a busy process can fail on every attempt.
- How it fails.
- The guest reports
first_sequence = Funder the journal lock and ends the stream. The data plane reconnects atFone data-plane-to-guest round trip later (native/sse.rs:617-627). - A reader that lagged never takes the primary-reader slot, which requires
cursor >= first_sequence(process.rs:1054-1059). With no primary, the journal evicts its oldest output on every append once it is full (process.rs:1143-1148,:1276-1281). - So a write during that round trip evicts
F, and the reopen lags again. After three such reopens the stream ends with a retryableerror(native/sse.rs:604-607). The retry from the gap idPosition{F,0}repeats the same chase.
- The guest reports
- Scenario. A process has written more than the retention window and keeps writing faster than about once per round trip, for example a verbose build or a log follower. A fresh attach, or a resume after a long disconnect, gets
process_started,cursor_out_of_windowand thenerror. It gets no output until the process slows down or exits. Onmain, and at the previous head, this case went to the live tail, became the primary reader and streamed. - Why it isn't caught.
window_miss_reopens_without_progress_are_capped(native/tests.rs:1234-1262) is exactly this guest behaviour, and it expects the error. The 8.6 MB Kind case reattached after the writer had finished, so it never hit the race. - Suggested fix.
- Cheapest: when an eviction reopen at the reported
firstlags again before any output, or when the cap is reached on an eviction, continue at the live tail.continue_after_gap(None)is enough. A tail cursor becomes primary and cannot lag. Keep the error for a cursor-ahead loop. - More complete: this guest change already needs a template rollout, so the guest could start a lagged connect at its oldest retained output under the journal lock and report where it started. That leaves no round trip between reading
first_sequenceand claiming it. - Either way, the cap test should expect tail output for evictions. A fake whose
firstadvances on every connect would pin it.
- Cheapest: when an eviction reopen at the reported
- How it fails.
-
New tests assert on metrics.
native/tests.rs:1688-1699asserts thedata_plane.attachresult:invalid_last_event_idtag, andrefused_attach_counts_no_replay_miss(:1702-1747) only asserts that a metric is absent. The repository's test rules (.agents/plugins/pplx-space/skills/space-code-quality/SKILL.md, "Tests") rule out unit tests that assert on metrics or metric tags. Keep the 400 status and no-backend-open checks, drop the metric assertions, and drop the metric-only 409 test. I asked for the metrics in round 1, not for tests of them; sorry for the confusion. -
One published bullet went stale this round.
public_docs.rs:68lists "one sent aftercursor_out_of_window" among ids that do not resume exactly. After an eviction gap, ids now keep their position, andnative/tests.rs:1108-1121resumes from the gap id with no resync. That also contradicts the bullet before it. Only a cursor-ahead or foreign-id restart, or the live tail on an older template, loses the position. Theprocess_frame_has_event_iddoc (native/sse.rs:886-891) makes the same claim, and the older-guest test atnative/tests.rs:1155-1158shows otherwise.
Worth deciding before merge
- The frame counter barely gets used now.
- Only output and
resync_requiredcarry ids, and every gap either moves the position or drops it. So a positional id's frame is only ever 0, or 1 forload'sPosition{0,1}. - The 8 frame bits still cost the field 7 bits. The published limit is 2^27 - 1 output chunks, about 3.7 hours at 10,000 writes a second.
- The no-position capacity is 2^35 ids whatever the split. Bit 52 is already the highest tag below 2^53, so changing the split after release would need a new tag scheme.
- Two frame bits would give 2^33 - 1 positions with no other change in behaviour. Your call; it's cheap now and expensive later.
- Only output and
Nits
scripts/tests/smoke-spaced-rest.shchecks only that ids increase, not which frames carry them (:639-643).ids_continuenow starts outtrue(:690), and the resumed stream of an exited process sends no ids, so that assertion cannot fail. The PR body says both smokes check which frames carry ids; only M1 does (m1_conformance.py:688-691). The no-resync and no-duplicate checks still catch regressions.- The const check says a stream "cannot run out" (
native/sse.rs:329), andnextsays "Always greater".Unknown{2^27-1, 255}.next()still wraps backwards in a release build. It is unreachable in practice, but the comments should state the finite headroom. - PR body and reply corrections:
- The reply says "300 of each on kind", but the Kind table only shows the 300-resume row.
- "As before" for older guests means as at the previous head; on
main, a mid-stream lag ended in anerror. - "Every process stream test also checks that only output and
resync_requiredcarry ids" holds only for tests that read the stream to its close.
ac-pplx
left a comment
There was a problem hiding this comment.
The problem
A native process session streams a command's output over SSE. On main, a client that reconnects loses what the process wrote while it was away. Without Last-Event-ID the stream silently jumps to the live tail. With it, the stream sends resync_required and then jumps to the tail. A node-side restart mid-stream replays stale output. The guest already keeps a bounded journal of each process's recent output, but the data plane never resumed from it, and its SSE ids carried no position.
Example
A command prints 1,000 numbered lines over 10 s; the client drops after line 100 and reconnects 2 s later.
main: no Last-Event-ID -> 731 lines, 1..269 missing, no warning
with Last-Event-ID -> resync_required, then the tail; 101..269 missing
this PR: Last-Event-ID = 4503599653584896 (output position 100)
-> lines 101..1000 exactly, no resync, ids keep increasing
Verdict
Approving. Everything from round 2 is fixed or deferred as agreed.
The busy-process reattach is fixed the right way. The guest picks the oldest retained output under the same journal lock that claims the primary-reader slot (process.rs:1057-1078), and reports that start in ProcessConnected.output_cursor (:2280). The data plane treats a start past its position as the gap (native/sse.rs:608-616). A stream that lags again before any output falls back to the live tail (:640-650).
Each hop compiles one operations.proto, so an old hop drops the flag and the reply field together, and the guest never moves the start without the data plane learning it.
The remaining items below don't block. Please do the PR body before enqueueing.
Round-2 items
| Item | Status |
|---|---|
1. Busy-process reattach ends in error |
Fixed |
| 2. Tests asserting on metrics | Fixed |
3. Stale OpenAPI bullet and process_frame_has_event_id doc |
Fixed (one related omission below) |
| 4. Frame-counter size | Fixed: 2 bits, positions for 2^33 - 1 outputs |
| REST smoke id checks | Fixed |
| Comments claiming ids cannot run out | Mostly fixed; first_after still says "a stream never runs out of blocks" (native/sse.rs:350-354) |
| PR body and reply corrections | Not done: the body is unchanged since round 2 (below) |
| Two-phase rollout, older-guest check, session-bound ids, backpressure docs, disconnect smoke coverage | Still deferred; fine |
Before merge
-
Update the PR body. It still describes the round-2 design:
- an 8-bit frame counter, a 28-bit field and the 2^27 - 1 limit;
- evictions ending in
errorafter 3 reopens; - the removed metric tests;
- "No schema ... changes", though
operations.protogainsConnectProcessRequest.resume_from_oldest_retainedandProcessConnected.output_cursor.
It doesn't describe the flag or its mixed-version behavior, and its validation is for
25561e4. Since the PR merges by squash, the body likely ends up as the commit message.
Follow-ups
- The live-tail fallback can happen on current guests too.
public_docs.rs:68says eviction is followed by the oldest retained output, "with live output on sandboxes from older templates".- A clamped reader is primary only if no other reader holds the slot and backpressure is enabled (
process.rs:1067-1072). Example: an earlier backend stream for the same process hasn't closed yet. Without the slot the reader can lag again, and the stream then follows the live tail (native/sse.rs:648). - Suggest "with live output when output keeps being evicted before the stream can read it, including on older templates".
- The proto comment (
operations.proto:245-246), the guestcursor()doc and the matching test doc say output written meanwhile "cannot" evict the start. Qualify them with "while the stream holds the primary reader's place". - The same bullet could also say that ids sent after a
cursor_replay_unavailablereplay don't resume exactly.
- Nothing tests the guest's reported start.
lagged_cursor_from_oldest_retained_starts_there_and_holds_it(process/tests.rs:1009) checks the journal clamp and backpressure, but nothing assertsconnect_process'soutput_cursor(process.rs:2280).- If that reported the requested cursor instead, the data plane would skip output without
cursor_out_of_windowand number later ids wrong, and every test would still pass. - One assertion in
spaced/guest/server/tests/it/process.rs(a connect from an evicted cursor reports the clamped start) would pin it.
- The cap reset is untested on the live-tail path.
- The eviction arm with a reported first no longer checks the cap. So
window_misses_with_output_between_are_not_capped(native/tests.rs:1407-1436) still passes if the reset atnative/sse.rs:893is deleted. - The live-tail arm does check the cap. A variant in which the fake lags without reporting a first sequence, with output between misses, would pin that output resets it there.
- The eviction arm with a reported first no longer checks the cap. So
|
thanks for the second pass. all addressed in 43971f6..329fcb8:
nits: the rest smoke now checks which frames carry ids and the dead |
A process stream that reattached never asked the guest for the output it retains. Without Last-Event-ID the backend cursor was unset, so a reattach after the process exited got only process_started and process_exited, and a reattach to a running process followed the live tail. With Last-Event-ID the stream resumed live behind resync_required(cursor_replay_unavailable). A backend reconnect after a reattach then passed a cursor counted from that attach's own frames, replaying stale output, and always sent resync_required(process_stream_reconnected). Process SSE ids now carry the guest journal position: (position + 1) * 1_000_000 + frames since output last advanced it. A reattach without Last-Event-ID replays from the start of the journal, a resume continues from the position its id carries, and a backend reconnect reopens at the position the client has reached. resync_required is sent only when the guest reports the requested output evicted (cursor_out_of_window, after which the stream follows the live tail), or when a reconnect has no known position. Ids stay increasing integers clients echo back; ids below the stride, as earlier data planes issued, keep the old live-tail resume. The events docs describe the new resume contract, and the M1 conformance and REST smokes assert it.
Process SSE ids are now tagged with bit 62 and keep their low 16 bits zero, above a 12-bit frame counter and a 34-bit position field whose top bit marks blocks of ids without a position. Ids from earlier data planes, ids an earlier plane counted on from by +1, and ids rounded through a double all fail to decode, so a resume from one replays the retained output behind resync_required(cursor_replay_unavailable) instead of reading a wrong position. Ids never repeat: running out of frames at one position, a position the field cannot hold, or losing the position after an eviction moves the stream to a fresh block without a position, which sorts above every position. The guest journal now ends a stream whose cursor is past the output the process has written with OUT_OF_RANGE, marked process-output-cursor-ahead, instead of waiting for output to reach it and skipping everything before. The data plane answers it with resync_required(cursor_out_of_window) and replays the output from the start. An attach that starts the process counts its output from the start even when the client sent another stream's Last-Event-ID, and a stream reopened after a gap no longer reports the process start again.
A resume from an id without a position, and an attach that started the process for a client holding another stream's id, replayed from the start of the guest output numbered from position 0, below ids the client had already seen. They now number those frames in a fresh block without a position above Last-Event-ID (positional ids from the start when the id is untagged, which sort above it already), so every id a resumed process stream sends is above Last-Event-ID. Both restarts and the cursor-ahead recovery share one path. A Last-Event-ID too close to the end of the id space to number a later frame above it is refused with 400. A backend reconnect after a route change no longer reports the process start a second time, as the gap reconnects already did not.
The guest now tags the RESOURCE_EXHAUSTED that ends a lagged process stream with `space-error-reason: process-output-lagged` and reports the oldest output it still retains in `space-process-output-first-sequence`. The data plane reopens there behind one `cursor_out_of_window` and keeps positional ids, so later resumes stay exact instead of replaying behind `resync_required`. A fresh attach to a process whose start was evicted now gets the retained output rather than only its exit. Only a tagged status, or a guest older than the tag sending its lag message, counts as evicted output; another RESOURCE_EXHAUSTED (a hop's refusal of an oversized chunk, an HTTP/2 reset) ends the stream with an error instead of jumping to the live tail. Reopens after a window miss are capped when no output follows, and one refused as a stale route goes through the route refresh path. Process SSE ids move their tag from bit 62 to bit 52: every id is below 2^53, so a JavaScript Number holds it exactly, with 16 guard bits, an 8-bit frame counter and a 28-bit position field. An id at or above 2^53 cannot have a later id and is refused with a 400. Each response now sends `process_started` once, even when its first backend stream fails before reporting the process. A gap reported once is not reported again before output follows it. The 400 is counted as an attach result, and `resync_required` is counted only once the stream opens. The node simulator fails a cursor past the output, as the guest does, and operations.proto documents both statuses.
The events_url schema text, the drain diagram and the API catalog still said streams do not replay missed output. The OpenAPI text now says a stream without Last-Event-ID starts at the beginning of the output, that evicted output is followed by the oldest output still retained, that ids stay below 2^53, and that an id is only valid for the session that sent it. The schema example uses ids in the current layout.
A resumed response spent a frame of its 8-bit counter on `process_started`, and each backend reconnect two, all at the client's position, so about 255 resumes or 100 reconnects against an idle process moved its ids out of positions for good. Frames that carry no output (`process_started`, `process_exited`, `backend_reconnecting`, `backend_reconnected`, `error`, `closed`) now go without an `id`, so an EventSource keeps the id of the last output it received. Only output and `resync_required`, which acknowledges a gap and so moves the position, consume ids. File-watch streams are unchanged. The OpenAPI text, the getting-started and troubleshooting docs, and both smokes say which frames carry ids. They also state that ids carry a position only for a process's first 2^27 - 1 output chunks.
process_started no longer carries an SSE id, so the data-plane restart scenario reads through the command's first output, the first frame with one, before taking its reconnect cursor.
A reader whose cursor was evicted never becomes the guest journal's primary reader, and without one the journal evicts its oldest output on every append once it is full. Reopening at the first sequence the lag status reported took a data-plane-to-guest round trip, so against a process that keeps writing that output was usually gone again: after three reopens the stream ended with a retryable error, and a retry from the gap's id repeated the chase, so the client got no output until the process slowed down or exited. ConnectProcessRequest gains resume_from_oldest_retained. With it, the guest starts a cursor before its oldest retained output at that output, chosen under the lock that also claims the primary reader's place, so output written afterwards waits for the reader instead of evicting it. ProcessConnected.output_cursor reports where the stream starts, and sandbox-manager forwards both fields. Older guests ignore the flag, and the ECW streams do not set it. The native data plane sets the flag on every connect and treats a reported start past its position as the eviction gap: one cursor_out_of_window, then the output from there, with no reopen. When the guest ends the stream as lagged instead, it still reopens at the reported first sequence. A stream already moved to the oldest retained output that falls behind again before any output follows the live tail rather than chasing it; only a cursor past the output, or a live tail that keeps falling behind, ends with an error once the reopen cap is reached.
Only output and resync_required carry ids, and every gap either moves the position or drops it, so a position never has more than two ids: the unsent start and the resync_required that begins a replay for an id without a position. The 8-bit frame counter therefore cost the position field 6 bits for nothing. Two frame bits leave ids a position for a process's first 2^33 - 1 output chunks (about 99 days at 1,000 writes a second) instead of 2^27 - 1 (about 37 hours). The blocks without a position still hold 2^35 ids, bit 52 stays the tag, and the 2^53 bound holds. Decided before the first release because the split cannot change after it without a new tag scheme. The OpenAPI text, first-sandbox.md and troubleshooting.md publish the new limit, and the example id in the process stream schema is the new encoding of position 1. The comments on the id layout now state its finite headroom: a stream has at least 2^34 ids after the block it starts in, and would wrap past them.
The repository's test rules rule out unit tests that assert on metrics or their tags. The 400 test keeps its status and no-backend-open checks without the attach-result tag, and the 409 test, which only checked that no replay-miss counter was recorded, is gone.
After an eviction gap a process stream keeps its position, at the oldest output retained, so a resume from the cursor_out_of_window frame's id, or from any later one, continues without a resync. The OpenAPI text still listed "one sent after cursor_out_of_window" among ids that do not resume exactly, which also contradicted the bullet before it. Only ids sent after the stream replayed the output from its start behind cursor_out_of_window (a Last-Event-ID past the output, another stream's id on the starting attach) or after it continued with live output lose the position. The doc comment on which frames carry ids made the same claim and now says the same.
The REST smoke checked only that ids increase. It now also checks that only stdout, stderr and resync_required frames carry one, on both connections. The check that the resumed stream's ids follow Last-Event-ID is gone: the resumed stream of an exited process sends no ids, so it could not fail. The no-resync and no-duplicate checks on the resume stay.
329fcb8 to
6e68697
Compare
ac-pplx
left a comment
There was a problem hiding this comment.
The problem
A native process session streams a command's output over SSE. On main, a client that reconnects loses what the process wrote while it was away: without Last-Event-ID the stream silently jumps to the live tail, and with it the stream sends resync_required and then jumps to the tail. The guest already keeps a bounded journal of each process's recent output, but the data plane never resumed from it, and its SSE ids carried no position.
Example
A command prints 1,000 numbered lines over 10 s; the client drops after line 100 and reconnects 2 s later.
main: no Last-Event-ID -> 731 lines, 1..269 missing, no warning
with Last-Event-ID -> resync_required, then the tail; 101..269 missing
this PR: Last-Event-ID = 4503599653584896 (output position 100)
-> lines 101..1000 exactly, no resync, ids keep increasing
Verdict
Approving. The rebase onto 9b1985d leaves the PR's own patch unchanged; only diff alignment and context differ. It also combines cleanly with what main changed in the same files:
- #795: every process-stream error path, including the new ones, sends the fixed
PROCESS_STREAM_FAILEDmessage, and the revocation rule stays insession_is_revoked. - #702: its untagged
OUT_OF_RANGEsize refusal can't be taken for the tagged cursor-ahead status. - #801 and #811: they don't reach the events route.
The items below don't block. license/cla shows pending on the rebased head.
Round-3 items
| Item | Status |
|---|---|
| Update the PR body | Done for the design; a few CONTRIBUTING.md rule issues remain (below) |
| Live-tail fallback on current guests in the docs | Not yet: public_docs.rs:74 and operations.proto:245-246 are unchanged |
| Test of the guest's reported start | Partly. My round-3 wording was too broad: spaced/guest/server/tests/it/process.rs:258-263 does assert the start, but only for an unclamped request at 0. A connect from an evicted cursor is still untested |
| Cap reset on the live-tail path | Not yet |
first_after "never runs out of blocks" (native/sse.rs:350-354) |
Not yet |
| Two-phase rollout, older-guest check, session-bound ids, backpressure docs, disconnect smoke coverage | Still deferred; fine |
Before merge (cheap)
docs/api/index.md:31still says "Streams are live, with no replay."docs/api/build.mjsrenders that page as the overview of the published API reference, right above the new OpenAPI text, so the two now contradict each other. The sentence predates this PR; I missed it in earlier rounds. Suggested wording: "Save output as it arrives. The sandbox retains a bounded amount of each process's output; reconnect withLast-Event-IDto resume after the last event, and check forresync_required."- The PR body breaks the "Submit a pull request" rules in
CONTRIBUTING.md(merged onmainsince the previous head):- It describes how the PR got here: "previous head", "this round", "It fails at the previous head". Say what merging does, and leave review history to comments.
- It cites branch-only commits (
25561e4,329fcb8) that won't be onmainafter the squash, and its validation is headed "At this head (329fcb8)" while the head is6e68697. - It names
infra/space/kind/scripts/bootstrap.sh, whichmainremoved in #735. - "No issue has been opened for these yet": open an issue for each follow-up and link it.
- Smaller points:
- "nothing written afterwards can evict it" (Summary) holds only while the stream holds the primary reader's place, as the next bullet says.
- The Kind log row
from: Some(0)applies only to the attempts withoutLast-Event-ID. - "On
mainthe same reattach follows the live tail without a resync" holds only withoutLast-Event-ID.
Follow-ups (carried from round 3)
- Live-tail fallback wording.
public_docs.rs:74: say live output also follows when output keeps being evicted before the stream can read it, not only on older templates.operations.proto:245-246, the guestcursor()doc and thelagged_cursor_from_oldest_retained_starts_there_and_holds_itdoc: qualify "cannot evict" with "while the stream holds the primary reader's place".
- Tests.
- A guest connect from an evicted cursor should report the clamped start, not the requested one; one case in
spaced/guest/server/tests/it/process.rswould pin it. - A live-tail variant of
window_misses_with_output_between_are_not_capped, where the fake lags without reporting a first sequence, would pin the reset atnative/sse.rs:893.
- A guest connect from an evicted cursor should report the clamped start, not the requested one; one case in
- Cleanup.
crates/libs/data/plane/server/Cargo.toml:67keeps thepplx-space-metricstest-capturedev-dependency the removed metric tests needed. Nothing in the crate uses it now;maindropped the same line in #795.
…rom-guest-journal # Conflicts: # crates/libs/data/plane/server/src/native/sse.rs # crates/libs/data/plane/server/src/native/tests.rs
#799 added the field to ConnectProcessRequest; the two guest tests from this PR built the request without it.
Summary
Merging this makes an HTTP (SSE) reattach to a native process session replay the output the guest still retains (up to 4 MiB per process), instead of losing it.
resync_requiredis sent only when the stream cannot continue exactly from where the client is. After evicted output, the stream continues from the oldest output still retained rather than jumping to the live tail, including against a process that keeps writing.Before this change, on
main:Last-Event-ID.sse.rsnever sent a cursor. An exited process came back as onlyprocess_startedandprocess_exited, with no stdout and noresync_required. A running one followed the live tail.Last-Event-ID. The stream only renumbered behindresync_required(cursor_replay_unavailable), and missed output was dropped.resync_required(process_stream_reconnected).What changes:
Event ids carry the guest journal position. Process SSE ids are tagged integers that a resume decodes, so any data-plane replica can serve it.
Number(or any double) holds it exactly. The field's top bit marks blocks of ids without a position; those blocks sort above every position.stdout,stderrandresync_required, which acknowledges a gap.process_started,process_exited,backend_reconnecting/backend_reconnected,errorandclosedcarry none, so an EventSource keeps the id of the last output it received. Resumes and backend reconnects against an idle process therefore spend no frames at its position. Output and every gap that keeps the position move it, so a position has at most two ids (the unsent start and theresync_requiredthat begins a replay for an id without a position). File-watch ids are unchanged.+1, fail to decode, so they are never read as a position. The exception is a count of exactly k × 65,536 frames (see Rollout).Last-Event-ID.Last-Event-ID. This covers a resume from an id without a position, a cursor past the output, and a foreign id on the attach that starts the process. For an untagged id, positional ids already sort above it.Last-Event-IDat or above 2^53, or too close to the end of the id space to number a later frame above it, is refused. Only a forged id can be that high. The refusal is counted as anattachresult (invalid_last_event_id).Resume behavior:
Last-Event-ID, the stream replays from the start of the output.Last-Event-ID, it continues at the position the id carries, with no resync. That includes an attach that finds the process already started but its binding never persisted.resync_required(cursor_replay_unavailable).resync_required(process_stream_reconnected)only when the stream has no known position.resync_required(cursor_out_of_window)once, then the oldest output the guest still retains, with ids that keep their position, so a resume from the gap's id is exact. The guest starts the stream there itself (see Guest), so this holds against a process that keeps writing: the reattach becomes the guest's primary reader at that output, and nothing written afterwards can evict it before it is read. A fresh attach to a process that wrote more than the window gets the retained output this way, not only its exit.main, a lag in the middle of a stream ended it with anerror.) A stream already moved to the oldest retained output that falls behind again before any output, which can happen when another reader holds the primary place, also follows the live tail rather than chasing the next oldest output.cursor_out_of_window, even if the client sent another stream'sLast-Event-ID.process_startedper response. It is sent once, even when the first backend stream fails before reporting the process. A gap is reported once until output follows it, so a cursor past the output whose start is also evicted sends onecursor_out_of_window.error. Evicted output never does. A reopen refused as a stale route (for example an expired route token) goes through the same route refresh as a backend reconnect. Each gap is logged with its cause (cursor_ahead,output_evicted,foreign_last_event_id).Guest (
process.rs, constants incommon/core):OUT_OF_RANGE, taggedspace-error-reason: process-output-cursor-ahead.ConnectProcessRequest.resume_from_oldest_retained(new, internal proto) starts a cursor before the oldest retained output at that output. The guest chooses the start under the journal lock that also claims the primary reader's place, and reports it inProcessConnected.output_cursor(new). The native data plane sets it on every connect; ECW streams do not. Sandbox-manager and the spaced host client forward both fields.RESOURCE_EXHAUSTEDthat ends a lagged stream is taggedspace-error-reason: process-output-lagged, withspace-process-output-first-sequencenaming the oldest retained cursor; the data plane then reopens there. It treats only a tagged status, or an untagged one with the older guest's lag message, as evicted output. Any otherRESOURCE_EXHAUSTED(a hop's refusal of an oversized chunk, an HTTP/2 reset, "replay state was evicted") ends the stream with a retryableerrorinstead of a jump to the tail.ConnectProcessRequestandProcessConnectedinoperations.proto. The node simulator fails a cursor past the output as the guest does, instead of clamping it, and reports the start it connects at.Docs. The OpenAPI text for
GET /v1/sessions/{session_id}/eventsand itsLast-Event-IDparameter (public_docs.rs,schema_contract.rs) now states:stdout,stderrandresync_requiredcarry ids;Last-Event-IDa stream starts at the beginning of the output; evicted output is followed bycursor_out_of_windowand the oldest output still retained;cursor_out_of_windowor continued with live output;Last-Event-ID, and whenresync_requiredis sent.The
DataPlaneAttachInfo.events_urlschema text (public/api), the drain diagram, the API catalog,first-sandbox.md,troubleshooting.mdandarchitecture.mdare updated to match.No public schema, enum or migration changes:
Last-Event-IDstaysint64, andcursor_out_of_windowwas already a published resync reason. The two new proto fields are on internal node and guest messages.m1_conformance.pyandsmoke-spaced-rest.shnow assert increasing (not consecutive) ids, that only output andresync_requiredcarry them, and no resync or duplicated output on a lossless resume. The data-plane restart scenario takes its reconnect cursor from the first output instead ofprocess_started. File-watch streams, WebSocket sessions and ECW are unchanged. This changes documented native-API behavior and targets the next 0.9 release.Validation
Rebased onto main at
9b1985d.At this head (
329fcb8):cargo fmt --checkpasses.cargo clippy --all-features --all-targets -- -D warningsonpplx-space-data-plane-server,pplx-space-api,pplx-space-spaced-guest-execution,pplx-space-spaced-guest-server,pplx-space-sbx-mgr,pplx-space-spaced-host-client,pplx-space-spaced-host-server,pplx-space-spaced-host-cli,pplx-space-node-api-sim,pplx-space-common-coreandpplx-space-public-apipasses.cargo nextest run --no-fail-fast --config 'profile.test.package.pplx-space-api.opt-level=0' --all-featureson those crates except the two guest crates, with Postgres and Redis in Docker: 4236 passed, 1 failed, 2 skipped. The failure (spaced-host-cliread_run_and_connect_preserve_bytes_without_waiting_for_process_exit) getsPermissionDeniedspawning a guest child as an unprivileged user; it passes run as root.pplx-space-spaced-guest-execution168 passed;pplx-space-spaced-guest-serverit52 passed, 1 failed. The failure,meta::bridge_guest_snapshot_restore_discards_recorded_diagnostics, reads process-wide diagnostics and passes on its own; it does not touch process streams.shim/protowas regenerated with the pinned protoc 35.0, protoc-gen-go v1.36.11 and protoc-gen-go-grpc v1.6.1;go build ./...andgo vet ./proto/...pass, and a secondgo generateleaves no diff.Tests:
native/tests.rs, against a fake guest journal that asserts every connect asks for the oldest retained output:Last-Event-IDresume; lossless backend reconnect after a resume;evicted_output_that_falls_behind_again_follows_the_live_tail: a fake whose oldest retained output moves past every cursor it is connected at, with and without the reported start. The stream gets tail output and noerror. It fails at the previous head;cursor_replay_unavailableon resume);RESOURCE_EXHAUSTEDthat is not a gap;error; output between misses resets it;process_started;Last-Event-IDpast the output, an untagged one, a foreign one on the starting attach,AlreadyExistsrecovery, and the 400 with no backend opened;idle_resumes_keep_positional_ids,idle_backend_reconnects_keep_positional_ids).resync_requiredcarry ids.native/sse/tests.rs, next to the now-private layout: the replay start at the layout edges, every following id staying aboveLast-Event-ID, ids increasing through frame saturation and lost positions, mangled ids rejected, every id printing back unchanged through a double, and positions up to 2^33 - 1 and none after, with the docs' example id decoding to position 1. A const check keeps the layout below 2^53.lagged_cursor_from_oldest_retained_starts_there_and_holds_it(the start is the oldest retained output, and later output waits for the reader instead of evicting it),cursor_past_the_output_fails_instead_of_waiting,lagged_cursor_reports_the_oldest_retained_output, andbridge_guest_process_connect_replays_from_output_cursornow checks the reported start. Sandbox-manager:process_event_to_spaced_keeps_the_connected_output_cursor. Spaced host client: the connect request keeps the flag. Simulator:connect_replays_the_journal_from_the_cursor_onlyexpects the cursor-ahead failure and the reported start.Mutation checks, one at a time, each restored and verified. All 15 Rust mutations made for this round failed at least one test, and the REST smoke's new check failed its own mutation on Kind: the guest ignoring the flag; the guest not claiming the primary place at the start it chose; the guest, sandbox-manager or simulator dropping the reported start; the host client dropping the flag; the data plane not setting it; the reported start ignored; the reopen at the oldest not marked (the chase then never ends); output not clearing that mark; an error instead of the tail after a reopen at the oldest falls behind again (both a broad and a narrow version); the tail and cursor-ahead reopens uncapped; and 8 frame bits. Earlier rounds' 18 mutations are unaffected by this round's changes. Sandbox-manager forwarding the flag has no unit test (neither does its existing cursor); the Kind run below covers it: without the flag, a busy reattach would reopen and follow the live tail, and it did neither.
End-to-end on a Kind cluster, recreated with
infra/space/kind/scripts/bootstrap.shfor each column, Firecracker, every SPACE image andseed:latestbuilt from that checkout:25561e4(this pull request's previous head) and329fcb8(this head). The data-plane and worker images are certified clean at329fcb8. The seed publish image is labelled329fcb8948a4-dirty-…only because I regeneratedshim/proto, byte-identical, while it was building. Its guest comes from the certified Rust artifact image.Reattach to a process that keeps writing after it has passed the window. The command runs
i=0; while [ $i -lt 100000000 ]; do i=$((i+1)); echo "line $i"; done. The client reads to line 100, drops the stream for 2 s, then reattaches and reads for 10 s. Three attempts each with and withoutLast-Event-ID:25561e4)attachedprocess_started,cursor_out_of_window,error(retryable, "process output stream fell behind retained output buffer") in 6 of 6process_started,cursor_out_of_window, then stdout from the first frame, in 6 of 6errorLast-Event-IDfrom: Some(0)to the reported start), no reopenOn
mainthe same reattach follows the live tail without a resync. That last row is from readingmain's code, not from a run.Also at this head, rerun after the id layout change. In every row there are no
errorframes, oneprocess_started, ids only on output andresync_required, and ids that increase, are all aboveLast-Event-ID, and are below 2^53. Each command printed 1000 numbered lines over about 10 s unless noted; the client dropped after 100 lines and reattached 2 s later:Last-Event-IDLast-Event-IDbackend_reconnecting/backend_reconnected, no resync, byte-exactNumber()round trip (4503599640477696, unchanged, position 50)cursor_out_of_window, then lines 164740..200000 contiguous toEND; last id positionalcursor_out_of_windowframe's idLast-Event-IDfar past a running process's outputcursor_out_of_windowand all 6 lines from the startLast-Event-ID: 1500000cursor_replay_unavailable, retained output replayedlast-event-id is past every event id this stream can sendprocess_started) against a process idle between two outputs, in 0.88 sTyped smokes:
scripts/tests/smoke-kind-typed.sh nativewithSPACE_SMOKE_KIND_ADMIN_TOKEN_FILEset, 26 passed, 0 failed. That includesnative.m1_generated_sdk_conformance,native_recovery.command_survives_data_plane_restart,native_recovery.attached_stream_drains_through_data_plane_restart,native_concurrency.slow_reader,native_concurrency.independent_commands,native_contract.permanent_command_errorandnative_contract.token_scope.scripts/tests/smoke-spaced-rest.shpasses, including its Last-Event-ID resume stage and the new check of which frames carry ids; that check fails whenstdoutids are treated as unexpected.Not validated:
Severity and risk
Severity: medium. A client that reattached to a process after a disconnect silently lost the output it missed. When
maindid sendresync_required, the output was gone anyway. A backend reconnect after a reattach could silently duplicate output.Risk:
4503599627632640instead of1, 2, 3), and only output andresync_requiredframes carry one. Clients that echo the last id received, as EventSource does, or parse it into a JavaScriptNumber, are unaffected. Clients that expected consecutive ids, or an id on every frame, must accept increasing ones on some frames.Last-Event-ID, or from an id without a position, replays retained output from the start, which can repeat output the client already has. The second case is flagged byresync_required.resync_required.resync_required. Documented in the OpenAPI text,first-sandbox.mdandtroubleshooting.md.RESOURCE_EXHAUSTEDnow ends the stream with a retryableerror(the client reattaches withLast-Event-ID) instead of reporting a gap and skipping to the tail.main's behavior. Ids issued by this version are then treated by the older data plane as plain counters, and get its usual live-tail resume behindresync_required.Rollout
Old and new data planes together. Nothing resumes at a wrong position, with one exception:
resync_required(cursor_replay_unavailable), numbered above that id.New guest with an old data plane. An old data plane does not decode ids or set
resume_from_oldest_retained. It sends no cursor on reattach, and on a backend reconnect a cursor counted from its own attach, which trails the real position, so it should not send a cursor past the output; this pairing was not run. The tagged lag status keeps the old code and message, so an old data plane treats it as before.Old node components. Sandbox-manager, the spaced host and node-api each decode and re-encode the connect request and the process events, so a component older than this change drops the flag and the reported start together. The guest then ends a lagged stream with the tagged status, and the data plane reopens at the reported first sequence. Against a busy process that reopen can fall behind again, and the stream then follows the live tail.
Old guest with a new data plane. The guest change reaches a sandbox only through a template built with this version.
Last-Event-IDpast the output: an older guest waits until the process writes past that cursor, skipping the output before it, or until it exits. Only a forged or foreign id gets there.Not covered
No issue has been opened for these yet.
docs/versioning.mdagainst a retained 0.1.x template before relying on exact positions there. Older guests replay a retriedStartfrom the output tail (ecw_process/stream.rs), so on the native lost-binding path positions would be counted from 0 on a stream that begins at the tail, and a later positional resume would duplicate output silently.Starton the new guest still replays from cursor 0 without the flag, so when that output was evicted it goes through the lag status and one reopen.resync_required(process_stream_reconnected).cursor_out_of_windowstill covers eviction, cursor-ahead and a foreign id on the starting attach; only the logs tell them apart.AI assistance: Claude Code drafted the code, tests, and this description; the author reviewed them.