feat(canvas): payload cap + agent write rate limit (#1800 slice 5)#1829
feat(canvas): payload cap + agent write rate limit (#1800 slice 5)#1829jaylfc wants to merge 2 commits into
Conversation
Docs-Reviewed: canvas payload cap + agent write rate limit per lead-agent-identity-and-canvas-access.md
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| def is_limited(self, key: str) -> bool: | ||
| with self._lock: | ||
| self._prune(key) | ||
| return len(self._log.get(key, [])) >= self._max |
There was a problem hiding this comment.
WARNING: Non-atomic check-then-act allows the rate limit to be exceeded under concurrency
The limit decision here and the increment in record are separate locked operations, but record is only called after the awaited store write (await cs.add_element / update_element / delete_element). Under a burst, more than 30 concurrent requests can all observe count < 30 from is_limited before any of them calls record, so all of them succeed and the 30/60s budget is exceeded.
Fix: combine the check and increment into a single atomic method (acquire lock once, if count >= max return limited without recording, else append timestamp and return allowed) and call it exactly once per request.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
|
|
||
| def _check_payload_size(payload: dict) -> None: | ||
| size = len(json.dumps(payload, default=str).encode("utf-8")) |
There was a problem hiding this comment.
WARNING: Payload cap is enforced after full request parsing and may mis-measure
_check_payload_size runs on the already-deserialized pydantic payload, i.e. only after FastAPI/pydantic have fully materialized the entire JSON request body in memory. A 64 KB payload cap therefore does not protect against a multi-MB request body (memory/DoS): the oversized body is already parsed before this guard returns 413. Also json.dumps(payload, default=str) may over/under-estimate size versus the store's actual serialization if payload ever holds non-JSON-native objects.
For real protection, enforce a raw request-body size limit (ASGI/gateway layer or a streaming Request body read) before deserialization.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| self._log.move_to_end(key) | ||
|
|
||
|
|
||
| _canvas_write_limiter = _CanvasWriteLimiter() |
There was a problem hiding this comment.
SUGGESTION: In-memory singleton limiter: per-process, non-persistent, and global per agent
The limiter is a module-level in-memory dict, so (a) it resets on every process restart, (b) it is not shared across multiple workers/instances — so the effective limit becomes 30 * workers per window — and (c) it keys only on actor_id, meaning one agent's 30/60s budget is shared across all projects it can write to rather than being scoped per (agent, project).
If cross-project isolation or multi-worker correctness matters, back this with a shared store (Redis/DB) and key on (actor_id, project_id). Also consider adding a Retry-After header on the 429 responses for correct client backoff.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 1c05d54)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 1c05d54)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Reviewed by hy3:free · Input: 73.9K · Output: 6.8K · Cached: 77.1K |
… 5 per lead-agent-identity-and-canvas-access.md
|
Superseded by #1838, which landed all seven slices as one consistent set on dev (with the CI-caught active-handle migration-order fix folded). Closing. |
1 participant
Builds on feat/lead-identity-base (slices 1+2+3).
What changed:
tinyagentos/routes/project_canvas.py: added a 64 KB payload size cap (HTTP 413) checked on element create and PATCH update; added a per-agent rolling-window write rate limiter (30 writes / 60 s, HTTP 429) scoped to POST create, PATCH update, and DELETE; session/human writes are never rate-limited.tests/test_routes_project_canvas.py: addedTestCanvasPayloadSizeCap(413 on oversized create/patch payload) andTestAgentWriteRateLimit(429 for agent exceeding window, 429 on agent DELETE exceeding window, human writes unaffected by rate limit).DO NOT MERGE.