feat(world-postgres): support Hook minimum retention - #3276
Conversation
🦋 Changeset detectedLatest commit: 03ae58a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🧪 E2E Test Results✅ All tests passed E2E Test SummarySummary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
✅ vercel-multi-region
|
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: approved.
AI Review: Note
Before merge, both Worlds should enforce a default maximum retention of 30d, overridable by environment variable.
experimental_minRetention is unbounded today. createHook() validates the webhook combination and the World capability, then hands whatever the caller passed to parseDurationToDate with no ceiling, so '10y' or a Date in 2099 is honored verbatim by both backends. Two reasons that matters more here than it looks:
- Neither World reaps an expired retained Hook on its own. world-postgres never deletes the row unless someone creates a Hook with the same token again; world-local only reaps lazily when a read happens to touch it. An unbounded deadline is therefore effectively a permanent row, and the token stays reserved for as long as it says.
- The deadline is pinned once at
createHook()and re-pinned from the persistedhook_createdon replay. That is the right design, but it also means a mistake is durable: there is no later opportunity to correct an over-long value for runs already in flight.
A few things worth settling while wiring the cap:
- Clamp, don't reject. Silently truncating 90d to 30d is surprising, so the clamp should be visible in a log at minimum.
- Clamp before the value reaches the event log, so the persisted deadline is the effective one and replay stays deterministic. A consequence to state explicitly: raising or lowering the env var later will not retroactively move deadlines already persisted, which I think is the behavior you want.
- Use one env var name shared by both Worlds rather than a
WORKFLOW_LOCAL_*/WORKFLOW_POSTGRES_*pair, so the two cannot drift, and document it per the repo rule that every user-configurable environment variable is documented. - The
create-hookdocs page should state the default ceiling next to the existing retention section, since"30d"is the example value there and would sit exactly at the limit.
My agent misunderstood, I think it's fine to have separate variables |
…is missing the `tokenRetentionUntil` key that was added to `HookSchema`, causing TS2741 and breaking every Vercel build.
This commit fixes the issue reported at packages/web-shared/src/components/sidebar/attribute-panel.tsx:426
## Bug
Commit `ad58321` added `tokenRetentionUntil: z.coerce.date().optional()` to `HookSchema` in `packages/world/src/hooks.ts:106`. This adds `tokenRetentionUntil` to the inferred `Hook` type.
In `packages/web-shared/src/components/sidebar/attribute-panel.tsx`, `AttributeKey` is a union that includes `keyof Hook`, so `tokenRetentionUntil` becomes a required member of the **exhaustive** `Record<AttributeKey, (value: unknown, context?: DisplayContext) => ...>` object literal `attributeToDisplayFn` (starting at line ~426).
Because the literal had no `tokenRetentionUntil` entry, `tsc` fails:
```
src/components/sidebar/attribute-panel.tsx(426,7): error TS2741:
Property 'tokenRetentionUntil' is missing in type '{ ... }' but required in type
'Record<AttributeKey, (value: unknown, context?: DisplayContext | undefined) => ReactNode>'.
```
This breaks `@workflow/web-shared#build` and therefore every Vercel deployment (17 failing deployments observed, all with this identical error).
## Fix
Added a `tokenRetentionUntil` entry to `attributeToDisplayFn`, placed alongside the other Hook date fields (`lastReceivedAt`, `disposedAt`):
```ts
tokenRetentionUntil: timestampWithTooltipOrNull,
```
`tokenRetentionUntil` is a `Date` field, and `timestampWithTooltipOrNull` (defined at line 402) is the display helper used by all the other surfaced date fields (`createdAt`, `startedAt`, `completedAt`, `retryAfter`, `resumeAt`, `occurredAt`). Given the intent of `ad58321` was to expose the hook retention deadline, surfacing it as a tooltip-annotated timestamp is the consistent choice.
Only `attributeToDisplayFn` is a fully exhaustive `Record<AttributeKey, ...>`; the other maps are `Partial<...>` / `Set`, so no other edits are required.
## Verification
`node_modules` are not installed in this sandbox, so `tsc` could not be executed directly. Verified structurally instead: the newly added `tokenRetentionUntil` entry (line 449) references `timestampWithTooltipOrNull`, which is defined in-file at line 402 and already used by the sibling date entries, so the fix satisfies the missing-key requirement without introducing new type errors.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: VaguelySerious <mittgfu@gmail.com>
📊 Workflow Benchmarkscommit Backend:
📈 STSO distribution vs main (inline / queue-hop histograms)1020 steps (inline) Cumulative STSO time: main 145277ms → this run 415304ms (Δ +270027ms, +186%) 1020 steps (queue-hop) Cumulative STSO time: main 3471ms → this run 9317ms (Δ +5846ms, +168%) ℹ️ Metric definitions & methodologyThe collapsed STSO distribution section above buckets every step gap of the sequential-steps run (not a sampled window), split by whether the step ending the gap ran inline — in the same warm process as the step before it, so the gap is pure framework overhead — or after a queue-hop — the first step of a fresh process, which pays queue dispatch, client reinit and event-log replay. Bars overlay the two runs: Best/P75/P90/P99 deltas compare against the most recent benchmark run on Metrics — TTFS: time to first step body (in-deployment start() → first step body, deployment clocks) · STSO: step-to-step overhead (gap between consecutive step bodies) · WO: workflow overhead (whole-run time outside step bodies, in-deployment anchored) · SL: stream latency (in-deployment write → read propagation, readAt - writtenAt) · SO: stream overhead (end-to-end write+consume time beyond the modelled generation window) Scenarios — step: one trivial no-op step, no stream; no hooks, so the run stays in turbo mode (in-process fast path) · stream: one streaming step; no hooks, so the run stays in turbo mode (in-process fast path) · hook + stream: registers a hook before one step, which exits turbo mode (dispatch path) · 1020 steps: 1020 trivial sequential steps; STSO is measured between consecutive steps in the given step ranges, and WO is the whole-run overhead outside step bodies · stream latency: parallel reader/writer steps on a dedicated stream; SL is the in-deployment write->read propagation (readAt - writtenAt) · stream overhead (text): writer streams 300 variable-length text token deltas paced at 100/s for 3s (a haiku-size LLM's token throughput) while a parallel reader drains the whole stream; SO is the end-to-end write+consume time beyond the 3s generation window (overhead/backpressure) · stream overhead (structured): same workload as stream overhead (text), but each delta is an AI-SDK-style structured object ({ type: 'text-delta', id, text }) instead of a raw string, so the SO gap vs the text scenario is the added serialization cost 🔴 marks a percentile over its target (within target is left unmarked). Targets (p75/p90/p99, ms) — TTFS 200/300/600 · SL 50/60/125 · SO 250/500/1000 All metrics are measured from deployment-side timestamps only. Runs are triggered by an in-deployment route that stamps the anchor ( Cold starts are kept in the numbers on purpose — they are part of real bursty-workload latency. The workbench deployment cold-starts the |
…ostgres # Conflicts: # docs/content/docs/v5/api-reference/workflow/create-hook.mdx
…ostgres # Conflicts: # docs/content/docs/v5/api-reference/workflow/create-hook.mdx
|
No backport to This is feature work: it implements the To override, re-run the Backport to stable workflow manually via |
2 participants
Summary
experimental_minRetentionin the Postgres WorldhookRetentionWorld capabilityWorkflowWorldErrorWORKFLOW_POSTGRES_HOOK_RETENTION_LIMIT_DAYSto mirror another production limithook.dispose()This PR is stacked on #2866. The core API/event contract is already on
main; this PR only implements that contract for@workflow/world-postgres.Behavior
experimental_minRetentionis measured from Hook creation:hook.dispose()deletes the Hook and releases its token immediatelyWORKFLOW_POSTGRES_HOOK_RETENTION_LIMIT_DAYScan change that limitImplementation
The existing Hook row remains the only token-reservation state.
token_retention_untilis a nullabletimestamptzcolumn because the runtime supplies an absolute JavaScriptDateevents.create()rejects an oversizedhook_createdrequest before starting any database writeWorkflowWorldErrorwith status 400This intentionally does not solve the existing non-atomic token-admission race tracked by #2376. Adding a token uniqueness/locking protocol is the separate atomic-admission project, not part of this retention-only port.
Verification
@workflow/world-postgresTypeScript check passedhookMinRetentionWorkflow - terminal Hook cannot resume and its token stays unavailableDocs Preview
createHook()minimum retention