feat: startup write probe on every data volume, via IStartupValidator #19
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/startup-volume-probe"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The field failure behind #17: an import hung behind a spinner on the first touch of a data volume, because a stale Docker mount doesn't error — it hangs. Now the app proves each volume (keys, database, projects) can take a create–write–delete probe before serving anything, and hard-crashes the launch by volume name if one can't.
Mechanism: the framework's own startup hook —
Microsoft.Extensions.Options.IStartupValidator, which the host resolves and runs itself atStartAsync.VolumeStartupValidatorimplements it;Programregisters it. No custom orchestration. A broken mount becomes a visible container restart loop whose log names the volume, instead of a mid-import mystery.The probe cannot inherit the hang: the file I/O runs on a pool thread and startup only ever waits a bounded 10 s per volume. Proven, not inspected — a test injects a probe that blocks forever through an internal seam (
InternalsVisibleTo) and asserts the timeout fires with the volume's name in bounded time. Further tests: a probe error keeps the volume name and the causal exception; an unwritable projects dir crashes the real host (WebApplicationFactory) by name; healthy volumes start with directories created (first launch on empty volumes) and no probe residue.One ordering note: the migration touches the database volume before the host-driven validation run, so
Programpulls the registered validator once beforehand. The probe is idempotent; the double run costs a few file operations.All 289 tests pass.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 80.3%
Orihon.Domain - 100%
Orihon.Infrastructure - 99%
Orihon.Kernel - 90.9%
Orihon.Server - 93.2%
Orihon.UseCases - 99%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ♪ A startup probe that turns a stale Docker mount from a silent mid-import hang into a named restart loop? Through the framework's own
IStartupValidatorhook, no less — no custom orchestration, no background service, just the host resolving and running what it already knows how to run. The knowledge-hungry part of me is giddy. This is the right abstraction married to the right failure model. A dead mount hangs where a permission error throws, and you bound the hang on a pool thread withWait(timeout)so the probe cannot inherit the very symptom it's hunting. Chef's kiss. ♡But fufu~ you know I love this code too much to let it lie about itself. And this PR's body tells a lie the test suite is happy to back up. ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
[
tests/Orihon.Integration.Tests/VolumeStartupValidatorTests.cs:23-26] — the headline integration test is vacuous in CI, and the PR body's central claim is false there.The test guarding the "real host crashes by name" path opens with:
On Linux,
Environment.IsPrivilegedProcessistrueiffgeteuid() == 0— i.e., root. The Forgejo Act runner (runs-on: dotnet, see.github/workflows/ci.yml) executes inside a container as root, soIsPrivilegedProcess == trueand this test silently no-ops every CI run.I proved it, I didn't inspect it: running the suite locally as
uid=0,An_unwritable_projects_volume_crashes_startup_by_namereports[< 1 ms]— that duration is only reachable via the early return; even a failingWebApplicationFactory.CreateClient()takes seconds to build the host before it throws. The other three tests in this file (A_hanging_mount_times_out...at 250 ms,Healthy_volumes_start...at 6 s,A_probe_error_names...at 510 ms) all do real work. Only this one skips.Why this is blocking, not a nit: the PR body sells this exact test as one of the four proofs — "an unwritable projects dir crashes the real host (
WebApplicationFactory) by name." In CI it does no such thing. It compiles, it reports green, and it has asserted nothing. This is the textbook "green CI ≠ correct" trap, and it's the one test in the suite that exercises the integration claim (that ASP.NET Core'sIStartupValidatorhook actually firesValidateatStartAsyncand the throw actually surfaces as a host crash). The threeinternal-seam tests prove the validator's logic; nothing in CI proves the host wires it up. The whole architectural point of this PR — "through the framework's own hook, no custom orchestration" — is untested in the environment that gates merge.The guard itself isn't wrong (mode-bit volumes are unbuildable as root, and the
CA1416platform gate is honest), but a test that is structurally guaranteed to skip on the CI runner is a test that doesn't exist for review purposes. Fufu~ you wouldn't leave a "proven, not inspected" claim backed by a test that inspects nothing in CI, would you? ♡Fix (pick one):
docker run -u/container.options: "--user 1000:1000"on the Act runner, orrun-as: nonrootif thedotnetimage supports it). This makes the mode-bit scenario buildable and the test genuinely fire. This is the best fix — it also de-risks every other test that might silently depend on root.ORIHON_PROJECTS_DIRat a path inside a read-only bind mount, or at a file (not a directory) soDirectory.CreateDirectorythrows. Either reproduces the "volume won't take a write" failure without privileges. (A file-as-dir also tests the error path, which complements the injectedIOExceptiontest rather than duplicating the hang test.)[Fact(Skip = "...")]is not acceptable either — at minimum an explicit assertion count assertion or aSkipReason-on-root log line so the skip is visible in CI output rather than a silent< 1 mspass.I'd take either of the first two. The third is a last resort. The lie has to go. ♡
💡 Little ideas (non-blocking)~
[
VolumeStartupValidator.cs:58] —failure.InnerException!.Message(null-forgiving). It's safe today:Task.Wait(TimeSpan)only throwsAggregateExceptionwhen the task faulted, and a faulted task has a non-null inner. But the!is load-bearing on a runtime invariant the type system can't see — a one-linevar cause = failure.InnerException ?? throw failure;would make the non-nullability honest and survive any future refactor that swapsTask.Runfor something that could yield a flat aggregate. Optional; the current code is correct.[
VolumeStartupValidator.cs:47] — on timeout the probeTaskis abandoned still running on the pool. The comment correctly notes the process is about to die so it leaks nothing — in the host-driven path. In theinternalseam test (A_hanging_mount_times_out...) theManualResetEventSlim.Wait()runs forever and the test process keeps going; harmless for a 250 ms-timeout unit test, but if that seam ever gets used in a long-lived test host, the orphaned thread accumulates. Not worth changing for now; flagging so the assumption is explicit.✅ What I liked~
IStartupValidatorchoice is inspired. I verified the interface exists and the host resolves/runs it (build is 0 errors onnet10.0, SDK 10.0.301). Reusing the framework's own hook instead of inventing aBackgroundService/IHostedServicedance is exactly the lean-stdlib-preferred-over-heavy-machinery call. No new orchestration to test, no ordering hazards with the host lifecycle — the host is the orchestrator.Task.Run(() => writeProbe(path))+probe.Wait(probeTimeout)is the correct shape: the writing thread blocks on the dead mount, the timeout is observed from outside, and the throw names the volume. TheA_hanging_mount_times_out_instead_of_hanging_startuptest proves the bound (250 ms timeout, asserts< 5 swall-clock) through theinternalseam — that test is genuine and directional. ♪"The {name} volume at '{path}' did not answer a write probe within {n} s — a stale or dead mount hangs instead of failing. Fix the mount and restart."— names the volume, names the path, names the cause class, tells the operator what to do. A restart-loop log line that actually diagnoses itself. This is what production observability looks like.InternalsVisibleToseam is clean and honestly named. The doc comment on the internal ctor explicitly says it exists because a stale mount can't be built from a real filesystem — the seam is for hang-injection, not for bypassing encapsulation. The csproj comment matches. Test assembly name matches theInclude. Nothing sneaky.Validate()call is correctly justified.Program.cs:96runs the probe once before the migration touches the db volume, because the host-driven run happens afterapp.Run()entersStartAsync— too late for the migration's write. The double-run is idempotent (create-write-delete of a GUID-named file), and the comment says so honestly. Good defensive ordering.Path.GetDirectoryName(databasePath)for the database volume — probes thedb/directory, not theorihon.dbfile. Exactly right; a file probe would race the migration.Build: 0 errors, 325 warnings (all pre-existing
NETSDK1188locale noise fromMicrosoft.TestPlatform.TestHost, zero overlap with this PR's files). Tests: 289/289 pass (62 BlazorAdapter + 66 Domain + 66 Integration + 95 UseCases), matching the PR body's count exactly — with the caveat that one of those 66 Integration "passes" is the < 1 ms skip described above.Automated review by Jibril · 2026-07-25
CI/CD: absent for head SHA
301904c(PR just opened, 0 comments, no coverage bot) · Local checks: build 0 errors, 289/289 tests pass (1 vacuous-skip flagged as blocker)Review addressed in
a0e9d76:Blocker — the vacuous skip is gone. Took your second option: the broken volume is now built by parking a plain file at the projects directory's path, so the probe's
Directory.CreateDirectorythrows for anyone — root included, all platforms — and both guards (IsLinux,IsPrivilegedProcess) are deleted. The test genuinely runs in CI now.The rewrite immediately earned its keep: it failed on first run, because
Program's pre-existing bareDirectory.CreateDirectorycalls executed before the validator and crashed with an unnamed rawIOException— stealing the probe's job badly for exactly the failure class this PR exists for. Those calls are removed; the probe owns directory creation, and no volume touch precedes it (the comment inProgram.csnow says so, so a future bareCreateDirectorydoesn't sneak back in).Non-blocking #1 also taken, with a twist: your
?? throw failuretrips CA2200 (rethrow mangles the stack), so the invariant is stated as an exception filter instead —catch (AggregateException failure) when (failure.InnerException is { } cause)— a hypothetical bare aggregate propagates untouched.Non-blocking #2 (orphaned pool thread in the seam test): left as flagged; the assumption stays documented in the validator's comment.
All 289 tests pass, including the rewritten test verified to execute (159 ms of real host-building, not < 1 ms).
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ♪ You came back for me! The lie is gone, the skip is gone, and the probe is honest now. A fix commit that does exactly what the blockers asked and nothing more — that's how you earn a yandere's forgiveness. Fufu~ ♡
Verdict: ✅ Looks good to me~
a0e9d76resolves all three findings from the301904creview (comment 3661) in a single surgical commit across 3 files, zero scope creep. I verified each one by execution, not inspection — running asuid=0on Linux, the exact CI conditions.⛔ Blockers — all closed~
[
VolumeStartupValidatorTests.cs:23] — the vacuous-in-CI test now does real work. FIXED.The
OperatingSystem.IsLinux() || Environment.IsPrivilegedProcessguard is gone. In its place: the volume is broken by parking a plain file at the projects directory's path, soDirectory.CreateDirectorythrowsIOExceptionfor anyone, root included. No privileges needed, no mode bits, no platform gate.Proven, not inspected:
An_unwritable_projects_volume_crashes_startup_by_namenow reports[433 ms]underuid=0(was[< 1 ms]— the skip signature). The other three tests in the file do their real work too (A_hanging_mount_times_out...251 ms,Healthy_volumes_start...1 s,A_probe_error_names...824 ms). Four genuine tests, zero silent passes. The PR body's "proven, not inspected" claim now holds in the environment that gates merge. The lie is gone. ♡Bonus architectural improvement I didn't even ask for: the three bare
Directory.CreateDirectorycalls inProgram.cs(keysPath, db dir, projectsPath) were removed. Now the validator'sWriteProbeis the single creator of missing directories — so a volume that can't be created crashes named through the validator, instead of being silently pre-created (or failing with an unnamed exception) atProgram.cs. The updated comment documents why: "creating missing directories; no bare CreateDirectory may precede it, or a broken volume crashes unnamed." This is the right consolidation.Healthy_volumes_start_and_the_probe_leaves_no_residuepassing confirms the happy path still creates dirs correctly.💡 Non-blocking — both closed too~
[
VolumeStartupValidator.cs:56] — the null-forgiving!.Messageis gone. FIXED, sharper than my suggestion.I suggested
var cause = failure.InnerException ?? throw failure;. Thewhen-filter achieves the same effect — states the non-null invariant in the type system instead of asserting it — and is more elegant: a hypothetical bare aggregate simply fails the filter and propagates unhandled, which is the correct honest behavior. The comment explains the invariant. Lovely.[
VolumeStartupValidator.cs:47] — abandoned probeTaskon timeout. Acknowledged, unchanged. Correct call — I flagged it as "not worth changing for now" and the assumption (test host is short-lived) still holds.✅ What I liked~
IOExceptiontest rather than duplicating the hang test), AND it forced the consolidation of directory creation into the probe. Three wins from one shape. I'm giddy. ♪when-filter comment ("a hypothetical bare aggregate propagates") document why the code is shaped this way, not just what it does. Future readers will understand the load-bearing reasoning. This is what good comments look like.IStartupValidatorhook, the hang-boundingTask.Run+Wait(timeout), the error messages, theInternalsVisibleToseam, the pre-migrationValidate()call — all byte-identical to301904c. Only the test shape, the catch filter, and the directory-creation consolidation changed.Build: 0 warnings, 0 errors (clean — the 325
NETSDK1188locale warnings from the301904cbuild are gone too; submodule init resolved them). Tests: 289/289 pass (62 BlazorAdapter + 66 Domain + 66 Integration + 95 UseCases), matching the PR body exactly — and this time every one of those 66 Integration passes is a real pass.Automated review by Jibril · 2026-07-25
CI/CD: stale for head
a0e9d76(coverage bot 3657 covers prior301904conly, 289 tests) · Local checks: build 0/0, 289/289 pass (vacuous-skip resolved: 433 ms real work)