Skip to content

feat(ci): check-clock-usage — enforce the wall-clock allowlist - #1861

Merged
tato123 merged 7 commits into
mainfrom
feat/1728-check-clock-usage
Aug 13, 2026
Merged

feat(ci): check-clock-usage — enforce the wall-clock allowlist#1861
tato123 merged 7 commits into
mainfrom
feat/1728-check-clock-usage

Conversation

@tato123

@tato123 tato123 commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

Lands cargo xtask check-clock-usage — the mechanical guard that makes the wall-clock allowlist unfakeable — and resolves every wall-clock read that was not one of the four permitted observability surfaces.

The plan permits wall clock on exactly four surfaces (log record host_ts and source_ts, log file naming, control-plane pubsub event timestamp_ns) and requires monotonic everywhere else. Prose alone does not hold that line: SystemTime::now() is the reflexive spelling for "what time is it", and once a wall-clock value reaches the data plane it is indistinguishable from a media timestamp until someone subtracts one from the other.

The gate. Scans runtime/ sdk/ adapters/ xtask/ packages/test-fixtures/ for wall-clock reads in Rust and Python. Four things keep the list from growing by accident:

  • No per-line pragma, no opt-out attribute. The file allowlist is the only way past, and every entry names one of four ObservabilitySurface variants — a fifth surface means adding a variant, which is not something anyone does while reaching for a convenient timestamp.
  • Every allowlist entry must still read a wall clock. A surface that moves otherwise leaves a licence behind on a path a later change inherits.
  • Every scan root and language arm must read files, and prose it cannot delimit is refused rather than blanked. A gate that reads nothing and reports clean is this design's only failure mode.
  • The banned set includes CLOCK_REALTIME, because the likeliest way to introduce a wall-clock read in this engine is to copy core/media_clock.rs's libc::clock_gettime(libc::CLOCK_MONOTONIC, ..) and flip one token.

The 16 conversions, across 15 files. Every non-observability site minted a unique name, not a timestamp. All converted; none allowlisted — the allowlist is per-file, so an entry for iceoryx2/output.rs (the frame-stamp file) or thread_runner.rs would have licensed a wall-clock read in the exact data-plane files the guard exists to protect.

mint_machine_global_unique_name_suffix() is <pid>-<uuid v4>, built on the uuid crate the engine already links, and it keeps the two namespaces with no length limit: iceoryx2 service names and pipeline-cache file names. Every test socket path takes a TempDir instead — sun_path is 108 bytes and a composed unique name spends most of that budget before TMPDIR is accounted for. The api-server's name seed takes the OS CSPRNG it already links, and the dead apple/time.rs::system_time_to_ns was deleted.

Closes

Closes #1728

Exit criteria

  • cargo xtask check-clock-usage exists, fails on a wall-clock read outside the allowlist, and passes on the current tree
  • Runs in CI — an entry in ALL_SOURCE_WALKING_GATES, executed by the existing source-gates job (note 1)
  • The allowlist contains exactly the four observability surfaces, each with a stated reason (five files; host_ts has two readers)
  • Every SystemTime-for-unique-names site resolved — 16 of 16 converted, none grandfathered
  • The self-contradictory host_ts doc corrected (the stale spike claim is void — note 2)

Test plan

Every test lands in an existing test binary — no new tests/*.rs, no new [[test]] target, nothing new to link.

  • 22 gate fixture tests in xtask/src/check_clock_usage.rs, in the existing cargo test -p xtask run (0.01s). Cover: planted reads in a data-plane file for each banned spelling; each permitted surface passes; Rust doc comments and Python module docstrings naming the banned APIs are skipped (the real clock.py shape); the monotonic spellings pass; an unterminated docstring fails the scan instead of hiding the file; a stale allowlist entry fails; a scan root or language arm reading nothing fails; discovery excludes .venv-pyright.
  • 3 mint tests in core/machine_global_unique_name.rs, in the engine lib binary.
  • Negative proofs, run by hand and reverted — a planted SystemTime::now() and a planted libc::clock_gettime(libc::CLOCK_REALTIME, ..) in iceoryx2/output.rs each produced ✗ check-clock-usage: 1 violation naming the file and line; green after revert.
  • Mutant-tested the mint tests: replacing Uuid::new_v4() with Uuid::nil() fails both. (The first version of these tests did not fail under mutation — see note 3.)
  • cargo xtask run-local-ci-gates: all 9 source gates, 200 xtask tests, 37 ship-gate tests, license headers, SDK/macros/emission-lock suites — all green. cargo fmt --check --all clean.

Notes for owner

  1. No new workflow. The ticket said "plus its workflow", written before ci: one job for the source gates, and caching that is not a net loss #1857 folded thirteen CI jobs into six. The gate is registered in ALL_SOURCE_WALKING_GATES and the existing source-gates job runs it. Ticket body corrected with strikethroughs preserved.

  2. No Date.now arm. The Deno SDK is deleted and the engine tree holds no .ts/.js source, so that scan root does not exist — and a zero-file root would fail the gate's own read-source contract. Rust and Python only. The ticket's other stale claim (the pyembed spike doc fix) is void: feat(engine): helper-process placement, the only mode — spawn host, in-process rip-out, identity, cross-process pixels #1714 deleted the spike tree whole.

  3. Two review rounds materially changed this. Worth knowing what they caught, because both were things I got wrong:

    • I claimed no existing unique-name mint and built one. That search was wrongstreamlib-engine has linked uuid with v4 all along and pubsub/integration_tests.rs already minted an iceoryx2 service name with it. The mint now rides Uuid::new_v4(), and that pre-existing site moved onto the helper so the tree has one idiom. This also retired a hazard my composite carried (the monotonic component resets on reboot while pids recycle).
    • My first mint test passed with the counter gutted and with the clock gutted, and its failure message asserted a premise the evidence disproved. Replaced with one that strips the fixed pid prefix and compares what is left.
    • The Python prose blanker swallowed an unterminated triple-quoted span, blanking the rest of the file while files_scanned still incremented — a file with a live time.time_ns() would have scanned clean.
    • The gate missed CLOCK_REALTIME, the one spelling this codebase is most likely to actually write.
  4. The gate label — I read it as stale (the clock-scope decision was RESOLVED 2026-08-03 and fix(engine)!: MediaClock carries the machine monotonic epoch #1725/fix(engine)!: audio tick timestamps carry the machine epoch #1726/refactor(python)!: delete the wheel duplicate media_clock_now_ns export #1727 all shipped). Correct me if something is still parked.

  5. docs/plan/changes/one-monotonic-clock.md updated with factual records, annotated not overwritten: the guard's actual shape, the conversion disposition, and where the epoch-parity test landed (as the host + wheel pair the bullet itself anticipated — both halves already exist). No DECIDED or OPEN entry moves. Flagging because it is a plan file, not because it is a decision.

  6. Two things I could not verify, stated plainly:

    • The macOS cross-compile. cargo check --target aarch64-apple-darwin -p streamlib-engine cannot run here — iceoryx2-pal-posix's build script dies on a missing libproc.h (no macOS SDK), which stale fingerprints show predates this branch. My Apple-path change is a deletion of system_time_to_ns + its test + one import, and git grep system_time_to_ns finds no remaining reference anywhere. That is grep evidence, not a compile. Someone with an SDK should run it.
    • An intermittent engine-lib abort, now identified but not attributed. It is not a test failure: the whole test binary takes SIGABRT from glibc (corrupted size vs. prev_size while consolidating), so no individual test ever reports FAILED and the harness attributes it as 1182 passed; 1 failed. Observed ~2 times in ~14 runs on this branch. Evidence it is the suite's documented pre-existing flake rather than this diff: an independent run in this session hit the identical message at 8bdd7c08, before the UUID and TempDir changes existed; it does not reproduce under --test-threads=1 (consistent with a race between concurrent tests); it aborts after the Vulkan RHI tests, nowhere near the sockets; and this diff adds no unsafe and no buffer handling — only format! composition and TempDir. What I did not manage: reproducing it on base main, where 3 runs were clean — at a ~14% rate that neither confirms nor refutes. Stating it that way rather than calling it pre-existing.
  7. Findings I did not act on (outside scope, no ticket filed per the P0 rule):

    • apple/time.rs is entirely unreferenced — mach_now_ns and mach_ticks_to_ns have no callers, and the file carries your own TODO(@jonathan) asking whether it can go. I deleted only the one function the gate forced.
    • docs/testing-hardware.md's workspace-baseline command excludes five packages that are no longer workspace members; the --exclude flags are now a no-op.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added a source check for unintended wall-clock usage, with recommendations for monotonic alternatives.
    • Added a command to run the check directly; source validation now includes nine gates.
    • Improved temporary socket, cache, and service-name handling with reliable uniqueness and automatic cleanup.
  • Bug Fixes

    • Runtime name generation now uses secure random data, with a safe fallback when unavailable.
  • Documentation

    • Clarified timestamp semantics and documented the completed clock-validation approach.

Sixteen sites reached for `SystemTime::now()` to mint a unique name — iceoryx2
service names in `/dev/shm`, pipeline-cache temp files, test socket paths, a
runtime-name seed. None of them wanted a timestamp; they wanted a string no
concurrent process and no earlier run could collide with. Reading a wall clock
for that is both wrong in kind and weaker in practice: two mints in the same
nanosecond produced the same name.

`mint_machine_global_unique_name_suffix()` is the one primitive for the job —
pid (concurrent processes) + `MediaClock::now()` (runs on one boot, so a
recycled pid cannot resurrect a stale `/dev/shm` entry) + a sequence (two mints
in one nanosecond). It carries no separator a path or a file name would reject,
so callers compose it into their own naming convention rather than the mint
guessing at one.

Not the mint:
- `streamlib-surface-client`'s test sockets take a `TempDir` each. The kernel
  answers uniqueness and the socket is unlinked on drop rather than littering
  /tmp. That crate reaches no engine code, and a second local mint would be a
  parallel abstraction.
- The api-server's runtime-name seed takes the OS CSPRNG it already links for
  bearer tokens, falling back to the pid with a warning rather than failing a
  runtime over a display name. Wall clock ^ pid was the weaker entropy source.
- `apple/time.rs::system_time_to_ns` is deleted. Nothing imports `apple::time`,
  and a wall-clock-to-nanoseconds converter is the exact shape the
  one-monotonic-clock ADR retires.

Behavior is unchanged everywhere; only the source of the unique part moves.

Refs #1728
The plan permits wall clock on exactly four observability surfaces and requires
monotonic everywhere else. Prose alone does not hold that line: `SystemTime::now()`
is the reflexive spelling for "what time is it", and a wall-clock value that
reaches the data plane is indistinguishable from a media timestamp until someone
subtracts one from the other.

`cargo xtask check-clock-usage` scans `runtime/ sdk/ adapters/ xtask/` for
wall-clock reads in Rust and Python and fails on any outside the permitted files.
It is an entry in `ALL_SOURCE_WALKING_GATES`, so the existing `source-gates` job
runs it — #1857 consolidated the per-gate workflows, and this ticket's "plus its
workflow" predates that.

What keeps the list from growing by accident:
- No per-line pragma and no opt-out attribute. The file allowlist is the only way
  past, and every entry names one of four `ObservabilitySurface` variants — so a
  fifth surface means adding a variant, which is not something anyone does while
  reaching for a convenient timestamp.
- Every entry must still read a wall clock. A surface that moves leaves a licence
  behind on a path some later change would inherit; the gate fails instead.
- Every scan root and every language arm must read files. A gate whose tree moved
  out from under it reports clean, which is the failure mode that matters most.

Discovery is `git ls-files`, not a filesystem walk: `sdk/streamlib-python-wheel`
carries `.venv-pyright` with ~6000 third-party sources that are not ours to gate.
Prose is blanked before scanning, so the wheel's own `clock.py` — whose module
docstring names `time.time` / `datetime.now` / `time.time_ns` precisely to warn
readers off them — is not a violation.

Also corrects `RuntimeLogEvent::host_ts`, whose doc read "Host monotonic receipt
timestamp, nanoseconds since UNIX epoch" — self-contradictory, and exactly the
confusion this gate exists to prevent.

Closes #1728
Two claims in the one-monotonic-clock change file were open or falsified, both
derivable from the tree now: the guard has no workflow of its own (#1857
consolidated them) and no `Date.now` arm (no JS/TS source exists), and the
unique-name uses the recon left "final at implementation" all converted rather
than any joining the allowlist. Also records where the epoch-parity test landed —
as the host + wheel pair the bullet itself anticipated, not one cross-process test.

Records, not decisions: no DECIDED or OPEN entry moves.

Refs #1728
Review found the one hole that mattered: `blank_out_python_prose` tracked
triple-quoted spans across lines and threw the state away at EOF, so a span that
never closed blanked every line after it and the file scanned clean. `files_scanned`
still incremented, so neither read-source check noticed. That is this gate's only
failure mode — it ships with no per-line pragma, so silent blindness is the whole
risk surface — and the sibling `check-no-in-process-placement` already guards the
identical hazard for `~~` spans.

`blank_out_prose` now returns `Result`, and the Python arm refuses an unterminated
span naming the file. Two tests cover it: the blanker's own error, and a scan that
fails rather than reporting a file with a live `time.time_ns()` as clean.

Also from review:
- `SCAN_EXEMPT_FILES` replaces the single hardcoded gate-source path, so the next
  file that legitimately spells a banned pattern in source text is a reviewed list
  entry rather than a redesign.
- The allowlist-liveness check no longer builds a violation list and a String per
  hit to answer a boolean.
- Tests take the workspace root from `CARGO_MANIFEST_DIR` like the three sibling
  gates, instead of spawning `cargo locate-project` twice.
- The api-server name fallback drew both indices from a u64 whose high half is
  zero when seeded from a pid, pinning every degraded runtime to the same noun.
- Dropped three comments that narrated the change rather than explaining the code,
  per `.claude/rules/comments.md`.

The adapter-cuda test takes a `TempDir` rather than reaching into
`streamlib_engine::*` for the mint — `check-boundaries` rejects a consumer-tree
test importing engine internals directly, and a socket path wants the kernel's
uniqueness answer anyway.

Refs #1728
Review falsified the search that justified this module: `streamlib-engine` has
carried `uuid` with `v4` since before this branch, and `pubsub/integration_tests.rs`
already minted an iceoryx2 service name with `Uuid::new_v4()`. Adding a second
answer to "give me a unique service name" is the parallel abstraction the engine
doctrine exists to prevent, and the search that should have caught it was mine.

`mint_machine_global_unique_name_suffix()` is now `<pid>-<uuid>`. A v4 UUID's 122
random bits make every mint distinct across processes, runs and reboots on their
own, so the monotonic read and the sequence counter are both gone. That also
retires a hazard the composite carried: the monotonic component resets on reboot
while pids recycle, so a crash-left `/tmp` temp file plus a post-reboot pid
collision at a similar offset was a real, if unlikely, collision the old doc
claimed was impossible. The pid prefix stays, now honestly documented as
diagnostic only — it is what names the process that left a stale entry.

The one pre-existing service-name mint in `pubsub/integration_tests.rs` moves onto
the helper, so the tree has one idiom for this concern rather than two. The other
`Uuid::new_v4()` sites mint runtime ids and handle ids — a different concern, left
alone.

Review also showed the old `successive_mints_never_repeat` locked nothing: it
passed with the counter replaced by a constant and with the clock replaced by a
constant, because any one component makes 10k in-process mints distinct. Its
message asserted a premise the evidence disproved. The new
`the_uuid_component_is_what_differs_between_two_mints` strips the fixed pid prefix
and compares what is left, so it fails if the random component is ever dropped.

Refs #1728
Review planted four wall-clock spellings the gate walked past. The load-bearing
one: `libc::clock_gettime(libc::CLOCK_REALTIME, ..)`. The engine's own canonical
clock read is `libc::clock_gettime(libc::CLOCK_MONOTONIC, ..)` in
`core/media_clock.rs` — the file this gate's failure message points readers at —
so the likeliest way to introduce a wall-clock read here is to copy that line and
flip one token, which the gate could not see. `CLOCK_REALTIME` has zero hits in
the tree, so banning it in both language arms cannot false-positive. Python has
the mirror hole via `time.clock_gettime_ns(time.CLOCK_REALTIME)`, and the existing
`accepts_the_monotonic_spellings` test enshrined that call without ever reading
its clock-id argument. `UNIX_EPOCH.elapsed` joins them.

The other two evasions — a call split at the `::`, and a wall clock renamed at the
import — are the accepted floor of a substring scan, now stated in the module doc
next to the trailing-comment limit rather than left for the next reader to
discover.

`packages/test-fixtures` joins the scan roots. It is engine-side test
infrastructure compiled into the engine's own runs, not a consumer, and its 9
processors stamp data. It is clean today so the addition is free. The scan-root
doc now names why `packages/escalate` and `packages/core` stay out (schemas only,
no source to read) instead of implying the whole of `packages/` is downstream.

Also: the api-server's degraded naming path took both indices from disjoint halves
of a seed that is a bare pid on that path, and a pid is under 2^16 on any host with
the kernel-default `pid_max` — so `>> 32`, and then `>> 16`, both pinned every
degraded runtime to one noun. The seed is diffused through a hasher before
indexing, as it was before this branch touched it.

Corrects two facts in the change-file record: 16 sites span 15 files, not 13, and
the cuda adapter's round-trip test took a `TempDir` too.

Refs #1728
@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 36fd6aab-dd3e-48b8-8154-c74334ef5b09

📥 Commits

Reviewing files that changed from the base of the PR and between 1815158 and 999fdbf.

📒 Files selected for processing (5)
  • docs/plan/changes/one-monotonic-clock.md
  • runtime/streamlib-engine/src/core/machine_global_unique_name.rs
  • runtime/streamlib-engine/src/linux/surface_share/unix_socket_service.rs
  • runtime/streamlib-engine/tests/surface_share_subprocess_crash.rs
  • xtask/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • docs/plan/changes/one-monotonic-clock.md
  • xtask/src/main.rs

📝 Walkthrough

Walkthrough

The PR adds the check-clock-usage source gate, integrates it into the nine-gate workflow, replaces timestamp-based unique names, and gives Linux socket tests isolated temporary directories.

Changes

Clock policy enforcement

Layer / File(s) Summary
Clock usage scanner and validation
xtask/src/check_clock_usage.rs
The gate scans tracked Rust and Python sources, detects banned wall-clock APIs, applies four permitted observability surfaces, and validates scan coverage and allowlist entries.
Clock gate integration and documentation
xtask/src/main.rs, .github/workflows/source-gates.yml, docs/plan/changes/one-monotonic-clock.md, runtime/streamlib-engine/src/core/logging/event.rs
The CLI and unified source-gate runner execute the new check. Documentation describes nine gates, the final allowlist, and monotonic-clock parity tests.

Runtime uniqueness

Layer / File(s) Summary
Machine-global unique-name generation
runtime/streamlib-engine/src/core/machine_global_unique_name.rs, runtime/streamlib-engine/src/core/mod.rs, runtime/streamlib-api-server/processors/api_server.rs
The engine exposes a <pid>-<UUID> suffix generator. Runtime-name seeding uses CSPRNG bytes, with a process-ID fallback.
Timestamp-free service and cache names
runtime/streamlib-engine/benches/*, runtime/streamlib-engine/src/core/..., runtime/streamlib-engine/src/iceoryx2/*, runtime/streamlib-engine/src/vulkan/rhi/*
Service names, channel names, and Vulkan cache paths use the shared unique suffix helper.

Socket test isolation

Layer / File(s) Summary
Temporary socket directory ownership
adapters/streamlib-adapter-cuda/*, runtime/streamlib-surface-client/*, runtime/streamlib-engine/src/linux/surface_share/*, runtime/streamlib-engine/tests/*
Linux socket tests create private TempDir directories and retain their owners until test completion. Temporary directories and sockets are removed automatically.

Estimated code review effort: 4 (Complex) | ~45 minutes

Mergeability Score: 🔵 Low · up to 999fd

The PR replaces wall-clock-derived names with unique identifiers and adds enforcement, but temporary Unix-socket paths can still exceed Linux path limits under a long TMPDIR, causing bounded runtime or test failures. The change is mergeable with owner awareness and follow-up to use a bounded temporary root and add coverage.

Sequence Diagram(s)

sequenceDiagram
  participant Developer
  participant CheckClockUsage
  participant Git
  participant RustPythonSources
  Developer->>CheckClockUsage: run check-clock-usage
  CheckClockUsage->>Git: discover tracked Rust and Python files
  Git-->>CheckClockUsage: source paths
  CheckClockUsage->>RustPythonSources: scan wall-clock API usage
  RustPythonSources-->>CheckClockUsage: violations and scan metrics
  CheckClockUsage-->>Developer: pass or detailed failure report
Loading

Possibly related issues

  • tatolab/streamlib#1728 — The PR implements the issue’s clock-usage allowlist, migrations, documentation, and CI integration.
  • tatolab/streamlib#1726 — The PR updates the monotonic-clock plan and epoch-parity validation.
  • tatolab/streamlib#1727 — The PR updates the same monotonic-clock plan and epoch-unification work.

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the new CI gate that enforces the wall-clock allowlist.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/1728-check-clock-usage

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (3)
xtask/src/check_clock_usage.rs (2)

380-392: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Rust prose blanking ignores block comments.

blank_out_rust_prose blanks whole-line // only. A banned spelling inside /* ... */ is reported as a violation. The failure is loud, not silent, so the gate stays safe. Consider naming this limit in the module docs next to the trailing-comment note, so a future author knows to move such prose to // lines.

🤖 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 `@xtask/src/check_clock_usage.rs` around lines 380 - 392, Document the
limitation of blank_out_rust_prose: it only blanks whole-line // comments and
does not ignore prose inside /* ... */ block comments. Add this note to the
module documentation alongside the existing trailing-comment limitation, without
changing the function’s behavior.

400-450: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Trailing # comments can open a false Python prose span.

blank_out_python_prose skips a line only when the whole line starts with #. A trailing comment that contains one unbalanced ''' or """ opens a span and hides the rest of the file. The final ensure! then fails the gate, so the outcome is a refusal and not a silent pass. The message names the span but not the line, which makes the fix harder in a long file. Consider recording the opening line number and including it in the error.

♻️ Report the opening line
-    let mut open_quote: Option<&str> = None;
+    let mut open_quote: Option<&str> = None;
+    let mut opened_at_line: usize = 0;
 ...
                         Some((at, quote)) => {
                             code.push_str(&rest[..at]);
                             open_quote = Some(quote);
+                            opened_at_line = code_lines.len() + 1;
                             rest = &rest[at + quote.len()..];
                         }
 ...
     anyhow::ensure!(
         open_quote.is_none(),
-        "unterminated triple-quoted span — every line after it would be hidden from \
-         check-clock-usage"
+        "unterminated triple-quoted span opened on line {opened_at_line} — every line after \
+         it would be hidden from check-clock-usage"
     );
🤖 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 `@xtask/src/check_clock_usage.rs` around lines 400 - 450, Update
blank_out_python_prose to record the source line number when opening a
triple-quoted span, including spans found in trailing comments. Use that
recorded line in the unterminated-span error from the final ensure!, while
preserving the existing detection and refusal behavior.
runtime/streamlib-engine/src/core/execution/thread_runner.rs (1)

583-586: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use explicit names for complete test service names.

The three changed helpers return complete test/.../{pid}-{uuid} service names but use the name unique_suffix.

  • runtime/streamlib-engine/src/core/execution/thread_runner.rs#L583-L586: rename unique_suffix to unique_thread_runner_service_name.
  • runtime/streamlib-engine/src/iceoryx2/input.rs#L689-L693: rename unique_suffix to unique_input_test_service_name.
  • runtime/streamlib-engine/src/iceoryx2/output.rs#L503-L508: rename unique_suffix to unique_output_test_service_name and change “service-name prefix” to “service name”.

As per coding guidelines, names must pass the zero-context test: LinkOutputDataWriter, never Writer. Explicit beats short.

🤖 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 `@runtime/streamlib-engine/src/core/execution/thread_runner.rs` around lines
583 - 586, Rename the complete service-name helpers and update all call sites:
in runtime/streamlib-engine/src/core/execution/thread_runner.rs:583-586 use
unique_thread_runner_service_name; in
runtime/streamlib-engine/src/iceoryx2/input.rs:689-693 use
unique_input_test_service_name; and in
runtime/streamlib-engine/src/iceoryx2/output.rs:503-508 use
unique_output_test_service_name. In the output helper’s documentation, change
“service-name prefix” to “service name.”

Source: Coding guidelines

🤖 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 `@runtime/streamlib-engine/src/core/machine_global_unique_name.rs`:
- Around line 12-16: Update the documentation near the UUID generation logic to
describe v4 UUID uniqueness as probabilistic rather than guaranteed. In the
regression test, parse both UUID components and assert that each has UUID
version 4, so malformed or timestamp-based values fail explicitly.

In `@runtime/streamlib-engine/tests/surface_share_subprocess_crash.rs`:
- Around line 51-53: Update the socket-path construction in the subprocess crash
test to keep the resulting Unix socket path within Linux’s 108-byte limit,
either by using a shorter temporary-directory root or bounding the generated
path length. Add test coverage that sets a long TMPDIR and verifies
service.start() succeeds.

In `@xtask/src/main.rs`:
- Around line 268-278: Update the CheckClockUsage help text in the command
definition to include packages/test-fixtures alongside the existing scan roots,
so the --help description matches all entries in SCAN_ROOTS.

---

Nitpick comments:
In `@runtime/streamlib-engine/src/core/execution/thread_runner.rs`:
- Around line 583-586: Rename the complete service-name helpers and update all
call sites: in
runtime/streamlib-engine/src/core/execution/thread_runner.rs:583-586 use
unique_thread_runner_service_name; in
runtime/streamlib-engine/src/iceoryx2/input.rs:689-693 use
unique_input_test_service_name; and in
runtime/streamlib-engine/src/iceoryx2/output.rs:503-508 use
unique_output_test_service_name. In the output helper’s documentation, change
“service-name prefix” to “service name.”

In `@xtask/src/check_clock_usage.rs`:
- Around line 380-392: Document the limitation of blank_out_rust_prose: it only
blanks whole-line // comments and does not ignore prose inside /* ... */ block
comments. Add this note to the module documentation alongside the existing
trailing-comment limitation, without changing the function’s behavior.
- Around line 400-450: Update blank_out_python_prose to record the source line
number when opening a triple-quoted span, including spans found in trailing
comments. Use that recorded line in the unterminated-span error from the final
ensure!, while preserving the existing detection and refusal behavior.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5326ea4f-7778-4870-9492-7c8c0574e2e3

📥 Commits

Reviewing files that changed from the base of the PR and between fd44620 and 1815158.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (25)
  • .github/workflows/source-gates.yml
  • adapters/streamlib-adapter-cuda/Cargo.toml
  • adapters/streamlib-adapter-cuda/tests/opaque_fd_consumer_rhi_round_trip.rs
  • docs/plan/changes/one-monotonic-clock.md
  • runtime/streamlib-api-server/processors/api_server.rs
  • runtime/streamlib-engine/benches/output_writer_ffi_hop.rs
  • runtime/streamlib-engine/src/apple/time.rs
  • runtime/streamlib-engine/src/core/compiler/compiler_ops/open_iceoryx2_service_op.rs
  • runtime/streamlib-engine/src/core/execution/thread_runner.rs
  • runtime/streamlib-engine/src/core/logging/event.rs
  • runtime/streamlib-engine/src/core/machine_global_unique_name.rs
  • runtime/streamlib-engine/src/core/mod.rs
  • runtime/streamlib-engine/src/core/pubsub/integration_tests.rs
  • runtime/streamlib-engine/src/core/runtime/tap.rs
  • runtime/streamlib-engine/src/iceoryx2/input.rs
  • runtime/streamlib-engine/src/iceoryx2/node.rs
  • runtime/streamlib-engine/src/iceoryx2/output.rs
  • runtime/streamlib-engine/src/linux/surface_share/unix_socket_service.rs
  • runtime/streamlib-engine/src/vulkan/rhi/vulkan_compute_kernel.rs
  • runtime/streamlib-engine/src/vulkan/rhi/vulkan_graphics_kernel.rs
  • runtime/streamlib-engine/tests/surface_share_subprocess_crash.rs
  • runtime/streamlib-surface-client/Cargo.toml
  • runtime/streamlib-surface-client/src/linux.rs
  • xtask/src/check_clock_usage.rs
  • xtask/src/main.rs
💤 Files with no reviewable changes (1)
  • runtime/streamlib-engine/src/apple/time.rs

Comment thread runtime/streamlib-engine/src/core/machine_global_unique_name.rs Outdated
Comment thread runtime/streamlib-engine/tests/surface_share_subprocess_crash.rs Outdated
Comment thread xtask/src/main.rs
CodeRabbit caught a regression this branch introduced. `sun_path` is 108 bytes,
and moving these paths onto `<pid>-<uuid>` cost 18 of them: headroom under a
default `/tmp` fell from 31 bytes to 14, so a `TMPDIR` longer than 15 bytes now
fails a bind that used to fit in 46. CI is unaffected — Actions leaves `TMPDIR`
unset — but the failure it would produce is an opaque bind error, not a message
naming the cause.

Both remaining socket sites take a `TempDir`, which is what the surface-client
and cuda-adapter tests already did: the path is ~35 bytes with 72 to spare, the
socket is unlinked on drop, and uniqueness is the kernel's answer rather than a
name we compose. That leaves one idiom for a test socket path across all four
sites, and the mint keeps the two namespaces that have no length limit —
iceoryx2 service names and pipeline-cache file names.

Also from that review:
- The mint's docs claimed a collision was impossible. 122 random bits make one
  vanishingly improbable, which is a different and true statement.
- The version test asserted only that the suffix parses, so a timestamp-based
  UUID would have satisfied it — and a timestamp-based UUID is a wall-clock read,
  the one thing this ticket exists to keep out of the tree. It now asserts v4.
  The `uuid` dependency enabling only `v4` is the guard that fires first, via the
  compiler; this is the one that survives another feature being switched on.
- `--help` for `check-clock-usage` listed four scan roots, not five.

Refs #1728
@tato123
tato123 merged commit 1494b08 into main Aug 13, 2026
9 of 10 checks passed
@tato123
tato123 deleted the feat/1728-check-clock-usage branch August 13, 2026 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(ci): check-clock-usage — enforce the wall-clock allowlist

1 participant