Skip to content

fix: cache the project registration created, not a partial copy of it - #363

Open
FrameAutomata wants to merge 2 commits into
mainfrom
claude/repo-issues-pjnio0
Open

fix: cache the project registration created, not a partial copy of it#363
FrameAutomata wants to merge 2 commits into
mainfrom
claude/repo-issues-pjnio0

Conversation

@FrameAutomata

Copy link
Copy Markdown
Collaborator

Found while verifying #317. Unrelated to the multi-replica staleness that issue is about — this one only bites the single-instance builds.

The bug

Register and the SSO FinishSetup handler both cached a hand-built five-field copy of the project they had just created, instead of the project itself:

cache.ProjectCache.AddProject(&models.Project{
    Id: project.Id, Name: project.Name, Token: project.Token,
    Framework: project.Framework, OrganizationId: project.OrganizationId,
})

Every field the copy omits then disagrees with the row, and UseClientAuth hands that cached object straight to the ingest path:

  • DropHealthyHealthchecks is true on a new project but false in the copy, so FilterHealthchecksShouldDropHealthcheck returned false and healthy healthchecks were ingested rather than dropped for every newly registered project.
  • SourceMapTokenCreateWithOrganization mints one for ios projects; the copy left it nil, so GetBySourceMapToken missed and the first symbol upload 401'd.
  • CreatedAt stayed zero, which reorders GetAll().

On the PostgreSQL build the project_cache_changed notification triggers a full Refresh() that overwrites the bad entry within milliseconds, so this only persisted on the SQLite and DuckDB builds, where NotifyProjectCacheChanged is a no-op and nothing refreshes the cache after boot. It lasted until the process restarted or someone saved the project's settings (UpdateProject copies the field and heals it by accident).

Both call sites also ran under middleware.Transactional and mutated the cache before the commit. Register can still AbortWithError afterwards at FindByUserIdWithRoles, and FinishSetup at GenerateToken — either rolls the transaction back and leaves a project in the cache that does not exist in the database.

The fix

Pass the repository's own project and queue the write with middleware.OnCommit, matching what project_batch.go already does:

middleware.OnCommit(c, func() { cache.ProjectCache.AddProject(project) })

ToProjectWithBackendUrl takes a value receiver and copies, so passing the pointer aliases nothing the handler goes on to mutate.

Tests

project_cache_registration_test.go asserts the cached project matches the row field for field, and that the handler performs no cache write of its own. Reverting the fix fails it on all five symptoms:

cached DropHealthyHealthchecks = false, stored = true; healthy healthchecks would be ingested
cached SourceMapToken = <nil>, stored = 527fb8e3dcab4af596a854b5da43e631
source map token does not resolve from the cache; symbol upload would 401
cached AiFlaggedLanguages = [], stored = [en]
cached CreatedAt is zero, which reorders GetAll()
the handler cached the project inline; a rolled back registration would leave a phantom entry

The second commit adds commit_hooks_test.go, because OnCommit/runCommitHooks had no coverage at all — the property this fix depends on was unverified, and moving runCommitHooks onto the rollback branch left the whole suite green. It now fails with hook ran 1 times, want 0. That commit also widens the OnCommit doc comment, since three of its five callers are now cache writes and "visible to other connections" read as excluding an in-process map.

Verification

gofmt, go vet ./... and go test -race -count=1 ./... are clean, and all three supported tag combinations build (default, transactional_pg telemetry_ch, telemetry_duckdb).

Not included

Two pre-existing problems in Transactional that the review surfaced, neither introduced nor worsened here, both worth their own issue: a failed Commit() still delivers the handler's already-written 2xx (so a client can store a valid JWT for a user that was never persisted), and runCommitHooks has no per-hook recover, so one panicking hook silently skips the rest.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S7f9Ad34w2iGg3mqangVsw


Generated by Claude Code

Register and the SSO finish-setup handler both cached a five-field copy of
the project they had just created instead of the project itself, so the
cached row disagreed with the database on every field the copy omitted:

- DropHealthyHealthchecks is true on a new project but false in the copy,
  and UseClientAuth hands that cached object straight to FilterHealthchecks,
  so healthy healthchecks were ingested rather than dropped.
- CreateWithOrganization mints a source map token for ios projects; the copy
  left it nil, so GetBySourceMapToken missed and the first symbol upload 401d.
- CreatedAt stayed zero, which reorders GetAll().

On the PostgreSQL build the project_cache_changed notification triggers a
full refresh that overwrites the bad entry within milliseconds, so this only
persisted on the SQLite and DuckDB builds, where NotifyProjectCacheChanged is
a no-op and nothing refreshes the cache after boot. It lasted until the
process restarted or someone saved the project's settings.

Both call sites also ran under middleware.Transactional and mutated the cache
before the commit, so a later failure in the same handler rolled the
transaction back and left a project in the cache that does not exist. Queue
the write with middleware.OnCommit instead, matching what project_batch.go
already does.

ToProjectWithBackendUrl takes a value receiver and copies, so passing the
repository's own pointer aliases nothing the handler goes on to mutate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S7f9Ad34w2iGg3mqangVsw
Follow-up to the previous commit, from a review pass over it.

middleware.OnCommit and runCommitHooks had no tests at all, so the property
the cache fix depends on — a queued side effect fires only once its write is
durable — was unverified. Moving runCommitHooks onto the rollback branch left
the whole suite green. Cover both outcomes and the ordering.

Also from the review:

- Assert the cached project against every field of the row rather than the
  four a past bug happened to drop. The old length-only check on
  AiFlaggedLanguages passed for a same-length but different pack, which
  changes which flagged-term packs ingest scans.
- Register the ProjectCache cleanup before issuing the request. The commit
  hook inserts the entry during ServeHTTP, so a t.Fatal between the request
  and the old cleanup registration leaked an entry pointing at a project
  whose in-memory database was about to close.
- Move initRegistrationJWT into setup_test.go, which owns the shared harness,
  now that TestRegisterWithoutProject calls it. Its inline copy is gone.
- Widen the OnCommit doc comment: three of its five callers are now cache
  writes, and "visible to other connections" read as excluding an in-process
  map. Note that only routes carrying the middleware run queued hooks.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S7f9Ad34w2iGg3mqangVsw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants