Run specs in parallel, fix concurrency bugs - #44
Draft
apiology wants to merge 10 commits into
Draft
Conversation
Rebases castwide#1167 (apiology/parallel_rspec) onto castwide/master instead of v0.59, since master and v0.59 have diverged substantially and v0.59 carries unrelated changes. This commit is the net diff of that branch (plus its merge-conflict and CI-regression fixes) applied directly against master; history was not preserved per request. Highlights: - Parallelize per-gem YARD/RBS pin caching with a thread pool (doc_map.rb, shell.rb) instead of caching gems serially. - Fix a mutex re-entry deadlock in Library#sync_catalog when the next cacheable gemspec is already being processed elsewhere (castwide#1220), with its regression test. - Fix an exponential-blowup bug in Pin::Method#combine_same_type_arity_signatures (O(n^2) bail-out for large signature sets), with its regression test. - shell.rb's gems 'core' command called PinCache.core?/PinCache.cache_core, which never existed; use the real Solargraph::RbsMap::CoreMap#pins API instead. - Bundler::LazySpecification#materialize_for_installation is an internal, undocumented Bundler API whose arity changed without a deprecation path; guard against all known shapes (modern wrapper, old zero-arg method, incompatible-arity method) instead of assuming one signature. - Misc RuboCop/YARD-doc fixes and .rubocop_todo.yml updates.
Each of these explained a distinct class of intermittent CI failure observed across this PR's rspec matrix/parallel_tests jobs, and will only get more frequent as test parallelism increases. - Diagnoser: an uncaught exception during a background diagnosis (e.g. a file/directory disappearing mid-diagnosis, such as protocol_spec's around-block temp dir cleanup racing the async diagnoser thread) killed the thread before it reached the line that marks it fully stopped, so Host#fully_stop hung for its full 240-second timeout every time. Now rescues broadly around individual diagnoses and guarantees the fully_stopped flag is set via ensure regardless of how the thread's loop exits. - MessageWorker: stop() never signaled its condition variable, so a thread blocked in tick's wait() with an empty queue could never wake up to notice stopped? and exit - a permanently leaked thread. Also added fully_stopped? tracking (matching Diagnoser) and wired it into Host#fully_stopped?, which previously didn't wait on MessageWorker's thread at all. - PinCache#save wrote directly to the final cache path with no atomicity. Multiple parallel_tests workers (separate OS processes) racing to cache the same not-yet-warm gem for the first time could corrupt or truncate each other's writes on the shared cache directory. Now writes to a temp file and renames into place (atomic on the same filesystem). - Yardoc.cache invoked a bare `yardoc` command, relying on it being found via shell PATH - which fails for unbundled environments/subprocesses where it only exists inside the current bundle's own bin directory. Now resolves the actual executable via Gem.bin_path, independent of PATH. - rubocop_helpers_spec.rb's "custom version" test unconditionally removed the process-global RuboCop constant in its cleanup, even when its own version-swap had been a no-op (because something else in the process, e.g. protocol_spec.rb's top-level require, had already loaded the real gem first) - i.e. even when there was nothing to restore. That left RuboCop undefined for the rest of the process, cascading into failures in unrelated specs (library_spec, protocol_spec's formatting/environment handlers, rubocop_spec) whenever this spec happened to run first. Now only cleans up (and reloads the real version) when the swap actually took effect. Local full-suite run: 13 failures -> 2, both isolated/self-contained and already understood (rubocop_helpers_spec's version-swap doesn't work when rubocop was already required by something else first, and a pre-existing gem_pins_spec bug in this PR's own test content).
Yardoc.cache's "check cached, else build" was a classic check-then-act race: two OS processes (e.g. two parallel_tests workers, each caching the same not-yet-cached gem for the first time) could both see "not cached" and run `yardoc --db path` concurrently against the same .yardoc database directory, corrupting or truncating each other's output. This is very likely the actual cause of spec/pin/base_spec.rb's intermittent "deals well with known closure combination issue" failure and the strict_spec.rb Kramdown-constant failure in CI (both build/read a gem's YARD pins via this path) - my earlier PinCache#save atomic- write fix only covered Solargraph's own Marshal cache files, not this separate tool-managed directory. Wrap the build in a per-gem flock'd lock file, re-checking cached? after acquiring the lock (double-checked locking) so only one process actually builds a given gem; the rest wait for the lock and then reuse what the first process built instead of racing. This is the same mechanism a prior, incomplete attempt at this (Yardoc.processing?, referenced from Library#diagnose but never actually used to coordinate Yardoc.cache itself) was clearly reaching for.
This reverts commit 68561b2.
The two remaining intermittent failures (Kramdown constant in strict_spec.rb, 0 pins in pin/base_spec.rb) were never a cross-process caching race at all - reproduced deterministically locally with a cold cache, in complete isolation, no concurrency involved. My earlier PinCache atomicity and yardoc-locking fixes were solving a real but different problem than this one. Both tests called ApiMap#cache_gem(spec) for a gem before ever telling the ApiMap's DocMap that gem was needed (via #catalog with external_requires). DocMap#cache only builds pins for gemspecs in its own uncached_yard_gemspecs/uncached_rbs_collection_gemspecs lists, which are only populated from requires resolved during #catalog - so cache_gem was silently a no-op, and building only happened to succeed when something else had already warmed the gem's cache earlier in the same process (hence "intermittent", depending entirely on test/file run order and cache state, not timing). Fixed both to use the catalog -> cache_all_for_doc_map! -> catalog sequence already used correctly elsewhere in this same PR (see rbs_map/conversions_spec.rb's "with superclass pin for Parser::AST::Node" context): catalog first so DocMap learns about the dependency, cache_all_for_doc_map! to build it, catalog again to reload the ApiMap's pin store with the now-cached pins. Verified: both pass individually and together with ~/.cache/solargraph completely cleared beforehand (previously guaranteed to fail cold, pass only by accident once something else had warmed the cache). Full local suite with a cold cache: 1625 examples, 2 failures - both isolated, already-known, unrelated issues (rubocop_helpers_spec's version-swap test doesn't work when rubocop was already required by something else first, and a real RBS/YARD merge bug in gem_pins_spec.rb) - no other instances of this cache-ordering bug found anywhere else in the suite.
protocol_spec.rb's around block chdirs into a per-example temp directory and back, on the main thread, without holding Solargraph::CHDIR_MUTEX - the same mutex that Diagnostics::Rubocop#diagnose and the textDocument/formatting handler already use specifically because RuboCop::Runner internally chdirs (with a block) to read config files. An earlier fix in this series made the background diagnoser thread resilient to errors instead of dying on the first one (rescuing broadly, guaranteeing fully_stopped? via ensure), so it now keeps running background diagnoses - including RuboCop ones - for longer during a test run. That made it far more likely to have an active chdir in flight from that mutex right as protocol_spec's own (unsynchronized) chdir ran, which Ruby raises as "conflicting chdir during another chdir block" (visible in CI as a wave of Protocol example failures, e.g. "handles textDocument/definition"). Route protocol_spec's chdir calls through the same Solargraph::CHDIR_MUTEX so they can't overlap with RuboCop's.
An audit of the full diff against master (prompted by "are we dragging in v0.59 changes that weren't intended?") found several small items inherited from the original apiology/parallel_rspec branch history, predating this session's rebase: - Diagnostics::Base#diagnose and TypeCheck#diagnose gained a `workspace:` kwarg (commit "Spec performance fixes", a 33% local speedup) so TypeChecker.new could reuse an already-loaded Workspace instead of implicitly building a fresh one via Workspace.new(File.dirname(filename)) on every diagnose call. The kwarg was added but the one production call site, Library#diagnose (library.rb), was never updated to pass it, so the optimization was inert. Wire it through, and add the same kwarg to the other Diagnostics::Base subclasses (Rubocop, UpdateErrors, RequireNotFound) so the polymorphic call in Library#diagnose doesn't raise ArgumentError for reporters that don't use it. - Remove RbsMap::StdlibMap.possible_stdlibs: added, never called. - Remove duplicate/redundant YARD @PARAM comments added to ComplexType#qualify and .parse alongside the existing docs. - Revert a no-op reordering of Workspace#gemfile?/gemspec?/gemspec_files back to their master position; both locations are public, so this wasn't a visibility change, just unexplained churn. Left alone: a stray "@todo Missed nil violation" comment in source/chain.rb, and RuboCop-autocorrect-driven formatting diffs elsewhere in the branch (YARD/CollectionStyle, quote style) that predate this rebase and are needed to keep Overcommit clean.
The Hash{Array(String, String) => ...} -> Hash{Array, String, String
=> ...} docstring reformatting in api_map/constants.rb,
api_map/store.rb, doc_map.rb, source_map.rb, and the quote/block-style
cleanup in spec/source/chain_spec.rb weren't related to this PR's
stated purpose (parallel specs, concurrency fixes) - they were fixing
YARD/CollectionStyle and Style/StringLiterals offenses that a fresh
`rubocop --auto-gen-config` surfaces under the currently-installed
RuboCop/rubocop-yard versions but that master's own committed
.rubocop_todo.yml doesn't yet grandfather (a pre-existing drift, not
something this branch introduced).
Revert those files to master's content and add a scoped
YARD/CollectionStyle todo exclusion for the same 4 files, so this
branch stays green without carrying the unrelated reformatting. The
actual fix now lives in a standalone PR:
#42.
Two follow-ups to the earlier extraction: - The per-file YARD/CollectionStyle todo exclusion was a stand-in for actually fixing the cop. Replace it with disabling the cop outright in .rubocop.yml, matching castwide#1237's fix: for nested-generic/tuple Hash key types, the cop's own long-style autocorrect produces syntax that doesn't preserve the original tuple's meaning, so there's no safe autocorrected form to converge on for those cases. - UniqueType#to_rbs's all_params.empty? nil-safety fix is an unrelated, pre-existing bug (from the original branch's own history, unrelated to parallel specs) - move it to #43.
Rebases this branch's net contribution onto apiology/speed_up_specs_master (castwide#1237) instead of master directly, since both branches independently fixed the same catalog-before-cache_gem gem-pin-caching ordering bug and both added Diagnostics::Base#diagnose's workspace: kwarg. Resolving those as one shared fix (favoring castwide#1237's implementation, already under review upstream) instead of carrying two competing copies. Conflict resolutions: - ApiMap#resolve_require: kept castwide#1237's version (raises on a nil workspace, per its own review feedback) over this branch's safe-navigation version. - spec/api_map_method_spec.rb, spec/pin/base_spec.rb: kept castwide#1237's structure/helpers (`let(:catalog)`, before-hook pattern) where it already covers the same ordering fix; kept this branch's cache_all_for_doc_map! call in pin/base_spec.rb since it exercises the parallel-caching path this PR is actually about. - spec/rbs_map/conversions_spec.rb: kept castwide#1237's shared before(:all) ApiMap load across all examples in the file (faster than this branch's per-example ApiMap.new) - same test cases either way. - spec/yard_map/mapper_spec.rb: kept castwide#1237's removal of a test whose description ("marks correct return type from RuboCop::Options.new") no longer matched its body (had been repointed at Open3.capture2e, already covered by spec/rbs_map/conversions_spec.rb). - .github/workflows/rspec.yml: kept castwide#1237's pinned `bundler: 2.5.23` install step alongside this branch's `rake full_spec`/pre-caching additions.
2 tasks
Closed
5 tasks
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
1 participant
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.
Summary
Net diff on top of castwide#1237: runs specs in parallel and fixes the concurrency
bugs that surfaced along the way.
(
DocMap#cache_doc_map_gems!,shell.rb'sgemscommand), plus theparallel_tests/rspec-time-guardtest infrastructure and CI changesneeded to actually run specs in parallel.
Library#sync_catalogwhen the nextcacheable gemspec is already being processed elsewhere
(Fix recursive-mutex deadlock in Library#sync_catalog castwide/solargraph#1220).
could hang
Host#fully_stop, a non-atomicPinCachewrite race, and anunsynchronized
Dir.chdirinprotocol_spec.Pin::Method#combine_same_type_arity_signatures, dead-code fix inshell.rb'sgems core, andyardoc.rbresolving theyardbinary viaGem.bin_pathinstead of depending onPATH.Test plan
bundle exec rspec,rubocop,overcommit --diff apiology/speed_up_specs_master: clean (2 pre-existing, unrelatedfailures:
rubocop_helpers_spec.rborder-dependence,gem_pins_spec.rbHashdiff combined-pin, both reproduce identically on unmodified Spec performance fixes castwide/solargraph#1237)
Generated with Claude Code