fix: serialise redirect writes so concurrent saves cannot store a loop (WR-10)
The save-time simulation read the rules at READ COMMITTED without a lock, so A -> B and B -> A written concurrently both passed. CheckRedirect, AddressChanged and every batch now take a transaction-level advisory lock before they read the rules.
This commit is contained in:
@@ -23,7 +23,7 @@ The frontend (a Nuxt layer or app) asks `GET /_seo/api/v1/resolve?url=...` for t
|
||||
- **Settings** (`golem15_seo_settings`, one row): site name, title and home templates with `%title%`, `%description%` and `%site%`, default description and social image, X account, the query-parameter list (content, filter and tracking parameters, optionally per path), never-indexed and search paths, address options and verification codes and files. One tab per content type (`golem15_seo_type_settings`) holds the inject switch, the title and description templates and the default robots (tag types default to `noindex,follow`).
|
||||
- **Resolver.** Canonical URLs are self-referencing and absolute from the configured site URL, keep content parameters and drop filter and tracking parameters; overrides may be relative or absolute. Robots resolve per directive and are also answered as the `X-Robots-Tag` value. Open Graph and Twitter values inherit from the effective title, description and image. Verification meta tags come with the home page.
|
||||
- **Redirects** (`golem15_seo_redirects`): 301, 302 and 410 rules with exact, pattern (`/old/*` → `/new/*`) and regex (`^/p/(\d+)$` → `/post/$1`) sources, evaluated exact, then patterns (longest literal prefix first), then regex. Options: ignore letter case, match with and without a trailing slash, ignore or pass the query string; a source with `?` matches its query exactly and wins over the same path without a query. Regex sources use the standard library (RE2, linear time) and are capped at 512 characters. A capture never turns a path target into another host, and an absolute target keeps its own host.
|
||||
- **Loops and chains.** Every redirect write runs a simulation with the rule in place (a probe URL for patterns and regex, at most 10 hops): a loop is refused, from the admin with `redirects.loop` on the target and nothing saved; a chain is saved with `redirects.chain_notice` naming the final target and is marked in the list.
|
||||
- **Loops and chains.** Every redirect write runs a simulation with the rule in place (a probe URL for patterns and regex, at most 10 hops): a loop is refused, from the admin with `redirects.loop` on the target and nothing saved; a chain is saved with `redirects.chain_notice` naming the final target and is marked in the list. Redirect writes take a transaction-level advisory lock before the simulation, so two concurrent writes cannot together store a loop.
|
||||
- **Address changes.** When a published record's address changes, a 301 from the old to the new address is created, earlier redirects to the old address are pointed straight at the new one, and an exact redirect that matches the new address (with its case and trailing-slash options) is removed (a slug changed back and forth ends with one redirect per old address). The change is simulated first: when it would loop through a pattern or regex rule, or an address is not a path on the site, no redirect is written and a warning is logged. In the admin this happens inside the save transaction and the editor gets `redirects.auto_created` on the slug field; writes outside the admin (Journal's API, MCP, imports) are handled after commit through `pact.RecordSaved`. Deleting or unpublishing a record creates nothing.
|
||||
- **404 log** (`golem15_seo_notfound`): the frontend reports each 404 to `POST /_seo/api/v1/notfound`; the log keeps the address, hits, first seen, last hit and last referer. An entry is resolved with one click: **Create redirect…** (a prompted 301), **Mark as gone** (410), or a bulk 410.
|
||||
- **URL normalisation** (settings tab Addresses): https, `www` or bare host, trailing slash added or removed, optional lowercasing of ASCII letters, double slashes and `/index.html` collapsed. The defaults change nothing but scheme, slashes and index files.
|
||||
|
||||
@@ -96,6 +96,9 @@ func (s *Service) applyAddressRedirects(ctx context.Context, tx *gorm.DB, res *A
|
||||
return nil
|
||||
}
|
||||
|
||||
if err := lockRedirectWrites(tx.WithContext(ctx)); err != nil {
|
||||
return err
|
||||
}
|
||||
rules, err := LoadRedirects(tx)
|
||||
if err != nil {
|
||||
return err
|
||||
|
||||
@@ -241,6 +241,11 @@ func (s *Service) runBatch(ctx context.Context, opts BatchOptions, build func(ct
|
||||
if opts.Via != "" {
|
||||
ctx = WithVia(ctx, opts.Via)
|
||||
}
|
||||
// One lock for the whole batch, so a refused change rolled back to
|
||||
// its savepoint does not let another redirect writer in (WR-10).
|
||||
if err := lockRedirectWrites(tx); err != nil {
|
||||
return err
|
||||
}
|
||||
changes, err := build(ctx, tx)
|
||||
if err != nil {
|
||||
return err
|
||||
|
||||
@@ -165,7 +165,14 @@ func classHas(ranges []rune, c rune) bool {
|
||||
// matches comes back round or does not end within MaxRedirectHops, a
|
||||
// chain when it takes more than one hop or when another rule's target
|
||||
// leads into r. A 410 rule ends every chain and is never a loop.
|
||||
//
|
||||
// db must be the writer's transaction: CheckRedirect takes the redirect
|
||||
// write lock (lockRedirectWrites) before it reads, so two concurrent writes
|
||||
// cannot each pass the simulation without the other's rule.
|
||||
func (s *Service) CheckRedirect(ctx context.Context, db *gorm.DB, r models.Redirect) (RedirectCheck, error) {
|
||||
if err := lockRedirectWrites(db.WithContext(ctx)); err != nil {
|
||||
return RedirectCheck{}, err
|
||||
}
|
||||
if r.Status == models.Status410 {
|
||||
return RedirectCheck{}, nil
|
||||
}
|
||||
@@ -176,6 +183,18 @@ func (s *Service) CheckRedirect(ctx context.Context, db *gorm.DB, r models.Redir
|
||||
return checkRedirectRules(s.SiteURL(), rules, r)
|
||||
}
|
||||
|
||||
// redirectLockKey is the transaction-level advisory lock that serialises
|
||||
// redirect writes ("g15seord").
|
||||
const redirectLockKey int64 = 0x6731357365_6f7264
|
||||
|
||||
// lockRedirectWrites takes the redirect write lock for the rest of tx (it
|
||||
// is released on commit or rollback). Every path that simulates a redirect
|
||||
// write takes it before reading the rules (WR-10): at READ COMMITTED the
|
||||
// next statement after the wait sees what the other writer committed.
|
||||
func lockRedirectWrites(tx *gorm.DB) error {
|
||||
return tx.Exec("SELECT pg_advisory_xact_lock(?)", redirectLockKey).Error
|
||||
}
|
||||
|
||||
// checkRedirectRules is CheckRedirect over the stored rules given, for the
|
||||
// site URL site.
|
||||
func checkRedirectRules(site string, rules []models.Redirect, r models.Redirect) (RedirectCheck, error) {
|
||||
|
||||
@@ -428,6 +428,47 @@ func TestAddressChangeGuards(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestConcurrentRedirectWrites checks that redirect writes are serialised
|
||||
// (WR-10): while one transaction holds A -> B uncommitted, a concurrent
|
||||
// B -> A waits, then sees A -> B and is refused as a loop.
|
||||
func TestConcurrentRedirectWrites(t *testing.T) {
|
||||
env := bootEnv(t)
|
||||
svc := env.seoPlugin(t).Service()
|
||||
newRule := func(source, target string) *models.Redirect {
|
||||
r := models.NewRedirect()
|
||||
r.Source, r.Target = source, target
|
||||
return &r
|
||||
}
|
||||
first := env.db.Begin()
|
||||
if _, err := svc.SaveRedirect(t.Context(), first, newRule("/cw-a", "/cw-b")); err != nil {
|
||||
first.Rollback()
|
||||
t.Fatal(err)
|
||||
}
|
||||
done := make(chan error, 1)
|
||||
go func() {
|
||||
done <- env.db.Transaction(func(tx *gorm.DB) error {
|
||||
_, err := svc.SaveRedirect(context.Background(), tx, newRule("/cw-b", "/cw-a"))
|
||||
return err
|
||||
})
|
||||
}()
|
||||
select {
|
||||
case err := <-done:
|
||||
first.Rollback()
|
||||
t.Fatalf("B -> A finished while A -> B was uncommitted: %v", err)
|
||||
case <-time.After(300 * time.Millisecond):
|
||||
}
|
||||
if err := first.Commit().Error; err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
var loop *classes.LoopError
|
||||
if err := <-done; !errors.As(err, &loop) {
|
||||
t.Fatalf("B -> A after A -> B committed: err = %v, want a loop", err)
|
||||
}
|
||||
if got := redirectTo(env.redirects(t), "/cw-b"); got != "" {
|
||||
t.Fatalf("/cw-b -> %q was stored", got)
|
||||
}
|
||||
}
|
||||
|
||||
// redirectCount is the number of stored redirects.
|
||||
func (e *testEnv) redirectCount(t *testing.T) int64 {
|
||||
t.Helper()
|
||||
|
||||
Reference in New Issue
Block a user