fix: scope doctor probe cleanup and instrument component refreshes - #6
Conversation
Stop the round-trip probe from deregistering and deleting a caller process the application registered, which released its activations and unclaimed its messages. Report cleanup failures as a failed or warned check instead of raising a database lock error out of the command. Instrument component refreshes with identity, key, dependencies, revision, and outcome.
Greptile SummaryThe PR isolates the doctor probe from the application caller process, makes probe cleanup failures reportable, restores SQLite busy waits correctly after deadline-bound work, and adds component-refresh instrumentation.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
D[Doctor sync check] --> R[Create dedicated ProcessRegistry]
R --> P[Register probe caller process]
P --> E[Enqueue probe message]
E --> S[SynchronousInvocation with injected registry]
S --> C{Round trip result}
C -->|Success| PASS[Passing check]
C -->|Error| FAIL[Failed check]
PASS --> CLEAN[Remove probe actor and caller process]
FAIL --> CLEAN
CLEAN --> L{Cleanup leftovers?}
L -->|No| RESULT[Return check]
L -->|Yes and probe passed| WARN[Return warning]
L -->|Yes and probe failed| RESULT
Reviews (2): Last reviewed commit: "fix: suspend SQLite busy waits only when..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| leftovers << "probe caller process" unless delete_probe_caller_process(probe_process_id) |
There was a problem hiding this comment.
If an application synchronous call overlaps a doctor run that started without a registered caller process, that call reuses the registry created by the probe, but this cleanup treats the registry as exclusively probe-owned and deregisters it, clearing activation ownership and releasing claimed work for the live application call.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/solid_objects/doctor.rb
Line: 252-253
Comment:
**Shared caller process deregistration**
If an application synchronous call overlaps a doctor run that started without a registered caller process, that call reuses the registry created by the probe, but this cleanup treats the registry as exclusively probe-owned and deregisters it, clearing activation ownership and releasing claimed work for the live application call.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Valid, and the original id comparison could not close it: the caller process is a per-process singleton, so any thread can adopt the probe registry between the probe finishing and cleanup running.
Fixed in 9ab4ed0 by removing the sharing rather than narrowing the race. SynchronousInvocation now accepts an explicit process_registry: (defaulting to the shared singleton, so existing callers are unchanged), and the doctor registers a dedicated ProcessRegistry for its probe. The probe registry is never reachable through SolidObjects.caller_process, so no application call can adopt it and deleting it is unconditionally safe.
That also removed the need for the CallerProcess#process_record_id and #delete_process_record accessors the ownership check required, so caller_process.rb is back to its state on main.
Regression test: doctor_test.rb "runs its probe on a caller process the application cannot adopt" makes SolidObjects.caller_process.process_registry raise, and asserts the round trip still passes.
There was a problem hiding this comment.
Follow-up on the CI failure I attributed to a pre-existing flake: that was only half right, and the half I got wrong mattered.
Root cause, verified by measurement rather than inference. Rails installs SQLite's busy wait as a Ruby busy handler via the sqlite3 timeout configuration (busy_handler_timeout=), not via PRAGMA busy_timeout. PRAGMA busy_timeout reports 0 for such a connection, and writing it calls sqlite3_busy_timeout(), which replaces any registered handler. So the adapter's save/restore in with_transaction_deadline read 0, wrote 0 back, and stripped the handler permanently.
Measured on one pooled connection, before and after a single sync call, with an identical contended write:
BEFORE_WAIT: 5.006s
AFTER_WAIT: 0.000s
Every later writer on that connection, inside Solid Objects or anywhere else in the host application, then failed immediately with SQLite3::BusyException instead of waiting for the lock. That is a production bug, not a test artifact, and it explains the same failure on main's history (run 31132152323 on b6487a7).
Fixed in e015c6a: restoration now reinstalls the configured Ruby busy handler, falling back to the pragma when a pragma-based timeout was genuinely in effect. Same measurement after the fix: BEFORE_WAIT: 5.006s, AFTER_WAIT: 5.002s.
The immediate trigger for this PR's red build was mine, though: the busy_timeout helper I added in 9ab4ed0 to speed up a doctor test used the same broken pattern and poisoned a pooled connection that EnqueueTest later leased. Fixed in c661a03 by sharing one helper that suspends and restores correctly. Both suites had been reimplementing it wrongly.
Regression test: "sync restores the SQLite busy handler it suspended for the deadline" performs a write against a briefly held write lock after a sync call, which fails immediately without the fix.
CI is green across sqlite, postgresql, mysql, and static on both workflow runs, and green again on a full re-run.
Register a dedicated caller process for the round-trip probe and pass it to SynchronousInvocation, so an overlapping application call can no longer adopt the probe registry and have its activation ownership and claimed messages released when the probe cleans up. Removes the caller process accessors the previous ownership check needed.
Rails installs SQLite's busy wait as a Ruby busy handler through the sqlite3 timeout configuration. PRAGMA busy_timeout neither reports that handler nor preserves it: sqlite3_busy_timeout replaces any registered handler, so reading zero and writing zero back stripped the handler permanently. Every later writer on that pooled connection, inside or outside Solid Objects, then failed immediately with SQLite3::BusyException instead of waiting for the lock. Reinstall the configured handler when restoring, and stop the test helper from simulating contention the same broken way.
The doctor and synchronous invocation suites each simulated immediate lock failure by writing PRAGMA busy_timeout back over itself, which left the pooled connection with no busy handler and made later concurrent tests fail with SQLite3::BusyException. Share one helper that suspends and correctly restores the configured busy wait.
The adapter reads the configured busy wait through the connection pool's database configuration. If a future Active Record release stops exposing it, restoration would silently fall back to writing a zero pragma and strip lock waiting from a pooled connection the whole application shares. Skip the suspension instead, which only loosens the synchronous deadline bound, and assert the timeout stays discoverable so the looser bound cannot be adopted unnoticed.
|
@greptileai review |
1 participant
Fixes two defects in
solid_objects:doctorand adds observability for componentrefreshes. Both were found while integrating 0.5.0 into a host application.
Doctor: probe cleanup released a live caller process
check_sync_round_triptook the process-wideSolidObjects.caller_processsingleton and unconditionally stopped and deleted its process record.
ProcessRegistry.deregisteralso nullsactivation_owner_idon instances andunclaims
ClaimedMessagerows for that process, so running the doctor inside aprocess that already serves synchronous calls (console, health endpoint,
initializer) stripped activations from work in flight.
The probe now records
process_record_idbefore running and compares itafterwards, removing the caller process only when the probe itself registered
it. Ownership is decided by identity, so a stale in-memory registry whose row
was already deleted is still cleaned up correctly.
Doctor: cleanup errors escaped the command
Cleanup ran in a bare
ensure, which the method's ownrescuecannot cover.With a writer holding the SQLite lock,
Doctor#callraisedActiveRecord::StatementTimeoutout of the rake task instead of reportingFAIL sync_round_trip, and the escape at the first cleanup statement skippedthe rest, leaking the probe's caller process row.
Cleanup now runs as explicit guarded steps. A failure after a passing probe
downgrades the check to
warnand names what was left behind.Component refresh instrumentation
ComponentsController#showemitted no notifications, so operators could not seerefresh rate, authorization denials, superseded requests, or render cost. It now
wraps the refresh in
solid_objects.component.refreshedwithactor_type,actor_id,component_name,component_key,dependencies,refresh_method,instance_id,revision, and anoutcomeofrendered,conflict,unauthorized,unknown_component, orinvalid_token.Effects
CallerProcess#process_record_idand#delete_process_record(process_id)are new; both are in-memory andmutex-guarded and perform no database access before the probe's error
handling is established. No existing signature changed.
messages belonging to an application caller process.
notification payload, matching how message events exclude
arguments. Keysand actor identity are included, consistent with existing events.
non-SQLite adapters.
Validation
bundle exec rake237 runs, 961 assertions, 0 failures, 0 errors. Standard Ruby and RuboCop
clean, RBS regenerated and validated, Steep clean, Brakeman 0 warnings.
The PostgreSQL and MySQL suites were not run locally because neither server was
available:
Tests
Seven tests, each written failing first:
locked (reproduces the original
StatementTimeoutescape)