feat(world-postgres): replace pg-boss with graphile-worker#1124
feat(world-postgres): replace pg-boss with graphile-worker#1124VaguelySerious merged 8 commits intovercel:mainfrom
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: kschmelter13 <kschmelter13@gmail.com>
Signed-off-by: kschmelter13 <kschmelter13@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: kschmelter13 <kschmelter13@gmail.com>
🦋 Changeset detectedLatest commit: d7544bf The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@kschmelter13 is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
|
@VaguelySerious quick heads up: the required checks are blocked due to fork security gates. |
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
Code LGTM. Checks are running, will check back in a bit. Also this PR is missing a changeset pnpm changeset for world-postgres. Should be a minor bump since we're not changing schemas and are in beta.
Signed-off-by: kschmelter13 <kschmelter13@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: kschmelter13 <kschmelter13@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
LGTM - two wording changes to make
Minor to patch Co-authored-by: Peter Wielander <mittgfu@gmail.com> Signed-off-by: Kevin <kschmelter13@gmail.com>
Co-authored-by: Peter Wielander <mittgfu@gmail.com> Signed-off-by: Kevin <kschmelter13@gmail.com>
|
@VaguelySerious All changes made |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Thanks! I'm re-running CI and merging once it's green |
|
New schema/tables will be created automatically, right? Should it clean up the pgboss schema? |
packages/world-postgres/src/index.ts
Outdated
| const postgres = createPostgres(config.connectionString); | ||
| const drizzle = createClient(postgres); | ||
| const queue = createQueue(boss, config); | ||
| const queue = createQueue(config.connectionString, config); |
There was a problem hiding this comment.
Looks like createQueue() could simply get the connectionString from the config?
There was a problem hiding this comment.
diff --git a/packages/world-postgres/src/index.ts b/packages/world-postgres/src/index.t
s
index 21d8f4a4..41c09a92 100644
--- a/packages/world-postgres/src/index.ts
+++ b/packages/world-postgres/src/index.ts
@@ -34,7 +34,7 @@ export function createWorld(
): World & { start(): Promise<void> } {
const postgres = createPostgres(config.connectionString);
const drizzle = createClient(postgres);
- const queue = createQueue(config.connectionString, config);
+ const queue = createQueue(config);
const storage = createStorage(drizzle);
const streamer = createStreamer(postgres, drizzle);
diff --git a/packages/world-postgres/src/queue.ts b/packages/world-postgres/src/queue.ts
index c5b8dc61..3f622a44 100644
--- a/packages/world-postgres/src/queue.ts
+++ b/packages/world-postgres/src/queue.ts
@@ -43,10 +43,7 @@ export type PostgresQueue = Queue & {
close(): Promise<void>;
};
-export function createQueue(
- connectionString: string,
- config: PostgresWorldConfig
-): PostgresQueue {
+export function createQueue(config: PostgresWorldConfig): PostgresQueue {
const port = process.env.PORT ? Number(process.env.PORT) : undefined;
const localWorld = createLocalWorld({ dataDir: undefined, port });
@@ -73,7 +70,7 @@ export function createQueue(
if (!startPromise) {
startPromise = (async () => {
workerUtils = await makeWorkerUtils({
- connectionString,
+ connectionString: config.connectionString,
logger: stderrLogger,
});
await workerUtils.migrate();
@@ -136,7 +133,7 @@ export function createQueue(
}
runner = await run({
- connectionString,
+ connectionString: config.connectionString,
concurrency: config.queueConcurrency || 10,
logger: stderrLogger,
pollInterval: 500, // 500ms = 0.5s (graphile-worker uses LISTEN/NOTIFY when avai
lable)There was a problem hiding this comment.
Yes good call, changes made.
|
@rovo89 Inflight pg-boss items be lost, i.e. any queued but unprocessed workflow/step jobs in pg-boss at upgrade time will be silently dropped. Graphile will run its own migration to set up the table, but there's no migration for inflight requests |
|
Yes, just saw the "Inflight" might also mean that a workflow waiting for And with cleanup, I meant deleting the pgboss schema. |
|
@rovo89 I'll create migrations at the very least for cleaning up the old tables, and potentially a full migration if it seems feasible, though for now I'd expect that upgrading to the the code in this PR and the new package version will halt inflight runs. |
|
@kschmelter13 Thanks for the PR! I reviewed the code as well, although I haven't used Graphile Worker before. LGTM, with one nitpick: #1124 (comment)
@VaguelySerious That's still the way to go? |
Yes, that's still the intended use. The actual workers can be on a different host than postgres and are managed by the user, but the workers need to be running in the same environment as the server making these endpoints available. |
|
Migrations added in #1126, see this commit diff. Did some upgrade testing and seems to pick up existing jobs correctly. |
Signed-off-by: kschmelter13 <kschmelter13@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
@rovo89 Of course! I made those changes, thanks for looking at it |
|
@VaguelySerious Thanks for the migrations. The vercel world and some comment tests are failing because of a vercel token. Do those need to pass for merge? |
Thanks, but I think @VaguelySerious had already considered them in his migration commit (which would now conflict). 😉 |
|
@kschmelter13 Vercel world tests don't need to run on this PR. I'm waiting with merging mainly for us to be able to release a new version before this lands on No worries about the merge conflicts, I'll rebase my PR once this PR lands |
|
@VaguelySerious @rovo89 Perfect sounds good, thanks guys |
| // Redirect graphile-worker logs to stderr so CLI --json on stdout stays clean. | ||
| // TODO: When CI=1 suppresses logging, replace with conditional stdout (e.g. log to stdout when not in JSON/CI mode). | ||
| const stderrLogger = new Logger( | ||
| () => (level: string, message: string, meta?: unknown) => { | ||
| const line = [level, message, meta].filter(Boolean).join(' ') + '\n'; | ||
| process.stderr.write(line); | ||
| } | ||
| ); |
There was a problem hiding this comment.
@kschmelter13 Graphile Worker is quite noisy in the logs. Can we change the log level to show only errors maybe? Currently, it even shows debug messages like "spawned" (multiple times, I hope that's not some indicator of unexpected/unsupported behavior).
There was a problem hiding this comment.
By default it logs all levels, I can default to errors only and add an env for override?
There was a problem hiding this comment.
Yeah, something like that. Just check yourself which kinds of messages come up in npm run dev. The default logger seems to ignore debug messages unless some env is set: https://worker.graphile.org/docs/library/logger
Probably that's gone because you override the logger. But I think it also wrote several non-debug messages, like which queues it's listening to, that's why I suggested error only, maybe warning.
There was a problem hiding this comment.
These are the messages I see:
info Failed to read crontab file '/workspaces/playground/crontab'; cron is disabled
debug Registering termination signal handlers (SIGUSR2, SIGINT, SIGTERM, SIGHUP, SIGABRT) [object Object]
debug Spawned
debug Spawned
debug Spawned
debug Spawned
debug Spawned
debug Spawned
debug Spawned
debug Spawned
debug Spawned
debug Spawned
info Worker connected and looking for jobs... (task names: 'workflow_flows', 'workflow_steps')
debug Graphile Worker Cron fired 1.796s too early (clock skew?); rescheduling [object Object]
debug Graphile Worker Cron fired 3.557s too early (clock skew?); rescheduling [object Object]
debug Graphile Worker Cron fired 3.581s too early (clock skew?); rescheduling [object Object]
3 participants
Implements Graphile Worker as the queue/runner for world-postgres, replacing pg-boss while keeping the existing PostgreSQL durability guarantees and public world API intact. Lead from conversations in pr #155