Skip to content

fix(#8): surface lease-release failure on forced exit and guard hydrate - #6

Open
Jackallink wants to merge 1 commit into
mainfrom
fix/wiring-stop-backstop-exit-code-8
Open

fix(#8): surface lease-release failure on forced exit and guard hydrate#6
Jackallink wants to merge 1 commit into
mainfrom
fix/wiring-stop-backstop-exit-code-8

Conversation

@Jackallink

Copy link
Copy Markdown
Owner

Closes: Jackallink/qm-integration#8

src/wiring.ts stopWithBackstop hard-exit path used .finally(() => process.exit(0)). If releaseInFlightRuns rejected, the failure was logged implicitly and the exit was still 0, so containers/systemd saw a clean shutdown and never restarted. The graceful-stop failure path had the same issue at line 328. Also, void identity.hydrate() at line 460 dropped the derived promise, so a slow/dead identity store unhandled-rejected into a Node crash.

Replace the hard-exit .finally with a .then(onResolve, onReject) that exits 1 on rejection after logging errMessage(e); the graceful-stop failure path now .catch()es then .finally()es exit(1). Add .catch(() => undefined) to identity.hydrate so a dead identity backend cannot crash the boot. configStore.hydrate at line 491 is the same pattern and filed as Jackallink/qm-integration#14.

Affected tests

  • No direct unit test (boot/shutdown path)
  • Manual: SIGTERM during stuck graceful-stop, observe exit code 1

Reviewer checklist

@Jackallink

Copy link
Copy Markdown
Owner Author

Independent review — 2026-09-04

Findings on PR #6

1. Hard-exit handler: releaseInFlightRuns may still be running after exit

The hardExit path now uses a Promise.race between releaseInFlightRuns and a 3s sleep. If releaseInFlightRuns takes longer than 3s, sleep resolves first, the race wins with sleep, and process.exit(0) runs. But releaseInFlightRuns is still in flight — it keeps running in the background until the process dies. The exit happens synchronously so the orphan promise is GC'd with the process; net effect is small but the in-flight call could have done partial writes.

Same shape for the graceful-stop failure path. In practice, process.exit is the hammer, not a graceful cancel — this matches the function name stopWithBackstop. Acceptable.

2. Commit message clarity

The commit message says "surface lease-release failure on forced exit". The hardExit path's new exit code is 1 (was 0); the graceful-stop failure path's exit code was already 1 (was 1). Only the hardExit changes behavior. Suggest making that clearer in the PR body / commit message: only ONE of the two paths actually had its exit code changed.

3. configStore.hydrate sibling

Filed as qm-integration#14. Same pattern, separate fix path.

4. AGENTS.md check

  • Refs Jackallink/qm-integration#8
  • No upstream issue numbers
  • 0 new comments
  • Item 2 (commit message could be clearer)

No blocking findings. The fix is correct.

@Jackallink

Copy link
Copy Markdown
Owner Author

Deep-review pass addendum — 2026-09-04

Verified on branch fix/wiring-stop-backstop-exit-code-8 (f695e8a):

  1. F6.3 (new): void identity.hydrate().catch(() => undefined) swallows the failure with zero signal. In src/identity/identity-service.ts, hydrate() caches hydrateP forever — if the initial load rejects (e.g. PG blip at boot), every later hydrate() returns the same rejected promise; there is no retry path inside hydrate. The only self-heal is refresh(), which resets refreshP and reloads, and it is called refresh-before-classify in the gate (src/api/server.ts:174/258), plus run-trigger.ts:136, app-turn.ts:62, git-http-broker.ts:135, deployments.ts:64. Consequences: (a) the boot failure is invisible (no log line); (b) until the first refresh-triggering request, deactivated principals classify as "internal" — a transient fail-open window. Suggest: log the error in the catch, and consider resetting hydrateP = null on failure so a later hydrate can retry.

  2. F6 test-gap stands: no test covers stopWithBackstop (grep across test/ finds zero references). Typecheck passes on the branch. The exit-code change (hardExit path 0 → 1) is entirely untested. Suggest a unit test that injects a mock runtime (releaseInFlightRuns rejects / hangs) and asserts the exit code — with process.exit mocked or the function refactored to return the code.

  3. F6.1 / F6.2 stand as stated; neither is blocking.

…guard hydrate

src/wiring.ts stopWithBackstop hard-exit path used
.finally(() => process.exit(0)). If releaseInFlightRuns rejected, the failure
was logged implicitly and the exit was still 0, so containers/systemd saw a
clean shutdown and never restarted. The graceful-stop failure path had the
same issue at line 328. Also, void identity.hydrate() at line 460 dropped the
derived promise, so a slow/dead identity store unhandled-rejected into a Node
crash.

Replace the hard-exit .finally with a .then(onResolve, onReject) that exits 1
on rejection after logging errMessage(e); the graceful-stop failure path now
.catch()es then .finally()es exit(1). Add .catch(() => undefined) to
identity.hydrate so a dead identity backend cannot crash the boot. configStore.
hydrate at line 491 is the same pattern and filed as
Jackallink/qm-integration#14.

Refs: Jackallink/qm-integration#8
@Jackallink
Jackallink force-pushed the fix/wiring-stop-backstop-exit-code-8 branch from f695e8a to 792a9ad Compare September 6, 2026 02:00
@Jackallink

Copy link
Copy Markdown
Owner Author

Amended — 2026-09-04 (addresses deep-review F6.3, partially)

  • void identity.hydrate().catch(() => undefined) now logs the failure via errMessage:
    void identity.hydrate().catch((e) => console.error("[wiring] identity hydrate failed:", errMessage(e))).
  • The deeper half of F6.3 — hydrateP caches a rejected promise forever with no retry inside hydrate (refresh self-heals on the first gated request, leaving a transient fail-open window) — stays for v6; it is a change to identity-service.ts, outside this PR's wiring scope.
  • Typecheck clean on the amended branch. F6 test-gap (no test covers stopWithBackstop) remains v6 polish.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant