Skip to content

restartRequired stays true after a successful restart:true deploy: the post-deploy tails race resetRestartNeeded() #2740

Description

@kriszyp

restartRequired stays true after a successful restart:true deploy: the post-deploy tails race resetRestartNeeded()

Summary

After a deploy_component with restart: true that verifiably succeeds — code refreshed (codeVersion 1 → 3), both .env keys current — get_status.restartRequired is still true. This is the flag operators and deploy automation poll to decide whether a deploy landed, so a clean deploy reports itself as still needing a restart.

This is not the mechanism harper#1934 closed. #1934 was closed 2026-08-04 by #1936, which guarded Scope.requestRestart() behind #deployInFlight. The same PR left a structurally identical race one level up.

Mechanism (read on main)

components/Scope.ts #onDeployEnd() clears the guard first, then fires two un-awaited tails that can re-arm the flag after it is down:

#onDeployEnd(componentName: string): void {
    this.#deployInFlight = false;              // guard is now down
    ...
    for (const entryHandler of this.#entryHandlers) {
        void entryHandler.resume().catch(() => {});          // async chokidar rescan + digest compare
    }
    if (restartRequestedDuringDeploy) this.requestRestart();
    void this.applicationScope
        ?.finishDeploy()
        .then((runtimeChanged) => {
            if (runtimeChanged) this.requestRestart();       // <-- resolves after the guard is down
        })
        .catch((error) => { ...; this.requestRestart(); });

and requestRestart() only suppresses while the guard is up:

requestRestart() {
    if (this.#deployInFlight) { this.#restartRequestedDuringDeploy = true; ...; return; }
    ...
    requestRestart();   // the real shared-buffer setter
}

Nothing synchronizes those tails with the restart itself. server/threads/manageThreads.js restartWorkers() still clears the flag synchronously near its top:

const { resetRestartNeeded } = require('../../components/requestRestart.ts');
resetRestartNeeded();

and components/operations.js invokes it on the req.restart === true branch (awaitRestart(onProgress => manageThreads.restartWorkers('http', ...))) without waiting for resume() or finishDeploy() to settle.

So the ordering resetRestartNeeded() → finishDeploy() resolves runtimeChanged: true → requestRestart() leaves the flag set after a restart that already happened. For a code-changing deploy, runtimeChanged: true is the expected outcome of RuntimeModuleTracker#compare(), not an edge case — this is the common path, and only the relative timing of the async hash comparison decides whether the flag survives.

What was ruled out

The competing "each worker rebuilds its own flag" explanation does not apply: the flag is a single cross-thread shared buffer — components/requestRestart.ts backs it with Status.primaryStore.getUserSharedBuffer('restart-needed', ...), and get_status reads it through restartNeeded() in server/status/index.ts.

Impact

restartRequired: true after a successful deploy is a false positive in the operator-facing status surface. Automation that polls it either reports the deploy as unlanded or issues a redundant restart — the restart-storm pressure #1936 itself cites harper#488 for. The flag does clear on the next real restart, so it is misleading rather than wedging.

Suggested direction

Either await the post-deploy tails before the restart is issued, or make resetRestartNeeded() the last step of the restart rather than the first — clearing a flag at the start of an operation that can legitimately re-set it during that operation is the invariant being violated.

Provenance

Found in passing during QA (dispatch finding qa-wave-2026072415:2, 2026-07-24) and logged rather than asserted; blast radius unprobed. The mechanism above is a source read on main after #1934 closed, not the original hypothesis — the original cited resources/loadEnv.ts's eventType !== 'add' path, which #1936 does guard. A live timing repro (or a unit test that delays finishDeploy()'s resolution past resetRestartNeeded()) is the remaining gap.


🤖 Prepared by Claude Opus 5 (finding-triage wave 10)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    Fields

    Priority

    P2

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions