Repository navigation
test: process-level fault-injection harness and the contract tests that pass today - #1473
duncanista wants to merge 3 commits into
Conversation
…act tests that pass today
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0e4939146
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
|
|
||
| s.intake.set_mode(FaultMode::Status(503)); | ||
| let (released, took) = end_invocation_and_wait(&s, "req-1", 2, secs(10)).await; |
There was a problem hiding this comment.
/next against the invocation deadline
When retries consume nearly the full invocation window, this test can still pass after Lambda's deadline: the event gets a 10-second deadline in Scenario::invoke, but end_invocation_and_wait starts its timer only after both telemetry requests complete and then grants another 10 seconds here. Measure elapsed time from when the INVOKE is queued, or compute the remaining absolute deadline, so a late /next is detected.
Useful? React with 👍 / 👎.
| self.api.push_shutdown("spindown", secs(2)); | ||
| bc.wait_for_exit(bottlecap_process::EXIT_BUDGET).await |
There was a problem hiding this comment.
If the extension takes between 2 and 8 seconds to terminate, every shutdown_cleanly assertion still passes even though the generated SHUTDOWN event advertises only a 2-second deadline and Lambda may terminate the environment at that point. Derive the wait budget from the event deadline, or separately assert that exit occurred within two seconds, so these lifecycle tests catch shutdown overruns.
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
duncanista
changed the title
- fault_injection_test: measure the /next budget from when the INVOKE was queued - harness: shutdown_cleanly asserts the exit came within the SHUTDOWN deadline
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51f541581a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .await | ||
| .unwrap(); | ||
| eprintln!("trace agent answered {status} to a 4 MB span"); | ||
| let (released, _) = end_invocation_and_wait(&s, "req-1", 2, invoked, secs(15)).await; |
There was a problem hiding this comment.
In oversized_trace_is_dropped_and_later_traces_flow, both INVOKE events advertise a 10-second deadline, but the test allows /next up to 15 seconds after invocation. If rejected-trace processing completes between 10 and 15 seconds, released is true even though Lambda would already have terminated the invocation, so this contract test misses the regression; use the 10-second event deadline for these waits.
Useful? React with 👍 / 👎.
| ) | ||
| .await; | ||
|
|
||
| s.intake.set_reject_over(None); |
There was a problem hiding this comment.
The test removes the size rejection immediately after observing the first 413 and never checks whether that large request is sent again. If a regression starts retrying 413 responses, the queued 4 MB trace can be accepted after this line and the test still passes once the separate small trace arrives, despite its stated dropped-not-redriven contract; record the rejected request and assert that no subsequent large trace request occurs.
Useful? React with 👍 / 👎.
| bc.log_tail(40) | ||
| ); | ||
|
|
||
| s.shutdown_cleanly(&mut bc).await; |
There was a problem hiding this comment.
shutdown_cleanly triggers the extension's final blocking shutdown flush before these intake assertions are evaluated. If the end-of-invocation flush regresses and /next is called without sending the buffered logs or metrics, the shutdown flush can deliver them and this test still passes, so inspect or snapshot the intake before sending SHUTDOWN.
Useful? React with 👍 / 👎.
- oversized trace: wait for /next against the 10 s INVOKE deadline, and assert the rejected trace is never sent again after the rejection is lifted - lifecycle: read the intake before SHUTDOWN so the final flush cannot hide a missing end-of-invocation flush
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 399b90b8e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| s.intake.set_mode(FaultMode::Ok); | ||
| let exit = s.shutdown(&mut bc).await; |
There was a problem hiding this comment.
When the intake returns a truncated 200 response, this test only observes that the fault served a request; after clearing the fault here, it shuts down without checking that the request was classified as failed. In src/logs/flusher.rs:117-132, the body-read result is discarded and a 2xx is still accepted, so the current data-loss behavior passes despite the test's stated contract that truncation is a failed request; assert a retry or another customer-visible recovery signal before shutdown.
Useful? React with 👍 / 👎.
| s.api.push_invoke("req-1", secs(5)); | ||
| if deliver_runtime_done(&s.api, "req-1", true).await.is_ok() { | ||
| flushed = s.api.wait_for_next_calls(2, secs(10)).await; |
There was a problem hiding this comment.
When a held listener makes end-of-invocation processing take more than five seconds, this helper can still pass: the INVOKE advertises a five-second deadline, but the test waits ten seconds and stores the result in flushed only for diagnostic output. Even if that wait expires, the subsequently queued SHUTDOWN may be consumed by a still-late /next, so the final exit assertion succeeds without enforcing the invocation contract; wait only for the remaining five-second budget and assert flushed before shutdown.
Useful? React with 👍 / 👎.
| // A connection accepted in blackhole mode is never read. | ||
| if *accept_shared.mode.lock().expect("mode lock") == FaultMode::Blackhole { | ||
| accept_shared.held.lock().expect("held lock").push(stream); | ||
| continue; |
There was a problem hiding this comment.
When the global mode is Blackhole and a specific path has an Ok or status override, the accept loop holds the socket before reading the HTTP request, so effective_mode never sees the path and the documented override is ignored. This prevents scenarios such as keeping a credentials endpoint available while blackholing intake traffic; only apply the accept-level blackhole when no path override needs routing, or read enough of the request to select the effective mode.
Useful? React with 👍 / 👎.
TL;DR: Tests that run the real
bottlecapbinary against a fake Lambda API and an intake that fails on purpose. This PR adds the harness and the 10 checks that pass today.Why: Our worst bugs (hangs past the invocation deadline, SIGKILL at shutdown, lost data on intake errors) show up only at the process level, so unit tests cannot catch them. Each later fix adds its own test here.
Overview
Jira: SVLS-9978
Stacked on #1472 (
jordan.gonzalez/test-fixtures/feature).Contract tests that run the real
bottlecapbinary against a fake Lambda control plane and aDatadog intake that fails on demand. Each test asserts what Lambda or the customer sees:
/nextbefore the deadline, exit on SHUTDOWN, no panic. Seebottlecap/tests/HARNESS.md.tests/common/: fake Extensions and Telemetry API, fault intake (blackhole, SYN blackhole,slow, 5xx, 429, close mid body, size limits), process wrapper with RSS and CPU, port guard.
maintoday: invoke then shutdown exitscleanly; a 503 storm does not hold
/next; an oversized trace is dropped and later tracesflow; an intake that closes mid body does not hold
/next; 4 config fields with a wrong typecost only that field; ports 8124 and 8126 held by another process.
bottlecap/.config/nextest.toml: the 4 binaries run one test at a time (fixed-portsgroup), because the binary binds fixed ports.
No production code and no Cargo change. The tests for the open bugs come in a later PR, each
ignored with the backlog item that fixes it.
Size: 20 core lines, 2184 lines in total.
Testing
cargo fmt --all -- --check: passcargo clippy --workspace --all-targets --features default -- -D warnings: passcargo clippy --workspace --all-targets --features default,test-mode -- -D warnings: passcargo nextest run --workspace: passcargo test --test <binary>for the 4 harness binaries: pass#[ignore], no backlog reference: pass