Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| Effect.gen(function* () { | ||
| const activation = yield* ServerActivation; | ||
| const detached = withoutAmbientParentSpan(effect); |
There was a problem hiding this comment.
🟠 High src/serverActivation.ts:25
withoutAmbientParentSpan casts the context after removing Tracer.ParentSpan back to Context.Context<NoInfer<R>>, so the return type still claims to satisfy effects whose environment explicitly includes Tracer.ParentSpan. If an effect passed to withoutAmbientParentSpan (or to forkParked, which wraps every input with it) directly uses Tracer.ParentSpan, the code type-checks but the forked fiber fails at runtime with Service not found because the service was removed from the child context. The cast masks a missing-service defect rather than surfacing it as a type error. Consider constraining the input type so Tracer.ParentSpan cannot appear in R.
- (context) => Context.omit(Tracer.ParentSpan)(context) as Context.Context<NoInfer<R>>,
+ (context) => Context.omit(Tracer.ParentSpan)(context) as Context.Context<Exclude<NoInfer<R>, Tracer.ParentSpan>>,
+ // Note: this cast is unsafe if R explicitly requires Tracer.ParentSpan;
+ // callers should ensure the input does not use that service directly.🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/serverActivation.ts around lines 25-27:
`withoutAmbientParentSpan` casts the context after removing `Tracer.ParentSpan` back to `Context.Context<NoInfer<R>>`, so the return type still claims to satisfy effects whose environment explicitly includes `Tracer.ParentSpan`. If an effect passed to `withoutAmbientParentSpan` (or to `forkParked`, which wraps every input with it) directly uses `Tracer.ParentSpan`, the code type-checks but the forked fiber fails at runtime with `Service not found` because the service was removed from the child context. The cast masks a missing-service defect rather than surfacing it as a type error. Consider constraining the input type so `Tracer.ParentSpan` cannot appear in `R`.
There was a problem hiding this comment.
Addressed in 17580be — return type is now Effect.Effect<A, E, Exclude<R, Tracer.ParentSpan>> with a matching context cast, so the stripped env is reflected in the type.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
There was a problem hiding this comment.
Pull request overview
Fixes idle private-memory growth in the desktop/server process by preventing long-lived fibers (parked roots + background pollers) from inheriting short-lived startup Tracer.ParentSpan context, and by reducing tracing work on hot/idle paths.
Changes:
- Add
withoutAmbientParentSpanand apply it inforkParkedto detach inheritedTracer.ParentSpanfor long-lived roots. - Reduce tracing overhead on idle/background loops (PortDiscovery polling, secret reads) and remove the
PortDiscovery.makewrapping span. - Add a focused unit test validating that
withoutAmbientParentSpanclears inheritedTracer.ParentSpanwithout affecting sibling effects.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/server/src/serverRuntimeStartup.ts | Documents why parked startup roots must detach ambient ParentSpan to avoid pinning startup spans. |
| apps/server/src/serverActivation.ts | Introduces withoutAmbientParentSpan and applies it to all forkParked forks. |
| apps/server/src/serverActivation.test.ts | Adds a unit test asserting ParentSpan detachment behavior. |
| apps/server/src/preview/PortScanner.ts | Detaches the poll fiber from ambient parent spans and disables tracing in the poll loop; removes PortDiscovery.make span wrapper. |
| apps/server/src/auth/ServerSecretStore.ts | Disables tracing for frequent secret reads to reduce idle trace volume and span retention. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(stillHasParent).toBe(true); | ||
|
|
||
| const stripped = Context.omit(Tracer.ParentSpan)(Context.make(Tracer.ParentSpan, ambient)); | ||
| expect(Context.getOption(stripped, Tracer.ParentSpan)._tag).toBe("None"); |
There was a problem hiding this comment.
Fixed in 17580be — switched to Option.isNone.
| // Single layer-scoped polling fiber. Ticks are no-ops when no client is | ||
| // currently retained, so the cost is one Ref.get every POLL_INTERVAL. | ||
| yield* Effect.forkScoped(pollTick().pipe(Effect.repeat(Schedule.spaced(POLL_INTERVAL)))); | ||
|
|
||
| // currently retained. Run detached from any ambient ParentSpan (e.g. | ||
| // PortDiscovery.make) and without tracing idle no-ops — otherwise every | ||
| // 3s tick parents under an immortal span and grows server private memory. | ||
| yield* Effect.forkScoped( | ||
| withoutAmbientParentSpan( | ||
| pollTick().pipe( | ||
| Effect.repeat(Schedule.spaced(POLL_INTERVAL)), | ||
| Effect.withTracerEnabled(false), | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Good catch — fixed in 17580be. Idle early-return now sits outside Effect.fn("PortDiscovery.pollTick") so no-op ticks create no span, and the blanket withTracerEnabled(false) on the poll loop is gone so active scans still get traced. ParentSpan detach stays via withoutAmbientParentSpan.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This memory-leak fix modifies span/tracing behavior for long-lived server processes. An unresolved high-severity comment identifies a type cast that could mask runtime 'Service not found' errors, which warrants human review to ensure correctness. You can customize Macroscope's approvability policy. Learn more. |
Long-lived forkParked roots and PortDiscovery poll loops were inheriting Effect ParentSpan from short-lived startup/make spans. Nested hot-path spans (secret reads, idle poll ticks) kept parenting under those immortal spans and drove ~25-30MB/h private growth while idle. Detach ambient ParentSpan in forkParked, untrace idle port polls and secret gets, and drop PortDiscovery.make's wrapping span. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Keep active PortDiscovery polls traced, tighten withoutAmbientParentSpan's R, and use Option.isNone in the test. Co-authored-by: Cursor <cursoragent@cursor.com>
17580be to
655748b
Compare
|
Closing in favor of #9824, which reimplements the same idle ParentSpan / background-fiber diagnosis with a type-safe Thanks @Benchance — the original analysis and first fix attempt here were what unblocked the follow-up. |
Summary
forkParkedroots and the PortDiscovery poll loop were inheriting EffectParentSpanfrom short-lived startup/makespans, so hot-path nested spans kept parenting under immortal spans.ParentSpaninforkParked, untrace idle port polls + secret gets, and dropPortDiscovery.make's wrapping span.Test plan
vp test run src/serverActivation.test.ts src/preview/PortScanner.test.ts(apps/server)server.trace.ndjsonno longer floods withPortDiscovery.pollTick/ServerSecretStore.getunderstartup.phase=reactors.startFixes #5410
Note
Low Risk
Changes only Effect tracing context and span emission on background fibers and secret reads; no auth, data, or business-logic paths. Main risk is reduced trace detail during idle periods, not functional regression.
Overview
Fixes steady idle Private memory growth (#5410) by breaking the link between short-lived startup spans and long-lived background work.
Adds
withoutAmbientParentSpaninserverActivation.tsto removeTracer.ParentSpanfrom an effect’s context.forkParkednow forks through that helper so reactor and other long-running roots no longer pin immortal parent spans.PortDiscovery polling: the retain-count idle check lives outside
Effect.fnso no-op ticks don’t create spans; the repeating poll fiber runs insidewithoutAmbientParentSpan. ThePortDiscovery.makewrapper span is removed.ServerSecretStore.getswitches from a per-call named span toEffect.withTracerEnabled(false), cutting trace noise on a hot path.Observability: fewer or no spans for idle poll ticks, secret reads, and some layer roots; preview port discovery behavior is unchanged when retained.
Reviewed by Cursor Bugbot for commit 655748b. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Stop idle private-memory growth from immortal parent spans in background fibers
withoutAmbientParentSpanhelper in serverActivation.ts that stripsTracer.ParentSpanfrom an effect's context, preventing long-lived forked fibers from holding a reference to a parent span.withoutAmbientParentSpaninforkParkedso all long-running roots it forks no longer inherit an ambient parent span.PortDiscoverypolling in PortScanner.ts so idle no-op ticks skip span creation entirely, and the background polling fiber runs without an inherited parent span.ServerSecretStore.getin ServerSecretStore.ts instead of creating a named span per call.ServerSecretStore.get, or thePortDiscovery.makeandforkParkedroots.Macroscope summarized 655748b.