Skip to content

Commit a036acc

Browse files
authored
docs(claude): consolidate CLAUDE.md rules from stale branches (#608)
* docs(claude): consolidate CLAUDE.md rules from stale branches Fold the genuinely valuable guidance scattered across several now-stale worktree branches (fix/promise-race-leak, chore/parallel-claude-rule, docs/claude-fix-warnings-rule) into a single coherent pass on CLAUDE.md, and pull in two rules the socket-repo-template gold standard already carries but this repo was missing. - Restructure the Promise.race guidance into a dedicated "### Promise.race in loops" subsection with the template's Safe/Leaky/Fix triad — same content, but the three-state framing is easier to apply at review time than a single run-on bullet. - Add null-prototype rule: `{ __proto__: null, ...rest }` is already the idiom used throughout src/socket-sdk-class.ts and src/file-upload.ts (config objects, internal state, return shapes); codify it so new contributions follow suit and prototype pollution surface stays small. - Add Linear-reference rule: keep code and PR history tool-agnostic. Forward-looking — no violations currently in the repo. Deliberately NOT pulled from template: the "NEVER use dynamic imports" rule. SDK uses `await import('node:fs')` legitimately in two places (socket-sdk-class.ts:1931, :3818) for lazy loading; a blanket ban would be wrong here. * docs(sdk): expand batchPackageStream Promise.race rationale Replace the 6-line comment landed in #600 with the fuller narrative that was sitting uncommitted on the fix/promise-race-leak worktree — concrete walkthrough of the handler-stacking trap, what each piece of the single-waiter machinery is responsible for, and why the snapshot-and- clear dance in deliverStep/deliverError matters. No behavioral change; comments only. The mechanical fix already shipped in #600 (376a743); this captures the reasoning someone will want when they look at this code six months from now.
1 parent 1ef2398 commit a036acc

2 files changed

Lines changed: 96 additions & 7 deletions

File tree

CLAUDE.md

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,16 @@ The umbrella rule: never run a git command that mutates state belonging to a pat
4141
- 🚨 **NEVER use `npx`, `pnpm dlx`, or `yarn dlx`** — use `pnpm exec <package>` or `pnpm run <script>`. Add tools as pinned devDependencies first.
4242
- **minimumReleaseAge**: NEVER add packages to `minimumReleaseAgeExclude` in CI. Locally, ASK before adding — the age threshold is a security control.
4343
- File existence: ALWAYS `existsSync` from `node:fs`. NEVER `fs.access`, `fs.stat`-for-existence, or an async `fileExists` wrapper. Import form: `import { existsSync, promises as fs } from 'node:fs'`.
44-
- `Promise.race` / `Promise.any`: NEVER pass a long-lived promise (interrupt signal, pool member) into a race inside a loop. Each call re-attaches `.then` handlers to every arm; handlers accumulate on surviving promises until they settle. For concurrency limiters, use a single-waiter "slot available" signal (resolved by each task's `.then`) instead of re-racing `executing[]`. See nodejs/node#17469 and `@watchable/unpromise`. Race with two fresh arms (e.g. one-shot `withTimeout`) is safe.
44+
- Null-prototype objects: ALWAYS use `{ __proto__: null, ...rest }` for config, return, and internal-state objects. Prevents prototype pollution and accidental inheritance. See `src/socket-sdk-class.ts` and `src/file-upload.ts` for examples.
45+
- Linear references: NEVER reference Linear issues (e.g. `SOC-123`, `ENG-456`, Linear URLs) in code, code comments, or PR titles/descriptions/review comments. Keep the codebase and PR history tool-agnostic — tracking lives in Linear.
46+
47+
### Promise.race in loops
48+
49+
**NEVER re-race the same pool of promises across loop iterations.** Each call to `Promise.race([A, B, ...])` attaches fresh `.then` handlers to every arm; a promise that survives N iterations accumulates N handler sets. See [nodejs/node#17469](https://github.com/nodejs/node/issues/17469) and `@watchable/unpromise`.
50+
51+
- **Safe**: `Promise.race([fresh1, fresh2])` where both arms are created per call (e.g. one-shot `withTimeout` wrappers).
52+
- **Leaky**: `Promise.race(pool)` inside a loop where `pool` persists across iterations (the classic concurrency-limiter bug) — also applies to `Promise.any` and long-lived arms like interrupt signals.
53+
- **Fix**: single-waiter "slot available" signal — each task's `.then` resolves a one-shot `promiseWithResolvers` that the loop awaits, then replaces. No persistent pool, nothing to stack.
4554

4655
---
4756

src/socket-sdk-class.ts

Lines changed: 86 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -867,30 +867,96 @@ export class SocketSdk {
867867
/* c8 ignore stop */
868868
const { components } = componentsObj
869869
const { length: componentsCount } = components
870-
// Tracks in-flight generators only for pool-size accounting.
871-
// Completed steps and errors flow through the single-waiter queue below,
872-
// not through per-generator promises re-raced each iteration — repeated
873-
// Promise.race() over the same pool accumulates unreleased .then
874-
// handlers on each still-pending arm until the pool drains.
875-
// See https://github.com/nodejs/node/issues/17469.
870+
// ─────────────────────────────────────────────────────────────────────
871+
// Why this isn't `Promise.race(running.values())` in a loop.
872+
//
873+
// The old version kept a Map<generator, promise> of every in-flight
874+
// generator step, then each iteration did:
875+
//
876+
// await Promise.race(running.values())
877+
//
878+
// That looks fine, but here's the trap: `Promise.race` attaches a
879+
// *fresh* pair of `.then(resolve, reject)` handlers to EVERY promise
880+
// it's passed — every single time it's called. Those handlers stay
881+
// attached until the promise they're on settles. The race itself
882+
// settling doesn't detach them from the losers.
883+
//
884+
// So imagine 10 generators running, and one finishes quickly while
885+
// the other 9 are slow. Each loop iteration:
886+
// - race returns (say) generator #3's step
887+
// - we kick off generator #3's next step → new race over 10 promises
888+
// - BUT the 9 slow ones still have handlers from the PREVIOUS race
889+
// hanging off them, plus the 9 new ones we just added
890+
//
891+
// After N iterations on a long-running generator's promise, that
892+
// single promise has ~N dead handler closures queued on it, each
893+
// holding references to closure state. For a batch of thousands of
894+
// components this adds up to a real memory leak, and the GC can't
895+
// help until every last generator in the pool settles.
896+
//
897+
// See https://github.com/nodejs/node/issues/17469 for the canonical
898+
// write-up, and the `@watchable/unpromise` package for the
899+
// one-shot-handler pattern we're adopting here.
900+
//
901+
// The fix: flip the direction. Instead of the main loop repeatedly
902+
// racing the pool, each generator's `.then` pushes its result into a
903+
// tiny queue (`completed`), and the main loop awaits one promise at
904+
// a time via `takeStep()`. Each generator attaches its handlers
905+
// exactly ONCE per step — no stacking, nothing to leak.
906+
// ─────────────────────────────────────────────────────────────────────
907+
908+
// `running` is now just a Set for pool-size accounting (how many
909+
// generators are still in flight). We no longer store promises here
910+
// because we don't race them — see the block comment above.
876911
const running = new Set<AsyncGenerator<BatchPackageFetchResultType>>()
912+
913+
// Buffer of steps that finished while the main loop wasn't waiting.
914+
// Happens when multiple generators resolve in the same microtask tick:
915+
// the first one wakes the waiter, the rest land here until takeStep()
916+
// drains them.
877917
const completed: GeneratorStep[] = []
918+
919+
// At most ONE waiter at a time, because the main loop awaits one step
920+
// per iteration. `undefined` means "nobody is currently awaiting".
921+
// When a step arrives and a waiter exists, we hand it the step and
922+
// clear the slot. When a step arrives and no waiter exists, we queue
923+
// it in `completed` above.
878924
let waiter:
879925
| {
880926
reject: (err: unknown) => void
881927
resolve: (step: GeneratorStep) => void
882928
}
883929
| undefined
930+
931+
// If a generator rejects while nobody is awaiting, we stash the error
932+
// here so the NEXT takeStep() call can surface it. We only keep the
933+
// first error (matches the old Promise.race behavior — first rejection
934+
// wins, later ones are swallowed). Wrapped in an object so we can
935+
// distinguish "no error" (undefined) from "error was literally
936+
// undefined" (a `{ err: undefined }` object).
884937
let pendingError: { err: unknown } | undefined
938+
939+
// Called from a generator's `.then` success path. Two cases:
940+
// 1. The main loop is parked in takeStep() → wake it directly.
941+
// 2. The main loop is busy → buffer the step for later.
942+
// Either way, `.then` fires exactly once per generator step, so no
943+
// handlers pile up on long-lived promises.
885944
const deliverStep = (step: GeneratorStep) => {
886945
if (waiter) {
946+
// Snapshot + clear before calling resolve, in case resolve
947+
// synchronously triggers another deliverStep/takeStep cycle and
948+
// we don't want to hand the next step to a stale waiter.
887949
const w = waiter
888950
waiter = undefined
889951
w.resolve(step)
890952
} else {
891953
completed.push(step)
892954
}
893955
}
956+
957+
// Mirror of deliverStep for the rejection path. Same snapshot-then-
958+
// clear dance. If no waiter and no prior pendingError, remember this
959+
// one so the next takeStep() can throw it.
894960
const deliverError = (err: unknown) => {
895961
if (waiter) {
896962
const w = waiter
@@ -900,6 +966,15 @@ export class SocketSdk {
900966
pendingError = { err }
901967
}
902968
}
969+
970+
// The main loop's only way to wait for progress. Priority:
971+
// 1. Surface any stashed error immediately (fail-fast).
972+
// 2. Return a buffered step if one is queued (no await needed —
973+
// `Promise.resolve(x)` still yields a microtask, but no new
974+
// handler chains get attached to long-lived promises).
975+
// 3. Otherwise register ourselves as THE waiter and park on a
976+
// fresh promise. Because only one slot exists, there's never
977+
// more than one handler outstanding.
903978
const takeStep = (): Promise<GeneratorStep> => {
904979
if (pendingError) {
905980
const { err } = pendingError
@@ -927,6 +1002,11 @@ export class SocketSdk {
9271002
continueGen(generator)
9281003
index += chunkSize
9291004
}
1005+
// Kick off (or continue) a single generator. The key detail: we
1006+
// attach `.then` to the `.next()` promise EXACTLY ONCE. That promise
1007+
// will settle once, our handlers fire once, and nothing else is ever
1008+
// chained on top of it. This is the whole point of the refactor —
1009+
// no `Promise.race` loop means no re-attaching handlers every tick.
9301010
const continueGen = (
9311011
generator: AsyncGenerator<BatchPackageFetchResultType>,
9321012
) => {

0 commit comments

Comments
 (0)