fix(agentd): use custom idle timeout annotation for garbage collection#343
fix(agentd): use custom idle timeout annotation for garbage collection#343sicaario wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for custom idle timeouts in the agent reconciler via sandbox annotations and includes comprehensive tests for this new functionality. It also updates RSA key comparisons in tests to handle precomputed values and corrects a status code comment in the server middleware. Feedback was provided to improve the robustness of the RSA key verification by comparing prime factors and using more idiomatic big integer comparisons.
There was a problem hiding this comment.
Pull request overview
Fixes agentd sandbox garbage collection to honor per-Sandbox idle timeout overrides (runtime.agentcube.io/idle-timeout) instead of always using the hardcoded 15-minute default, aligning behavior with how the Workload Manager annotates sandboxes for custom session durations (Issue #342).
Changes:
- Parse
runtime.agentcube.io/idle-timeoutfrom Sandbox annotations inagentdand use it to compute GC expiration (fallback to default on missing/invalid values). - Add unit tests covering short/long custom timeouts and invalid-timeout fallback behavior in
agentd. - Minor Router/JWT test adjustments (comment correction + RSA key equality comparison changes).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/agentd/agentd.go | Uses Sandbox runtime.agentcube.io/idle-timeout annotation to compute expiration time for GC. |
| pkg/agentd/agentd_test.go | Adds unit test coverage for custom/invalid idle-timeout handling. |
| pkg/router/server.go | Updates middleware comment to match the 429 response code used. |
| pkg/router/jwt_test.go | Makes private key PEM test compare RSA components instead of struct equality. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #343 +/- ##
==========================================
+ Coverage 47.57% 49.17% +1.60%
==========================================
Files 30 30
Lines 2819 2861 +42
==========================================
+ Hits 1341 1407 +66
+ Misses 1338 1301 -37
- Partials 140 153 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: sicaario <hrmnp8@gmail.com>
0e138fe to
e9eb09e
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @acsoto, @YaoZengzeng I have updated this PR to completely streamline its scope and address the automated review feedback. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR fixes a bug where
agentdignores the customruntime.agentcube.io/idle-timeoutannotation set on a Sandbox. Previously, the garbage collection reconciliation loop used a hardcoded15 * time.Minutevalue for every sandbox.With this change,
agentdsuccessfully parses the Sandbox's custom idle timeout from the metadata annotations and correctly honors shorter or longer custom session durations. It will fall back to the default15 * time.Minuteif the annotation is missing or improperly formatted.It also introduces new unit test coverage inside
agentd_test.goto explicitly verify custom short timeouts, custom long timeouts, and invalid timeout fallback paths.Which issue(s) this PR fixes:
Fixes #342
Special notes for your reviewer:
Does this PR introduce a user-facing change?: