Document that deploy_component waits for restart: true and reports restart_completed - #638
Conversation
…rue and reports restart_completed
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-638 This preview will update automatically when you push new commits. |
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-638 This preview will update automatically when you push new commits. |
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-638 This preview will update automatically when you push new commits. |
…oved response field
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-638 This preview will update automatically when you push new commits. |
…se take install + restart time
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-638 This preview will update automatically when you push new commits. |
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
Approving. I verified every claim in the diff against harper origin/main and they all hold:
restart: truewaits before responding —components/operations.js:809-811awaitsawaitRestart(...)- Progress-based rather than fixed-timeout —
awaitRestart.ts's own header says exactly that - The three give-up conditions map to real
RestartOutcomefields (stalled, the ceiling,workersKeptOnOldCode/replacementsNotStarted) - The outcome is logged —
logRestartOutcome(restart, ...)at:814 - A failed restart genuinely does not fail the deploy — the result is not thrown on, and execution proceeds to the success message
"rolling"starts areplicated: truerestart_servicejob and returnsrestartJobId(:818-826)drop_componentwaits the same way — secondawaitRestartcall site at:1343restart_completedis correctly absent: zero occurrences anywhere in harper
On the badge: components/awaitRestart.ts does not exist at v5.2.6, v5.2.7 or v5.2.8, so the change is merged but unreleased and v5.3.0 is defensible. (Checked by testing for the file at each tag rather than --contains, and dereferencing with ^{} — git ls-remote refs/tags/v5.2.8 reports the annotated tag object 85fbf969e, not the commit 23264cebb, which is an easy way to conclude a phantom divergence.)
Two non-blocking notes, neither worth holding this up for.
The two bounds are exported constants but not documented. awaitRestart.ts exports RESTART_IDLE_TIMEOUT_MS = 60_000 and RESTART_WAIT_CEILING_MS = 600_000. The text says the response "can take as long as the install plus the restart — tens of seconds on a slow install with many worker threads" and advises that a caller with a short request timeout should use rolling instead. That advice is right, but "tens of seconds" anchors an order of magnitude below the real ten-minute ceiling, and "short" is undefined when there is an exact number available. A CI author sizing a client timeout from this could pick 60s and get cut off partway through a legitimately slow restart — the same failure this change prevents, just relocated from the server to the client. Worth stating both numbers in a follow-up.
The title is stale. It still says "and reports restart_completed", which the body correctly explains was dropped from #2341 and therefore from here. I am correcting it on the squash so it does not land in the commit message.
I also updated the branch before merging — it was 32 commits behind, and operations.md had moved in five of them, so CI had passed against a stale base.
sent with Claude Opus 5
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-638 This preview will update automatically when you push new commits. |
🧹 Preview CleanupThe preview deployment for this PR has been removed. |
Documents the
deploy_componentrestart behavior that HarperFast/harper#2341 changes:"restart": truenow waits for the worker restart before responding, so a successful response means every worker thread is serving the new code.drop_componentwaits the same way.The old behavior was undocumented and load-bearing for callers: the operation reported success while the rolling restart was still running, so a client acting on that success could be served by a worker that had never loaded the component (harper#2335).
For the human reviewer
v5.3.0, per Kris's call — the change ships in the next minor rather than a 5.2 patch. PerAGENTS.mdthis still gets re-checked at merge time.#### Restarting (restart)subsection underdeploy_component, next to the existing credentials subsection, rather than expanding the one-line mention in the intro paragraph. The alternative is a shorter note inline; the subsection wins if therestart_completedsemantics are worth spelling out for CI callers, which is the audiencelearn/developers/deploying-from-ci.mdxtargets.Verification
npm run format:writethennpm run format:check— clean. Claims checked against the implementation: the awaited restart and its give-up conditions incomponents/operations.jsandcomponents/awaitRestart.ts, and the"rolling"path's replicatedrestart_servicejob (it does restart peers — an earlier draft of this text said it did not). An earlier revision of this PR documented arestart_completedresponse field; that field was dropped from #2341, and so from here.