Test analyses load - #6313
Draft
JorisGoosen wants to merge 17 commits into
Draft
Conversation
JorisGoosen
commented
Sep 9, 2026
| } | ||
| }, | ||
| { | ||
| "name": "analysis_getOptions", |
Contributor
Author
There was a problem hiding this comment.
JorisGoosen
pushed a commit
to JorisGoosen/jasp-desktop
that referenced
this pull request
Sep 10, 2026
Review comment on PR jasp-stats#6313 pointed out that get_analyses_state (with a single analysisId, include_options=true, options_meta_diff=false) already returns the identity, lifecycle status, options and full optionMeta of an existing analysis, making analysis_getOptions redundant API surface. Removed the handler and its JASP_RPC.json spec entry, and dropped it from the gate test's expected-methods sanity check. No other code used it (the gate test drives analyses through create/run/remove). Note for the jasp-mcp repo: its README and SERVER_INSTRUCTIONS reference a 'jasp_analysis_getOptions' tool that never existed in jasp-desktop's spec (jasp-mcp registers tools dynamically from rpc.discover, so the tool simply was not there). That documentation should be updated to point to jasp_get_analyses_state instead.
added 12 commits
September 10, 2026 10:14
…two RPC methods and a filter re-bind crash fix
Gate test (Tests/gatetest/):
- Launches JASP headless (-platform offscreen; -platform minimal crashes the
scene graph via QtWebEngine during the blocking RPC waits), enables the RPC
server through the new --rpcPort flag, loads Resources/Data Sets/debug.csv
and then creates + runs + removes every analysis of every loaded module
through jasp-mcp (the MCP layer AI agents use), verifying each analysis
reaches status 'complete' with non-error results.
- Drives jasp-mcp as an MCP client; any step failing on the MCP layer is
automatically retried over direct JSON-RPC so the report immediately shows
whether JASP or the MCP layer is broken.
- jasp-mcp is pinned as a submodule (Tests/gatetest/jasp-mcp); run through
Tests/gatetest/run_gatetest.sh which sets up the venv (mcp pinned <2, 2.x
removed the lowlevel Server API jasp-mcp uses) and builds JASP if needed.
- Full run: 269/271 analyses pass; the 2 remaining failures (jaspAudit
workflows, see below) are documented in Tests/gatetest/skip-list.txt.
New RPC methods (handlers in Desktop/analysis/analyses.cpp, spec entries in
Resources/JASP_RPC.json):
- analysis_remove {analysisId}: removes an analysis from the workspace,
needed by the gate test (and agents) to avoid analyses accumulating.
Workspace-dirty bookkeeping is handled by AgentStateTracker via the
analysisRemoved signal and the dispatcher's post-mutation markClean().
- analysis_getOptions {analysisId}: returns options + optionMeta of an
existing analysis. jasp-mcp's README already documented this tool; the
method was missing from our spec.
--rpcPort=<n> CLI flag (Desktop/main.cpp):
- Enables the RPC server on the given port at startup, persisted like
--safeGraphics, for automation/gate-test use. Also fixes a latent bug in
the --timeOut=N parser, which read from the wrong string (--timeOut=N
never actually parsed N).
Fix re-bind crash in ListModelFilteredDataEntry (found by the gate test):
- assert(!_filter && !_filterName.empty()) in initTableTerms fired whenever
an audit form was re-bound with the default (empty-filterName) options
table, which happens on every analysis_run through the RPC/agent API, and
crashed JASP (SIGABRT). Since jasp-stats#5783 createFilter() loads the existing
DB-backed named filter, keeping the already-created _filter on re-bind is
correct; the assert now only guards the constructor invariant (_filterName
always generated).
Known remaining issue (flagged by the gate test, NOT fixed here):
- jaspAudit/auditClassicalWorkflow and auditBayesianWorkflow get stuck in an
infinite named-filter re-run loop (~20 filterByName requests/s) after a
re-bind: the audit data-entry column write
(ListModelFilteredDataEntry::informDataSetOfInitialValues -> sendInfo)
perturbs the dataset, which re-triggers the filter, starving the engine
pool so the analysis run is never scheduled. Engine and DB results are
correct and the analysis itself can reach 'complete'; only the loop is
broken. Likely related to the jasp-stats#5783 DataSetQ/filter refactor; details in
Tests/gatetest/skip-list.txt.
…heckForUpdates now reports only actual changes
The gate test (Tests/gatetest) exposed an endless filterByName loop with
jaspAudit's Sampling Workflow analyses: after the first named-filter run
(e.g. ListModelFilteredDataEntry_0), the filter was re-sent to the engine
~20x/second forever (2629 requests in one gate run), starving the engine
pool so the analysis runs never got scheduled (analyses stuck in
'empty'/'aborted' until timeout). Not RPC-specific: any creation of an
audit analysis would churn engines indefinitely.
Root cause:
1. Engine runs a named filter and bumps its DB revision (+3 per run:
filterWrite->filterIncRevision, explicit incRevision, and
dbUpdateErrorMsg->incRevision).
2. On the reply, EngineRepresentation::processFilterByNameReply calls
Workspace::checkForUpdates -> DataSet::checkForUpdates ->
Filter::checkForUpdates, which returned true unconditionally after
dbLoad() merely because the engine revision was ahead — even though
the revision bump was the direct result of our own request and the
freshly loaded results were identical.
3. DataSet::checkForUpdates then emitted datasetChanged(...) even with
no changed columns/rows, which reaches the analysis filter's
VarInfoSignaller::dataSetChanged (filter.cpp), which
ListModelFilteredDataEntry::dataSetChangedHandler listens to, which
calls runFilter() -> new filterByName request -> goto 1.
Fix (CommonData/filter.{h,cpp}):
- Filter::dbLoad() now returns bool: whether the rFilter/generatedFilter/
constructorJson/constructorR or the results/error actually changed
compared to the cached values (dbLoadResultAndError's existing
'changed' logic is reused).
- Filter::checkForUpdates() returns dbLoad()'s result instead of an
unconditional true. Syncing our own cache with a recompute we just
triggered is not a dataset change; genuine result changes (e.g. after
data edits) still return true and still emit datasetChanged, so all
existing re-run/refresh behaviour is preserved.
- After this, the identical-results sync returns false, no
datasetChanged is emitted, and the loop terminates (verified: 366
filterByName replies -> 2; auditClassicalWorkflow runs to 'complete'
in ~4s via the RPC/agent API).
Also updated Tests/gatetest/skip-list.txt: the jaspAudit entries are
removed since the full gate run now passes 271/271 analyses.
Note (left as-is, flagged for follow-up): Filter::_invalidated is never
cleared to false in C++, and Filter::dbUpdate() stomps invalidated=false
into the DB via the DatabaseInterface::filterUpdate default argument,
while setInvalidated(true) re-emits sendFilterByName even when already
invalidated. That re-emit is currently load-bearing (it is what makes
DataSet::runFilters() after manual edits actually re-run already-
invalidated filters), so it was deliberately not changed; a proper
'fresh results' state transition would be a cleaner design.
Known pre-existing, unrelated test failures (verified identical on a
tree without this change): TestAll::testDataImport CSV/TSV
'hardcodedIsSame' hardcoded-json mismatches.
…nd close the remaining audit filter-loop paths Follow-up on 03c6bd1 (which broke the audit infinite filterByName loop by making Filter::checkForUpdates report only actual changes). That fix removed the loop's perpetuator but left the underlying lifecycle wart: Filter's _invalidated flag was set but never cleared, the DB row always got invalidated=false stomped into it via the DatabaseInterface::filterUpdate default argument, and setInvalidated(true)'s unconditional re-emit of sendFilterByName was load-bearing precisely because of that. This commit introduces the proper state transition: a filter is stale from the moment its inputs change until the engine reply for the LATEST request has been consumed. Request identity for filterByName (mirrors the existing default-filter requestId design): - EngineSync::sendFilterByName assigns a unique requestId (shared _waitingFilterRequestIDCounter) and RFilterByNameStore carries it. - Engine::receiveFilterByNameMessage/runFilterByName read it and sendFilterByNameDone echoes it in the reply. - EngineRepresentation::runScriptOnProcess(RFilterByNameStore) records it as _lastFilterByNameRequestId; processFilterByNameReply DROPS replies for superseded requests (with a fallback: replies without a requestId, i.e. from older engines, are let through unchanged). Dropping happens inside EngineRepresentation so no downstream signal signatures change. Fresh transition: - DataSet::filterByNameDone now calls setInvalidated(false) after dbLoadResultAndError: the reply that reaches it is guaranteed to answer the latest request, so the filter's cached state matches its inputs afterwards, regardless of whether the values changed. - Filter::dbUpdate passes the actual _invalidated member to filterUpdate instead of the false default, so the DB flag is now truthful (it is loaded again by filterLoad on session restore). Consequences of the flag now being truthful: - setInvalidated(true)'s sendFilterByName emit stays UNCONDITIONAL on purpose. Gating it on wasChange is tempting but racy: an input change landing while a request is already in flight would be suppressed, and the in-flight reply (computed from older inputs) would then mark the filter fresh with outdated results. With duplicates being harmless (the newest request computes the newest inputs; superseded replies are dropped) the emit is simply 'ensure a run is queued for the current inputs'. Remaining audit filter-loop entry paths closed (item 5 of the plan): - ListModelFilteredDataEntry::initTableTerms: a re-bind with identical terms (analysis re-run with unchanged options) no longer re-runs the filter at all; cell values are not part of TableTerms so a value-only change cannot be missed by this. - ListModelFilteredDataEntry::filterDoneHandler: the filteredRowCount==0 retry is bounded to one attempt per state change (_retriedEmptyRun, reset on binds and dataset changes) instead of retrying forever. - DataSet::runFilters(editedColumn): after manual edits only filters that actually use the edited column (plus the default filter, whose generated label filter can depend on any column) are invalidated, instead of blanket-invalidating every named filter on every edit. Verified: both audit workflows now run to 'complete' via the RPC/agent API (auditClassicalWorkflow and auditBayesianWorkflow, 2 bounded filterByName runs each, no engine starvation), full gate test passes 271/271 analyses through the MCP layer, JASPTest shows no new failures (the 2 testDataImport CSV/TSV hardcoded-json failures predate this work, verified on a clean tree in 03c6bd1).
…rt of Filter::columnUsed DataSet::runFilters special-cased the default filter with an unconditional setInvalidated(true) because columnUsed() only knew about the R filter code (_columnsUsedInRFilter) and the drag&drop json (_columnsInConstructorJson), while the default filter's generated filter additionally incorporates LabelFilterGenerator output for every column with active label filtering. That dependency was invisible to columnUsed, so the default filter always had to be blanket-invalidated on manual edits. Instead of keeping the special case, make the dependency predicate tell the whole truth: - Filter::columnUsed now also returns true for the default filter when the column has active label filtering (_labelGen only exists for the default filter, named filters have none). This is a genuine dependency: a value edit can move a row between levels without changing the generated code itself, so the filter still has to be re-run. - Filter::datasetChanged now uses columnUsed() for changedColumns instead of duplicating the set-membership check, so the R-filter/constructorJson/ label-filter dependency logic lives in exactly one place. - DataSet::runFilters drops the _defaultFilter special case and treats all filters identically through columnUsed (the default filter is registered in _filters like any other, via Filter::connectionCreation). Behaviour changes for the better: editing a value in a column that is not used by any filter (no R-code reference, no drag&drop usage, no label filter) no longer re-runs ANY filter, including the default one — previously every manual edit re-ran the default filter in the engine. Editing a column with active label filtering still re-runs the default filter exactly as before. Verified: both audit workflows still run to 'complete' via the RPC/agent API with a bounded number of filterByName runs, full gate test passes 271/271 analyses through the MCP layer, JASPTest shows no new failures (the 2 testDataImport CSV/TSV hardcoded-json failures predate this work).
Review comment on PR jasp-stats#6313 pointed out that get_analyses_state (with a single analysisId, include_options=true, options_meta_diff=false) already returns the identity, lifecycle status, options and full optionMeta of an existing analysis, making analysis_getOptions redundant API surface. Removed the handler and its JASP_RPC.json spec entry, and dropped it from the gate test's expected-methods sanity check. No other code used it (the gate test drives analyses through create/run/remove). Note for the jasp-mcp repo: its README and SERVER_INSTRUCTIONS reference a 'jasp_analysis_getOptions' tool that never existed in jasp-desktop's spec (jasp-mcp registers tools dynamically from rpc.discover, so the tool simply was not there). That documentation should be updated to point to jasp_get_analyses_state instead.
…n.py Tests/gatetest/fuzztest.py fuzzes analysis options through the agent API: for every analysis it generates mutations of the default options based on the machine-readable optionMeta schema (checkbox/combo/variables/number/ integer/percent/string/array, with deliberately invalid values mixed in), runs them via analysis_run and classifies the outcome. Hunted (immediate abort + minimal .repro.json, exit 1): - JASP process death - hangs (run not terminal within --timeout-per-run) - malformed/non-JSON responses - engine-queue wedges: --max-consecutive-stuck consecutive runs that never even get scheduled (the audit-style wedge, observed to precede a SIGABRT with MixedModelsLMM/jaspMixedModels) Tolerated and counted: validationError/rejected options, R fatalError, JSON-RPC validation errors. -32603 internal errors and unexpected statuses are listed as suspicious (they do not fail the run unless --strict). Reproducibility: - a fresh random SEED is printed at start and end and stored in the report; --seed N replays a run exactly. The RNG stream is deterministic because a fresh analysis instance is created per mutation (reusing one instance leaked server-side option state into outcomes and optionMetaDelta, which perturbed the stream) and schema-derived orderings are normalised: JASP's combo choices array order varies between process starts, which is itself a small nondeterminism worth knowing about. - --repro <file.repro.json> replays a recorded failure in isolation. - the last mutated options are recorded with process-level crashes. Shared harness (RPC clients with MCP-first + direct fallback, JASP process handling, data loading, analysis enumeration) moved from gatetest.py into gatecommon.py; gatetest.py is unchanged behaviourally (full gate re-run: 271/271 through the MCP layer after the refactor). First findings from real runs (documented in Tests/gatetest/README.md, unfixed): huge numeric options surface as JsonCpp 'LargestInt out of Int range' -32603 internal errors instead of clean validation errors; garbage in the raincloudPlots customizationTable wedges the engine queue (analyses never scheduled again, one observed run ended in SIGABRT); JsonCpp 'requires objectValue' -32603s on jaspAnova/Anova with non-object options.
…lidation errors instead of -32603 internal errors The option fuzzer (Tests/gatetest/fuzztest.py) found two classes of uncaught Json::LogicError escaping from AnalysisForm::parseOptions (reached via the analysis_run RPC handler): - 'LargestInt out of Int range': TextInputBase::bindTo called asInt() on any isNumeric() value for integer options, so 1e300 / 2^63-1 threw in jsoncpp. No numeric range validation existed at all (analyses.json min/max only feed QML validators). - 'in Json::Value::find(begin, end): requires objectValue or nullValue': const operator[]/isMember/get throw on non-objects, and the isJsonValid gates for array-valued options only check 'is an array', never the element types. Affected: BoundControlTableView::fillTableTerms (row[...]), BoundControlFilteredTableView::fillTableTerms (firstRow[...]) and Term::Term(json, keyValue, ...) via BoundControlTerms/ComponentsList/ InputList bindTo. The RPC dispatcher caught these as std::exception and reported -32603 'Internal error' — misleading (it is invalid input, not a JASP bug), and worse: the same unguarded bindTo also runs from setAnalysisUp / setOptions / R-syntax rebinding / file load, i.e. OUTSIDE the dispatcher, where an escaping exception would terminate the process instead of surfacing as an error. A stored bad value (e.g. 1e300 for an integer option in a .jasp file) was therefore a potential user-reachable crash. Fixes: - AnalysisForm::parseOptions wraps parseRSyntaxOptions + bindTo in try/catch(Json::Exception, std::exception) and converts to a normal validation error (errorMsg -> 'Validation errors on analysis options'). - AnalysisForm::bindTo guards each control individually: a throwing option is flagged like any other wrong-valued option (warning + reset to defaults), and the rest of the form still binds. This also covers the non-dispatcher rebind paths. - Root-cause hardenings: TextInputBase integer binding checks the int range instead of just isNumeric(); Term requires a JSON object before isMember; table-view fillTableTerms skips non-object rows and unrepresentable cell values instead of throwing/creating invalid QVariants.
…RT found by the fuzzer
The option fuzzer found that removing an analysis while its run was in flight
(which happens after every fuzzed mutation) could wedge the entire engine
queue — analyses were never scheduled again (status stayed 'empty' until
removed) and one run ended in SIGABRT (exit -6).
Root-cause chain, all verified in logs and code:
1. Analysis removed while in flight: EngineRepresentation::analysisRemoved
sends perform=abort, pins _engineState at 'analysis' ('until the aborted
one gets the message') and records _idRemovedAnalysis.
2. The engine processed the abort but NEVER replied: runAnalysis' post-run
switch (case Status::aborted) returned silently, so the desktop always
fell through to its ENGINE_KILLTIME (750ms) kill+restart — aborts were
effectively useless, exactly as suspected.
3. Engine-side abort ack added (sendAbortAck -> analysisResultStatus::aborted,
new enum value) from both abort paths: the post-run switch and the
pre-run 'does not need to be run' branch.
4. But the ack still never reached the desktop's bookkeeping: the IPC channel
is a single-slot mailbox, so the engine's idle-reload messages
(sendEngineLoadingData/sendEngineResumed, every ~15s) overwrote the ack
before the desktop's 50ms tick read it, and
processEngineResumedReply then blindly resend() the last message — the
abort request — re-triggering the cycle forever. With _engineState pinned
away from idle, willProcessAnalysis() was always false and
processAnalysisRequests silently skipped every analysis.
Also: processAnalysisReply's removed-analysis switch accepted
changed/complete/fatalError/validationError but not 'aborted', so even a
read ack would not have released the engine state.
Fixes:
- Engine: sendAbortAck() on both abort paths (new analysisResultStatus::
aborted in Common/enginedefinitions.h).
- Desktop: processAnalysisReply handles aborted (cleares in-progress
bookkeeping, both for live and removed analyses).
- Desktop: processEngineResumedReply no longer blindly resend()s the last
message. A resumed reply means the engine has nothing running: only a
genuinely Running analysis gets its request replayed (the original
purpose — recovering a run request lost in an engine restart); an abort
in flight instead recovers to idle (clearing removed-analysis
bookkeeping) so the scheduler re-dispatches.
- EngineSync::createNewEngine off-by-one: freeChannel == maxEngineCount()
passed the '> maxEngineCount()' guard and wrote _engineStopTimes out of
bounds (silent heap corruption candidate) — now '>='.
Verified with the fuzzer on the two seeds that previously wedged/aborted
(234304393 and 585249745): both now complete all 34 analyses x 4 mutations
with zero wedges, zero crashes and zero -32603 internal errors (outcomes:
complete/rejected/fatalError/validationError only). Full gate test still
271/271 through the MCP layer; JASPTest unchanged (only the 2 pre-existing
testDataImport CSV/TSV hardcoded-json failures).
Known remaining wart (not addressed here): the IPC message id is a single
decimal digit (_msgIDSend % 10), so message-loss detection wraps after 10
unread messages; widening it would be a small protocol change on both
sides.
The shared-memory IPC channel prefixes every message with a message id so the receiver can detect fresh messages (and resend() can mark a replay). The id was a single decimal digit (_msgIDSend % 10), which meant loss-detection wrapped after only 10 unread sends: any message arriving while 10+ earlier sends had gone unread looked identical to the last-read id and was silently ignored by tryWait() until another 10 sends changed the digit. That was not theoretical: it made the engine's abort ack (from the previous commit's fix) deterministically invisible whenever the engine's idle-reload messages (loadingData/resumed, ~15s cycle) raced it, feeding the resend-abort-forever wedge fixed there. It also gave resend() a second loss mode: 10+ replays without a read collide with the last-read digit. Change: fixed-width zero-padded 4-digit prefix (modulo 10000), consistent across send / resend / tryWait / receive: - send(): assign(msgIDPrefix(_msgIDSend)) + payload - resend(): replace(0, MsgIDWidth, ...) instead of overwriting char 0 - tryWait(): parse exactly the first 4 characters (slots shorter than the prefix width cannot be from a matching build and are ignored) - receive(): strip 4 characters, with a length guard (the slot can be overwritten by a newer message between tryWait and the read, so a bogus slot must not underflow) Why not a naive % 100: the prefix width is part of the wire format on both ends (tryWait parsed 1 char, receive stripped 1 char), so widening the modulo alone would have misparsed every message and thrown 'Malformed reply' in processReplies. Both sides always ship from the same build, so there is no version skew in practice; mixing a new desktop with an old engine during development fails loudly (parse errors) rather than silently. Verified: full gate test 271/271 analyses through the MCP layer (thousands of IPC messages across the run), option fuzzer on seed 234304393 clean (no wedges/crashes; only complete/rejected/fatalError outcomes), JASPTest unchanged (only the 2 pre-existing testDataImport CSV/TSV hardcoded-json failures).
…fuzz sweep); ack aborts of removed analyses
Full option-fuzz sweep (~2600 mutations across all 271 analyses, excluding
jaspTestModule) surfaced a remaining crash family:
jaspMetaAnalysis analyses crashed JASP (SIGABRT, exit -6) after a mutation
whose option bind threw mid-way. Root cause (captured under lldb):
1. AnalysisForm::parseOptions catches the Json::Exception (per the previous
commit) and returns a validation error, but the bind was interrupted
mid-way: some option arrays were left with fuzzed non-object elements.
2. Later (deferred, QML-side), destroying the analysis' controls fires
RadioButton::destroyed -> unregisterRadioButton -> _setCheckedButton ->
BoundControlBase::setBoundValue -> AnalysisBase::boundValue ->
_getParentBoundValue, which called boundValue.isMember(parent.key) on a
NON-OBJECT array element -> Json::LogicError thrown from inside a Qt/QML
signal handler -> std::terminate -> SIGABRT (exit -6). lldb stack captured.
3. Aggravator found in the same logs: the removed-analysis switch in
processAnalysisReply accepted changed/complete/fatalError/validationError
but NOT the new 'aborted' ack, so a removed analysis' abort ack kept the
engine state pinned ('Analysis ignores the abort it got and keeps
going...') — re-creating the starvation wedge from fe5cafd in the
remove-while-in-flight path.
Fixes:
- QMLComponents/analysisbase.cpp: _getParentBoundValue now skips non-object
elements of parent value arrays instead of throwing (root cause: garbage
or half-bound arrays must not escape through QML signal handlers, which
cannot propagate exceptions).
- Desktop/engine/enginerepresentation.cpp: removed-analysis switch handles
analysisResultStatus::aborted (idle + clear bookkeeping), completing the
abort-ack handling for the removed case.
Verified: the exact crash sequence (EffectSizeComputation, 3 mutations,
remove, recreate) now completes with fatalError (graceful R error) and the
process stays alive; the jaspMetaAnalysis shard that consistently died
(2 analyses in, SIGABRT/-6) now passes 17/17 analyses x 8 mutations twice in
a row; full gate test 271/271 through the MCP layer; JASPTest unchanged
(only the 2 pre-existing testDataImport CSV/TSV failures).
Remaining fuzz findings documented in Tests/gatetest/README.md (unfixed):
unbounded R simulation loops for extreme/non-numeric 'pointsToWin' in
jaspLearnBayes game-of-chance/skill analyses, and a dispatcher hang with
jaspVisualModeling/mixedmod plot options (both have recorded repros).
Adds the Tests/gatetest section (gate test, fuzzer, shared harness, headless platform requirement, tolerable vs gate-aborting outcomes) and a 'Debugging JASP desktop crashes' section with the pitfalls hit while chasing the option-fuzzer findings: - lldb batch mode ends supervision at the first stop; fuzzy '-n abort' name matching resolves to all matching symbols (Analysis::abort alone has many uses) — pin exact symbols per library instead. - Silent clean exit(0) is a real JASP death mode: quitOnLastWindowClosed defaults to true and nothing disables it, so a closed/destroyed main QML window exits the app silently; the fuzzer summary filters exit code 0, so clean quits surface only as connection-refused transport errors. - Prefer launching under lldb over attaching mid-run; engine 'no parent alive' messages mean the desktop already died; the IPC channel is a single-slot mailbox (4-digit id prefix) where sends overwrite unread replies. Open item: post-rebase, the jaspMetaAnalysis option-fuzz shard exits cleanly (code 0) mid-run — likely the quit-on-last-window-closed path above; exact trigger still unidentified.
… DYLD interpose pitfalls in AGENTS.md Investigation of the remaining flaky fuzz deaths (silent 'exit code 0', one-off SIGBUS, SIGSEGV) across many instrumented runs (lldb attach/launch, SIGTERM/atexit diagnostics, a DYLD interposition tracer) resolved them into one real crash mode plus its disguises: The desktop dies in QV4::markDrain (QtQml incremental GC marking) with EXC_BAD_ACCESS on a mangled/PAC-style address — a stale JS reference to a C++-backed QML object freed during the analysis_remove-while-in-flight + fresh-create re-binding churn. Captured under guaranteed lldb launch supervision (wrapper script + process launch + auto-continue breakpoints). The same finding ended the first full option-fuzz sweep at 158/271 as a mystery 'exit code 0'; sibling manifestations observed: SIGBUS (-10), SIGSEGV (-11). Qt/QML ownership territory (JS heap vs AnalysisForm teardown timing) — not gate-harness tooling, and predates this branch. Documented in Tests/gatetest/README.md with the crash signature, and the AGENTS.md debugging section gained: lldb batch-attach ends supervision (wrapper + process launch + auto-continue breakpoint commands instead), fuzzer-output-based pid discovery, DYLD dlsym(RTLD_NEXT) interposition recursion pitfall (use raw SYS_exit), and the death-mode/exit-code map. Diagnostic instrumentation (SIGTERM/atexit loggers, exitSignal tracer) was temporary and is reverted; the gate test re-verified 271/271 after the revert.
JorisGoosen
force-pushed
the
testAnalysesLoad
branch
from
September 10, 2026 12:35
7d1f070 to
851394c
Compare
added 5 commits
September 10, 2026 16:31
…time-slicing; deferred analysis/form teardown; guard stale-reply null deref
The long-standing intermittent desktop crash ('hard to reproduce' for a
while) is now root-caused thanks to the option fuzzer, which can trigger it
reliably (seed 0, jaspMetaAnalysis shard: ~100% death rate, 3/3 control
runs).
Root cause, verified by experiment and Qt 6.11.1 source:
- Qt's QV4 GC runs incrementally: GCStateMachine::transition() slices
mark/sweep into ~5ms event-loop passes (default QV4_GC_TIMELIMIT =
(1000/60)/3), suspending mid-cycle via a queued onEventLoop() re-invocation
and resuming on the next pass.
- JASP destroys C++-backed QML objects (AnalysisForm trees, controls, list
models) from the same event loop at arbitrary points — most aggressively
when analysis_remove lands while an R run is still in flight and a fresh
analysis_create immediately re-binds.
- Qt's wrapper invalidation handles wrappers discovered unmarked mid-cycle,
but has NO re-validation for wrappers already marked when their QObject is
destroyed between slices (markWeakValues only checks liveness at mark time;
cleanupDeletedQObjectWrappersInSweep runs at sweep). The next markDrain
slice dereferences the freed object: EXC_BAD_ACCESS on a mangled/PAC-style
address, surfacing as SIGSEGV (-11), SIGBUS (-10), or, depending on timing,
a confusingly 'clean' exit (Chromium's shutdown detector converts delivered
signals into exit(0), which the fuzzer summaries filtered out).
The experiment that pinned it: setting QV4_GC_TIMELIMIT=0 makes transition()
run the whole GC atomically (QDeadlineTimer::Forever) — control runs 3/3
crashed, env-var runs 3/3 clean, same binary, same seed. This commit bakes
that in: qputenv('QV4_GC_TIMELIMIT', '0') before any QQmlEngine exists.
Cost: GCs are no longer time-sliced, so a full GC pause is longer — but
rare; JASP's QML heap per analysis form is small.
Also in this commit (secondary hardening from the same investigation):
- Deferred teardown on the removal path (Analyses::removeAnalysis and
Analyses::clear now deleteLater the Analysis instead of a synchronous
delete; ~Analysis synchronously deletes the form at the deferred quiet
point; AnalysisBase::destroyForm on the live path deleteLater's the form).
This aligns JASP with the codebase's own cleanUp+deleteLater pattern and
removes the form-tree teardown from the middle of the GC window. It did
NOT fix the crash by itself (6/6 still died), but it is correct on its own
terms and reduces GC churn around teardown.
- EngineRepresentation::processAnalysisReply: a reply arriving while
_analysisInProgress is null and the id does not match the tracked removed
analysis is now ignored instead of dereferencing the null pointer.
Verification: jaspMetaAnalysis seed-0 shard was 6/6 dead before, 3/3 clean
after (in-process fix); full-gate test to be re-run; full 271-analysis fuzz
sweep to follow in a separate run.
Known machine-crash note: during one post-fix full-sweep attempt the whole
machine crashed while the fuzzer was churning (separate from the JASP
SIGSEGV; suspect GPU/driver under offscreen-WebEngine load — unverified).
The sweep is being re-run in chunks.
…lopment/qv4-gc-crash.md) Complete record of the intermittent desktop crash that the option fuzzer made reproducible: crash signature (EXC_BAD_ACCESS in QV4::markDrain during incremental GC marking), the trigger (analysis_remove + fresh analysis_create churn), the mechanism (Qt 6.11.1 wrapper invalidation has no re-validation for wrappers already marked when their QObject is destroyed between GC slices), the experiments that ruled things out (incl. QV4_GC_TIMELIMIT=0 appearing to fix but not), the flakiness that defeated repeated verification, the fixes we committed alongside (web-view JS leak, deferred teardown, null-deref guard, tree-RSS watchdog), and the options for a future fix or a Qt issue.
…res; drop ineffective QV4_GC_TIMELIMIT=0 - fuzztest.py --restart-on-death: when the desktop dies (e.g. the flaky QV4 GC crash) or wedges, restart it and continue the sweep instead of aborting. Deaths stay in the report; this makes full-coverage sweeps completable on a codebase with a known flaky crash, and the crash count becomes real data. - fuzztest.py --max-tree-rss-mb (default 6000): samples the RSS of the whole JASP process tree (desktop + QtWebEngine helpers + R engines) and kills it before the machine chokes. The 35GB RAM incident showed WebEngine helpers must be included (early probes measuring only the desktop saw ~2GB). - Remove qputenv(QV4_GC_TIMELIMIT, 0): the QV4::markDrain crash recurs with it (verified under launch-supervised lldb), so the setting only added longer atomic GC pauses while claiming to close the race. The comment claiming it fixed the crash was wrong and is gone; the authoritative record is Docs/development/qv4-gc-crash.md. - Tests/gatetest/repros/: durable copies of the recorded failure repros (LSgameofchance/LSgameofskill unbounded R sim loops, mixedmod dispatcher hang) — the originals lived in /tmp and were lost to disk cleanup. The mixedmod wedge itself is now fixed by the deferred teardown (032c749): the original seed-234304393 shard completes cleanly; its repro is kept for regression checking. - .gitignore: Tests/gatetest/.venv/ and __pycache__/.
…s survived via --restart-on-death Seed 77, 2152 runs in 95 min: 1092 complete, 1020 rejected (graceful validation), 35 fatalError (tolerated R errors), 3 validationError, and exactly 2 crash occurrences — both the known flaky QV4::markDrain GC crash (see Docs/development/qv4-gc-crash.md), recovered through by --restart-on-death. Zero wedges (the mixedmod dispatcher hang fix held), zero watchdog kills (the web-view JS leak fix held: tree RSS under the 5GB cap for the whole sweep).
boost::container::string (the shared-memory string type) has no overloads taking std::string for assign/replace, so MSVC rejects msgIDPrefix(...), which returns std::string. Use .c_str() to hit the const CharT* overloads. clang (macOS) accepted the implicit conversion, hence the branch building fine there.
Contributor
|
what am I supposed to review here? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements a test where all modules+analyses are loaded via rpc/mpc at least once to see if they load at least the default.
This pointed the bot to a bug in filtereddataentry, which it then fixed.
I still need to review all of this as well.
Should implement https://github.com/jasp-stats/INTERNAL-jasp/issues/3323