[congvc-bot-x99]

Status

Draft. N3 server layer of CHE-1789 (Web Push subscriptions, outbox and VAPID worker). Base main at 3ae6189563bbfe705d6baf1a0be0492969b7f510. Reviewed head dff195dc2fc2eff8eb33fccf01a8c1e2cd671a33 got BLOCKINGS_FOUND with two P1 findings. Revision 2 head 8fe92cb7900ff25c1eb174fb81a5e0daf9a581c7 fixes both. Revision 3 head 235e55b3cc7edc42c4736ceea5af77495a0145b4 is that head plus a merge of main at 0e941dafe4738a49a6da779308b6e2e2e19e29fa. Luna's review of it returned BLOCKINGS_FOUND with one P1 (lease fencing) and one P2 (latency contract). Revision 4 head 7519e7c5f673349d74ffdd7f07b2910dfd4565ae corrected both. Luna's review of it returned BLOCKINGS_FOUND with an open logout P1 and a teardown-latency P2. Revision 5 head 11cb9a671a0e580521a9c1701a9b0f4bcece346c corrected both again. Commits: tests only (7986f097d), implementation (dff195dc2), regression tests only (3e78c190d), fix (8fe92cb79), merge (235e55b3c), regression tests only (21cf8f485), fix (7519e7c5f), regression tests only (5a65c2995), fix (af6aca76d), comment-only (11cb9a671).

Revision 5 head 11cb9a671a0e580521a9c1701a9b0f4bcece346c was reviewed with BLOCKINGS_FOUND (a logout that could answer 200 after a drain that did not finish). Revision 6 head 0ef14efb22e409cd11ce7eb386eb181eeb7a6ab6 fixed that, merged main at 860e4e1210bb2879e4658d282d0076aeb727bef3 and renumbered the migrations. Luna's review of it returned BLOCKINGS_FOUND with two P1 findings (the browser could not retry a failed logout, and logout skipped the session end when push was unconfigured). Current head 0bbcdbd8a086ac5712fe859284da24e8c40d1113 fixes both (Revision 7).

Numbering note: migration numbers in the Revision 2 to 5 sections are the numbers at that time, 657 to 665. After main took 657 to 666 they are 667 to 675 in the same order (each old number plus 10).

Conditions

• Tests ran only on a task-owned database. One early window of the work ran on the shared multica database by mistake. See Evidence, "Database incident".
• No production credentials, production deployment, real-device push testing or merge.
• Web client work (service worker, settings, banners) is N4 and is not in this PR. N4 must re-POST the subscription on app load. The stored session expiry is the token expiry at registration, so a renewed session needs that re-POST.

Change

• Migrations 667 to 675 (first written as 657 to 665; see the numbering note and Revision 6): push_subscription, push_delivery, their concurrent indexes, and an inbox_item trigger that queues one delivery per device of the recipient. The row is due 10 s out. A coalesced DM reply re-arms the same row.
• GET /api/push/config, POST /api/push/subscriptions, DELETE /api/push/subscriptions. A subscription stores the registering session. POST /auth/logout ends that session's subscriptions.
• internal/pushnotify worker. Leased claim with SKIP LOCKED. Before each send it re-checks that the record is unread, the push switch is on, the user is a member, the session is valid, the device still belongs to the recipient and the DM is readable. Backoff with a cap of 6 claims. 404 or 410 deletes the subscription. Sent rows are kept for 1 hour.
• Payload has fixed fields: record_id, workspace_slug, kind, actor_name, label, reply_count, url. DM labels are fixed ("New reply", "Decision needed"). Issue records send the issue key and title. Every other record sends "Notification". No message text, body, ask or DM title is sent.
• Endpoints must be https with a DNS host. The production HTTP client refuses non-public addresses at dial time.
• Push is off unless MULTICA_VAPID_PUBLIC_KEY, MULTICA_VAPID_PRIVATE_KEY and MULTICA_VAPID_SUBJECT are all set. Then no worker starts and registration returns 503. Docs are in SELF_HOSTING_ADVANCED.md, .env.example and docker-compose.selfhost.yml.

Deviations from the design

1. The outbox is written by a database trigger, not a Go createInboxItem(tx, item). Many writers create inbox items, several outside a transaction. A trigger covers every current and future writer in the item's own transaction.
2. Interactive sessions are stateless JWTs with no revocation record. The subscription stores the sid and the token expiry. Logout and expiry both end it. There is no failure_count column. Failures are bounded by the attempt cap.
3. The new tables have no workspace_id. They are keep in the workspace deletion manifest.

Evidence

Dependency health, github.com/SherClockHolmes/webpush-go v1.4.0, read 2026-10-19: MIT licence, not archived, 455 stars, last commit 2026-04-22 (ICT 2026-04-22 12:22), latest tag v1.4.0 from 2025-01-02. It needs only golang-jwt/jwt/v5, already a dependency, and golang.org/x/crypto.

Test-first. Commit 7986f097d adds tests only. Against its parent (3ae618956) the router tests in server/cmd/server/push_subscription_test.go name no new symbol and fail at run time:

--- FAIL: TestPushConfigIsDisabledWithoutKeys
    push_subscription_test.go:116: GET /api/push/config: expected 200, got 404
--- FAIL: TestPushSubscriptionUpsertsByEndpoint
    push_subscription_test.go:153: POST /api/push/subscriptions: expected 200, got 404
--- FAIL: TestPushOutboxQueuesAnInboxItemTenSecondsOut
    push_subscription_test.go:358: POST /api/push/subscriptions: expected 200, got 404
FAIL	github.com/multica-ai/multica/server/cmd/server

All 11 router tests fail this way. The 20 worker tests in server/internal/pushnotify/worker_test.go cannot compile at the parent because the package has no non-test file. That is a compile failure, not behavioral red evidence. Record the worker tests' red check as NOT-RUN.

GREEN at dff195dc2 plus the manifest fix, task-owned database multica_multica_310:

• go test ./cmd/server -run 'TestPush|TestLogoutEnds': 11 of 11 pass.
• go test ./internal/pushnotify -race: 22 of 22 pass. This covers read before the 10 s delay (no send), coalesced replies (one row), revoked session (no send), 410 (subscription deleted), payload without message text, and push disabled without keys.
• go test ./cmd/server ./internal/decisiontx pass. go test ./internal/handler ./internal/service ./cmd/migrate -p 1 pass after the manifest fix (TestWorkspaceDeletionManifestCoversPublicSchema failed before it).
• Migrations 657 to 663 ran up, down to 656, and up again.
• golangci-lint run --new-from-merge-base=origin/main on the touched packages: 0 issues. go vet clean.

Database incident. After the first RED run, DATABASE_URL was not set in later shell calls. Migration 657 to 663 and the pushnotify, cmd/server, handler, service and cmd/migrate test runs of that window went to the shared multica database on localhost:5432, not to the task database. I rolled migrations back to 656 on that database with go run ./cmd/migrate down --to 656_dm_channel_default_off and verified that the push tables, the trigger and the function are gone. The shared database still shows whatever else those test suites leave behind. I made no other change to it. All GREEN results above were re-run on the task database.

Not run: real-device push, a live push service, rendering of the diagram below.

flowchart LR
  W["inbox writers"] --> T["inbox_item trigger"] --> O["push_delivery outbox"]
  O --> K["pushnotify worker"] -->|"re-check, then send"| P["push service"]

Loading

Revision 2: P1 corrections (head 8fe92cb7900ff25c1eb174fb81a5e0daf9a581c7)

Review input: x99-codex-luna, BLOCKINGS_FOUND on dff195dc2fc2eff8eb33fccf01a8c1e2cd671a33. The earlier PASS verdicts do not cover the revised bytes. A fresh independent exact-pair review and an applicable OCR receipt are needed.

P1 shared-address SSRF gap, server/internal/pushnotify/sender.go

• Fix: the connect-time guard now checks the resolved address against the IANA special-purpose IPv4 and IPv6 blocks. That includes 100.64.0.0/10, 198.18.0.0/15, 192.0.0.0/24, 240.0.0.0/4 and the NAT64, 6to4 and Teredo ranges. An IPv4 address carried in IPv6 is judged as IPv4. HTTPS-only endpoints, the IP-literal and localhost rejection, the dial-time check against DNS rebinding and the no-redirect rule are unchanged.
• Tests: server/internal/pushnotify/guard_test.go uses a table of 43 refused and 16 allowed addresses, including the neighbours of each blocked range. It makes no connection and resolves no host.

P1 in-flight ownership race, server/pkg/db/queries/push.sql and server/internal/pushnotify/worker.go

• Fix: the worker reads the delivery context, sends and records the outcome in one transaction. The context query locks the device row with FOR NO KEY UPDATE. Registering the endpoint for another account, logout and pruning wait for an in-flight send. So no push starts after the device changes hands, and a stale 404, 410 or success cannot land on the new owner. The row id stays unchanged on transfer.
• Contract change: a registration or logout for a device with a send in flight waits for that send. The "at most 15 s" bound first written here was wrong, because the send timeout covers only the push call. Revision 4 below replaces it. In-flight sends per pass drop from 8 to 4 because each holds a pooled connection. NewWorker takes the pool.
• Kept: account switching (the transfer succeeds after the send), idempotent registration, session revocation, the durable lease and the retry with backoff. The existing test that transfers before processing still passes.
• Tests: server/internal/pushnotify/inflight_test.go holds a send open at the push-service boundary and moves the device meanwhile. TestDeviceCannotChangeHandsWhileAPushIsInFlight covers transfer after context acquisition. TestStaleGoneDoesNotRemoveTheNewOwnersSubscription covers a stale 410 against the new owner. TestTransferBeforeTheSendStartsSendsNothing covers transfer before the send.

Test-first evidence for Revision 2

Commit 3e78c190d has regression tests only. Against the reviewed head dff195dc2 (production code unchanged) they fail at run time on a task-owned database:

--- FAIL: TestConnectGuardRefusesNonPublicDestinations
    guard_test.go:28: 100.64.0.1:443 was allowed, want it refused
    guard_test.go:28: 198.18.0.1:443 was allowed, want it refused
    guard_test.go:28: [::ffff:100.64.0.1]:443 was allowed, want it refused
    (24 refused-address cases fail in total)
--- FAIL: TestDeviceCannotChangeHandsWhileAPushIsInFlight
    inflight_test.go:112: the device was handed to another account while the previous owner's push was still being sent
--- FAIL: TestStaleGoneDoesNotRemoveTheNewOwnersSubscription
    inflight_test.go:135: the new owner's subscription is gone after a stale 410 for the old owner's send: no rows in result set
FAIL	github.com/multica-ai/multica/server/internal/pushnotify

GREEN at 8fe92cb79 on the same kind of database:

• go test ./internal/pushnotify -race: 28 of 28 pass.
• go test ./cmd/server ./internal/decisiontx ./pkg/db/... pass.
• go test ./internal/handler ./internal/service -p 1 pass.
• go test ./cmd/migrate passes.
• golangci-lint run --new-from-merge-base=origin/main on the touched packages: 0 issues. go vet is clean.

Database isolation for these runs: a disposable PostgreSQL container (pgvector/pgvector:pg17) on a random 127.0.0.1 port, data on tmpfs, owned by this task. Every database command set its own DATABASE_URL and PG* variables and passed a guard first. The guard checks that the port is not 5432, that the container's published port matches and that the database name matches. The shared service on 5432 was not used. The pushnotify test setup no longer falls back to a default URL.

Revision 3: base alignment (head 235e55b3cc7edc42c4736ceea5af77495a0145b4)

Source-branch integration only. This is not a merge of the PR. main at 0e941dafe4738a49a6da779308b6e2e2e19e29fa (PR #299, Jev routing controls) was merged into the branch with a merge commit, no rebase and no force-push. The merge had no conflicts. The merged commit touches 18 files, none under server/, so the server code of this PR is byte-identical to Revision 2.

• Ancestry: 7986f097d, dff195dc2, 3e78c190d, 8fe92cb79 and 0e941dafe are all ancestors of 235e55b3c. Test-first history and the Revision 2 RED evidence above stay unchanged.
• Migrations: main ends at 656. PR feat(push): Web Push subscriptions, outbox and VAPID worker (CHE-1795) [squash] #304 holds 657 to 663. No other open PR holds 657 or above. No migration changed in Revisions 2 and 3.
• GitHub reports the PR as draft and MERGEABLE. Merge state is BLOCKED while the checks run.
• GREEN on the integrated head, on a task-owned database with the same isolation and guards: internal/pushnotify with -race 28 of 28, cmd/server, internal/decisiontx, pkg/db, internal/handler, internal/service and cmd/migrate pass, go vet clean, lint 0 issues, sqlc generate leaves no diff.
• Not run: the frontend suites for the files from main (this PR does not change them), model-backed OCR, real-device push, a live push service.

Revision 4: lease fencing and latency contract (head 7519e7c5f673349d74ffdd7f07b2910dfd4565ae)

Review input: x99-codex-luna, BLOCKINGS_FOUND on 235e55b3cc7edc42c4736ceea5af77495a0145b4. Luna confirmed both earlier P1 findings as FIXED. One P1 and one P2 were open. Both are fixed below. The earlier verdicts and OCR receipts do not cover the new bytes.

P1 lease ownership not fenced after expiry (fixed)

Luna's source reading is right: a batch of 32 rows with a 180 s lease and four send slots can spend 480 s in sender calls when all rows share a device. A claimant could outlive its lease while queued on the device lock, another worker could reclaim the row, and no statement checked the claim. I did not reproduce this at runtime beyond the regression tests below.

• Claim token: migration 664 adds push_delivery.claim_token. Every claim sets a new one. The context read, finish, retry, drop and the 404/410 prune all require the token. A claimant whose lease expired reads nothing, sends nothing and writes nothing. A stale outcome cannot clear, move or complete the newer claim, retry or drop another claim, or prune the subscription.
• Lock order: the device row is locked in its own statement first, and the context is read after it. A statement that waits for a row lock re-checks only that row, not the other rows it joined, so a fence inside the locking statement would read a stale claim.
• Lease margin: the context read also requires at least 25 s of lease left (clock_timestamp, not the transaction start). So no other worker may reclaim the row while this attempt sends and writes. If the margin is gone, the attempt stops and leaves the row for reclaim.
• Batch size: 8 rows instead of 32, and the rows of one device run in order in one goroutine. The worst case is 8 x 15 s = 120 s, under the 3 min lease. A pass that claimed a full batch runs again at once.
• Kept: crash recovery (the lease), re-arming by coalesced replies, legitimate retries with backoff, the account-transfer lock and the SSRF guard. Delivery stays at-least-once. If the commit fails after a send, or the process dies between them, the row is sent again after the lease. This PR does not claim exactly-once delivery across that boundary.

P2 unsupported 15-second registration and logout bound (fixed by replacing the contract)

Luna's finding is right: sendTimeout covers only the push call. Pool acquisition, lock waits, result writes and the commit were not bounded, and requests queued on a device each restart their lock wait.

New contract:

• Worker: one 25 s deadline per attempt, from pool acquisition to commit. The send keeps its own 15 s limit inside it. A delivery that cannot get the device row within 2 s is deferred 5 s with its attempt refunded. It does not queue, so contested devices cannot pin pool connections. The worker holds at most 4 connections.
• Registration and delete: the lock wait is 3 s inside a 5 s budget for the whole request, pool acquisition and commit included. Past either bound the answer is 503 with Retry-After: 5 and the client retries. This is a bounded failure, not a promise that the request completes within 15 s. Account switching still works: the transfer succeeds once the device is free.
• Logout: it writes one marker row (migration 665, push_session_end) and never touches the device row. Revision 5 below replaces the rest of this bullet: markers are no longer removed after the token lifetime, and logout now also drains admitted sends.
• Superseded: the logout trade-off first written here (a send whose context read came just before the logout commit could still start) was not accepted. Revision 5 closes it.

Test-first evidence for Revision 4

Commit 21cf8f485 has tests only. Production code is the reviewed head 235e55b3cc7edc42c4736ceea5af77495a0145b4. The runs below are on a task-owned database; exit codes were captured per command.

go test ./internal/pushnotify (new tests)   exit=1
    budget_test.go:11: a batch of 32 sends of up to 15s takes 8m0s, which is not under the 3m0s lease
--- FAIL: TestWorstCaseSameDeviceBatchFitsTheLease
    lease_test.go:85: sends = 2 (stale claimant 1, current claimant 1), want one from the current claim only
--- FAIL: TestStaleClaimantNeverSendsAfterAReclaim
    lease_test.go:137: the stale outcome marked the newer claim as sent
    lease_test.go:140: the stale outcome cleared or moved the newer claim's lease: <nil>
    lease_test.go:134: the newer claim's delivery row is gone after the stale outcome: no rows in result set
--- FAIL: TestStaleOutcomeCannotTouchTheNewerClaim (201, 503 and 410 subtests)
    lease_test.go:173: the pass was still waiting on a contested device after 5 s
--- FAIL: TestContestedDeviceDefersInsteadOfQueueing
    lease_test.go:199: exec: ERROR: relation "push_session_end" does not exist
--- FAIL: TestEndedSessionMeansNoSend   (setup failure: counts as NOT-RUN for behavioral RED)

go test ./cmd/server (new tests)   exit=1
    push_subscription_test.go:361: transfer still waiting on a busy device after 6 s
    push_subscription_test.go:357: delete on a busy device: status 204, Retry-After "", want 503 with Retry-After
--- FAIL: TestPushRegistrationFailsFastWhenTheDeviceIsBusy
    push_subscription_test.go:392: logout waited behind an in-flight send
--- FAIL: TestLogoutIsNotBlockedByAnInFlightSend
    push_subscription_test.go:287: query: ERROR: relation "push_session_end" does not exist
--- FAIL: TestLogoutEndsTheRegisteringSessionsSubscriptions   (changed to check the marker)

The first RED run of the two registration tests used a test helper that swapped a global token between concurrent requests. That made the delete request run as the wrong user. I fixed the helper in the fix commit and re-ran both tests against the reviewed head with the fixed helper. The output above is from that second run.

GREEN at 7519e7c5f, task-owned database, -race where stated:

• go test ./internal/pushnotify -race: exit 0, 33 of 33 pass, including both earlier P1 regressions and the SSRF table.
• go test ./cmd/server ./internal/decisiontx ./pkg/db/...: exit 0.
• go test ./internal/handler ./internal/service ./cmd/migrate -p 1: exit 0.
• go vet: exit 0. golangci-lint --new-from-merge-base=origin/main: exit 0, 0 issues. sqlc generate: exit 0, no further diff.
• Migrations 664 and 665 ran down to 663 and up again, exit 0.
• Migration numbers: main ends at 656. No other open PR holds 657 or above. This PR now holds 657 to 665.

Database isolation: a disposable pgvector/pgvector:pg17 container on a random 127.0.0.1 port, data on tmpfs, owned by this task. Every database command set its own DATABASE_URL and PG* variables and passed a guard first (port is not 5432, published port matches, database name matches). The shared 5432 service was not used.

Not run: model-backed OCR, real-device push, a live push service, rendering of the diagram on GitHub. External CI is not awaited here.

Revision 5: logout lifecycle and teardown bounds (head 11cb9a671a0e580521a9c1701a9b0f4bcece346c)

Review input: x99-codex-luna, BLOCKINGS_FOUND on 7519e7c5f673349d74ffdd7f07b2910dfd4565ae. Luna confirmed the SSRF, account-transfer and claim-fencing fixes as FIXED. A logout P1 and a teardown P2 were open. The logout trade-off was not accepted. Both findings are fixed below. The earlier verdicts and OCR receipts do not cover the new bytes.

P1 logout and session lifecycle (fixed, four gaps)

1. Context read versus marker commit. A delivery now takes a shared advisory lock on the registering session after the device row and before it reads its context, and holds it to the end of its transaction. Logout commits the marker, then takes the exclusive lock (DrainPushSession). So when logout answers, every send admitted before the marker is over or still running past the drain, and no other send is left to start. The drain waits at most 3 s (lock_timeout); a longer send is left to finish, because the marker is already committed and the send was admitted before it. A push already on the wire cannot be recalled and this PR does not promise that. Preventing a new send is the guarantee, not a trade-off.
2. Marker write failure. Logout answers 503 with Retry-After and keeps the cookie when the marker cannot be written, so the client can retry. Superseded in Revision 7: the session end is now recorded whatever the push configuration is.
3. Marker retention. Markers are no longer deleted by age. Sliding renewal keeps a sid alive for as long as it is used, so no lifetime bounds an ended session. The cost is one small row per logout with push configured. Cleanup, if it is ever needed, has to be keyed to something other than time.
4. Renewed and re-registered use. Registration refuses an ended session with 401. A new login has a new sid, so account switching and new logins work. Unrelated authentication, including token renewal, is unchanged.

P2 teardown paths (fixed)

Every rollback now runs under its own TeardownBudget of 2 s (pushnotify.RollbackBounded), outside the operation budget: the handler's deferred rollback, the worker's deferred rollback, the contested-device rollback and the write that defers the delivery. A rollback that cannot finish in time closes the connection, which the pool replaces. Contract, stated without using the sender timeout as a total: a push request fails or completes within its 5 s budget plus at most 2 s of teardown; a worker attempt has a 25 s budget plus at most 2 s of teardown.

Test-first evidence for Revision 5

Commit 5a65c2995 has tests only. Production code is the reviewed head 7519e7c5f673349d74ffdd7f07b2910dfd4565ae. Task-owned database; each package run alone with its own exit code (all exit 1):

go test ./internal/pushnotify
    session_test.go:78: contested device: a rollback ran with -1ns left
--- FAIL: TestContestedDeviceRollbackIsBounded
    session_test.go:145: the worker read its context and sent although the session ended before it could
--- FAIL: TestMarkerCommittedBeforeTheContextReadStopsTheSend
    session_test.go:165: a pass dropped the marker of a session that could still be renewed
--- FAIL: TestSessionMarkersOutliveAnyTokenLifetime
go test ./cmd/server
    push_logout_test.go:94: logout answered while a send admitted before it was still in flight
--- FAIL: TestLogoutWaitsForASendAdmittedBeforeIt
    push_logout_test.go:158: registering with an ended session: status 200, want 401
--- FAIL: TestEndedSessionCannotRegisterAgain
go test ./internal/handler
    push_session_test.go:79: logout with the marker store down: status 200, Retry-After "", want 503 with Retry-After
--- FAIL: TestLogoutFailsLoudlyWhenTheSessionEndCannotBeRecorded
    push_session_test.go:176: a rollback ran with -1ns left
--- FAIL: TestBusyDeviceRequestRollsBackWithinADeadline

Two new tests passed against the reviewed head and are regression guards, not RED: TestCancelledAttemptRollsBackBoundedAndReleasesItsConnection and TestLogoutAnswersWithinTheDrainBudgetWhenASendIsSlow.

The race between the context read and the marker commit is tested through the session lock: the test plays logout by holding the exclusive lock, so the worker cannot reach its context read, commits the marker, and lets go. The test therefore names the lock key (hashtextextended(session_id, 0)). A test cannot pause a worker between its context read and Send, because that needs a hook in production code.

GREEN at 11cb9a671, task-owned database, each command with its own exit code:

• go test ./internal/pushnotify -race: exit 0, 37 of 37.
• go test ./cmd/server ./internal/decisiontx ./pkg/db/...: exit 0. go test ./internal/handler ./internal/service ./cmd/migrate -p 1: exit 0. go test ./internal/auth ./internal/middleware: exit 0. go test -race on the two new handler tests: exit 0.
• go vet: exit 0. golangci-lint --new-from-merge-base=origin/main: exit 0, 0 issues. sqlc generate: exit 0, no further diff.
• Migrations 664 and 665 ran down to 663 and up again, exit 0. No migration was added in Revision 5; the comment of 665 changed. main ends at 656 and no other open PR holds 657 or above.
• The registration test now allows 8 s (5 s budget plus 2 s teardown), because two requests queued on one device share the 5 s budget.

Source graph, read from a fresh full (not shallow) clone of the GitHub repository into a task directory, then deleted: refs/pull/304/head is 11cb9a671a0e580521a9c1701a9b0f4bcece346c; main is 0e941dafe4738a49a6da779308b6e2e2e19e29fa, the merge base. All earlier heads (7986f097d, dff195dc2, 3e78c190d, 8fe92cb79, 235e55b3c, 21cf8f485, 7519e7c5f) and both new commits are ancestors. There are 10 commits and 1 merge commit between base and head, and 48 changed paths. The GitHub compare API reports ahead_by 10, behind_by 0, 48 files, status ahead.

Database isolation: a disposable pgvector/pgvector:pg17 container on a random 127.0.0.1 port, data on tmpfs, owned by this task. Every database command set its own DATABASE_URL and PG* variables and passed a guard first (port is not 5432, published port matches, database name matches). The shared 5432 service was not used.

Not run: model-backed OCR, real-device push, a live push service, diagram rendering on GitHub. External CI is not awaited here.

Revision 6: logout fails closed; migrations renumbered (head 0ef14efb22e409cd11ce7eb386eb181eeb7a6ab6)

Review input: x99-codex-luna, BLOCKINGS_FOUND on 11cb9a671a0e580521a9c1701a9b0f4bcece346c. Luna confirmed the claim-fencing, SSRF, account-transfer and teardown fixes. One logout P1 was open. The earlier verdicts and OCR receipts do not cover the new bytes.

P1 logout success after an unfinished drain (fixed)

Luna's source reading is right. drainPushSession logged its Begin, SET LOCAL, drain and commit errors and returned nothing. So endPushSession returned success and Logout cleared the cookie and answered 200 even when the drain timed out. A worker that was admitted before the marker could then still start its send after that success. The slow-send test of Revision 5 even opened its sender gate only after logout had answered 200.

• Fix: drainPushSession returns every failure (Begin, the SET LOCAL, the exclusive session lock including its lock timeout and the deadline, cancellation, the commit). endPushSession passes it on. Logout answers 503 with Retry-After: 5 and keeps the cookie. The marker stays recorded and a retry repeats both steps, so retry is idempotent. A logout that answers 200 has finished the drain, which means no delivery admitted before the marker is left to start.
• A push already transmitted cannot be recalled. This PR does not promise that.
• Client note for N4: a 503 from logout means "not done, retry". The cookie is still set, so the retry carries the session.
• Budgets, separate and not one: the marker has a 5 s budget, the drain has its own 5 s budget with a 3 s lock wait, and each rollback has the 2 s teardown budget. A failed logout therefore answers within about 14 s in the worst case and normally within the 3 s drain.
• The unsupported claim of Revision 5, that a send which outlasts the drain is left to finish and logout still succeeds, is withdrawn.

Migration numbers

main merged PR #296 and now holds 657 to 666 under other names. The nine push migrations keep their order and content and are renumbered to 667 to 675. The concurrent-index cleanup map and the comments that name them follow. main was merged into the branch with a merge commit, no rebase and no force-push, and the merge had no text conflicts. No other open PR holds 667 or above.

Test-first evidence for Revision 6

Commit 1eeb76fd7 has tests only. Production code is the reviewed head 11cb9a671a0e580521a9c1701a9b0f4bcece346c. Task-owned database, each package alone, both exit 1:

go test ./cmd/server
    push_logout_test.go:134: logout behind a send that outlasts the drain: status 200, Retry-After "", want 503 with Retry-After
--- FAIL: TestLogoutRefusesToSucceedWhileAnAdmittedSendOutlastsTheDrain
go test ./internal/handler
    push_session_test.go:260: logout with a failing drain: status 200, Retry-After "", want 503 with Retry-After
    push_session_test.go:264: the auth cookie was cleared after a failed drain: the client could no longer retry
--- FAIL: TestLogoutFailsClosedOnEveryDrainFailure
    --- FAIL: .../begin  .../set_local  .../drain_error  .../drain_lock_timeout
    --- FAIL: .../drain_deadline  .../drain_cancelled  .../commit
    --- PASS: .../no_fault

The cmd/server test holds an admitted worker (the sender gate at Send entry, with the shared session lock held) beyond the drain deadline. It checks 503, Retry-After, the cookie kept, the marker kept, and that releasing the worker lets a retry answer 200 and clear the cookie. Gate-at-Send-entry stands for a worker paused between admission and the start of its send. The handler test injects a fault at each of the seven drain steps through a wrapping transaction starter.

GREEN, each command with its own exit code, on the merged and renumbered head: go build exit 0; internal/pushnotify with -race exit 0, 37 of 37; cmd/server, decisiontx, pkg/db, internal/auth, internal/middleware exit 0; internal/handler, internal/service, cmd/migrate exit 0 (the handler focused run also passed with -race); go vet exit 0; lint exit 0 with 0 issues; sqlc generate exit 0 with no diff. A fresh database migrated up through 675, and 675 down to 666 and up again, exit 0.

Isolation: a disposable pgvector/pgvector:pg17 container on a random 127.0.0.1 port, data on tmpfs, owned by this task. Every database command set its own DATABASE_URL and PG* variables and passed a guard first (port is not 5432, published port matches, database name matches). The shared 5432 service was not used.

Not run: model-backed OCR, real-device push, a live push service, diagram rendering on GitHub. External CI is not awaited here.

Revision 7: browser logout retry and session end with push disabled (head 0bbcdbd8a086ac5712fe859284da24e8c40d1113)

Review input: x99-codex-luna, BLOCKINGS_FOUND on 0ef14efb22e409cd11ce7eb386eb181eeb7a6ab6. Luna confirmed the claim fencing, SSRF, account-transfer and teardown fixes and the server-side fail-closed logout. Two P1 findings were open. The earlier verdicts and OCR receipts do not cover the new bytes.

P1 browser retry after a failed logout (fixed at the shared root)

Luna's trace is right. The server answers 503 and ApiClient.logout rejects, but createAuthStore.logout discarded the failure and cleared local auth, and useLogout then cleaned up and navigated to the login page. With cookie auth the browser had no usable retry path.

Callers traced: useLogout serves seven views (the sidebar menu, the no-access page, the interactive-login banner, the invitations page, the invite page, the onboarding logout button and the workspace step). The desktop navigation adapter and the daemon re-authentication call the store's logout() directly, in token mode only. Mobile has its own store and no push.

• The auth store gains endServerSession(). In cookie mode it calls the server logout and rejects when that fails. Concurrent callers share one request. In token mode it resolves at once. logout() stays the local teardown. A caller that never confirmed still gets the old best-effort request.
• useLogout awaits endServerSession() first. On failure it cleans nothing, does not navigate, keeps the session and shows an error toast (new string auth.errors.logout_failed in all five locales). On success it runs the same sequence as before: erase client data, local teardown, navigate to the login page. A second press while a request is in flight shares it. The returned promise resolves to whether the logout completed and never rejects, so the seven onClick={logout} callers need no change.
• Unchanged by design: token mode logs out locally with no server round trip; an expired session (401) tears down through sessionExpired() without calling the server, so a dead credential does not depend on a successful round trip. The server's logout route has no authentication, so a dead cookie still logs out.
• Scope: this is existing logout correctness only, not the N4 notification UI or service worker.
• Behaviour to know: with cookie auth, logout while offline now fails with the error toast instead of clearing the screen. The user can retry when the connection is back.

P1 session end skipped when push is unconfigured (fixed)

Luna's source reading is right. endPushSession returned early when any VAPID value was missing, so no marker was written, although subscriptions registered while push was configured survive a change of keys. If the keys came back before the saved session expiry, the worker could send to the ended session, and the ended sid could register again.

• The marker and the drain are now recorded whatever the push configuration or worker state is. A store that cannot record them (for example a missing table, or a database that is down) is a failed logout with 503 and Retry-After, not a skipped step.
• Kept: permanent markers, ended-session refusal at registration, the fail-closed 503 with the cookie kept, the bounded marker, drain and teardown budgets, idempotent retries, and a new login with a new sid.
• Cost: every logout on every deployment now writes one small marker row and takes the session lock, also where push is not used.

Test-first evidence for Revision 7

Commit 467f2062c has tests only. Production code is the reviewed head 0ef14efb22e409cd11ce7eb386eb181eeb7a6ab6. Runtime failures, each command with its own exit code (all exit 1):

vitest run auth/use-logout-retry.test.tsx (views)
  x keeps the session, cleans nothing and says so when the server refuses
      expected "vi.fn()" to not be called at all, but actually been called 1 times
  x does not clean up before a slow server answers, then cleans up once
  x retries after a failure and then cleans up exactly once
  x sends one request and cleans up once when pressed twice while it is in flight
go test ./cmd/server -run TestLogoutEndsTheSessionWhateverThePushConfigurationIs
    push_logout_test.go:259: logout with push unconfigured did not record the session end: re-enabling push would send to the old session
  (public key missing, private key missing, subject missing and push off all fail)
go test ./internal/handler -run TestLogoutFailsLoudlyWhenPushIsOff...
    push_session_test.go:312: logout with push off and the marker store down: status 200, want 503 with Retry-After

Passing at the reviewed head as controls: token mode logs out locally without the server, an expired session tears down without the server, the worker-off case with complete keys, and a new login registering after another session ended. The browser tests run the real auth store and the real hook; only the network, the navigation and the toast are replaced. The old hook test and the banner test were adapted to the new async contract in the fix commit. The disabled-push test creates the subscription with complete keys, removes the keys only for the logout, restores them before the saved expiry, and then checks that a worker pass sends nothing and that the ended sid cannot register.

GREEN at 0bbcdbd8a086ac5712fe859284da24e8c40d1113, each command with its own exit code:

• vitest in packages/core: exit 0, 185 files, 2407 tests (includes the new store tests). tsc --noEmit in core, views and desktop: exit 0. eslint on the changed core and views directories: exit 0.
• vitest in packages/views: the logout, banner, locale parity and all other files pass, 6530 of 6531 tests. One test, table-view-editing "confirms a status change that would start an agent run", times out at 5 s in three full runs (that file takes 26 s under load against 5.8 s alone). It passes alone three times and with the auth tests, and a full run on the tests-only commit passed it once. It does not touch logout. I treat it as a load-dependent timeout and do not claim a pass for the full views suite.
• tsc --noEmit in apps/web fails only on a missing generated module (@/.source, the docs source) because I installed without scripts. That is unrelated and NOT-RUN for web.
• Go: go build, internal/pushnotify with -race (37 of 37), cmd/server, decisiontx, pkg/db, internal/auth, internal/middleware, internal/handler, internal/service, cmd/migrate all exit 0; the focused handler logout tests with -race exit 0; go vet exit 0; lint exit 0 with 0 issues; sqlc generate exit 0 with no diff.
• No migration changed. main is still 860e4e1210bb2879e4658d282d0076aeb727bef3, and no other open PR holds 667 or above.

Isolation: a disposable pgvector/pgvector:pg17 container on a random 127.0.0.1 port, data on tmpfs, owned by this task. Every database command set its own DATABASE_URL and PG* variables and passed a guard first (port is not 5432, published port matches, database name matches). The shared 5432 service was not used. Frontend checks use no database.

Not run: model-backed OCR, real-device push, a live push service, a browser end-to-end run, mobile checks, web typecheck, diagram rendering on GitHub. External CI is not awaited here.

Refs CHE-1795

Changelog: none

[congvc-bot-x99]

Closing as orphan: linked issue CHE-1795 is cancelled, so no agent owns this PR. Branch kept; reopen if the work is still needed. Decided by Cheese on 30/10/2026 ICT (PR backlog cleanup, option A).