Skip to content

fix(market): bound generation installs by progress, not one total duration - #860

Open
Yuncan050115 wants to merge 1 commit into
dataelement:mainfrom
Yuncan050115:fix/generation-install-idle-timeout
Open

Yuncan050115 wants to merge 1 commit into
dataelement:mainfrom
Yuncan050115:fix/generation-install-idle-timeout

Conversation

@Yuncan050115

@Yuncan050115 Yuncan050115 commented Nov 6, 2026 •

Copy link
Copy Markdown

Refs #851.

What

defaultRunInstall armed one timer at spawn and SIGKILLed the pnpm child when it fired:

const timer = setTimeout(
  () => child.kill('SIGKILL'),
  options.installTimeoutMs ?? GENERATION_INSTALL_TIMEOUT_MS
)

installTimeoutMs has no caller anywhere in the repo, so 90 seconds is a total-duration hard limit on every generation install. pnpm streams Progress: lines continuously while downloading, so a plugin carrying a large platform payload — dsh-ops@0.4.4 pulls @dsh-ops/bash-win32-x64, 392 MB unpacked — gets killed mid-download on a cold store, code ?? 1 records it as exit 1, and diagnosticLine() has nothing but the last Progress: line to report. The reporter measured pnpm itself finishing in 73.5s on a cold store; the market path adds staging, hoisting and peer validation on top, which lands past 90s deterministically.

Change

The timeout measures idleness again rather than duration. Every stdout/stderr chunk re-arms the 90-second bound, so only a process that stopped talking dies on it. A separate hard ceiling — six minutes per phase — still ends a process that keeps producing output but never finishes; six minutes is chosen so both phases plus staging stay inside the registry lock's own budget (waiters give up at 15 minutes, a lock is judged stale at 20, no heartbeat in between; pnpm-runner already treats that fifteen-minute ceiling as the host contract). Both bounds are overridable and accept only safe positive integers, same guard style as minimumReleaseAge — a 0 or NaN override would otherwise SIGKILL every install on the next tick. A kill whose close never arrives settles after pnpm-runner's five-second grace instead of hanging the promise. Every timeout names itself in the trace and the output (generation install timed out and the pnpm process was killed (…)), so a kill no longer masquerades as a pnpm error in generation.log.

Test plan

  • New test/generation-install-timeout.test.ts drives the real defaultRunInstall through a scripted pnpm-like child: continuous progress past the old kill point completes and a generation lands (chunk interval is idle/10 so slow CI cannot starve the re-arm); a silent child dies on the idle bound with the timeout named in detail and the trace; a churning child dies at the hard ceiling; 0 / -5 / NaN / 12.5 overrides fall back to the defaults. All four shapes verified red on origin/main (the first fails with install was killed, the others produce no timeout diagnostic), green with the fix.
  • vitest run test/generation-install-timeout.test.ts test/generation-boundary.test.js test/market-installer.test.js test/generation-installer.test.ts: 78 passed / 1 failed — the failure (accepts named Harness fallback links…) reproduces identically on a clean origin/main checkout in this environment.
  • tsc --noEmit -p tsconfig.node.json: exit 0. git diff --check clean.
  • No cold-store dsh-ops end-to-end: this machine's store is warm. What the fix guarantees is that installs are bounded by stall detection rather than 90s of wall time; the payload itself is bounded only by the six-minute per-phase ceiling.

Not changed deliberately: progress stays "any byte on stdout/stderr"; the six-minute ceiling is a chosen default aligned to the lock budget, not measured per package.

…ation

defaultRunInstall armed a single 90-second timer at spawn and SIGKILLed
whatever it caught. pnpm streams Progress lines continuously while it
downloads and unpacks, so a large platform package (dsh-ops@0.4.4 pulls a
392 MB Git-for-Windows payload; the reporter measured 73.5s of pnpm alone
on a cold store) crossed the line mid-work, exited null->1, and the log
recorded only the last Progress: line as if pnpm had failed. Cold-cache
users could not install such plugins at all until their store was warm.

Idle now means idle: every chunk re-arms the 90-second clock, and a
separate per-phase hard ceiling still ends a process that keeps talking
but never finishes. The ceiling is six minutes so both phases plus
staging stay inside the registry lock's own budget (waiters give up at
15 minutes, a lock is judged stale at 20, with no heartbeat in between).
Both bounds are overridable (installTimeoutMs keeps its name with the new
meaning; installHardTimeoutMs is new) and only safe positive integers
count, mirroring the minimumReleaseAge guard: a 0 or NaN override would
otherwise SIGKILL every install on the next tick. A kill that never
reports close settles after pnpm-runner's five-second grace instead of
hanging the promise. Every timeout names itself in the trace and output,
so a kill no longer masquerades as a pnpm error in generation.log.

Adds test/generation-install-timeout.test.ts driving the real
defaultRunInstall through a scripted pnpm-like child, with chunk
intervals an order of magnitude below each idle bound: continuous
progress past the old total-duration point completes, a stalled child
dies on the idle bound, a churning child dies on the hard ceiling, and
invalid override ceilings fall back to the defaults. All fail against
the previous single-clock implementation.

Refs dataelement#851
@Yuncan050115
Yuncan050115 force-pushed the fix/generation-install-idle-timeout branch from 5f338ec to 48e818e Compare November 6, 2026 04:49
@Yuncan050115

Copy link
Copy Markdown
Author

Adversarial review folded in (48e818e):

  • Registry-lock budget conflict. The first cut used a 20-minute hard ceiling, exactly the lock's stale deadline: a two-phase install could hold withRegistryLock ~40 minutes with no mtime heartbeat, and a waiter arriving >5 minutes later would steal the lock from a live owner at 20 — two pnpm processes writing one generation registry. Ceiling is now six minutes per phase, inside the 15-minute waiter line pnpm-runner already defers to.
  • Kill grace. A SIGKILL whose close never arrives hung the promise; both clocks now arm a 5-second settle, mirroring KILL_GRACE_MS.
  • Unvalidated overrides. Both timeouts accept only safe positive integers now (0/NaN previously meant SIGKILL-on-next-tick); covered by it.each cases.
  • Test margins. The continuous-progress case runs chunk interval = idle/10 with success 650ms after the last chunk, instead of 90ms-vs-40ms spacing that could starve on a loaded Windows runner.

Wording: the notice contains the substring "error", so if pnpm emitted an ERR_PNPM_* code before stalling, diagnosticLine() still surfaces that code in detail — the trace and output tail carry the timeout line either way. The close-vs-kill race was confirmed identical to the previous single-timer behavior, not newly introduced. Re-run on 48e818e: timeout suite 7/7, families 78 passed / 1 pre-existing env failure, tsc exit 0.

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