Skip to content

Add offpeak_location_hint compat flag for Durable Objects - #8001

Merged
rushilmehra merged 1 commit into
mainfrom
rm/offpeak-location-hint-flag
Oct 26, 2026
Merged

rushilmehra merged 1 commit into
mainfrom
rm/offpeak-location-hint-flag

Conversation

@rushilmehra

Copy link
Copy Markdown
Contributor

Durable Object stubs can now pass locationHint: "offpeak" when the experimental offpeak_location_hint compatibility flag or the catch-all experimental flag is enabled. In production, a new object created with this hint is placed in a region that is off-peak at the current UTC time. workerd itself passes the hint through unchanged like any other location hint.

Without either flag, get() and getByName() reject the hint with the same TypeError used for other unsupported location hints.

@rushilmehra
rushilmehra requested review from a team as code owners October 26, 2026 15:22
@github-actions

github-actions Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@rushilmehra

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

github-actions Bot added a commit that referenced this pull request Oct 26, 2026
@clanky-the-clanker

clanky-the-clanker Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

💬 Approved with comments — The runtime gating is correct and covered by tests. The only problem is one WARNING: the TypeScript declarations don't include the new "offpeak" hint, which makes the flag harder to use but doesn't affect runtime safety.

Findings: 1 warning

The decision is advisory: this comment never approves, blocks or requests changes on the pull request.

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #8001.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## WARNING (1)

### [Documentation] The `DurableObjectLocationHint` TS unions (actor.h:252, :294) leave out "offpeak", so TypeScript code that enables the flag can't pass the newly supported hint without a cast. Add it in the source definition (for example the experimental-only `GetDurableObjectOptionsWithRetry`) and regenerate the types.
- **File:** src/workerd/api/actor.h:294

📝 Documentation: ⚠️ 1 warning

The TypeScript declarations for Durable Object location hints don't include "offpeak".

Recommendation: 📝 Minor Comments

  • [WARNING] The DurableObjectLocationHint TS unions (actor.h:252, :294) leave out "offpeak", so TypeScript code that enables the flag can't pass the newly supported hint without a cast. Add it in the source definition (for example the experimental-only GetDurableObjectOptionsWithRetry) and regenerate the types. (src/workerd/api/actor.h:294)
View full review
  • src/workerd/api/actor.c++:334 now accepts "offpeak" when the flag is enabled. The DurableObjectLocationHint unions in JSG_STRUCT_TS_DEFINE still leave it out, in both GetDurableObjectOptions and GetDurableObjectOptionsWithRetry (src/workerd/api/actor.h:252 and :294). As a result, TypeScript code that turns on the flag gets a type error unless it casts. The flag is $experimental, so the cleanest place to add the value is probably the GetDurableObjectOptionsWithRetry definition, which only appears in the types of workers with the experimental flag. Then regenerate the types with just generate-types; don't edit types/generated-snapshot/ by hand.

✅ All other checks passed

🔒 Security: ✅ LGTM

No security concerns. The change only adds a check on a string value that already reaches the existing location-hint handling.


📖 AGENTS.md: ✅ LGTM

No AGENTS.md updates are needed for this change.


📋 Code Quality: ✅ LGTM

  • src/workerd/api/actor.c++:331-336: get(), getByName() and getExisting() all go through getImpl, so the check covers each of them. The check runs synchronously, so assert.throws in the test is the right assertion.
  • The error message says "get called" even when the caller used getByName. That matches the existing jurisdiction error message in the same function.
  • src/workerd/io/compatibility-date.capnp:1738: ordinal @197 follows @196. The flag is $experimental with no enable date, and the BUILD target passes --experimental as that requires.
  • The .wd-test covers the flag, the experimental flag and the no-flag case.

⚡ Performance: ✅ LGTM

The check adds one string comparison and one feature-flag lookup per get() call that passes a location hint. The cost is negligible.


Reviewed commit: 71ce624e

@ask-bonk

ask-bonk Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Since last review: 0 resolved, 2 still open, 0 new.

The formatting update leaves the compatibility gate and type-surface changes unchanged.

Earlier findings still present (thread resolved)

  • [BLOCKING] src/workerd/api/actor.c++:331-336: This still changes an existing unflagged JavaScript call from forwarding "offpeak" to throwing. The experimental flag only enables the new value; its disabled branch is reached by existing workers, so it cannot make that rejection backward-compatible. Preserve the legacy pass-through and introduce any future rejection only under a default-enabled migration flag.

  • [WARNING] src/workerd/api/actor.c++:334: The runtime now accepts "offpeak" when either flag is enabled, but both DurableObjectLocationHint definitions in actor.h still omit it. TypeScript users therefore cannot write the newly documented call, including in experimental Workers. Add "offpeak" to both unions and regenerate the type snapshots.


Reviewed commit: 71ce624e · github run

Comment thread src/workerd/api/actor.c++
Comment thread src/workerd/api/actor.c++ Outdated
Durable Object stubs can now pass locationHint: "offpeak" when the
experimental offpeak_location_hint compatibility flag or the catch-all
experimental flag is enabled. In production, a new object created with
this hint is placed in a region that is off-peak at the current UTC
time. workerd itself passes the hint through unchanged like any other
location hint.

Without either flag, get() and getByName() reject the hint with the
same TypeError used for other unsupported location hints.
@rushilmehra
rushilmehra force-pushed the rm/offpeak-location-hint-flag branch from f8973f0 to 71ce624 Compare October 26, 2026 16:18
@rushilmehra
rushilmehra merged commit ec3982b into main Oct 26, 2026
29 of 31 checks passed
@rushilmehra
rushilmehra deleted the rm/offpeak-location-hint-flag branch October 26, 2026 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants