chore: prepare 0.10.0 - #27
Conversation
Two features and a behaviour change since 0.9.0, so a minor bump: the supervisor now schedules retention, payload broadcasts gained payload_authorization_context, and payload blocks run against the actor instance rather than the class. The manual QA pass turned up three things, fixed here. A denied CLI command printed a thirty-line Ruby backtrace. Administration denies by default, so that is what an unconfigured host sees from its first solid_objects command. A policy decision is not a crash, and the backtrace buried the one line saying how to grant access. Two documented query counts were wrong. A message turn costs 26 queries, not the documented 29, and a synchronous call 53, not 49. Both are deterministic. The synchronous number had drifted because no script measured it; query_count.rb now reports both. The roadmap claimed the Redis wake-up adapter was not implemented, which has been false since 0.9.0, and listed outboxes as partially implemented when the only outstanding item is a possible future optimisation.
Greptile SummaryThe PR prepares the 0.10.0 release and addresses findings from manual QA.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "fix: scope query counting to the measuri..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def synchronous_query_count | ||
| reference = CounterActor.ref("sync-queries") | ||
| worker = SolidObjects::Worker.new | ||
| runner = Thread.new { worker.run } |
There was a problem hiding this comment.
The process-wide sql.active_record subscriber measures the synchronous call while worker.run concurrently performs heartbeat, activation-maintenance, claim, and drain queries. Those background queries are included in the reported total, so the result does not isolate one synchronous invocation and its documented determinism depends on thread timing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: benchmark/support.rb
Line: 319
Comment:
**Background SQL contaminates query count**
The process-wide `sql.active_record` subscriber measures the synchronous call while `worker.run` concurrently performs heartbeat, activation-maintenance, claim, and drain queries. Those background queries are included in the reported total, so the result does not isolate one synchronous invocation and its documented determinism depends on thread timing.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Correct, and it changed the answer. Fixed in 8bb4491.
count_queries now scopes to the measuring thread. Notifications run on the thread that issued the query, so Thread.current.equal?(measuring) excludes the worker entirely. The caller and the worker turn are measured separately rather than by watching both threads at once, because a poll belongs to no particular call.
This also corrects a claim I made in the PR body. With the worker excluded, the caller of a synchronous call costs 49 queries, which is exactly what the docs already said. The 53 I reported was four stray polls landing in the window. Five identical runs made it look deterministic when it was only consistent on this machine, which is the trap you caught.
The message-turn drift is real and stands: 26 against a documented 29. That measurement calls worker.run_once on the measuring thread with no other thread alive, so it was never contaminated.
The script now reports three numbers, all stable across five runs:
database queries: 26 for 1 message turn
database queries: 49 for the caller of 1 synchronous call
database queries: 75 for 1 synchronous call, caller plus the turn it waits on
The 75 is new. A synchronous call is a caller plus a worker turn, and only the sum says what the database actually serves for one call. docs/benchmarks.md and the changelog now carry all three plus the reason counting is thread-scoped.
bundle exec rake: 366 runs, 1253 assertions, 0 failures, 0 errors, 13 skips.
@greptileai review
There was a problem hiding this comment.
The fix in 8bb4491 is correct and directly addresses the contamination concern.
count_queries now captures Thread.current as measuring before subscribing, and the sql.active_record block exits early unless Thread.current.equal?(measuring). Because ActiveSupport notifications fire on the thread that issued the query, the worker's polls are structurally excluded — no timing window to fall into.
The separation is also the right design: message_turn_query_count runs worker.run_once on the measuring thread with no other thread alive (clean by construction), while synchronous_caller_query_count runs the worker on a separate thread and counts only the caller. Combining them as caller + turn for the 75-query total is a sound way to express what the database actually serves for one synchronous call.
The queries counter is only ever incremented from the measuring thread (guarded by the equal? check), so no mutex is needed — that's correct too.
The original comment stands resolved.
The synchronous benchmark subscribed to sql.active_record process-wide while a worker ran on its own thread, so whatever the worker polled during the window landed in the count. Stability across runs made that look deterministic when it was only consistent. Counting is now scoped to the thread being measured, and the caller and the worker turn are measured separately rather than by watching both at once, because a poll belongs to no particular call. This corrects a claim made in the previous commit. The caller of a synchronous call costs 49 queries, exactly what was documented before; the 53 reported earlier was four stray polls. The message turn drift is real and stands at 26 against a documented 29, since that measurement always ran on the measuring thread with nothing else alive. The script also reports the total a synchronous call costs the database, 75, which was never recorded.
|
@greptileai review |
1 participant
Bumps to 0.10.0 and fixes what the manual QA pass turned up.
Why 0.10.0
Since 0.9.0 the unreleased set contains two features and one behaviour change, which is a minor bump under the 0.x convention this project has followed:
retention_intervalsettingpayload_authorization_context, a new configuration hookPatch releases here have been fixes only (0.7.1, 0.7.2, 0.7.3), so a patch bump would understate this.
QA pass
Everything below was run by hand on this branch.
bundle exec rake(SQLite)npm test/npm run test:browsergem build+ contentstest/ornode_modulescomponent_delivery,sync_latency,enqueue,processing,query_countschema matches the 0.10.0 runtime--help,statusThe wake-up suites were run with their services explicitly configured rather than left to skip, since a skip and a pass look identical in the summary line.
Three findings, fixed here
1. A denied CLI command printed a backtrace. Administration denies by default, so
solid_objects statuson an unconfigured host produced thirty lines of Ruby backtrace through Thor and Bundler. That is the most likely first experience a new adopter has with the CLI, and the backtrace buried the one line that says how to fix it. A policy decision is not a crash. Now:with exit 1. The internal contract is unchanged: commands still raise
Unauthorized, and the three existing tests asserting that still pass. Only the executable boundary presents it. Covered by a new subprocess test asserting non-zero exit, that the message names the setting, and that nocli.rb:NNframe appears.2. Two documented query counts were wrong.
docs/benchmarks.mdclaimed 29 queries for a message turn and 49 for a synchronous call. Measured: 26 and 53. Both are deterministic, stable across five runs each. The synchronous number had drifted precisely because nothing measured it:benchmark/query_count.rbonly ever reported the worker turn. It now reports both, so the claim has a script behind it. The docs record the new numbers, dated, with a note that they moved in opposite directions.I left the throughput and latency tables alone. Those are explicitly dated development measurements on a different machine, and re-measuring them here would replace one machine-specific snapshot with another.
3. Two stale roadmap entries. Also answers the question about what can move out of "Partially implemented":
The other three entries were checked against the code and are accurate, so they stay:
lib/orapp/instances_controllerhas no filtering parametersNote, not fixed
examples/application/config/initializers/solid_objects.rbstill branches onauthorization_context.respond_to?(:current_user), which is the same translationpayload_authorization_contextwas added to remove. It is still correct there:authorize_subscriptionreceives the raw Cable connection by design and was deliberately left alone in #26. Worth revisiting if that hook gets a resolver too, but changing it now would be a behaviour change dressed as an example edit.