fix: wait for gpg-agent forward readiness before opening browser IDEs - #893
Conversation
ForwardAgent spawned its forwarding child fire-and-forget and returned immediately, so browser IDEs (vscode-web, etc.) could open a terminal before the child had finished connecting and running remote setup-gpg. The first gpg command in that terminal would then create a fresh local keyring instead of seeing the forwarded agent. ForwardAgent now waits for the child's first attempt to signal readiness over an inherited pipe (with a timeout so a stuck forward can't block the caller indefinitely), mirroring the synchronous wait the SSH-terminal path already had.
✅ Deploy Preview for devsydev canceled.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughGPG forwarding now uses a readiness file descriptor to synchronize startup. ChangesGPG forwarding readiness
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ForwardAgent
participant superviseForward
participant runForwardOnce
participant ChildProcess
participant WorkspaceTunnel
ForwardAgent->>superviseForward: start supervised forwarding
superviseForward->>runForwardOnce: execute forwarding attempt
runForwardOnce->>ChildProcess: provide EnvForwardReadyFD on descriptor 3
ChildProcess-->>runForwardOnce: write readiness byte
runForwardOnce-->>ForwardAgent: report readiness
WorkspaceTunnel->>WorkspaceTunnel: signal readiness after successful setup
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for images-devsy-sh canceled.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/workspace/gpg_tunnel.go`:
- Line 288: Update the GPG tunnel setup flow around ensure and
signalGPGForwardReady so ensure reports whether the tunnel is running, and only
signal readiness after successful initial setup or retry. Ensure failures do not
send the readiness byte or allow ForwardAgent to return early, and add coverage
for an initial setup failure.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 44c6d8b8-92d1-470f-a1ff-7524798e1011
📒 Files selected for processing (3)
cmd/workspace/gpg_tunnel.gopkg/gpg/forward.gopkg/gpg/forward_test.go
ensure() previously reported nothing, so runGPGTunnelInBackground always signaled readiness right after the first synchronous ensure call even when that setup failed. ForwardAgent would then return believing the tunnel was up, letting a browser IDE terminal race a gpg-agent that was never forwarded. ensure now reports whether the tunnel is live, and readiness is only signaled the first time that's true - whether from the initial call or a later health-check retry. Also make signalGPGForwardReady report write success so a failed readiness write doesn't get latched as already-signaled, which would strand ForwardAgent until its timeout despite the tunnel being live.
superviseForward cleared the readiness channel after the first attempt regardless of outcome, and runForwardOnce treated a child exiting without ever writing to its readiness fd the same as a real readiness signal. A transient failure on the very first attempt therefore made ForwardAgent return immediately as "ready" and permanently gave up reporting readiness for any later successful retry - defeating the point of waiting at all. runForwardOnce now only reports readiness after an actual byte is read from the pipe, and superviseForward keeps the readiness channel live across restarts until an attempt actually reports it.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/workspace/gpg_tunnel.go`:
- Around line 268-278: Update signalGPGForwardReady to retain the *os.File
created by os.NewFile on gpgTunnel when the readiness write fails, keeping it
reachable until a later retry succeeds; release or clear the retained file only
after a successful write. Preserve the existing invalid-descriptor handling and
retry behavior.
🪄 Autofix
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fc3794b5-aa4e-475e-b3c5-bf105fa174b1
📒 Files selected for processing (3)
cmd/workspace/gpg_tunnel.gopkg/gpg/forward.gopkg/gpg/forward_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/gpg/forward.go
Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Summary
pkg/gpg.ForwardAgent), so the browser terminal could open before the forwarding child had finished connecting and running remotesetup-gpg. The firstgpgcommand in that terminal would then create a fresh local keyring instead of seeing the forwarded agent.ForwardAgentnow waits for its child's first attempt to signal readiness over an inherited pipe (bounded by a timeout so a stuck forward can't block the caller indefinitely), mirroring the synchronous wait the SSH-terminal path (cmd/workspace/gpg_tunnel.go) already had.devsy sshsessions are unaffected — the readiness signal is a no-op unless the process was spawned byForwardAgent.Summary by CodeRabbit