test(server): RFC 0019 localstack S3 e2e (.2–.5) + flip RFC 0019 green - #301
Conversation
Replace the `todo!()` stubs in `rfc0019_storage_backend.rs` with the four server-level acceptance tests, each spawning the `ourios-server` binary configured for the S3 backend against a LocalStack container and driving it over HTTP: - RFC0019.2 — WAL stays local while data lands on S3 (§3.6). - RFC0019.3 — ingest then query end to end on S3 (two-phase: drain on SIGTERM, then a fresh querier over the same bucket). - RFC0019.4 — background compactor consolidates a seeded small-file partition on S3 via the conditional-PUT manifest swap (polled, not slept). - RFC0019.5 — cross-tenant isolation: an alpha query never returns bravo's rows. The four are `#[ignore]`d and run only in the `s3 integration (localstack)` CI job (Docker + AWS_* env), invoked by name via `--ignored --exact`. Wire that second run step into the job, add the `testcontainers-modules` localstack dev-dep, and flip RFC 0019 `red` → `green` with the §6 testing strategy updated to map all seven scenarios to their landed tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 53 minutes and 59 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. 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)
📝 WalkthroughWalkthroughFour RFC 0019 S3 integration test scenarios ( ChangesRFC 0019 S3 Integration Tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
Adds RFC 0019 slice-3 server-level end-to-end acceptance coverage for the S3 backend using LocalStack/testcontainers, wires those ignored tests into the existing s3 integration (localstack) CI job, and flips RFC 0019 from red to green with updated test mapping documentation.
Changes:
- Replace RFC0019
.2–.5todo!()stubs with#[ignore]d LocalStack-backed e2e tests that spawnourios-serverconfigured for S3 and drive it over HTTP. - Update CI to run the four new ignored server S3 tests by name in the
s3 integration (localstack)job. - Mark RFC 0019
status: greenand update §6 to map scenarios to the concrete test locations.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/rfcs/0019-storage-backend-selection.md | Flip RFC status to green and document where each scenario is tested. |
| crates/ourios-server/tests/rfc0019_storage_backend.rs | Implement RFC0019.2/.3/.4/.5 LocalStack S3 server-level acceptance tests. |
| crates/ourios-server/Cargo.toml | Add testcontainers-modules dev-dependency for LocalStack-based tests. |
| Cargo.lock | Lockfile update for the new dev-dependency. |
| .github/workflows/ci.yml | Extend s3 integration (localstack) job to run the new ignored server S3 tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ext) The s3-integration run showed .3/.5 returning the correct rows (3 / 2) with correct tenant scoping, but their body-text asserts failed: a clean row's body renders empty because the read-time template registry is empty — the receiver never persists the miner's template_created audit events (issue #302), a cross-backend RFC 0017/0001 gap orthogonal to RFC 0019. RFC0019.3 wants 'the rows with pruning stats, the same result the local backend produces' and .5 wants tenant isolation; assert those (row count, stats present, no cross-tenant service.name leak), not body text. Body-text round-trip lands with #302. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot: http_post_query swallowed stream.flush() errors (hiding a broken/early- closed connection) — expect() it like http_post_logs; and seed_record reused the literal 9 instead of SEVERITY_INFO (converted, since MinedRecord's field is u8 vs the proto i32 constant). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ourios-server/tests/rfc0019_storage_backend.rs`:
- Around line 462-465: The storage backend tests are comparing json["rows"] as
if it were a scalar count, but the /v1/query response treats rows as the
returned row array. Update the affected assertions in the
rfc0019_storage_backend test cases to validate the length of the rows array
instead of the raw field value, and keep the surrounding round-trip/raw-response
checks aligned with the actual row objects returned by the query endpoint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d2cf199c-0f7a-4599-a313-3b971589708b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/ci.ymlcrates/ourios-server/Cargo.tomlcrates/ourios-server/tests/rfc0019_storage_backend.rsdocs/rfcs/0019-storage-backend-selection.md
* docs(rfc): accept RFC 0019 — storage-backend selection (maintainer sign-off) Promote RFC 0019 green -> accepted (terminal). All eight §5 criteria pass (green since #301, amended #306/#307): backend selection + credential scrub unit tests, and the localstack S3 integration covering WAL-stays-local, ingest→query on S3, the compactor manifest swap, and cross-tenant isolation. No validated stage applies — it's server wiring, not a thesis gate — so acceptance follows green directly (RFC 0001/0008 precedent). §9 records the sign-off. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * docs(rfc): sync §6 stage line with accepted status (RFC 0019) Copilot review on #455: §6 still said "the RFC is `green`" after the status flipped to `accepted` — internal inconsistency. Now "accepted (§9)". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
The RFC 0019 slice 3 — server-level end-to-end acceptance tests on real (localstack) S3 for the §5 scenarios, and the flip of RFC 0019 →
green. All four data seams (querier #292, compactor #297, receiver #298, audit #299) already run onStore; these tests prove the whole server does, end to end, on S3.Replaces the four
todo!()stubs intests/rfc0019_storage_backend.rswith#[ignore]d testcontainers + LocalStack tests that spawn theourios-serverbinary configured for the s3 backend and drive it over HTTP:OURIOS_WAL_ROOTand the S3 bucket hasdata//audit/objects but nowal/keys (§3.6).rows == 3+ each body present.OURIOS_COMPACTION_INTERVAL_SECS=1), poll the manifest (≤30s) until the partition collapses to one file withgeneration ≥ 2— the conditional-PUT (publish_cas) swap.Plus: testcontainers dev-dep on
ourios-server; thes3 integration (localstack)CI job runs all four via--ignored --exact;docs/rfcs/0019-storage-backend-selection.mdstatus: red → greenwith §6 mapping all seven scenarios to their tests.Validation
s3 integration (localstack)CI job (they need a Docker-API runtime +AWS_*env; LocalStack accepts any creds). They were authored to compile + clippy clean and#[ignore]cleanly in the defaultcargo test. Thegreenflip rests on this PR's s3-integration job passing — that job is the gate.Verified locally:
cargo fmt --all --check,cargo clippy --workspace --all-targets --all-features -D warnings,cargo build -p ourios-server --tests,RUSTDOCFLAGS=-D warnings cargo doc -p ourios-server,cargo test -p ourios-server --test rfc0019_storage_backend(4 ignored, skip cleanly), unitrfc0019_1/6/7(3 passed),mdbook build— all green.Invariants
§3.6 (WAL never on object storage) is the subject of
.2; §3.7 (tenant isolation) of.5; §3.5 manifest conditional-PUT of.4. No code change — tests + CI + RFC status only.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation