Skip to content

fix(gorillas3processor): single critical section for ensureRoleState + consume/flush (TOCTOU race) - #395

Merged
zzylol merged 1 commit into
mainfrom
fix/gorillas3-race-toctou-rolestate
May 18, 2026
Merged

zzylol merged 1 commit into
mainfrom
fix/gorillas3-race-toctou-rolestate

Conversation

@zzylol

@zzylol zzylol commented May 18, 2026

Copy link
Copy Markdown
Contributor

Summary

ConsumeMetrics (line 114) and flushWindow (line 298) each called
ensureRoleState() which took p.mu, optionally created
p.rawBuilder / p.fragmentEncoder / p.fragmentFinalizer, then
released p.mu — and then the callers re-acquired p.mu to do their
real work. Between the two acquisitions, another goroutine could
mutate the role state.

Concretely for the default (gateway-raw) role:

  1. ConsumeMetricsensureRoleState() takes p.mu, lazily creates
    p.rawBuilder, releases p.mu.
  2. Timer goroutine flushWindow runs, takes p.mu, calls
    flushGatewayRawLocked which at line 337 does
    p.rawBuilder = nil (the next sample is expected to lazily
    recreate it), releases p.mu.
  3. ConsumeMetrics re-acquires p.mu and calls
    consumeGatewayRawLockedingestRawMetricLocked
    p.rawBuilder.AddSample → SIGSEGV (rawBuilder is now nil).

Observed agent panic in the multinode harness:

panic: runtime error: invalid memory address or nil pointer dereference
asap-gorilla-go.(*StreamingTSDBBlockBuilder).AddSample(0x0, ...)
gorillas3processor/processor.go:233

The agent crash-loops every ~10s, which silently breaks the DDSketch
flush path. PR #394's multinode validation masked this because
sum by (zone) (http_requests_total) travels via raw_passthrough
and is aggregated at the backend; quantile_over_time(_latency_ms)
queries fail because the DDSketch state never makes it across.

Fix (Option 2)

  • Rename ensureRoleStateensureRoleStateLocked and remove its
    internal p.mu.Lock()/Unlock(). Caller's responsibility now.
  • ConsumeMetrics: take p.mu, call ensureRoleStateLocked, then
    do consume work — all inside the SAME critical section.
  • flushWindow: take p.mu, call ensureRoleStateLocked, then
    dispatch to flush{Agent,GatewayFragment,GatewayRaw}Locked.
  • Start: take/release p.mu around the init call (the
    background flush goroutine isn't running yet, but the lock is
    cheap and keeps the contract uniform).

The same fix protects the flushAgentLocked /
flushGatewayFragmentLocked paths that also nil out
p.fragmentEncoder / p.fragmentFinalizer after a successful flush.

Drive-by harness fix

deploy/mvp-multinode/scripts/run_demo.sh invokes
thanos-query with no --grpc-address, which defaults to
:10901 — that collides with thanos-store-gateway (also :10901)
under --network host on node2. :10902 also collides (store-gateway
HTTP). Pin query's gRPC listener to :10905 so all three thanos
containers coexist. Without this, the 5-query validation can't run
at all — asap-backend's thanos_query engine returns
connection-refused. Only --grpc-address is added; nothing else
moves.

Validation (multinode)

go test -race ./... clean (3.5s).

Crash-loop / nil-deref counters after 5 min uptime under load:

signal node0 agent-a node3 agent-b node1 gateway
panic|SIGSEGV count 0 0 0
AddSample (nil deref site) 0 0 n/a
status Up 5 minutes Up 5 minutes Up 5 minutes

5-query gate (all status:success, all data_source: asap_query):

=== quantile_over_time(0.99, http_requests_total_latency_ms[5m]) ===
result[0]: zone=z3 value=104.6   result[1]: zone=z2 value=122.75
result[2]: zone=z1 value=89.13   result[3]: zone=z3 value=115.6  ... (many series)

=== quantile_over_time(0.5,  http_requests_total_latency_ms[5m]) ===
result[0]: zone=z3 value=17.99   result[1]: zone=z2 value=...    ... (many series)

=== sum by (zone) (http_requests_total) ===
z0=305962242  z1=305966329  z2=305964653  z3=305965054

=== sum by (zone) (rate(http_requests_total[5m])) ===
z0=2.257e6  z1=2.258e6  z2=2.258e6  z3=2.258e6

=== topk(5, sum by (zone) (rate(http_requests_total[5m]))) ===
z1=2.258e6  z3=2.258e6  z2=2.258e6  z0=2.257e6

The two quantile queries are the regression check — they failed
pre-fix because the agent's DDSketch path crash-looped and never
shipped state. They pass post-fix. The three sum/topk queries
continue working, so PR #394's behavior is preserved.

Test plan

Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com

…+ consume/flush (TOCTOU race)

ConsumeMetrics (line 114) and flushWindow (line 298) each called
ensureRoleState() which took p.mu, optionally created
p.rawBuilder/p.fragmentEncoder/p.fragmentFinalizer, then released
p.mu — and then the callers re-acquired p.mu to do their work.
Between the two acquisitions another goroutine could mutate the
role state. Concretely, for the default (gateway-raw) role:

  - ConsumeMetrics → ensureRoleState() takes p.mu, lazily creates
    p.rawBuilder, releases p.mu.
  - Timer goroutine flushWindow runs, takes p.mu, calls
    flushGatewayRawLocked which at line 337 sets p.rawBuilder = nil
    (the next sample is expected to lazily recreate it), releases.
  - ConsumeMetrics re-acquires p.mu and calls
    consumeGatewayRawLocked → ingestRawMetricLocked →
    p.rawBuilder.AddSample → SIGSEGV (rawBuilder is now nil).

Observed agent crash in the multinode harness:

  panic: runtime error: invalid memory address or nil pointer dereference
  asap-gorilla-go.(*StreamingTSDBBlockBuilder).AddSample(0x0, ...)
  gorillas3processor/processor.go:233

The agent crash-loops every ~10s, which silently breaks the DDSketch
flush path. PR #394's multinode validation masked this because
sum-by-zone travels via raw_passthrough and is aggregated at the
backend; the quantile_over_time(_latency_ms) queries fail because
the DDSketch state never makes it across.

Fix (Option 2): rename ensureRoleState → ensureRoleStateLocked,
remove its internal lock, and have ConsumeMetrics / flushWindow /
Start take p.mu first, then call ensureRoleStateLocked, then do
their work — all inside the SAME critical section. The init step
and the subsequent read/mutation of the role-state field are now
atomic with respect to each other. Same fix protects the
flushAgentLocked / flushGatewayFragmentLocked paths that also
nil out p.fragmentEncoder / p.fragmentFinalizer.

Tests: go test -race ./... passes (3.5s).

Validation (multinode, post-fix):

  panic/SIGSEGV count on node0 agent-a:       0
  AddSample (nil-deref site) on node0:        0
  node0/node3 agent + node1 gateway uptime:   5+ minutes (no crash loop)

  quantile_over_time(0.99, _latency_ms[5m])   status:success, asap_query, real values
  quantile_over_time(0.5,  _latency_ms[5m])   status:success, asap_query, real values
  sum by (zone) (http_requests_total)         status:success, 4 zones, ~3e8 ea
  sum by (zone) (rate(http_requests_total))   status:success, 4 zones, ~2.3e6 ea
  topk(5, sum by (zone) (rate(...)))          status:success, 4 zones ranked

Drive-by harness fix in deploy/mvp-multinode/scripts/run_demo.sh:
thanos-query --grpc-address defaulted to :10901 which collides
with thanos-store-gateway (also :10901) under --network host on
node2 (and :10902 collides with the store-gateway HTTP port).
Pin query's gRPC listener to :10905 so all three thanos containers
coexist. Without this, the 5-query validation can't run at all —
asap-backend's thanos_query engine returns connection-refused.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol
zzylol merged commit 162d3c6 into main May 18, 2026
zzylol added a commit that referenced this pull request May 18, 2026
…nt, aggregate_report (#396)

Three orthogonal fixes surfaced by a clean post-#395 all-arms run:

1. queries-e2e.json: change [1m] → [5m] for the three *_over_time
   entries (quantile_over_time x2, sum_over_time x1). The agent
   window is 60s, backend tumbling window 30s, multinode soak 90s
   — a [1m] range fired early in the soak frequently lands between
   open windows and returns empty. Convention used everywhere else
   (smoke test, manual probes, #394 wave queries) is [5m]. Leave
   `quantile by (zone) (...)` alone — no range arg.

2. run_demo.sh::arm_measure: point the b0/b1 PromQL replay endpoint
   at VictoriaMetrics :8428, not Prometheus :9090. Diagnosis: the
   harness brings up `asap-victoriametrics` (not Prometheus) on
   node2 for b0/b1 (see backend_up, line 104–109), and the agents
   PRW to `http://victoriametrics:8428/api/v1/write`. The query
   endpoint at :9090 was unreachable — `curl http://10.10.1.3:9090`
   → connection refused → urllib URLError → `status: timeout`
   → 100% of 899 attempts failed with http_code=null. Live probe
   of :8428 against a fresh b0 arm returns 10000 series of
   http_requests_total and resolves `sum by (zone) (rate(...[5m]))`
   into 4 zones with real values; a 15s replay yielded 78/138
   success (the 60 empties are the 3 *_latency_ms queries — that
   metric isn't in the b0 workload).

3. New `deploy/mvp-multinode/scripts/aggregate_report.py` to fill
   the missing script run_demo.sh::all referenced. Pure stdlib,
   handles the multinode dir shape (per-arm subdir containing
   replay.jsonl + edge-<node>.csv + stages-<node>.csv). Emits a
   summary table, per-query success/empty/error breakdown, edge
   bandwidth and stages aggregates. Idempotent. Failure stays
   non-fatal (run_demo.sh swallows it).

Validation (all-arms run after fixes):

  Arm   Total  Success  Empty  Error
  asap    413      332      0     81
  b0      829      472    357      0
  b1      835      476    359      0

- asap wave queries (sum-by-zone, sum-by-zone-rate, topk-of-rate,
  sum_over_time): 59/59 each → #395 wave-query gate not regressed.
- asap [5m] quantile_over_time: 48/59 (was 0/119 with [1m]) →
  Fix 1 verified.
- b0/b1: 472/829 + 476/835 (was 0/899 + 0/899) → Fix 2 verified.
  The 357/359 "empty" results are the 3 *_latency_ms queries —
  the b0/b1 workload's producer emits only http_requests_total,
  so latency_ms returns [] (not an error).
- MVP_REPORT.md emitted at run-dir root with full per-arm tables
  → Fix 3 verified.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol
zzylol deleted the fix/gorillas3-race-toctou-rolestate branch July 17, 2026 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant