Потокобезопасный перенос двоичных данных в память

[sfaqer]

ДвоичныеДанные больше binaryData.inMemoryMaxSize лежат во временном файле, а первое обращение к Buffer (Base64Строка, ПолучитьБуферДвоичныхДанныхИзДвоичныхДанных, XMLСтрока и т.п.) переносит их в память и удаляет файл — без блокировки. Если одно значение читают несколько заданий, часть падает с NullReferenceException / UnauthorizedAccessException на временном файле, часть молча получает неверные данные (на 1200 заданиях — 380 ошибок и 41 неверный результат). Теперь перенос идет под блокировкой, массив публикуется только заполненным, а файл открывается под той же блокировкой.

Заодно ПолучитьБуферДвоичныхДанныхИзДвоичныхДанных и ПолучитьДвоичныеДанныеИзБуфераДвоичныхДанных копируют массив: раньше буфер и двоичные данные делили его, и запись в буфер меняла «неизменяемые» двоичные данные.

Тесты: BinaryDataThreadSafetyTests в StandardLibrary.Tests и два теста в tests/BinaryData-global.os.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability when reading large binary data from multiple threads.
  • Changes to a binary data buffer no longer affect the original data or other buffers created from it, and changes to a buffer no longer affect binary data created from that buffer.
• Tests
  • Added coverage for concurrent reads and for keeping binary data and buffers independent.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: ff507c85-ceab-480d-a9c4-cfa1ddc5cd9a

📥 Commits

Reviewing files that changed from the base of the PR and between 36e2ca9 and 8923ac8.

📒 Files selected for processing (4)

• src/OneScript.StandardLibrary/Binary/BinaryDataContext.cs
• src/OneScript.StandardLibrary/Binary/GlobalBinaryData.cs
• src/Tests/OneScript.StandardLibrary.Tests/BinaryDataThreadSafetyTests.cs
• tests/BinaryData-global.os

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

📝 Walkthrough

Walkthrough

The changes synchronize access to file-backed BinaryDataContext data and clone byte arrays during conversions between binary data and buffers. Tests cover concurrent reads and array independence.

Changes

File-backed BinaryDataContext access

Layer / File(s)	Summary
Buffer storage and publication src/OneScript.StandardLibrary/Binary/BinaryDataContext.cs	_buffer is volatile and indicates in-memory storage. Small streams assign the buffer returned by LoadToBuffer.
File-backed reads and synchronization src/OneScript.StandardLibrary/Binary/BinaryDataContext.cs, src/Tests/OneScript.StandardLibrary.Tests/BinaryDataThreadSafetyTests.cs	Buffer, GetStream, CopyTo, Size, and ToString use a buffer snapshot or access the backing file. Backing-file reads and disposal use a lock. The test exercises concurrent reads through Buffer and GetStream.

Binary-data conversion array isolation

Layer / File(s)	Summary
Clone arrays during conversion src/OneScript.StandardLibrary/Binary/GlobalBinaryData.cs, tests/BinaryData-global.os	Both conversion directions clone the byte array. Two registered tests check that modifying a source or converted array does not change the other object.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 8923a

The changes synchronize file-backed reads and isolate conversion arrays. The reported size race is not present; the PR is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 8923a

The changes strengthen concurrent read safety and prevent converted buffers from modifying their source data. No introduced security concern was substantiated. Remaining uncertainty concerns external callers and failure behavior not directly exercised by the added tests.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The demonstrated integrity boundary is shared binary content within a process: concurrent readers of one context and callers converting between immutable-intended data and mutable buffers. File access continues under the hosting process's existing authority. The reviewed changes do not establish new tenant, service or credential reachability.

Trust Boundaries and Controls

• observed — Both conversion entrypoints now clone before construction, removing the previous route by which mutation of a converted buffer could alter its source binary data, or subsequent source-buffer mutation could alter converted binary data.

Resilience and Maintainability Implications

• inferred — Using one stable lock for file acquisition, materialization and disposal reduces race-induced corruption and read failures for shared values. This is a data-integrity improvement, not an authentication or sandbox control, and it does not make concurrent direct mutation of exposed arrays safe.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 3 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	Заголовок точно описывает основное изменение: потокобезопасный перенос двоичных данных из временного файла в память.
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.

Full details: Docstring Coverage

Explanation

Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 3 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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