-
-
Notifications
You must be signed in to change notification settings - Fork 722
Allow creating and monitoring run replication services with different settings #2055
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,122 @@ | ||||||||||||||||||||||||||||||||||||||||||||
import { ActionFunctionArgs, json } from "@remix-run/server-runtime"; | ||||||||||||||||||||||||||||||||||||||||||||
import { prisma } from "~/db.server"; | ||||||||||||||||||||||||||||||||||||||||||||
import { authenticateApiRequestWithPersonalAccessToken } from "~/services/personalAccessToken.server"; | ||||||||||||||||||||||||||||||||||||||||||||
import { z } from "zod"; | ||||||||||||||||||||||||||||||||||||||||||||
import { ClickHouse } from "@internal/clickhouse"; | ||||||||||||||||||||||||||||||||||||||||||||
import { env } from "~/env.server"; | ||||||||||||||||||||||||||||||||||||||||||||
import { RunsReplicationService } from "~/services/runsReplicationService.server"; | ||||||||||||||||||||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||||||||||||||||||||
getRunsReplicationGlobal, | ||||||||||||||||||||||||||||||||||||||||||||
setRunsReplicationGlobal, | ||||||||||||||||||||||||||||||||||||||||||||
} from "~/services/runsReplicationGlobal.server"; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
const CreateRunReplicationServiceParams = z.object({ | ||||||||||||||||||||||||||||||||||||||||||||
name: z.string(), | ||||||||||||||||||||||||||||||||||||||||||||
keepAliveEnabled: z.boolean(), | ||||||||||||||||||||||||||||||||||||||||||||
keepAliveIdleSocketTtl: z.number(), | ||||||||||||||||||||||||||||||||||||||||||||
maxOpenConnections: z.number(), | ||||||||||||||||||||||||||||||||||||||||||||
maxFlushConcurrency: z.number(), | ||||||||||||||||||||||||||||||||||||||||||||
flushIntervalMs: z.number(), | ||||||||||||||||||||||||||||||||||||||||||||
flushBatchSize: z.number(), | ||||||||||||||||||||||||||||||||||||||||||||
leaderLockTimeoutMs: z.number(), | ||||||||||||||||||||||||||||||||||||||||||||
leaderLockExtendIntervalMs: z.number(), | ||||||||||||||||||||||||||||||||||||||||||||
leaderLockAcquireAdditionalTimeMs: z.number(), | ||||||||||||||||||||||||||||||||||||||||||||
leaderLockRetryIntervalMs: z.number(), | ||||||||||||||||||||||||||||||||||||||||||||
ackIntervalSeconds: z.number(), | ||||||||||||||||||||||||||||||||||||||||||||
waitForAsyncInsert: z.boolean(), | ||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
type CreateRunReplicationServiceParams = z.infer<typeof CreateRunReplicationServiceParams>; | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
export async function action({ request }: ActionFunctionArgs) { | ||||||||||||||||||||||||||||||||||||||||||||
// Next authenticate the request | ||||||||||||||||||||||||||||||||||||||||||||
const authenticationResult = await authenticateApiRequestWithPersonalAccessToken(request); | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
if (!authenticationResult) { | ||||||||||||||||||||||||||||||||||||||||||||
return json({ error: "Invalid or Missing API key" }, { status: 401 }); | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
const user = await prisma.user.findUnique({ | ||||||||||||||||||||||||||||||||||||||||||||
where: { | ||||||||||||||||||||||||||||||||||||||||||||
id: authenticationResult.userId, | ||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
if (!user) { | ||||||||||||||||||||||||||||||||||||||||||||
return json({ error: "Invalid or Missing API key" }, { status: 401 }); | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
if (!user.admin) { | ||||||||||||||||||||||||||||||||||||||||||||
return json({ error: "You must be an admin to perform this action" }, { status: 403 }); | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
try { | ||||||||||||||||||||||||||||||||||||||||||||
const globalService = getRunsReplicationGlobal(); | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
if (globalService) { | ||||||||||||||||||||||||||||||||||||||||||||
return json( | ||||||||||||||||||||||||||||||||||||||||||||
{ error: "Global runs replication service already exists. Stop it first." }, | ||||||||||||||||||||||||||||||||||||||||||||
{ status: 400 } | ||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
const params = CreateRunReplicationServiceParams.parse(await request.json()); | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
const service = createRunReplicationService(params); | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
setRunsReplicationGlobal(service); | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
await service.start(); | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+65
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move global registration after a successful If - const service = createRunReplicationService(params);
-
- setRunsReplicationGlobal(service);
-
- await service.start();
+ const service = createRunReplicationService(params);
+
+ await service.start(); // ensure we’re fully up
+
+ setRunsReplicationGlobal(service); Optionally wrap the 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
return json({ | ||||||||||||||||||||||||||||||||||||||||||||
success: true, | ||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||
} catch (error) { | ||||||||||||||||||||||||||||||||||||||||||||
return json({ error: error instanceof Error ? error.message : error }, { status: 400 }); | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+74
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Avoid leaking internal error messages to API consumers Returning raw - } catch (error) {
- return json({ error: error instanceof Error ? error.message : error }, { status: 400 });
+ } catch (error) {
+ console.error("Failed to create runs replication service", error);
+ return json({ error: "Failed to create runs replication service" }, { status: 400 });
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
function createRunReplicationService(params: CreateRunReplicationServiceParams) { | ||||||||||||||||||||||||||||||||||||||||||||
const clickhouse = new ClickHouse({ | ||||||||||||||||||||||||||||||||||||||||||||
url: env.RUN_REPLICATION_CLICKHOUSE_URL, | ||||||||||||||||||||||||||||||||||||||||||||
name: params.name, | ||||||||||||||||||||||||||||||||||||||||||||
keepAlive: { | ||||||||||||||||||||||||||||||||||||||||||||
enabled: params.keepAliveEnabled, | ||||||||||||||||||||||||||||||||||||||||||||
idleSocketTtl: params.keepAliveIdleSocketTtl, | ||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||
logLevel: "debug", | ||||||||||||||||||||||||||||||||||||||||||||
compression: { | ||||||||||||||||||||||||||||||||||||||||||||
request: true, | ||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||
maxOpenConnections: params.maxOpenConnections, | ||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
const service = new RunsReplicationService({ | ||||||||||||||||||||||||||||||||||||||||||||
clickhouse: clickhouse, | ||||||||||||||||||||||||||||||||||||||||||||
pgConnectionUrl: env.DATABASE_URL, | ||||||||||||||||||||||||||||||||||||||||||||
serviceName: params.name, | ||||||||||||||||||||||||||||||||||||||||||||
slotName: env.RUN_REPLICATION_SLOT_NAME, | ||||||||||||||||||||||||||||||||||||||||||||
publicationName: env.RUN_REPLICATION_PUBLICATION_NAME, | ||||||||||||||||||||||||||||||||||||||||||||
redisOptions: { | ||||||||||||||||||||||||||||||||||||||||||||
keyPrefix: "runs-replication:", | ||||||||||||||||||||||||||||||||||||||||||||
port: env.RUN_REPLICATION_REDIS_PORT ?? undefined, | ||||||||||||||||||||||||||||||||||||||||||||
host: env.RUN_REPLICATION_REDIS_HOST ?? undefined, | ||||||||||||||||||||||||||||||||||||||||||||
username: env.RUN_REPLICATION_REDIS_USERNAME ?? undefined, | ||||||||||||||||||||||||||||||||||||||||||||
password: env.RUN_REPLICATION_REDIS_PASSWORD ?? undefined, | ||||||||||||||||||||||||||||||||||||||||||||
enableAutoPipelining: true, | ||||||||||||||||||||||||||||||||||||||||||||
...(env.RUN_REPLICATION_REDIS_TLS_DISABLED === "true" ? {} : { tls: {} }), | ||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+100
to
+108
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Convert Environment variables are always strings; passing a string where a number is expected can cause connection failures in some redis client versions. - port: env.RUN_REPLICATION_REDIS_PORT ?? undefined,
+ port:
+ env.RUN_REPLICATION_REDIS_PORT !== undefined
+ ? Number(env.RUN_REPLICATION_REDIS_PORT)
+ : undefined, (Apply the same conversion wherever a numeric env var is forwarded.) 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
maxFlushConcurrency: params.maxFlushConcurrency, | ||||||||||||||||||||||||||||||||||||||||||||
flushIntervalMs: params.flushIntervalMs, | ||||||||||||||||||||||||||||||||||||||||||||
flushBatchSize: params.flushBatchSize, | ||||||||||||||||||||||||||||||||||||||||||||
leaderLockTimeoutMs: params.leaderLockTimeoutMs, | ||||||||||||||||||||||||||||||||||||||||||||
leaderLockExtendIntervalMs: params.leaderLockExtendIntervalMs, | ||||||||||||||||||||||||||||||||||||||||||||
leaderLockAcquireAdditionalTimeMs: params.leaderLockAcquireAdditionalTimeMs, | ||||||||||||||||||||||||||||||||||||||||||||
leaderLockRetryIntervalMs: params.leaderLockRetryIntervalMs, | ||||||||||||||||||||||||||||||||||||||||||||
ackIntervalSeconds: params.ackIntervalSeconds, | ||||||||||||||||||||||||||||||||||||||||||||
logLevel: "debug", | ||||||||||||||||||||||||||||||||||||||||||||
waitForAsyncInsert: params.waitForAsyncInsert, | ||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
return service; | ||||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
import { ActionFunctionArgs, json } from "@remix-run/server-runtime"; | ||
import { z } from "zod"; | ||
import { prisma } from "~/db.server"; | ||
import { startTcpBufferMonitor } from "~/services/monitorTcpBuffers.server"; | ||
import { authenticateApiRequestWithPersonalAccessToken } from "~/services/personalAccessToken.server"; | ||
import { getTcpMonitorGlobal, setTcpMonitorGlobal } from "~/services/runsReplicationGlobal.server"; | ||
|
||
const schema = z.object({ | ||
intervalMs: z.number().min(1000).max(60_000).default(5_000), | ||
}); | ||
|
||
export async function action({ request }: ActionFunctionArgs) { | ||
// Next authenticate the request | ||
const authenticationResult = await authenticateApiRequestWithPersonalAccessToken(request); | ||
|
||
if (!authenticationResult) { | ||
return json({ error: "Invalid or Missing API key" }, { status: 401 }); | ||
} | ||
|
||
const user = await prisma.user.findUnique({ | ||
where: { | ||
id: authenticationResult.userId, | ||
}, | ||
}); | ||
|
||
if (!user) { | ||
return json({ error: "Invalid or Missing API key" }, { status: 401 }); | ||
} | ||
|
||
if (!user.admin) { | ||
return json({ error: "You must be an admin to perform this action" }, { status: 403 }); | ||
} | ||
|
||
try { | ||
const body = await request.json(); | ||
const { intervalMs } = schema.parse(body); | ||
|
||
const globalMonitor = getTcpMonitorGlobal(); | ||
|
||
if (globalMonitor) { | ||
return json( | ||
{ | ||
error: "Tcp buffer monitor already running, you must stop it before starting a new one", | ||
}, | ||
{ | ||
status: 400, | ||
} | ||
); | ||
} | ||
|
||
setTcpMonitorGlobal(startTcpBufferMonitor(intervalMs)); | ||
|
||
return json({ | ||
success: true, | ||
}); | ||
} catch (error) { | ||
return json({ error: error instanceof Error ? error.message : error }, { status: 400 }); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
import { ActionFunctionArgs, json } from "@remix-run/server-runtime"; | ||
import { prisma } from "~/db.server"; | ||
import { authenticateApiRequestWithPersonalAccessToken } from "~/services/personalAccessToken.server"; | ||
import { | ||
getTcpMonitorGlobal, | ||
unregisterTcpMonitorGlobal, | ||
} from "~/services/runsReplicationGlobal.server"; | ||
|
||
export async function action({ request }: ActionFunctionArgs) { | ||
// Next authenticate the request | ||
const authenticationResult = await authenticateApiRequestWithPersonalAccessToken(request); | ||
|
||
if (!authenticationResult) { | ||
return json({ error: "Invalid or Missing API key" }, { status: 401 }); | ||
} | ||
|
||
const user = await prisma.user.findUnique({ | ||
where: { | ||
id: authenticationResult.userId, | ||
}, | ||
}); | ||
|
||
if (!user) { | ||
return json({ error: "Invalid or Missing API key" }, { status: 401 }); | ||
} | ||
|
||
if (!user.admin) { | ||
return json({ error: "You must be an admin to perform this action" }, { status: 403 }); | ||
} | ||
|
||
try { | ||
const globalMonitor = getTcpMonitorGlobal(); | ||
|
||
if (!globalMonitor) { | ||
return json({ error: "Tcp buffer monitor not running" }, { status: 400 }); | ||
} | ||
|
||
clearInterval(globalMonitor); | ||
unregisterTcpMonitorGlobal(); | ||
|
||
return json({ | ||
success: true, | ||
}); | ||
} catch (error) { | ||
return json({ error: error instanceof Error ? error.message : error }, { status: 400 }); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
// monitorTcpBuffers.ts | ||
import fs from "fs/promises"; | ||
import os from "os"; | ||
import { logger } from "./logger.server"; | ||
|
||
/** | ||
* Parse /proc/net/sockstat and /proc/sys/net/* every `intervalMs` | ||
* and log the numbers. You can pivot these logs into CloudWatch | ||
* metrics with a filter pattern if you like. | ||
*/ | ||
export function startTcpBufferMonitor(intervalMs = 5_000) { | ||
async function sampleOnce() { | ||
try { | ||
const [sockstat, wmemMax, tcpMem] = await Promise.all([ | ||
fs.readFile("/proc/net/sockstat", "utf8"), | ||
fs.readFile("/proc/sys/net/core/wmem_max", "utf8"), | ||
fs.readFile("/proc/sys/net/ipv4/tcp_mem", "utf8"), | ||
]); | ||
|
||
logger.debug("tcp-buffer-monitor", { | ||
sockstat, | ||
wmemMax, | ||
tcpMem, | ||
}); | ||
|
||
// /proc/net/sockstat has lines like: | ||
// TCP: inuse 5 orphan 0 tw 0 alloc 6 mem 409 | ||
const tcpLine = sockstat.split("\n").find((l) => l.startsWith("TCP:")) ?? ""; | ||
const fields = tcpLine.trim().split(/\s+/); | ||
const inUse = Number(fields[2]); // open sockets | ||
const alloc = Number(fields[8]); // total sockets with buffers | ||
const memPages = Number(fields[10]); // pages (4 kB each) | ||
const memBytes = memPages * 4096; | ||
|
||
const wmemMaxBytes = Number(wmemMax.trim()); | ||
const [low, pressure, high] = tcpMem | ||
.trim() | ||
.split(/\s+/) | ||
.map((n) => Number(n) * 4096); // pages → bytes | ||
|
||
logger.debug("tcp-buffer-monitor", { | ||
t: Date.now(), | ||
host: os.hostname(), | ||
sockets_in_use: inUse, | ||
sockets_alloc: alloc, | ||
tcp_mem_bytes: memBytes, | ||
tcp_mem_high: high, | ||
wmem_max: wmemMaxBytes, | ||
}); | ||
} catch (err) { | ||
// Log and keep going; most errors are “file disappeared for a moment” | ||
console.error("tcp-buffer-monitor error", err); | ||
} | ||
} | ||
|
||
return setInterval(sampleOnce, intervalMs); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Tighten parameter validation to reject nonsensical or dangerous input
Most numeric parameters (intervals, counts, batch sizes, TTL, etc.) must be positive integers.
z.number()
currently acceptsNaN
, fractions, and negative values (as well as numbers far larger than what the service can realistically handle).Rejecting invalid values early prevents runaway timers, mis-configured pools, and DoS vectors.
🏁 Script executed:
Length of output: 12923
Enforce strict integer and positivity constraints on replication parameters
All numeric fields in
CreateRunReplicationServiceParams
currently usez.number()
, which permits negatives, fractions, andNaN
. To prevent mis-configurations, resource exhaustion, and DoS scenarios, these should be constrained to integers—and where applicable, strictly positive or non-negative.In apps/webapp/app/routes/admin.api.v1.runs-replication.create.ts, update the schema as follows:
This aligns with existing
.int()
usage elsewhere (e.g., batchTrigger and ClickHouse schemas) and ensures invalid values are rejected at parse time.📝 Committable suggestion