fix(kubernetes): node list now syncs — prunes departed nodes (#1975) #257

Merged

stack72 merged 3 commits from fix/kubernetes-node-list-prune-1975 into main 2026-09-03 23:56:05 +00:00

Conversation 6 Commits 3 Files changed 4 +49 -5

stack72 commented 2026-09-03 22:22:48 +00:00

Owner

Copy link

Summary

• Node list method now performs a full sync: upserts live nodes, then prunes datastore records for nodes no longer present in the cluster
• Types deleteResource and readModelData on K8sContext and NodeContext — these runtime methods were always available but never declared by the kubernetes extension
• Prune logic uses readModelData(context.definition.name, "node") to query existing records scoped to the current model instance, then deleteResource to remove stale ones
• Both methods are guarded with existence checks for graceful degradation on older swamp versions
• Version bump to 2026.09.03.1 with upgrade entry

Context

@swamp/kubernetes/node's list method upserted every node it found in the live cluster but never deleted records for nodes that departed. Records for scaled-down, drained, or replaced nodes accumulated in the datastore forever, misleading debugging (phantom nodes with reused IPs).

The swamp data delete silent revert bug that compounded this issue was fixed separately in swamp-club/swamp#1996.

nodePod pruning is intentionally skipped — nodePod resources already have lifetime: "1h" which handles stale pod records automatically.

Closes swamp-club#1975
Filed swamp-club#1991 (swamp data delete silent revert — fixed by swamp-club/swamp#1996)

Test plan

• deno check extensions/models/*.ts — all 16 models pass
• deno lint extensions/models/ — clean
• deno fmt --check extensions/models/ — clean
• deno install --frozen — lockfile matches
• Manual verification against a live cluster with node autoscaling

🤖 Generated with Claude Code

## Summary - Node `list` method now performs a full sync: upserts live nodes, then prunes datastore records for nodes no longer present in the cluster - Types `deleteResource` and `readModelData` on `K8sContext` and `NodeContext` — these runtime methods were always available but never declared by the kubernetes extension - Prune logic uses `readModelData(context.definition.name, "node")` to query existing records scoped to the current model instance, then `deleteResource` to remove stale ones - Both methods are guarded with existence checks for graceful degradation on older swamp versions - Version bump to `2026.09.03.1` with upgrade entry ## Context `@swamp/kubernetes/node`'s `list` method upserted every node it found in the live cluster but never deleted records for nodes that departed. Records for scaled-down, drained, or replaced nodes accumulated in the datastore forever, misleading debugging (phantom nodes with reused IPs). The `swamp data delete` silent revert bug that compounded this issue was fixed separately in swamp-club/swamp#1996. nodePod pruning is intentionally skipped — `nodePod` resources already have `lifetime: "1h"` which handles stale pod records automatically. Closes swamp-club#1975 Filed swamp-club#1991 (swamp data delete silent revert — fixed by swamp-club/swamp#1996) ## Test plan - [x] `deno check extensions/models/*.ts` — all 16 models pass - [x] `deno lint extensions/models/` — clean - [x] `deno fmt --check extensions/models/` — clean - [x] `deno install --frozen` — lockfile matches - [ ] Manual verification against a live cluster with node autoscaling 🤖 Generated with [Claude Code](https://claude.com/claude-code)

stack72 added 1 commit 2026-09-03 22:22:48 +00:00

fix(kubernetes): node list now syncs — prunes departed nodes (#1975) ... 971f370d27

The node model's list method only upserted live nodes but never removed
records for nodes that left the cluster. Records for scaled-down, drained,
or replaced nodes accumulated in the datastore forever.

list now performs a full sync: upserts live nodes, then queries existing
node records via readModelData and deletes any not present in the live set
via deleteResource. Both methods are guarded with existence checks so the
extension degrades gracefully on older swamp versions.

Also types deleteResource and readModelData on K8sContext and NodeContext —
these runtime methods were always available but never declared by the
kubernetes extension.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-09-03 22:29:04 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. Unhandled error in prune loop (node.ts:333–338): If deleteResource throws for one record, the loop aborts and remaining departed nodes are left un-pruned with no error logged. Wrapping each call in a try/catch with a context.logger.warning(...) would make partial failures visible and allow the loop to continue.

2. Transitive semver range in deno.lock (deno.lock): The new @systeminit/swamp-testing@0.20260521.16 dependency introduces "npm:zod@^4.3.6" as a range entry in the lock file. This is a transitive dep declared by the testing package (not something this PR can control), and the lock file resolves it to 4.3.6 at runtime, so there's no real drift risk. Flagging for awareness — the lock file itself is committed and frozen, so this is safe.

3. No tests for prune logic: The list method now has meaningful stateful behavior (reading existing records, diffing, deleting stale ones). The CLAUDE.md testing rules specifically target vault/ and datastore/ extensions, so this isn't a rule violation, but the prune path (readModelData → set diff → deleteResource) is the kind of logic that benefits from a mock-context unit test to catch regressions (e.g., a future sanitization change that causes a name-match mismatch between liveNames and record.name).

---

Summary: The change is well-structured. The optional-chaining guard (if (context.deleteResource && context.readModelData)) correctly degrades gracefully on runtimes that don't yet support the new context methods. The sanitized name symmetry (both liveNames and record.name use the post-sanitizeInstanceName form) is consistent. Version bump, upgrade entry, and manifest are all aligned. No type errors, no any usage, no default exports.

## Code Review ### Blocking Issues None. ### Suggestions 1. **Unhandled error in prune loop** (`node.ts:333–338`): If `deleteResource` throws for one record, the loop aborts and remaining departed nodes are left un-pruned with no error logged. Wrapping each call in a `try/catch` with a `context.logger.warning(...)` would make partial failures visible and allow the loop to continue. 2. **Transitive semver range in deno.lock** (`deno.lock`): The new `@systeminit/swamp-testing@0.20260521.16` dependency introduces `"npm:zod@^4.3.6"` as a range entry in the lock file. This is a transitive dep declared by the testing package (not something this PR can control), and the lock file resolves it to `4.3.6` at runtime, so there's no real drift risk. Flagging for awareness — the lock file itself is committed and frozen, so this is safe. 3. **No tests for prune logic**: The `list` method now has meaningful stateful behavior (reading existing records, diffing, deleting stale ones). The CLAUDE.md testing rules specifically target `vault/` and `datastore/` extensions, so this isn't a rule violation, but the prune path (`readModelData` → set diff → `deleteResource`) is the kind of logic that benefits from a mock-context unit test to catch regressions (e.g., a future sanitization change that causes a name-match mismatch between `liveNames` and `record.name`). --- **Summary**: The change is well-structured. The optional-chaining guard (`if (context.deleteResource && context.readModelData)`) correctly degrades gracefully on runtimes that don't yet support the new context methods. The sanitized name symmetry (both `liveNames` and `record.name` use the post-`sanitizeInstanceName` form) is consistent. Version bump, upgrade entry, and manifest are all aligned. No type errors, no `any` usage, no default exports.

stack72 commented 2026-09-03 22:29:51 +00:00

Author

Owner

Copy link

Adversarial Review

Critical / High

1. Label-filtered list prunes nodes that exist but don't match the filter — node.ts:306-335

   The list method fetches nodes with labelSelector: labels (line 306–308), so only nodes matching the current label filter enter liveNames. The prune loop (lines 327–344) then deletes every existing record whose name is not in liveNames. This means nodes that are alive in the cluster but excluded by the label filter get pruned as "departed."

   Breaking example: User configures labels: "role=worker". Three worker nodes and two control-plane nodes are synced in a previous full-list run. User then runs list — only the three workers come back from the API. The two control-plane node records are pruned, even though those nodes are perfectly healthy. Any downstream consumer that relies on the complete node inventory now sees a truncated view.

   Even without label changes between runs: if a user always uses a label filter, the first list call will prune records written by other methods like get for nodes outside the filter. The get method (line 351–369) writes individual node records without a label check, and list will delete them on its next sync.

   Suggested fix: Either (a) skip pruning entirely when labels is set, or (b) scope readModelData to only return records that were originally written by a label-filtered list (e.g., by tagging records with the filter used). Option (a) is simpler and safer:

   if (context.deleteResource && context.readModelData && !labels) {
   

2. Empty API response wipes all node records — node.ts:308-344

   If coreApi.listNode() returns an empty items array — due to transient API failure returning 200 {}, RBAC misconfiguration returning an empty list, or a cluster with no matching nodes — liveNames will be empty. The prune loop will then delete every existing node record.

   Breaking example: A network blip causes the K8s API server to return { items: [] }. The extension deletes all node records. When the API recovers on the next list call, the records are re-created, but any downstream system that read the datastore between those two calls sees zero nodes.

   Suggested fix: Guard against pruning when the live set is empty, or when the ratio of pruned-to-existing exceeds a threshold:

   if (context.deleteResource && context.readModelData && liveNames.size > 0) {
   

   This prevents a complete wipe while still allowing incremental pruning when at least some nodes are returned.

Medium

3. A failed delete aborts the entire method, discarding successful write handles — node.ts:327-345

   If deleteResource throws for any single record during pruning, the exception propagates out of list and the return { dataHandles: handles } on line 347 is never reached. The upserts (writes) already succeeded as side effects, but the caller never receives the handles. Depending on the framework's contract, this could mean the caller treats the entire sync as failed and retries, potentially causing duplicate work or confusing error reporting.

   Suggested fix: Wrap the prune loop in a try/catch so that prune failures are logged but don't prevent the method from returning its write handles:

   try {
     // prune loop
   } catch (err) {
     context.logger.warning("Prune failed: {error}", { error: String(err) });
   }
   

4. Concurrent list calls can race on read-then-delete — node.ts:327-344

   Two concurrent list invocations both call readModelData, get the same set of existing records, and both attempt to delete the same "departed" records. Depending on deleteResource's behavior on already-deleted records, this could throw or silently double-count. If deleteResource throws on a missing record, one of the concurrent calls will fail (see finding #3).

Low

5. readModelData is not guaranteed to return sanitized names — node.ts:335

   The comparison !liveNames.has(record.name) assumes readModelData returns names in the same format as sanitizeInstanceName(). If the framework stores or returns the raw name instead of the sanitized instance name, the comparison will never match and no pruning will occur (a silent no-op, not data loss). This is safe-by-default but worth verifying against the framework contract.

Verdict

FAIL — The label-filter pruning interaction (#1) and the empty-response wipe (#2) are both data-loss bugs in realistic production scenarios. The core sync idea is sound but the prune guard needs to account for filtered queries and degenerate API responses before this is safe to merge.

## Adversarial Review ### Critical / High 1. **Label-filtered list prunes nodes that exist but don't match the filter** — `node.ts:306-335` The `list` method fetches nodes with `labelSelector: labels` (line 306–308), so only nodes matching the current label filter enter `liveNames`. The prune loop (lines 327–344) then deletes *every* existing record whose name is not in `liveNames`. This means nodes that are alive in the cluster but excluded by the label filter get pruned as "departed." **Breaking example:** User configures `labels: "role=worker"`. Three worker nodes and two control-plane nodes are synced in a previous full-list run. User then runs `list` — only the three workers come back from the API. The two control-plane node records are pruned, even though those nodes are perfectly healthy. Any downstream consumer that relies on the complete node inventory now sees a truncated view. Even without label changes between runs: if a user *always* uses a label filter, the first `list` call will prune records written by other methods like `get` for nodes outside the filter. The `get` method (line 351–369) writes individual node records without a label check, and `list` will delete them on its next sync. **Suggested fix:** Either (a) skip pruning entirely when `labels` is set, or (b) scope `readModelData` to only return records that were originally written by a label-filtered list (e.g., by tagging records with the filter used). Option (a) is simpler and safer: ```typescript if (context.deleteResource && context.readModelData && !labels) { ``` 2. **Empty API response wipes all node records** — `node.ts:308-344` If `coreApi.listNode()` returns an empty `items` array — due to transient API failure returning `200 {}`, RBAC misconfiguration returning an empty list, or a cluster with no matching nodes — `liveNames` will be empty. The prune loop will then delete *every* existing node record. **Breaking example:** A network blip causes the K8s API server to return `{ items: [] }`. The extension deletes all node records. When the API recovers on the next `list` call, the records are re-created, but any downstream system that read the datastore between those two calls sees zero nodes. **Suggested fix:** Guard against pruning when the live set is empty, or when the ratio of pruned-to-existing exceeds a threshold: ```typescript if (context.deleteResource && context.readModelData && liveNames.size > 0) { ``` This prevents a complete wipe while still allowing incremental pruning when at least some nodes are returned. ### Medium 3. **A failed delete aborts the entire method, discarding successful write handles** — `node.ts:327-345` If `deleteResource` throws for any single record during pruning, the exception propagates out of `list` and the `return { dataHandles: handles }` on line 347 is never reached. The upserts (writes) already succeeded as side effects, but the caller never receives the handles. Depending on the framework's contract, this could mean the caller treats the entire sync as failed and retries, potentially causing duplicate work or confusing error reporting. **Suggested fix:** Wrap the prune loop in a try/catch so that prune failures are logged but don't prevent the method from returning its write handles: ```typescript try { // prune loop } catch (err) { context.logger.warning("Prune failed: {error}", { error: String(err) }); } ``` 4. **Concurrent `list` calls can race on read-then-delete** — `node.ts:327-344` Two concurrent `list` invocations both call `readModelData`, get the same set of existing records, and both attempt to delete the same "departed" records. Depending on `deleteResource`'s behavior on already-deleted records, this could throw or silently double-count. If `deleteResource` throws on a missing record, one of the concurrent calls will fail (see finding #3). ### Low 5. **`readModelData` is not guaranteed to return sanitized names** — `node.ts:335` The comparison `!liveNames.has(record.name)` assumes `readModelData` returns names in the same format as `sanitizeInstanceName()`. If the framework stores or returns the raw name instead of the sanitized instance name, the comparison will never match and no pruning will occur (a silent no-op, not data loss). This is safe-by-default but worth verifying against the framework contract. ### Verdict **FAIL** — The label-filter pruning interaction (#1) and the empty-response wipe (#2) are both data-loss bugs in realistic production scenarios. The core sync idea is sound but the prune guard needs to account for filtered queries and degenerate API responses before this is safe to merge.

stack72 added 1 commit 2026-09-03 22:36:46 +00:00

fix(kubernetes): skip node prune when label-filtered or API returns empty ... c750658aad

Prune only runs when no label filter is active (otherwise it would delete
records for nodes outside the filter scope) and when at least one live
node was returned (prevents a complete wipe on transient API failures).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-09-03 22:45:12 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. NodeContext duplicates methods now in K8sContext (node.ts:35-39, helpers.ts:37-41). deleteResource and readModelData were just added to the shared K8sContext interface, but NodeContext re-declares them verbatim. A future pass could have NodeContext extend K8sContext to avoid drift. Out of scope for this PR per the "only touch what's necessary" rule, but worth noting for follow-up.

2. liveNames vs readModelData name comparison assumption (node.ts:337-339). The prune loop compares record.name (from readModelData) against liveNames, which contains sanitized instance names. This is correct only if readModelData returns instance names (i.e., the keys passed to writeResource), not raw data-payload names. In practice, Kubernetes node names are DNS subdomain labels where sanitizeInstanceName is a no-op, so this works — but a brief comment documenting the expected shape of readModelData results would help future maintainers reason about the invariant.

3. No test for the pruning path. The prune-on-label-filter and prune-on-empty-response guards are subtle. CLAUDE.md only mandates tests for vault/ and datastore/ extensions, so this isn't blocking, but a unit test that mocks readModelData/deleteResource and exercises the !labels && liveNames.size > 0 guard condition would significantly improve confidence — especially the "skip when 0 live nodes" safety rail.

## Code Review ### Blocking Issues None. ### Suggestions 1. **`NodeContext` duplicates methods now in `K8sContext`** (`node.ts:35-39`, `helpers.ts:37-41`). `deleteResource` and `readModelData` were just added to the shared `K8sContext` interface, but `NodeContext` re-declares them verbatim. A future pass could have `NodeContext` extend `K8sContext` to avoid drift. Out of scope for this PR per the "only touch what's necessary" rule, but worth noting for follow-up. 2. **`liveNames` vs `readModelData` name comparison assumption** (`node.ts:337-339`). The prune loop compares `record.name` (from `readModelData`) against `liveNames`, which contains sanitized instance names. This is correct only if `readModelData` returns instance names (i.e., the keys passed to `writeResource`), not raw data-payload names. In practice, Kubernetes node names are DNS subdomain labels where `sanitizeInstanceName` is a no-op, so this works — but a brief comment documenting the expected shape of `readModelData` results would help future maintainers reason about the invariant. 3. **No test for the pruning path**. The prune-on-label-filter and prune-on-empty-response guards are subtle. CLAUDE.md only mandates tests for `vault/` and `datastore/` extensions, so this isn't blocking, but a unit test that mocks `readModelData`/`deleteResource` and exercises the `!labels && liveNames.size > 0` guard condition would significantly improve confidence — especially the "skip when 0 live nodes" safety rail.

stack72 commented 2026-09-03 22:47:03 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. node.ts:326-348 -- Prune failure discards all upsert handles

   If readModelData or deleteResource throws during the prune phase (network error, permission denied, backend unavailable), the exception propagates out of execute, and the return statement at line 350 never executes. The upserts at lines 315-325 have already been committed via writeResource, but the caller never receives the handles. Depending on how the framework uses those handles (e.g. to confirm what was synced), this could cause the caller to believe the entire operation failed even though the core work (upserting live nodes) succeeded.

   Breaking scenario: readModelData succeeds and returns 200 existing records, but deleteResource fails on the 3rd call (transient network error). The 2 successful deletes are committed, the remaining 197 stale records stay, and the caller gets an unhandled rejection instead of the 50 upserted handles.

   Suggested fix: Wrap the prune block in try/catch. On prune failure, log a warning and continue to return the upsert handles.

2. node.ts:329 -- !labels is falsy check on a string, empty string passes through

   labels is z.string().optional(), so it can be undefined or any string. The check !labels is true for both undefined and "". An empty string label selector is equivalent to no selector in Kubernetes, so this is currently correct behavior. However, it is a fragile idiom -- if the schema were ever changed to .default(""), pruning would silently activate even when the caller intended to pass a label selector that happened to be empty. Not a bug today, but a latent footgun.

   Suggested fix: Use labels === undefined or labels == null for an explicit check.

Low

1. node.ts:23 -- definition is required in the type but unguarded at runtime

   definition is required (not optional) in NodeContext, but deleteResource and readModelData are optional. The prune block (line 327) guards on the optional methods but not on definition. If a runtime provides the optional methods but does not provide definition, context.definition.name at line 332 would throw TypeError. In practice this is unlikely -- a runtime new enough to supply the new methods would supply definition too -- but the asymmetry between the type-level guarantee (required) and the runtime contract (implicitly assumed) is worth noting.

2. node.ts:23 vs pod_summary.ts:26 -- inconsistent definition shape

   NodeContext declares definition with a name field while PodSummaryContext (out of scope, but the only other model using definition) declares it with an id field. If the framework provides a single definition object, one of these models is looking at the wrong field. This is not a bug in this PR (out of scope), but the divergence is suspicious and worth verifying.

Verdict

PASS -- The core sync logic is sound. The prune guards (skip when label-filtered, skip when API returns empty) are correct and match the commit messages. The two medium findings are robustness improvements, not correctness bugs in the happy path.

## Adversarial Review ### Medium 1. **node.ts:326-348 -- Prune failure discards all upsert handles** If `readModelData` or `deleteResource` throws during the prune phase (network error, permission denied, backend unavailable), the exception propagates out of `execute`, and the return statement at line 350 never executes. The upserts at lines 315-325 have already been committed via `writeResource`, but the caller never receives the handles. Depending on how the framework uses those handles (e.g. to confirm what was synced), this could cause the caller to believe the entire operation failed even though the core work (upserting live nodes) succeeded. **Breaking scenario:** `readModelData` succeeds and returns 200 existing records, but `deleteResource` fails on the 3rd call (transient network error). The 2 successful deletes are committed, the remaining 197 stale records stay, and the caller gets an unhandled rejection instead of the 50 upserted handles. **Suggested fix:** Wrap the prune block in try/catch. On prune failure, log a warning and continue to return the upsert handles. 2. **node.ts:329 -- `!labels` is falsy check on a string, empty string passes through** `labels` is `z.string().optional()`, so it can be `undefined` or any string. The check `!labels` is `true` for both `undefined` and `""`. An empty string label selector is equivalent to no selector in Kubernetes, so this is currently correct behavior. However, it is a fragile idiom -- if the schema were ever changed to `.default("")`, pruning would silently activate even when the caller intended to pass a label selector that happened to be empty. Not a bug today, but a latent footgun. **Suggested fix:** Use `labels === undefined` or `labels == null` for an explicit check. ### Low 1. **node.ts:23 -- `definition` is required in the type but unguarded at runtime** `definition` is required (not optional) in `NodeContext`, but `deleteResource` and `readModelData` are optional. The prune block (line 327) guards on the optional methods but not on `definition`. If a runtime provides the optional methods but does not provide `definition`, `context.definition.name` at line 332 would throw TypeError. In practice this is unlikely -- a runtime new enough to supply the new methods would supply `definition` too -- but the asymmetry between the type-level guarantee (required) and the runtime contract (implicitly assumed) is worth noting. 2. **node.ts:23 vs pod_summary.ts:26 -- inconsistent `definition` shape** `NodeContext` declares `definition` with a `name` field while `PodSummaryContext` (out of scope, but the only other model using `definition`) declares it with an `id` field. If the framework provides a single `definition` object, one of these models is looking at the wrong field. This is not a bug in this PR (out of scope), but the divergence is suspicious and worth verifying. ### Verdict **PASS** -- The core sync logic is sound. The prune guards (skip when label-filtered, skip when API returns empty) are correct and match the commit messages. The two medium findings are robustness improvements, not correctness bugs in the happy path.

stack72 added 1 commit 2026-09-03 23:02:50 +00:00

chore: revert unrelated root deno.lock drift ... 23422077c7

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-09-03 23:08:54 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. No tests for the new pruning logic — The list method gained meaningful new behavior (record pruning), but no test file is in the changed files list. CLAUDE.md only mandates tests for vault/ and datastore/ extensions, so this doesn't block merge, but a unit test exercising the prune path (mock readModelData returning a stale record, assert deleteResource is called) would make future refactors safer.

2. definition is not guarded alongside deleteResource/readModelData — The prune block correctly gates on context.deleteResource && context.readModelData before using them, but context.definition.name is accessed inside that branch without a guard. definition is declared as a required field on NodeContext, so TypeScript ensures the harness provides it; if the harness ever makes it optional in the future this would silently become a runtime crash. Low risk given current conventions, but worth noting.

3. liveNames.size > 0 semantics — The guard correctly prevents mass deletion when the API returns an empty list (transient error or cluster tear-down). This is a good safety choice. One minor edge case: if a cluster genuinely has no nodes but previously had records, pruning will never run. This is probably acceptable for operational safety, and can be revisited if it becomes a real problem.

---

Overall: The sync/prune implementation is correct. The two key safety guards (!labels skips pruning on partial label-filtered results; liveNames.size > 0 prevents catastrophic wipe on empty API response) are both present and correct. The interface additions in helpers.ts (deleteResource?, readModelData?) are consistent with how NodeContext uses them. Version bump and upgrade entry look correct. No security, type, or CLAUDE.md compliance issues found.

## Code Review ### Blocking Issues None. ### Suggestions 1. **No tests for the new pruning logic** — The `list` method gained meaningful new behavior (record pruning), but no test file is in the changed files list. CLAUDE.md only mandates tests for `vault/` and `datastore/` extensions, so this doesn't block merge, but a unit test exercising the prune path (mock `readModelData` returning a stale record, assert `deleteResource` is called) would make future refactors safer. 2. **`definition` is not guarded alongside `deleteResource`/`readModelData`** — The prune block correctly gates on `context.deleteResource && context.readModelData` before using them, but `context.definition.name` is accessed inside that branch without a guard. `definition` is declared as a required field on `NodeContext`, so TypeScript ensures the harness provides it; if the harness ever makes it optional in the future this would silently become a runtime crash. Low risk given current conventions, but worth noting. 3. **`liveNames.size > 0` semantics** — The guard correctly prevents mass deletion when the API returns an empty list (transient error or cluster tear-down). This is a good safety choice. One minor edge case: if a cluster genuinely has no nodes but previously had records, pruning will never run. This is probably acceptable for operational safety, and can be revisited if it becomes a real problem. --- **Overall:** The sync/prune implementation is correct. The two key safety guards (`!labels` skips pruning on partial label-filtered results; `liveNames.size > 0` prevents catastrophic wipe on empty API response) are both present and correct. The interface additions in `helpers.ts` (`deleteResource?`, `readModelData?`) are consistent with how `NodeContext` uses them. Version bump and upgrade entry look correct. No security, type, or CLAUDE.md compliance issues found.

stack72 commented 2026-09-03 23:37:39 +00:00

Author

Owner

Copy link

Adversarial Review

Critical / High

None.

Medium

None.

Low

1. kubernetes/extensions/models/node.ts:327-329 — Race between K8s list and model-data read could prune a just-added node.
   If another sync concurrently writes a new node record between the listNode call (line 308) and the readModelData call (line 331), that node would appear in existing but not in liveNames, and would be incorrectly pruned. Self-corrects on the next sync cycle, and the liveNames.size > 0 guard already prevents the catastrophic variant (mass-delete on empty API response). No fix needed — this is inherent to non-transactional sync and the impact is a single missed record that reappears next cycle.

2. kubernetes/extensions/models/node.ts:329 — !labels relies on JavaScript falsiness for string | undefined.
   The check !labels is true for both undefined and "". An empty-string label selector is semantically equivalent to no selector (K8s API returns all nodes), so pruning correctly runs in both cases. However, this relies on the caller never passing a whitespace-only string like " " which would be truthy, bypass the guard, and the K8s API would likely return all nodes — meaning pruning runs correctly anyway. No practical risk, but documenting the intent (e.g. labels == null vs !labels) would make the distinction explicit if this ever matters.

Verdict

PASS — The pruning logic is well-guarded: it skips when label-filtered (partial view), when the API returns zero nodes (protects against accidental mass deletion), and when the context lacks deleteResource/readModelData (graceful degradation). The interface additions to K8sContext are non-breaking (both optional). The upgrade chain and version bump are consistent across node.ts and manifest.yaml. No correctness, security, or resource management issues found.

## Adversarial Review ### Critical / High None. ### Medium None. ### Low 1. **`kubernetes/extensions/models/node.ts:327-329` — Race between K8s list and model-data read could prune a just-added node.** If another sync concurrently writes a new node record between the `listNode` call (line 308) and the `readModelData` call (line 331), that node would appear in `existing` but not in `liveNames`, and would be incorrectly pruned. Self-corrects on the next sync cycle, and the `liveNames.size > 0` guard already prevents the catastrophic variant (mass-delete on empty API response). No fix needed — this is inherent to non-transactional sync and the impact is a single missed record that reappears next cycle. 2. **`kubernetes/extensions/models/node.ts:329` — `!labels` relies on JavaScript falsiness for `string | undefined`.** The check `!labels` is `true` for both `undefined` and `""`. An empty-string label selector is semantically equivalent to no selector (K8s API returns all nodes), so pruning correctly runs in both cases. However, this relies on the caller never passing a whitespace-only string like `" "` which would be truthy, bypass the guard, and the K8s API would likely return all nodes — meaning pruning runs correctly anyway. No practical risk, but documenting the intent (e.g. `labels == null` vs `!labels`) would make the distinction explicit if this ever matters. ### Verdict **PASS** — The pruning logic is well-guarded: it skips when label-filtered (partial view), when the API returns zero nodes (protects against accidental mass deletion), and when the context lacks `deleteResource`/`readModelData` (graceful degradation). The interface additions to `K8sContext` are non-breaking (both optional). The upgrade chain and version bump are consistent across `node.ts` and `manifest.yaml`. No correctness, security, or resource management issues found.

stack72 merged commit b02178bde3 into main 2026-09-03 23:56:05 +00:00

stack72 deleted branch fix/kubernetes-node-list-prune-1975 2026-09-03 23:56:06 +00:00

stack72 referenced this pull request from a commit 2026-09-03 23:56:06 +00:00

fix(kubernetes): node list now syncs — prunes departed nodes (#1975) (#257)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

swamp-club/swamp-extensions!257

No description provided.