Skip to content

AsyncFileIO: propagate save/load failures to callers and log them (#453) - #1027

Open
parthod0x wants to merge 4 commits into
qubic:developfrom
parthod0x:fix/file-io-error-propagation
Open

parthod0x wants to merge 4 commits into
qubic:developfrom
parthod0x:fix/file-io-error-propagation

Conversation

@parthod0x

@parthod0x parthod0x commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Implements the error-propagation part of #453.

Problem

flushIO() / flushIOMaxPriorityItem() run the real save()/load() but discard the result, and the blocking paths (asyncBlockingSave, blocking asyncLoad) always return (long long)totalSize. So a failed write/read looks like success to the caller — and the existing checks in virtual_memory.h (if (sz != pageSize) …) and the ant-colony export (if (saved != totalBytes) return false) are effectively dead code.

This PR (additive, no threading/queue changes)

  • Record the real save()/load() result on the FileItem (mResult) in both flush paths, and log failed reads/writes with the file name (always-on, via logToConsole).
  • Return mResult from the blocking paths instead of totalSize, so callers see the real outcome; a stop during the wait now returns kStop rather than a false success.

No caller changes are needed for virtual-memory paging or the ant-colony export — their existing result checks simply start working (save()/load() already return <0 on failure).

Mapping to the #453 checklist

  • Pass the real save()/load() result back to callers.
  • Log failed reads and writes with the file name.
  • Make the existing result checks work (VM paging, ant-colony export) — they do, unchanged.
  • Unit tests for a failed load/save — added to test/file_io.cpp: AsyncBlockingLoadMissingFilePropagatesFailure (blocking load of a missing file) and AsyncBlockingSaveFailurePropagatesFailure (blocking save to an unopenable path). Both assert the blocking result is now < 0. The stop-during-wait → kStop case is noted in a comment as not deterministically reachable through the public API (mIsStop is private and only set by deInit(), which also flushes and frees gAsyncFileIO), so it's left to inspection rather than a racy test.
  • End-of-epoch / revenue saves can gain an explicit check now that the result is meaningful — happy to add.
  • Testnet failure-simulation (missing / read-only dir) — needs the node environment; I'd coordinate with you on that.

@cyber-pc — this is the approach from above (result on the item + return it from the blocking paths + log on failure), now with the tests in. Happy to adjust if you'd prefer anything different.

…bic#453)

flushIO()/flushIOMaxPriorityItem() ran the real save()/load() but discarded the
result, and the blocking paths (asyncBlockingSave, blocking asyncLoad) always
returned totalSize, so a write/read failure looked like success to the caller.
The existing checks in virtual_memory (sz != pageSize) and the ant-colony export
(saved != totalBytes) were therefore dead code.

- Record the real save()/load() result on the FileItem (mResult) in both flush
  paths, and log failed reads/writes with the file name (always-on, not debug).
- Return mResult from the blocking paths instead of totalSize, so callers see the
  real outcome; a stop during the wait now returns kStop rather than a false
  success.

This is additive: no change to the queue/threading, and no caller changes needed
for virtual-memory paging and the ant-colony export — their existing result
checks simply start working. save()/load() already return <0 on failure.

Remaining from qubic#453, to do next on this branch: unit tests for a failed
load/save and for a stop during a wait (modeled on the TestAsyncFileIO harness),
and the end-of-epoch/revenue saves can gain an explicit check now that the
result is meaningful. The testnet failure-simulation (missing / read-only
directory) needs the node environment — happy to coordinate on that.
Add two regression tests under TestAsyncFileIO that drive a blocking
asyncLoad()/asyncSave() which is guaranteed to fail (a missing file and
an unopenable path) and assert the returned result is now negative.

Before qubic#1027 the blocking paths returned the request size even when the
underlying save()/load() failed, so a failed read/write was
indistinguishable from success. These tests return 64 (totalSize) on the
pre-fix code and -1 afterwards, so they lock in the propagation fix.
@parthod0x

Copy link
Copy Markdown
Contributor Author

Tests added and verified locally (built test.vcxproj's file_io.cpp with MSVC v143 + the same Microsoft.googletest 1.8.1.7 package CI uses, /std:c++20 /Zc:wchar_t-).

Regression proof — the two new tests fail on the pre-#1027 code and pass after:

Against develop (pre-fix) the blocking call returns the request size on failure:

error: Expected: (gFailResult) < (0), actual: 64 vs 0   # AsyncBlockingLoadMissingFilePropagatesFailure
error: Expected: (gFailResult) < (0), actual: 64 vs 0   # AsyncBlockingSaveFailurePropagatesFailure

(64 = 16 × 4 bytes = totalSize, i.e. failure masked as success.)

With this PR they pass, and you can see the new failure logging fire:

[ RUN      ] TestAsyncFileIO.AsyncBlockingLoadMissingFilePropagatesFailure
[file_io] failed to read file: nonexistent_file_for_1027_propagation_test.bin
[       OK ] TestAsyncFileIO.AsyncBlockingLoadMissingFilePropagatesFailure (27 ms)
[ RUN      ] TestAsyncFileIO.AsyncBlockingSaveFailurePropagatesFailure
[file_io] failed to write file: nonexistent_dir_for_1027/cannot_be_created.bin
[       OK ] TestAsyncFileIO.AsyncBlockingSaveFailurePropagatesFailure (31 ms)

Full TestAsyncFileIO suite is green: 19/19 (17 existing + 2 new). The new tests are fast (~30 ms each) and use only the public async API, mirroring the existing runTestAsyncSaveFile/runTestAsyncLoadFile worker-thread + flush pattern.

@fnordspace

Copy link
Copy Markdown
Contributor

Looks in general good to me. To additions would be nice:

  1. Invalidate the cache slot when a page load fails (virtual_memory.h, loadPageToCache). Set cachePageId[cache_page_id] to an invalid value before return -1. Without this, a partial read can leave corrupt data under a page that was valid before.
  2. Free the slot on the main-thread path (asyncBlockingSave and asyncLoad). Read mResult, set kFree, then return. The bug already exists on develop, but the PR rewrites exactly those return lines, and as it stands each main-thread call permanently uses up one of the 1024 slots.

I also think the end-of-epoch/revenue checks would be nice.

And a test on BM is needed before we could merge it. We definitely could help to run it by our test team but would be nice to have a test strategy to also force some of the failure cases.

@cyber-pc do you have any additions?

@fnordspace fnordspace added this to qubic Oct 28, 2026
@fnordspace fnordspace moved this to 🏗 In progress in qubic Oct 28, 2026
@fnordspace fnordspace added the bug Something isn't working label Oct 28, 2026
…c#453)

Review follow-ups for the error-propagation change:

- file_io.h: the asyncBlockingSave()/asyncLoad() main-thread (synchronous
  flush) path now reads mResult, frees the slot (kFree) and returns, instead
  of returning without freeing. flushWrite()/flushRead() run markAsDone(),
  which leaves a kBlockingWait slot in kProcessed, so without this each
  main-thread call permanently consumed one of the fixed blocking slots.

- virtual_memory.h: loadPageToCache() invalidates the cache slot
  (cachePageId[cache_page_id] = (unsigned long long)-1, the same empty-slot
  sentinel reset() fills via setMem(..., 0xff)) before returning -1 on a
  failed/partial page load, so a later findCachePage() cannot return this
  slot and serve corrupt data under a page id that was valid before the load.
…ubic#453)

Drive more than a full blocking-queue depth (ASYNC_FILE_IO_BLOCKING_MAX_QUEUE_ITEMS * 2)
of guaranteed-failing blocking load/save ops and assert that each one propagates a
real (<0) failure and that the fixed queue is never exhausted, i.e. the slot is
recycled on the failure path rather than leaked.

These exercise the NO_UEFI-reachable (non-main-thread) blocking path. The
main-thread flush path fixed in this PR is only reachable on bare metal; see the
PR test strategy for how that path is forced and asserted there.
@parthod0x

Copy link
Copy Markdown
Contributor Author

Thanks for the review — pushed f927517. Point by point:

  1. Cache-slot invalidation on failed load (virtual_memory.h, loadPageToCache): before return -1 on a failed/partial page load, the slot is now invalidated with cachePageId[cache_page_id] = (unsigned long long)-1 — the same empty-slot sentinel reset() fills via setMem(cachePageId, …, 0xff). Since findCachePage() matches on exact page id, the corrupt buffer can no longer be served under the previously-valid page.
  2. Main-thread slot free (file_io.h, asyncBlockingSave + asyncLoad): the isMainThread() branch now reads mResult, sets the slot to FileItem::kFree, then returns — mirroring the non-main path. As you noted, flush*() → markAsDone() left kBlockingWait slots in kProcessed, so each main-thread blocking call was permanently consuming one of the 1024 blocking slots.
  3. End-of-epoch / revenue checks: agreed these are worth doing now that failures propagate — the clearest targets are the ignored returns at qubic.cpp:6889-6891 (REVENUE_DATA_END_OF_EPOCH_FILE_NAME / MULTIDIM_REVENUE_END_OF_EPOCH_FILE_NAME), part of ~61 save/load call sites. I've deliberately left these out of this PR: hardening them is a behaviour change at the epoch boundary and the right reaction to a failed persist is a policy call. On a failed end-of-epoch save, which would you prefer — halt the node, retry, or log-and-continue/alert — and should the scope be just the revenue saves or a sweep of all critical end-of-epoch persistence? Happy to do it as a focused follow-up PR once the behaviour is decided.
  4. Tests: added AsyncBlockingLoad/SaveFailureDoesNotLeakSlots (drive >1 full queue depth of failing blocking ops; assert each propagates <0 and the queue is never exhausted). Verified with the portable-MSVC build — the four failure-path tests fail pre-fix (actual: 64 = totalSize) and pass post-fix; full TestAsyncFileIO = 21/21.

One caveat worth flagging for the test team: under NO_UEFI (the gtest build) isMainThread() is always false and loadPageToCache's failure branch is UEFI-only, so the unit tests exercise the non-main blocking path; the main-thread free (#2) and cache invalidation (#1) are only reachable on bare metal. I've written a BM failure-injection strategy for forcing and asserting those two — bad path / missing file / short read / full disk / permission denied, with return-value, slot-pool, and stale-page assertions. Want that pasted into the PR description or added as a test/README note?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Status: 🏗 In progress

Development

Successfully merging this pull request may close these issues.

2 participants