Fix thread-unsafe Generator publication race in HttpRouter#compile

[hazel-tan]

Summary

Surfaced in https://appsignal.com/kaligo/sites/5a7a817101925b30127a73d7/exceptions/incidents/612/traces/60ccb4b3070014d393591b73078771ad

Stacktrace:

"/app/vendor/bundle/ruby/3.4.0/bundler/gems/http_router-8aac509ac22c/lib/http_router/generator.rb:69:in 'HttpRouter::Generator#max_param_count'",
"/app/vendor/bundle/ruby/3.4.0/bundler/gems/http_router-8aac509ac22c/lib/http_router/generation_helper.rb:4:in 'HttpRouter::GenerationHelper#max_param_count'",
"/app/vendor/bundle/ruby/3.4.0/bundler/gems/http_router-8aac509ac22c/lib/http_router.rb:300:in 'block (2 levels) in HttpRouter#compile'",

HttpRouter::Generator#initialize publishes itself to the shared Route object (@route.generator = self) before it finishes computing @path_generators.

    def initialize(route, paths)
      @route, @paths = route, paths
      @router = @route.router
      @route.generator = self
      @path_generators = @paths.map do |p|
        PathGenerator.new(route, p.is_a?(String) ? p : route.path_for_generation, p.is_a?(Regexp) ? p : nil)
      end
    end

Combined with HttpRouter#compile's unsynchronized return if @compiled guard, concurrent threads racing into compile for the first time can read route.generator.max_param_count on a Generator that's still mid-construction — hitting undefined method 'map' for nil on the not-yet-assigned @path_generators.

  def compile
    return if @compiled
    @root.compile(@routes) # e.g thread_1 and thread_2 are here, they respectively publish generator_1 and generator_2 (@route.generator = self above)
    @named_routes.each do |_, routes| # once thread_1 is done with the above line it will proceed here
      # but @named_routes includes the routes (and hence generators) published by both thread_1 and thread_2
      # but for thread_2 routes, the generator might not have finished initializing hence @path_generators for it might be nil
      routes.sort!{|r1, r2| r2.max_param_count <=> r1.max_param_count } # hence throwing NoMethodError when it tries to access .max_param_count
    end

This surfaces in multi-threaded Rack servers (e.g. Puma with multiple threads) as an intermittent NoMethodError: undefined method 'map' for nil, typically right after a fresh worker boots and the first burst of concurrent requests all hit the router before it's compiled.

Some examples encountered:

• https://grafana.prd.int.kaligo.com/goto/efvqhgxkfpdkwa?orgId=default
• https://grafana.prd.int.kaligo.com/goto/dftckfybwtvcwf?orgId=default

Fix

Move @route.generator = self to the end of initialize, after @path_generators is fully computed, so no other thread can ever observe a partially-built Generator.

Verification

Reproduced the original failure by widening the race window with an injected sleep at the point our theory says the unsafe ordering exists, then ran the same test against the fix.

def build_router
  router = HttpRouter.new
  router.add('/foo/:a').name = :dup
  router.add('/bar/:b/:c/:d').name = :dup
  router
end

class HttpRouter
  class Generator
    def initialize(route, paths)
      @route, @paths = route, paths
      @router = @route.router
      # @route.generator = self # pre fix
      @path_generators = @paths.map do |p|
        PathGenerator.new(route, p.is_a?(String) ? p : route.path_for_generation, p.is_a?(Regexp) ? p : nil)
      end
      sleep 0.05
      @route.generator = self # post fix
    end
  end
end

hits = 0
100.times do
  router = build_router
  errors = []
  threads = 8.times.map { Thread.new { begin; router.send(:compile); rescue => e; errors << e; end } }
  threads.each(&:join)
  hits += 1 if errors.any?
end
puts "hits: #{hits}/100"

Pre-fix (with the sleep inserted at the old, buggy position — publish before @path_generators is computed):

REPRODUCED: NoMethodError: undefined method 'map' for nil
generator.rb:69:in 'HttpRouter::Generator#max_param_count'
generation_helper.rb:4:in 'HttpRouter::GenerationHelper#max_param_count'
http_router.rb:300:in 'block (2 levels) in HttpRouter#compile'
http_router.rb:300:in 'Array#sort!'
http_router.rb:300:in 'block in HttpRouter#compile'

Post-fix (sleep inserted at the new position, same test, 100 iterations):

hits: 0/100

[hazel-tan]

Rollout plan for TC

1. Merge this and release a new version
2. Upgrade the http_router gem in TC to the new version
3. Let it ferment in staging for ~week to ensure there is no regression
4. Merge to prod and monitor

[hazel-tan]

Hey @ngsikai, requested your review as you're on-call and also since you might have more context on this gem!

Let me know if you want a separate sync up on this 🙏

[ngsikai]

Hey Hazel will set up a session with you on this for Monday

[ngsikai]

Synced up offline with @hazel-tan, will pull this custom branch on TC staging for fermentation and eventually with TC production.

Once it is stable, we will roll out on master and inform other teams

[hazel-tan]

Fermenting in TC staging here: https://github.com/Kaligo/transfer_connect/pull/5230

[hazel-tan]

Fix has been deployed to TC prod - will let it ferment to verify that there is no regression