Add updateCohort RPC to ActorStorage and limit cohort length#6599
Add updateCohort RPC to ActorStorage and limit cohort length#6599
Conversation
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
Adds an updateCohort RPC to ActorStorage and a client-side length limit (10 bytes) on cohort values, silently dropping oversized inputs.
Issues (ranked by severity):
- [MEDIUM] The capnp comment says "If
cohortis omitted, clear the stored value" butcohort :Textis not optional in Cap'n Proto — it always has a value (defaults to empty string). The comment should document the actual convention (e.g., "empty string clears") or the field should be rethought. - [LOW] Silent drop of oversized cohorts may frustrate developers debugging routing behavior. Consider whether a
JSG_REQUIRE/ thrown exception would be more appropriate here.
This review was generated by an AI assistant and may contain inaccuracies.
| # Get the storage capability for the given stage of the pipeline, identified by its stable ID. | ||
|
|
||
| updateCohort @1 (cohort :Text); | ||
| # Persist the actor's resolved cohort. If `cohort` is omitted, clear the stored value. |
There was a problem hiding this comment.
The comment says "If cohort is omitted" but in Cap'n Proto a non-optional Text parameter always has a value (defaults to empty string) — it can never truly be omitted. Should the comment say "If cohort is empty, clear the stored value" instead, to match the actual wire semantics?
| # Persist the actor's resolved cohort. If `cohort` is omitted, clear the stored value. | |
| # Persist the actor's resolved cohort. If `cohort` is empty, clear the stored value. |
| // Cohort length is limited to kMaxActorCohortLength bytes, so silently drop | ||
| // oversized values rather than passing them through. | ||
| if (c.size() > kMaxActorCohortLength) { | ||
| v.cohort = kj::none; |
There was a problem hiding this comment.
Silently discarding the cohort is a defensive choice, but it may make debugging hard for developers who set a cohort and see it ignored with no feedback. Would a JSG_REQUIRE(c.size() <= kMaxActorCohortLength, RangeError, "cohort must be at most ", kMaxActorCohortLength, " characters") be more appropriate? That way callers learn about the limit immediately instead of having their cohort silently cleared.
There was a problem hiding this comment.
This is good advice, FWIW. It's better to reject invalid input than to silently ignore it.
|
I'm Bonk, and I've done a quick review of your PR. Review posted on PR #6599 with 2 inline comments:
|
maxmcd
force-pushed
the
mmcdonnell/do-cohort-persistence
branch
from
20229d9 to
0ed4732
Compare
|
Please don't post commits nor PRs with empty descriptions. The PR description should explain not just what is changing, but why. Do not assume that anyone already knows what you're up to. (I know this is a draft, but even for a draft, please do this, or if it's just a test with no intent to ever merge, write that.) |
3 participants
No description provided.