Check Linked from sources again inside the store's lock

[SirLouen]

Closes #249

What

• The registry hands its own check to eight store writes: create a field, edit a field, update a group, move a field, delete a field, delete a sub field, delete a group, and delete a group's named fields.
• The Postgres store runs that check again right after it takes the field groups lock, on the groups and types it reads inside the lock. When the check fails, the store writes nothing and answers with the error the registry would give.
• Editing a field now takes the field groups lock too.

Why

The registry checked a Linked from field and the relation it reads before the store took its lock. Two admins saving at the same moment could both pass that check. One could delete a relation while the other declared a Linked from reading it, and the site kept a Linked from that reads nothing.

Testing Instructions

1. Run make cover GOTESTFLAGS=-race. Every package stays at 100%.
2. Run go test ./test/features/ -run TestBacklinks. Three new scenarios send a delete and a Linked from change at the same moment, and the second request is refused each time.
3. Run go test ./internal/postgres/ -run 'TestALinkedFrom|TestARelationDeletedAsALinkedFrom|TestTheGroupWritesStoreNothingTheirCheckRefuses'. The race tests queue two real writes on the lock, and the second one is refused with nothing stored.

Summary by CodeRabbit

• Bug Fixes
  • Group and field changes are checked against the latest content before they’re saved, reducing conflicts when edits happen at the same time.
  • Deleting or moving fields is prevented when they’re still referenced by other fields or readers.
  • Backlink declarations and updates are rejected when their relation source no longer exists. Conflicting changes report clearer errors, such as a missing group or field.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: gopherium/gophenberg/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 3a906f4d-827f-46d7-b274-1a4e7a622c3d

📥 Commits

Reviewing files that changed from the base of the PR and between 3e490fa and 57232f0.

📒 Files selected for processing (1)

• test/features/memorytypes_test.go

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Group and field mutations now pass recheck callbacks to stores. Stores re-evaluate group, field, and backlink relationships after acquiring locks and before applying changes. New tests cover concurrent backlink operations and mutations whose referenced groups, fields, or readers change meanwhile.

Changes

Content registry and recheck contract

Layer / File(s)	Summary
Define and wire registry rechecks internal/content/recheck.go, internal/content/types.go, internal/content/registry_groups.go, internal/content/*_test.go, internal/seed/*_test.go, internal/server/*_test.go	TypeStore mutation methods accept Recheck callbacks. Registry operations build checks for group and field writes, deletes, and moves, then pass them to stores. Registry tests cover concurrent changes and errors surfaced during judgment.

PostgreSQL mutation checks

Layer / File(s)	Summary
Run rechecks within PostgreSQL mutations internal/postgres/groups.go	Group and field mutation paths run callbacks after locking groups and before writing or deleting. Callback errors are returned unchanged.
Validate PostgreSQL rechecks and update call sites internal/postgres/groups_recheck_test.go, internal/postgres/*_test.go	New tests cover rejected writes, read and lock failures, and competing backlink operations. Existing store tests pass nil callbacks to the updated methods.

Memory store and feature coverage

Layer / File(s)	Summary
Apply rechecks in the memory store and feature tests test/features/memorytypes_test.go, test/features/features/backlinks.feature, test/features/steps_backlinks_test.go	The memory store runs callbacks under its mutex before mutations. Feature scenarios test simultaneous backlink declarations and repoints with relation deletion.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: 🔵 Low · up to 57232

Concurrent requests in the feature-test server could lose a newly written field value. The risk is confined to that test implementation and does not block merging.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 26.80% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 194 functions across 35 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: rechecking Linked from sources while the store lock is held to prevent concurrent invalid references.
Linked Issues check	✅ Passed	Issue #249 requires rechecking the Linked from source and relation readers inside the locked transaction. internal/content/recheck.go builds checks for source validity and reader validity. `internal…
Out of Scope Changes check	✅ Passed	The production changes thread the recheck through the store interface and affected mutation paths. The memory store, test fakes, call sites, and added tests support this interface and verify the linke…

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[greptile-apps]

Confidence Score: 5/5

No blocking findings remain. The two outstanding concerns are non-blocking.

Findings

1.  Race scenarios run sequentially ▶
2.  Label edits load every definition ▶

Fix with agent prompt

### Issue 1
test/features/steps_backlinks_test.go:113-116
These scenarios say requests arrive at the same moment, but the first request finishes before the second begins. They test a stale registry view, not overlapping requests, so they cannot catch failures that require lock contention. Relabel them or add a concurrent case. This coverage gap does not block merging.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

### Issue 2
internal/postgres/groups.go:undefined-614
A plain text-field label edit now reads every group, field, and type under the field-groups lock before updating one row. This adds work as definitions grow and lets an unrelated group write delay the edit. Narrow the locked recheck when the edit cannot affect a Linked from source. This performance concern does not block merging.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

Bulk field removal now checks and removes declarations under one lock. The previously reported sequential race scenarios and broad reads during label edits remain unchanged.

_{Reviews (2) · Last reviewed commit: "test(features): check and drop a group's..."}

[greptile-apps]

Race scenarios run sequentially

These scenarios say requests arrive at the same moment, but the first request finishes before the second begins. They test a stale registry view, not overlapping requests, so they cannot catch failures that require lock contention. Relabel them or add a concurrent case. This coverage gap does not block merging.

Artifacts

Authored request-order probe

• The executed Go test instruments the test server’s HTTP transport and checks request order and the registry view.

Plain sequential control output

• The control run shows DELETE finishing before POST begins, with the deleted field absent from the subsequent listing.

Judged-together output

• The PR-path run shows the same non-overlapping request order, but the frozen listing still contains the deleted field.

Backlinks feature-suite output

• The targeted feature-suite run passed all 24 scenarios, including the three described as occurring at the same moment.

_{Ran code and verified through T-Rex}

Prompt To Fix With AI

This is a comment left during a code review.
Path: test/features/steps_backlinks_test.go
Line: 113-116

Comment:
**Race scenarios run sequentially**

These scenarios say requests arrive at the same moment, but the first request finishes before the second begins. They test a stale registry view, not overlapping requests, so they cannot catch failures that require lock contention. Relabel them or add a concurrent case. This coverage gap does not block merging.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[greptile-apps]

Label edits load every definition

A plain text-field label edit now reads every group, field, and type under the field-groups lock before updating one row. This adds work as definitions grow and lets an unrelated group write delay the edit. Narrow the locked recheck when the edit cannot affect a Linked from source. This performance concern does not block merging.

Artifacts

Authored PostgreSQL label-edit test

• The test runs both label-edit paths against a migrated database and records their SQL and lock behavior, making the comparison reproducible.

Direct label edit before the change

• The baseline command edited a text-field label with one SQL statement and succeeded while another session held the group lock, showing the previous path did not wait on it.

Label edit through the changed store method

• The changed-method command traced seven statements and a lock timeout during the same competing-lock scenario, confirming the added reads and contention.

_{Ran code and verified through T-Rex}

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/postgres/groups.go
Line: 614

Comment:
**Label edits load every definition**

A plain text-field label edit now reads every group, field, and type under the field-groups lock before updating one row. This adds work as definitions grow and lets an unrelated group write delay the edit. Narrow the locked recheck when the edit cannot affect a Linked from source. This performance concern does not block merging.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!