Skip to content

Commit e9ee072

Browse files
d-csd-cs
committed
fix(webapp): signal when run-ops split read fan-out is silently disabled
The read fan-out gate decides split-mode fan-out purely by the JS object identity of the NEW vs control-plane clients. If a future change made the NEW client alias the control-plane client while both split URLs are set, fan-out would silently disable with no signal, and no test exercised the real wiring from topology selection into the gate. Warn when both run-ops URLs are set but the NEW client is not a distinct instance, and add a glue test feeding the real topology output into the real gate. The gate's enabled value is unchanged.
1 parent 6e1b607 commit e9ee072

5 files changed

Lines changed: 200 additions & 2 deletions

File tree

‎.server-changes/run-ops-split-read-gate-warn.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
area: webapp
3+
type: fix
4+
---
5+
6+
Warn when the run-ops split read configuration is present but read fan-out is silently disabled, instead of failing silently

‎apps/webapp/app/db.server.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,7 @@ export const runOpsSplitReadEnabled: boolean = computeRunOpsSplitReadEnabled({
290290
controlPlaneReplica: $replica,
291291
hasNewUrl: !!env.RUN_OPS_DATABASE_URL,
292292
hasLegacyUrl: !!env.RUN_OPS_LEGACY_DATABASE_URL,
293+
logger,
293294
});
294295

295296
// Boot-time interlock: if the flag is on but the distinct-DB sentinel does not

‎apps/webapp/app/v3/runOpsMigration/runOpsSplitReadGate.ts‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,21 @@ export function computeRunOpsSplitReadEnabled(args: {
66
controlPlaneReplica: unknown;
77
hasNewUrl: boolean;
88
hasLegacyUrl: boolean;
9+
logger?: { warn: (msg: string, meta?: Record<string, unknown>) => void };
910
}): boolean {
1011
const newIsDistinctDedicatedClient =
1112
args.newReplica !== args.controlPlaneWriter && args.newReplica !== args.controlPlaneReplica;
1213

13-
return newIsDistinctDedicatedClient && args.hasNewUrl && args.hasLegacyUrl;
14+
const enabled = newIsDistinctDedicatedClient && args.hasNewUrl && args.hasLegacyUrl;
15+
16+
// Configured for split but the identity check failed: fan-out is being silently disabled.
17+
if (!newIsDistinctDedicatedClient && args.hasNewUrl && args.hasLegacyUrl) {
18+
args.logger?.warn(
19+
"run-ops split read fan-out is configured (RUN_OPS_DATABASE_URL and " +
20+
"RUN_OPS_LEGACY_DATABASE_URL are both set) but the NEW client is not a distinct " +
21+
"instance from the control-plane client; read fan-out is silently disabled."
22+
);
23+
}
24+
25+
return enabled;
1426
}

‎apps/webapp/test/runOpsSplitReadGate.glue.test.ts‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
import { describe, expect, it, vi } from "vitest";
2+
import { selectRunOpsTopology } from "~/db.server";
3+
import { computeRunOpsSplitReadEnabled } from "~/v3/runOpsMigration/runOpsSplitReadGate";
4+
5+
// Glue test: nothing previously asserted that selectRunOpsTopology's ACTUAL output, fed into
6+
// computeRunOpsSplitReadEnabled, yields the right gate. Each unit was only tested against
7+
// hand-rolled objects, so a refactor that made the NEW client alias a control-plane client
8+
// (even with correct URLs) would silently disable read fan-out with zero test failure.
9+
const cp = { writer: { __tag: "cp-writer" } as any, replica: { __tag: "cp-replica" } as any };
10+
11+
describe("selectRunOpsTopology -> computeRunOpsSplitReadEnabled (seam)", () => {
12+
it("split-configured with a genuinely distinct NEW client: gate TRUE, no warn", () => {
13+
const dedicatedNew = { __tag: "dedicated-new" } as any;
14+
const topo = selectRunOpsTopology(
15+
{ splitEnabled: true, legacyUrl: "postgres://legacy", newUrl: "postgres://new" },
16+
{
17+
controlPlane: cp,
18+
buildNewWriter: vi.fn().mockReturnValue(dedicatedNew),
19+
buildNewReplica: vi.fn(),
20+
}
21+
);
22+
const warn = vi.fn();
23+
24+
const enabled = computeRunOpsSplitReadEnabled({
25+
newReplica: topo.newRunOps.replica,
26+
controlPlaneWriter: topo.controlPlane.writer,
27+
controlPlaneReplica: topo.controlPlane.replica,
28+
hasNewUrl: true,
29+
hasLegacyUrl: true,
30+
logger: { warn },
31+
});
32+
33+
expect(enabled).toBe(true);
34+
expect(warn).not.toHaveBeenCalled();
35+
});
36+
37+
it("regression: both URLs set but the client factory aliases the control-plane instance -> gate FALSE, warn fires", () => {
38+
// Stand-in for the bug this test guards against: a builder refactor that accidentally
39+
// returns the shared control-plane client instead of opening a dedicated connection.
40+
const topo = selectRunOpsTopology(
41+
{ splitEnabled: true, legacyUrl: "postgres://legacy", newUrl: "postgres://new" },
42+
{
43+
controlPlane: cp,
44+
buildNewWriter: vi.fn().mockReturnValue(cp.replica),
45+
buildNewReplica: vi.fn(),
46+
}
47+
);
48+
const warn = vi.fn();
49+
50+
const enabled = computeRunOpsSplitReadEnabled({
51+
newReplica: topo.newRunOps.replica,
52+
controlPlaneWriter: topo.controlPlane.writer,
53+
controlPlaneReplica: topo.controlPlane.replica,
54+
hasNewUrl: true,
55+
hasLegacyUrl: true,
56+
logger: { warn },
57+
});
58+
59+
expect(enabled).toBe(false);
60+
expect(warn).toHaveBeenCalledTimes(1);
61+
});
62+
63+
it("single mode (URLs unset): topology naturally collapses to control-plane refs -> gate FALSE, no warn", () => {
64+
const topo = selectRunOpsTopology(
65+
{ splitEnabled: false },
66+
{
67+
controlPlane: cp,
68+
buildNewWriter: vi.fn(),
69+
buildNewReplica: vi.fn(),
70+
}
71+
);
72+
const warn = vi.fn();
73+
74+
const enabled = computeRunOpsSplitReadEnabled({
75+
newReplica: topo.newRunOps.replica,
76+
controlPlaneWriter: topo.controlPlane.writer,
77+
controlPlaneReplica: topo.controlPlane.replica,
78+
hasNewUrl: false,
79+
hasLegacyUrl: false,
80+
logger: { warn },
81+
});
82+
83+
expect(enabled).toBe(false);
84+
expect(warn).not.toHaveBeenCalled();
85+
expect(topo.newRunOps.replica).toBe(cp.replica); // sanity: genuinely the shared instance
86+
});
87+
});

‎apps/webapp/test/runOpsSplitReadGate.test.ts‎

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { describe, expect, it } from "vitest";
1+
import { describe, expect, it, vi } from "vitest";
22
import { computeRunOpsSplitReadEnabled } from "~/v3/runOpsMigration/runOpsSplitReadGate";
33

44
// Distinct sentinel objects standing in for the prisma client singletons.
@@ -72,4 +72,96 @@ describe("computeRunOpsSplitReadEnabled", () => {
7272
false
7373
);
7474
});
75+
76+
// Observability regression guard: split-configured (both URLs set) but the NEW client is not a
77+
// distinct instance must WARN loudly. Without this signal, an accidental refactor that makes the
78+
// NEW client alias a control-plane client silently disables read fan-out with zero error/warning.
79+
describe("warn signal when configured-but-aliased", () => {
80+
it("warns when both URLs are set but NEW aliases the control-plane replica", () => {
81+
const warn = vi.fn();
82+
const enabled = computeRunOpsSplitReadEnabled({
83+
newReplica: cpReplica, // aliasing regression: NEW === control-plane replica
84+
controlPlaneWriter: cpWriter,
85+
controlPlaneReplica: cpReplica,
86+
hasNewUrl: true,
87+
hasLegacyUrl: true,
88+
logger: { warn },
89+
});
90+
91+
expect(enabled).toBe(false);
92+
expect(warn).toHaveBeenCalledTimes(1);
93+
expect(warn.mock.calls[0][0]).toMatch(/split.*configured/i);
94+
});
95+
96+
it("warns when both URLs are set but NEW aliases the control-plane writer", () => {
97+
const warn = vi.fn();
98+
const enabled = computeRunOpsSplitReadEnabled({
99+
newReplica: cpWriter, // aliasing regression: NEW === control-plane writer
100+
controlPlaneWriter: cpWriter,
101+
controlPlaneReplica: cpReplica,
102+
hasNewUrl: true,
103+
hasLegacyUrl: true,
104+
logger: { warn },
105+
});
106+
107+
expect(enabled).toBe(false);
108+
expect(warn).toHaveBeenCalledTimes(1);
109+
});
110+
111+
it("does NOT warn in ordinary single mode (both URLs unset, clients naturally aliased)", () => {
112+
const warn = vi.fn();
113+
const enabled = computeRunOpsSplitReadEnabled({
114+
newReplica: cpReplica,
115+
controlPlaneWriter: cpWriter,
116+
controlPlaneReplica: cpReplica,
117+
hasNewUrl: false,
118+
hasLegacyUrl: false,
119+
logger: { warn },
120+
});
121+
122+
expect(enabled).toBe(false);
123+
expect(warn).not.toHaveBeenCalled();
124+
});
125+
126+
it("does NOT warn when only one URL is set (not truly configured for split)", () => {
127+
const warn = vi.fn();
128+
computeRunOpsSplitReadEnabled({
129+
newReplica: cpReplica,
130+
controlPlaneWriter: cpWriter,
131+
controlPlaneReplica: cpReplica,
132+
hasNewUrl: true,
133+
hasLegacyUrl: false,
134+
logger: { warn },
135+
});
136+
137+
expect(warn).not.toHaveBeenCalled();
138+
});
139+
140+
it("does NOT warn when the NEW client is genuinely distinct (healthy split)", () => {
141+
const warn = vi.fn();
142+
const enabled = computeRunOpsSplitReadEnabled({
143+
newReplica: dedicatedNew,
144+
controlPlaneWriter: cpWriter,
145+
controlPlaneReplica: cpReplica,
146+
hasNewUrl: true,
147+
hasLegacyUrl: true,
148+
logger: { warn },
149+
});
150+
151+
expect(enabled).toBe(true);
152+
expect(warn).not.toHaveBeenCalled();
153+
});
154+
155+
it("does not throw when no logger is supplied (logger stays optional)", () => {
156+
expect(() =>
157+
computeRunOpsSplitReadEnabled({
158+
newReplica: cpReplica,
159+
controlPlaneWriter: cpWriter,
160+
controlPlaneReplica: cpReplica,
161+
hasNewUrl: true,
162+
hasLegacyUrl: true,
163+
})
164+
).not.toThrow();
165+
});
166+
});
75167
});

0 commit comments

Comments
 (0)