Skip to content

[DB-2193] Remove UnbufferedFileStream and replace its usage with a regular pooled stream - #5687

Merged
Inok merged 2 commits into
masterfrom
pavel/remove-unbufferedstream
Jul 30, 2026
Merged

[DB-2193] Remove UnbufferedFileStream and replace its usage with a regular pooled stream#5687
Inok merged 2 commits into
masterfrom
pavel/remove-unbufferedstream

Conversation

@Inok

@Inok Inok commented Jul 22, 2026

Copy link
Copy Markdown
Member

UnbufferedFileStream attempts to do direct IO (NO_BUFFERING / O_DIRECT). However:

  • It's used in a single place (PTable verification when it's opened), and only on Windows.
  • In that place, replacing it with the Unix's branch (regular FileStream) actually makes loading PTable 2x faster
  • The internals are quite tricky, with a lot of old-style unsafe code, alignment handling, interop and inefficient pointer-based file reads.
  • There are a few problems in interop with method signatures, error handling, etc.

So I switched PTable loading to the universal (Unix) path and removed this custom stream with all its internals.

@Inok
Inok force-pushed the pavel/remove-unbufferedstream branch 3 times, most recently from c4bd3ac to 9ce910e Compare July 23, 2026 10:19
@Inok Inok self-assigned this Jul 23, 2026
@Inok
Inok force-pushed the pavel/remove-unbufferedstream branch from 9ce910e to 53b9705 Compare July 23, 2026 10:43
@Inok
Inok marked this pull request as ready for review July 23, 2026 11:03
@Inok
Inok requested review from a team as code owners July 23, 2026 11:03
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Remove UnbufferedFileStream and use pooled FileStream for PTable verification

✨ Enhancement ⚙️ Configuration changes 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Switch PTable verification to the pooled FileStream path on all platforms for simpler, faster IO.
• Remove the custom UnbufferedFileStream/native direct-IO implementation and related
 solution/project wiring.
• Update disk IO tests to drop file cache via posix_fadvise (Unix) and read via RandomAccess.
Diagram

graph TD
  PTable["PTable verification"] --> Pool["WorkItem pool"] --> PooledFS["Pooled FileStream"] --> PTableFile[("PTable file")]
  ProcTest["ProcessStatsTests"] --> Handle["OpenHandle + RandomAccess"] --> TmpFile[("Temp file")]
  Handle --> Advise{{"posix_fadvise"}} --> TmpFile

  subgraph Legend
    direction LR
    _c["Component"] ~~~ _f[("File")] ~~~ _s{{"Syscall"}}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Keep UnbufferedFileStream but fix/modernize it
  • ➕ Preserves true direct-IO behavior (NO_BUFFERING/O_DIRECT) where it helps.
  • ➕ Avoids any behavior changes in callers relying on direct-IO semantics.
  • ➖ High ongoing maintenance cost (unsafe code, alignment rules, interop correctness).
  • ➖ Hard to make portable and testable across OS/filesystems; easy to regress.
  • ➖ This PR’s measurements indicate it was slower in the primary usage (PTable open).
2. Use platform APIs directly in the one call site (Windows FILE_FLAG_NO_BUFFERING)
  • ➕ Keeps direct-IO experimentation localized to PTable verification only.
  • ➕ Avoids carrying a full native-IO abstraction project.
  • ➖ Still requires careful alignment/size rules and complex error handling.
  • ➖ Ends up reintroducing the complexity this PR removes, for uncertain benefit.

Recommendation: Proceed with this PR’s approach: always use the pooled FileStream work item for PTable verification and delete the bespoke unbuffered stream/native project. The direct-IO implementation is complex and low-leverage (single caller), and the simplified path both improves performance in the measured scenario and reduces correctness risk from unsafe/interop code.

Files changed (9) +39 / -50

Refactor (3) +3 / -19
PTable.csAlways use pooled stream for PTable hash verification +3/-17

Always use pooled stream for PTable hash verification

• Removes the Windows-only UnbufferedFileStream branch and uses the existing pooled WorkItem FileStream path universally. Simplifies cleanup by always returning the work item to the pool.

src/KurrentDB.Core/Index/PTable.cs

IndexReader.csRemove unused TransactionLog using +0/-1

Remove unused TransactionLog using

• Cleans up a now-unused TransactionLog namespace import after removing unbuffered/native IO references.

src/KurrentDB.Core/Services/Storage/ReaderIndex/IndexReader.cs

ReaderExtensions.csRemove unused TransactionLog using +0/-1

Remove unused TransactionLog using

• Removes an unused TransactionLog import to keep compilation clean and reflect the removed unbuffered IO code paths.

src/KurrentDB.SecondaryIndexing/Indexes/ReaderExtensions.cs

Tests (1) +33 / -21
ProcessStatsTests.csReplace UnbufferedFileStream usage with posix_fadvise + RandomAccess +33/-21

Replace UnbufferedFileStream usage with posix_fadvise + RandomAccess

• Refactors the test to write/read a temp file using File.OpenHandle + RandomAccess.Read. On Unix, calls posix_fadvise(DONTNEED) to reduce cache effects before reading, then asserts disk IO counters increased (with OSX ops caveat).

src/KurrentDB.SystemRuntime.Tests/ProcessStatsTests.cs

Documentation (1) +0 / -1
NOTICE.mdRemove Mono.Posix.NETStandard from NOTICE +0/-1

Remove Mono.Posix.NETStandard from NOTICE

• Removes the Mono.Posix.NETStandard entry from the third-party notice list, consistent with it no longer being part of the shipped/native component set.

NOTICE.md

Other (4) +3 / -9
KurrentDB.slnxRemove KurrentDB.Native project from the solution +0/-1

Remove KurrentDB.Native project from the solution

• Drops the KurrentDB.Native .csproj from the solution project list, reflecting the removal of the native direct-IO implementation.

KurrentDB.slnx

qodana.yamlRemove Mono.Posix.NETStandard license override +0/-7

Remove Mono.Posix.NETStandard license override

• Deletes the Qodana dependency license override for Mono.Posix.NETStandard since it’s no longer a product dependency in the removed native project.

qodana.yaml

KurrentDB.Core.csprojRemove reference to KurrentDB.Native from Core +0/-1

Remove reference to KurrentDB.Native from Core

• Removes the project reference to KurrentDB.Native, eliminating the transitive dependency on the deleted native direct-IO subsystem.

src/KurrentDB.Core/KurrentDB.Core.csproj

KurrentDB.SystemRuntime.Tests.csprojAdd Mono.Posix.NETStandard to SystemRuntime tests +3/-0

Add Mono.Posix.NETStandard to SystemRuntime tests

• Adds Mono.Posix.NETStandard as a test-only dependency to support Unix syscall interop used in ProcessStats tests.

src/KurrentDB.SystemRuntime.Tests/KurrentDB.SystemRuntime.Tests.csproj

@qodo-code-review

qodo-code-review Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Stale slnf project reference ✓ Resolved 🐞 Bug ≡ Correctness
Description
KurrentDB.Native was removed/deleted, but KurrentDB.ContainerTests.slnf still references
src\\KurrentDB.Native\\KurrentDB.Native.csproj, which can break CI/tooling that loads/builds using
that solution filter. This mismatch is introduced by the project removal in this PR without updating
the filter.
Code

KurrentDB.slnx[106]

-  <Project Path="src/KurrentDB.Native/KurrentDB.Native.csproj" />
Evidence
The main solution no longer includes KurrentDB.Native, but the container test solution filter still
lists it as a project, so the filter can reference a non-existent project file.

KurrentDB.slnx[96-109]
KurrentDB.ContainerTests.slnf[2-45]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`KurrentDB.ContainerTests.slnf` still lists the deleted `src/KurrentDB.Native/KurrentDB.Native.csproj`, so loading/building the solution filter can fail after this PR.

### Issue Context
The PR removes `KurrentDB.Native` from the main solution and deletes the project, but doesn’t update the container-tests solution filter.

### Fix Focus Areas
- KurrentDB.ContainerTests.slnf[2-45]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Mono.Posix metadata mismatch ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The PR removes Mono.Posix.NETStandard from NOTICE.md and from qodana.yaml dependencyOverrides, but
adds Mono.Posix.NETStandard as a direct PackageReference in KurrentDB.SystemRuntime.Tests, which can
cause NOTICE/license-audit CI failures. This leaves license metadata inconsistent with the
solution’s restored dependency set.
Code

qodana.yaml[L51-57]

-  # the referenced license file is not trivial but appears to imply that this library is MIT
-  - name: "Mono.Posix.NETStandard"
-    version: "1.0.0"
-    licenses:
-      - key: "MIT"
-        url: "https://github.com/mono/mono/blob/main/LICENSE"
-
Evidence
The test project explicitly references Mono.Posix, while the repository’s license metadata files no
longer include a corresponding entry/override (and the NOTICE section where it would appear
alphabetically no longer lists it).

src/KurrentDB.SystemRuntime.Tests/KurrentDB.SystemRuntime.Tests.csproj[2-12]
qodana.yaml[21-56]
NOTICE.md[116-123]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Mono.Posix.NETStandard` is now a direct dependency of `KurrentDB.SystemRuntime.Tests`, but the PR removed the corresponding entries from `NOTICE.md` and the Qodana `dependencyOverrides`, creating an inconsistency that can break license-audit/NOTICE validation.

### Issue Context
The package appears to have been removed along with `KurrentDB.Native`, but it is reintroduced in this PR for tests.

### Fix Focus Areas
- src/KurrentDB.SystemRuntime.Tests/KurrentDB.SystemRuntime.Tests.csproj[2-9]
- qodana.yaml[28-56]
- NOTICE.md[116-123]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

3. Flaky Linux disk IO test ✓ Resolved 🐞 Bug ☼ Reliability
Description
ProcessStatsTests asserts Linux physical disk counters (from /proc/self/io read_bytes/write_bytes)
increased, but attempts to force physical reads only via posix_fadvise(DONTNEED), which is advisory
and environment/filesystem-dependent. This can make the test intermittently fail on Linux runners
(e.g., cache not evicted, tmpfs/overlayfs semantics).
Code

src/KurrentDB.SystemRuntime.Tests/ProcessStatsTests.cs[R54-69]

+	private static string ReadAllText(string filePath) {
+		using var handle = File.OpenHandle(filePath);
+
+		if (RuntimeInformation.IsUnix) {
+			// reset cache for this file, so the OS really reads it from disk
+			int r;
+			do {
+				r = Syscall.posix_fadvise(handle.DangerousGetHandle().ToInt32(), 0, 0, PosixFadviseAdvice.POSIX_FADV_DONTNEED);
+			} while (UnixMarshal.ShouldRetrySyscall(r));
+			UnixMarshal.ThrowExceptionForLastErrorIf(r);
+		}
+
+		Span<byte> buffer = stackalloc byte[128];
+		var read = RandomAccess.Read(handle, buffer, 0);
+		return Encoding.UTF8.GetString(buffer[..read]);
+	}
Evidence
The production code’s Linux implementation uses /proc/self/io physical counters, while the test
performs a normal buffered read and relies on a best-effort cache hint before asserting those
physical counters increased.

src/KurrentDB.SystemRuntime/Diagnostics/ProcessStats.cs[23-51]
src/KurrentDB.SystemRuntime.Tests/ProcessStatsTests.cs[35-69]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test tries to validate `ProcessStats.GetDiskIoLinux()` using assertions on `ReadBytes`/`WrittenBytes`, but its cache-avoidance mechanism (`posix_fadvise(...DONTNEED)` + buffered read) is not deterministic across Linux environments.

### Issue Context
`GetDiskIoLinux()` reads `/proc/self/io`’s `read_bytes`/`write_bytes` (physical I/O). Buffered reads may not increment `read_bytes` if served from page cache.

### Fix Focus Areas
- src/KurrentDB.SystemRuntime.Tests/ProcessStatsTests.cs[35-69]
- src/KurrentDB.SystemRuntime/Diagnostics/ProcessStats.cs[23-51]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Comment thread KurrentDB.slnx
Comment thread qodana.yaml
Comment thread src/KurrentDB.SystemRuntime.Tests/ProcessStatsTests.cs
@Inok
Inok force-pushed the pavel/remove-unbufferedstream branch from 53b9705 to fede545 Compare July 23, 2026 11:22
@Inok
Inok force-pushed the pavel/remove-unbufferedstream branch from 28d7adb to cf29a9b Compare July 23, 2026 12:42
@sakno

sakno commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Let's wait for @timothycoleman for extra approval

@Inok
Inok requested a review from timothycoleman July 28, 2026 10:35
@linear-code

linear-code Bot commented Jul 29, 2026

Copy link
Copy Markdown

DB-2193

@Inok Inok changed the title Remove UnbufferedFileStream and replace its usage with a regular pooled stream [DB-2193] Remove UnbufferedFileStream and replace its usage with a regular pooled stream Jul 29, 2026
@Inok
Inok merged commit 78bb64d into master Jul 30, 2026
134 of 135 checks passed
@Inok
Inok deleted the pavel/remove-unbufferedstream branch July 30, 2026 16: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.

3 participants