-
Notifications
You must be signed in to change notification settings - Fork 262
[web] Probe deployment specVersion before replaying run #1782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@workflow/web': patch | ||
| --- | ||
|
|
||
| Replay/Re-run probes the target deployment's specVersion via health check before recreating the run, so the correct queue transport (JSON for old deployments, CBOR for new) is used. Falls back to the original run's specVersion if the probe fails. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -815,11 +815,35 @@ export async function recreateRun( | |
| ): Promise<ServerActionResult<string>> { | ||
| try { | ||
| const world = await getWorldFromEnv({ ...worldEnv }); | ||
|
|
||
| // Probe the target deployment's specVersion via health check so we use | ||
| // the correct queue transport (JSON for old deployments, CBOR for new). | ||
| // Falls back to the run's specVersion inside recreateRunFromExisting | ||
| // if the probe fails (e.g. old deployment without health check support). | ||
| let specVersion: number | undefined; | ||
| try { | ||
| let targetDeploymentId = deploymentId; | ||
| if (!targetDeploymentId) { | ||
| const run = await world.runs.get(runId, { resolveData: 'none' }); | ||
| targetDeploymentId = run.deploymentId; | ||
| } | ||
| const hc = await healthCheck(world, 'workflow', { | ||
| deploymentId: targetDeploymentId, | ||
| timeout: 10_000, | ||
| }); | ||
| if (hc.healthy && hc.specVersion != null) { | ||
| specVersion = hc.specVersion; | ||
| } | ||
| } catch { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Latency concern: this adds up to 10s of UI wait for the exact case the PR is trying to help. The This means: a user clicking "Replay" on a run from a pre-health-check deployment pays 10 seconds of UI latency before the replay even starts. That's the exact scenario the PR needs to handle gracefully (old deployment \u2192 JSON transport), but it's now the slowest case. Options to consider:
Option 1 is the simplest and probably sufficient. |
||
| // Health check failed — fall back to run's specVersion. | ||
| } | ||
|
|
||
| const newRunId = await workflowRunHelpers.recreateRunFromExisting( | ||
| world, | ||
| runId, | ||
| { | ||
| deploymentId, | ||
| specVersion, | ||
| } | ||
| ); | ||
| return createResponse(newRunId); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: when
deploymentIdis not provided, this callsworld.runs.get(runId)to get the run's deploymentId, and thenrecreateRunFromExistinginternally callsworld.runs.get(runId, { resolveData: 'all' })again a moment later. Not a correctness issue, just a duplicated round-trip. Could be avoided by fetching the run once here and passing bothrunand the resolved ID through, but that would require a larger signature change onrecreateRunFromExisting. Probably not worth it for a non-hot path.