Implement SCYLLA_USE_METADATA_ID protocol extention negotiat…

[nikagra]

Issue gocql#527

Implements driver-side negotiation and use of Scylla's SCYLLA_USE_METADATA_ID protocol extension so that prepared-statement result-metadata invalidation works correctly (see scylladb/scylladb#20860, fixed server-side by scylladb/scylladb#23292).

When the extension is negotiated, on prepare the server hands out a result-metadata id; on execute the driver sends that id back, and the server responds with METADATA_CHANGED + a new id + fresh metadata whenever the cached metadata is stale. The driver updates its prepared-statement cache before deserializing, fixing outdated-metadata errors after schema changes (e.g. ALTER TABLE).

What it does

• Negotiate SCYLLA_USE_METADATA_ID: parse it from the SUPPORTED frame and send it in STARTUP (scyllaUseMetadataIDExt implementing cqlProtocolExtension, registered in parseCQLProtocolExtensions).
• Thread the negotiated flag through frame.go/framer.go/conn.go, broadening the existing v5 metadata-id read/write gates from proto > v4 to proto > v4 || scyllaUseMetadataID — no duplicated primitives; the v5 read/write paths are reused.
• Track result-metadata ids for prepared statements and handle the METADATA_CHANGED flag, updating the cached prepared statement.
• Skip result metadata once a metadata id is in hand, on native v5 as well as on v4 with the extension (see Behaviour changes).

v5 is not auto-negotiated — discoverProtocol() caps at protoVersion4. Against Scylla the capability is reached via this extension on v4; native v5 is only reachable with an explicit ProtoVersion: 5.

Behaviour changes

Skip-metadata becomes the effective default wherever a result metadata id is exchanged.

NewCluster() sets DisableSkipMetadata: true, so today gocql never sends skip_metadata. That default is not upstream's — apache/cassandra-gocql-driver leaves it false and skips on every protocol version. It was flipped in f292aaf ("Disable skipping metadata by default", Ref: scylladb/scylladb#20860) precisely because prepared-statement metadata could not be invalidated safely. This PR retires that workaround: where the driver holds a result metadata id, the server reports staleness via METADATA_CHANGED, skipping is safe, and DisableSkipMetadata is ignored — including when it was set to true explicitly.

This follows the rule stated in the parent task, scylla-drivers#81:

Default value of skip metadata flag should be treated as safest option. Which means that if SCYLLA_USE_METADATA_ID was negotiated or CQL v5 is used then it is true, result metadata is skipped.

Conn.tracksResultMetadataID() reports either mechanism. Scoping the override to the extension alone would leave gocql with opposite defaults for two encodings of the same mechanism, and the losing one would be the one where the id is guaranteed by the protocol rather than negotiated — since #593 landed native v5, a ProtoVersion: 5 session would carry full result metadata on every response for no reason.

The sibling drivers split on this, which is worth recording:

driver	knob	behaviour once a metadata id is in play
python-driver#770	none	skip_meta = bool(result_metadata) and result_metadata_id is not None; _should_skip_metadata() emits the flag only when the connection negotiated the extension, deliberately not on native v5 ("a separate behavior change out of scope")
java-driver#599 + #758 (4.x), #663 + #757 (3.x)	advanced.prepared-statements.skip-cql4-metadata-resolve-method (smart|enabled|disabled)	DefaultPreparedStatement.resolveSkipMetadata() returns true before ever reading the option once resultMetadataId is non-empty — which native v5 always supplies, so java skips there too

gocql follows #81 and java-driver here rather than python-driver. Consequences:

• Per-query Query.NoSkipMetadata() still forces metadata, and is now the only way to do so — there is deliberately no gocql equivalent of java-driver's session-level disabled. ScanCAS/MapScanCAS set it internally, so conditional statements are unaffected either way; and in practice a prepared conditional statement's result metadata is empty, so the column-set gate below already prevents skipping for them.
• The Validate() warning from 5e07ffd fires on !DisableSkipMetadata, so after this change the population that actually gets skipping (default config) is unwarned while those who opted in explicitly still are. The protocol version is negotiated per connection, and the extension long after Validate runs, so the trigger can't be narrowed; the text now names the case that is still risky — a connection that exchanges no result metadata id at all.

A metadata id is required, not just a capable connection. Both siblings gate on a non-empty result metadata id on the statement being executed, not only on the connection. That distinction is reachable here: the prepared cache is keyed (hostID, keyspace, statement) and is evicted only on prepare failure or UNPREPARED, never on connection close — so a statement prepared before the extension was negotiated survives a reconnect to a now-extension-enabled connection. Without the gate the driver would request skip_metadata while sending an empty id. Such a statement now asks for metadata for one more round trip, acquires an id from the resulting METADATA_CHANGED response, and skips from then on.

The non-empty-column-set gate is load-bearing, not an optimization. A statement whose RESULT/Prepared carries no result metadata is handed an id hashed from empty metadata. Current Scylla compares the client-supplied id against that same empty-metadata id, always matches, and so never sets METADATA_CHANGED — leaving a driver that asked to skip with a response it has no columns to decode. LIST ROLES OF is the motivating case. The server-side fixes for this, scylladb#29233 and scylladb#29275, are both closed unmerged, so no released Scylla has them. gocql never skips without cached columns, which is what keeps such statements working; java-driver checks the same thing first of all, and python-driver's bool(result_metadata) does the same. It is now documented and unit-tested as such rather than left to look incidental.

dialer.GetFrameHash gained a parameter. GetFrameHash(frame []byte, useMetadataID bool) is a source-incompatible change to an exported symbol. The v4 EXECUTE resultMetadataID field cannot be inferred from the frame bytes, so the negotiated state has to be plumbed in. The break is deliberate rather than papered over with a defaulting wrapper: a hash used to match recorded frames is better served by a compile error than by a silently-wrong default. dialer is the record/replay and single-connection benchmark harness the driver runs against itself, has no consumers outside its own subpackages, and now carries a package doc saying its signatures track the wire handling they mirror and may change without a major bump. Record gains a use_metadata_id field, which defaults to false and so leaves existing recordings valid.

Commits

commit	what
scylla,conn,frame: negotiate SCYLLA_USE_METADATA_ID and skip result metadata under it	the feature; the skip-metadata rule and its three gates; one derivation of the negotiated flag (newFramerWithExts, which had no non-test callers, is deleted rather than kept in sync); the METADATA_CHANGED-without-metadata guard
dialer: derive the header shift once, bound the metadata-id read, parse the option map	one masked headerShift(); bounds the now-live v4 length reads; walks the STARTUP [string map] instead of substring-scanning it; package doc
tests,CI: cover the v4 read path, the malformed response and proto v5	v4 read-side unit coverage; rolling-upgrade integration test; the malformed-METADATA_CHANGED regression test, which needed the mock server to speak the extension and so gives the negotiated v4 path its first unit-level coverage end to end; a proto-5 CI step; one existing test strengthened

The frame: bound pkeyCount allocation in parsePreparedMetadata commit that used to ride along here is an unrelated pre-existing defect and an upstream backport candidate. It has been split into #976 so it can merge on its own timeline.

Notable review outcomes

• METADATA_CHANGED with a new id but no column metadata is rejected as an error. Two things had to be prevented, not one. Adopting the id while keeping the old columns is unrecoverable — the server would match the id from then on and stop sending metadata, leaving the driver decoding against stale columns indefinitely; python-driver guards that identically. But returning the response's own rows is the same misdecode one execute earlier, since the server has just declared those columns stale and the skip-metadata path would reuse them anyway. An earlier revision logged and carried on, which meant a malformed response produced silently wrong rows instead of an error. The query now fails with the old id still cached, so a retry resends it and the server gets another chance to answer with the metadata it owes.
• Conn and its framers can no longer disagree. The flag was derived independently in three places, one of which (newFramerWithExts) had no non-test callers. A Conn that believed the extension was on while its framers did not would request skip_metadata while writing no id. newFramerWithExts is now deleted and its scylla_test.go call sites go through connFramers.initCache, so there is one derivation rather than two plus a comment asking future authors to keep them aligned. One framer is still not built from framerConfig — framerPool.get falls back to newFramer when the pool is disabled — but that is correct during the handshake and unreachable from the request path afterwards, because execInternal takes its framer before addCall rejects a closed connection. The accessor's doc comment says exactly that rather than claiming an invariant that holds by call ordering rather than by construction; #982 tracks the fix, which also covers the pre-existing flagLWT case (zero makes meta.lwt unconditionally true).
• The dialer EXECUTE parser's resultMetadataID read was dead code before this series and is now live on v4, where loadResponseFramesFromFiles feeds it bytes straight out of a recording file. The two length reads are bounded and the final slice range validated, falling back to raw-bytes hashing like the existing v5 guard. Recorded hashes are unchanged wherever the parse was already in bounds. Note the limit: addQueryParams, which runs after them, is still unbounded — pre-existing and shared with the opQuery branch.
• StartupNegotiatesMetadataID no longer substring-scans the frame. startupOptions puts caller-supplied DRIVER_NAME, DRIVER_VERSION and ApplicationInfo values into the same STARTUP [string map], so a caller could latch the flag by naming their application after the extension — and a spuriously latched flag makes GetFrameHash skip a field that isn't on the wire, surfacing as silent replay hash mismatches rather than an error. The map is now walked properly and only keys are compared.
• The integration tests no longer skip unconditionally. The extension case skips only when the server does not advertise the capability, and fails when the server advertises it but negotiation did not happen. It is the only end-to-end coverage of the feature, so a negotiation regression must not be able to turn it green.

Tests

• Unit: extension negotiation/registration/serialize; framer propagation; Conn.tracksResultMetadataID over both mechanisms and both protocol versions, including that it masks the direction bit and that usesMetadataID stays narrower so a v5 connection cannot pass for a negotiated extension; EXECUTE encoding on v4+extension, v4 without it, v5, and with a nil id (zero-length short bytes); the skip-metadata decision including the nil/zero-length-id, id-without-exchange, explicit-opt-in and empty-column-set cases; a truncated resultMetadataID in RESULT/Prepared reported as an error rather than a serve-goroutine panic; the read side of METADATA_CHANGED on v4 as well as v5; dialer truncation fallbacks, the v2 header shape, option-map parsing (key-only matching, truncation, overstated count) and a SUPPORTED response not latching the flag.
• Unit, end to end over a negotiated extension: TestExecuteMetadataChangedWithoutColumns drives a METADATA_CHANGED+NO_METADATA response through a TestServer that advertises SCYLLA_USE_METADATA_ID, so the driver reads a result metadata id out of RESULT/Prepared and writes one back on EXECUTE — the negotiated v4 path's first coverage outside the integration suite. The mock server rejects that execute unless skip_metadata was requested, which pins the skip-metadata decision on a live connection rather than only against shouldSkipResultMetadata's arguments. Reverting the rejection makes the test fail by returning rows, which was the defect.
• Integration: TestPrepareExecuteMetadataChangedFlag is now table-driven over {native v5, v4 + SCYLLA_USE_METADATA_ID} rather than carrying a second near-verbatim copy of its ~150-line flow, and drops/recreates its table so it is rerunnable against the same keyspace. TestPrepareExecuteScyllaEmptyMetadataID covers the rolling-upgrade sentinel, mirroring java-driver#758's should_handle_empty_metadata_id_when_executing_statement_when_supported and python-driver#770's test_empty_sentinel_id_triggers_metadata_changed.
• The native-v5 case now runs in CI. Makefile pins TEST_CQL_PROTOCOL ?= 4, so every ProtoVersion >= 5 integration test skipped on every leg — including the v5 half of the skip-metadata default this PR adds, which rested on unit tests only. The Cassandra 5-LATEST job gains one step at TEST_CQL_PROTOCOL=5, scoped with -run to TestPrepareExecuteMetadataChangedFlag. TEST_COMPRESSOR=no-compression goes with it out of necessity, not tidiness: the default is snappy, and Validate() rejects any compressor that is not a SegmentCompressor once ProtoVersion >= 5, which would fail every session in the suite. A full v5 lane would also light up ~8 unrelated tests that have never run, and a gap in any of them would block this work — that lane stays #960.
• make test-unit green (root and the nested lz4 module, -race); make check reports 0 lint issues and no go.mod drift; gofmt -l ., go vet ./... and go vet -tags "integration gocql_debug" ./... clean; each of the three commits builds and vets standalone.

Fixes

• DRIVER-841

[dkropachev]

@nikagra , please rebase and update PR description

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The change adds SCYLLA_USE_METADATA_ID support for protocol v4. It propagates negotiated state through framers, prepared execution, metadata caching, dialer recording, and replay hashing. It adds bounded parsing and malformed-frame validation. Tests cover metadata refresh, stale and empty IDs, segmented reads, deadlines, and protocol-v5 execution.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Conn
  participant Framer
  participant Server
  participant MetadataCache
  Client->>Conn: execute prepared statement
  Conn->>Framer: encode metadata ID and skip decision
  Framer->>Server: send EXECUTE request
  Server-->>Framer: return metadata response
  Framer-->>Conn: decode metadata ID and columns
  Conn->>MetadataCache: replace valid cached metadata
  Conn-->>Client: return decoded rows

Loading

Possibly related PRs

• scylladb/gocql#593: Adds related protocol-v5 prepared-statement metadata handling.
• scylladb/gocql#976: Also modifies prepared-result metadata parsing in frame.go.

Suggested labels: area/Driver_-_gocql

Suggested reviewers: dkropachev, sylwiaszunejko

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 68.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies implementation of SCYLLA_USE_METADATA_ID protocol-extension negotiation, which is the main change.
Description check	✅ Passed	The description directly and comprehensively explains the protocol-extension negotiation, metadata handling, tests, and behavior changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{Comment @coderabbitai help to get the list of available commands.}

[dkropachev]

I left one inline comment about a test reliability issue. The protocol behavior itself looks aligned with how the Rust driver handles SCYLLA_USE_METADATA_ID, including ignoring the cached-metadata opt-out when the extension is available.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

dialer/utils.go (1)

317-353: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Make all EXECUTE parsing bounds-aware before indexing.

GetFrameHash reads stream-ID bytes before it validates a complete legacy header. It also calls addQueryParams before the check at Lines 351-353. A short v1-v4 frame, or an EXECUTE frame truncated in query parameters, can still panic while loadResponseFramesFromFiles loads a damaged recording. Validate the header before stream normalization and make query-parameter parsing return failure before indexing.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dialer/utils.go` around lines 317 - 353, Make EXECUTE parsing in GetFrameHash
fully bounds-aware before any frame indexing. Validate that the complete legacy
header, including stream-ID bytes, is present before stream normalization, and
update addQueryParams to signal truncated input so callers return the raw-byte
Murmur3H1 fallback instead of indexing invalid offsets. Ensure the EXECUTE path
handles short v1–v4 frames and truncated query parameters without panicking.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@dialer/recorder/recorder.go`:
- Around line 97-108: The recorder currently evaluates
StartupNegotiatesMetadataID against incomplete data after f.record is reset
across Write calls. Update the framing logic in Write to retain and append the
in-progress STARTUP frame until its declared length is complete, then evaluate
dialer.StartupNegotiatesMetadataID and latch f.useMetadataID; preserve the
existing f.record.UseMetadataID assignment for subsequent records.

---

Outside diff comments:
In `@dialer/utils.go`:
- Around line 317-353: Make EXECUTE parsing in GetFrameHash fully bounds-aware
before any frame indexing. Validate that the complete legacy header, including
stream-ID bytes, is present before stream normalization, and update
addQueryParams to signal truncated input so callers return the raw-byte
Murmur3H1 fallback instead of indexing invalid offsets. Ensure the EXECUTE path
handles short v1–v4 frames and truncated query parameters without panicking.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Pro Plus

Run ID: d92ba146-b4e2-48dc-9020-83f19862e22b

📥 Commits

Reviewing files that changed from the base of the PR and between 4bee517 and 6de9131.

📒 Files selected for processing (15)

• cassandra_test.go
• cluster.go
• conn.go
• conn_test.go
• dialer/recorder/recorder.go
• dialer/replayer/replayer.go
• dialer/replayer/replayer_test.go
• dialer/utils.go
• dialer/utils_test.go
• frame.go
• frame_test.go
• framer.go
• scylla.go
• scylla_test.go
• session.go

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

dialer/utils.go (1)

278-299: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Validate the full header and short-bytes payloads before indexing.

A nonempty v4 frame shorter than 8 + headerShift(frame) panics on header access. A truncated resultMetadataID payload also moves endIndex beyond the slice before addQueryParams runs. The same payload check is missing after preparedIDLen.

Fix

 p := headerShift(frame)
+if len(frame) < 8+p {
+	return murmur.Murmur3H1(frame)
+}
 if p == 1 {
@@
 preparedIDLen := int(frame[index])<<8 | int(frame[index+1])
 endIndex = endIndex + 2 + preparedIDLen
+if endIndex > len(frame) {
+	return murmur.Murmur3H1(frame)
+}
@@
 resultMetadataIDLen := int(frame[endIndex])<<8 | int(frame[endIndex+1])
 endIndex = endIndex + 2 + resultMetadataIDLen
+if endIndex > len(frame) {
+	return murmur.Murmur3H1(frame)
+}

Also applies to: 322-343

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dialer/utils.go` around lines 278 - 299, In the frame-hashing/parsing flow
around headerShift, validate that nonempty v4 frames contain at least 8 +
headerShift(frame) bytes before accessing header fields. Before advancing
endIndex or calling addQueryParams, validate the resultMetadataID payload
length, and add the equivalent bounds check after preparedIDLen so truncated
payloads return safely instead of indexing past the slice.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@dialer/utils.go`:
- Around line 278-299: In the frame-hashing/parsing flow around headerShift,
validate that nonempty v4 frames contain at least 8 + headerShift(frame) bytes
before accessing header fields. Before advancing endIndex or calling
addQueryParams, validate the resultMetadataID payload length, and add the
equivalent bounds check after preparedIDLen so truncated payloads return safely
instead of indexing past the slice.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Pro Plus

Run ID: a4257ecd-8245-4ea8-9490-a05d40543e49

📥 Commits

Reviewing files that changed from the base of the PR and between 6de9131 and 32ff7ef.

📒 Files selected for processing (16)

• .github/workflows/main.yml
• cassandra_test.go
• cluster.go
• conn.go
• conn_test.go
• dialer/recorder/recorder.go
• dialer/replayer/replayer.go
• dialer/replayer/replayer_test.go
• dialer/utils.go
• dialer/utils_test.go
• frame.go
• frame_test.go
• framer.go
• scylla.go
• scylla_test.go
• session.go

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@dialer/utils.go`:
- Around line 279-283: Update GetFrameHash and the legacy-frame parsing path
around headerShift and addQueryParams to validate bounds before masking, reading
opcodes, or indexing frame data. Reject v4 frames shorter than the complete
header, and verify every variable-length field—including EXECUTE
resultMetadataID—fits within the remaining bytes before passing control to
addQueryParams or continuing parsing.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Pro Plus

Run ID: 65f09c61-a97f-4edc-a6f0-0d8c557d73a7

📥 Commits

Reviewing files that changed from the base of the PR and between 32ff7ef and f90cdab.

📒 Files selected for processing (16)

• .github/workflows/main.yml
• cassandra_test.go
• cluster.go
• conn.go
• conn_test.go
• dialer/recorder/recorder.go
• dialer/replayer/replayer.go
• dialer/replayer/replayer_test.go
• dialer/utils.go
• dialer/utils_test.go
• frame.go
• frame_test.go
• framer.go
• scylla.go
• scylla_test.go
• session.go

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@dialer/recorder/recorder.go`:
- Around line 104-118: Update the frame-recording logic around the to_record
handling so each CQL frame becomes a separate record. Limit the current append
to the remaining bytes in the frame, then loop over surplus bytes to initialize
and record subsequent frames, preserving their lengths and metadata handling
through StartupNegotiatesMetadataID and UseMetadataID.

In `@dialer/utils.go`:
- Around line 148-150: Update fits to validate index is nonnegative and no
greater than len(frame) before performing subtraction, then validate n is
nonnegative and at most len(frame)-index; avoid calculating index+n so malformed
lengths cannot overflow.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Pro Plus

Run ID: a009dbd2-7078-4e64-9ce3-66e697690a35

📥 Commits

Reviewing files that changed from the base of the PR and between 0fdab60 and 5eb5170.

📒 Files selected for processing (17)

• .github/workflows/main.yml
• cassandra_test.go
• cluster.go
• conn.go
• conn_test.go
• dialer/recorder/recorder.go
• dialer/recorder/recorder_test.go
• dialer/replayer/replayer.go
• dialer/replayer/replayer_test.go
• dialer/utils.go
• dialer/utils_test.go
• frame.go
• frame_test.go
• framer.go
• scylla.go
• scylla_test.go
• session.go

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (2)

dialer/utils_test.go-316-321 (1)

316-321: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

The "truncated at the metadata id length" case never reaches that check.

v4ExecuteFrame(true) is 21 bytes: header 0–8, preparedID length 9–10, preparedID payload 11–13, resultMetadataID length 14–15. full[:11] fails the preparedID payload check (fits(frame, 9, 2+3)), so both cases exercise the same guard. Use full[:15] to make the resultMetadataID length field the truncation point.

💚 Proposed fix

 		{
 			// preparedID parses, then the metadata-id length field runs off the end.
 			name:          "truncated at the metadata id length",
-			frame:         full[:11],
+			frame:         full[:15],
 			useMetadataID: true,
 		},

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dialer/utils_test.go` around lines 316 - 321, The test fixture named
“truncated at the metadata id length” uses a slice that truncates during the
preparedID payload instead. Update its frame slice to full[:15] so
v4ExecuteFrame(true) reaches the resultMetadataID length-field boundary, while
leaving the neighboring truncation case unchanged.

dialer/utils.go-460-468 (1)

460-468: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Bound the v1 values-count read.

The v1 branch reads frame[index] and frame[index+1] with no fits check. The only preceding guard is fits(frame, 0, 8+p), which for v1 (p == 0) proves only len(frame) >= 8. A v1 EXECUTE record with a header-length body panics here, which is the failure mode the rest of this function now avoids.

🛡️ Proposed guard

 		} else {
+			if !fits(frame, index, 2) {
+				return murmur.Murmur3H1(frame)
+			}
 			valuesLen := int(frame[index])<<8 | int(frame[index+1])
 			index = index + 2

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dialer/utils.go` around lines 460 - 468, In the v1 branch of the surrounding
parser, validate that two bytes are available at the current index before
reading frame[index] and frame[index+1] for valuesLen. If the bounds check
fails, return the existing Murmur3H1 fallback, preserving the current behavior
for valid records and preventing short v1 EXECUTE frames from panicking.

🧹 Nitpick comments (1)

dialer/utils.go (1)

291-298: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Advance index past the custom-payload count unconditionally.

index is advanced by 2 only when customPayloadLenght > 0. For a zero-length custom payload the count short stays in the range, so addQueryParams then reads the count bytes as the consistency field. The hash stays deterministic, so replay still matches, but the extracted range is wrong.

♻️ Proposed change

 	customPayloadLenght := int(frame[8+p])<<8 | int(frame[9+p])
-	if customPayloadLenght > 0 {
-		index = index + 2
-	}
+	index = index + 2

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dialer/utils.go` around lines 291 - 298, Update addCustomPayload so index is
advanced past the two-byte custom-payload count unconditionally after the bounds
check, regardless of customPayloadLenght. Preserve the existing length
evaluation and return behavior while ensuring subsequent addQueryParams parsing
starts after the count field.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Other comments:
In `@dialer/utils_test.go`:
- Around line 316-321: The test fixture named “truncated at the metadata id
length” uses a slice that truncates during the preparedID payload instead.
Update its frame slice to full[:15] so v4ExecuteFrame(true) reaches the
resultMetadataID length-field boundary, while leaving the neighboring truncation
case unchanged.

In `@dialer/utils.go`:
- Around line 460-468: In the v1 branch of the surrounding parser, validate that
two bytes are available at the current index before reading frame[index] and
frame[index+1] for valuesLen. If the bounds check fails, return the existing
Murmur3H1 fallback, preserving the current behavior for valid records and
preventing short v1 EXECUTE frames from panicking.

---

Nitpick comments:
In `@dialer/utils.go`:
- Around line 291-298: Update addCustomPayload so index is advanced past the
two-byte custom-payload count unconditionally after the bounds check, regardless
of customPayloadLenght. Preserve the existing length evaluation and return
behavior while ensuring subsequent addQueryParams parsing starts after the count
field.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Pro Plus

Run ID: c4d8b30f-9001-49db-ad91-9a42d0d9183e

📥 Commits

Reviewing files that changed from the base of the PR and between 0fdab60 and 3b42491.

📒 Files selected for processing (17)

• .github/workflows/main.yml
• cassandra_test.go
• cluster.go
• conn.go
• conn_test.go
• dialer/recorder/recorder.go
• dialer/recorder/recorder_test.go
• dialer/replayer/replayer.go
• dialer/replayer/replayer_test.go
• dialer/utils.go
• dialer/utils_test.go
• frame.go
• frame_test.go
• framer.go
• scylla.go
• scylla_test.go
• session.go

[nikagra]

@coderabbitai on the two grouped comments, which have no thread to reply in:

dialer/utils_test.go 316-321 — correct, fixed. full[:11] stopped at the preparedID payload check, so both cases hit the same guard; it is full[:15] now. Verified by deleting the resultMetadataID length guard and watching that case panic at frame[15].

dialer/utils.go 460-468 — false positive. index is unchanged between the fits(frame, index, 2) at line 430, which guards the preparedID length read, and the v1 valuesLen read at 461: the only thing that moves it is addCustomPayload, above both. Added a comment there saying so.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

ast-grep outline dialer/utils.go --items all --type function --match 'GetFrameHash|fits|addCustomPayload'
ast-grep outline dialer/utils_test.go --items all --type function --match 'Test.*Metadata|Test.*Execute|Test.*FrameHash'

printf '\n--- dialer/utils.go target region ---\n'
nl -ba dialer/utils.go | sed -n '400,485p'

printf '\n--- dialer/utils_test.go target region ---\n'
nl -ba dialer/utils_test.go | sed -n '285,340p'

Length of output: 1607

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- dialer/utils.go: GetFrameHash relevant region ---'
awk 'NR >= 400 && NR <= 485 { printf "%6d  %s\n", NR, $0 }' dialer/utils.go

printf '%s\n' '--- dialer/utils_test.go: truncated EXECUTE cases ---'
awk 'NR >= 302 && NR <= 360 { printf "%6d  %s\n", NR, $0 }' dialer/utils_test.go

Length of output: 7150

---

@nikagra Verified.

• full[:15] reaches the result-metadata-ID length guard.
• The valuesLen read at Line 463 is covered by the fits(frame, index, 2) check at Line 430. No code changes index between those lines.

---

✏️ Learnings added

Learnt from: nikagra
URL: https://github.com/scylladb/gocql/pull/590

Timestamp: 2026-08-06T20:55:05.078Z
Learning: In `dialer/utils.go`, the protocol-v1 `valuesLen` read in `GetFrameHash` uses the same `index` as the earlier prepared-ID length read. `fits(frame, index, 2)` guards both reads because only `endIndex` changes between them.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

_{You are interacting with an AI system.}

[sylwiaszunejko]

Nice change overall

[sylwiaszunejko]

I am not sure if this is a real issue, but worth verifying I guess

[dkropachev]

Approved. The metadata-ID negotiation, wire framing, cache refresh, and focused coverage are sound. The two deeper replay limitations found during review are pre-existing and explicitly deferred to #990 and #991; neither blocks this change.