fix(l7): keep decoding HTTP/2 headers after the HPACK table falls out of sync

[mayankpande88]

Summary

The HTTP/2 parser decoded header blocks with hpack.Decoder and replaced it with a fresh decoder after any error. That never recovers once the decoder's dynamic table falls out of sync with the peer's, which happens whenever the agent joins a connection after it opened or loses a header block.

The peer keeps referencing entries the decoder never saw. hpack.Decoder stops at the first such reference, so that block's later insertions are never applied, the next block fails the same way, and the reset changes nothing. On a test cluster this ran at about 2.5 decode errors per HTTP/2 stream. Everything the table supplies is lost:

• :path and :authority of requests, so spans have no path and destinations are not named from :authority.

• grpc-status, so a failed gRPC call is reported with its HTTP status.

• Any :status outside the static table (201, 401, 403, 503, …).

• New decoder: hpackDecoder skips references to entries it doesn't hold and applies every insertion. HPACK indices count back from the newest entry, so everything inserted since the decoder lost track is at the index the peer uses. The decoder converges on the peer's table as old entries are evicted.

• Known loss resets the table: missing a block's insertions would shift older indices, so the table is reset wherever a header block is known to be lost:

  • a HEADERS or CONTINUATION frame cut by truncation;
  • a pending header block dropped;
  • a CONTINUATION with no HEADERS before it;
  • pseudo-headers impossible for the direction, such as :method in a response.

• Metrics: unresolved references are counted as a new hpack_partial stage of node_agent_http2_stage_total. node_agent_hpack_decode_errors_total now counts only blocks that are not valid HPACK or decode to implausible headers.

• Larger tables: dynamic table size updates above 4096 are accepted. hpack.Decoder(4096) rejected them, which broke peers that agreed on a larger SETTINGS_HEADER_TABLE_SIZE.

• Size updates in later blocks: Go's HTTP/2 client starts a header block with a dynamic table size update once it has the server's SETTINGS, a few requests into every connection. hpack.Decoder, fed with Write and never Close as the parser must, doesn't know where a block starts, and rejected the update with "dynamic table size update MUST occur at the beginning of a header block". So every Go client connection fell into the cascade within its first few requests. In a local capture of a Go client, that error was the first one, and it was followed by one error per request.

Stacked on #356.

Engineering detail

• Correctness: with the whole connection seen, the decoder emits exactly what hpack.Decoder does over 2,000 random blocks, covering all indexing modes, Huffman coding, evictions and size updates.
• Mid-stream join: joining after request 20, every static-named header decodes from request 63 on, and nothing decodes to a wrong value. The reset-on-error decoder fails all 380 blocks.
• Lost block: without the reset on a truncated HEADERS frame, a later request decodes the wrong :path. The parser test fails if that reset is removed.
• Custom header names: a name like x-request-id is copied from the newest entry with the same name. If that chain started before the join, the name stays unknown. Static-table names (all pseudo-headers, content-type, …) are unaffected.
• Speed: about 1.6× faster than hpack.Decoder with fewer allocations (BenchmarkHpackDecoder).
• Not covered: a Go net/http client joined mid-stream isn't detected as HTTP/2 at all, because it sends :authority first. The kernel heuristic needs a static-table byte. This PR doesn't change that.

Local e2e: built agent images from this branch and its parent and ran each in a local Docker VM (kernel 6.10), started after a gRPC-ordered HTTP/2 client was already connected, so the agent joins mid-stream. Two 90s runs each, ~30k requests per run. Parent: 59,379 and 59,606 HPACK decode errors (two per request), requests completed with :path in 3 of 29 logged samples. This branch: 0 decode errors, the unresolved references counted as hpack_partial, :path in 11 of 17 logged samples, the same number of requests completed. Unit and parser tests, go vet and golangci-lint pass.

[gemini-code-assist]

Code Review

This pull request introduces a custom hpackDecoder to improve the robustness of HTTP/2 header parsing, specifically handling mid-stream joins and lost header blocks by gracefully resetting or skipping problematic entries rather than failing the entire connection. The review feedback identifies several critical memory leak risks related to slice management and suggests improvements for Go idiomatic usage and portability across architectures.

[gemini-code-assist]

In Go, when elements are removed from a slice by reslicing (e.g., d.dynamic = append(d.dynamic[:0], d.dynamic[n:]...)), the elements past the new length of the slice but within its capacity still hold references to their values in the underlying array. Since hpackEntry contains string fields (Name and Value), these strings (and their underlying byte arrays) will not be garbage collected as long as they are referenced by the unused capacity of the d.dynamic slice. This can lead to a significant memory leak over time on long-lived connections. To prevent this, we should use copy to shift the elements, explicitly zero out the obsolete entries at the end of the slice to release the references, and then reslice.

func (d *hpackDecoder) evict(room uint32) {
	n := 0
	for d.size+room > d.maxSize && n < len(d.dynamic) {
		f := d.dynamic[n]
		d.size -= uint32(len(f.Name)+len(f.Value)) + hpackEntryOverhead
		n++
	}
	if n > 0 {
		copy(d.dynamic, d.dynamic[n:])
		for i := len(d.dynamic) - n; i < len(d.dynamic); i++ {
			d.dynamic[i] = hpackEntry{}
		}
		d.dynamic = d.dynamic[:len(d.dynamic)-n]
	}
}

[mayankpande88]

Fixed in 05641fc: evicted, reset and emptied entries are cleared before the slice is shortened. Retention was bounded by the table's capacity (at most ~128 entries per direction, overwritten as the table refills), but there is no reason to keep them.

[gemini-code-assist]

Similar to the eviction logic, resetting the decoder by reslicing d.dynamic = d.dynamic[:0] keeps all existing hpackEntry elements in the underlying array's capacity. This prevents the garbage collector from reclaiming the string memory of the cleared dynamic table. We should zero out the elements of d.dynamic before reslicing to release these references.

Suggested change

	func (d *hpackDecoder) reset() {
	d.dynamic = d.dynamic[:0]
	d.size = 0
	d.maxSize = hpackDefaultTableSize
	}
	func (d *hpackDecoder) reset() {
	for i := range d.dynamic {
	d.dynamic[i] = hpackEntry{}
	}
	d.dynamic = d.dynamic[:0]
	d.size = 0
	d.maxSize = hpackDefaultTableSize
	}

[mayankpande88]

Fixed in 05641fc: evicted, reset and emptied entries are cleared before the slice is shortened. Retention was bounded by the table's capacity (at most ~128 entries per direction, overwritten as the table refills), but there is no reason to keep them.

[gemini-code-assist]

Using uint64 as a slice index can be problematic on 32-bit architectures and is less idiomatic in Go. Since len(d.dynamic) is an int, we can convert the index offset i to int and perform the slice lookup using standard int types.

Suggested change

	func (d *hpackDecoder) field(idx uint64) (hpackEntry, bool) {
	if idx <= uint64(len(hpackStaticTable)) {
	return hpackEntry{HeaderField: hpackStaticTable[idx-1]}, true
	}
	i := idx - uint64(len(hpackStaticTable)) // 1 = newest
	if i > uint64(len(d.dynamic)) {
	return hpackEntry{}, false
	}
	return d.dynamic[uint64(len(d.dynamic))-i], true
	}
	func (d *hpackDecoder) field(idx uint64) (hpackEntry, bool) {
	if idx <= uint64(len(hpackStaticTable)) {
	return hpackEntry{HeaderField: hpackStaticTable[idx-1]}, true
	}
	i := int(idx - uint64(len(hpackStaticTable))) // 1 = newest
	if i > len(d.dynamic) {
	return hpackEntry{}, false
	}
	return d.dynamic[len(d.dynamic)-i], true
	}

[mayankpande88]

Changed to int indexing in 05641fc, after the bounds checks. For the record, slicing with a uint64 index compiles on 32-bit targets too: Go accepts any integer type as an index and checks the value at run time.

[gemini-code-assist]

When an entry larger than the table is inserted, the dynamic table is emptied. Reslicing d.dynamic = d.dynamic[:0] without zeroing the elements keeps references to the strings in the underlying array, causing a memory leak. We should zero out the elements before reslicing.

func (d *hpackDecoder) insert(f hpackEntry) {
	size := uint32(len(f.Name)+len(f.Value)) + hpackEntryOverhead
	if size > d.maxSize {
		// RFC 7541 4.4: an entry larger than the table empties it.
		for i := range d.dynamic {
			d.dynamic[i] = hpackEntry{}
		}
		d.dynamic = d.dynamic[:0]
		d.size = 0
		return
	}
	d.evict(size)
	d.dynamic = append(d.dynamic, f)
	d.size += size
}

[mayankpande88]

Fixed in 05641fc: evicted, reset and emptied entries are cleared before the slice is shortened. Retention was bounded by the table's capacity (at most ~128 entries per direction, overwritten as the table refills), but there is no reason to keep them.

[gemini-code-assist]

Using a uint64 variable (n) directly as a slice index (b[:n] and b[n:]) can cause compilation failures on 32-bit architectures where int is 32-bit. It is safer and more portable to explicitly convert n to int when slicing, especially since we have already verified that n <= uint64(len(b)).

func hpackString(b []byte) (string, []byte, error) {
	if len(b) == 0 {
		return "", b, nil
	}
	// ...
	return string(b[:int(n)]), b[int(n):], nil
}

[mayankpande88]

Changed to int indexing in 05641fc, after the bounds checks. For the record, slicing with a uint64 index compiles on 32-bit targets too: Go accepts any integer type as an index and checks the value at run time.

[mayankpande88]

Folded into #364 with the rest of this stack, so it can be reviewed and merged as one unit (squash-only merges made each stacked merge conflict). The commit is unchanged there; its description and review thread here stay as the detailed write-up.