Repository navigation
Retire hosted tenants before account deletion - #131
Conversation
📝 WalkthroughWalkthroughThe pull request adds shared release verification with streamed GitHub comparison parsing. It also adds tenant retirement, Stack tenant deletion, cross-platform usage locks, fresh resolution, stale-request protection, authorization, recovery, and lifecycle tests. ChangesRelease verification
Tenant lifecycle management
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant StackClient
participant MultitenantProxy
participant TenantRegistry
participant UseLock
StackClient->>MultitenantProxy: Request tenant deletion
MultitenantProxy->>TenantRegistry: Authorize and retire tenant
MultitenantProxy->>UseLock: Acquire exclusive tenant-use lock
UseLock-->>MultitenantProxy: Lock acquired or pending
MultitenantProxy->>TenantRegistry: Delete retired tenant
TenantRegistry-->>MultitenantProxy: Deletion result
MultitenantProxy-->>StackClient: Deletion response
sequenceDiagram
participant DeploymentScript
participant VerifyReleaseOnMain
participant GitHub
DeploymentScript->>VerifyReleaseOnMain: Repository, tag, expected revision
VerifyReleaseOnMain->>GitHub: Resolve tag and request comparison
GitHub-->>VerifyReleaseOnMain: Commit and comparison data
VerifyReleaseOnMain-->>DeploymentScript: Verified revision or error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96118f2e26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
internal/proxy/multitenant.go (3)
384-385: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared team authorization rule.
The same two-part check now exists here and at Lines 485-486 in
handleStackTenantDelete, including theStackTeamsfallback. One helper that takesteamID,claims, and the token keeps both handlers aligned when the rule changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/proxy/multitenant.go` around lines 384 - 385, Extract the shared two-part team authorization check from the current handler and handleStackTenantDelete into a helper accepting teamID, claims, and the token, including the StackTeams fallback. Replace both inline checks with the helper so both handlers use the same authorization rule.
511-519: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable
ErrTenantRetiredbranch.
RetireExternalininternal/tenant/tenant.go:402-433returns onlynil, a validation error, or an I/O error. It never returnsErrTenantRetired; an already-retired tenant returns(true, nil). The 409 branch is dead code.♻️ Proposed cleanup
retired, err := m.Registry.RetireExternal(teamID) if err != nil { - if errors.Is(err, tenant.ErrTenantRetired) { - http.Error(w, "tenant is retired", http.StatusConflict) - return - } http.Error(w, "tenant retirement failed", http.StatusInternalServerError) return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/proxy/multitenant.go` around lines 511 - 519, Remove the unreachable errors.Is check for tenant.ErrTenantRetired in the retirement handler around Registry.RetireExternal, including its 409 response, and retain the generic internal-server-error response for all returned errors.
532-540: 🩺 Stability & Availability | 🔵 TrivialAdd a server-side sweep for tenants stuck in the retired state.
When
TryAcquireExclusiveUsefails, the tenant stays retired with its credential-bearing directory intact, and cleanup depends on the client sending another DELETE. If the client never retries, the state persists. A background sweep that retriesDeleteRetiredfor retired, keyless tenants makes the deletion durable without client cooperation. Add a metric for the count of tenants in the retired-pending state so the backlog is observable.[operational_advice]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/proxy/multitenant.go` around lines 532 - 540, Add a background cleanup sweep that identifies retired, keyless tenants and retries Registry.DeleteRetired for each, including tenants left behind when TryAcquireExclusiveUse fails; ensure the sweep runs independently of subsequent DELETE requests and safely handles concurrent cleanup. Expose a metric counting tenants currently in the retired-pending state, and update it as the sweep observes changes. Integrate this with the existing multitenant lifecycle near forgetTenant and DeleteRetired without changing successful DELETE behavior.internal/tenant/use_lock_unix.go (1)
18-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pruning stale lock files.
Every tenant ID creates a permanent file under
tenant-use-locks.DeleteRetiredremovesr.Dir(id)but not the lock file, so entries accumulate for the lifetime of the state directory. A periodic sweep of lock files whose tenant has a tombstone keeps the directory bounded. Do not delete a lock file while a holder can still be insideAcquireUse, because a new opener would then get a different inode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tenant/use_lock_unix.go` around lines 18 - 27, Update the tenant-use-lock maintenance flow around Registry.openUseLock and DeleteRetired to periodically remove lock files only for tenants with tombstones, while retaining locks for active or non-tombstoned tenants. Coordinate pruning with AcquireUse so a lock file is never deleted while an existing holder may still be inside the critical section; preserve inode continuity for concurrent openers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/gcp/verify-release-on-main.sh`:
- Around line 47-53: Update the comparison pipeline in the release verification
check to inspect PIPESTATUS[0] after gh api and jq run. If gh api fails, report
the transport/API failure distinctly and exit nonzero; otherwise retain the
existing "${tag} is not on main" message for a valid comparison that fails the
jq condition, without introducing response buffering or a here-string.
In `@internal/tenant/tenant.go`:
- Around line 455-479: Update the tenant deletion logic around the found/kept
filtering loop to build kept as a new independent slice rather than using
file.Tenants[:0]. Preserve validation and deletion behavior while ensuring
validation failures and the interval before save cannot mutate or expose cached
tenant state.
- Around line 467-489: Update the tenant retirement flow around RetireExternal
to remove the tenant state before creating the tombstone, and only write the
tombstone when found or hadState is true. Ensure os.RemoveAll failures return
without creating a tombstone, while preserving the existing save behavior and
idempotent repeated-deletion result.
In `@internal/tenant/use_lock_windows.go`:
- Around line 36-42: Update Registry.TryAcquireExclusiveUse so Windows reports
existing lock contention instead of always returning acquired=true. Implement
actual Windows-exclusive locking with LockFileEx, or defer tombstone creation in
DeleteRetired until os.RemoveAll succeeds and retry while the tenant directory
remains; preserve the 202 pending path for blocked tenants.
---
Nitpick comments:
In `@internal/proxy/multitenant.go`:
- Around line 384-385: Extract the shared two-part team authorization check from
the current handler and handleStackTenantDelete into a helper accepting teamID,
claims, and the token, including the StackTeams fallback. Replace both inline
checks with the helper so both handlers use the same authorization rule.
- Around line 511-519: Remove the unreachable errors.Is check for
tenant.ErrTenantRetired in the retirement handler around
Registry.RetireExternal, including its 409 response, and retain the generic
internal-server-error response for all returned errors.
- Around line 532-540: Add a background cleanup sweep that identifies retired,
keyless tenants and retries Registry.DeleteRetired for each, including tenants
left behind when TryAcquireExclusiveUse fails; ensure the sweep runs
independently of subsequent DELETE requests and safely handles concurrent
cleanup. Expose a metric counting tenants currently in the retired-pending
state, and update it as the sweep observes changes. Integrate this with the
existing multitenant lifecycle near forgetTenant and DeleteRetired without
changing successful DELETE behavior.
In `@internal/tenant/use_lock_unix.go`:
- Around line 18-27: Update the tenant-use-lock maintenance flow around
Registry.openUseLock and DeleteRetired to periodically remove lock files only
for tenants with tombstones, while retaining locks for active or non-tombstoned
tenants. Coordinate pruning with AcquireUse so a lock file is never deleted
while an existing holder may still be inside the critical section; preserve
inode continuity for concurrent openers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4443aa19-a890-4991-a034-476cac512ad2
📒 Files selected for processing (8)
cmd/subrouter/deploy_scripts_test.godeploy/gcp/golden-local-mac-production-continuity.shdeploy/gcp/verify-release-on-main.shinternal/proxy/multitenant.gointernal/proxy/multitenant_test.gointernal/tenant/tenant.gointernal/tenant/use_lock_unix.gointernal/tenant/use_lock_windows.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc9a0e4b3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
internal/tenant/lock_windows.go (1)
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare one set of Win32 lock bindings.
lock_windows.goanduse_lock_windows.goboth bindkernel32.dll,LockFileEx, andUnlockFileEx, and both repeat the same six-argumentCallshape and the same unlock-then-close sequence. Extract one Windows-only helper file with the procedure bindings and smalllockWholeFile(file, flags)andunlockWholeFile(file, overlapped)functions. Both lock types can then use it. This removes the duplicated flag values and keeps one place to fix if the failure-code handling changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tenant/lock_windows.go` around lines 19 - 23, Consolidate the duplicated Win32 bindings and lock/unlock calls from lock_windows.go and use_lock_windows.go into one Windows-only helper file. Define shared kernel32.dll procedure bindings plus lockWholeFile(file, flags) and unlockWholeFile(file, overlapped), then update both lock implementations to use these helpers while preserving existing failure-code handling and unlock-before-close behavior.internal/tenant/tenant_test.go (1)
163-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a unit test for
PendingDeletionIDs.The new tests cover retirement intent and key rejection. They do not cover
PendingDeletionIDs, which drives crash recovery. Add a test that leaves only aretiring-tenantsmarker and asserts the ID is returned, and a test that asserts a retired registry entry is returned. This also pins the deduplication and sort order.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tenant/tenant_test.go` around lines 163 - 183, Add unit coverage for PendingDeletionIDs by testing a registry containing only a retiring-tenants marker and asserting that marker’s tenant ID is returned, then testing a retired registry entry and asserting its ID is returned. Include both sources together to verify duplicate IDs are returned once and results are sorted deterministically, using the existing registry setup and tenant identifiers.internal/proxy/multitenant.go (1)
583-612: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGive the retry loop a stop signal that does not depend on
Lifecycle.The loop exits only on success or when
m.Base.Lifecycle != nil && m.Base.Lifecycle.Draining()is true. WhenLifecycleis nil, the worker retries forever and logs an error every 30 seconds.cmd/subrouter/main.goalways setsLifecycle, so production is covered. Tests and any other embedding are not:TestStackTenantDeletionRetriesAfterExclusiveLockSetupFailurebuildsServer{MaxBodyBytes: 1024}with noLifecycle, so a worker that keeps failing outlives the test and keeps writing to the test logger.Add a stop channel on
MultiTenant, or accept acontext.Context, and select on it beside the backoff timer. This also lets a caller cancel recovery deterministically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/proxy/multitenant.go` around lines 583 - 612, Add a cancellation signal to the background tenant-deletion retry loop surrounding Registry.AcquireExclusiveUse and DeleteRetired, using a MultiTenant stop channel or context.Context; select on that signal alongside the backoff timer and return when cancellation occurs, without relying on Base.Lifecycle. Ensure the signal can be triggered by callers or teardown so retries stop deterministically when Lifecycle is nil.internal/tenant/lock_windows_test.go (1)
10-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Windows execution coverage for the lock tests.
.github/workflows/ci.yml,.github/workflows/release.yml, and release build coverage all run onubuntu-latestand useGOOScross-builds. The new//go:build windowstests compile only on Windows, so theLockFileExbehavior is not executed by the existing CI jobs. Run these tests on a Windows runner, or document that Windows lock behavior is unverified. Also renameTestWindowsRegistryLockSerializesProcessesto avoid implying separate processes; this test uses two handles in one process.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tenant/lock_windows_test.go` around lines 10 - 13, Rename TestWindowsRegistryLockSerializesProcesses to reflect that it verifies serialization between two handles in one process, not separate processes. Add Windows-runner execution coverage for the Windows-only lock tests in the CI or release workflow, ensuring LockFileEx behavior is exercised; if workflow changes are out of scope, explicitly document that Windows lock behavior remains unverified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/gcp/verify-release-on-main.sh`:
- Around line 47-58: Update the comparison failure handling after the gh api/jq
pipeline to inspect comparison_status[1] for jq processing failures. Report a
comparison-fetch/processing error when jq fails, and reserve the "${tag} is not
on main" message for successfully processed responses that do not satisfy the
ancestor check.
In `@internal/proxy/multitenant_test.go`:
- Around line 687-712: Update the two wait loops in the test to use independent
deadlines, assigning a fresh, longer timeout immediately before the second loop
that checks multi.deletions. Keep the existing polling and timeout assertions
unchanged while increasing the timeout budget so background deletion retries can
complete.
In `@internal/proxy/multitenant.go`:
- Around line 519-529: Update the RetireExternal error path in the tenant
retirement handler to call scheduleTenantDeletion(teamID) before returning the
500 response, matching the subsequent TryAcquireExclusiveUse failure path. Keep
the existing error response and control flow unchanged after scheduling.
- Around line 95-97: Update the proxied-request flow around serveResolvedTenant
so AcquireUse and useLock.Close cover only the short tenant validation, then
release the lock before handler initialization and forwarding. Ensure
serveResolvedTenant no longer holds the per-tenant use lock across the
downstream handler lifetime, while preserving the existing resolved-tenant
validation behavior.
---
Nitpick comments:
In `@internal/proxy/multitenant.go`:
- Around line 583-612: Add a cancellation signal to the background
tenant-deletion retry loop surrounding Registry.AcquireExclusiveUse and
DeleteRetired, using a MultiTenant stop channel or context.Context; select on
that signal alongside the backoff timer and return when cancellation occurs,
without relying on Base.Lifecycle. Ensure the signal can be triggered by callers
or teardown so retries stop deterministically when Lifecycle is nil.
In `@internal/tenant/lock_windows_test.go`:
- Around line 10-13: Rename TestWindowsRegistryLockSerializesProcesses to
reflect that it verifies serialization between two handles in one process, not
separate processes. Add Windows-runner execution coverage for the Windows-only
lock tests in the CI or release workflow, ensuring LockFileEx behavior is
exercised; if workflow changes are out of scope, explicitly document that
Windows lock behavior remains unverified.
In `@internal/tenant/lock_windows.go`:
- Around line 19-23: Consolidate the duplicated Win32 bindings and lock/unlock
calls from lock_windows.go and use_lock_windows.go into one Windows-only helper
file. Define shared kernel32.dll procedure bindings plus lockWholeFile(file,
flags) and unlockWholeFile(file, overlapped), then update both lock
implementations to use these helpers while preserving existing failure-code
handling and unlock-before-close behavior.
In `@internal/tenant/tenant_test.go`:
- Around line 163-183: Add unit coverage for PendingDeletionIDs by testing a
registry containing only a retiring-tenants marker and asserting that marker’s
tenant ID is returned, then testing a retired registry entry and asserting its
ID is returned. Include both sources together to verify duplicate IDs are
returned once and results are sorted deterministically, using the existing
registry setup and tenant identifiers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fa66da5a-9aa3-43b3-b5fd-7cfac8bde8a3
📒 Files selected for processing (13)
cmd/subrouter/deploy_scripts_test.gocmd/subrouter/main.gocmd/subrouter/main_test.godeploy/gcp/verify-release-on-main.shinternal/proxy/multitenant.gointernal/proxy/multitenant_test.gointernal/tenant/lock_windows.gointernal/tenant/lock_windows_test.gointernal/tenant/tenant.gointernal/tenant/tenant_test.gointernal/tenant/use_lock_unix.gointernal/tenant/use_lock_windows.gointernal/tenant/use_lock_windows_test.go
1 participant
Retires Stack-owned hosted tenants before identity deletion, revokes keys before draining in-flight requests, and leaves durable non-secret tombstones. Also streams release ancestry verification so large GitHub comparisons cannot deadlock the continuity harness.
Tests: go test ./...
Tests: go test -race ./internal/proxy ./internal/tenant
Tests: Windows cross-compile for internal/tenant and internal/proxy
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Retires Stack-owned hosted tenants before identity deletion. Revokes keys, blocks new requests, drains in-flight traffic, and deletes state with durable tombstones and crash-safe recovery (including when the tenant never existed). Also streams release ancestry verification and clearly classifies failures.
New Features
/_subrouter/auth/stack/tenant: requiresX-Subrouter-Tenant-Delete-Token(set via--stack-tenant-delete-token/SUBROUTER_STACK_TENANT_DELETE_TOKEN) and Stack team membership; token must be ≥32 bytes. Returns 202 while draining, then deletes; idempotent, resumes pending deletions on restart, and writes durable tombstones for both existing and absent tenants.ResolveFresh; exclusive locks (AcquireUse,TryAcquireExclusiveUse) gate deletion; registry exposesRetireExternal,DeleteRetired,PendingDeletionIDs, andErrTenantRetired./_subrouter/auth/stackreturns 410 Gone for retired tenants; unknownsrt_credentials always return 401.Bug Fixes
SUBROUTER_STACK_TENANT_DELETE_TOKENis unset; the delete endpoint is disabled unless configured.deploy/gcp/verify-release-on-main.shto stream large GitHub comparisons intojqand accurately classify transport vs. processing failures; allowed test-only probe/egress tolerance overrides to reduce CI jitter.Written for commit fedb1d0. Summary will update on new commits.
Summary by CodeRabbit
New Features
410 Gone, and support crash recovery and cleanup.Bug Fixes