You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Cleanup RPCs from the Global Controller were dialing the host-published port
(localhost:8001-8006), which isn't reachable from inside its own container --
every cleanup call failed, but the completed-request batch got dropped from
Redis anyway, so memory grew unbounded. Fixed to dial the container-reachable
address instead, and to only drop a batch once every instance confirms receipt.
Fixing that exposed a second issue: once cleanup actually succeeds, it can
delete a request's data before the telemetry poll loop has read it, silently
dropping spans under load. Switched that cleanup to expire the keys with a
grace period instead of deleting them immediately, same pattern already used
in deploy.py.
See commit message for the full investigation and benchmark numbers.
…e telemetry reads it (CAN-391)
Global Controller's cleanup broadcast dialed instance["endpoint"], the
host-published port (e.g. localhost:8001), which is unreachable from
inside the GC's own container. Every Cleanup RPC failed, but the
completed-request batch was drained from Redis regardless, so futures
and affinity keys behind those requests were never deleted and Redis
grew unbounded.
- global_controller.py: resolve the cleanup RPC endpoint through
instance_manager._routing_endpoint_for() (container-reachable),
matching every other in-container RPC caller in this file. Only
drain "request:completed" once every instance has confirmed
receipt; on any failure (or no instances at all), leave the batch
queued for retry next cleanup cycle. The receiving RPC is already
idempotent (setnx lock, no-op if futures are already gone), so
retrying is safe.
- local_controller_frontend.py: once cleanup RPCs actually succeed,
a second issue surfaces under load -- _cleanup_request deleted
future:{id} keys immediately, racing GlobalController's poll loop
(every 5s), which reads those same keys to build OTel spans
(telemetry_logging.pull_runtime_information). Whichever side lost
the race silently dropped that request's span. Verified via a
controlled before/after on the same image build: reverting just
the reachability fix restored 7000/7000 spans landing, while
keeping it landed only ~4900/7000. Switched future-key cleanup
from redis.delete to redis.expire(30s), the same grace-period
pattern deploy.py already uses for request:*:status/:result, so
the poll loop has time to capture telemetry before the keys
disappear. Affinity bindings are untouched since nothing else
reads them.
Verified with both benchmark.py runs and unit tests: cleanup succeeds
with zero failures, future:*/affinity:*/request:*:futures keys no
longer accumulate (confirmed expiring rather than piling up), and
otel_counts_match is back to true with all 7000/7000 expected spans
landing, on the same 1000-request/concurrency-5 workload that
previously showed the leak and the span-loss regression.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
not a big issue since this would only happen if the polling_interval in global_controller.yaml files is >30 seconds, but maybe something to change, like config.get("poll_interval") * 5 or something, idk.
No behavior change -- verified with the existing unit tests (all 257
still pass) and a repeat 1000-request/concurrency-5 benchmark run,
which showed the same result as before the refactor: zero cleanup
failures, no leftover future:*/affinity:*/request:*:futures keys, and
otel_counts_match true with all 7000/7000 expected spans landing.
Replaces computing all_sent in two branches and checking it two lines
later with two early-return guard clauses (no instances -> return,
not all(...) -> return), so there's no intermediate boolean carrying
state between where it's computed and where it's checked.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… retry
FUTURE_CLEANUP_GRACE_SECONDS was hardcoded to 30s, so a configured
poll_interval >= 30s (or a slow poll cycle) could let the TTL expire
before GlobalController's poll loop ever read the future data, silently
dropping telemetry spans. The grace period now scales with the actual
CANYONOS_POLL_INTERVAL. RedisClient.expire() also gained an nx flag so
retried Cleanup RPCs (e.g. against an unreachable instance) don't keep
resetting the TTL on keys that already have one.
Also adds RoutingEndpointTests to test_global_controller_cleanup.py,
which give an instance's published and routing endpoints distinct
values so a regression back to instance["endpoint"] (the CAN-391 bug)
would actually fail the suite -- every existing fixture collapses the
two to the same value and can't catch that.
'
This grace period is computed once from the agent's launch-time environment, but GlobalController.reload_config() updates self.poll_interval in place without restarting existing agent containers. If SIGHUP changes the interval from 5s to 60s, those agents still use a 30s TTL and can expire future:{id} before the next telemetry poll, so the span-loss race remains. Propagate the reloaded interval to running agents or make the grace value come from state that reload updates.
Updated expire method to include 'nx' parameter for conditional expiration.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.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
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.
Saaketh0
left a comment
userAugustos
changed the title
userAugustos
left a comment
Cleanup RPCs from the Global Controller were dialing the host-published port
(localhost:8001-8006), which isn't reachable from inside its own container --
every cleanup call failed, but the completed-request batch got dropped from
Redis anyway, so memory grew unbounded. Fixed to dial the container-reachable
address instead, and to only drop a batch once every instance confirms receipt.
Fixing that exposed a second issue: once cleanup actually succeeds, it can
delete a request's data before the telemetry poll loop has read it, silently
dropping spans under load. Switched that cleanup to expire the keys with a
grace period instead of deleting them immediately, same pattern already used
in deploy.py.
See commit message for the full investigation and benchmark numbers.
🤖 Generated with Claude Code