mirror of
https://github.com/alibaba/higress.git
synced 2026-07-25 05:40:23 +08:00
Compare commits
1 Commits
add-deep-t
...
issue-spec
| Author | SHA1 | Date | |
|---|---|---|---|
|
d01c41de83 |
@@ -1,61 +0,0 @@
|
||||
---
|
||||
name: higress-deep-troubleshooting
|
||||
description: Deep-dive troubleshooting methodology for Higress gateway bugs that require code-trace evidence across the higress-group/envoy and higress-group/proxy-wasm-cpp-host forks — CPU spins, infinite loops, Wasm VM reentrancy, filter-chain stage-mismatch, callback-ordering issues, SaveRestoreContext/current_context_ bugs. Use when investigating a complex Higress/Wasm/proxy-wasm incident that needs root-cause analysis with file:line citations, when ramping up on a reported CPU-spin or crash, or when verifying whether a proposed fix covers related bug classes. Accumulates case-study patterns under patterns/ — add a new pattern after each major investigation.
|
||||
---
|
||||
|
||||
# Higress Deep Troubleshooting
|
||||
|
||||
A methodology plus an accumulating pattern library for deep-diving into Higress gateway bugs that require code-level root cause analysis. The codebase spans two Higress-maintained forks — [`higress-group/envoy`](https://github.com/higress-group/envoy) (filter chain, Wasm `Context` integration) and [`higress-group/proxy-wasm-cpp-host`](https://github.com/higress-group/proxy-wasm-cpp-host) (Wasm VM host runtime, `WasmBase`, `ContextBase`) — and bugs often cross the boundary between them. This skill exists so that the next investigation doesn't start from scratch.
|
||||
|
||||
## When to use
|
||||
|
||||
Reach for this skill when ANY of these apply:
|
||||
|
||||
- A Higress issue reports a CPU spin, infinite loop, crash, or stage-mismatched behavior.
|
||||
- The bug involves the Wasm plugin lifecycle, `addAfterVmCallAction` / `doAfterVmCallActions`, `SaveRestoreContext`, `current_context_`, or the filter chain reentry paths.
|
||||
- You need to verify whether a one-repo fix also covers an upstream bug class (e.g., `proxy-wasm/proxy-wasm-cpp-host#326`).
|
||||
- You're ramping up on an incident and need a structured way to collect evidence before proposing a fix.
|
||||
- The maintainer is pushing back on a conclusion and you need to re-verify with code.
|
||||
|
||||
Do NOT reach for this skill for routine plugin development (`higress-wasm-go-plugin` covers that), config issues, or anything solvable by reading a single file without cross-repo context.
|
||||
|
||||
## Investigation methodology (brief)
|
||||
|
||||
1. **Symptom → code surface.** From perf data / repro steps / stack trace, identify which functions and files are involved. Note which fork each file lives in.
|
||||
2. **Parallel investigation.** Dispatch focused worker agents for independent aspects (envoy side, proxy-wasm-cpp-host side, upstream references). Each worker returns file:line evidence — never narration.
|
||||
3. **Code-trace verification.** Every claim must cite `file:line`. No "I think it does X" — only "see envoy context.cc:2085 where it calls ... synchronously".
|
||||
4. **Synthesize root cause.** Combine worker findings into a numbered call-chain trace, showing program state (queue contents, `current_context_` value, etc.) at each hop.
|
||||
5. **Fix design + side effects.** Propose a primitive, analyze what else it touches, check if it covers related bug classes upstream.
|
||||
6. **Verify when challenged.** If the maintainer pushes back, re-verify with code — don't restate the conclusion. The earlier analysis is often wrong in some detail that only a fresh code read catches.
|
||||
|
||||
Full process: [`references/investigation-methodology.md`](references/investigation-methodology.md). Verification patterns: [`references/code-trace-verification.md`](references/code-trace-verification.md).
|
||||
|
||||
## Accumulated patterns
|
||||
|
||||
Each major investigation that uses this skill should add a pattern under `patterns/`. Patterns are the "living" part of this skill — they capture both the technical root cause AND the meta-lessons (what tripped up the analysis, what the maintainer caught, what the fix primitive generalizes to).
|
||||
|
||||
Current patterns:
|
||||
|
||||
- [`patterns/2026-07-wasm-doaftervmcallactions-reentry.md`](patterns/2026-07-wasm-doaftervmcallactions-reentry.md) — #4034 CPU 100% from `onRedisCallFailure` + `injectEncodedDataToFilterChain` nested reentry. Root cause: unguarded `while(!empty())` drain in `WasmBase::doAfterVmCallActions`. Fix: drain-to-local via `std::deque::swap`. Meta-lesson: the same primitive covers the upstream #326 stage-mismatch class, which the first analysis round missed.
|
||||
|
||||
To add a new pattern after an investigation, see [`patterns/README.md`](patterns/README.md).
|
||||
|
||||
## Templates
|
||||
|
||||
- [`templates/investigation-report.md`](templates/investigation-report.md) — structure for a new investigation's findings.
|
||||
- [`templates/code-trace-table.md`](templates/code-trace-table.md) — step-by-step state-trace table format (very effective for communicating queue/state transitions to maintainers).
|
||||
|
||||
## Key code maps (quick orientation)
|
||||
|
||||
When investigating a Wasm-related Higress bug, the usual suspects:
|
||||
|
||||
| Concern | Repo | File |
|
||||
| --- | --- | --- |
|
||||
| Plugin callback entry points (`onRequestHeaders`, `onResponseBody`, `onHttpCallResponse`, etc.) | `higress-group/proxy-wasm-cpp-host` | `src/context.cc` |
|
||||
| Deferred-action queue (`addAfterVmCallAction`, `doAfterVmCallActions`, `DeferAfterCallActions`) | `higress-group/proxy-wasm-cpp-host` | `include/proxy-wasm/wasm.h`, `src/context.cc` |
|
||||
| `SaveRestoreContext` / `current_context_` | `higress-group/proxy-wasm-cpp-host` | `include/proxy-wasm/wasm_vm.h`, `src/exports.cc` |
|
||||
| Higress envoy `Context` (filter chain integration, foreign functions) | `higress-group/envoy` | `source/extensions/common/wasm/context.cc` |
|
||||
| Foreign-function registration (`proxy_inject_encoded_data_to_filter_chain`, etc.) | `higress-group/envoy` | `source/extensions/common/wasm/foreign.cc` |
|
||||
| envoy fork's pinned proxy-wasm-cpp-host commit | `higress-group/envoy` | `bazel/repository_locations.bzl` |
|
||||
|
||||
The Higress envoy fork pins a specific `proxy-wasm-cpp-host` commit via bazel — always check which fork commit is pinned before reasoning about `WasmBase` behavior, because the fork diverges from upstream (RedisCall, rebuild/recover, weak_handle additions).
|
||||
@@ -1,116 +0,0 @@
|
||||
# Wasm `doAfterVmCallActions` reentry — CPU-spin and stage-mismatch, fixed by drain-to-local
|
||||
|
||||
> Source: [#4034](https://github.com/higress-group/higress/issues/4034) — Wasm 插件死循环：onRedisCallFailure + injectEncodedDataToFilterChain 嵌套导致 Envoy CPU 100% 跑满
|
||||
> Upstream relative: [proxy-wasm/proxy-wasm-cpp-host#326](https://github.com/proxy-wasm/proxy-wasm-cpp-host/issues/326)
|
||||
> Date: 2026-07
|
||||
> Status: fix designed (SPEC-001), implementation pending
|
||||
|
||||
## Symptom
|
||||
|
||||
`release-2.2.2` Envoy pods CPU 100% on all 6 pods within ~3 minutes under concurrent load. Trigger: Redis unreachable + plugin calls `InjectEncodedDataToFilterChain` from an HTTP/Redis callback. perf:
|
||||
|
||||
| Overhead | Function |
|
||||
| --- | --- |
|
||||
| 39.21% | `Envoy::Extensions::Common::Wasm::Context::onRedisCallFailure` |
|
||||
| 25.41% | `proxy_wasm::WasmBase::doAfterVmCallActions` |
|
||||
| **合计** | **64.62%** |
|
||||
|
||||
## Root cause
|
||||
|
||||
`WasmBase::doAfterVmCallActions` (`higress-group/proxy-wasm-cpp-host` `include/proxy-wasm/wasm.h:158-169`) drains `after_vm_call_actions_` in an unguarded `while(!empty())` loop:
|
||||
|
||||
```cpp
|
||||
while (!self->after_vm_call_actions_.empty()) {
|
||||
auto f = std::move(self->after_vm_call_actions_.front());
|
||||
self->after_vm_call_actions_.pop_front();
|
||||
f(); // if f() re-adds an action, loop never terminates
|
||||
}
|
||||
```
|
||||
|
||||
Several Envoy-side callbacks re-defer themselves when `current_context_ != nullptr` (meaning: "VM is still on the stack, defer until it unwinds"):
|
||||
|
||||
- `Context::onRedisCallFailure` (envoy `context.cc:1273-1277`)
|
||||
- `Context::onRedisCallSuccess` (envoy `context.cc:1243-1249`)
|
||||
- `Context::onHttpCallSuccess` (envoy `context.cc:2352-2358`)
|
||||
- `Context::onHttpCallFailure` (envoy `context.cc:2382-2386`)
|
||||
|
||||
Under a synchronous reentry path — `injectEncodedDataToFilterChain` (envoy `context.cc:2085-2091`) is called from inside an HTTP/Redis callback — the outer `SaveRestoreContext` keeps `current_context_` non-null even after the inner `~SaveRestoreContext` unwinds. So the drain loop pops a self-reenqueueing callback, fires it, it sees `current_context_ != nullptr`, re-adds itself, loop iterates again → infinite → CPU 100%.
|
||||
|
||||
Full call chain (10 hops, each with file:line) is in [the #4034 proposal issue](https://github.com/higress-group/higress/issues/4064) body and SPEC-001.
|
||||
|
||||
## Fix
|
||||
|
||||
Swap the member queue to a local deque before iterating, so any action re-added during the drain lands in the (now-empty) member queue and is picked up by the next-outer `DeferAfterCallActions` frame — not re-fired within the same drain.
|
||||
|
||||
```cpp
|
||||
void doAfterVmCallActions() {
|
||||
if (!after_vm_call_actions_.empty()) {
|
||||
auto self = shared_from_this();
|
||||
std::deque<std::function<void()>> local;
|
||||
local.swap(self->after_vm_call_actions_);
|
||||
for (auto& f : local) f();
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
`std::deque::swap` is O(1) whole-content exchange. After swap: `local` has the pre-existing queue, `self->after_vm_call_actions_` is empty. The for-loop iterates `local`; any `addAfterVmCallAction` call during iteration appends to the empty member, invisible to this drain.
|
||||
|
||||
## Coverage
|
||||
|
||||
The same primitive covers BOTH:
|
||||
|
||||
- **#4034 (CPU-spin)**: re-added actions go to the empty member queue → picked up by next outer frame → loop terminates.
|
||||
- **#326 (stage-mismatch)**: pre-existing actions stay in `local` → inner drain (from a nested `~DeferAfterCallActions`) sees empty member → does nothing → pre-existing actions fire in the outer drain's correct phase.
|
||||
|
||||
The coverage of #326 is non-obvious — see Meta-lessons below.
|
||||
|
||||
## Evidence
|
||||
|
||||
| Claim | Citation |
|
||||
| --- | --- |
|
||||
| Unguarded drain loop | `higress-group/proxy-wasm-cpp-host` `include/proxy-wasm/wasm.h:158-169` |
|
||||
| Self-reenqueue guard `if (current_context_ != nullptr)` | `higress-group/envoy` `source/extensions/common/wasm/context.cc:1273-1277` (onRedisCallFailure); also lines 1243, 2352, 2382 |
|
||||
| Sync `injectEncodedDataToFilterChain` (the trigger) | `higress-group/envoy` `source/extensions/common/wasm/context.cc:2085-2091` |
|
||||
| Async sibling `injectEncodedDataToFilterChainOnHeader` (the contrast) | `higress-group/envoy` `source/extensions/common/wasm/context.cc:2093-2103` |
|
||||
| `sendLocalReply` is deferred via `addAfterVmCallAction` | `higress-group/envoy` `source/extensions/common/wasm/context.cc:1638-1657` |
|
||||
| `continueDecoding` is deferred via `addAfterVmCallAction` | `higress-group/envoy` `source/extensions/common/wasm/context.cc:1508-1514` |
|
||||
| `~DeferAfterCallActions` drains unconditionally | `higress-group/proxy-wasm-cpp-host` `src/context.cc` (top, `~DeferAfterCallActions` calls `wasm_->doAfterVmCallActions()`) |
|
||||
| `SaveRestoreContext` save/restore semantics | `higress-group/proxy-wasm-cpp-host` `include/proxy-wasm/wasm_vm.h:392-405` |
|
||||
| `current_context_` is thread_local | `higress-group/proxy-wasm-cpp-host` `src/exports.cc:26` |
|
||||
|
||||
## Meta-lessons
|
||||
|
||||
This investigation had three rounds of analysis. The meta-lessons are the most valuable part for future investigators.
|
||||
|
||||
### Lesson 1: a single primitive can cover two bug classes — check coverage explicitly
|
||||
|
||||
The first analysis round concluded "Layer A fixes #4034 but NOT #326" and proposed them as separate work items. That was wrong. The `swap-to-local` primitive covers BOTH because:
|
||||
|
||||
- For #4034: re-added actions go to the empty member.
|
||||
- For #326: pre-existing actions stay in `local`, invisible to the nested drain.
|
||||
|
||||
**Generalization**: whenever a fix changes how a queue/collection is iterated (swap-to-local, snapshot-then-iterate, copy-on-iterate), check whether it ALSO changes cross-frame or cross-stage visibility. These primitives often cover more than the primary bug.
|
||||
|
||||
### Lesson 2: model the PATCHED code, not the unpatched code
|
||||
|
||||
When analyzing whether a fix covers a related bug, it's easy to accidentally reason about the current (unpatched) behavior instead of the patched behavior. The first analysis round said "the inner drain sees `[continueDecoding]`" — which is true for the CURRENT code but FALSE for the patched code, because the patch's swap empties the member before the inner drain runs.
|
||||
|
||||
**Generalization**: when someone proposes a fix and asks "does this also cover X?", re-read the PATCHED code, not just the diff. The patch changes program state at moments you might not be tracking.
|
||||
|
||||
### Lesson 3: "X is deferred" vs "X is direct" is a load-bearing distinction
|
||||
|
||||
Several rounds of analysis hinged on whether `sendLocalReply` and `continueDecoding` are called directly or deferred via `addAfterVmCallAction`. Both are deferred — and that determines the queue order that produces the bug.
|
||||
|
||||
**Generalization**: when a function's name suggests "does X immediately" (like `sendLocalReply`), don't trust the name. Read the function body and check whether it actually calls X directly or wraps it in `addAfterVmCallAction` / `dispatcher().post(...)` / similar defer primitives. The defer-vs-direct distinction changes everything about reentrancy analysis.
|
||||
|
||||
### Lesson 4: maintainer pushback is a signal to re-verify, not restate
|
||||
|
||||
When the maintainer challenged the "Layer A doesn't fix #326" conclusion, the right move was to re-read the code with their specific objection in mind — not to restate the conclusion with different words. The re-read found that the swap-to-local empties the member queue, which the first analysis had missed.
|
||||
|
||||
**Generalization**: maintainer pushback is high-signal information. They know the codebase. If they say "I think X", dispatch a fresh worker to verify X with code evidence. Don't confirm your own previous answer; that's confirmation bias.
|
||||
|
||||
### Lesson 5: the #326 reproducer's plugin call order matters
|
||||
|
||||
The #326 reproducer calls `SendHttpResponse` THEN `ResumeHttpRequest`. If the order were reversed, the queue would be `[continueDecoding, sendLocalReply]`, FIFO drain would pop `continueDecoding` first (in the correct phase), then `sendLocalReply` (triggering encoder reentry with empty queue) — and there would be no stage mismatch even WITHOUT Layer A.
|
||||
|
||||
**Generalization**: when analyzing a reproducer, note the exact order of plugin calls. Stage-mismatch bugs often depend on call order, and a "harmless" reorder can mask the bug.
|
||||
@@ -1,71 +0,0 @@
|
||||
# Patterns — How to Add a New Investigation Case-Study
|
||||
|
||||
This directory accumulates case-studies from past deep investigations. Each pattern captures both the technical root cause AND the meta-lessons (what tripped up the analysis, what the maintainer caught, what the fix primitive generalizes to). The library grows by one entry per major investigation.
|
||||
|
||||
## When to add a pattern
|
||||
|
||||
Add a pattern after an investigation that:
|
||||
|
||||
- Required cross-repo code-trace evidence (envoy + proxy-wasm-cpp-host, or similar).
|
||||
- Resulted in a non-trivial fix (not just a one-line config change).
|
||||
- Has a meta-lesson worth carrying forward (a gotcha, a verification failure, a primitive that generalizes).
|
||||
|
||||
If the investigation was routine (single-file bug, obvious fix, no pushback), skip the pattern — the diff IS the documentation.
|
||||
|
||||
## File naming
|
||||
|
||||
`YYYY-MM-short-slug.md` where the date is when the investigation concluded. Examples:
|
||||
|
||||
- `2026-07-wasm-doaftervmcallactions-reentry.md`
|
||||
- `2026-09-filter-chain-headers-double-dispatch.md`
|
||||
- `2026-11-redis-pool-exhaustion-under-failover.md`
|
||||
|
||||
## Pattern structure
|
||||
|
||||
Use this template. Sections in `[brackets]` are required; sections in `<angle brackets>` are optional but encouraged.
|
||||
|
||||
```markdown
|
||||
# <Bug class one-line summary>
|
||||
|
||||
> Source: #ISSUE_NUMBER — <issue title>
|
||||
> Date: YYYY-MM
|
||||
> Status: <fixed / investigating / deferred>
|
||||
|
||||
## Symptom
|
||||
|
||||
<What observable behavior was wrong? Include perf data / repro steps if relevant.>
|
||||
|
||||
## Root cause
|
||||
|
||||
<One-paragraph explanation. Cite file:line for key claims.>
|
||||
|
||||
<Numbered call-chain trace, one hop per line, with file:line.>
|
||||
|
||||
## Fix
|
||||
|
||||
<The primitive. Code snippet if non-trivial.>
|
||||
|
||||
<Why it fixes the bug — the causal chain.>
|
||||
|
||||
## Coverage
|
||||
|
||||
<What bug classes does this fix cover? What does it NOT cover? Why?>
|
||||
|
||||
## Evidence
|
||||
|
||||
<File:line citations grouped by claim.>
|
||||
|
||||
## Meta-lessons
|
||||
|
||||
<What tripped up the first analysis round? What did the maintainer catch? What generalizes to other bugs in this area?>
|
||||
|
||||
<If the analysis was wrong initially and then corrected, document BOTH the wrong reasoning AND the correction — this is the most valuable part for future investigators.>
|
||||
```
|
||||
|
||||
## Worked example
|
||||
|
||||
See [`2026-07-wasm-doaftervmcallactions-reentry.md`](2026-07-wasm-doaftervmcallactions-reentry.md) for a complete pattern that follows this structure, including a meta-lesson section documenting an initial wrong conclusion that was corrected after maintainer pushback.
|
||||
|
||||
## Linking from new investigations
|
||||
|
||||
When a new investigation reuses a primitive or hits a similar gotcha to a past pattern, link to the pattern in the new investigation's report. Patterns are most valuable when they're cross-referenced — the second time you see a `std::deque::swap`-style fix, you should be able to find the #4034 pattern and reason by analogy.
|
||||
@@ -1,73 +0,0 @@
|
||||
# Code-Trace Verification
|
||||
|
||||
The format and rules for verifying claims with code evidence during a deep investigation. Maintainers trust analysis they can audit; file:line citations are the audit trail.
|
||||
|
||||
## Core rule
|
||||
|
||||
**Every claim must cite `file:line`.** If you can't cite it, you haven't read it; if you haven't read it, you don't know.
|
||||
|
||||
## Acceptable evidence
|
||||
|
||||
| Claim type | What to cite |
|
||||
| --- | --- |
|
||||
| "Function X does Y" | `repo/path/file.cc:NN` + ≤5-line excerpt |
|
||||
| "Function X is synchronous" | The call site that lacks `dispatcher().post(...)` (or equivalent) + the sibling function that DOES use it, as contrast |
|
||||
| "The queue state is `[a, b]` at this point" | The sequence of `push_back` / `pop_front` calls leading to that state, each with file:line |
|
||||
| "The maintainer's claim is wrong" | Re-state their claim, then cite the specific code that contradicts it |
|
||||
| "This fix covers bug class Y too" | The mechanism by which it covers Y, with the call chain showing why |
|
||||
|
||||
## Unacceptable evidence
|
||||
|
||||
- "I think" / "probably" / "should be" — read the code.
|
||||
- "Similar to upstream" — check the fork commit pin in `bazel/repository_locations.bzl`.
|
||||
- "The function name implies X" — names lie. `injectEncodedDataToFilterChain` sounds innocuous; it's the trigger for a CPU-spin bug.
|
||||
- Reasoning from the bug report without code verification — the report describes symptoms.
|
||||
|
||||
## State-trace table format
|
||||
|
||||
The most effective format for communicating queue/state transitions. Use it whenever the bug involves a sequence of state changes (queue operations, `current_context_` saves/restores, reference-counting transitions).
|
||||
|
||||
Template:
|
||||
|
||||
| Step | Action | `<state variable 1>` | `<state variable 2>` |
|
||||
| --- | --- | --- | --- |
|
||||
| 1 | Enter context | `[]` | `nullptr` |
|
||||
| 2 | Operation X | `[x]` | `nullptr` |
|
||||
| 3 | Operation Y | `[x, y]` | `ctx` |
|
||||
| ... | ... | ... | ... |
|
||||
|
||||
Rules:
|
||||
|
||||
- One row per discrete state change.
|
||||
- State columns should be the variables that matter for the bug (queue contents, `current_context_`, depth counter, etc.).
|
||||
- Mark the critical transition with ★ and explain it below the table.
|
||||
- If a state is "unknown" or "depends on plugin behavior", say so — don't fake precision.
|
||||
|
||||
## Worked example (from #4034)
|
||||
|
||||
The #4034 investigation hinged on the queue state at the moment of a nested drain. The state-trace table that settled the disagreement:
|
||||
|
||||
| Step | Action | `self->after_vm_call_actions_` | outer drain's `local` |
|
||||
| --- | --- | --- | --- |
|
||||
| 1 | Enter `onHttpCallResponse` | `[]` | — |
|
||||
| 2 | Plugin calls `SendHttpResponse` | `[sendLocalReply]` | — |
|
||||
| 3 | Plugin calls `ResumeHttpRequest` | `[sendLocalReply, continueDecoding]` | — |
|
||||
| 4 | Outer `~DeferAfterCallActions` fires | same | — |
|
||||
| 5 | **Layer A: `local.swap(after_vm_call_actions_)`** ★ | `[]` | `[sendLocalReply, continueDecoding]` |
|
||||
| 6 | iter 1: fire `sendLocalReply` → `encodeHeaders` → `onResponseHeaders` | `[]` | same |
|
||||
| 7 | Inner `~DeferAfterCallActions` fires | `[]` (empty → no-op) | same |
|
||||
| 8 | iter 2: fire `continueDecoding` in outer context | `[]` | same |
|
||||
|
||||
The ★ at step 5 is the key insight: `std::deque::swap` empties the member, so the inner drain at step 7 sees an empty queue and does nothing. The earlier (wrong) analysis confused "queue before swap" with "queue at inner-drain time".
|
||||
|
||||
## Re-verification protocol (when the maintainer pushes back)
|
||||
|
||||
When a maintainer disagrees with your conclusion:
|
||||
|
||||
1. **Don't restate the conclusion.** Restating without new evidence wastes their time.
|
||||
2. **Identify the specific point of disagreement.** What exactly do they claim?
|
||||
3. **Dispatch a fresh worker** with the specific objection. The worker brief should say: "Maintainer claims X. Verify whether X is true. Cite file:line."
|
||||
4. **If the worker confirms the maintainer**: acknowledge the error publicly, explain what you missed, update artifacts.
|
||||
5. **If the worker refutes the maintainer**: respond with the specific code evidence, walk through the state-trace table, be respectful.
|
||||
|
||||
The third verification (yours) doesn't count if it just restates the second (also yours). You need to actually re-read the code with the maintainer's objection in mind, not just re-derive your previous answer.
|
||||
@@ -1,118 +0,0 @@
|
||||
# Investigation Methodology
|
||||
|
||||
The step-by-step process for deep-diving a Higress bug. This is the canonical workflow; adapt it to the bug at hand but don't skip the evidence-collection steps.
|
||||
|
||||
## 0. Before you start: scope the investigation
|
||||
|
||||
Confirm the bug is real and worth a deep dive. Signs it's worth it:
|
||||
|
||||
- Reproducer exists (even if intermittent).
|
||||
- perf data or stack trace points at specific functions.
|
||||
- The fix likely requires code changes (not config/docs).
|
||||
- The blast radius is significant (production-affecting, multiple users).
|
||||
|
||||
If the bug is a one-off without a reproducer, or clearly a user-config issue, don't burn a deep investigation on it.
|
||||
|
||||
## 1. Symptom → code surface
|
||||
|
||||
Read the bug report and extract:
|
||||
|
||||
- **Symptom**: what observable behavior is wrong? (CPU spin, crash, wrong response, hang.)
|
||||
- **Trigger conditions**: what's the minimal repro path?
|
||||
- **perf / stack data**: which functions dominate?
|
||||
|
||||
Then map symptoms to code surfaces. For Wasm-related bugs, the surface is almost always split across:
|
||||
|
||||
- `higress-group/envoy` `source/extensions/common/wasm/context.cc` — Higress-specific `Context` subclass, foreign functions like `injectEncodedDataToFilterChain`.
|
||||
- `higress-group/proxy-wasm-cpp-host` `include/proxy-wasm/wasm.h` + `src/context.cc` — VM host runtime, `WasmBase`, `ContextBase`, deferred-action queue.
|
||||
|
||||
**Always check the envoy fork's `bazel/repository_locations.bzl` for the pinned `proxy-wasm-cpp-host` commit.** The fork diverges from upstream; reasoning about upstream code without checking the pin leads to wrong conclusions.
|
||||
|
||||
## 2. Parallel investigation (worker dispatch)
|
||||
|
||||
For non-trivial bugs, dispatch focused worker agents IN PARALLEL for independent aspects. Each worker gets a self-contained brief (it has no shared context with you). Example dispatch for a Wasm reentrancy bug:
|
||||
|
||||
- **Worker A (envoy side)**: verify the trigger path in `higress-group/envoy`. Find the synchronous call, the self-reenqueueing callbacks, the foreign-function registration. Return file:line evidence.
|
||||
- **Worker B (proxy-wasm-cpp-host side)**: verify the engine. Find the queue, the drain loop, the `SaveRestoreContext` semantics. Return file:line evidence.
|
||||
- **Worker C (upstream references)**: fetch any linked upstream issues/PRs, determine if they're related/resolved/portable.
|
||||
|
||||
Worker rules:
|
||||
|
||||
- Each worker is **research-only** unless you explicitly authorize code changes. Don't let workers edit code on their own initiative.
|
||||
- Each worker must **cite file:line for every claim**. Reject worker reports that say "the function does X" without a citation.
|
||||
- Cap report length (e.g., 600-800 words) to keep synthesis tractable.
|
||||
- Use `general-purpose` agent type for investigations that need to clone repos and reason across files; use `Explore` for narrow single-repo lookups.
|
||||
|
||||
## 3. Code-trace verification
|
||||
|
||||
Every claim in the final analysis must reduce to a file:line citation. The output format that works best for maintainer communication is the **step-by-step state-trace table**:
|
||||
|
||||
| Step | Action | Member queue state | Local queue state |
|
||||
| --- | --- | --- | --- |
|
||||
| 1 | Enter callback | `[]` | — |
|
||||
| 2 | Plugin calls X | `[x]` | — |
|
||||
| ... | ... | ... | ... |
|
||||
|
||||
See [`code-trace-verification.md`](code-trace-verification.md) for the full format and a worked example.
|
||||
|
||||
Anti-patterns to avoid:
|
||||
|
||||
- **"I think the function does X"** — unacceptable. Read the code and cite the line.
|
||||
- **"This is probably similar to upstream"** — check the fork commit pin.
|
||||
- **"The drain loop terminates because of course it does"** — verify the actual loop condition.
|
||||
- **Reasoning from the bug report without reading the code** — the report describes symptoms, not causes.
|
||||
|
||||
## 4. Synthesize root cause
|
||||
|
||||
Combine worker findings into a numbered call-chain trace. For each hop:
|
||||
|
||||
- The function name.
|
||||
- The file:line.
|
||||
- What state it reads/writes (e.g., `current_context_` value, queue contents).
|
||||
|
||||
End with a one-sentence root cause: "X happens because Y, which is unguarded because Z."
|
||||
|
||||
## 5. Fix design + side effects
|
||||
|
||||
Propose a fix primitive. For each candidate:
|
||||
|
||||
- **What it changes**: the specific code transformation.
|
||||
- **Why it fixes the bug**: the causal chain.
|
||||
- **Side effects**: what else it touches. Be honest — even a 5-line fix can have ABI or lifecycle implications.
|
||||
- **Coverage**: does it also cover related bug classes? (E.g., drain-to-local covers both CPU-spin and stage-mismatch.)
|
||||
|
||||
If a fix has unacceptable side effects, document it as "deferred" and explain why. Don't sneak a behavior-change through as a "bug fix".
|
||||
|
||||
## 6. Cross-check related bug classes
|
||||
|
||||
Before declaring done, check: does the fix also cover bug classes that share the same machinery?
|
||||
|
||||
- Look at upstream issues in `proxy-wasm/proxy-wasm-cpp-host` that reference the same functions.
|
||||
- Look at the Higress fork's divergence from upstream — are there Higress-specific code paths that have the same bug?
|
||||
- Look at the `addAfterVmCallAction` callsites in envoy — are there other self-reenqueueing or cross-stage patterns?
|
||||
|
||||
Document the coverage explicitly: "Fixes X. Does NOT fix Y because Z. Y tracked separately as ..."
|
||||
|
||||
## 7. Verify when challenged
|
||||
|
||||
If the maintainer pushes back on a conclusion, **do not just restate the conclusion**. Re-read the code with their specific objection in mind.
|
||||
|
||||
Common failure modes in the first analysis round:
|
||||
|
||||
- **Confusing "state before operation" with "state after operation"**. (E.g., "the queue is `[X]`" — but is that before or after the swap?)
|
||||
- **Assuming a function is sync/async without checking**. (E.g., assuming `sendLocalReply` is direct when it's actually deferred via `addAfterVmCallAction`.)
|
||||
- **Reasoning about the patched code as if it were the unpatched code**. (E.g., analyzing Layer A's `doAfterVmCallActions` as if it still iterates the member queue in place, when actually it swaps to a local first.)
|
||||
- **Assuming plugin call order**. (E.g., assuming `ResumeHttpRequest` is called before `SendHttpResponse` — verify from the reproducer.)
|
||||
|
||||
When you re-verify, dispatch a fresh worker with the specific objection. The worker should be told: "the maintainer claims X; verify whether X is true with code evidence." This avoids confirmation bias.
|
||||
|
||||
## 8. Document the pattern
|
||||
|
||||
After the investigation concludes, add a pattern file to `patterns/` capturing:
|
||||
|
||||
- The bug class (one-line summary).
|
||||
- The root cause (with file:line evidence).
|
||||
- The fix primitive (with the code snippet).
|
||||
- The meta-lesson (what tripped up the analysis, what the maintainer caught, what generalizes).
|
||||
|
||||
This is what makes the skill accumulate value over time. See [`patterns/README.md`](../patterns/README.md) for the format.
|
||||
@@ -1,64 +0,0 @@
|
||||
# Code-Trace Table Template
|
||||
|
||||
Use this template for the state-trace table that communicates queue/state transitions to maintainers. This format was developed during the #4034 investigation and proved effective for settling disagreements about program state at specific moments.
|
||||
|
||||
## When to use
|
||||
|
||||
- The bug involves a sequence of state changes (queue operations, `current_context_` saves/restores, reference-count transitions).
|
||||
- You need to communicate "what the program state is at moment X" to someone who wasn't in your head.
|
||||
- A maintainer is challenging a conclusion about state at a specific point in the call chain.
|
||||
|
||||
## Template
|
||||
|
||||
```markdown
|
||||
| Step | Action | `<state variable 1>` | `<state variable 2>` | ... |
|
||||
| --- | --- | --- | --- | ... |
|
||||
| 1 | <entry point> | `<initial value>` | `<initial value>` | ... |
|
||||
| 2 | <operation> | `<value after>` | `<value after>` | ... |
|
||||
| 3 | <operation> | `<value after>` | `<value after>` | ... |
|
||||
| ... | ... | ... | ... | ... |
|
||||
| N ★ | <critical transition> | `<value>` | `<value>` | ... |
|
||||
```
|
||||
|
||||
## Rules
|
||||
|
||||
1. **One row per discrete state change.** Don't combine multiple operations into one row.
|
||||
2. **State columns are the variables that matter for the bug.** Common ones for Wasm bugs:
|
||||
- `self->after_vm_call_actions_` (the deferred-action queue)
|
||||
- `current_context_` (the thread_local ContextBase*)
|
||||
- `SaveRestoreContext` depth (how many nested VM frames)
|
||||
- `local` (the outer drain's swap target, post-Layer-A)
|
||||
3. **Mark the critical transition with ★** and explain it below the table. This is the moment where the bug happens (or doesn't).
|
||||
4. **Show the queue contents explicitly** — `[sendLocalReply, continueDecoding]`, not "non-empty". Order matters for FIFO drains.
|
||||
5. **If state is unknown / depends on plugin behavior, say so.** Don't fake precision. Better: "depends on whether the plugin called X before Y; both branches shown below".
|
||||
6. **Pair the table with a prose explanation of the ★ transition.** The table shows WHAT; the prose explains WHY.
|
||||
|
||||
## Worked example
|
||||
|
||||
From #4034 — the table that settled whether Layer A's swap-to-local covers #326:
|
||||
|
||||
```markdown
|
||||
| Step | Action | `self->after_vm_call_actions_` | outer drain's `local` |
|
||||
| --- | --- | --- | --- |
|
||||
| 1 | Enter `onHttpCallResponse` | `[]` | — |
|
||||
| 2 | Plugin calls `SendHttpResponse` | `[sendLocalReply]` | — |
|
||||
| 3 | Plugin calls `ResumeHttpRequest` | `[sendLocalReply, continueDecoding]` | — |
|
||||
| 4 | Outer `~DeferAfterCallActions` fires | `[sendLocalReply, continueDecoding]` | — |
|
||||
| 5 ★ | **Layer A: `local.swap(after_vm_call_actions_)`** | `[]` | `[sendLocalReply, continueDecoding]` |
|
||||
| 6 | iter 1: fire `sendLocalReply` → `encodeHeaders` → `onResponseHeaders` | `[]` | `[sendLocalReply, continueDecoding]` |
|
||||
| 7 | Inner `~DeferAfterCallActions` fires | `[]` (empty → no-op) | `[sendLocalReply, continueDecoding]` |
|
||||
| 8 | iter 2: fire `continueDecoding` in outer context | `[]` | (iterating) |
|
||||
```
|
||||
|
||||
**Why the ★ matters**: `std::deque::swap` is O(1) whole-content exchange. After step 5, the member queue is empty — it's NOT `[continueDecoding]` anymore. So when step 7's inner drain checks `if (!after_vm_call_actions_.empty())`, it sees `false` and returns. `continueDecoding` stays in the outer drain's `local` and fires in step 8, in the outer drain's context (correct phase).
|
||||
|
||||
The earlier (wrong) analysis had assumed the member queue still contained `continueDecoding` at step 7 — confusing "queue before swap" with "queue after swap".
|
||||
|
||||
## Anti-patterns
|
||||
|
||||
Don't do these:
|
||||
|
||||
- **Combining steps**: "steps 5-8: drain happens, continueDecoding fires" — too coarse; the reader can't see the state at each moment.
|
||||
- **Hiding the critical transition**: burying the ★ in a paragraph of prose. The table IS the argument; make it sharp.
|
||||
- **Vague state**: "queue has some items" — which items? In what order? FIFO drains care about order.
|
||||
- **No prose explanation**: the table alone isn't enough; pair it with 2-3 sentences explaining the ★.
|
||||
@@ -1,109 +0,0 @@
|
||||
# Investigation Report Template
|
||||
|
||||
Use this structure when starting a new deep investigation. Fill in the brackets; remove sections that don't apply. The goal is a report that a maintainer can audit end-to-end: every claim has a citation, every fix has a side-effect analysis.
|
||||
|
||||
---
|
||||
|
||||
# Investigation: <one-line bug summary>
|
||||
|
||||
> Source: #ISSUE_NUMBER
|
||||
> Investigator: <name / agent session>
|
||||
> Date: YYYY-MM-DD
|
||||
> Status: <in-progress / root-caused / fix-designed / fixed>
|
||||
|
||||
## Symptom
|
||||
|
||||
<What observable behavior is wrong? Include:>
|
||||
|
||||
- **Observable**: <what the user sees — CPU spin, crash, wrong response, hang>
|
||||
- **Trigger conditions**: <minimal repro path>
|
||||
- **perf / stack data**: <paste perf sampling, stack trace, or relevant telemetry>
|
||||
- **Blast radius**: <which versions, which components, how many users>
|
||||
|
||||
## Code surfaces
|
||||
|
||||
<Which repos and files are involved? Use a table:>
|
||||
|
||||
| Concern | Repo | File | Why it matters |
|
||||
| --- | --- | --- | --- |
|
||||
| <concern> | <repo URL> | <path:line> | <one-line role> |
|
||||
|
||||
<For Wasm bugs, always check the envoy fork's `bazel/repository_locations.bzl` for the pinned proxy-wasm-cpp-host commit. Note it here.>
|
||||
|
||||
## Worker dispatch
|
||||
|
||||
<If you dispatched parallel workers, list them with their brief and key finding:>
|
||||
|
||||
- **Worker A (<repo>) — <one-line brief>**: <VERDICT — one-line finding>. Key evidence: <file:line>.
|
||||
- **Worker B (<repo>) — <one-line brief>**: ...
|
||||
|
||||
<If the investigation was single-threaded (no workers), say so and explain why.>
|
||||
|
||||
## Root cause
|
||||
|
||||
<One-paragraph explanation. Be specific about the mechanism.>
|
||||
|
||||
<Numbered call-chain trace, one hop per line:>
|
||||
|
||||
1. <Function> (`repo file.cc:NN`) — <what it does>
|
||||
2. <Function> (`repo file.cc:NN`) — <what it does>
|
||||
3. ...
|
||||
|
||||
<For queue/state bugs, include a state-trace table here. See `code-trace-verification.md` for the format.>
|
||||
|
||||
**Root cause (one sentence)**: <X happens because Y, which is unguarded because Z>.
|
||||
|
||||
## Fix
|
||||
|
||||
<The primitive. Code snippet if non-trivial.>
|
||||
|
||||
```cpp
|
||||
// Before
|
||||
<current code>
|
||||
|
||||
// After
|
||||
<fixed code>
|
||||
```
|
||||
|
||||
**Why it fixes the bug**: <causal chain, 2-3 sentences.>
|
||||
|
||||
**Side effects**: <what else the change touches. Be honest — even a 5-line fix can have ABI or lifecycle implications. If there are none, say "none identified" and explain the verification.>
|
||||
|
||||
## Coverage analysis
|
||||
|
||||
<Does the fix cover related bug classes? Check upstream issues, related callbacks, similar patterns.>
|
||||
|
||||
| Bug class | Covered? | Why |
|
||||
| --- | --- | --- |
|
||||
| <primary bug> | yes | <mechanism> |
|
||||
| <related class 1> | yes/no | <mechanism or reason> |
|
||||
| <related class 2> | yes/no | ... |
|
||||
|
||||
<For each "no", note where it's tracked (separate issue, deferred SPEC, etc.).>
|
||||
|
||||
## Evidence index
|
||||
|
||||
<All file:line citations grouped by claim. This is the audit trail.>
|
||||
|
||||
| Claim | Citation |
|
||||
| --- | --- |
|
||||
| <claim> | `repo/path/file.cc:NN` |
|
||||
| ... | ... |
|
||||
|
||||
## Open questions
|
||||
|
||||
<Any unresolved questions for the maintainer. Each should be specific and actionable.>
|
||||
|
||||
- **Q1**: <question>
|
||||
- **Q2**: <question>
|
||||
|
||||
## Next steps
|
||||
|
||||
<What happens next. Be concrete:>
|
||||
|
||||
- [ ] <action 1>
|
||||
- [ ] <action 2>
|
||||
|
||||
---
|
||||
|
||||
<After the investigation concludes, distill the meta-lessons and add a pattern file to `patterns/`. See `patterns/README.md` for the format.>
|
||||
Reference in New Issue
Block a user