chore(office): delete unwired GC and fix stale scheduler doc - #3478
chore(office): delete unwired GC and fix stale scheduler doc#3478nova28 wants to merge 1 commit into
Conversation
gc.go's GarbageCollector was never constructed in production (office.Services.GC was always nil); the same container/worktree reclamation is already handled by the wired, scheduled internal/system/storage subsystem with stronger guards (canonical terminal-state check, archived_at, live env/executor rows, and quarantine-with-retention instead of os.RemoveAll). Delete gc.go and gc_test.go and move the infra package doc onto reconcile.go. Also corrects the scheduler package comment, which claimed a tick loop, event subscribers, and idle-timeout management it does not own (idle-timeout management was removed package-wide; the tick loop lives in office/service/scheduler_integration.go; the scheduler only publishes to the event bus, it does not subscribe).
GitHub Actions
Inactive
|
Claude finished @nova28's task in 3m 51s —— View job FindingsSuggestion (recommended, doesn't block)
Summary
Verdict: Ready to merge The deletion is clean and well-justified:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Team Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (3)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe office garbage collector implementation and tests were deleted. The ChangesOffice garbage collector removal
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This removes an unused office garbage collector and updates package descriptions without changing the retained scheduler or reconciler wiring. No current merge-readiness risk is identified. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 369881a1a5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Workspaces *officeservice.Service | ||
| Documents *taskservice.DocumentService | ||
| GC *infra.GarbageCollector | ||
| Reconciler *infra.Reconciler |
There was a problem hiding this comment.
After removing gc.go and gc_test.go, accepted ADR docs/decisions/0009-fail-closed-gc-semantics.md still identifies those deleted files as the implementation and regression tests (lines 51-52), and its consequences still claim the Office collector is wired (line 41). Maintainers following the accepted decision will therefore reach nonexistent code and an obsolete architecture; amend the ADR to record that internal/system/storage superseded this collector and point its implementation and test references at the replacement.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -1,3 +1,4 @@ | |||
| // Package infra provides infrastructure-level background jobs for the office domain. | |||
There was a problem hiding this comment.
Nit: "background jobs" no longer fits — the GarbageCollector (a scheduled background job) was the only occupant matching that description. What remains is Reconciler, which runs once synchronously at startup. Consider:
| // Package infra provides infrastructure-level background jobs for the office domain. | |
| // Package infra provides infrastructure-level startup reconciliation for the office domain. |
|
| Filename | Overview |
|---|---|
| apps/backend/internal/office/infra/gc.go | Deletes a garbage collector with no production construction or lifecycle wiring; equivalent cleanup remains independently scheduled. |
| apps/backend/internal/office/infra/gc_test.go | Removes tests dedicated solely to the deleted, unwired implementation. |
| apps/backend/internal/office/services.go | Removes the unused GarbageCollector field without affecting observed production composition. |
| apps/backend/internal/office/infra/reconcile.go | Adds the package comment to a surviving infra source file. |
| apps/backend/internal/office/scheduler/run.go | Corrects several stale ownership claims, but still inaccurately attributes production run processing to this package. |
Reviews (1): Last reviewed commit: "chore(office): delete unwired GC and fix..." | Re-trigger Greptile
1 participant
Tip
PR walkthrough: Open the visual walkthrough
Summary
office/infra/gc.go'sGarbageCollectorwas never constructed in production —office.Services.GCwas always nil, and its only construction site wasgc_test.go. The wired, scheduledinternal/system/storagesubsystem already does both sweeps (containers, worktrees) with strictly stronger guards (canonical terminal-state check viamodels.IsTerminalTaskState,archived_at, live env/executor rows, and quarantine-with-retention instead ofos.RemoveAll). Deletedgc.goandgc_test.go, moved theinfrapackage doc ontoreconcile.go.office/scheduler/run.go's package comment claimed a tick loop, event subscribers, and idle-timeout management it does not own (idle-timeout management was removed package-wide; the tick loop lives inoffice/service/scheduler_integration.go; the scheduler only publishes to the event bus, it does not subscribe). Corrected to describe what it actually owns: run processing, dispatch/tier routing, retry logic, reactivity, and mentions.No runtime/behavior change — the deleted GC never ran, and the doc comment was not user-visible.
Test plan
go build -tags fts5 ./...go test -tags fts5 ./internal/office/...— all packages passgo test -tags fts5 ./internal/backendapp/...— confirmsoffice.Servicesconstruction site still compiles/works without the removed fieldgofmt -lon touched files — cleangolangci-lint run ./internal/office/... --new-from-rev=cd7823631— 0 issuesgo vet -tags fts5 ./internal/office/...— cleango test -tags fts5 ./internal/worktree/...has ~30 pre-existing failures on this host (macOS temp-dir/symlink issues), confirmed identical against the unmodified base commit — unrelated to this change, which never touchesinternal/worktree.Follow-up filed for
worktree.Manager.ListActiveWorktreePaths/SQLiteStore.ListActiveWorktreePaths, which have zero production callers after this delete (out of scope here — needs an interface change and touches 3 test files).Preview Environment
369881a