fix(rest): actionable 404 for a deployed-but-not-restarted component - #1806
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where deploying a component without restarting Harper leaves its routes unregistered. It introduces a check in server/REST.ts to identify inactive components and return an actionable 404 error, accompanied by integration tests. The review feedback suggests using stat instead of access to ensure the inactive component path is actually a directory (preventing false positives for files), and importing from node:assert/strict rather than node:assert in the test suite.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
Reviewed. One open item remains: the still-unresolved thread on |
… component deploy_component writes/extracts a new component to the components root immediately, but its routes aren't registered into the live router until Harper restarts. Hitting the component's URL in that window previously fell through to the same generic 404 as any nonexistent path, giving no signal that a restart is pending. When a route lookup misses, check whether the URL's first segment names a directory under the components root; if so, respond with a 404 naming the component and explaining that a restart is required, instead of the bare "Not found". Routing for already-active components and genuinely nonexistent paths is unaffected. Refs #674 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… detection These directories live under componentsRoot but aren't themselves deployed components; matching them would produce a misleading 404 message. Mirrors the same exclusion already used by the getComponents operation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t coverage - findInactiveComponent: use stat/isDirectory instead of access, so a stray non-directory file directly under the components root (README.md, .DS_Store, etc.) can't produce a false-positive actionable message. - Gate the actionable message behind an authenticated super_user request (request?.user?.role?.permission?.super_user), matching the exact pattern already used to gate the OpenAPI JSON endpoint in this same file. Without this, an unauthenticated caller could use the response difference (actionable vs. generic 404) as an oracle for which component directories exist on disk -- getComponents already treats that as super_user-only information elsewhere. - New tests: a non-directory file under components root gets the generic 404; node_modules/.deploy-aside stay excluded. - Import fix: node:assert/strict is restricted by this repo's own lint rule (AGENTS.md test style) -- the bot's suggestion was wrong for this repo. Uses plain node:assert with explicit strictEqual/deepStrictEqual instead. Also corrected a flawed test added in this pass: an "unauthenticated request" test using a bare fetch() with no Authorization header does not actually exercise a non-super_user path in this integration harness -- Harper's AUTHORIZE_LOCAL bypass treats loopback requests (127.0.0.x/::1) as an implicit super_user regardless of whether an auth header is present (security/auth.ts's `bypassUser ?? getSuperUser()` branch), so "no auth header" and "super_user" are indistinguishable from a local integration test. Replaced it with a real non-super_user role+user, authenticated as that role, which does exercise the gate. Sanity-checked both the stat/isDirectory fix and the auth-gate: temporarily reverted each, confirmed the corresponding new test fails with a clear assertion error, then restored the fix and confirmed all 6 tests pass. Also ran deploy-from-source.test.ts (incl. its throughput benchmark) unmodified to confirm no regression to hot-path routing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per review: only return the "deployed but Harper must be restarted" 404 when a restart is genuinely pending (restartNeeded()), not for any URL segment that happens to name a directory under componentsRoot. Without a pending restart, a matching directory is either an already-active component or not a live component, so fall back to the generic 404. Tests now toggle the real restart-needed signal (mutating a watched file in the already-loaded active app, which is what actually sets the flag -- a never-loaded deployed component has no watcher) and assert both sides of the gate: the same deployed-but-inactive component returns a generic 404 with no restart pending and the actionable 404 once one is. fixture-active-app gains a jsResource file so it has a live watcher to drive that signal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A component deployed with restart:false is written to disk but its routes are not live until Harper restarts. Previously this left get_status.restartRequired false and (with the new gate) the actionable inactive-component 404 never fired for the harper#674 scenario -- a freshly deployed, never-loaded component has no file watcher, so nothing called requestRestart(). Set the flag explicitly in the restart:false deploy branch, mirroring how a watched-file change flips the same restart-needed shared buffer. This is the setter only: it does not itself restart (matching a watched-file change; in DEV_MODE auto-reload reacts exactly as it would to any file change). The restart:true and rolling paths are unchanged -- they already restart. The call sits in the per-node branch, so a peer applying the replicated deploy sets its own flag the same way (no new cross-node signal). Tests: a fresh deploy_component restart:false now asserts get_status.restartRequired true and drives the actionable 404 without an unrelated restart pending; the "no restart pending" case uses the already-active app's directory at boot. Reverts the fixture-active-app watched-file addition from the previous commit -- no longer needed since the deploy itself sets the flag. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Only a brand-new component (no prior directory under componentsRoot) gets an unconditional requestRestart() from deployComponent now. Application#isNewComponent, set by extractApplication based on whether it had to rename an existing directory aside, is the new/existing signal — no new state-tracking needed. An existing, already-active component's redeploy is left to its own running file watcher (Scope/EntryHandler via deployLifecycle.ts's deploy:start/deploy:end), since some updates (e.g. static files only) may not need a restart at all, and when one is genuinely needed the watcher's post-deploy rescan already requests it independently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
kriszyp
force-pushed
the
fix/inactive-component-404
branch
from
July 16, 2026 00:05
6d5ba5a to
4ccd8b0
Compare
cb1kenobi
reviewed
Jul 16, 2026
… to") Per Kris: a restart is not always strictly required, and the message keys off the global restartNeeded() flag which can be set by a prior deploy_component restart:false (cb1kenobi review thread) -- "may need to" avoids over-asserting a restart is required. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
Author
|
Decision: going with the current global- — Claude |
dawsontoth
added a commit
that referenced
this pull request
Jul 21, 2026
The merge with main brought harper#674/#1806: a deploy_component with restart:false must set get_status restartRequired for a genuinely-new (never-loaded) component, scoped so a redeploy of an already-active component stays quiet. Main implemented this only in the one-shot deploy path (requestRestart() gated on Application.isNewComponent). Two-phase deploy — the default whenever the system db is replicated — never carried that behavior, so inactive-component-404.test.ts failed: a fresh deploy left restartRequired false and the actionable 404 never surfaced. Two-phase also can't set isNewComponent the way the one-shot path does: stageApplication extracts into a fresh staging dir, so extractApplication never sees the live directory and isNewComponent stayed default-true for everything (which would wrongly mark a restart on a redeploy). Fix both: - activateApplication now sets isNewComponent from whether the live dir existed BEFORE the swap (the two-phase equivalent of extract's in-place check) — true for a first deploy, false for a redeploy. - Extract markRestartRequiredForNewComponent() and call it on every no-restart path: one-shot (unchanged behavior), two-phase origin, activate-existing, and the per-node peer activate — so a new component deployed cluster-wide with restart:false reports restartRequired on every node, matching the one-shot peer behavior. Unit coverage: deployStaging asserts activate sets isNewComponent true for a first-ever activate and false when replacing an existing live version. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 24, 2026
kriszyp
pushed a commit
that referenced
this pull request
Jul 27, 2026
…1934) Since #1806 (5.2.0-beta.2), redeploying an already-active component recreates the file watchers instead of raising restartRequired, so a changed file re-emits as a fresh `add`. #1820 handled this for jsResource; loadEnv and fastifyRoutes still had the bare `if (eventType !== 'add') requestRestart()` gate, so a redeployed `.env` (or route file) was silently ignored — the new env values reached only the scanning thread, running workers kept the old ones, get_status stayed restartRequired:false, and the post-deploy restart was skipped (observed in the field on a replicated 5.2.0-beta.2 cluster). Extract the #1820 tracking into a shared `handleEntryWithRedeployRestart` helper (re-add of a known file, change/unlink, or a file missing from the post-deploy re-scan all request a restart) and route loadEnv + fastifyRoutes through it. jsResource keeps its inline copy (it additionally rejects non-file entries before the gate). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0188G62J9fZQg4J9rVuqLzjy
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
2 participants
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
deploy_componentwrites/extracts a new component to disk immediately, but its routes aren't registered into the live router until Harper restarts. Hitting the component's URL in that window fell through to the same generic 404 as any nonexistent path.server/REST.tsnow checks whether the URL's first segment names a directory under the components root. If it does, the response is a 404 naming the component and explaining that a restart is required (Component 'X' is deployed but Harper must be restarted before its routes are active.), instead of a bareNot found.node_modulesand the.deploy-asidestaging directory are excluded from the check (mirrors the existing exclusion in thegetComponentsoperation), since they live under componentsRoot but aren't themselves components.Why
Fixes harper#674.
Note on scope: I was not able to reproduce the literal
TypeError: rg.getMatch is not a functionfrom the issue on currentmain— everyresources.getMatch(...)call site (REST, WebSocket, GraphQL, MQTT) already guards against a route miss and returns a clean error, so a genuinely unloaded component's URL already 404s without crashing. What was still missing was the actionable part of the acceptance criteria: the 404 gave no indication that the path belonged to a deployed-but-inactive component. This PR adds that signal for the REST path (the scenario in the issue — "navigate to the component's URL" in a browser). WebSocket/GraphQL/MQTT route misses still return their existing (non-crashing, but generic) errors; extending the same messaging to those is a natural follow-up if wanted, but felt like scope creep for this fix.Where to look
server/REST.ts: newfindInactiveComponent()helper + theif (!entry)branch inhttp(). The helper only does a singlefs.accessstat call, and only on the already-cold "no route matched" path, so it shouldn't affect hot-path throughput (verified via the existingdeploy-from-source.test.tsPUT/GET throughput benchmark, which is unaffected).integrationTests/deploy/inactive-component-404.test.ts+ its two fixtures: reproduces the exact scenario (pre-existing active app + a second component deployed withrestart: false) and asserts both the new actionable 404 and that a truly nonexistent path is unaffected.Test plan
npm run build,npm run lint:required,npx tsc --noEmit— all clean.npm run test:unit:main,test:unit:resources— all passing (pre-existingtest:unit:serverfailure inworkerData-fixture.jsreproduces identically on a cleanmaincheckout; unrelated to this change).deploy-from-source.test.ts(including the throughput benchmark) passes unaffected.Generated by Claude Sonnet 5.
🤖 Generated with Claude Code