Skip to content

Sync fork to upstream v0.91.1 (drop re-add GHA SHA pins, merge 353 upstream commits) - #6

Open
ajbt200128 wants to merge 357 commits into
mainfrom
austin/sync-upstream
Open

Sync fork to upstream v0.91.1 (drop re-add GHA SHA pins, merge 353 upstream commits)#6
ajbt200128 wants to merge 357 commits into
mainfrom
austin/sync-upstream

Conversation

@ajbt200128

@ajbt200128 ajbt200128 commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

this reverts leif's github sha pinning commits, pulls in upstream, then re-applies the sha pinning

c-cube added 30 commits January 20, 2026 00:15
this should result in lower overhead for single threaded situations such
as lwt or eio.
for shutdown processes, it's really preferable to use level-triggered
primitives rather than edge-triggered callbacks. Switch is fairly
robust. It's named Aswitch here, "A" means atomic and is also used to
avoid name collision with Eio.

Util_atomic provides a convenience CAS loop, with backoff.
c-cube and others added 13 commits April 10, 2026 15:09
…metrics

we can now know how big the batches we drop are
do not look for an ambient trace ID if parent is explicitly set to none!
in case of a program that forks worker subprocesses, eager
initialization means each subprocess starts off with the same
initialized state. Instead we do this lazily and each subprocess will
get its own state.
This reverts commit 53199e3.
Brings in the 353 upstream commits since `refactor: move the Mutex.protect
backport into `Util_mutex``, up to `prepare for 0.91.1`.
@CLAassistant

CLAassistant commented Sep 2, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 4 committers have signed the CLA.

✅ ajbt200128
❌ ushitora-anqou
❌ c-cube
❌ raphael-proust
You have signed the CLA already but the status is still pending? Let us recheck it.

let delete = ignore

let wait self ~should_keep_waiting =
Mutex.lock self.mutex;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Semgrep identified an issue in your code:

wait can leave self.mutex locked when the wait predicate or condition wait raises, causing subsequent callers to deadlock.

More details about this

wait acquires self.mutex with Mutex.lock and releases it only after the while should_keep_waiting () loop. If should_keep_waiting () or Condition.wait self.cond self.mutex raises an exception, execution skips Mutex.unlock self.mutex, leaving the mutex locked and potentially deadlocking every later caller that waits on this queue condition.

To resolve this comment:

✨ Commit fix suggestion
  1. Replace the manual lock/unlock sequence in wait with Mutex.protect.
  2. Move the loop into the protected function: Mutex.protect self.mutex (fun () -> while should_keep_waiting () do Condition.wait self.cond self.mutex done).
  3. Remove the separate Mutex.unlock self.mutex call. Mutex.protect releases self.mutex both on normal completion and when should_keep_waiting or Condition.wait raises an exception.
💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by mutex-lock-exn.

You can view more details about this finding in the Semgrep AppSec Platform.

Comment thread src/lib/trace_provider.ml
with e ->
let bt = Printexc.get_raw_backtrace () in
finally (Error (e, bt));
raise e

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Semgrep identified an issue in your code:

raise e discards the original backtrace captured for the exception from thunk (), so callers may see a misleading stack trace rooted in this handler.

More details about this

The with e -> handler correctly captures the exception’s raw backtrace in bt and passes it to finally (Error (e, bt)), but raise e then re-raises e without that backtrace. The caller therefore receives a stack trace rooted at this re-raise site rather than the original failure in thunk (), making the exception harder to diagnose and potentially hiding the operation that actually failed.

To resolve this comment:

✨ Commit fix suggestion

Suggested change
raise e
Printexc.raise_with_backtrace e bt
View step-by-step instructions
  1. Keep capturing the raw backtrace immediately in the exception handler, before calling finally.
  2. Replace raise e with Printexc.raise_with_backtrace e bt:
    with e ->
      let bt = Printexc.get_raw_backtrace () in
      finally (Error (e, bt));
      Printexc.raise_with_backtrace e bt
  3. Preserve the existing bt value when reporting the error to finally, so the exception is re-raised with its original stack trace.
💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by bad-reraise.

You can view more details about this finding in the Semgrep AppSec Platform.

Comment thread src/core/span_ctx.ml

let[@inline] sampled self = self.flags land (1 lsl Flags.sampled) != 0

let[@inline] is_remote self = self.flags land (1 lsl Flags.remote) != 0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Semgrep identified an issue in your code:

is_remote uses physical inequality for an integer bitmask check, which can misclassify whether the remote flag is set. The comparison should use structural integer semantics.

More details about this

is_remote uses OCaml’s physical inequality operator != when comparing the integer result of self.flags land (1 lsl Flags.remote) with 0. This check is intended to test the numeric value of the remote flag bit; using physical inequality makes the comparison depend on value representation rather than integer structure and can produce incorrect flag detection. The same comparison pattern also appears in sampled, so the flag-state checks are inconsistent with their intended numeric semantics.

To resolve this comment:

✨ Commit fix suggestion
  1. Replace the physical inequality operator != with the structural inequality operator <> in is_remote:
    let[@inline] is_remote self = self.flags land (1 lsl Flags.remote) <> 0
  2. Verify that the comparison remains against the integer literal 0; <> correctly compares the computed integer value with zero.
💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by physical-not-equal.

You can view more details about this finding in the Semgrep AppSec Platform.

Comment thread src/domain/gen.ml
|}

let write_file file s =
let oc = open_out file in

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Semgrep identified an issue in your code:
'open_out' behaves differently on Windows and on Unix-like systems with respect to line endings. To get the same behavior everywhere, use 'open_out_bin' or 'open_out_gen [Open_binary]'. If you really want LF-to-CRLF translations to take place when running on Windows, use 'open_out_gen [Open_text]'.

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by prefer-write-in-binary-mode.

You can view more details about this finding in the Semgrep AppSec Platform.

@@ -0,0 +1,23 @@
let copy_file src dst =
let ic = open_in src in
let oc = open_out dst in

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Semgrep identified an issue in your code:
'open_out' behaves differently on Windows and on Unix-like systems with respect to line endings. To get the same behavior everywhere, use 'open_out_bin' or 'open_out_gen [Open_binary]'. If you really want LF-to-CRLF translations to take place when running on Windows, use 'open_out_gen [Open_text]'.

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by prefer-write-in-binary-mode.

You can view more details about this finding in the Semgrep AppSec Platform.

@@ -0,0 +1,23 @@
let copy_file src dst =
let ic = open_in src in

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Semgrep identified an issue in your code:
'open_in' behaves differently on Windows and on Unix-like systems with respect to line endings. To get the same behavior everywhere, use 'open_in_bin' or 'open_in_gen [Open_binary]'. If you really want CRLF-to-LF translations to take place when running on Windows, use 'open_in_gen [Open_text]'.

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by prefer-read-in-binary-mode.

You can view more details about this finding in the Semgrep AppSec Platform.

Comment thread .github/workflows/main.yml Outdated
steps:
- name: Checkout code
uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0
- uses: actions/checkout@v6

@semgrep-zcs-prod-semgrep semgrep-zcs-prod-semgrep Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

GitHub Actions step uses a mutable tag or branch reference. Tags and branch names can be silently repointed by the action owner, enabling supply-chain attacks — as seen in the trivy-action and kics-github-action compromises. Pin the reference to a full 40-character commit SHA instead, e.g. uses: actions/checkout@8ade135a41bc03ea155e62e844d188df1ea18608.

🎈 Fixed in commit 7f34132 🎈

Comment thread .github/workflows/gh-pages.yml Outdated
Comment thread .github/workflows/format.yml Outdated
Comment thread .github/workflows/gh-pages.yml Outdated
@semgrep-zcs-prod-semgrep

Copy link
Copy Markdown

Semgrep found 5 ocamllint-unsafe findings:

Unsafe functions do not perform boundary checks or have other side effects, use with care.

Restores SHA pinning on top of the upstream v0.91.1 sync, per Semgrep's
org-wide policy. Kept as a separate commit on top of the merge so the next
upstream sync can revert just this one instead of conflicting on every
workflow file.

Pins the versions upstream declares rather than bumping them:
  actions/checkout             v6 -> d23441a4 # v6.1.0
  peaceiris/actions-gh-pages   v3 -> 373f7f26 # v3.9.3

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ajbt200128

Copy link
Copy Markdown
Collaborator Author

⚠️ CI is red, and it is not the code

All build jobs and format fail in ~12s at Initialize containers, before any OCaml is compiled:

docker pull ghcr.io/ocaml-tracing/ocaml-opentelemetry/ci-4.14:latest
Error response from daemon: denied

Root cause: upstream reworked CI in 5031be8d ("use docker images for CI") to run every job inside container: ghcr.io/ocaml-tracing/ocaml-opentelemetry/ci-<version>:latest. Those packages are private to the ocaml-tracing org — an anonymous manifest fetch returns HTTP 403 — so this fork's GITHUB_TOKEN, which is scoped to semgrep/ocaml-opentelemetry, cannot pull them.

Evidence this is not a regression from this PR: upstream's own CI is green on the exact same commit 3b403aaf (build 4.08 / 4.14 / 5.4, format, deploy all success), because it runs inside the org that owns the images. The tree here is identical to that commit apart from the three SHA-pin lines.

So the sync itself is fine; the fork just has no way to run upstream's new container-based CI as-is. Options, roughly in order of how little divergence they reintroduce:

  1. Ask the ocaml-tracing maintainers to make those GHCR packages public — fixes it for every fork, no local patch.
  2. Mirror/build the CI images into semgrep's own GHCR and override container: here.
  3. Keep a fork-local workflow using ocaml/setup-ocaml instead of containers.

Flagging for a decision rather than picking one, since each reintroduces a different amount of the fork divergence this PR just removed.

@ajbt200128 ajbt200128 changed the title Sync fork to upstream v0.91.1 (drop GHA SHA pins, merge 353 upstream commits) Sync fork to upstream v0.91.1 (drop re-add GHA SHA pins, merge 353 upstream commits) Sep 2, 2026
@ajbt200128
ajbt200128 requested a review from liukatkat September 2, 2026 21:20
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.

6 participants