I think this test should be xfail, not "scylla_only". This is a feature that DynamoDB has an Scylla doesn't - it's not a Scylla extension, it's a scylla deficiency.

You're right.
I wanted the final assert to be with pytest.xfail for the non-AWS case, but that's not possible, so I will make it as two tests instead.

In this end-of-shard decision logic, the subsequent else-branch (when the shard is still open) constructs a next_iter but then returns the old iter as NextShardIterator. That leaves next_iter unused (will fail under -Werror) and prevents advancing the iterator threshold, risking callers repeatedly querying the same empty range. Ensure the returned NextShardIterator actually uses the advanced iterator value.

I didn't understand what the AI meant here, but please investigate this.

In disable_stream(), the polling loop now sleeps only 0.1s between ListStreams/DescribeStream calls. This can exceed DynamoDB’s documented DescribeStream rate limits (and will scale with the number of historic streams), making the tests more likely to be throttled/flaky. Consider restoring a slower poll interval (e.g., 0.5s) or adding backoff/jitter while still using time.time() for the timeout.

I think this is fine.
I didn't realize that Copilot is right -DescribeStream indeed has a 10-per-second limit (https://docs.aws.amazon.com/streams/latest/dev/service-sizes-and-limits.html). But sleeping 0.1, and adding the unavoidable network lag, I think 0.1 is exactly right. Not to mention that if this request does get throttled, it just means that the SDK will retry, so no harm done. On Alternator, lower sleeps between retries can make tests faster.

_count_stream_records() claims to count “all shards” and “total number of records”, but it only reads the first DescribeStream page and only a single GetRecords page per shard (ignoring NextShardIterator). This can undercount and make the new assertions unreliable. Consider iterating DescribeStream pages (LastEvaluatedShardId) and paging GetRecords until NextShardIterator is absent to actually consume the full stream (or rename/comment it to reflect what it really does).

Copilot is right.
One option is to fix this function as copilot suggested. The other option, if you only care whether the count is zero or not, is to rename this function stream_is_empty() or something and return true or false. But even with stream_is_empty(), you should look at all shards, not just the first page.

The new tests assert that stream records are immediately visible right after update_item(). Alternator streams are asynchronous (and this file already uses retry loops/timeouts elsewhere), so these immediate “> 0” assertions are likely to be flaky on both AWS and Scylla. Consider adding a bounded retry loop (like fetch_and_compare_events()/other tests above) to wait until at least one record becomes visible before asserting on counts.

Good catch.

After dropping the old CDC log table, the code reloads tab/builder and re-applies add_stream_options(), but it does not re-run the follow-up steps that were applied to the original builder when enabling streams (e.g., validate_cdc_log_name_length() and, on tablet tables, defer_enabling_streams_block_tablet_merges()). Because builder was reconstructed, those adjustments are lost and re-enable behavior can diverge from first-time enablement on tablet tables. Consider re-applying the same post-processing after the second add_stream_options() call (or refactor to avoid duplicating the enable flow).

I think my proposal to just do "continue" will do the right thing for free.

I think this code is not concurrency-safe:

Between dropping the old guard (the std::move above) and starting a new one here, things might have changed. Some parallel operation might have re-enabled CDC or deleted the table entirely or I don't know what.

I think there's an easy solution: you just need to call "continue" here to retry from the beginning, and not try to assume that re-capturing the guard will work. In the "good" case, after the "continue" you reach will this code again, and it will notice that the log table no longer exists, so it will have nothing to do.

I think my proposal to just do "continue" will do the right thing for free.

I didn't understand what the AI meant here, but please investigate this.

Nitpick: I don't think we need to reference issue #7239 in every comment. The explanation that we need to keep the old options so DescribeStream can show them is good enough - nobody should go to issue 7239 for more information, you wont find additional options there.

The comment says "when CDC is enabled or ...", but I don't see the "when CDC is enabled" (opts.enabled()) in the new code. I am worried that because of this, UpdateTable may not add the right stream info in the response because the log table doesn't exist yet (?). I don't remember if we have a test for this - what UpdateTable adding a stream actually returns.

You will be closing #7239, so please either create a new issue about this last remaining problem and mention that issue (in the comment and xfail), or don't mention any issue. I think it's confusing to list a closed issue as an xfail reason.

Good catch.

You'll be closing this issue so please don't leave a link to it as a closed issue.
You should create a new issue for the last remaining point.

Very sadly, this new link be a JIRA issue that will users who can see compatibility.md will not actually be able to see. What a colossal mistake it was to outlaw github issues :-(

CC @dani-tweig - this was one of the reasons I didn't want to drop the public github issues.

yikes. good catch. How did I ever write this code like this? :-( Thanks for fixing it.

In any case, in the working case the timeout will never be reached which is why we never noticed this problem.

Please merge this fix with the first patch, that introduced this bug.