feat: invalidate provisioner daemon sessions on key deletion - #26532
Conversation
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.8.0 | Round 1 | Last posted: Round 1, 9 findings (2 P3, 6 Nit, 1 Note), COMMENT. Review Finding inventoryFindings
Round logRound 1Panel. 0 P0-P1, 2 P3, 4 Nit, 1 Note, 1 CRF-1 from Netero (not posted). 4 dropped. Reviewed against 61fa2ab..7e68348. 17 reviewers: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Ging-Go, Gon, Leorio, Chopper, Takumi, Meruem, Kurapika, Komugi, Knov, Robin, Kite, Razor. Hisoka P3 dropped: claimed pubsub-driven cancellation produces StatusInternalError. DRPC Server.Serve returns nil when ctx is canceled (drpcserver/server.go:132: Gon raised 6 P2s for comment verbosity. Downgraded to Nit (style, not correctness) and consolidated to 2 representative examples (CRF-8, CRF-9). The remaining 4 trim suggestions (ProvisionerKeyDeletedChannel, IsReservedProvisionerKey, terminateOnDeletedKey, publish comment) are valid but posting 6 nits for comment trimming is noise. Pattern noted in review body. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-constructed three-layer defense for provisioner key session invalidation. The subscribe-then-recheck ordering is textbook for closing the Postgres LISTEN/NOTIFY race, the per-key channel design keeps LISTEN count proportional, and the ctx-to-srvCtx fix is necessary and safe. Test coverage is strong: 219 test LOC covering both RPC variants, reserved-key exemption, end-to-end pubsub teardown, and the subscribe-startup race via a custom store interceptor. The deleteKeyOnReadStore wrapper is particularly well-done.
17 reviewers, 2 P3, 4 Nit, 1 Note. No correctness issues found. Takumi, Kurapika, Komugi, and Razor independently verified the concurrency, auth, and determinism properties. Pariston validated the problem framing and confirmed the three layers cover distinct failure modes without redundancy.
Gon identified a comment verbosity pattern across the PR (6 doc comments that partially restate their function bodies). Two representative examples are filed inline; the remaining four (ProvisionerKeyDeletedChannel, IsReservedProvisionerKey, terminateOnDeletedKey, and the publish comment in provisionerkeys.go) would also benefit from trimming to their non-obvious content.
"Subscribe first, then read. A deletion that commits between auth and subscribe is caught by the re-check. A deletion after the re-check is caught by the subscription. A deletion between subscribe and re-check is caught by both. Three layers, no gap. Pleased to find no seam." (Hisoka)
enterprise/coderd/provisionerkeys.go:201
Nit [CRF-1] Lines 201-203 check reserved keys by string comparison (provisionerKey.ID.String() == codersdk.ProvisionerKeyIDBuiltIn || ...). This PR adds codersdk.IsReservedProvisionerKey(uuid.UUID) which does the same check via UUID comparison. The guard here should use codersdk.IsReservedProvisionerKey(provisionerKey.ID) to avoid duplication. (Netero)
🤖
🤖 This review was automatically generated with Coder Agents.
This comment has been minimized.
This comment has been minimized.
johnstcn
left a comment
johnstcn
left a comment
There was a problem hiding this comment.
I think a better approach would be to modify the AcquireJob query to never acquire a job when the associated key is deleted. A provisioner daemon's tags come directly from the key (except in case of PSK, which is deprecated AFAIR).
| // ProvisionerKeyDeletedChannel returns the pubsub channel that carries a | ||
| // notification when the provisioner key with the given ID is deleted. The | ||
| // payload is empty; the channel name encodes the key ID. |
There was a problem hiding this comment.
Too far away from implementation detail to make this kind of statement.
| // ProvisionerKeyDeletedChannel returns the pubsub channel that carries a | |
| // notification when the provisioner key with the given ID is deleted. The | |
| // payload is empty; the channel name encodes the key ID. | |
| // ProvisionerKeyDeletedChannel returns the pubsub channel that carries a | |
| // notification when the provisioner key with the given ID is deleted. The | |
| // channel name encodes the key ID. |
| // terminateOnDeletedKey cancels the session, when a cancel is configured, so | ||
| // the daemon stops after its key is deleted. | ||
| func (s *server) terminateOnDeletedKey() { | ||
| if s.sessionCancel != nil { | ||
| s.sessionCancel() | ||
| } | ||
| } |
There was a problem hiding this comment.
What happens if someone forgets to pass in sessionCancel?
| ProvisionerTags: dbTags, | ||
| }) | ||
| var job database.ProvisionerJob | ||
| err := a.store.InTx(func(tx database.Store) error { |
There was a problem hiding this comment.
This is executed in a transaction to ensure that a job is claimed iff the provisioner's associated key is still live. The transaction adds an 4 extra round trips to the database, holding the key lock for 3 of these. imo this is worthwhile - the backup acquisition polling is every 30s per domain, much larger than the milliseconds added by the chained queries. If this is a concern, an earlier version of this code did the check within AcquireProvisionerJob, but overloading that query to handle key deletion sacrificed a good bit of clarity.
That makes sense, and I have implemented a version of it. Do you think the pubsub fast path is still worthwhile? Terminating the process quickly is nice, but isn't essential as either acquisition or a new key-deleted heartbeat check (added in this PR) would catch it. My feeling is remove it. I may also remove that new heartbeat check - because it can fire during a running job, some extra machinery is needed to put the provisioner server into a terminating state in order to allow the current job to finish before shutting down. If we are OK only relying on the acquisition signal alone, I'll rip that out. The only downside of the acquisition signal is that it's delay is technically unbounded if you have >1 provisioner in a domain, but in practice that shouldn't be an issue. Anyway, I'm spending too much time on this (though it's been a good intro to the provisioner). Let me know if you're cool with the acquirer-only path. |
| // IsReservedProvisionerKey reports whether the given ID is one of the reserved | ||
| // provisioner keys (built-in, user-auth, PSK). Reserved keys are created by the | ||
| // system and cannot be deleted. | ||
| func IsReservedProvisionerKey(id uuid.UUID) bool { | ||
| switch id { | ||
| case ProvisionerKeyUUIDBuiltIn, ProvisionerKeyUUIDUserAuth, ProvisionerKeyUUIDPSK: | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
This is only exported for use in tests. Could this be inlined into IsDeletableProvisionerKey?
2467209 to
03987ab
Compare
Closes PLAT-305. When a deletable provisioner key is deleted, tear down the external provisioner daemon sessions authenticated with it via four layers: publish-on-delete + pubsub subscribe/teardown (with post-subscribe and dropped-message re-checks), an acquire-time key lock in the claim transaction, and a heartbeat watchdog. Session termination is deferred while a claimed job is active so the daemon can report its result.
TestAcquirer_ProvisionerKeyDeleted counted AcquireProvisionerJob calls across every acquiree, so the unkeyed acquiree's call could land before the assertion once the keyed acquiree handed over its clearance. Count calls per worker instead. TestProvisionerDaemonServe/DroppedMessageKeyCheckErrorKeepsSession mutated the provisioner key while the handler's post-subscribe re-check could still be pending or in flight, and that re-check closes the connection on both the deleted and the error branch. Wait for a served RPC before touching the key, and serialize key reads against the mutation with a gate in the fake store. Also restores gci import grouping in the files the branch reformatted.
2f39ab4 to
db9eb68
Compare
3 participants
Summary
Closes PLAT-305.
When a provisioner key is deleted, the associated daemon kept operating on its existing WebSocket connection, because authentication was only checked at connection establishment and deletion was a bare
DELETEwith no session invalidation.This adds four layers of defense so a deleted key promptly stops doing work:
deleteProvisionerKeypublishes to a new per-key pubsub channel (coderd/pubsub.ProvisionerKeyDeletedChannel) after a successful delete. Publish errors are logged but still return204, since layer 3 is the durable backstop.UpdateJob/CompleteJobhave no key check), and the last active job's completion performs the cancellation. Because PostgresLISTEN/NOTIFYdoes not buffer for non-listeners, the handler also performs a synchronous key-existence re-check immediately after subscribing to close the race between auth and subscription. The subscription usesSubscribeWithErrso that anErrDroppedMessagessignal (emitted when the pubsub listener reconnects) triggers the same key re-check, closing the listener-outage window in which a deletion notification could be missed.AcquireJobandAcquireJobWithCancelverify the key still exists before waiting for a job, and theAcquirerclaims jobs in a transaction that first locks the worker's deletable key (LockProvisionerKeyByIDForShare, aFOR KEY SHARErow lock held until commit) before running theAcquireProvisionerJobclaim, so a claim cannot commit after the key's deletion. This guards against a missed pubsub message. A missing key row surfaces as its own result rather than overloading the claim query's no-rows response: the acquire terminates withErrProvisionerKeyDeleted(terminating the session, with the same active-job deferral) and hands the consumed wakeup to another waiting daemon in the same domain, rather than silently re-parking and starving peers of job postings.A job that is claimed but never delivered (the session or connection dies between the database claim and the stream send) is marked failed immediately on a fresh context, instead of staying assigned to the worker until the job reaper.
Reserved keys (built-in, user-auth, PSK) are exempt throughout, since they are not deletable rows. The acquire-time lookup runs as
dbauthz.AsSystemReadProvisionerDaemons, because the provisionerd role cannot read provisioner keys and a provisioner key's RBAC object is a provisioner daemon.A single key can back many daemons (and span HA replicas), so the per-key channel fans out to invalidate all of them at once. Per-key channels keep the
LISTENcount proportional to distinct keys rather than waking every daemon on unrelated deletions.Known limitations
UpdateJob/CompleteJobintentionally have no key check. By the time those RPCs arrive the work has already run; rejecting completion would strand a build in "running" (until the job reaper fails it) with real infrastructure left unreconciled. Session termination is deferred while a job is active so the completion can be reported; the daemon may not receive the final RPC response when the deferred termination fires, but the job's outcome is already persisted.Tests
coderd/provisionerdserver:TestAcquireJob_ProvisionerKeyDeleted(both RPC variants),TestAcquireJob_ReservedProvisionerKey,TestHeartbeat_ProvisionerKeyDeleted(heartbeat watchdog cancels the session after key deletion),TestAcquirer_ProvisionerKeyDeleted(a dead-key acquiree exits terminally and its clearance is promoted to a peer in the same domain), andTestTerminateSession_Deferral(termination is immediate when idle and deferred until the last active job finishes).coderd/database:TestAcquireProvisionerJob/ProvisionerKeyLockcovers the lock query against real Postgres: it returns the key ID while the row exists and no rows once it is deleted. The lock-then-claim composition is pinned byTestAcquirer_ProvisionerKeyDeleted.enterprise/coderd:TestProvisionerDaemonServe/KeyDeletionClosesSessionasserts an active session closes after its key is deleted.KeyDeletedDuringSetupClosesSessioncovers the post-subscribe re-check when a key is deleted between auth and subscription, andDroppedMessageClosesSessioncovers theErrDroppedMessagesre-check when a deletion is missed during a listener outage.Validation
makepre-commit (gen/fmt/lint/build) passed via git hooks.coder provisionerd start. Confirmed it authenticated via the key and connected, appearing asidlein bothcoder provisioner list(with the key name) and the organization Provisioners UI.provisioner key deleted, terminating session, the daemon's session closed immediately, and it dropped fromcoder provisioner list(then entered the known 401 redial loop, PLAT-452).sleep 45inlocal-exec) pinned to the external daemon and deleted the key mid-build. The server loggeddeferring session cancellation until active jobs finish; the heartbeat watchdog re-checked mid-build and re-deferred rather than force-killing. The build ran to completion (Apply complete, workspaceStarted) and only then didcanceling session after job completionfire. The documented caveat reproduced: the daemon lost the finalCompleteJoback, and the build outcome was still persisted correctly.Implementation plan and design decisions
Design
provisioner_key_deleted:<keyID>) so daemons do not wake on unrelated deletions. The cost is oneLISTENper distinct key per replica on the shared listener connection, which is negligible against Coder's existing channels.authorize -> UpsertProvisionerDaemon -> Subscribe -> GetProvisionerKeyByID. The post-subscribe re-check handles a deletion that committed before theLISTENregistered (Postgres does not buffer notifications for non-listeners; the in-process buffer only smooths bursts and drops on overflow).NewServerchange:KeyIDwas added toprovisionerdserver.Optionsto avoid a positional signature change across call sites. The in-memory (built-in) daemon leaves it unset and is therefore exempt.Files
coderd/pubsub/provisionerkeydeleted.go(new) — channel helper.enterprise/coderd/provisionerkeys.go— publish on delete.enterprise/coderd/provisionerdaemons.go— subscribe, re-check, cancel session; passKeyID.coderd/provisionerdserver/provisionerdserver.go—KeyIDoption and acquire-time existence check.This pull request was created by Coder Agents on behalf of @jscottmiller.