chore: july hardening - #211
Merged
Merged
Conversation
…nted benchmark, stale lint settings
…ed 8KiB, defeating Olla's 64KiB zero-check
…t wedge the endpoint
… now-dead Enhanced/AccessLoggingMiddleware
…not fully-defaulted ones
…sn't a silent no-op
CombinedLoggingMiddleware accepted a styledLogger but sent everything through slog.Default() instead, so a configured logger's level/theme/output had no effect on request logging.
…lue can't fail it
…earch order not the root copy
![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 `@config/profiles/llamacpp.yaml`:
- Around line 308-343: The model-size matching logic associated with the
llamacpp model_sizes patterns must avoid partial numeric matches such as 7b
matching 17b or 1b matching 11b. Update the matcher to parse the model parameter
count or enforce non-numeric boundaries, while preserving valid matches for the
configured sizes, and add regression coverage for model names containing 11B,
17B, 18B, and 27B.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b0287990-ba61-401d-8881-fb1febe65bf5
📒 Files selected for processing (20)
config/profiles/llamacpp.yamlconfig/profiles/ollama.yamldocs/content/configuration/reference.mdinternal/adapter/health/checker_test.gointernal/adapter/proxy/factory_defaults_test.gointernal/adapter/proxy/olla/service.gointernal/adapter/proxy/sherpa/service.gointernal/adapter/registry/profile/builtin_drift_test.gointernal/adapter/registry/profile/loader.gointernal/adapter/security/factory.gointernal/adapter/unifier/metadata_extractor.gointernal/adapter/unifier/metadata_extractor_test.gointernal/app/middleware/logging.gointernal/app/middleware/logging_test.gointernal/app/services/manager_test.gointernal/app/services/proxy_config_defaults_test.gointernal/config/shipped_config_test.gointernal/config/types.gointernal/core/constants/content.gointernal/core/domain/profile.go
🚧 Files skipped from review as they are similar to previous changes (14)
- internal/adapter/unifier/metadata_extractor_test.go
- internal/adapter/proxy/factory_defaults_test.go
- internal/adapter/health/checker_test.go
- internal/adapter/proxy/sherpa/service.go
- internal/adapter/unifier/metadata_extractor.go
- docs/content/configuration/reference.md
- internal/app/services/proxy_config_defaults_test.go
- internal/app/services/manager_test.go
- internal/adapter/proxy/olla/service.go
- internal/config/types.go
- internal/config/shipped_config_test.go
- internal/app/middleware/logging.go
- internal/adapter/registry/profile/loader.go
- internal/adapter/security/factory.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/eventbus/eventbus_stress_test.go (1)
134-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the test comments limited to the timing rationale.
Each comment describes the polling action and its reason. Remove the action description. State only why a fixed delay is unreliable or why the polling bounds are necessary.
pkg/eventbus/eventbus_stress_test.go#L134-L136: State why the drain time varies on loaded runners.pkg/eventbus/eventbus_test.go#L13-L15: State why the chosen bounds balance CI jitter and test duration.pkg/eventbus/eventbus_test.go#L378-L379: State why context cancellation needs asynchronous cleanup.pkg/eventbus/eventbus_worker_test.go#L55-L56: State why worker exit timing varies.pkg/eventbus/eventbus_worker_test.go#L107-L116: State why asynchronous publishing needs bounded observation.pkg/eventbus/eventbus_worker_test.go#L212-L215: State why delivery can lag after publishers finish.As per coding guidelines, comments must state why, not what.
🤖 Prompt for 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. In `@pkg/eventbus/eventbus_stress_test.go` around lines 134 - 136, Revise only the timing comments, removing descriptions of the polling or cleanup actions. In pkg/eventbus/eventbus_stress_test.go lines 134-136, explain why drain time varies on loaded runners; in pkg/eventbus/eventbus_test.go lines 13-15, explain how the bounds accommodate CI jitter while limiting test duration, and lines 378-379, explain why context cancellation requires asynchronous cleanup; in pkg/eventbus/eventbus_worker_test.go lines 55-56, explain why worker exit timing varies, lines 107-116, explain why asynchronous publishing requires bounded observation, and lines 212-215, explain why delivery may lag after publishers finish.Source: Coding guidelines
🤖 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 `@pkg/eventbus/eventbus_worker_test.go`:
- Around line 107-119: Update TestWorkerPool_HandlesBackpressure at
pkg/eventbus/eventbus_worker_test.go:107-119 and the related assertion at
pkg/eventbus/eventbus_worker_test.go:216-219 to measure drops from
WorkerPool.PublishAsync’s worker queue rather than
EventBus.Stats().TotalDropped, which only reports subscriber-channel drops.
Track or expose a queue-drop metric, or deterministically saturate the
subscriber channel before publishing, and assert that the relevant drop count
becomes positive.
---
Nitpick comments:
In `@pkg/eventbus/eventbus_stress_test.go`:
- Around line 134-136: Revise only the timing comments, removing descriptions of
the polling or cleanup actions. In pkg/eventbus/eventbus_stress_test.go lines
134-136, explain why drain time varies on loaded runners; in
pkg/eventbus/eventbus_test.go lines 13-15, explain how the bounds accommodate CI
jitter while limiting test duration, and lines 378-379, explain why context
cancellation requires asynchronous cleanup; in
pkg/eventbus/eventbus_worker_test.go lines 55-56, explain why worker exit timing
varies, lines 107-116, explain why asynchronous publishing requires bounded
observation, and lines 212-215, explain why delivery may lag after publishers
finish.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b686cfc-5c57-4594-be45-862d5dd7c235
📒 Files selected for processing (4)
.github/workflows/ci.ymlpkg/eventbus/eventbus_stress_test.gopkg/eventbus/eventbus_test.gopkg/eventbus/eventbus_worker_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
…der backpressure The high-volume test polled for 1000 delivered events after a 100k-event unthrottled storm, but PublishAsync is deliberately non-blocking and drops under backpressure - on a loaded runner (2 cores) delivery never reaches that floor, and once publishing finishes nothing is left in flight so Eventually just burns the full 10s ceiling before failing. Rewritten to assert what the bus actually promises: the publisher never blocks, the storm drains and stabilises, the bus is still alive and delivering afterwards, and the tracked counters stay internally consistent. Checked the other three tests converted to Eventually in a9855fd for the same flaw - they poll conditions that genuinely become true after the storm ends (goroutine teardown, ticker-driven cleanup, or delivery within buffer capacity), so left them as is.
…very is observable
…/lmdeploy/lemonade/vllm-mlx/dmr profiles
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/adapter/registry/profile/configurable_profile.go (1)
215-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the helper comment focused on the resource-selection risk.
Lines 215-216 restate the helper operation. Keep the reason that token boundaries prevent selecting an incorrect resource tier, and remove the implementation description.
Proposed comment change
-// containsSizeToken reports whether s contains token bounded by non-alphanumeric -// characters (or the start/end of s), rather than a bare substring match. This -// stops a size token like "7b" matching inside "17b" or "70b" while still -// matching legitimate forms such as "llama-7b", "7b-chat" or "codellama:7b". +// Size tokens must not select a larger model such as "17b" or "70b". +// Keep token boundaries so each model receives the correct resource tier.As per coding guidelines, “Comment on why, not what.”
🤖 Prompt for 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. In `@internal/adapter/registry/profile/configurable_profile.go` around lines 215 - 220, Update the comment for containsSizeToken to remove the implementation details about bounded tokens and matching examples. Keep only the rationale that correct token matching prevents selecting an incorrect resource tier, preserving the reference to resource-selection risk.Source: Coding guidelines
🤖 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 `@internal/adapter/registry/profile/configurable_profile_test.go`:
- Around line 409-417: Update the test loop around GetResourceRequirements to
iterate over every token in bucket.Patterns, rather than only
bucket.Patterns[0]. Build modelName and perform the existing MinMemoryGB
assertion inside that per-pattern loop, preserving the bucket-level validation
and diagnostic context.
In `@internal/adapter/registry/profile/configurable_profile.go`:
- Line 265: Prevent quantisation from mutating shared resource defaults in the
profile configuration flow around containsSizeToken: copy
p.config.Resources.Defaults before scaling it for an unmatched quantised model,
and use that copy for the model’s resources.defaults. Add a request-order test
covering an unmatched quantised model followed by an unmatched non-quantised
model, verifying the latter receives unscaled defaults.
In `@pkg/eventbus/eventbus_stress_test.go`:
- Around line 151-182: Update the stabilization assertion before closing done:
poll until received.Load() plus bus.Stats().TotalDropped and QueueDropped
exactly equals totalEvents, rather than using a quiet interval and LessOrEqual.
Only after this exact accounting condition is met should the test stop the
background drainer and perform the post-storm delivery probe.
In `@pkg/eventbus/eventbus_worker_test.go`:
- Around line 229-255: Update the observer goroutine around wp.Dropped to signal
completion through a dedicated channel when it returns. After close(stop), wait
for that completion signal before the test exits, ensuring the observer has
stopped and cannot overlap later tests.
---
Nitpick comments:
In `@internal/adapter/registry/profile/configurable_profile.go`:
- Around line 215-220: Update the comment for containsSizeToken to remove the
implementation details about bounded tokens and matching examples. Keep only the
rationale that correct token matching prevents selecting an incorrect resource
tier, preserving the reference to resource-selection risk.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f3dabb0-41a2-47f1-ab18-f563a38681d5
📒 Files selected for processing (12)
config/profiles/dmr.yamlconfig/profiles/lemonade.yamlconfig/profiles/lmdeploy.yamlconfig/profiles/sglang.yamlconfig/profiles/vllm-mlx.yamlconfig/profiles/vllm.yamlinternal/adapter/registry/profile/configurable_profile.gointernal/adapter/registry/profile/configurable_profile_test.gopkg/eventbus/eventbus.gopkg/eventbus/eventbus_stress_test.gopkg/eventbus/eventbus_worker.gopkg/eventbus/eventbus_worker_test.go
Same bug as the high-volume test: PublishAsync drops under an unthrottled storm by design, and once the storm ends those drops are permanent. A 2-core CI runner can't keep the receiver fed, so the 30% floor was never safe. Now it proves the bus doesn't wedge and reconciles delivered + tracked drops against what was published instead.
Blocks until every event queued via PublishAsync has actually run through bus.Publish, not just until eventChan looks empty - useful for graceful shutdown wanting to flush in-flight events, and gives tests a way to synchronise on delivery instead of polling for it. Also fixes a real race the new pending counter surfaced immediately: Add(1) was called after the channel send succeeded, so a worker could dequeue and call Done() before the sender's Add() ran, driving the counter negative under load. Add() now happens before the send.
Replaces wall-clock sleeps and delivery-floor polling with event- synchronised assertions now that Drain gives tests a real completion signal. Storm tests keep the storm but prove survival deterministically: publish, Drain (blocks until every queued event has actually run through Publish), then reconcile delivered+dropped exactly against what was published, wrapped in a hang-guard timeout rather than a success-condition one. Also found and fixed a latent hang risk while redesigning TestEventBus_ConcurrentSubscribers: subscriber goroutines looped reading until they'd received a quota of events with no way out if a starved runner never delivered enough, so wg.Wait() could block forever. They now stop on a signal fired once publishing finishes. Remaining require.Eventually uses are all ticker-driven or otherwise guaranteed-eventually-true by construction, with comments saying so.
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 `@pkg/eventbus/eventbus_test.go`:
- Around line 385-393: Update the cleanup test around the subscription setup to
keep its subscription context active while establishing the stale-timeout
condition; do not call cancel before the assertion, since Subscribe’s
cancellation watcher invokes eb.unsubscribe directly. Ensure the test reaches
the cleanupTicker-driven cleanupInactiveSubscribers path and retain the eventual
assertion that TotalSubscribers becomes zero.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3569f22b-f6af-4d5d-bcab-c9d843975ba1
📒 Files selected for processing (5)
pkg/eventbus/eventbus.gopkg/eventbus/eventbus_stress_test.gopkg/eventbus/eventbus_test.gopkg/eventbus/eventbus_worker.gopkg/eventbus/eventbus_worker_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/eventbus/eventbus_worker_test.go
…ead patterns it finds CodeRabbit flagged that the shipped-profile table test only built a model name from bucket.Patterns[0], so a broken later pattern (e.g. "72b") would never be caught. Iterating every pattern immediately surfaced a real one: dmr.yaml and lemonade.yaml both check a "7b/8b" bucket before their "1.7b" bucket, and "1.7b" contains "7b" on a word boundary (the "." counts as a separator), so 1.7B models were silently getting 7B-sized resource requirements. Reordered both profiles so the smaller bucket is checked first.
…ating the shared struct GetResourceRequirements pointed baseReqs straight at p.config.Resources.Defaults when no size bucket matched, then scaled MinMemoryGB/RecommendedMemoryGB/MinGPUMemoryGB on it in place for a quantised model name. That mutated the profile's shared defaults, so every later unmatched (or concurrent) caller saw the scaled-down numbers instead of the real defaults. Reproduced it with a two-call sequence before fixing: first call quantised, second call unquantised, second call returned the first call's scaled values. Now copies Defaults into a local struct before scaling. Added a sequential regression test and a concurrent -race one.
Subscribe's own cancellation watcher calls eb.unsubscribe directly as soon as ctx.Done() fires, so cancelling the context in TestEventBus_CleanupInactiveSubscribers let the assertion pass through that path instead of the ticker-driven cleanupInactiveSubscribers sweep the test is meant to exercise - confirmed by TestEventBus_ContextCancellation covering the same watcher already, and by the fixed test now taking ~100ms to pass (waiting out InactiveTimeout) rather than resolving near-instantly. Leaves the context live and lets lastActive go stale instead.
This was referenced Aug 8, 2026
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
1 participant
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.
Olla testing and hardening along with Alloy and FoundryOS work.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation