-
Notifications
You must be signed in to change notification settings - Fork 73
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
feat(pb, ds): move to edge #1942
base: 01-27-chore_workflows_move_wf_gc_and_metrics_publish_into_worker
Are you sure you want to change the base?
feat(pb, ds): move to edge #1942
Conversation
Deploying rivet with Cloudflare Pages
|
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.
PR Summary
This PR moves pegboard and ds services to the edge infrastructure, introducing new workflow handling capabilities and SQLite database integration.
- Added new
edge-monolith-workflow-worker
in/packages/services/edge/monolith/standalone/workflow-worker/
with concerning error handling that unconditionally bails on worker exit - Empty test table migrations in
/packages/services/edge/pegboard/src/workflows/actor/migrations.rs
and/packages/services/edge/pegboard/src/workflows/client/migrations.rs
need proper schema definitions - Added SQLite database access in
ActivityCtx
andConnection
for per-workflow state storage with workflow-specific database names - Implemented comprehensive client workflow in
/packages/services/edge/pegboard/src/workflows/client/mod.rs
for handling registration, events, and actor management - New feature flag system in
/packages/services/edge/pegboard/Cargo.toml
organizes dependencies into tiers (default, workflows, chirp)
12 file(s) reviewed, 16 comment(s)
Edit PR Review Bot Settings | Greptile
ds.workspace = true | ||
pegboard.workspace = true |
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.
logic: ds and pegboard dependencies should specify features needed for workflow integration, e.g. pegboard = { workspace = true, features = ["workflows"] }
[dependencies] | ||
chirp-workflow.workspace = true | ||
rivet-config.workspace = true | ||
rivet-health-checks.workspace = true | ||
rivet-metrics.workspace = true | ||
rivet-runtime.workspace = true |
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.
style: consider adding tokio with appropriate runtime features since this is a worker service that will likely need async functionality
|
||
// Start worker | ||
worker.wake_start(config, pools).await?; | ||
bail!("worker exited unexpectedly"); |
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.
logic: unconditional bail! means worker will always exit with error, even for graceful shutdowns - should check exit condition first
let reg = ds::registry()? | ||
.merge(pegboard::registry()?)?; | ||
|
||
let db = db::DatabaseFdbSqliteNats::from_pools(pools.clone())?; |
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.
style: pools is cloned here but original pools object is passed to wake_start - consider reusing the cloned pools
workflows = ["chirp"] | ||
chirp = ["chirp-workflow", "sqlx", "nix", "server-spec"] |
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.
logic: workflows feature depends on chirp but chirp is not listed as a dependency in the feature definition - should be workflows = ["chirp-workflow"]
or include both
sql_execute!( | ||
[ctx, pool] | ||
" | ||
CREATE TABLE test ( |
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.
style: 'test' appears to be a placeholder name. Use a descriptive table name that reflects its purpose in production.
pub mod client; | ||
pub mod actor; |
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.
style: module order should be alphabetical - 'actor' should come before 'client'
// TODO(RVT-4450): `last_event_idx < $2` and `ON CONFLICT DO NOTHING` is a workaround | ||
let inserted_rows = sql_fetch_all!( |
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.
logic: The workaround for RVT-4450 using last_event_idx < $2
and ON CONFLICT DO NOTHING
could potentially miss events if they arrive out of order. Consider adding additional validation or logging.
// TODO: Send as a single message | ||
for (i, raw_command) in raw_commands.into_iter().enumerate() { | ||
let wrapped_command = protocol::CommandWrapper { | ||
index: index + i as i64, | ||
inner: raw_command, | ||
}; | ||
|
||
// Forward signal to ws as message | ||
ctx.msg(ToWs { | ||
client_id, | ||
inner: protocol::ToClient::Commands(vec![wrapped_command]), | ||
}) | ||
.send() | ||
.await?; | ||
} |
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.
style: Sending commands individually could cause performance issues with high command volumes. Consider implementing the TODO to batch commands into a single message.
if matches!(signal.try_into()?, Signal::SIGTERM | Signal::SIGKILL) { | ||
let res = ctx |
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.
logic: The signal type check should handle conversion errors from try_into() more explicitly to avoid potential panics
1023d01
to
9f75ea9
Compare
df8b94d
to
f6f8a7b
Compare
9f75ea9
to
176d520
Compare
Deploying rivet-hub with Cloudflare Pages
|
9c05edc
to
0ce8234
Compare
f6f8a7b
to
d7a614b
Compare
0ce8234
to
f532f05
Compare
d7a614b
to
cbfb3c8
Compare
f532f05
to
0a58d12
Compare
cbfb3c8
to
f993bd7
Compare
0a58d12
to
ba14c9f
Compare
f993bd7
to
ca16fe8
Compare
e057146
to
5e1789e
Compare
ca16fe8
to
e8b385a
Compare
5e1789e
to
dfa1115
Compare
e8b385a
to
962e10c
Compare
dfa1115
to
1162ce3
Compare
Changes