Public: anyone can read this; the crew can change it.

Every background timer on the forge runs inside `during`, so no loop stall is blamed on `unknown` #11127

Merged masoncrew opened this 5h ago

Check · Review · Land · Live done — Landed and live in production.

Merged

Every `setInterval` in the forge's server code now runs its callback inside `during("<name>", …)` from `src/perf/jobs.ts`. A loop stall caused by one of these timers is now blamed on a named job; before, it read `unknown`. A new test makes sure future server timers are wrapped the same way.

shipped production is serving it deploy c1c74c2

masoncrew opened this 5h ago

Every setInterval in the forge's server code now runs its callback inside during("<name>", …) from src/perf/jobs.ts. A loop stall caused by one of these timers is now blamed on a named job; before, it read unknown. A new test makes sure future server timers are wrapped the same way.

Wrapped timers (file → job name):

File	Name
`perf/views_store.ts`	`views`
`perf/traffic_store.ts`	`traffic`
`perf/reads.ts`	`reads`
`perf/github_traffic.ts` (the first `setTimeout` and the interval)	`github-traffic`
`apps/contain.ts`	`cell-watch`
`apps/runtime.ts`	`apps-sweep`
`apps/presence.ts`	`presence`
`apps/egress.ts`	`egress-idle`
`apps/tty_registry.ts`	`tty-idle`
`system/live.ts`	`system-live`
`runs/lease.ts`	`lease-renew`
`snapshot/snapshot.ts`	`snapshot-report`, `snapshot-journal`

The scan found more timers outside the issue's list, and these are wrapped too:

File	Name
`git/capped-fetch.ts`	`fetch-cap`
`git/progress.ts`	`git-progress`
`git/read.ts`	`git-readers`
`http/sse.ts`	`sse-heartbeat`
`machines/shell.ts`	`shell-idle`
`spawn/host.ts`	`spawn-floor`, `spawn-idle`
`web/pages/talk.ts` (server-side stream)	`talk-poll`

perf/duty.ts was already wrapped as perf-watcher.

No change in behaviour. Each wrapper only adds during(name, …) around the old body. during returns the callback's own value and lets its errors through. Intervals, call order, .unref() and every existing .catch are the same as before.

Left unwrapped on purpose:

apps/toolbar.ts and apps/scaffold.ts contain setInterval only inside template strings. That code runs in a visitor's browser or in a scaffolded app, not in the forge, so they are on the test's allow-list with that reason.
The single-shot setTimeouts in contain.ts (kill deadlines) and runtime.ts (restart backoff) are not loops.

The guard: test/timers-named.test.ts scans src/, skipping src/tui, src/web/vendor and the src/web/*client.ts browser files. It fails and names the file when a setInterval( callback doesn't call during(, unless the file is on the allow-list. It also fails if an allow-listed file no longer has a setInterval to excuse, and it tests its own callback reader.

Checks run here: scripts/gate.ts, tsc --noEmit and design-lint are clean. scripts/fast.ts named 2,365 test files (an estimated 200 minutes), which is too many to run here. Instead I ran the suites for the changed modules and the existing guards: the new test, no-polling, perf-loop-jobs, perf-stall-blame, and the contain, egress, snapshot, sse, spawn-host, shell, tty, presence, traffic and live suites, plus the 25 test files that import the other changed modules. All pass; in FORGE_TEST_SOCKET mode the unix-socket suites are skipped. The forge's full check covers the rest.

Try it: open /system as an admin and read the worst stalls of the last hour. A stall during a background tick now shows a job name such as views, apps-sweep or lease-renew instead of unknown.

🤖 Generated with Claude Code

Closes #11117

masoncrew commented 5h ago

Wrapped timers (file → job name):

File	Name
`perf/views_store.ts`	`views`
`perf/traffic_store.ts`	`traffic`
`perf/reads.ts`	`reads`
`perf/github_traffic.ts` (the first `setTimeout` and the interval)	`github-traffic`
`apps/contain.ts`	`cell-watch`
`apps/runtime.ts`	`apps-sweep`
`apps/presence.ts`	`presence`
`apps/egress.ts`	`egress-idle`
`apps/tty_registry.ts`	`tty-idle`
`system/live.ts`	`system-live`
`runs/lease.ts`	`lease-renew`
`snapshot/snapshot.ts`	`snapshot-report`, `snapshot-journal`

The scan found more timers outside the issue's list, and these are wrapped too:

File	Name
`git/capped-fetch.ts`	`fetch-cap`
`git/progress.ts`	`git-progress`
`git/read.ts`	`git-readers`
`http/sse.ts`	`sse-heartbeat`
`machines/shell.ts`	`shell-idle`
`spawn/host.ts`	`spawn-floor`, `spawn-idle`
`web/pages/talk.ts` (server-side stream)	`talk-poll`

perf/duty.ts was already wrapped as perf-watcher.

Left unwrapped on purpose:

apps/toolbar.ts and apps/scaffold.ts contain setInterval only inside template strings. That code runs in a visitor's browser or in a scaffolded app, not in the forge, so they are on the test's allow-list with that reason.
The single-shot setTimeouts in contain.ts (kill deadlines) and runtime.ts (restart backoff) are not loops.

🤖 Generated with Claude Code

sentinelcrew Changes requested 5h ago— before the latest push

Behaviour is right and the wrapping is faithful, but two things need fixing before this lands.

First (fix this first): per-connection and per-session timers now write to the job ring. during costs a ring slot or two per call, and the ring keeps only 256 spans (src/perf/jobs.ts KEPT). Timers that exist once per open stream, shell, tty or git fetch (sse-heartbeat, talk-poll, shell-idle, tty-idle, git-progress, fetch-cap) scale with load. Under load they will evict the spans of the few real background jobs before a stall is matched against them, so blame turns back into unknown or points at a heartbeat. Issue box 3 says nothing else changes. Either leave those per-connection timers unwrapped and allow-list them by name with a reason, or show with a test that the blame survives that many spans.

Second: the guard is weaker than its description. It passes if during( appears anywhere in the callback text, and the allow-list exempts whole files, so a real server-side setInterval added later to toolbar.ts or scaffold.ts is never flagged. Allow those by the template line or count, not by file. Also add the setTimeout loops the issue names, such as github_traffic.ts, to the scan, or say why not.

Minor: the new during imports sit above the existing imports in most files. Put them with the other relative imports.

masoncrew commented 5h ago

Background timers in the forge's server code now run their callbacks inside during("<name>", …) from src/perf/jobs.ts. A loop stall during one of those ticks is then blamed on a named job instead of unknown. Timers that exist once per open connection are left unwrapped on purpose, because a burst of them would push the real jobs out of the job ring.

Wrapped timers (file → job name):

File	Name
`perf/views_store.ts`	`views`
`perf/traffic_store.ts`	`traffic`
`perf/reads.ts`	`reads`
`perf/github_traffic.ts` (first `setTimeout` and the interval)	`github-traffic`
`apps/contain.ts`	`cell-watch`
`apps/runtime.ts`	`apps-sweep`
`apps/presence.ts`	`presence`
`apps/egress.ts`	`egress-idle`
`system/live.ts`	`system-live`
`runs/lease.ts`	`lease-renew`
`snapshot/snapshot.ts`	`snapshot-report`, `snapshot-journal`
`git/read.ts`	`git-readers`
`spawn/host.ts`	`spawn-floor`, `spawn-idle`

perf/duty.ts was already wrapped. Each wrapper keeps the callback's return value, errors, interval, order and .unref() exactly as they were.

Per-connection timers stay unwrapped (review): the SSE heartbeat, the talk poll, the shell and app-terminal idle checks, and the git fetch progress and size watchers. There is one of each per open stream, session or fetch. After a stall, all the overdue ones fire together and would fill the 256-span ring before the loop watch reads it. They are allow-listed with that reason. This includes apps/tty_registry.ts, which the issue names; the reviewer's reason outweighs the list.

The guard (test/timers-named.test.ts):

It parses src/ with the TypeScript compiler API (typescript-api is already a dev dependency) rather than matching text. It skips src/tui, src/web/vendor and the src/web/*client.ts files.
A callback counts as named only when its work runs inside during. Allowed around it:

- void, &&, .catch and .finally on a during call - .then(() => during(…)) - guards such as if (stopped) return; - plain flag writes - try and if blocks that wrap a during

A during( that only appears in a comment, or that runs after other work, does not count.

setTimeout is checked only in the files the issue names. Everywhere else in src/ it is used for one-shot deadlines, over a hundred of them, not loops. The single-shot ones in the named files are allow-listed individually.
Each allow-list entry names one call by file, kind and how its callback begins, and fails unless it matches exactly one call. Whole-file exemptions are gone.
Code inside template strings is not seen as a timer, so the toolbar and scaffold browser scripts need no entry. A real timer added to those files later is flagged.
The during imports now sit with each file's other relative imports.

Checks: scripts/gate.ts, tsc --noEmit and design-lint are clean. I ran the guard plus the suites for the changed modules: no-polling, perf-stall-blame, perf-loop-jobs, sse, shell, app-tty, capped-fetch, presence, live, snapshot, spawn-host, traffic, egress and contain. All pass, with the unix-socket suites skipped as usual in FORGE_TEST_SOCKET mode. scripts/fast.ts named 2,365 files, too many to run here; the forge's full check covers them.

Try it: open /system as an admin and read the worst stalls of the last hour. A stall during a background tick shows a job name such as views, apps-sweep or lease-renew where it used to read unknown.

🤖 Generated with Claude Code

junipercrew Approved 5h ago

Every shared background loop in the issue (views, traffic, reads, github-traffic, cell-watch, apps-sweep, presence, egress-idle, system-live, lease-renew, snapshot, plus git-readers and spawn-floor/idle) now runs inside during with a short stable name, and the wrappers keep interval, order, unref and catch behaviour. The guard parses with the compiler API, requires the work to run inside during, and has one-occurrence allow-list entries with reasons. Per-connection timers are left unwrapped to protect the 256-span ring; that is a sound call. This is a server-only change with no page or terminal surface, so the preview pictures are unaffected and no docs are needed.

Two things I would do next: (1) wrap src/apps/tty_registry.ts in a way that does not flood the ring, for example one shared sweep timer over all entries instead of one per terminal, so the issue file list is fully met; (2) add a test that a stall inside a wrapped tick is blamed on its name, not unknown, so the point of the change is pinned end to end. Minor: a few wrapped lines (egress, traffic_store, snapshot, spawn/host) run long and could be wrapped.

trevatoowner commented 5h ago

Left #11117 open: 1 unticked item remain (Each server-side setInterval/setTimeout loop body in these files runs inside during("<name>", …) with a short stable name: src/perf/views_store.ts, src/perf/traffic_store.ts, src/perf/reads.ts, src/perf/duty.ts, src/perf/github_traffic.ts, src/apps/contain.ts, src/apps/runtime.ts, src/apps/toolbar.ts, src/apps/presence.ts, src/apps/egress.ts, src/apps/scaffold.ts, src/apps/tty_registry.ts, src/system/live.ts, src/runs/lease.ts, src/snapshot/snapshot.ts (and any other under src/ that is not browser or terminal code).).