feat: refresh dynamic parameters on secret changes#24786
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
d1c033e to
bc9d432
Compare
c2353f8 to
07e4175
Compare
bc9d432 to
a107410
Compare
07e4175 to
054e408
Compare
e3033de to
e55e194
Compare
b61307b to
d2f81bc
Compare
e55e194 to
e760f0b
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
The pubsub approach is the right mechanism for this feature, and the implementation details are mostly solid: event coalescing via a size-1 buffered channel, RBAC gating before subscribing, monotonic response IDs, and a clean integration test covering the full secret lifecycle (create/delete/re-create/update). The sendRender extraction and nextDynamicParametersResponseID helper are well-structured.
Two P2s, six P3s. The P2s are a documentation/contract mismatch in response IDs and a test coverage gap for the owner-switch subscription lifecycle. The P3s are a subscribe-after-render race window (also raised by @zedkipp), a dedup guard bug, and several smaller gaps.
Severity counts: 2× P2, 6× P3.
"I pulled every thread in the closure and they all held." (Hisoka)
codersdk/parameters.go:166
P2 [DEREM-1] Response IDs no longer match request IDs, violating this documented contract.
This comment says: "The response contains the same ID so that the client can match it to the request." The new nextDynamicParametersResponseID breaks this: when a secret event fires, lastResponseID advances, and the next client request gets a bumped response ID that differs from the request ID.
Sequence: client sends ID=0, gets ID=0. Secret event fires, server sends unsolicited ID=1. Client sends ID=1 (its local counter), server returns nextDynamicParametersResponseID(1, 1) = 2. Client sent ID=1, got ID=2.
The current frontend (CreateWorkspacePage.tsx:148) uses latestResponse.id >= response.id for staleness ordering, not ID matching, so no bug manifests today. But any future consumer following the documented contract would silently discard or misattribute responses. The comment should describe the actual behavior: response IDs are monotonically increasing and may exceed the request ID. (Mafuuu)
🤖
coderd/parameters.go:160
P3 [DEREM-7] This initial render block duplicates the sendRender closure defined six lines later. Replacing with if !sendRender(-1, initial.OwnerID, initial.Inputs) { return } removes 10 lines and unifies the response-building and error-handling paths. (Netero)
🤖
🤖 This review was automatically generated with Coder Agents.
e760f0b to
9f4db1e
Compare
9f4db1e to
6605621
Compare
|
Is the idea that a secret change should immediately update on the |
Yup! The following PR in the stack adds the UI |
There was a problem hiding this comment.
Had brief DMs with @Emyrk on using the pubsub for this. Steven asked
Are people really updating secrets when the Create Workspace form is open?
There's an intent to have a modal for a user to enter missing secrets when the form is open. It's also possible a user fixes the secret requirement in another tab or via the CLI. I don't think there's a firm requirement to live refresh user secrets, but doing so seems to fall in line with how other things are handled (e.g. template active version changes, notification inbox).
We could instead reload the page on modal close (or similar), but I don't think there's much impact if there are no subscribers listening because Postgres will just drop the messages.
There was a problem hiding this comment.
Yeah that matches my thinking. The follow-up PR adds the modal flow, and refreshing also covers another tab or CLI fixing the missing secret while the form is open
There was a problem hiding this comment.
I don't mind the pubsub as long as it's cost is low.
PostgreSQL LISTEN and NOTIFY is a native, lightweight pub/sub system where messages sent via NOTIFY are dropped immediately if no active clients are currently LISTENing to that channel.
It should be cheap most of the time 👍
Emyrk
left a comment
There was a problem hiding this comment.
LGTM 👍
I did not look closely at the tests.
Two minor lint failures surfaced by 'make lint' after the revert series:
- coderd/dynamicparameters/resolver_test.go: replaces two pre-existing
emdashes (in comments restored by the revert of #24785) with commas,
matching the form on origin/main. Required by 'make lint/emdash'.
- coderd/usersecrets.go: trims two trailing blank lines left over from
the conflict resolution in the revert of #24786. Required by
goimports / trailing-whitespace lint.
Co-authored-by: Coder Agents <agents@coder.com>
Two minor lint failures surfaced by 'make lint' after the revert series:
- coderd/dynamicparameters/resolver_test.go: replaces two pre-existing
emdashes (in comments restored by the revert of #24785) with commas,
matching the form on origin/main. Required by 'make lint/emdash'.
- coderd/usersecrets.go: trims two trailing blank lines left over from
the conflict resolution in the revert of #24786. Required by
goimports / trailing-whitespace lint.
Co-authored-by: Coder Agents <agents@coder.com>
Two minor lint failures surfaced by 'make lint' after the revert series:
- coderd/dynamicparameters/resolver_test.go: replaces two pre-existing
emdashes (in comments restored by the revert of #24785) with commas,
matching the form on origin/main. Required by 'make lint/emdash'.
- coderd/usersecrets.go: trims two trailing blank lines left over from
the conflict resolution in the revert of #24786. Required by
goimports / trailing-whitespace lint.
Co-authored-by: Coder Agents <agents@coder.com>
Publishes user secret create, update, and delete events and subscribes dynamic parameter websockets to authorized owner secret changes.
Secret changes trigger fresh renders with monotonic response IDs, with backend tests covering subscription authorization and websocket refresh behavior.