mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 23:15:40 +00:00
xarmian
ec7fd027fc
* feat(store): race-free status/assignment mutation signal (TASK-2533)
Adds models.Item.LastMutation (ItemMutationSignal), populated inside the
SAME transaction that already writes status_transitions / assigned_user_id
in UpdateItemWithParentLink and MoveItemWithPreCheck. This is the
foundation for TASK-2533's watch-notification pipeline: a before/after
snapshot taken in the HTTP handler layer would race concurrent writers of
the same item, so the signal is computed where the authoritative diff
already happens, in-transaction.
* feat(store): watches table migration, both drivers (TASK-2533)
watches(id, workspace_id, user_id, item_id, predicate, created_at) per
DOC-2479's subscription-table design: durable, server-side subscriptions
that survive both the plugin-monitor process and a padd restart.
uq_watches_user_item makes `pad watch <ref>` idempotent (re-watching
upserts the predicate). Wires watches into the workspace-purge child-delete
list, mirroring item_stars.
* feat(watchevents): add in-process notification bus (TASK-2533)
New package: a global (not per-workspace) in-process pub/sub bus carrying
watch-worthy Notifications (status-change / assignment / comment; ask
reserved in the enum with no producer yet — see the follow-up server
commit). Bus is an interface specifically so a Redis-backed implementation
can slot in later without touching any caller; only MemoryBus exists today.
Package doc comment states the single-process/multi-instance limitation
explicitly, mirroring internal/events' shape.
* feat(store): watches CRUD (TASK-2533)
models.Watch + Store.CreateWatch (upsert on user+item)/GetWatchByUserItem/
ListWatchesForUser (unscoped by workspace — a watch is personal, and the
event-stream handler needs every watch a caller holds across all their
workspaces)/DeleteWatch.
* feat(server): watch/nudge event stream + CRUD endpoints (TASK-2533)
GET /api/v1/events/stream (DOC-2479): a user-scoped, cross-workspace SSE
stream, filtered server-side to the caller's watches (with optional
--until field=value predicate) plus "addressed to you" — narrowed to
assignment-to-you only for Phase 1, confirmed with the dispatcher: this
codebase has neither a Collection.Kind field nor any user->active-role
binding to ground DOC-2479's "human-gate-shaped collection targets your
active role" half mechanically. watchevents.KindAsk stays in the wire
enum with no producer. `pad session register` is the natural future hook
for a session-carried role identity.
POST/DELETE .../items/{slug}/watch, GET /api/v1/watches (unscoped,
mirrors /auth/tokens' shape for a personal, not workspace, resource).
Producer wiring (TASK-2533 audit) publishes from every live mutation path
that can produce a LastMutation signal or a new comment: handleUpdateItem
(incl. its collab sub-paths and the comment-attached-to-update path, which
bypasses handleCreateComment entirely), handleMoveItem, handleCreateComment,
item creation with an initial assignee, and the bulk-items loop (covers
archive/restore/move/set-priority/tag/untag/assign uniformly via one call
site). Named, not silent, bypasses: import bundle, status_transitions
backfill, workspace restore/purge — none are live human-facing mutations.
Known Phase-1 tradeoff, flagged not fixed: bulk mutations are NOT batched
into one notification the way the existing SSE/webhook bulk path is — a
bulk-assign of N items surfaces N individual notifications. Each is still
correctly scoped by the recipient's own watches/addressed-to-you filter
(a narrower audience than the workspace-wide SSE firehose the existing
batching protects), so this is a noise-discipline tradeoff, not a leak.
* feat(cli): pad watch + pad session register (TASK-2533)
pad watch <ref> [--until field=value] creates/upserts a durable watch;
pad watch list / pad watch remove <ref> are the hygiene companions the
dispatcher asked to be included explicitly rather than silently added.
pad watch --stream --for-session is the plugin-monitor command: one
stdout line per matching event ("PAD TASK-214 -> kind (actor): summary"),
silent on startup with no .pad.toml (hourly retry) or an unreachable padd
(backoff retry) per DOC-2479's noise-discipline contract. The retry/
backoff math and line formatting are pure, unit-tested functions; the
actual sleep loop is not (per the dispatcher's ask).
pad session register writes ~/.pad/sessions/<pid>.json (pid, cwd,
CLAUDE_CODE_MESSAGING_SOCKET when set) -- forward-looking infra for
Phase 3's live-sessions/presence surface; nothing consumes it yet in
Phase 1/2.
* fix(server): comment replies never published a watch notification (TASK-2533)
Codex round 1 finding 2 (verified real, not a false positive):
handleCreateReply is a SEPARATE code path from handleCreateComment — it
calls store.CreateComment directly via POST .../comments/{id}/replies,
not POST .../comments — and was missing the watch-notification hook
entirely. A reply to a comment on a watched item produced zero
notification. Same kind=comment publish as the top-level path, plus a
regression test covering the reply route specifically.
* fix(server): re-check current access before serving/delivering watches (TASK-2533)
Codex round 1 finding 1: ListWatchesForUser filtered only by user_id — a
watch row survives a revoked workspace membership or grant (nothing
deletes it), so GET /api/v1/watches and the event-stream's notification
filter could keep leaking item title/ref, workspace slug, actor, and
summary for access the caller no longer has.
Adds Store.ListWatchesForUser's ItemCollectionID column (needed for the
visibility check) and server.filterWatchesByCurrentAccess, which mirrors
computeSSEVisibility's RBAC resolution (handlers_events.go) — admin
bypass, VisibleCollectionIDs for member/guest full-collection access,
GuestVisibleResources for item-level grants — grouped by workspace since
a caller's watches can span many, unlike a single SSE connection scoped
to one. Fails closed on any lookup error.
Wired into handleListWatches here; the event-stream's loadWatchPredicates
call site picks up the same filter in the next commit, which also
restructures that function's Subscribe/replay sequence and therefore
touches the same lines.
* fix(watchevents): atomic ID assignment + subscribe-and-replay (TASK-2533)
Codex round 1, findings 3 and 4 (same subsystem, fixed together):
Finding 4 — sequence assignment and replay-buffer insertion happened
under SEPARATE locks in MemoryBus.Publish. Two concurrent Publish calls
could append to the ring buffer out of ID order, corrupting since()'s
ordering assumptions (it walks the ring oldest→newest assuming monotonic
IDs). Fixed by unifying seq assignment, buffer append, and the
subscriber-list snapshot under one lock; the (already non-blocking)
fan-out send still happens after releasing it.
Finding 3 — GET /api/v1/events/stream called Subscribe() and, later
(when resuming via Last-Event-ID), EventsSince() as two separate calls.
A Notification published in the window between them landed in BOTH the
replay result and the live channel, double-delivering it. Bus gains
SubscribeAndReplaySince(sinceID), which atomically subscribes and reads
the replay buffer under the SAME lock; the stream handler now uses it
whenever a Last-Event-ID is present (this commit carries that call-site
change, plus the finding-1 loadWatchPredicates filter wiring from the
previous commit — both land in the same lines of this function).
Adds a concurrent-publish ID-ordering test and a subscribe-then-
concurrent-publish no-duplicate test, both run with -race.
* fix(cli): monitor silent-start ordering + sync_required handling (TASK-2533)
Codex round 1, findings 5 and 6:
Finding 5 (P1) — runWatchMonitor called getClient() once, before the
loop and before the .pad.toml check. getClient() -> getConfiguredConfig()
os.Exit(1)s when unconfigured with no TTY, or launches an INTERACTIVE
configuration wizard when one is attached — either way a direct violation
of DOC-2479's silent-start contract, which requires "not ready yet" to be
a silent retry, never a crash or a prompt. Adds monitorClient(), which
builds the client the same way but returns a plain error instead of
exiting or prompting; client construction now happens INSIDE the loop,
after the .pad.toml gate, on every iteration, and its failure folds into
the existing padd-unreachable backoff path.
Finding 6 (P2) — streamWatchEvents ignored "sync_required" (the server's
signal that the requested Last-Event-ID was evicted from its replay
buffer), so a stale cursor got resent on every reconnect forever. Now
clears the cursor on sync_required so the next reconnect is a fresh,
non-resuming subscription instead.
Both covered by tests that assert the goroutine returns promptly on
context cancellation (proving no os.Exit / no blocking prompt was hit,
since the test process itself is still running to observe the return)
and that streamWatchEvents clears/re-tracks the cursor correctly around
sync_required.
* fix(server): uniform current-access gate for watch AND addressed-to-you delivery (TASK-2533)
Codex round 2, findings 1 and 2 — same subsystem (watch/nudge delivery
access control), fixed together; finding 2 explicitly falsifies finding
1's fix's own admin-bypass argument, so this replaces that reasoning
rather than patching around it.
Finding 1 (confirmed real): VisibleCollectionIDs / GuestVisibleCollectionIDs
deliberately over-widen for navigation — a collection ID is included if the
caller has an item grant on ANY item inside it, explicitly leaving
item-level narrowing to the caller (their own doc comments say so).
computeWatchAccessVisibility used that over-wide set directly as the
"fully visible" gate, so a guest granted item A was treated as having full
access to A's WHOLE collection, including an ungranted sibling item B.
Fixed by building the "genuinely full access" set from
GuestVisibleResources' fullCollectionIDs (populated only from direct
collection_grants, never widened by an item grant) + GetMemberCollectionAccess
/ ListSystemCollectionIDs for an actual member — exactly computeSSEVisibility's
own fullCollSet construction, not an approximation of it.
Finding 2 (confirmed real): the addressed-to-you (KindAssignment) branch in
watchNotificationVisible returned true unconditionally, with NO access
check. validateAssignmentScope (internal/store/items.go) only checks
WORKSPACE membership, never collection access, so an item can be assigned
to a "specific"-access member whose granted collections don't include it
at all — an ordinary assignment, no revocation timing required. Fixed by
gating EVERY notification kind — watch-matched and addressed-to-you alike —
through the SAME watchAccessVisibility check before either branch runs.
watchevents.Notification gains CollectionID so the check has what it needs
without a second lookup; the stream handler resolves it lazily per
workspace via a small connection-scoped cache (workspaces aren't known in
advance for addressed-to-you the way watch workspaces are), cleared on the
same reval tick that reloads the watches map.
This also required replacing computeWatchAccessVisibility's admin-bypass
argument, not just its code: "every call site filters the caller's OWN
watches" stopped being a sufficient justification once addressed-to-you
(which is fundamentally about *this* caller's own assignment activity
across every workspace) shares the same gate — a bearer-borne admin token
unconditionally trusted for that is exactly BUG-1616's blast radius. Now
mirrors computeSSEVisibility's cookie-vs-bearer distinction exactly.
Tests: guest-with-item-grant no longer sees a sibling item's watch or
stream notification (filter-level and HTTP/SSE-level); an assignment
outside a restricted member's granted collections is denied at both
levels; addressed-to-you is proven still gated (denied with no access,
visible once granted) as a pure unit test.
* fix(store): always re-read existing under lock, not just for precheck/patch updates (TASK-2533)
Codex round 2 finding 4, verified real: updateItemWithParentLinkOnce's
`existing` snapshot was only refreshed under the write lock when precheck
!= nil, ExpectedUpdatedAt != "", or FieldsPatch != nil — any update
touching none of those (e.g. a plain title-only PATCH) kept the STALE
pre-tx `existing` for the rest of the function, including the
LastMutation assignment-delta comparison added in TASK-2533's first
round. A concurrent OTHER transaction's assignment change landing between
this transaction's pre-tx read and its lock acquisition would get
misattributed to THIS transaction: a title-only update could report a
spurious, wrongly-attributed AssignmentChanged for a transition it never
made, duplicating the one the other transaction already reported
correctly (or missing a real one, depending on interleaving).
The status-transition capture already defended against exactly this with
its own separate conditional re-read; the assignment-delta capture added
later did not replicate that guard. Fixed by making the re-read
unconditional — once, right after the locks are held, before any SET-
clause building or the UPDATE itself — so every existing.* comparison in
this function is race-free by construction, not by each caller
remembering to guard itself. Also removes the now-redundant duplicate
re-read the status code had of its own.
Reproduces the exact race deterministically using UpdateItemWithPreCheck's
precheck hook as a synchronization point (TX2's assignment change blocks
mid-transaction while TX1's title-only update races its own pre-tx read
against it) — the new test fails reliably against the pre-fix code and
passes reliably (including under -race, and in Postgres mode) against
the fix.
* fix(watchevents): send under the same lock Unsubscribe/Close use (TASK-2533)
Codex round 2 finding 3, confirmed real and high-severity: Publish
snapshotted subscriber channels under the lock, released it, and only
then sent to them. A concurrent Unsubscribe or Close could close one of
those channels in the window between the snapshot and the send — a send
on a closed channel PANICS in Go, which crashes the whole padd process,
not just one subscriber's connection. The reasoning for releasing the
lock before sending ("a slow subscriber would stall everyone else") didn't
hold up: the send is already non-blocking (select/default — a full
channel is dropped-and-logged, never awaited), so holding the lock
through it costs nothing and closes the window structurally.
Adds a hammer test (many iterations of concurrent Publish / Subscribe /
Unsubscribe / Close, short-lived churned channels, recover()-wrapped so a
regression fails cleanly instead of crashing the whole `go test` run) that
reproduces "send on closed channel" dozens of times per run against the
pre-fix code (plus an independent -race detection) and passes cleanly,
repeatedly, against the fix.
* fix(server): re-fetch the user, not just the vis map, on each reval tick (TASK-2533)
Codex round 3, confirmed real: watchVisCache captured *models.User ONCE
at connect time (newWatchVisCache) and never re-fetched it; reset()
cleared only the per-workspace visibility map. computeSSEVisibility's own
doc comment explains why it re-fetches the user fresh on every call —
"so mid-stream role changes (admin demotion, user.disabled flips) take
effect on the next tick" — and the round-2 commit claimed to mirror that
"exactly," but only carried over the collection/bearer logic, not the
re-fetch itself. Net effect: a demoted or disabled admin kept fullAccess
on an open stream (both watch-matched and addressed-to-you delivery,
since both go through this same cache) until reconnect.
Adds watchVisCache.refreshUser, called by both the constructor and
reset() so the cadence matches computeSSEVisibility's actual cadence in
handlers_events.go (that function is invoked once at connect and again
only on each membershipCheck tick — never per event — so "per cache
reset" here is the same cadence, not a narrower one). Deliberately fails
CLOSED (not open-to-stale like computeSSEVisibility's own transient-error
fallback) on a fetch error, a deleted user, or a disabled user — a nudge
stream's wrong failure mode is delivering a fact to someone who
shouldn't see it, not a dropped UI update, so this trades
computeSSEVisibility's availability-leaning fallback for a stricter one
and says so in the comment rather than repeating the "mirrors exactly"
claim the fix falsified.
Tests: a unit-level pair (mirroring handlers_events_revalidation_test.go's
existing admin-demotion/disable coverage of the analogous SSE gap
exactly) proves an admin loses fullAccess after a demotion + reset(),
and a disabled user is denied outright; an HTTP/SSE-level test proves a
live stream stops delivering entirely once its connected user is
disabled and a reval tick passes. All three reproduce the bug reliably
against the pre-fix code and pass cleanly against the fix.
The HTTP-level test deliberately runs serially (not t.Parallel()): it
mutates the package-level watchListRevalInterval var, which every other
parallel watch-stream test in this package also reads via its own
ticker — writing to it from a t.Parallel() test raced against those
reads under -race (misattributed by the race detector to a whole
cluster of unrelated concurrently-running tests before this was
diagnosed). Full server package -race pass is clean after the fix.
* fix(server): decouple vis-cache reset from watch-list reload success (TASK-2533)
Codex round 4, confirmed real: on a reval tick, if ListWatchesForUser
errored, the handler's `continue` skipped visCache.reset() entirely —
the two were coupled, with reset() only reachable on the reload's
success path. A demoted or disabled user's stale identity/visibility
(round 3's fix) stayed live for exactly as long as that UNRELATED query
kept failing, so the round-3 leak reopens for the duration of any
watch-list reload error.
Fixed by running visCache.reset() first, unconditionally, before
attempting the watch-list reload. On a reload failure, the stale watch
list is kept (its own staleness is already bounded by
watchListRevalInterval's "eventually consistent" contract) but is now
gated by the FRESH visCache regardless — a demoted/disabled user is
denied via visCache even while the watch list itself lags a tick.
Chose this over dropping all delivery for the tick (the other option the
finding offered) because tying stream availability to an unrelated
query's transient health seemed like the wrong tradeoff; the comment at
the call site states this choice explicitly.
Adds a watchPredicatesLoadFault test seam on *Server (mirrors the
existing restoreAckFault pattern) so the reload failure can be forced
deterministically without breaking the DB connection for the whole test.
Reproduces the exact bug: forces the reload to fail on every tick while
concurrently disabling the connected user, and asserts addressed-to-you
delivery (which depends only on visCache, never the watch list) is
denied anyway. Fails reliably against a reverted (pre-fix, coupled)
version of the reval branch and passes cleanly against the fix.
Full server package -race pass, full suite (SQLite + Postgres) pass,
lint clean — this is the pre-PR verification matrix; round 5 will be a
narrow re-verify of this fix only.
* fix(server): bound stale watch set under persistent reload failure; atomic test seam (TASK-2533)
Codex round 5, two P2s, both confirmed real:
Finding 1 — `watches = fresh` only ran on the reload's success path, so
under a PERSISTENT (not single-tick) reload failure the watch set stayed
live indefinitely: a dead watch (removed, item deleted) kept matching
forever, and a watch created during the outage was silently missed
forever — visCache (round 4) gates current ACCESS, not whether a watch
still legitimately exists, so it couldn't catch this on its own. Fixed
by tracking consecutive reload failures and clearing the watch set once
maxConsecutiveWatchReloadFailures (3 ticks) is crossed, failing closed
on watch-matched delivery specifically while addressed-to-you delivery
(visCache-only, unaffected either way) continues throughout. Updated the
tradeoff comment at the call site so the "eventually-consistent" claim
now matches the bounded, not unbounded, behavior it actually describes.
Finding 2 — the watchPredicatesLoadFault test seam was a plain `func()
error` field, written by a test AFTER the SSE stream's background
goroutine was already running and reading it on every reval tick:
genuinely racy, unlike restoreAckFault's own use of the identical field
shape, which is set once, synchronously, before the single HTTP request
that reads it — goroutine creation's happens-before edge makes THAT
usage safe without any extra synchronization. Verified restoreAckFault
does not share the flaw and left it untouched. Fixed the watch seam with
atomic.Pointer[func() error] instead.
Test for finding 1: forces maxConsecutiveWatchReloadFailures+1
consecutive reload failures via the (now-atomic) fault seam and asserts
watch-matched delivery is suppressed once the bound is crossed while
addressed-to-you keeps delivering, then clears the fault and confirms
watch-matched delivery resumes on the next successful reload — a bounded
outage response, not a one-way ratchet. Fails reliably against the
bound disabled, passes cleanly restored.
This is the (re-run) pre-PR verification matrix per the dispatcher:
SQLite + Postgres + full-suite -race + lint + gofmt, all clean. Round 6
is a narrow re-verify of these two fixes only.
* test(store): bound the concurrent mutation-signal test's wait (TASK-2533)
CI-triage follow-up: PR #1082's plain Postgres step hit go test's default
10-minute per-binary timeout. Investigated whether any store test added by
this branch scales with runner slowness (lock-wait defaults, sleep-based
polling, transaction-hold durations):
- Watches CRUD tests (8): 0.63-0.80s each under Postgres, isolated and in
the full 741-test package run.
- Mutation-signal tests (6), including the precheck-hook two-transaction
race test: 0.48-0.80s each; the race test held at 0.48-0.51s across 10
consecutive runs (no variance) and across the full-package run.
- Full store package under Postgres: 279.17s and 277.12s across two runs
on this branch, matching the ~275s/297s baseline team-lead measured
locally and on PR #1081 — no reproducible slowdown from anything this
branch adds.
No pathological test found locally. The one test with genuine
cross-goroutine DB lock contention (TestLastMutation_AssignmentDelta_
NotMisattributedUnderConcurrentWrite) had an unbounded wg.Wait() as its
only unbounded wait — TX2's release was already unconditional (fixed 50ms
sleep, not gated on TX1's progress), so there's no deadlock risk, but
there was no ceiling on how long legitimate lock contention could
stretch it under a slow/shared runner. Replaced with a bounded 10s wait
that fails fast with a diagnostic instead of silently consuming
test-binary budget if it's ever exceeded. Verified the regression test
still fails reliably (5/5) against a revert of the round-2 fix it guards.
Could not reproduce the CI timeout locally; likely the pre-existing
~297s CI baseline (already noted as close to the 10-minute ceiling)
plus environmental variance on the shared runner, not a specific test
this branch adds.
40 lines
1.8 KiB
Go
40 lines
1.8 KiB
Go
package models
|
|
|
|
import "time"
|
|
|
|
// Watch is a durable, server-side subscription created by `pad watch <ref>`
|
|
// (TASK-2533, per DOC-2479's subscription-table design). Unlike an SSE
|
|
// connection or a plugin-monitor process, a Watch survives both — it is the
|
|
// thing that makes "the monitor restarts every session" not lose track of
|
|
// what a user asked to be told about.
|
|
type Watch struct {
|
|
ID string `json:"id"`
|
|
WorkspaceID string `json:"workspace_id"`
|
|
UserID string `json:"user_id"`
|
|
ItemID string `json:"item_id"`
|
|
// Predicate is the raw `--until field=value` string (e.g. "status=done"),
|
|
// or "" for an unconditional watch that fires on any matching
|
|
// notification for this item. DOC-2479 specs only this single
|
|
// field=value grammar — no boolean combinators.
|
|
Predicate string `json:"predicate,omitempty"`
|
|
CreatedAt time.Time `json:"created_at"`
|
|
|
|
// Populated by joins (not stored) — the CLI's `pad watch list` and
|
|
// the event-stream handler's summary text both want the item's
|
|
// human-facing identity without a second lookup.
|
|
ItemRef string `json:"item_ref,omitempty"`
|
|
ItemTitle string `json:"item_title,omitempty"`
|
|
ItemSlug string `json:"item_slug,omitempty"`
|
|
WorkspaceSlug string `json:"workspace_slug,omitempty"`
|
|
// ItemCollectionID is the watched item's collection ID (populated by
|
|
// the same join as the fields above). Internal-only (`json:"-"`,
|
|
// like models.Item.LastMutation) — not part of the wire contract,
|
|
// consumed only by server.filterWatchesByCurrentAccess (TASK-2533,
|
|
// codex round 1 finding 1) to re-check the caller's CURRENT
|
|
// visibility into the watched item's collection before delivering or
|
|
// listing a watch, so a workspace membership or grant revoked after
|
|
// the watch was created can't keep leaking item metadata/events
|
|
// through a stale row.
|
|
ItemCollectionID string `json:"-"`
|
|
}
|