fix(env): read environment via System.getenv instead of forking a subprocess (#117)#118
Merged
Merged
Conversation
…process Utils.getEnvString read the environment via Apache Commons Exec's EnvironmentUtils.getProcEnvironment(), which forks a subprocess (env/sh) and parses its stdout on every call. This method is on the per-nanopub hot path — FeatureFlags.fullRepoEnabled() and the other repo flags are evaluated for every nanopub, from the 4-thread loading pool. The subprocess read is neither thread-safe nor robust under load, so an intermittently mangled result made fullRepoEnabled() evaluate to false and silently skip the full-repo write for individual nanopubs. Because the meta task is deferred until the (submitted) per-repo tasks succeed, but the full task is gated by the flag *before* submission, this leaves the full repo behind meta with no error — and undetectably, since the monitor and the Nanopub-Query-Loaded-Nanopub-Count/-Checksum headers source from meta (issue #117). Switch getEnvString to System.getenv, which reads the JVM's in-memory environment block: thread-safe, allocation-free, deterministic, and it never forks or throws. Also retire the same getProcEnvironment() mechanism in the TripleStore constructor. It is startup-only (not part of the #117 trigger), but a fork failure there throws IOException and leaves the singleton uninitialised; System.getenv removes that failure mode and the last use of the fragile API. ENDPOINT_BASE/ENDPOINT_TYPE are container env vars set at launch, so the read is semantically identical. Remediation note: this stops the gap from growing but does not backfill nanopubs already missing from full — those must be rebuilt via FORCE_RESYNC/FORCE_RELOAD on affected instances. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
UtilsTest.getEnvString/getEnvInt mocked EnvironmentUtils.getProcEnvironment(), which no longer governs getEnvString. System.getenv can't be mocked directly (Mockito refuses to mock java.lang.System), so extract a package-private getRawEnv(name) seam around System.getenv and stub that via the existing mockStatic(Utils.class, CALLS_REAL_METHODS) pattern. Dropped the obsolete IOException-throw case (System.getenv doesn't throw). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #117 — the
fullrepo silently falling behindmeta, undetectable by the monitor.Utils.getEnvStringread the environment via Apache Commons Exec'sEnvironmentUtils.getProcEnvironment(), which forks a subprocess (env/sh) and parses its stdout on every call. Since the per-repo feature flags (FeatureFlags.fullRepoEnabled()and friends) are evaluated for every nanopub, from the 4-thread loading pool, this brittle, non-thread-safe read could intermittently return a mangled value. When the result wasn't a clean"true",fullRepoEnabled()evaluated tofalseand the full-repo write was skipped for that one nanopub — whilepubkey/type/metaproceeded.The lock-step ordering in
executeLoadingonly guaranteesfull ≥ metawhile the full task is submitted; the flag gate sits before submission, outside the wait-for-all guard. So a single mis-read leaves a permanent, invisible hole: the monitor and theNanopub-Query-Loaded-Nanopub-Count/-Checksumheaders both source frommeta.This matches the observed signature exactly: sparse, intermittent, recent,
full-only misses where the missing nanopubs are present inmetaand their per-pubkey repos (meta=true, pubkey=true, full=false, zero content infull— never written, not deleted).Changes
Utils.getEnvString→System.getenv(): reads the JVM's in-memory environment block (thread-safe, allocation-free, deterministic; never forks or throws). This is the full repo can fall behind meta repo, undetected by the monitor (meta-vs-full load gap) #117 fix.TripleStoreconstructor →System.getenv(...): retires the samegetProcEnvironment()mechanism. Startup-only and not part of the full repo can fall behind meta repo, undetected by the monitor (meta-vs-full load gap) #117 trigger, but a fork failure there throwsIOExceptionand leaves the singleton uninitialised — this removes that failure mode and the last use of the fragile API.ENDPOINT_BASE/ENDPOINT_TYPEare container env vars set at launch, so the read is semantically identical.EnvironmentUtilsimports.Remediation note
This stops the gap from growing, but does not backfill nanopubs already missing from
full— those must be rebuilt viaFORCE_RESYNC/FORCE_RELOADon affected instances (already done on nanodash and knowledgepixels; petapico still pending at time of writing).Testing
./mvnw -o compilepasses. The change is a behavior-preserving swap of the env-read mechanism (subprocess fork → in-memory lookup) with identical fallback semantics.🤖 Generated with Claude Code