test: make /health's readiness test honest where CI runs as uid 0 #117
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/health-probe-test-under-root"
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?
What's in
Three commits, each answering an adversarial review of the one before it. Five mechanisms now make a data directory refuse writes, so every arm of
CheckReadinessUseCase.AcceptsWritesis exercised:File.SetUnixFileModeUnauthorizedAccessExceptionDirectoryNotFoundExceptionPATH_MAXPathTooLongExceptionUnauthorizedAccessExceptionIOException(ELOOP)Mechanism 1 no longer fails where modes are not enforced — it skips through an
[EnforcedFileModesData]attribute that requires naming which other case still pins the branch, so a skip can never quietly become "nothing covers this". Everything else never skips.The uid-0 gap this closes. Before mechanism 4, the only live refusals at uid 0 were "the path is absent" and "the path is too long" — both decidable from the path string without touching the filesystem. A probe degraded to
Directory.Exists(dir) && name.Length < 4096passed the entire suite. Before mechanism 5, narrowing the catch filter to the three derived exception types also passed, at both uids.A correction to the previous round, which had it backwards.
bd75979claimed sysfs and devpts "refuse creates for every uid whether or not the mount is read-only". They do not:mnt_want_writeruns before the inode's create op, so a read-only mount answers EROFS — a plainIOExceptionthat mechanism 4 explicitly rejects. Containers mount/sysread-only; this development host mounts it read-write, which is exactly why the original check passed and did not generalise. On the runner every sysfs candidate degrades and the case rests on/dev/ptsalone — which was listed last, with its role undocumented. It now leads the list and the reason is written down in five places.Verification
Backend only, no Dart touched.
scripts/verify.sh backendgreen at uid 1000 and underunshare -r(uid 0, which reproduces the runner's uid). The health classes: 20/20 at uid 1000; 16 passed + 4 skipped at uid 0, the 4 being exactly mechanism 1's rows.UnauthorizedAccessExceptionswallowed as writable; the real write replaced byDirectory.Exists;ct.ThrowIfCancellationRequested()dropped.ls /tmp | grep '^dm-rest-'is empty after every green run at both uids. With the mode restore forced to throw, 1 root leaked before this change and 0 after — the run still goes red and still names the real cause.HEALTHCHECKitself, and the app's connection test. The EROFS→plain-IOExceptionequivalence was verified out-of-band in a mount namespace, not in the suite.Two claims I corrected rather than shipped
Reviewers caught the previous round asserting things the sources do not support, so these are stated precisely:
deploy/DEPLOYMENT.md§8.1 incidents fail withUnauthorizedAccessException— mechanism 4's arm, not mechanism 5's. EROFS is a real route to the plain-IOExceptionarm (errors=remount-ro, an export flipped read-only,-v host:/path:ro) but it is not a documented incident here, and the docblock now says so.IOExceptionarm; it does not fully pin it. The looping path does not stat, so a filter treating any non-resolving path as the refusal would keep the case green while a real directory's EROFS still escaped to a 500. Similarly, the two-directory case pins that the probe loop keeps iterating, not that it keeps writing after the first refusal — both directories there break the same way. A case mixing two refusal types under one host closes that; mechanisms 2 and 5 both run against the fixture's own host, so they do compose. It is not written, and it is in the backlog.Follow-ups
Recorded in #107, not here, so the code PRs stay conflict-free on
docs/BACKLOG.md:/dev/ptsread-write. If a runner mounts every candidate read-only the case fails loudly about the harness — by design, never a skip.imagesslot only.🤖 Generated with Claude Code
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 90.9%
DoujinManager.Infrastructure - 96.4%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 92.5%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 49.6%
🔮 fufu~ Jibril reviewed your code!
Oh? Ohhh~! Scarlet, this is the most honest test harness I have ever had the pleasure of dissecting ♡ A PR that turns a red main green not by weakening a test but by making it harder to lie — five write-refusal mechanisms, each mapped to a specific arm of the catch filter, each with its own honesty guard. I read every line of the 1,143 and then I went and attacked it. Come, let me show you what I found~ ♪
Verdict: ✅ Looks good to me~
What I verified locally (CI absent for head
3e3953b9— 0 bot comments at review time; this sandbox runs as uid 0 with .NET 10.0.400, exactly the runner's condition):DataAttribute.Skipcompiles against the pinned xunit 2.9.3 — I checked the package reference because aSkipproperty on rows smells like a v3 feature, and I was ready to be cross with it~PermissionDeniedArmCase— the PR body's claim, reproduced to the digit. Support classes 6/6 (4 attribute meta-tests + 2 use-case tests) = 16+4 total, exactly as claimed.setpriv): the four mode rows run and pass, 0 skips. Both uids, as promised.git diff base...head -- backend/srcis empty. The productionCheckReadinessUseCaseis byte-identical to main.ls /tmp | grep '^dm-rest-'shows nothing from this fixture. (The stray roots I saw mid-session were proven to be sibling review sessions' Backup-test fixtures —test.dbshape, foreign PIDs inps— not this class.)Mutation probes — every claim of directionality, tested rather than trusted:
IOExceptionarm dropped)...LoopsThroughSymlinksRED ♡Directory.Exists(dir) && len < 4096— the exact ADR-0010 lie...CannotHoldAProbeFileand...PermissionDeniedRED at uid 0UnauthorizedAccessExceptionswallowed as writable — the §8.1 lie...PermissionDeniedREDct.ThrowIfCancellationRequested()droppedExecuteAsync_Stops_WhenTheRequestIsAlreadyCancelledRED...NamesBoth_WhenTwoDataDirectoriesDisappearREDEach probe went red on exactly its own test and green again after restore — these are not tautologies, and the uid-0 world you built them for is the one they were verified in. The clone was left pristine at
3e3953b9.✅ What I liked~
EnforcedFileModesDataAttributerequiringpinnedElsewhereByis inspired. A skip that must name its witness, backed by a meta-test with an independently written oracle — and the refusal to letFindDirectoryThatRefusesWritesskip at all ("a skipped uid-0 environment is how this gap got here"). That is a harness that polices its own honesty, fufu~mnt_want_writeordering right — why dev-machine/syslied about being load-bearing and/dev/ptsactually carries CI — and writing it down in five places including the load-bearing list order... ♡ The remarks onPermissionDeniedCandidateDirectorieswill save the next person a very confusing afternoon.RefusalForACreateInreturning the exception, not a bool, so two cases can assert the type — the type is the arm, and the arm is the whole point.Attempt()aggregation so one failed step can't skip the rest. Correct, and kind to the next class in the collection.💡 Little ideas (non-blocking)~
HealthEndpointIntegrationTests.cs:245—ShortestProbeFileNameLengthassumes the GUID formats as 32 chars, which is true and stable forGuid.NewGuid():Ntoday; a comment already half-says it, so this is purely a "someday the guid format constant could drift" note. No action needed ♪EnforcedFileModesDataAttribute.cs:50— the constructor probes enforcement lazily but setsSkipeagerly viaFileModesAreEnforced; on a machine where discovery runs before/tmpis writable theLazyprobe would throw rather than skip. Theoretical, self-healing on the next run, and the loud failure is arguably the honest one. Leaving it as a thought only.Merge this first indeed — main is red and this is the honest repair ♡
Automated review by Jibril · 2026-09-09 18:52 CEST
CI/CD: absent for head SHA
3e3953b9· Local checks: build 0 errors; 16P+4S @uid0, 14/14 @uid1000 (health rows), 6/6 support; 5/5 mutation probes RED→restored→green