[APMSVLS-501] refactor(bottlecap): preparatory work for testmode binary

[lucaspimentel]

Overview

Preparatory refactors for the upcoming bottlecap-testmode binary (APMSVLS-501), which reuses TraceAgent / handle_traces but has no Lambda lifecycle to drive. Three logical changes:

1. InvocationProcessorHandle::noop()

Constructor backed by a background task that acknowledges every ProcessorCommand with a sensible default so callers never block on their response oneshots:

• Request-response commands (GetReparentingInfo, UpdateReparenting, SetColdStartSpanTraceId, PlatformRuntimeDone, PlatformReport) reply with empty/default values.
• Fire-and-forget commands are dropped silently.

The match is exhaustive: adding a new ProcessorCommand variant 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 receive ProcessorError::ChannelReceive instead of the intended default.

The noop channel capacity matches the real InvocationProcessorService to avoid backpressure surprises.

2. Promote start_trace_agent to a top-level library module

Moved the start_trace_agent helper from the private bin/bottlecap/main.rs into a new public module, bottlecap::startup, so callers outside the Lambda binary can construct the full trace-processing pipeline through a single call instead of duplicating ~110 lines of wiring. Placed at the crate root (not under traces/) because the module wires trace, stats, proxy, config, lifecycle, tags, appsec, and flushing pieces together: it is cross-cutting orchestration, not a trace-domain API. No behavior change; the Lambda binary's call site is unchanged (same function name, same signature, same internal spawn).

The companion TraceAgentPipeline return type is a pub struct with named pub fields (trace/stats/proxy flushers, the trace-channel sender, the shutdown token, and the aggregator/concentrator handles), rather than a positional tuple.

3. RouterExtension seam on TraceAgent

Adds a generic extension point that lets a caller merge additional axum routes into the trace-agent's HTTP router without TraceAgent knowing what those routes do. When no caller attaches an extension (the Lambda binary case), the HTTP surface on port 8126 is unchanged.

• New pub trait RouterExtension: Send + Sync { fn extend(&self, router: Router) -> Result<Router, Box<dyn Error>>; } defined in traces::trace_agent. Returning Err aborts agent startup and surfaces the error in the existing log path, preventing silent failures from a panicking or misconfigured extension.
• TraceAgent holds router_extension: Option<Arc<dyn RouterExtension>> with a consuming builder method with_router_extension(self, ext: Arc<dyn RouterExtension>) -> Self.
• make_router calls extension.extend(router)? when the field is set, before applying the outer fallback and body-limit layers.
• Split the library helper in startup.rs into build_trace_agent (returns an unspawned TraceAgent plus the TraceAgentPipeline handles) and a thin start_trace_agent wrapper that spawns. Callers that need to attach a RouterExtension before startup use build_trace_agent; the Lambda binary's call site continues to use start_trace_agent.
• A #[cfg(test)]-only SpyExtension validates the seam end-to-end: the trait shape compiles, state is carried via Arc, and merged routes are reachable through the composed Router returned by make_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 bottlecap
• cargo check --lib
• cargo test --lib
• cargo clippy --workspace --all-targets --features default -- -D warnings
• cargo fmt --all -- --check
• cargo nextest run --workspace (532/532 passed)
• New unit tests on 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_timeout
• New unit tests on the RouterExtension seam: with_router_extension_adds_reachable_route_to_make_router, make_router_returns_404_for_extension_route_when_none_attached

"Why did the noop handler go to therapy? Because it had to acknowledge every command without ever really doing anything." — Claude 🤖

[Copilot]

Pull request overview

Preparatory refactors to enable an upcoming bottlecap-testmode binary to reuse the trace-processing pipeline (TraceAgent / handle_traces) without relying on the Lambda lifecycle, and to optionally expose a deterministic flush hook for test harnesses.

Changes:

• Added InvocationProcessorHandle::noop() backed by a background task that acknowledges all ProcessorCommand variants with sensible defaults (plus unit tests).
• Promoted start_trace_agent wiring from the Lambda binary into a reusable library module (bottlecap::traces::startup), including a build_trace_agent variant that returns an unspawned TraceAgent.
• Added an opt-in POST /flush route on the trace-agent listener when a FlushingService is attached via TraceAgent::with_flushing_service.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
bottlecap/src/traces/trace_agent.rs	Adds optional FlushingService plumbing and conditionally registers POST /flush.
bottlecap/src/traces/startup.rs	Introduces shared library helpers to build/spawn the trace-agent pipeline.
bottlecap/src/traces/mod.rs	Exposes the new startup module and re-exports pipeline helpers/types.
bottlecap/src/lifecycle/invocation/processor_service.rs	Adds InvocationProcessorHandle::noop() plus tests to ensure defaults/non-blocking behavior.
bottlecap/src/bin/bottlecap/main.rs	Switches Lambda binary to use the new library start_trace_agent helper and removes duplicated wiring.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 723059ecf6

ℹ️ 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".

[duncanista]

this is binary specific code, not traces component related code, you could create a file/module in the bottlecap binary folder instead of here, those methods are supposed to be binary specific, this breaks that pattern

[duncanista]

Review

Appreciate the thorough PR description and the preparatory structure here. The noop() constructor and build_/start_trace_agent split are solid ideas. However, there are several Rust style and structural issues worth addressing before this merges, especially since this is establishing patterns that the test-mode binary will build on.

---

1. TraceAgentPipeline — 8-element tuple as public API

pub type TraceAgentPipeline = (
    Sender<SendDataBuilderInfo>,
    Arc<trace_flusher::TraceFlusher>,
    Arc<trace_processor::ServerlessTraceProcessor>,
    Arc<stats_flusher::StatsFlusher>,
    Arc<ProxyFlusher>,
    CancellationToken,
    StatsConcentratorHandle,
    TraceAggregatorHandle,
);

In Rust, tuples beyond 3-4 elements are a code smell. At every call site, someone destructures by position — if the order changes, or two fields happen to share a type, it silently breaks. Now that this is being promoted from a private function's return type to a pub type alias in the library, it should be a struct with named fields:

pub struct TraceAgentPipeline {
    pub trace_channel: Sender<SendDataBuilderInfo>,
    pub trace_flusher: Arc<trace_flusher::TraceFlusher>,
    pub trace_processor: Arc<trace_processor::ServerlessTraceProcessor>,
    // ...
}

Named fields are self-documenting, order-independent, and play nicely with IDE autocompletion. The original code had this problem, but promoting it to library API is the right time to fix it.

---

2. startup.rs doesn't belong in traces/

(Expanding on Jordan's inline comment.)

build_trace_agent imports from config, lifecycle, tags, traces, appsec, and flushing — it touches nearly every module in the crate. It's application-level wiring/orchestration, not a traces component. Placing it in traces::startup misrepresents its role and breaks the pattern where traces/ contains trace-domain code.

The existing pattern is that each binary owns its startup wiring in bin/<name>/main.rs. If sharing between the Lambda and test-mode binaries is truly needed, consider:

• A top-level bottlecap::startup module (not nested under a domain module)
• Or a shared helper in bin/ if both binaries live in the same crate

Re-exporting via pub use startup::{...} from traces::mod.rs further blurs the boundary — callers see bottlecap::traces::start_trace_agent and assume it's trace-domain API.

---

3. flush handler — over-engineered timeout logic

The current code (after accepting Copilot's suggestion):

Err(_) => {
    task.abort();
    match task.await {
        Err(join_err) if join_err.is_cancelled() => StatusCode::GATEWAY_TIMEOUT,
        Err(_) => StatusCode::INTERNAL_SERVER_ERROR,
        Ok(()) => StatusCode::GATEWAY_TIMEOUT,
    }
}

After task.abort(), the await will always resolve to Err(JoinError::Cancelled) (the Ok(()) arm is only reachable in a vanishingly small race window, and even then the right status is still 504). The inner match has 3 arms that all effectively return the same thing. Simplify to:

Err(_) => {
    task.abort();
    StatusCode::GATEWAY_TIMEOUT
}

We don't need to await after abort — we already know the status code. Simpler code is also more correct code because there are fewer branches to reason about.

---

4. Noop tests — reach through internals instead of using the public API

The third test (noop_platform_runtime_done_and_report_respond_without_blocking) is 50+ lines that manually construct ProcessorCommand::PlatformRuntimeDone and ProcessorCommand::PlatformReport, sending them through handle.sender directly. But InvocationProcessorHandle already has public methods for both:

• handle.on_platform_runtime_done(...)
• handle.on_platform_report(...)

The test should use those. Testing through the public API is more resilient to internal refactors (e.g., if the channel type changes) and is more readable — the reader immediately understands what behavior is being tested rather than having to mentally map command construction to handler behavior. It also avoids the 15+ lines of field construction for each command variant.

---

5. Doc comments leak consumer knowledge into library functions

build_trace_agent's doc references "bottlecap-testmode binary" and "POST /flush route" repeatedly. Library functions should document what they do and why a caller would use them, not who specifically calls them. Consumer-specific guidance belongs in the consumer's code. For example:

"Returns an unspawned TraceAgent so the caller can configure it (e.g., attach optional routes) before spawning."

is better than:

"notably the bottlecap-testmode binary, which uses it to back a POST /flush route"

---

6. flushing_service: Option<Arc<FlushingService>> on TraceAgent

Adding a test-only field to a production struct is pragmatic for one field, but worth flagging as a pattern to not repeat. Every TraceAgent in production carries a None that exists solely for test-mode. If more test-mode hooks are needed later, consider a trait or a router-extension pattern instead of accumulating Option fields.

---

Summary

Area	Suggestion
TraceAgentPipeline	Replace 8-tuple with a named struct
startup.rs location	Move out of traces/, it's orchestration not trace-domain code
flush timeout	Simplify the post-abort match to just return 504
Noop tests	Use handle.on_platform_runtime_done() / on_platform_report() instead of raw channel sends
Doc comments	Remove consumer-specific references from library function docs
flushing_service field	Fine for now, but flag as not-to-repeat pattern

Happy to discuss any of these — the overall direction is good, these are about getting the Rust patterns right before the test-mode binary builds on top.

[lucaspimentel]

@duncanista, all six items addressed across six commits on top of 605933a:

1. TraceAgentPipeline tuple → struct with named pub fields (6586fc5). Destructure by name at the call site, so reordering or same-typed-field swaps can't break silently.

2. startup.rs moved out of traces/ to a new top-level bottlecap::startup (80e7752). The Lambda binary calls bottlecap::startup::start_trace_agent now, no longer reaching into a domain module; the re-export from traces::mod.rs is gone.

3. POST /flush timeout arm collapsed to task.abort(); 504 (b043eb8). The redundant await-after-abort and the unreachable 500 branch are gone; a panic still surfaces through the outer Ok(Err(_)) arm as 500.

4. Noop test uses handle.on_platform_runtime_done / on_platform_report (962f394) instead of constructing ProcessorCommand variants by hand. About 15 lines of per-variant field plumbing removed.

5. Consumer-specific references stripped from library doc comments (5fa5bb2). No more bottlecap-testmode or POST /flush route naming in build_trace_agent, with_router_extension, InvocationProcessorHandle::noop, etc. The library docs describe behavior, not today's caller.

6. flushing_service field replaced with a RouterExtension trait (043d991, bc31322). Swapped the Option<Arc<FlushingService>> field for a single generic slot Option<Arc<dyn RouterExtension>> with pub trait RouterExtension { fn extend(&self, router: Router) -> Router; }. TraceAgent no longer imports or references FlushingService; the flush handler and FLUSH_ENDPOINT_PATH const are gone. Future test-mode hooks become new trait impls (or default-implemented methods added to the trait), not new Option fields. A cfg(test) test builds a real TraceAgent, attaches a spy extension via with_router_extension, and verifies the route is reachable through make_router; a second test confirms the None path still falls through to 404.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[Copilot]

build_trace_agent is a public library API but it uses .expect("Failed to create trace HTTP client"), which will panic on common misconfigurations (invalid proxy URL/cert file, IO errors). Consider returning a Result<(TraceAgent, TraceAgentPipeline), Box<dyn Error + Send + Sync>> (and similarly making start_trace_agent return Result<TraceAgentPipeline, _>) so callers can handle startup failures without aborting the process.

[Copilot]

InvocationProcessorHandle::noop() spawns a Tokio task internally. If this constructor is called outside of an active Tokio runtime, tokio::spawn will panic at runtime. Please either document this requirement explicitly in the doc comment (e.g., "must be called from within a Tokio runtime") or provide an alternative constructor that takes a tokio::runtime::Handle (or is async) to avoid surprising panics for library callers.