chore(server): mentor capacity observability + dead-code cleanup - #1089
Closed
FelixTJDietrich wants to merge 1 commit into
Closed
chore(server): mentor capacity observability + dead-code cleanup#1089FelixTJDietrich wants to merge 1 commit into
FelixTJDietrich wants to merge 1 commit into
Conversation
- frame_ring.dropped_total{userId} counter with debounce + cardinality cap (50 users)
- interactive_sandbox.evicted_total{reason} counter
- MAX_LIFETIME reaper wired into InteractiveSandboxRegistry; previously dead enum value
- mentor.chat.outcome{CAPACITY_EXCEEDED} label distinguishes cap-rejections from errors
- ArchUnit: agent.sandbox.spi.* may not depend on Micrometer
- docs/contributor/agent/observability.mdx documents the new metrics
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR extends the interactive sandbox layer with structured capacity-exceeded error handling, max-lifetime session eviction, and refactored metrics infrastructure featuring per-user drop tracking with debounce. It introduces a new exception type, Clock-based reaping logic with testable interfaces, decouples ring-buffer drop counting from Micrometer, and adds comprehensive observability documentation. ChangesInteractive Sandbox Capacity, Lifetime, and Metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR introduces multiple interlocking features: structured exception hierarchy, Clock-based time abstraction with testable seams, refactored metrics infrastructure with debounce logic, and registry eviction policy changes. The metrics refactoring (FrameRingBuffer + InteractiveSandboxMetrics) is dense with concurrent state management (ConcurrentHashMap, AtomicLong CAS). Broad test coverage includes deterministic Clock implementations and synthetic targets. Architectural boundary enforcement (SPI no-Micrometer rule) adds verification. The changes span capacity management, eviction scheduling, and metrics instrumentation with moderate logic density and cross-file dependencies requiring careful coordination. Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
📚 Documentation Preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/InteractiveSandboxRegistryTest.java (1)
60-127: ⚡ Quick winAdd exact-threshold tests (
== maxLifetime,== idleTtl) to lock in boundary semantics.Current cases validate strictly-over-threshold eviction. Adding equality cases will prevent off-by-one policy regressions and document intended
>behavior explicitly.🤖 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 `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/InteractiveSandboxRegistryTest.java` around lines 60 - 127, Add tests in InteractiveSandboxRegistryTest that assert equality-to-threshold does NOT trigger eviction (policy is strict '>'). Create two new tests (e.g. evictsAtMaxLifetime and evictsAtIdleTtl) that use clockOf/nowRef, buildRegistry, and propertiesWith to set maxLifetime and idleTtl, construct FakeReapTarget instances whose age or idle duration is exactly equal to the configured thresholds, call registry.reapInternal(...) and assert the target.terminatedWith is null (not EvictionReason.MAX_LIFETIME or EvictionReason.IDLE). Ensure you reuse FakeReapTarget, reapInternal, InteractiveSandboxProperties, and EvictionReason symbols so the tests mirror existing ones but check the == boundary.server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/mentor/chat/MentorChatServiceTest.java (1)
360-396: ⚡ Quick winAssert scope-specific error text in the two capacity tests.
These tests prove the metric outcome, but they don’t verify the per-user vs global user-facing message split that was just introduced. Please assert the emitted error payload text for each scope so this contract can’t regress silently.
🤖 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 `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/mentor/chat/MentorChatServiceTest.java` around lines 360 - 396, Add assertions in the two failing tests (runTurn_perUserCapExceeded_recordsCapacityExceeded and runTurn_globalCapExceeded_recordsCapacityExceeded) to verify the emitted error payload text matches the scope-specific message from the thrown InteractiveSandboxCapacityExceededException; after calling runTurnSync() and verifying persistence.interrupt(...), inspect the test emitter (use emitter.recordedPayloads() or the equivalent recorded payload accessor) and assert it contains "Per-user session cap exceeded" for Scope.PER_USER and "Per-replica session cap exceeded" for Scope.GLOBAL so the user-facing message contract is enforced alongside the outcome metric assertion (keep the existing verify and assertOutcomeRecorded calls).server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/FrameRingBufferTest.java (1)
66-66: ⚡ Quick winAlign the new test name with
should...When....
throwingCallbackIsSwallowedshould follow the standard naming convention for unit tests in this module.As per coding guidelines:
server/application-server/src/test/java/**/*.java:Use should[ExpectedBehavior]When[Condition] naming convention for test methods.🤖 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 `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/FrameRingBufferTest.java` at line 66, Rename the test method throwingCallbackIsSwallowed in FrameRingBufferTest to follow the module convention, e.g. shouldSwallowThrowingCallbackWhenInvoked (or similar should[ExpectedBehavior]When[Condition] form); update the method name on the test (and any references or test runners) so the test still runs and keep the implementation and `@Test` annotation unchanged.server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/SandboxArchitectureTest.java (1)
58-58: ⚡ Quick winUse
should...When...naming for the new architecture test method.The new method name is clear, but it doesn’t match the configured test naming convention.
As per coding guidelines:
server/application-server/src/test/java/**/*.java:Use should[ExpectedBehavior]When[Condition] naming convention for test methods.🤖 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 `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/SandboxArchitectureTest.java` at line 58, Rename the test method spiHasNoMicrometerDeps in class SandboxArchitectureTest to follow the should[ExpectedBehavior]When[Condition] pattern (e.g., shouldHaveNoMicrometerDependenciesWhenInspectingSpi or similar), update its method declaration name accordingly, and ensure any references (IDE run configurations or annotations) remain consistent after the rename so the test still runs.server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/FrameRingBufferMetricsTest.java (1)
31-31: ⚡ Quick winRename test methods to the repository’s
should...When...convention.Current method names are descriptive, but they don’t follow the enforced naming pattern for tests under
src/test/java.As per coding guidelines:
server/application-server/src/test/java/**/*.java:Use should[ExpectedBehavior]When[Condition] naming convention for test methods.Also applies to: 57-57, 94-94, 114-114
🤖 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 `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/FrameRingBufferMetricsTest.java` at line 31, Rename the test method debouncesWithinOneSecondWindow in FrameRingBufferMetricsTest to follow the repository convention should[ExpectedBehavior]When[Condition], e.g. change debouncesWithinOneSecondWindow to shouldDebounceWhenWithinOneSecondWindow; also apply the same renaming pattern to the other test methods referenced (lines 57, 94, 114) so each becomes should<ExpectedBehavior>When<Condition> (use clear verbs like "Debounce", "Drop", "Aggregate" as appropriate) and update any related `@Test` annotations or references to the method names accordingly.
🤖 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 `@docs/contributor/agent/observability.mdx`:
- Around line 14-35: Update the metric documentation to use the actual emitted
tag value: change the header and any inline references from
"mentor.chat.outcome{outcome=CAPACITY_EXCEEDED}" to
"mentor.chat.outcome{outcome=capacity_exceeded}" and update the explanatory text
to mention outcome=capacity_exceeded (keeping the guidance to distinguish it
from outcome=error); locate the metric block for mentor.chat.outcome and adjust
the tag value casing to snake_case to match other documented tag values like
reason=idle.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/DockerAttachedSandboxAdapter.java`:
- Around line 408-412: The metrics.evictDropDebounce(userId, sessionId) call is
only executed on the success path in DockerAttachedSandboxAdapter and can be
skipped if runClose(...) throws; move the debounce-entry cleanup into a finally
block so it always runs (even on exceptions). Update the try/catch in the method
that calls runClose(...) inside DockerAttachedSandboxAdapter to call
metrics.evictDropDebounce(userId, sessionId) from a finally block, and apply the
same change to the other similar block (around lines handling the other runClose
path referenced in the comment) so both code paths always evict the
per-(user,session) debounce entry.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/InteractiveSandboxMetrics.java`:
- Around line 60-66: The unbounded ConcurrentHashMap userDroppedCounters in
InteractiveSandboxMetrics can grow indefinitely; replace it with a bounded cache
(e.g., Caffeine Cache<String, Counter> with a sensible maximumSize and
expireAfterAccess) and change lookups to use cache.get(key, k ->
createCounterForUser(k)) or cache.asMap() where computeIfAbsent was used, so
counters auto-evict and you maintain thread-safety and lazy creation; apply the
same replacement to the other per-user counter map used later in the class (the
per-user counter block at lines ~212-219) so both caches are size-capped and use
the same eviction policy.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/spi/AttachedSandbox.java`:
- Around line 54-63: The default createdAt() implementation on AttachedSandbox
currently returns Instant.now() and masks missing SPI implementations; change
the contract so AttachedSandbox.createdAt() is abstract (remove the default
method) so implementors must provide a stable creation instant, or alternatively
replace the default with a fail-fast implementation that throws
UnsupportedOperationException referencing createdAt() to surface missing
overrides; update all classes implementing AttachedSandbox to supply a concrete
createdAt() that captures and returns the creation Instant.
---
Nitpick comments:
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/mentor/chat/MentorChatServiceTest.java`:
- Around line 360-396: Add assertions in the two failing tests
(runTurn_perUserCapExceeded_recordsCapacityExceeded and
runTurn_globalCapExceeded_recordsCapacityExceeded) to verify the emitted error
payload text matches the scope-specific message from the thrown
InteractiveSandboxCapacityExceededException; after calling runTurnSync() and
verifying persistence.interrupt(...), inspect the test emitter (use
emitter.recordedPayloads() or the equivalent recorded payload accessor) and
assert it contains "Per-user session cap exceeded" for Scope.PER_USER and
"Per-replica session cap exceeded" for Scope.GLOBAL so the user-facing message
contract is enforced alongside the outcome metric assertion (keep the existing
verify and assertOutcomeRecorded calls).
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/FrameRingBufferMetricsTest.java`:
- Line 31: Rename the test method debouncesWithinOneSecondWindow in
FrameRingBufferMetricsTest to follow the repository convention
should[ExpectedBehavior]When[Condition], e.g. change
debouncesWithinOneSecondWindow to shouldDebounceWhenWithinOneSecondWindow; also
apply the same renaming pattern to the other test methods referenced (lines 57,
94, 114) so each becomes should<ExpectedBehavior>When<Condition> (use clear
verbs like "Debounce", "Drop", "Aggregate" as appropriate) and update any
related `@Test` annotations or references to the method names accordingly.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/FrameRingBufferTest.java`:
- Line 66: Rename the test method throwingCallbackIsSwallowed in
FrameRingBufferTest to follow the module convention, e.g.
shouldSwallowThrowingCallbackWhenInvoked (or similar
should[ExpectedBehavior]When[Condition] form); update the method name on the
test (and any references or test runners) so the test still runs and keep the
implementation and `@Test` annotation unchanged.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/InteractiveSandboxRegistryTest.java`:
- Around line 60-127: Add tests in InteractiveSandboxRegistryTest that assert
equality-to-threshold does NOT trigger eviction (policy is strict '>'). Create
two new tests (e.g. evictsAtMaxLifetime and evictsAtIdleTtl) that use
clockOf/nowRef, buildRegistry, and propertiesWith to set maxLifetime and
idleTtl, construct FakeReapTarget instances whose age or idle duration is
exactly equal to the configured thresholds, call registry.reapInternal(...) and
assert the target.terminatedWith is null (not EvictionReason.MAX_LIFETIME or
EvictionReason.IDLE). Ensure you reuse FakeReapTarget, reapInternal,
InteractiveSandboxProperties, and EvictionReason symbols so the tests mirror
existing ones but check the == boundary.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/SandboxArchitectureTest.java`:
- Line 58: Rename the test method spiHasNoMicrometerDeps in class
SandboxArchitectureTest to follow the should[ExpectedBehavior]When[Condition]
pattern (e.g., shouldHaveNoMicrometerDependenciesWhenInspectingSpi or similar),
update its method declaration name accordingly, and ensure any references (IDE
run configurations or annotations) remain consistent after the rename so the
test still runs.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e193ec4-9988-4c49-8a71-9aaa9e21ce9b
📒 Files selected for processing (19)
docs/contributor/agent/observability.mdxserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/mentor/chat/MentorChatMetrics.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/mentor/chat/MentorChatService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/InteractiveSandboxProperties.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/DockerAttachedSandboxAdapter.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/DockerInteractiveSandboxAdapter.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/FrameRingBuffer.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/InteractiveSandboxMetrics.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/InteractiveSandboxRegistry.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/spi/AttachedSandbox.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/spi/InteractiveSandboxCapacityExceededException.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/config/MetricsCardinalityConfig.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/mentor/chat/MentorChatServiceTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/SandboxArchitectureTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/DockerInteractiveSandboxLiveTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/FrameRingBufferMetricsTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/FrameRingBufferTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/InteractiveSandboxRegistryTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/SubscribeOrderingPropertyTest.java
Comment on lines
+14
to
+35
| ### `interactive_sandbox.evicted_total{reason}` | ||
|
|
||
| Counter, one increment per sandbox eviction. | ||
|
|
||
| | Tag value | Meaning | | ||
| | --- | --- | | ||
| | `idle` | Session evicted because it crossed `hephaestus.mentor.idle-ttl-seconds`. | | ||
| | `max_lifetime` | Session evicted because it crossed `hephaestus.mentor.max-lifetime-minutes` (absolute lifetime cap, default 60 min). | | ||
| | `manual` | Explicit `close()` from the chat layer (e.g., poisoned sandbox after a `-32002`). | | ||
| | `error` | Forced eviction due to a pump/writer fault. | | ||
| | `natural_exit` | The runner subprocess exited cleanly. | | ||
| | `daemon_unhealthy` | Docker daemon health check failed during reap. | | ||
|
|
||
| `mentor.session.eviction{reason}` carries the same data with the legacy name retained for | ||
| back-compat — both increment together. Alert on the SPI-aligned name. | ||
|
|
||
| ### `mentor.chat.outcome{outcome=CAPACITY_EXCEEDED}` | ||
|
|
||
| Counter, increments when a turn is rejected because `InteractiveSandboxRegistry.tryRegister` | ||
| returned `MAX_SESSIONS_PER_USER` or `MAX_SESSIONS_TOTAL`. **Distinguish from | ||
| `outcome=ERROR` when alerting** — capacity rejections are policy-driven and expected under load | ||
| (answer: add a replica or raise the cap); `ERROR` is a genuine failure that needs investigation. |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify eviction reason enum values and chat outcome enum values match documented tag formats.
# Check EvictionReason enum definition
echo "=== EvictionReason enum ==="
rg -n "enum EvictionReason" -A 20 --type java
# Check for CAPACITY_EXCEEDED in chat outcome enum
echo -e "\n=== Chat outcome enum with CAPACITY_EXCEEDED ==="
rg -n "CAPACITY_EXCEEDED" -B 5 -A 5 --type java
# Check for eviction reason usage in metrics recording
echo -e "\n=== Eviction reason metric recording ==="
rg -n "evicted_total|session.eviction" -B 2 -A 2 --type javaRepository: ls1intum/Hephaestus
Length of output: 25098
Change mentor.chat.outcome{outcome=CAPACITY_EXCEEDED} to mentor.chat.outcome{outcome=capacity_exceeded}
The eviction reason values are correctly documented with snake_case tag values (idle, max_lifetime, etc.), but the chat outcome is documented with the enum constant name in SCREAMING_SNAKE_CASE (CAPACITY_EXCEEDED) instead of the actual snake_case tag value that is emitted (capacity_exceeded). For consistency and clarity, use the actual tag value format.
🤖 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 `@docs/contributor/agent/observability.mdx` around lines 14 - 35, Update the
metric documentation to use the actual emitted tag value: change the header and
any inline references from "mentor.chat.outcome{outcome=CAPACITY_EXCEEDED}" to
"mentor.chat.outcome{outcome=capacity_exceeded}" and update the explanatory text
to mention outcome=capacity_exceeded (keeping the guidance to distinguish it
from outcome=error); locate the metric block for mentor.chat.outcome and adjust
the tag value casing to snake_case to match other documented tag values like
reason=idle.
Comment on lines
+408
to
412
| metrics.interactiveEvictedBy(reason).increment(); | ||
| // Free the per-(user,sandbox) debounce entry: the 50-userId cardinality cap bounds | ||
| // the absolute table size, but evicting per session keeps churn under control. | ||
| metrics.evictDropDebounce(userId, sessionId); | ||
|
|
Contributor
There was a problem hiding this comment.
Run debounce-entry cleanup in finally so error-path closes don't retain stale entries.
metrics.evictDropDebounce(userId, sessionId) currently runs only on the happy path. If runClose(...) throws and falls into the catch block, the per-session debounce entry is never evicted.
Proposed fix
- metrics.interactiveEvictedBy(reason).increment();
- // Free the per-(user,sandbox) debounce entry: the 50-userId cardinality cap bounds
- // the absolute table size, but evicting per session keeps churn under control.
- metrics.evictDropDebounce(userId, sessionId);
+ metrics.interactiveEvictedBy(reason).increment();
@@
} finally {
+ // Ensure per-session debounce state is released on both success and error paths.
+ metrics.evictDropDebounce(userId, sessionId);
MDC.clear();
}Also applies to: 425-438
🤖 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
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/DockerAttachedSandboxAdapter.java`
around lines 408 - 412, The metrics.evictDropDebounce(userId, sessionId) call is
only executed on the success path in DockerAttachedSandboxAdapter and can be
skipped if runClose(...) throws; move the debounce-entry cleanup into a finally
block so it always runs (even on exceptions). Update the try/catch in the method
that calls runClose(...) inside DockerAttachedSandboxAdapter to call
metrics.evictDropDebounce(userId, sessionId) from a finally block, and apply the
same change to the other similar block (around lines handling the other runClose
path referenced in the comment) so both code paths always evict the
per-(user,session) debounce entry.
Comment on lines
+60
to
+66
| * Per-user counter for {@code interactive_sandbox.frame_ring.dropped_total}. The cardinality | ||
| * cap (50 distinct userIds across the JVM) is enforced upstream by a {@code MeterFilter} — | ||
| * any further userId triggers a {@code denyAll} for THIS specific counter only, so the rest | ||
| * of the registry is unaffected. | ||
| */ | ||
| private final ConcurrentHashMap<String, Counter> userDroppedCounters = new ConcurrentHashMap<>(); | ||
|
|
Contributor
There was a problem hiding this comment.
Bound the in-memory per-user counter cache to avoid unbounded growth.
userDroppedCounters is never evicted/capped, so it can grow with unique userId churn over process lifetime even if registry cardinality is limited. This creates a memory-growth path in a hot metric path.
💡 Suggested bounded-cache approach
+ private static final int USER_DROP_COUNTER_CACHE_MAX = 50;
+
private Counter userDroppedCounter(String userId) {
+ Counter existing = userDroppedCounters.get(userId);
+ if (existing != null) {
+ return existing;
+ }
+
+ // Keep local cache bounded; registry-side cardinality capping is separate.
+ if (userDroppedCounters.size() >= USER_DROP_COUNTER_CACHE_MAX) {
+ return Counter.builder("interactive_sandbox.frame_ring.dropped")
+ .tag("userId", userId)
+ .description("Per-user frame ring overflow drops (debounced to <=1/s per session, capped at 50 distinct users)")
+ .register(registry);
+ }
+
return userDroppedCounters.computeIfAbsent(userId, u ->
Counter.builder("interactive_sandbox.frame_ring.dropped")
.tag("userId", u)
.description("Per-user frame ring overflow drops (debounced to <=1/s per session, capped at 50 distinct users)")
.register(registry)
);
}Also applies to: 212-219
🤖 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
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/interactive/InteractiveSandboxMetrics.java`
around lines 60 - 66, The unbounded ConcurrentHashMap userDroppedCounters in
InteractiveSandboxMetrics can grow indefinitely; replace it with a bounded cache
(e.g., Caffeine Cache<String, Counter> with a sensible maximumSize and
expireAfterAccess) and change lookups to use cache.get(key, k ->
createCounterForUser(k)) or cache.asMap() where computeIfAbsent was used, so
counters auto-evict and you maintain thread-safety and lazy creation; apply the
same replacement to the other per-user counter map used later in the class (the
per-user counter block at lines ~212-219) so both caches are size-capped and use
the same eviction policy.
Comment on lines
+54
to
+63
| /** | ||
| * Wall-clock instant the session was attached. Used by the registry's lifetime reaper to | ||
| * evict sessions that have accumulated state past a configured maximum. Every implementation | ||
| * captures this at construction time — the {@code default} fallback here exists only so a | ||
| * future SPI consumer that forgets to implement it returns a sensible value (caller treats | ||
| * "now" as a brand-new session, so the reaper simply won't fire). | ||
| */ | ||
| default Instant createdAt() { | ||
| return Instant.now(); | ||
| } |
Contributor
There was a problem hiding this comment.
createdAt() default currently masks missing SPI implementation and bypasses lifetime eviction.
Because the default returns Instant.now() on each call, Duration.between(createdAt(), now) stays near zero, so MAX_LIFETIME never triggers for implementations that forget to override this method. Prefer enforcing this contract (abstract method) or fail-fast default instead of silent fallback.
Proposed contract-tightening diff
- default Instant createdAt() {
- return Instant.now();
- }
+ Instant createdAt();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Wall-clock instant the session was attached. Used by the registry's lifetime reaper to | |
| * evict sessions that have accumulated state past a configured maximum. Every implementation | |
| * captures this at construction time — the {@code default} fallback here exists only so a | |
| * future SPI consumer that forgets to implement it returns a sensible value (caller treats | |
| * "now" as a brand-new session, so the reaper simply won't fire). | |
| */ | |
| default Instant createdAt() { | |
| return Instant.now(); | |
| } | |
| /** | |
| * Wall-clock instant the session was attached. Used by the registry's lifetime reaper to | |
| * evict sessions that have accumulated state past a configured maximum. Every implementation | |
| * captures this at construction time — the {`@code` default} fallback here exists only so a | |
| * future SPI consumer that forgets to implement it returns a sensible value (caller treats | |
| * "now" as a brand-new session, so the reaper simply won't fire). | |
| */ | |
| Instant createdAt(); |
🤖 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
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/spi/AttachedSandbox.java`
around lines 54 - 63, The default createdAt() implementation on AttachedSandbox
currently returns Instant.now() and masks missing SPI implementations; change
the contract so AttachedSandbox.createdAt() is abstract (remove the default
method) so implementors must provide a stable creation instant, or alternatively
replace the default with a fail-fast implementation that throws
UnsupportedOperationException referencing createdAt() to surface missing
overrides; update all classes implementing AttachedSandbox to supply a concrete
createdAt() that captures and returns the creation Instant.
Collaborator
Author
|
Folding into #1086 (mentor sticky cookies) — the observability items belong with the replica-affinity work, not as a standalone follow-up. |
FelixTJDietrich
added a commit
that referenced
this pull request
May 17, 2026
Items folded in from closed PRs #1089, #1090, #1091 — they belong here alongside the sticky-cookie + replica-affinity work, not as standalone follow-ups. - MentorReplicaAffinityCheck HealthIndicator (refuses to start on multi-replica without operator-asserted sticky routing) - WorkspaceContextBuilder mentor striping: hash (contributorId, workspaceId) instead of returning null and collapsing every mentor session onto stripe 0 - AttachedSandbox.subscribe(Cursor) — collapse the default-method subscribeFromNow seam; reused sandboxes were replaying prior turn's agent_end - Per-thread frame routing pushed into the SPI as Predicate<JsonNode> - JsonlStdinWriter synchronized → ReentrantLock (JEP 444 pin risk under virtual-thread callers) - FrameRingBuffer dropped_total metric with bounded cardinality - EvictionReason.MAX_LIFETIME wired (165 MB RSS × N idle users) - MentorChatMetrics.Outcome.CAPACITY_EXCEEDED so dashboards stop being blind to per-user-cap rejections Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.
Summary
Retrospective housekeeping on the mentor pipeline after #1078 / #1080 / #1081. Three observability gaps + a small SPI hygiene rule, plus closing out three issues that #1081 already resolved in place.
interactive_sandbox.frame_ring.dropped_total{userId}— new counter. The ring buffer was bumping a global counter only; per-user attribution was unavailable. Per-user emission is debounced to <=1/s per(userId, sandboxId)and the userId tag is capped at 50 distinct values via aMeterFilter(MetricsCardinalityConfig). The 51st user still drops frames into the global counter; only attribution is lost. DefaultringBufferFrames=512is NOT bumped here — that decision waits on observed traffic.interactive_sandbox.evicted_total{reason}— new counter under the SPI-aligned name.mentor.session.eviction{reason}stays for back-compat; both increment together.MAX_LIFETIMEreaper —EvictionReason.MAX_LIFETIMEwas reserved-but-unwired. New propertyhephaestus.mentor.max-lifetime-minutes(default 60, range [5, 480]). Wired into the existing reap pass; checked before the idle check (a chatty user past lifetime should still evict). Backstop against runners that accumulate state, drift on context, or leak FDs over hours.mentor.chat.outcome{CAPACITY_EXCEEDED}— new outcome distinguishing per-user/global cap rejections from genuine errors. Previously routed throughERRORand was invisible on dashboards. Backed by a new typedInteractiveSandboxCapacityExceededExceptionso the routing is structural, not message-based.agent.sandbox.spi.*may not depend onio.micrometer... Keeps the SPI transport-agnostic; metrics belong indocker.interactive.*andagent.mentor.chat.*.docs/contributor/agent/observability.mdx— new page (sidebar position 6) cataloguing the new metrics and explaining how to read the per-user drop signal before raising the ring buffer default.SPI changes
AttachedSandbox.createdAt()added with a sensible default.DockerAttachedSandboxAdapteralready capturedattachedAtinternally — the SPI method now exposes it. The default keeps the contract additive (no SPI break for hypothetical consumers).DockerAttachedSandboxAdapter.terminate(...)promoted from package-private to public (now implements the newInteractiveSandboxRegistry.ReapTargetinterface — a narrow surface extracted so the reaper test can drive synthetic targets without mocking the final adapter).Tests
FrameRingBufferTestrewritten to aRunnable onDropcallback (no more Counter coupling at the buffer layer).FrameRingBufferMetricsTest(new) — debounce, distinct-user attribution, eviction reset, ceil-bound assertion across a 5.5s window.InteractiveSandboxRegistryTest(new) — MAX_LIFETIME eviction, lifetime-beats-idle precedence, non-ATTACHED states skipped, IDLE path still works.MentorChatServiceTest— two new cases forCAPACITY_EXCEEDED(per-user + global).SandboxArchitectureTest— extended SPI boundary block with the Micrometer rule.2,666unit + architecture tests pass locally.Stranded issue housekeeping
Three issues from the #1081 epic that are obsolete:
MentorProxyControllerdeleted,useMentorChatrewritten in-place in feat(mentor): server-side Pi mentor over SSE with live LLM tests #1081.1778756946278_changelog.xml).I'll close these once this PR opens cleanly.
Test plan
./mvnw test -Dsurefire.includedGroups="unit,architecture"— 2,666 passedagent.sandbox.spi.*↛io.micrometer..verified by adding the rule (no SPI files import Micrometer today)FrameRingBufferMetricsTestexercises debounce + cardinality at the unit levelInteractiveSandboxRegistryTestexercises MAX_LIFETIME with a fixedClockMentorChatServiceTestexercises CAPACITY_EXCEEDED routing for both PER_USER and GLOBAL scopesinteractive_sandbox.frame_ring.dropped_total{userId}counter on staging after merge — if drops show up at all, raiseringBufferFrames(this PR intentionally does not bump the default)interactive_sandbox.evicted_total{reason=max_lifetime}fires after one hour of soak🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation