docs(rfc-0003): resolve §9 process model + add RFC0003.16 served-binary scenario - #139
Conversation
…ry scenario Spec-first step for the served-socket-binary slice (the §9 process-model resolution). Resolves the "Receiver process model" open question — the receiver is a config-toggled role of the ourios-server binary (not a sidecar), sharing that binary's tokio runtime; default ports 4317 (gRPC) / 4318 (HTTP) per §6.1. Adds §5 scenario RFC0003.16 — the end-to-end served contract: both transports bind, a real OTLP client export over each socket round-trips (success only after the batch is WAL-durable), and shutdown is graceful (no mid-fsync Wal drop, no loss of an acked batch). §8 maps it to a real-socket integration test on ephemeral ports (the only scenario that crosses a socket; .1–.15 are in-process). This new acceptance criterion re-enters the ladder at `specified` (.1–.15 remain implemented; .16 is pending its red→green pass). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reached
More reviews will be available in 26 minutes and 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRFC 0003 moved to "specified" and added RFC0003.16 served-binary: an end-to-end real-socket acceptance contract (gRPC and HTTP) requiring WAL durability before transport success, graceful shutdown without losing acked batches, and a served-binary integration test using WAL::replay. Receiver is a role inside ChangesRFC0003 OTLP Receiver Specification
Possibly related PRs
Poem
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR amends RFC 0003 (OTLP receiver) to specify the “served-binary” slice: it resolves the §9 receiver process model (receiver as a config-toggled role within ourios-server) and adds a new end-to-end acceptance scenario that exercises real sockets and graceful shutdown semantics, regressing the RFC status back to specified until that scenario is implemented.
Changes:
- Regress RFC 0003 status from
green→specifieddue to the new not-yet-implemented acceptance criterion. - Add §5 Scenario RFC0003.16 defining the served-binary end-to-end contract (bind both transports, real client round-trip, WAL-before-ack, graceful shutdown).
- Resolve §9 “Receiver process model” and specify default ports 4317 (gRPC) / 4318 (HTTP), plus map RFC0003.16 to a real-socket integration test in §8.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - **Served binary** (RFC0003.16): an integration test boots the | ||
| `ourios-server` receiver role bound on ephemeral ports | ||
| (`:0`), exports a non-empty batch over each transport with a | ||
| *real* client — a `tonic` gRPC client and an HTTP client | ||
| (`reqwest`/`hyper`) — asserts transport success, then opens | ||
| the WAL and `replay`s to confirm each batch's `OtlpBatch` | ||
| frame is durable (WAL-before-ack over a real socket). A | ||
| second arm signals shutdown and asserts the server task joins | ||
| cleanly without losing an already-acked batch. Unlike | ||
| RFC0003.1–.15 (in-process: direct handler call / `oneshot`), |
There was a problem hiding this comment.
Good catch — fixed in the latest commit. Both the §5 scenario and the §8 bullet now make the durability check explicitly follow graceful shutdown: the server task joins (releasing the single-writer Wal handle, RFC 0008 §3.1), and only then does the test open the WAL and run Wal::replay. The WAL cannot be reopened while the server holds it, so replay-after-shutdown is the only correct ordering. Also switched the awkward "replays" to an explicit Wal::replay.
…s the WAL Review fix: the §5/§8 wording implied opening a second Wal for replay while the server still held the single-writer handle. The durability check necessarily follows graceful shutdown (which releases the Wal, RFC 0008 §3.1); reworded both, and reference Wal::replay explicitly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e-batch reject) The maintainer OTLP review confirms the all-or-nothing batch contract (§6.3 / RFC0003.4) is spec-compliant: OTLP mandates only 400 + no-retry for permanently-bad input and does not require accepting a valid subset; partial_success is supported but optional. Mark the §9 partial-success open question resolved (keep whole-batch reject; defer partial_success). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| > - **When** a real OTLP client exports a non-empty batch | ||
| > (resolvable tenant) over each bound socket — gRPC `Export` | ||
| > and HTTP `POST /v1/logs` (`application/x-protobuf`) — and | ||
| > the server is then signalled to shut down |
There was a problem hiding this comment.
Fixed in the latest commit — the When clause now states the client receives each transport response and only then is shutdown signalled (the steady-state export→ack→shutdown path), matching §8. In-flight-during-shutdown behaviour is explicitly out of scope for this scenario.
Review fix: the When clause was ambiguous about in-flight-during-shutdown. Make explicit that the client receives each transport response and only then is shutdown signalled (the steady-state export→ack→shutdown path), matching §8; in-flight-during-shutdown is out of scope for this scenario. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| - **Served binary** (RFC0003.16): an integration test boots the | ||
| `ourios-server` receiver role bound on ephemeral ports | ||
| (`:0`), exports a non-empty batch over each transport with a | ||
| *real* client — a `tonic` gRPC client and an HTTP client |
There was a problem hiding this comment.
Fixed — changed :0 to 127.0.0.1:0 (read back the OS-assigned port), which is a valid SocketAddr literal; the bare :0 was ambiguous.
Review fix: bare :0 is not a valid socket-address literal; the integration test binds 127.0.0.1:0 and reads back each OS-assigned port. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| > - **Given** `ourios-server` started with the receiver role | ||
| > enabled (config-toggled per the §9 resolution), the gRPC | ||
| > listener bound on its configured port (default 4317) and | ||
| > the HTTP listener on its (default 4318), both sharing one | ||
| > `IngestPipeline` over a single `Wal` |
There was a problem hiding this comment.
Addressed in #140 — the HTTP-listener Given clause now reads "bound on its configured port (default 4318)".
| > without dropping it mid-fsync or losing a batch that was | ||
| > already acked (no in-flight, already-fsync'd batch is lost | ||
| > on the way out) |
There was a problem hiding this comment.
Addressed in #140 — reworded to "no already-acked batch is lost on the way out", dropping the contradictory "in-flight, already-fsync'd" phrasing.
Red-gate step for the served-binary slice (docs/verification.md §3 two-loop): an #[ignore]'d ourios-server integration stub for RFC0003.16, greppable to the scenario, flipped by the implementation. Also folds in the two trailing Copilot prose nits on the RFC0003.16 scenario (from #139): the HTTP-listener Given clause now reads "bound on its configured port (default 4318)" (was missing the noun), and the shutdown bullet drops the contradictory "in-flight, already-fsync'd" phrasing for "no already-acked batch is lost on the way out". No semantic change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
#140) Red-gate step for the served-binary slice (docs/verification.md §3 two-loop): an #[ignore]'d ourios-server integration stub for RFC0003.16, plus the two trailing Copilot prose nits on the RFC0003.16 scenario from #139 (HTTP-listener Given clause noun; shutdown-bullet rewording). No semantic change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
What
Spec-first step for the served-socket-binary slice (the entry point chosen for the §9 work). Doc-only RFC 0003 amendment that:
ourios-serverbinary (not a separate sidecar), sharing that binary's tokio runtime alongside the other roles (e.g. the compaction daemon). Default ports 4317 (gRPC) / 4318 (HTTP) per §6.1 (was "TBD, likely…").Waldrop, no loss of an acked batch).Status
RFC0003.1–.15 are implemented and were
green(#138). Adding RFC0003.16 (a new, not-yet-implemented acceptance criterion) re-enters the ladder atspecified— honest per the maturity model. The implementation slice (red→green) restoresgreen.Why an amendment (not just code)
§9 was an explicit open question, and none of the existing §5 criteria covered "the served binary binds + a real client round-trips." Specifying it first matches how the receiver itself went (spec → red → green). Merging this PR is the spec approval; the implementation follows.
🤖 Generated with Claude Code
Summary by CodeRabbit