fix(graph): exempt deleted rows from the orphan-registration sweep

[jfrench9]

Summary

The OrphanedGraphRegistrations alarm has been firing on graph-registry rows for graphs that no longer exist. cleanup_stale_graphs removes a deleted row only once it is older than STALE_GRAPH_DAYS; a deleted row still inside that window fell through to the orphan check, which had no status predicate, and was stamped and counted as soon as the instance it named left the instance registry.

That instance is always about to leave — a deleted graph's writer is released with the graph and recycled by the ASG well inside seven days — so every rolling fleet replacement that followed a deletion raised the alarm for a week and then healed itself. This scopes the orphan check to rows that are actually routing.

Changes

robosystems/operations/graph/infrastructure.py

• cleanup_stale_graphs: add status != "deleted" to the instance_missing predicate, alongside the existing shared-repository exemption. Both exemptions say the same thing — the row is not a routing pointer — so they sit together and are documented together.
• Docstring and inline comment updated to record why, including the failure mode the predicate prevents (a page that clears itself after STALE_GRAPH_DAYS).
• The clear path needed no change and now covers the other half for free: a row stamped while it was live and subsequently deleted drops its instance_missing_since marker on the next sweep, rather than carrying it until the row ages out.

tests/operations/graph/test_infrastructure.py — three cases in TestCleanupStaleGraphs, mirroring the shared-repository exemption trio that already exists:

• test_recently_deleted_graph_is_not_orphaned — a deleted row whose instance is absent is neither counted nor stamped, and the published metric is 0.
• test_deleted_graph_marker_from_before_the_exemption_is_cleared — a marker left from a sweep that predates this rule is removed.
• test_live_graph_is_still_orphaned_alongside_a_deleted_row — the exemption is scoped; an active graph swept at the same time is still marked and counted.

Worth a close look: the first test is a negative control. Verified that it fails without the predicate, and that when it fails it emits the same warning line the production sweep did — so it reproduces the defect rather than asserting around it.

No behaviour change to routing, allocation, or any request path. cleanup_stale_graphs runs only from the daily instance_registry_cleanup_job and the weekly full_instance_maintenance_job; what changes is which rows those sweeps count and stamp.

Breaking Changes

None. No API surface is touched — no GraphQL schema, operations envelope, or REST shape changes — so there is no client-SDK impact in either tier and no regen is required.

Testing

• just test-code — clean (ruff, format, basedpyright, cf-lint).
• uv run pytest tests/operations/graph/test_infrastructure.py — 33 passed.
• uv run pytest tests/operations/graph tests/dagster — 992 passed.
• Negative control: reverted the predicate, confirmed test_recently_deleted_graph_is_not_orphaned fails and emits the production warning line, restored.

Full just test-all was not run; the unit suite above covers the changed module and its callers, and CI runs the full gate.

Certification

• I have the right to submit this work under the Apache 2.0 license, and do so. Where any part of it is owned by my employer, I have their permission.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PBkpeEv3gBUzL812xqfXnS

[github-actions]

Change classification: normal — platform or application code changed.

A normal change carries a documented automated review on this pull request before merge, in place of a second human approver. The change-classification job requests that review and passes once it has been posted. Recorded by the change-classification job.

[jfrench9]

@claude please review this PR

[claude]

Claude finished @jfrench9's task in 1m 25s —— View job

---

Review: PR #1367 — exempt deleted rows from the orphan-registration sweep

• Gather context (diff, changed files)
• Review infrastructure.py change
• Review test coverage
• Post review feedback

Summary

Straightforward, well-scoped fix. Read the full diff against origin/main for both files and traced the surrounding cleanup_stale_graphs logic in robosystems/operations/graph/infrastructure.py:394-544.

Correctness

• The new status != "deleted" clause (infrastructure.py:492) is added to the same instance_missing predicate as the existing shared-repository exemption, and only after the should_remove/continue block above it (infrastructure.py:453-467) — so it only ever applies to a deleted row still inside STALE_GRAPH_DAYS, exactly the gap described in the PR. Confirmed status = item.get("status") (infrastructure.py:446) is populated before this point, so the predicate reads the same variable the removal branch already used at line 459 — no new lookup, no risk of drift between the two checks.
• The clear path (elif already_marked → REMOVE instance_missing_since, infrastructure.py:518-531) needed no change, and I verified it does cover the case where a row was stamped while live and deleted afterward — instance_missing becomes False on the next sweep once status == "deleted", so the existing already_marked branch fires and clears the marker. Matches the PR's claim.
• status == "deleted" as a raw string matches the pre-existing convention in this exact function (line 459 already used the same literal) — this is DynamoDB graph-registry state, a separate store from the Postgres GraphStatus enum used elsewhere in the codebase, so there's no inconsistency being introduced here.
• Edge case: a row with status == "deleted" but no deleted_at (shouldn't normally happen) still gets exempted correctly, since the new predicate checks status directly rather than depending on should_remove.

Tests

The three new cases in tests/operations/graph/test_infrastructure.py are well-targeted:

• test_recently_deleted_graph_is_not_orphaned is a genuine negative control per the PR description (not counted, not stamped, metric is 0).
• test_deleted_graph_marker_from_before_the_exemption_is_cleared exercises the clear-path claim directly.
• test_live_graph_is_still_orphaned_alongside_a_deleted_row proves the exemption doesn't overreach — a live orphan in the same sweep is still caught.

All three reuse the existing _make_dynamo_table fixture and mocking pattern already used throughout the file, so they're consistent with the surrounding test style.

Nits (non-blocking)

• None worth raising — the diff is minimal, the comments explain the "why" (the alarm-then-self-heal failure mode) rather than restating the code, and the docstring update accurately reflects the new behavior.

Note on verification

I could not execute uv run pytest / just lint in this sandbox (command execution requires approval that isn't available to me here), so I reviewed the logic and tests by static reading rather than re-running them. The PR body reports uv run pytest tests/operations/graph/test_infrastructure.py (33 passed) and the broader tests/operations/graph tests/dagster suite (992 passed), plus a documented negative-control run (revert → red → restore), which is good practice and I have no reason to doubt given the code inspection above.

Verdict: LGTM. No correctness, security, or style issues found.