Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/server/src/auth/ServerSecretStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ export const make = Effect.gen(function* () {
}),
),
),
Effect.withSpan("ServerSecretStore.get"),
Effect.withTracerEnabled(false),
);

const set: ServerSecretStore["Service"]["set"] = (name, value) => {
Expand Down
19 changes: 12 additions & 7 deletions apps/server/src/preview/PortScanner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import * as Schedule from "effect/Schedule";
import * as Scope from "effect/Scope";

import * as ProcessRunner from "../processRunner.ts";
import { withoutAmbientParentSpan } from "../serverActivation.ts";

export class PortDiscovery extends Context.Service<
PortDiscovery,
Expand Down Expand Up @@ -292,9 +293,8 @@ export const make = Effect.gen(function* PortDiscoveryMake() {
yield* Effect.forEach(listeners, (listener) => listener(servers), { discard: true });
});

const pollTick = Effect.fn("PortDiscovery.pollTick")(
const pollTickActive = Effect.fn("PortDiscovery.pollTick")(
function* () {
if ((yield* Ref.get(stateRef)).retainCount <= 0) return;
const next = yield* scanOnce();
const changed = yield* Ref.modify(stateRef, (state) =>
serversEqual(state.lastSnapshot, next)
Expand All @@ -308,10 +308,15 @@ export const make = Effect.gen(function* PortDiscoveryMake() {
),
);

// 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))));
// Idle early-return stays outside Effect.fn so no-op ticks create no span; detach ParentSpan (#5410).
const pollTick = Effect.gen(function* () {
if ((yield* Ref.get(stateRef)).retainCount <= 0) return;
yield* pollTickActive();
});

yield* Effect.forkScoped(
withoutAmbientParentSpan(pollTick.pipe(Effect.repeat(Schedule.spaced(POLL_INTERVAL)))),
);
const acquireRetention = Effect.fn("PortDiscovery.retain")(function* () {
const wasIdle = yield* Ref.modify(stateRef, (state) => [
state.retainCount === 0,
Expand All @@ -320,7 +325,7 @@ export const make = Effect.gen(function* PortDiscoveryMake() {
if (wasIdle) {
// Run an immediate scan + broadcast so the new retainer doesn't have
// to wait up to POLL_INTERVAL for the first emission.
yield* pollTick();
yield* pollTick;
}
});

Expand Down Expand Up @@ -385,6 +390,6 @@ export const make = Effect.gen(function* PortDiscoveryMake() {
registerTerminalProcesses,
unregisterTerminal,
});
}).pipe(Effect.withSpan("PortDiscovery.make"));
});

export const layer = Layer.effect(PortDiscovery, make);
32 changes: 31 additions & 1 deletion apps/server/src/serverActivation.test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import { expect, it } from "@effect/vitest";
import * as Context from "effect/Context";
import * as Deferred from "effect/Deferred";
import * as Effect from "effect/Effect";
import * as Option from "effect/Option";
import * as Tracer from "effect/Tracer";

import { forkParked, ServerActivation } from "./serverActivation.ts";
import { forkParked, ServerActivation, withoutAmbientParentSpan } from "./serverActivation.ts";

it.effect("proves a root is parked before returning and releases it with one gate", () =>
Effect.scoped(
Expand All @@ -21,3 +24,30 @@ it.effect("proves a root is parked before returning and releases it with one gat
}),
),
);

it.effect("withoutAmbientParentSpan drops inherited ParentSpan for long-lived roots", () =>
Effect.gen(function* () {
const ambient = Tracer.externalSpan({
traceId: "00000000000000000000000000000001",
spanId: "0000000000000001",
sampled: true,
});

const sawParent = yield* Effect.serviceOption(Tracer.ParentSpan).pipe(
Effect.map(Option.isSome),
withoutAmbientParentSpan,
Effect.provideService(Tracer.ParentSpan, ambient),
);

expect(sawParent).toBe(false);

const stillHasParent = yield* Effect.serviceOption(Tracer.ParentSpan).pipe(
Effect.map(Option.isSome),
Effect.provideService(Tracer.ParentSpan, ambient),
);
expect(stillHasParent).toBe(true);

const stripped = Context.omit(Tracer.ParentSpan)(Context.make(Tracer.ParentSpan, ambient));
expect(Option.isNone(Context.getOption(stripped, Tracer.ParentSpan))).toBe(true);
}),
);
21 changes: 19 additions & 2 deletions apps/server/src/serverActivation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,42 @@ import * as Context from "effect/Context";
import * as Deferred from "effect/Deferred";
import * as Effect from "effect/Effect";
import type * as Scope from "effect/Scope";
import * as Tracer from "effect/Tracer";

export class ServerActivation extends Context.Reference<Effect.Effect<void> | undefined>(
"t3/serverActivation",
{ defaultValue: () => undefined },
) {}

// Clear inherited ParentSpan so long-lived forks don't pin short-lived startup spans (#5410).
export const withoutAmbientParentSpan = <A, E, R>(
effect: Effect.Effect<A, E, R>,
): Effect.Effect<A, E, Exclude<R, Tracer.ParentSpan>> =>
Effect.updateContext(
effect,
(context) =>
Context.omit(Tracer.ParentSpan)(context) as Context.Context<
Exclude<NoInfer<R>, Tracer.ParentSpan>
>,
);

/** Forks a long-running root before commit and proves it is parked at the activation boundary. */
export const forkParked = <A, E, R>(
effect: Effect.Effect<A, E, R>,
): Effect.Effect<void, never, Scope.Scope | R> =>
Effect.gen(function* () {
const activation = yield* ServerActivation;
const detached = withoutAmbientParentSpan(effect);
Comment on lines 28 to +30

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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`.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

if (activation === undefined) {
yield* Effect.forkScoped(effect);
yield* Effect.forkScoped(detached);
return;
}
const parked = yield* Deferred.make<void>();
yield* Effect.forkScoped(
Deferred.succeed(parked, undefined).pipe(Effect.andThen(activation), Effect.andThen(effect)),
Deferred.succeed(parked, undefined).pipe(
Effect.andThen(activation),
Effect.andThen(detached),
),
);
yield* Deferred.await(parked);
});
Loading