Skip to content

review: comment budget, declaration layout, request-path ownership - #48

Merged
CMGS merged 6 commits into
mainfrom
review/hygiene-2026-09-03
Sep 3, 2026
Merged

review: comment budget, declaration layout, request-path ownership#48
CMGS merged 6 commits into
mainfrom
review/hygiene-2026-09-03

Conversation

@CMGS

@CMGS CMGS commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Six review commits: comment budget, declaration layout, and ownership on the request path, first across the workspace and then over crates/handler. Behaviour is unchanged except where an allocation is removed; the one shutdown change is that the batch-drain task is now aborted with its six siblings instead of being dropped at process exit.

Commit Diff Comment lines
review: comment budget 6 files, +25 / −60 +25 / −60
review: layout 10 files, +366 / −357 +19 / −17
review: ownership on the request path 10 files, +118 / −84 +0 / −0
review: one-line docs in handler 4 files, +65 / −146 +65 / −146
review: layout in handler 1 file, +114 / −114 +10 / −10
review: borrow the realtime denial reason 2 files, +25 / −17 +0 / −0

review: comment budget

Twenty-one multi-line WHY blocks in crates/views/src/lib.rs collapse to one line each, keeping the fact and dropping the narrative. Same treatment for one block in crates/config/src/lib.rs, the section label and the private-fn /// in crates/server/src/main.rs, and the two-line godoc in control-plane/internal/user/user.go and internal/gateway/gateway.go. Both inline comments inside crates/models/src/cost.rs's test module are deleted. Every // SAFETY: line is untouched.

review: layout

Rust, declaration order only:

  • crates/config/src/lib.rs — the serde-default and hashing helpers (pick_variant, fnv1a, weight_one, default_priority, the six stability default_*, default_alert_dedup_seconds) move below the type+impl blocks, public free fn first. ProviderPreset is a private type, not a fn, so it moves with its sole consumer provider_preset to the file tail and the type region stays public-only.
  • crates/models/src/request.rsis_protected_anthropic_block moves below the type blocks.
  • crates/protocol/src/anthropic.rsblocks_text moves below the type blocks.
  • crates/engines/src/realtime.rsskip_scalar and delta_is_text move to the private tail. walk stays directly under its single caller visit_frame_text.
  • crates/engines/src/openai_engine.rs — the public merge_tool_call_fragments moves above the private apply_sse_event.

Go, the interface-parent/impl-subpackage split the module already uses for kv and user:

  • gateway.Client (the interface), the wire types and the request-id helpers stay in internal/gateway.
  • HTTPClient becomes http.Client in internal/gateway/http, constructed by New.
  • MockClient becomes mock.Client in internal/gateway/mock, constructed by New.
  • Callers in cmd/, internal/httpapi and internal/integration follow the existing kvmemory/userpostgres alias convention as gatewayhttp/gatewaymock.

The nineteen comment lines this commit adds are relocated lines; the net +2 is the two package docs the new packages require.

review: ownership on the request path

Constant wire fields become Cow<'static, str>: ChatMessage.role, Choice.finish_reason, ChatCompletionResponse.object, ChunkChoice.finish_reason and ReasoningParam.effort. finish_openai/finish_anthropic return Cow<'static, str> so a mapped constant reaches the wire borrowed. Deserialisation is unaffected: serde produces the owned form, so the request path allocates exactly as before.

user_erased_since built two owned Strings per probe and probed twice; erasures is now HashMap<String, HashMap<String, i64>> so the lookup borrows. image_to_image_url moves the url out of the owned source instead of copying it.

Two idiom fixes with no allocation delta: json!(name)/json!(args) on a &str become Value::from, dropping the to_value round-trip per tool-call fragment; MockTransport::ok_json encodes with serde_json::to_vec instead of Value::to_string, and the three unwrap_or(&vec![]) become map(Vec::as_slice).unwrap_or_default().

Entrypoint, per reload rather than per request: read_source_text is async and reads through tokio::fs, so a reload no longer blocks a runtime worker, and it returns Cow<'static, str> so the embedded default YAML is no longer copied on every reload. gw-server now declares tokio features = ["fs"] — the feature was only reaching it transitively. The batch-drain task keeps its JoinHandle and is aborted at shutdown like the other six background tasks, and 120 / 2s / 5s are named BATCH_STALE_SECS, BATCH_POLL and CONFIG_FEED_RETRY.

review: one-line docs in handler

Sixty-two multi-line /// docs across crates/handler collapse. Public items keep one line stating the contract; private fns and types keep at most one WHY line, and push_policies, whose doc only restated its name, loses it entirely. The four //! module docs go from three-to-five lines to two, dropping the design-rationale narrative. Test modules already carried no inline comments.

review: layout in handler

plugins.rs interleaved four type+impl blocks among its free fns. ScanCounts, SpanMasker, SignedThinking and EventFragments move up behind the public RuleHit and ScanOutcome, so the file reads consts → public types → private types → free fns → tests. Verified as a pure move: the sorted added and removed lines are identical.

review: borrow the realtime denial reason

RtModeration::Deny carries Cow<'static, str> — borrowed at the two 'static literal sites in moderate_rt, owned for a moderator-supplied reason.

The retype alone would have been allocation-neutral, because the reason immediately met rt_error's impl Into<String> and then a json! interpolation that deep-copies the owned String. So rt_error and rt_error_frame now take impl Into<Cow<'static, str>> and move the message into the envelope, and rt_inbound_policy carries the Cow through instead of collapsing it to a String. Every caller already passes a 'static literal, a format! result or an owned reason.

Per-request allocation accounting

String allocations removed, all on the response path:

Path Removed
chat completion, non-stream 3 (object, role, finish_reason)
chat completion, tool-call reply 3 (object, role, "tool_calls")
chat completion, stream 1 per stream (the finish frame)
anthropic stream, no engine finish_reason 1 per stream
request deriving effort from a thinking budget 1
request using the reasoning.enabled shorthand 1
anthropic to openai image block with a url source 1
MemoryStore::user_erased_since (batch drain) 4 per call
realtime frame denied by the scan or the moderator 1 per denied frame
every other realtime error frame 1 per frame

Per reload rather than per request, the embedded default config is no longer copied and the file read moved off the runtime worker.

Bench

cargo test --release -p gw-server --test bench -- --ignored --nocapture, run with the arms interleaved and the order swapped. No regression; the spread between rounds exceeds the difference between arms.

                              base(1)  head(1)  head(2)  base(2)
concurrent rps                 197651   193125   197248   204044
serial rps                      40052    41834    41783    45868
serial p50 / p95                23/31us  22/36us  22/28us  20/26us
big-payload p50 / p95          157/179  152/185  154/175  155/178
clone 48KB + 100KB raw         4.475us  4.357us  4.420us  4.340us

Gates

cargo fmt --all -- --check                              exit=0
cargo clippy --workspace --all-targets -- -D warnings    exit=0
cargo test --workspace                                   exit=0   555 passed, 0 failed, 3 ignored

The three ignored tests are the #[ignore]d benches above. The eleven GW_TEST_PG_URL / GW_TEST_REDIS_URL suites silently early-return without those variables set, so they were not exercised in this run.

control-plane, GOWORK=off:
make fmt-check                                exit=0
GOOS=darwin go vet ./...                      exit=0
GOOS=linux  go vet ./...                      exit=0
GOOS=darwin go vet -tags=integration ./internal/integration   exit=0
GOOS=linux  go vet -tags=integration ./internal/integration   exit=0
GOOS=darwin asl ./...                         exit=0
GOOS=linux  asl ./...                         exit=0
GOOS=linux  go build ./...                    exit=0
go test -race -count=1 ./...                  exit=0

The Postgres/Redis-backed internal/integration suite needs its docker compose stack and was not run; it compiles under go vet -tags=integration on both platforms.

CMGS added 6 commits September 3, 2026 14:10
Free helper fns move below the type+impl blocks and public above private;
HTTPClient and MockClient split into gateway/http and gateway/mock, matching
the kv and user interface-parent/impl-subpackage layout.
Constant wire fields (role, object, finish_reason, reasoning effort) become
Cow<'static, str>; the anthropic image url and the memory-store erasure lookup
stop copying; the entrypoint reads config through tokio::fs and keeps a handle
on the batch-drain task.
Type + impl blocks move ahead of the free fns in plugins.rs, public types first.
RtModeration::Deny carries Cow<'static, str>, and rt_error moves the message
into the envelope instead of letting json! deep-copy it.
@CMGS
CMGS merged commit 628c802 into main Sep 3, 2026
2 checks passed
@CMGS
CMGS deleted the review/hygiene-2026-09-03 branch September 3, 2026 06:52
CMGS added a commit that referenced this pull request Sep 3, 2026
01e6087 switched the mock transport's reply encoding from v.to_string() to serde_json::to_vec. A per-commit bisect of PR #48 on the testbed put the whole +88k instructions per 48 KB request of the big-body regression on that line (p3 4,039k vs p4 4,127k instr/req; restoring the file returns to 4,039k), while the four production groups of that commit run the same instruction stream within 0.01%. The mock transport is benchmark and demo code; production uses the HTTP transport. The remaining rps spread between builds is code placement, not added work.
CMGS added a commit that referenced this pull request Sep 3, 2026
01e6087 switched the mock transport's reply encoding from v.to_string() to serde_json::to_vec. A per-commit bisect of PR #48 on the testbed put the whole +88k instructions per 48 KB request of the big-body regression on that line (p3 4,039k vs p4 4,127k instr/req; restoring the file returns to 4,039k), while the four production groups of that commit run the same instruction stream within 0.01%. The mock transport is benchmark and demo code; production uses the HTTP transport. The remaining rps spread between builds is code placement, not added work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant