fix: seed default Space for both fresh installs and existing deployments (#105)

[dolphinsboy]

Problem

Fixes #105. Also addresses the blocking issue raised in PR #115 review by Jerry-Xin, lml2468, and yujiawei.

PR #115 placed ensureAdminDefaultSpace() after the early-return guard, so existing deployments (admin row already present) never reached the repair path:

if (user != nil && user.UID != "") || m.ctx.GetConfig().AdminPwd == "" {
    return  // ← ensureAdminDefaultSpace() below is never called for existing admins
}
// ...
m.ensureAdminDefaultSpace()  // only runs on first-ever boot

Fix

Split the early-return guard into two cases so ensureAdminDefaultSpace() runs whenever the admin row exists or was just created:

adminExists := user != nil && user.UID != ""
if adminExists {
    m.ensureAdminDefaultSpace()  // idempotent — skips if Space already exists
    return
}
if m.ctx.GetConfig().AdminPwd == "" {
    return
}
// ... Insert new admin ...
m.ensureAdminDefaultSpace()  // fresh install path

ensureAdminDefaultSpace() is idempotent (GetUserDefaultSpaceID guard), so calling it on every boot is safe and cheap (one indexed SELECT).

Changes

• modules/space/api.go: expose CreateDefaultSpace(creatorUID, name string) error wrapping createSpaceCore
• modules/user/api_manager.go:
  • add spaceAPI *space.Space field, initialized in NewManager
  • split early-return into adminExists and AdminPwd=="" branches
  • call ensureAdminDefaultSpace() in both the existing-admin branch and after fresh Insert
  • add ensureAdminDefaultSpace() with idempotency guard and Warn-only failure

Testing

• go build ./... ✅
• go vet ./modules/user/... ./modules/space/... ✅
• Integration tests require live MySQL — rely on CI

[dolphinsboy]

Closing in favor of updating PR #115 directly. The fix has been pushed to the original branch.

[lml2468]

[COMMENT] Correct fix for both paths. CI all green.

✅ Root Cause Fix is Right

The adminExists split is the key improvement over PR #115 — the early-return that bypassed ensureAdminDefaultSpace() for existing admins is now resolved. Both branches (existing admin on restart, fresh install) call ensureAdminDefaultSpace(), which is idempotent thanks to the GetUserDefaultSpaceID guard.

🟡 Warning — GetUserDefaultSpaceID silently discards errors

// modules/space/db.go (pre-existing)
_, _ = ctx.DB().SelectBySql(`...`).Load(&spaceID)
return spaceID

If the DB query fails (network hiccup, schema not yet migrated), it returns "" — identical to "no space exists". ensureAdminDefaultSpace will then call CreateDefaultSpace, which will either succeed (creating a duplicate), fail with a constraint error (suppressed by Warn), or succeed silently. In practice the idempotency guarantee relies on DB availability at boot, which is generally true but worth noting.

This is pre-existing code, not introduced here. A follow-up to return (string, error) from GetUserDefaultSpaceID would harden the contract.

🔵 Suggestions

1. CreateDefaultSpace comment is in Chinese — per project convention, exported function doc comments should be in English:

// CreateDefaultSpace creates a default Space for the given user.
// Used by bootstrap paths (e.g., superAdmin first boot) to avoid a
// post-login deadlock when GET /v1/space/my returns empty.
// JoinMode=direct, MaxUsers=0 — matches the user-facing createSpace defaults.
func (s *Space) CreateDefaultSpace(creatorUID, name string) error {

2. Missing blank line before func getShowPhoneNum at the bottom of api_manager.go — minor style inconsistency.

3. PR #115 is still open — should be closed in favour of this one.

---

Logic is correct, CI passes, no blockers.

[lml2468]

I cannot complete the code review until the PR is mergeable and the gate checks are green.

Blocking items

1. PR metadata: mergeable is CONFLICTING. Please rebase or merge main into fix/superadmin-default-space-existing-deploy and resolve the conflicts before review.
2. Checks: check-sprint / check-sprint is failing, and the current Build, Test, Vet, Lint, and analyze / CodeQL (go) checks are still pending. Please get the gate checks to a clean state before re-requesting review.

Non-blocking notes

• I did not review the diff because the requested gate rules say to stop on CI red or merge conflicts.

Highlights

• Not evaluated; review is blocked at Phase 1.

[Jerry-Xin]

Scope check passed: this PR is directly relevant to octo-server because it changes user/space startup provisioning.

Summary: The intended repair is valid, but the current implementation is not safely idempotent and can still skip repair while the admin has no visible active Space.

🔴 Blocking

🔴 Critical — Non-atomic check-then-create can create duplicate admin Spaces during rolling or parallel startup.
In modules/user/api_manager.go, ensureAdminDefaultSpace() checks for an existing membership and then calls CreateDefaultSpace() separately at line 1213. Because users are allowed to belong to multiple Spaces, there is no database uniqueness constraint that prevents two server instances from both observing “no default Space” and each creating one. This startup repair path should be protected with a DB-level lock or another atomic mechanism around the check and creation.

🔴 Critical — The guard can skip repair even when the admin has no active Space.
ensureAdminDefaultSpace() uses space.GetUserDefaultSpaceID() at modules/user/api_manager.go:1210, but that helper only reads space_member and does not join the space table or require space.status=1 (modules/space/db.go:407). Meanwhile /v1/space/my only returns active Spaces through an inner join with space.status=1 (modules/space/db.go:80). A stale or orphaned membership row can therefore make the repair skip creation while the admin still sees an empty Space list.

💬 Non-blocking

🟡 Warning — CreateDefaultSpace() is exported but performs no input validation. Its current caller passes controlled values, but if this becomes a broader API, consider rejecting empty creator UID or name at modules/space/api.go:1847.

✅ Highlights

The PR correctly moves the repair path before the admin-exists return, so single-instance existing deployments with a clean missing-Space state should be repaired.

go build ./modules/user ./modules/space passed. go vet ./modules/user/... ./modules/space/... passed. Targeted go test ./modules/user ./modules/space could not run because the local test harness requires MySQL on 127.0.0.1:3306.

[Jerry-Xin]

This PR is relevant to octo-server, but the repair guard can still skip creation when the admin has no usable active Space.

🔴 Blocking

🔴 Critical: ensureAdminDefaultSpace() checks space.GetUserDefaultSpaceID() at modules/user/api_manager.go:1222, but that helper only reads space_member rows and does not verify the referenced space row is active (modules/space/db.go:409). The user-visible /v1/space/my path does require s.status=1 (modules/space/db.go:70), and user-side disband only marks space.status=0 without clearing space_member (modules/space/db.go:62). Result: if the admin’s only Space was disbanded, /v1/space/my still returns [], but startup sees the stale membership and skips the repair, leaving the same deadlock this PR is meant to fix. Use an active-space helper for this boot check, e.g. join space_member to space with s.status=1, and handle the DB error explicitly instead of using the error-swallowing helper.

💬 Non-blocking

🟡 Warning: The read-before-create sequence at modules/user/api_manager.go:1222-1225 is not concurrency-safe across multiple server instances. Since the schema only enforces uniqueness on (space_id, uid), two pods starting at the same time can both observe no active admin Space and create duplicate default Spaces. Consider serializing on the admin user row with SELECT ... FOR UPDATE inside a transaction, or another DB-backed lock, then re-checking before creation.

🟡 Warning: Please add a regression test for the stale-membership case: admin has an active space_member row pointing to a disbanded space, startup runs, and an active default Space is created.

✅ Highlights

The PR correctly moves the repair call into the existing-admin path, and reusing createSpaceCore keeps owner membership, invite creation, BotFather membership, cache refresh, and join events consistent with normal Space creation.

Verification note: go test ./modules/user/... ./modules/space/... could not complete locally because the test suite requires MySQL on 127.0.0.1:3306; compile reached the package tests before failing on the missing DB.

[dolphinsboy]

Hi @Jerry-Xin and @lml2468 👋

Could you help assign the linked Issue #105 to Sprint W22 on the Octo Board?

The check-sprint CI is currently failing with:

"Linked issue has no Sprint set. Please ask a Maintainer to assign the linked issue to Sprint W22 on the Octo Board"

All other checks (Build / Test / Vet / Lint / CodeQL / add-to-project) are ✅. This is the only blocker before merge.

Thanks!

[lml2468]

Verdict

APPROVED. The fix correctly addresses issue #105 for the primary scenarios (fresh install and existing deployment where admin lacks a default Space). CI gate: Build ✅, Lint ✅, Vet ✅, Test pending, check-sprint is a Board config issue — not code.

Analysis

Call chain verified (locked to head SHA e0abc80):
NewManager → createManagerAccount() → ensureAdminDefaultSpace() → space.GetUserDefaultSpaceID() (idempotency guard) → m.spaceAPI.CreateDefaultSpace() → createSpaceCore() (tx: insert space + owner member).

Early-return split is correct: the original guard combined "admin exists" and "no AdminPwd" into one condition, unreachably placing ensureAdminDefaultSpace after it. Now adminExists → ensure + return and AdminPwd=="" → return are separate branches. Both the existing-admin and fresh-install paths call ensureAdminDefaultSpace.

Idempotency: GetUserDefaultSpaceID does SELECT space_id FROM space_member WHERE uid=? AND status=1 ORDER BY created_at ASC LIMIT 1. If admin already has a member row, returns non-empty → early return. First boot: no member row → creates space. One indexed SELECT per boot — cheap.

space.New(ctx) in user module: creates a second *Space instance (separate from the shared singleton in space 1module.go init). This is fine — *Space is stateless (ctx + Log + db + mdb), no singleton state conflicts.

Non-blocking notes (P2 follow-ups, won't block merge)

1. Race at concurrent first boot (api_manager.go:1208-1215): If N replicas start simultaneously before any commits the space row, all see empty from GetUserDefaultSpaceID and all call createSpaceCore with different UUIDs — resulting in N spaces for the admin. Mitigation: INSERT ... ON DUPLICATE KEY or an advisory lock. Probability is near-zero for admin account creation (single UID, one-time event), so this is acceptable as a follow-up.

2. Inactive-space blind spot (modules/space/db.go:407-413): GetUserDefaultSpaceID queries space_member.status=1 but does not join space.status=1. If the admin's only space was disbanded, the guard would still return a space_id and skip creation, while /v1/space/my (which joins space.status=1) returns []. This is a pre-existing semantic gap in GetUserDefaultSpaceID rather than a regression introduced by this PR. Consider adding INNER JOIN space s ON s.space_id = space_member.space_id AND s.status=1 in a follow-up.

Highlights

• Clean separation of concerns: CreateDefaultSpace as a public API on the space module keeps the cross-module coupling minimal.
• Thorough doc comments explaining the issue #105 deadlock scenario and the "Warn-only" rationale.
• Idempotent design with minimal boot-time cost (one indexed SELECT).

[Jerry-Xin]

This PR is in scope for Mininglamp-OSS/octo-server and correctly targets the admin bootstrap/default Space repair path.

💬 Non-blocking

🟡 Warning: ensureAdminDefaultSpace is check-then-create and is not atomic. In a multi-instance deployment, two servers starting at the same time can both observe no membership at modules/user/api_manager.go:1222, then both call CreateDefaultSpace at modules/user/api_manager.go:1225. Since the schema allows multiple Spaces per creator, this can seed duplicate admin Spaces. Consider a DB-level lock, deterministic bootstrap row, or transaction-safe repair path if clustered startup is supported.

🟡 Warning: The guard uses GetUserDefaultSpaceID, which only checks active space_member rows and does not join active space rows at modules/space/db.go:410. mySpaces does require s.status=1 at modules/space/db.go:73, so an orphaned or inactive Space membership would make the repair skip while /v1/space/my still returns empty. This is probably outside the reported no-membership case, but the repair helper would be more robust if it checked for an active visible Space.

🔵 Suggestion: Prefer GetUserDefaultSpaceIDE in modules/user/api_manager.go:1222 so startup logs can distinguish “no default Space” from “default Space lookup failed.” The current deprecated helper swallows DB errors.

🔵 Suggestion: Add a focused regression test for both branches: existing admin without Space gets repaired, and fresh admin creation seeds a Space plus owner membership. The repo’s tests require live MySQL, but this behavior is important enough to cover in CI.

✅ Highlights

The PR fixes the PR #115 control-flow issue by running the repair path when the admin row already exists at modules/user/api_manager.go:1174.

CreateDefaultSpace reuses createSpaceCore at modules/space/api.go:1988, preserving the normal Space creation side effects such as owner membership, default invite, cache refresh, category provisioning, and events.

Local verification: go test ./modules/user ./modules/space could not complete because the test environment has no MySQL server at 127.0.0.1:3306.