[APMSVLS-501] refactor(bottlecap): preparatory work for a new "test-mode" binary - #1344
Open
lucaspimentel wants to merge 36 commits into
Open
[APMSVLS-501] refactor(bottlecap): preparatory work for a new "test-mode" binary#1344lucaspimentel wants to merge 36 commits into
lucaspimentel wants to merge 36 commits into
Conversation
The forthcoming bottlecap-testmode binary (APMSVLS-511) reuses TraceAgent and handle_traces but has no Lambda lifecycle to drive. Adds a noop constructor that spawns a background task acknowledging every ProcessorCommand with a sensible default, so callers never block on their response oneshots. The match is exhaustive: a new ProcessorCommand variant forces a compile error here, keeping test-mode behavior explicit rather than silently dropping responses.
…isfy clippy 🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
Moves the start_trace_agent helper from the private bin/bottlecap/main.rs into a new public module, bottlecap::traces::startup, so callers outside the Lambda binary (notably the forthcoming bottlecap-testmode binary) can construct the full trace-processing pipeline through a single call instead of duplicating ~110 lines of wiring. No behavior change. The Lambda binary's call site is unchanged: same function name, same signature, same internal spawn. 🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
…ode /flush hook Adds an opt-in POST /flush route to the trace-agent listener on 8126, backed by a caller-supplied FlushingService. The route is only registered when the caller calls TraceAgent::with_flushing_service; otherwise the Lambda binary's HTTP surface is unchanged. - Add an optional flushing_service field to TraceAgent with a consuming builder method with_flushing_service(self, fs) -> Self. - In make_router, conditionally merge a /flush sub-router that calls FlushingService::flush_blocking_final when the field is set. - Split the library helper in src/traces/startup.rs into build_trace_agent (returns an unspawned TraceAgent plus the pipeline handles) and a thin start_trace_agent wrapper that spawns. Test-mode will use build_trace_agent so it can attach its FlushingService before spawning; Lambda's call site is unchanged. - No behavior change for the Lambda binary: it never attaches a flushing service, so /flush remains unexposed and start_trace_agent still spawns internally. Preparatory for APMSVLS-511 (bottlecap-testmode binary). 🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
build_trace_agent returns (TraceAgent, TraceAgentPipeline), a 2-tuple whose complexity is hidden behind the type alias — the lint never fires. 🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
Callers using build_trace_agent need the return type alias accessible via the same crate::traces path as the function itself. 🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
Wrap flush_blocking_final in a spawned task so panics return 500 instead of tearing down the handler, and add a 30-second timeout so a stuck flush returns 504 rather than hanging the test harness. 🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
Add a test that sends PlatformRuntimeDone and PlatformReport commands directly through the noop handle's internal channel and verifies the response oneshots are fulfilled without blocking. 🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
Tuples beyond 3-4 fields are fragile: every call site destructures by position, and any reordering (or two fields of the same type swapping) breaks silently. Now that the pipeline is a pub API type shared between the Lambda binary and future test-mode binary, promote it to a struct with named pub fields. Addresses review feedback on PR #1201.
This module wires trace, stats, proxy, config, lifecycle, tags, appsec, and flushing pieces together. It is cross-cutting orchestration, not trace-domain code, so placing it under traces/ misrepresents its role and makes callers treat bottlecap::traces::start_trace_agent as if it were a trace-domain API. Promote it to bottlecap::startup at the crate root so both the Lambda binary and the forthcoming test-mode binary can consume it without reaching into a domain module. Addresses review feedback on PR #1201.
After task.abort(), the awaited JoinError is effectively always Cancelled, and all three arms of the post-abort match returned (or should have returned) 504 for the test harness. The extra match adds branches to reason about without changing the result, and the lone Err(_) -> 500 arm is unreachable in practice (a panic surfaces through the Ok(Err(_)) branch of the outer timeout). Drop the await-after-abort and return 504 directly. Addresses review feedback on PR #1201.
The test previously built ProcessorCommand::PlatformRuntimeDone and PlatformReport by hand and pushed them through handle.sender, making the reader mentally map raw command construction to the handler behavior and coupling the test to the channel internals. Use the already-public InvocationProcessorHandle methods, which exercise the same code path, drop ~15 lines of field plumbing per variant, and stay resilient to future refactors of the command channel. Addresses review feedback on PR #1201.
Library doc comments named the bottlecap-testmode binary and the POST /flush route it registers. Library APIs should document what they do and why a caller might reach for them, not which specific caller today happens to. Rewrite the affected doc comments (and one internal impl comment) to describe behavior and motivation generically: "a deterministic on-demand drain hook", "a handle that has no Lambda lifecycle state to drive", and so on. Addresses review feedback on PR #1201.
…ension trait
The flushing_service: Option<Arc<FlushingService>> field was test-mode
scaffolding on the production struct. Every TraceAgent in production
carried a None that existed solely for a future test binary, and any
additional test-mode hook would have had to add another Option field
with the same shape, each pulling another consumer-specific dependency
into trace_agent.rs.
Replace it with a single generic extension seam:
- pub trait RouterExtension { fn extend(&self, router: Router) -> Router; }
- TraceAgent stores Option<Arc<dyn RouterExtension>> and exposes
with_router_extension(self, ext) -> Self in place of
with_flushing_service.
- make_router calls extension.extend(router) when set, before applying
the outer fallback and body-limit layers.
- FLUSH_ENDPOINT_PATH, the flush handler, and the crate::flushing
FlushingService import are removed from trace_agent.rs. The trace-
agent module now knows nothing about FlushingService.
Future test-mode hooks add new methods (with defaults) to the trait or
new impls behind it, not new Option fields on TraceAgent.
A #[cfg(test)]-only SpyExtension validates the seam end-to-end: trait
shape compiles, state is carried via Arc, and merged routes are
reachable through the composed Router.
Addresses review feedback on PR #1201 (deferred comment 6).
The previous SpyExtension test called `extension.extend(Router::new())` directly, which only proves axum's Router::merge works. Build an actual TraceAgent via a small test helper, attach the spy via with_router_extension, and hit /spy through the full router returned by make_router. Add a second test confirming that with no extension attached, /spy falls through to the 404 handler. Also trim the "deterministic drain endpoint" example from the with_router_extension and start_trace_agent doc blocks; keep it only in the trait doc to avoid repetition across three locations.
…comment Dropping a oneshot sender causes the receiver to return ProcessorError::ChannelReceive, not a hang. Update the comment to describe the actual observable behavior. 🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
…omment The previous note mentioned only the three tasks spawned directly in build_trace_agent (aggregator, concentrator, dedup). TraceAgent::new unconditionally spawns a fourth task: the trace-payload drain loop. Update the summary line and the leak warning to reflect all four tasks. 🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
…rrors Previously, a panic or error inside extend() would propagate as a panic through make_router() and start(), then be silently dropped by the tokio::spawn wrapper in start_trace_agent (which only logged Err returns, not panics). Changing the return type to Result<Router, Box<dyn std::error::Error>> means implementors must return errors explicitly. Those errors propagate through make_router via ?, then through start() to the spawn wrapper, where they are already logged. 🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
… timeout The exhaustive match in noop() prevents unhandled variants at compile time, but it cannot prevent a future contributor from placing a response-carrying variant in the fire-and-forget arm. In that case, rx.await returns ProcessorError::ChannelReceive, and the test would hang rather than fail cleanly. Add a test that wraps every request-response call in a 500ms timeout so any such regression produces a clear failure instead of a hung test suite. 🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
The RouterExtension::extend doc claimed errors abort agent startup, but the production caller start_trace_agent only logs and discards the error from TraceAgent::start; the surrounding pipeline keeps running with a dead trace channel. Soften the trait doc to describe this honestly, extend the start_trace_agent doc to point callers needing reactive error handling at build_trace_agent, and add a FailingExtension test that locks in the Err propagation contract through make_router. 🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
…ode feature The noop constructor exists for tests and the upcoming testmode binary but was a public function on a struct used pervasively in production code, so nothing prevented a future caller from reaching for it from main. Add a test-mode cargo feature (matching the existing fips pattern) and gate noop with cfg(any(test, feature = "test-mode")) so it does not exist in default or fips builds. 🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
The constructor calls tokio::spawn to start its draining task, which panics if there is no Tokio runtime registered for the current thread. Make that requirement explicit in the doc so future callers do not hit a surprise panic. 🤖 Co-Authored-By: Claude Code <noreply@anthropic.com>
Restores start_trace_agent to the Lambda binary and drops the bottlecap::startup library module. The extraction has no consumer in this PR: build_trace_agent was called only by start_trace_agent in the same file, so the split and the public module were unjustifiable here. Both move to the PR that adds the bottlecap-test-mode binary, where the second [[bin]] target imports them and the shared-library placement is self-evident. Keeps the RouterExtension seam and InvocationProcessorHandle::noop(), which are exercised by tests in this PR.
`Box<dyn Error>` is not `Send`, so the documented "spawn the agent yourself if you need to observe startup failures" pattern did not compile. Widen `RouterExtension::extend`, `TraceAgent::start`, and `make_router` to `Box<dyn Error + Send + Sync>`, which still converts into `Box<dyn Error>` via `?` for existing callers. Also document that extension routes carry no request body limit, and drop a dead `#[allow(clippy::unwrap_used)]`. 🤖
The compiler only catches *new* `ProcessorCommand` variants; adding a
`response` field to an existing fire-and-forget variant still matches
its `{ .. }` pattern and silently drops the sender.
Also remove a test fully subsumed by the timeout-wrapped variant.
🤖
This was referenced Aug 27, 2026
|
🔗 Commit SHA: b7bfcb6 | Docs | View more details | Give us feedback! |
Contributor
There was a problem hiding this comment.
Pull request overview
Prepares Bottlecap for a future lifecycle-free test-mode binary.
Changes:
- Adds a feature-gated no-op invocation processor with default responses.
- Adds an extensible TraceAgent router seam.
- Adds focused unit tests for both additions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
bottlecap/Cargo.toml |
Defines the test-mode feature. |
bottlecap/src/lifecycle/invocation/processor_service.rs |
Adds and tests the no-op processor handle. |
bottlecap/src/traces/trace_agent.rs |
Adds and tests router extensions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
lucaspimentel
changed the title
lucaspimentel
marked this pull request as ready for review
August 27, 2026 16:35
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR: refactoring here first to prepare for #1216 and keep that PR slightly smaller.
Part of a PR stack:
bottlecap-test-modebinary #1216Overview
Preparatory refactors for the upcoming
bottlecap-testmodebinary (APMSVLS-501), which reusesTraceAgent/handle_tracesbut has no Lambda lifecycle to drive. Two logical changes:The
bottlecap::startupextraction (build_trace_agentand the publicstartupmodule) lands in #1216 alongside thebottlecap-test-modebinary that consumes it, where it has a caller; this PR keeps its scope to the two seams below and leavesmain.rsandlib.rsuntouched.1.
InvocationProcessorHandle::noop()Constructor backed by a background task that acknowledges every
ProcessorCommandwith a sensible default so callers never block on their response oneshots:GetReparentingInfo,UpdateReparenting,SetColdStartSpanTraceId,PlatformRuntimeDone,PlatformReport) reply with empty/default values.The
matchis exhaustive: adding a newProcessorCommandvariant will cause a compile error here, forcing test-mode behavior to be decided explicitly. A response-carrying variant placed in the fire-and-forget arm would silently drop its sender, causing the caller to receiveProcessorError::ChannelReceiveinstead of the intended default.The noop channel capacity matches the real
InvocationProcessorServiceto avoid backpressure surprises.2.
RouterExtensionseam onTraceAgentAdds a generic extension point that lets a caller merge additional axum routes into the trace-agent's HTTP router without
TraceAgentknowing what those routes do. When no caller attaches an extension (the Lambda binary case), the HTTP surface on port 8126 is unchanged.pub trait RouterExtension: Send + Sync { fn extend(&self, router: Router) -> Result<Router, Box<dyn Error>>; }defined intraces::trace_agent. ReturningErraborts agent startup and surfaces the error in the existing log path, preventing silent failures from a panicking or misconfigured extension.TraceAgentholdsrouter_extension: Option<Arc<dyn RouterExtension>>with a consuming builder methodwith_router_extension(self, ext: Arc<dyn RouterExtension>) -> Self.make_routercallsextension.extend(router)?when the field is set, before applying the outer fallback and body-limit layers.#[cfg(test)]-onlySpyExtensionvalidates the seam end-to-end: the trait shape compiles, state is carried viaArc, and merged routes are reachable through the composedRouterreturned bymake_router.Any concrete flush/drain/diagnostic endpoint lives behind an impl of this trait in the consumer crate, not in
trace_agent.rs.Testing
cargo check --bin bottlecapcargo check --libcargo test --libcargo clippy --workspace --all-targets --features default -- -D warningscargo fmt --all -- --checkcargo nextest run --workspace(532/532 passed)InvocationProcessorHandle::noop():noop_request_response_methods_return_defaults,noop_fire_and_forget_commands_do_not_panic,noop_platform_runtime_done_and_report_respond_without_blocking,noop_request_response_variants_complete_within_timeoutRouterExtensionseam:with_router_extension_adds_reachable_route_to_make_router,make_router_returns_404_for_extension_route_when_none_attached