Workflow [PR], commit [59ebbd0]

Summary: ❌

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR reapplies the Silk fiber-aware plain and TLS socket implementations, wires them into the Poco socket abstractions, rejects external async polling for fiber sockets, and adds focused gtests. The plain socket short-write issue from the prior review is fixed in the current code, but the TLS BIO timeout issue is still present, so this needs changes before approval.

PR Metadata

• 💡 The Improvement category is plausible, but the changelog entry has a blank line after the reapply sentence. The changelog collector stops at that blank line, so the published entry would be only Reapply https://github.com/ClickHouse/ClickHouse/pull/106078 after https://github.com/ClickHouse/ClickHouse/pull/107539. and would omit the actual user-readable change.
  Suggested replacement:

  Added `Silk` fiber-aware implementations for plain and TLS sockets.
  

Missing context / blind spots

• ⚠️ I did not run the local gtests because contrib/silk is not initialized in this checkout. The fetched Praktika PR report currently shows no failed entries; a local or CI run of unit_tests_dbms with Silk and SSL enabled would close this gap.

Findings

• ⚠️ Majors
  • [src/IO/SilkSecureFiberStreamSocketImpl.cpp:41] SecureSocketImpl bounds TLS handshakes and SSL I/O by getMaxTimeoutOrLimit, so either a send or receive timeout should stop the operation. The custom BIO read still waits only on getReceiveTimeout, while the write side waits only on getSendTimeout. If only setSendTimeout is configured and the peer never sends TLS data, SSL_do_handshake can remain inside the BIO read indefinitely before the outer remaining-time accounting runs; the symmetric case applies to writes when only a receive timeout is configured. Use the same max(send, receive) timeout semantics in the BIO layer, or make the BIO return WANT_READ / WANT_WRITE promptly so SecureSocketImpl owns the remaining-time accounting.

Tests

• ⚠️ Add a focused Silk TLS test where the peer never sends handshake data and only setSendTimeout is configured, plus the symmetric SSL I/O case for only setReceiveTimeout. The existing timeout tests do not prove the single-timeout configurations that currently hang.

Final Verdict

Status: ⚠️ Request changes

Minimum required actions: fix the custom TLS BIO timeout semantics, add focused coverage for the single-timeout cases, and replace the changelog entry with a single user-readable paragraph.

@groeneai Help sort out the CI failures.

@nikitamikhaylov Triaged the 3 failing checks on 3f23dfc. None are caused by this PR: no silk-fiber/socket frames, and contrib/silk is not in any failing path. All three are pre-existing trunk flakes, each hitting master and many unrelated PRs in the last 30d.

Stress test (amd_tsan) - LOGICAL_ERROR: Unexpected return type from if. Expected UInt64. Got String (STID 1611-483a), in ExpressionActions::execute -> MergeTreeRangeReader::executePrewhere. Triggered by a fuzzer-mutated if(0, CAST(toString(...) AS UInt64), v). This is a real trunk bug in the if short-circuit / PREWHERE return-type path, not socket-related. CIDB 30d: 19 distinct refs incl. 4 on master (this branch is 1 of them, same as the rest). Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=107680&sha=3f23dfc85cace53649bd8ac14135dc2429aa2cd1&name_0=PR&name_1=Stress%20test%20%28amd_tsan%29

Stress test (amd_asan_ubsan) - Hung check. The hung query is an AST-fuzzer runaway: SELECT arrayFold(acc, x -> arrayPushFront(acc, x), range(number), emptyArrayUInt64()) FROM system.numbers LIMIT 5 (already is_cancelled=1, elapsed 2709s, 1.6 GiB). Generic fuzzer/hung-check flake. CIDB 30d: 39 distinct PRs + 9 master. Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=107680&sha=3f23dfc85cace53649bd8ac14135dc2429aa2cd1&name_0=PR&name_1=Stress%20test%20%28amd_asan_ubsan%29

Stateless tests (arm_binary, parallel) - 03518_alter_logical_race (showed up after this run). Chronic race, 17 PRs + 1 master in 30d.

vadimskipin already approved. None of these block the reapply. Suggest rebasing on master and re-running (the TSAN if/PREWHERE error is a separate trunk bug worth its own fix; it will recur on any PR until then).

@groeneai Link the existing issues or fixing PRs for all the failures.

@nikitamikhaylov Links per failure:

1. Stress test (amd_tsan), Unexpected return type from if (STID 1611-483a): fixing PR Fix LOGICAL_ERROR in on-fly mutation read when an UPDATE keep-old fallback is read under a non-constant condition #108128 (open, CI running), tracking issue ALTER MODIFY COLUMN after a lightweight DELETE fails when soft-deleted rows hold unconvertible values #105947. It is a post-Fix LOGICAL_ERROR in on-fly mutation prewhere when reading an overwritten LowCardinality source column #107475 on-fly-mutation if keep-old-fallback return-type bug, not socket-related.

2. Stress test (amd_asan_ubsan), Hung check: auto-generated CI tracking issue Hung check failed, possible deadlock found #107941 ("Hung check failed, possible deadlock found", labels testing+fuzz). The arrayFold runaway here is one instance of the generic fuzzer hung-check (cancelled query not terminating); there is no dedicated fixing PR for that specific query.

3. Stateless tests (arm_binary, parallel), 03518_alter_logical_race: fixing PRs Let 03518_alter_logical_race extend its budget when no progress is made #108003 (merged 2026-06-20) and Fix flaky 03518_alter_logical_race: unthrottle the MODIFY COLUMN mutation #108060 (open follow-up that unthrottles the MODIFY COLUMN mutation).

@nikitamikhaylov Links per failure:

Stress (amd_tsan) if return-type LOGICAL_ERROR: tracking issue #105947, fixing PR #108128 (mine, open, closes #105947). It is the if manifestation introduced by merged #107475, which fixed the earlier materialize manifestation of the same on-fly-mutation read bug.

Stress (amd_asan_ubsan) hung check (arrayFold runaway): umbrella issue #107941 ("Hung check failed, possible deadlock found"); diagnostic logging in open #105905. No dedicated fix for this specific fuzzer cancelled-query-not-terminating shape, it is the chronic fuzzer hung-check flake (39 PRs + 9 master in 30d).

Stateless (arm_binary, parallel) 03518_alter_logical_race: no tracking issue, two fixes by alexey-milovidov: merged #108003 (extend the test budget) and open #108060 (unthrottle the MODIFY COLUMN mutation).

@groeneai, provide a fix for runaway arrayFold. We can add limits for too complex operations or introduce cancellation checks inside.

It will be reasonable to update the branch and check for CI again.

@alexey-milovidov fix for the runaway arrayFold: #108192.

FunctionArrayFold::executeImpl folds an entire chunk in one call, and the only time/cancellation check is at the pipeline level between chunks, so a fold over one long array ignored KILL QUERY and max_execution_time (the hung-check query ran 2709s while already cancelled). The PR adds checkTimeLimit() inside the in-function loops (cancellation-checks approach), plus a regression test that times out promptly. Verified: without the fix the test query runs ~15-17s ignoring max_execution_time=1; with it, ~1.5s.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 375/441 (85.03%) | Lost baseline coverage (was covered on master, now uncovered in this PR): 5 line(s) · Uncovered code

Full report · Diff report

@groeneai what's up with the CI failures?

@nikitamikhaylov The only real failure on the current head (59ebbd08) is Stress test (arm_tsan). Everything else red is either a consequence of it or non-blocking infra, and none of it comes from this PR.

• Stress test (arm_tsan) — server aborts on Logical error: 'Unexpected exception in refresh scheduling' (STID 2508-3e7b), then "Cannot start clickhouse-server" (3/4). The crash is on a BackgroundSchedulePool thread in RefreshTask::doScheduling (RefreshTask.cpp:936); there are no socket/fiber/Throttler frames in the stack, so it is unrelated to this PR's diff (Poco sockets, silk fibers, Throttler, HTTP). It is a pre-existing trunk bug: 186 failures across 127 PRs and 21 master runs in the last 30 days, on every Stress/BuzzHouse sanitizer variant. Tracked in Logical error: 'Unexpected exception in refresh scheduling' (STID: 2508-3e7b) #106737 (still recurs on master); open candidate fix Fix null deref in RefreshTask::doScheduling under parallel shutdown #105588. Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=107680&sha=59ebbd08e29bf37893f2844f33e19b9d09f4f126&name_0=PR&name_1=Stress%20test%20%28arm_tsan%29
• CH Inc sync — CH Inc private monorepo-sync check; fails fork-wide, non-blocking, not actionable from a public PR. Unrelated to this change.
• Mergeable Check / PR — aggregate rollups, red only because the arm_tsan stress job is red.

Re-running the arm_tsan stress job should clear it; the assertion is intermittent and not produced by this PR.

The logical error in RMV refresh is unlikely to be related. The failure in private CI appears to be an infrastructure issue. Merging this PR.