Repository navigation
feat(convex): webhook-driven sandbox state sync - #83
mislavivanda wants to merge 4 commits into
Conversation
Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
There was a problem hiding this comment.
1 issue found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/convex/src/component/webhooks.ts">
<violation number="1" location="packages/convex/src/component/webhooks.ts:140">
P2: The staleness guard only compares against `remoteUpdatedAt`, which is written exclusively by this mutation — every other writer of the row (`upsertSandbox`, `start`, `stop`, `refresh`, `remove`) updates `state` without touching it. A retried or out-of-order delivery arriving within the 5-minute tolerance can therefore be applied even when it is older than state that was already pulled from the API (e.g. a late `stopped` event with `eventTime` 10:28 overwrites a `start` action that observed `started` at 10:30, because the last applied webhook time is older than 10:28). Stamp `remoteUpdatedAt` (e.g. `Date.now()`, or the API's `updatedAt` when available) on every successful API-observed write in `sandboxes.ts` so the discard rule also covers pull-based sync, or the docstring's "older ones are discarded" guarantee only holds for webhook-vs-webhook ordering.</violation>
</file>
This PR changes authentication, authorization, or input validation. Ultrareviews find 2.4x more serious bugs than standard reviews. Comment @cubic-dev-ai ultrareview to run one.
Fix all with cubic | Re-trigger cubic
| if (!sandbox) return "ignored-unknown"; | ||
| if ( | ||
| sandbox.remoteUpdatedAt !== undefined && | ||
| args.eventTime <= sandbox.remoteUpdatedAt |
There was a problem hiding this comment.
P2: The staleness guard only compares against remoteUpdatedAt, which is written exclusively by this mutation — every other writer of the row (upsertSandbox, start, stop, refresh, remove) updates state without touching it. A retried or out-of-order delivery arriving within the 5-minute tolerance can therefore be applied even when it is older than state that was already pulled from the API (e.g. a late stopped event with eventTime 10:28 overwrites a start action that observed started at 10:30, because the last applied webhook time is older than 10:28). Stamp remoteUpdatedAt (e.g. Date.now(), or the API's updatedAt when available) on every successful API-observed write in sandboxes.ts so the discard rule also covers pull-based sync, or the docstring's "older ones are discarded" guarantee only holds for webhook-vs-webhook ordering.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/convex/src/component/webhooks.ts, line 140:
<comment>The staleness guard only compares against `remoteUpdatedAt`, which is written exclusively by this mutation — every other writer of the row (`upsertSandbox`, `start`, `stop`, `refresh`, `remove`) updates `state` without touching it. A retried or out-of-order delivery arriving within the 5-minute tolerance can therefore be applied even when it is older than state that was already pulled from the API (e.g. a late `stopped` event with `eventTime` 10:28 overwrites a `start` action that observed `started` at 10:30, because the last applied webhook time is older than 10:28). Stamp `remoteUpdatedAt` (e.g. `Date.now()`, or the API's `updatedAt` when available) on every successful API-observed write in `sandboxes.ts` so the discard rule also covers pull-based sync, or the docstring's "older ones are discarded" guarantee only holds for webhook-vs-webhook ordering.</comment>
<file context>
@@ -0,0 +1,151 @@
+ if (!sandbox) return "ignored-unknown";
+ if (
+ sandbox.remoteUpdatedAt !== undefined &&
+ args.eventTime <= sandbox.remoteUpdatedAt
+ ) {
+ return "ignored-stale";
</file context>
There was a problem hiding this comment.
Valid — fixed in 2c319dd. Every API-observed state write (upsertSandbox, which backs create/start/stop/refresh/remove, plus setSandboxError when it records a state) now stamps remoteUpdatedAt, so the discard rule covers pull-based sync too: a late event older than state already observed via the API is ignored. New test: seed via the API path, deliver an event timestamped 30s earlier, assert ignored-stale. The stamp uses Convex's clock rather than Daytona's (API-observed writes don't carry Daytona's timestamp through every path), so the tradeoff is that an event emitted within clock skew of an observation may be discarded; the next real state change re-syncs it.
…writes - validate the parsed payload is an object and that id, newState and the timestamp are non-empty strings; signed-but-malformed deliveries get 400 instead of crashing the action or failing argument validation - every API-observed state write (upsertSandbox, setSandboxError) stamps remoteUpdatedAt, so a late webhook event older than state already pulled from the API is discarded rather than overwriting it - live script asserts CONVEX_SITE_URL is present instead of failing with an opaque URL parse error Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
There was a problem hiding this comment.
1 existing issue remains and 1 new issue found across 4 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
View guided diff | Turn on auto-fix | Re-trigger cubic
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/convex/src/component/sandboxes.ts">
<violation number="1" location="packages/convex/src/component/sandboxes.ts:173">
P3: This stores local API-observation times in `remoteUpdatedAt`, but `schema.ts` describes the field only as Daytona’s timestamp for the last applied webhook. Update that field comment to document both sources so readers do not assume the ordering watermark is always provider-sourced.
(Based on your team's feedback about tracking API-observed sandbox state.)</violation>
</file>
|
Verified end-to-end with real Daytona webhook deliveries (in addition to unit tests and the live suite):
Also confirmed along the way: webhook events are per-organization, so the endpoint must live in the same org as the API key the component uses (worth knowing when debugging "no events arrive"). |
…et is shown Both confirmed during the real-delivery test: the endpoint must live in the API key's organization, and the signing secret is on the endpoint's details page in the Webhooks table. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
Summary by cubic
Adds webhook-driven sandbox state sync so records update in real time when Daytona changes sandboxes on its own (auto-stop, auto-archive, auto-delete), instead of lagging until
refreshSandboxis called. The feature is opt-in and off by default.sandbox.state.updateddeliveries to the component's webhook route (mounted viahttpPrefix) and applies them to tracked sandbox records.DAYTONA_WEBHOOK_SECRETisn't passed down, and returns 400 on signed-but-malformed payloads instead of crashing.remoteUpdatedAttimestamp — which API-observed writes also stamp, so a webhook can't overwrite newer state; events for untracked sandboxes and non-state events are acknowledged and ignored.DAYTONA_WEBHOOK_SECRETon your deployment, pass it down in the app config, and point a Daytona webhook endpoint (in the same organization as yourDAYTONA_API_KEY) athttps://<deployment>.convex.site/daytona/webhook.Written for commit b545691. Summary will update on new commits.