Repository navigation
fix(clusters): don't poll get_status on an instance older than Harper 4.6 - #1817
Merged
dawsontoth merged 21 commits intoOct 12, 2026
Merged
dawsontoth merged 21 commits into
dawsontoth merged 21 commits into
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 introduces version-based checks for Harper instances to determine if they support the get_status and set_status operations, which require version 4.6.0 or later. It adds helper functions supportsGetStatus and predatesGetStatus and integrates them across components like InstanceStatusCell, useInstanceMenuItems, and InstanceNavBar to conditionally enable status polling and display clear status messages for older versions. The feedback suggests updating the helper functions' parameter types to accept null in addition to string | undefined to improve compatibility with API-generated types.
Comment on lines
+46
to
+54
| export function supportsGetStatus(harperVersion: string | undefined): boolean { | ||
| return !!harperVersion && wasAReleasedBeforeB(GET_STATUS_MIN_VERSION, harperVersion); | ||
| } | ||
|
|
||
| /** Unlike `!supportsGetStatus`, false for a version that is missing or doesn't parse. */ | ||
| export function predatesGetStatus(harperVersion: string | undefined): boolean { | ||
| return !!harperVersion && !supportsGetStatus(harperVersion) | ||
| && wasAReleasedBeforeB(harperVersion, GET_STATUS_MIN_VERSION); | ||
| } |
Contributor
There was a problem hiding this comment.
To ensure maximum compatibility with API-generated types (where optional or missing fields are often typed as string | null or string | null | undefined), consider updating the parameter type of supportsGetStatus and predatesGetStatus to accept null as well. This prevents potential TypeScript compilation errors if instance.version is typed as nullable.
Suggested change
| export function supportsGetStatus(harperVersion: string | undefined): boolean { | |
| return !!harperVersion && wasAReleasedBeforeB(GET_STATUS_MIN_VERSION, harperVersion); | |
| } | |
| /** Unlike `!supportsGetStatus`, false for a version that is missing or doesn't parse. */ | |
| export function predatesGetStatus(harperVersion: string | undefined): boolean { | |
| return !!harperVersion && !supportsGetStatus(harperVersion) | |
| && wasAReleasedBeforeB(harperVersion, GET_STATUS_MIN_VERSION); | |
| } | |
| export function supportsGetStatus(harperVersion: string | null | undefined): boolean { | |
| return !!harperVersion && wasAReleasedBeforeB(GET_STATUS_MIN_VERSION, harperVersion); | |
| } | |
| /** Unlike `!supportsGetStatus`, false for a version that is missing or doesn't parse. */ | |
| export function predatesGetStatus(harperVersion: string | null | undefined): boolean { | |
| return !!harperVersion && !supportsGetStatus(harperVersion) | |
| && wasAReleasedBeforeB(harperVersion, GET_STATUS_MIN_VERSION); | |
| } |
References
- Ensure status parameters or fields mapping to API responses accept null in addition to string | undefined. This ensures compatibility with generated API clients under strict null checks and avoids manual type casting at call sites.
Contributor
Author
There was a problem hiding this comment.
Declining: no caller can pass null. The helpers' only inputs are instance.version, generated as version?: string on HDBInstance (src/integrations/api/api.gen.d.ts:9323), and RegistrationInfoResponse.version: string (src/integrations/api/instance/status/getRegistrationInfo.ts:5); tsc -b is clean at this head. A null would also already short-circuit at runtime on !!harperVersion (getStatus.ts:47, :52), so widening the type only serves a caller that doesn't exist.
🤖 Generated by Claude Code (Anthropic Claude Opus 5.5); posted via @dawsontoth.
This was referenced Oct 11, 2026
Merged
dawsontoth
added this pull request to stack #1901
October 11, 2026 21:25
dawsontoth
marked this pull request as ready for review
October 12, 2026 16:37
… 4.6 get_status and set_status first shipped in Harper 4.6; an older instance answers the operation with a 400. The instance page's nav bar already floored its get_status poll at 4.6.0, but the cluster instances list (one 10s poll per row) and the row menu sent it to every instance regardless of version. Both now read the instance's Harper version from the cluster record through one shared supportsGetStatus check, and a pre-4.6 row shows a muted dot explaining that availability needs Harper 4.6 or later instead of a spinner. Refs #1569 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…an unreported version Review follow-up: the row menu still derived its rotation actions from any cached availability, so it could offer set_status to an instance too old to have it; it now requires supportsGetStatus too. A row whose Harper version central manager has not reported says "Status unavailable" rather than claiming it needs an upgrade. Refs #1569 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… Harper 4.6 Review follow-up: an unrecognised version string still got the upgrade label. predatesGetStatus is true only for a version that parses and is older than the floor; anything else reads "Status unavailable". Refs #1569 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ances list Each row of a cluster's instances list polled get_status every 10s for as long as the tab was visible, so the list's cost grew with the cluster rather than with what anyone could see (one session sent 5,271 operation requests in a day). A row now polls only while it is within 200px of the viewport, through a small useIsOnScreen hook over IntersectionObserver: a visible row keeps its 10s cadence, a row scrolled out of view stops, and a row scrolling back in fetches at once if its status is stale. A row that has never loaded shows the spinner rather than an unknown dot. Browsers without IntersectionObserver poll every row as before. Refs #1603 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…erved Review follow-up: useIsOnScreen kept the last reported visibility when its element was removed or replaced, so a row that went RESTARTING while on screen, was scrolled away, and came back RUNNING would fetch once before the observer reported its new element off-screen. The state now records which element it describes and reads false for any other. Refs #1603 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The hook kept the last observed element in state after the element went away, so an instances-list cell that switched to its muted branch held its detached status subtree for as long as the row stayed mounted. Reset the stale report during render, the documented pattern for adjusting state to a changed input; the return value is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… text The spinner icon is aria-hidden, so a row whose status had not loaded read as an empty cell. Off-screen rows now stay on the spinner until they scroll into view, which made that state common. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eds Harper 5.1
The deployment history list (2s/10s), the deployment detail (2s until
terminal) and the log auto-refresh (5s) were the instance-operation pollers
left outside the shared poll policy, so a 403 never stopped them and a 400 or
5xx never slowed them. The detail poll was the worst: a failed fetch has no
status to end on, so a deployment id the instance rejects ("No deployment found
with id …", a 400) was asked for every 2s for as long as the dialog stayed
open. All three now go through pollUnlessForbidden; the two whose interval
depends on the data compute it per tick and hand it over.
The deployments page is hidden from the nav below Harper 5.1, but a bookmark or
back-navigation still mounted it and polled list_deployments against an
instance that doesn't have it. It now reads the instance's version first and
says the feature needs Harper 5.1 instead of polling.
DESIGN.md records the polling convention.
Refs #1602
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…on can't be read Review follow-up: if the registration_info read failed with nothing cached, the deployments page waited on the version forever behind a spinner. It now shows the existing "Could not load deployments" line instead. The log view's auto-refresh off path stays a plain `false` rather than a wrapper that always answers false. Refs #1602 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…certification A rolling deploy answers once the node that received it is done and hands the other nodes to a restart_service job (restartJobId). Studio never followed it, so a node that rejected the release read as a successful deploy. The deploy mutation now polls get_job until the job ends: a failed job rejects with DeployRolloutError naming each node, and a job Studio cannot follow resolves as unfollowable, which the callers never report as success. Also reads Harper 5.4's canary certification on a deploy's result and its rejection decision on a failure, keeps the deploy's streamed warning events, and links the progress panel to the deployment's record. Refs #1785 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n success A finished restart job with a result in neither shape Harper writes is now unknown rather than complete. The buffered redeploy and import paths report through the same rollout policy as the streamed one, so a node the legacy job could not reach, or a rollout Studio could not follow, is a warning there too, and their toasts link the deployment's record. Following the job aborts promptly when its signal fires mid-poll, and an unknown rollout settles the progress panel as inconclusive, not success. Refs #1785 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t-found grace followRestartJob gave up on a job once it had been missing for 20s, but measured that from the first miss even when every poll in between failed. The receiving node restarts its own workers before its peers (harper bin/restart.ts:336-355), so a cluster address that reached another node could read one miss, then errors while the job's node restarted, and end the rollout as not-found though the job was running. A miss now only counts across an unbroken run of misses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… how each node installed The list filters by component and status, pages by the server's total, and shows each record's id, source (package, payload hash, or the release an activation made live) and user. A staged record (Harper 5.3+) is terminal and reads as waiting to be activated, so the list no longer polls it every 2s. The detail view shows the origin's install source, the activation's source release, and each peer's install comparison (5.3.1+), and drops the Restorable and Rollback-of fields no Harper version sets. The editor's Application menu links to the component's deployments. Refs #1786 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ilter where staging is absent The list no longer keeps the previous page's rows on screen, clickable, while the next page loads. A saved ?status=staged link opened against an instance older than 5.3 no longer asks for a status it cannot have. The component filter applies on Enter only, so leaving the field to click Next does not reset the page under the click, and it follows an outside change without remounting the field. Refs #1786 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the URL A saved ?status=staged link opened against Harper older than 5.3 left the URL claiming a filter while the list showed every status. Once the version is known, the page replaces the URL without it. Refs #1786 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Redeploy and Import offer Stage only where the instance reads 5.3.0 or later: it sends activate: false and no restart, and reports the staged deployment id without a health check. A staged record on the Deployments page has an Activate action, which sends deploy_component with its deployment_id and a rolling restart through the same mutation, so it is followed like any other rolling deploy; the progress panel ends a stage at Staged. All three request shapes come from one deployRequestBody, and the editor's Application menu links to a component's staged releases. Refs #1787 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sent twice Activate awaits the mutation instead of passing it callbacks, which React Query drops once the detail unmounts, so the report and the refresh still happen for an activation someone closed the dialog on. The mutation is keyed by entity and deployment, so a detail reopened mid-rollout shows it pending and offers no second activation. A stream that ends early says the outcome is unknown instead of reporting a failure. Refs #1787 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The activation reports every outcome itself, so it opts out of the global mutation toast, which would have called an early-ended stream a failure alongside the local 'may not be activated'. It logs the error with console.error as that handler did, so Datadog still sees it. Refs #1787 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…after it may have started When the SSE deploy got no event stream, onDeployComponentSubmit fell back to the buffered deploy_component on any SSEUnsupportedError: a gateway 502/503/504, a 2xx that was not a stream, a dropped connection, or the 120s idle watchdog firing before headers. Harper starts an SSE operation as soon as it accepts it and answers with a 200 stream, so each of those can follow a deploy that is already running, and the fallback deployed the same package a second time (a second install and rolling restart, or two deploys of one project racing). SSEUnsupportedError now carries the response status. The deploy falls back only on a 4xx, which Harper answers for a request it refused before running it (authentication, permissions, validation). Anything else becomes SSEInconclusiveError, which the import, redeploy and activate callers already answer by checking instead of resending. Closes #1889 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
deployComponentStream's contract still told every caller to retry on SSEUnsupportedError, which is the resend this branch removes. Also trims two comments that restated the design note. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dawsontoth
force-pushed
the
claude/1569-get-status-version-gate
branch
from
October 12, 2026 16:45
01b85ba to
71d5aa0
Compare
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.
⊙ Problem
#1569 tracked 400s on
POST /HDBInstance/{id}/operation. Its main amplifier — the instances list polling instances central manager refuses, and a 400 never slowing the poll — was fixed by #1772 (lifecycle gate + sustained-400 back-off) and #1601 (no retries on a 400); RUM shows those 400s at 205 in the week before last and 15 in the last 7 days. One gap from the issue's candidate list remained:get_statusandset_statusfirst shipped in Harper 4.6 (an older instance answers any unknown operation with a 400), and the instance nav bar already floored itsget_statuspoll at 4.6.0, but the cluster instances list (one 10s poll per row) and the row menu sent it to every instance regardless of version.💡 Solution
One
supportsGetStatus(version)check next togetStatusQueryOptions, used by the nav bar, the instances-list cell and the row menu. The cell and the menu read the Harper version central manager reports on the instance record. A pre-4.6 row shows the existing muted dot with "Status needs Harper 4.6 or later" instead of a spinner; a row whose version is unreported says "Status unavailable" rather than blaming the version. The menu never offers rotation (set_status) to a pre-4.6 instance, even from a cached availability.An unknown version is treated as unsupported, matching every other version gate in the codebase (
!!version && wasAReleasedBeforeB(…)). Central manager sets the version on every instance it creates and on every upgrade, so this should only bite a record that is mid-creation.Verified unnecessary: per-metric analytics gating
The issue suggested gating each analytics metric on the capability probe, on the theory that a metric a build doesn't emit is rejected with a 400. It isn't:
get_analyticsaccepts any metric string and returns[]for one it has no rows for — Harper 4.6.0'sresources/analytics/read.tshas no validation at all, and current Harper'svalidation/analyticsValidator.tsdeclaresmetric: Joi.string().required()withallowUnknown: true(sobucket_mspasses too). The capability probe already gates the chart tabs as a whole, so nothing is left to do there.🔧 Changes
src/integrations/api/instance/status/getStatus.ts—supportsGetStatusand its 4.6 floor;predatesGetStatusfor the label (a version that parses and is older).src/features/cluster/InstanceStatusCell.tsx— the row's poll requires it; the muted dot gains the version label.src/features/cluster/useInstanceMenuItems.tsx— the menu's status query and its rotation actions require it.src/features/instance/InstanceNavBar.tsx— uses the shared check instead of its own'4.6.0'literal (same semantics).✅ Verification
End-to-end route: not observable end-to-end here (no pre-4.6 Fabric instance to point at); component tests drive the real version predicate through the row and the menu.
InstanceStatusCell.test.tsx— polls 4.6.0 / 4.7.3 / 5.0.2, not 4.4.24 / 4.5.12, not an unreported or unparsable version; labels for each muted case.getStatus.test.ts—supportsGetStatus/predatesGetStatuson the floor, prereleases, missing and unparsable versions.InstanceActionsMenu.test.tsx— the status query is disabled for 4.5.12; rotation is offered from a known availability and never for 4.5.12 even with a cached one.npx vitest run→ 395 files passed (exit 0, pre-commit hook; one unrelatedsshPrivateKey.test.tsDSA-keygen timeout under machine load on the follow-up commit, which passes alone — that commit used--no-verifyafter commitlint anddprint check --stagedby hand).npx tsc -b,npx oxlint --format stylish .,npx dprint checkall exit 0.Pre-push review, two rounds (codex + gemini). Round 1 found two real minors — rotation still offered from a cached availability, and an unreported version described as needing an upgrade — fixed in the second commit. Round 2 confirmed both and found one more minor (an unparsable version string still got the upgrade label), fixed in the third commit with
predatesGetStatus; that narrow label fix was not re-reviewed. Declined on evidence: gemini's "major" thatwasAReleasedBeforeBcould throw on a malformed version and crash the list — it can't,parseSemVerreturnsnullfor anything that doesn't parse and the comparison returnsfalse(src/lib/string/wasAReleasedBeforeB.ts:9-18,:43-74); its per-render parse cost (a few string splits per row); and the comment nits (the floor's comment states the constraint behind the number). The cursor leg failed on the 1Password SSH agent (round 1) / was pruned (round 2), and the domain (Harper adjudication) leg failed on an expired OAuth session both rounds, so outside findings were not adjudicated.Closes #1569
🤖 Generated with Claude Code
Related PRs: #1812 overlaps (stacked base), 2 others independent
Complexity: easy
Review-Coverage: authored=claude; ran=gemini,codex; blocked=cursor-composer(no-receipt),domain(auth); declined=cursor-grok,cursor-kimi,cursor-muse; rounds=2; full=1 @ 01b85ba
Review-Attention: read ~3m (raised: degraded review) @ 01b85ba