Skip to content

SY-3685: Arc Channel and Value Assignment Bugs#1945

Merged
emilbon99 merged 15 commits into
sy-3687-synnax-v0506from
sy-3685-arc-channel-and-value-assignment-bugs
Feb 6, 2026
Merged

SY-3685: Arc Channel and Value Assignment Bugs#1945
emilbon99 merged 15 commits into
sy-3687-synnax-v0506from
sy-3685-arc-channel-and-value-assignment-bugs

Conversation

@emilbon99

@emilbon99 emilbon99 commented Feb 5, 2026

Copy link
Copy Markdown
Contributor

Issue Pull Request

Linear Issue

SY-####

Description

Basic Readiness

  • I have performed a self-review of my code.
  • I have added relevant, automated tests to cover the changes.
  • I have updated documentation to reflect the changes.

Greptile Overview

Greptile Summary

Fixed two critical bugs in the Arc language implementation and improved driver shutdown handling:

  • Fixed channel alias assignment in Arc analyzer by calling UnwrapChan() on expression types, allowing channel aliases (like local_ref := sensor) to be correctly assigned to scalar variables
  • Fixed Windows event loop watch() to be idempotent by checking handle equality, preventing false errors when re-watching the same notifier
  • Introduced NOMINAL_SHUTDOWN_ERROR sentinel to distinguish expected runtime shutdowns from actual errors, eliminating false error logs during normal pipeline termination

All changes include comprehensive unit tests covering the fixed scenarios. The PR description is incomplete (missing Linear issue link and checklist items), but the code changes are well-structured and properly tested.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • All three bug fixes are well-isolated, have comprehensive test coverage, and follow established patterns. The Arc analyzer fix adds proper type unwrapping, the Windows loop fix adds a simple condition check, and the shutdown error handling uses a standard error sentinel pattern. No breaking changes or risky refactoring.
  • No files require special attention

Important Files Changed

Filename Overview
arc/cpp/runtime/loop/loop_windows.cpp Fixed watch() to allow re-watching the same notifier on Windows by checking handle equality
arc/go/analyzer/statement/statement.go Added UnwrapChan() to assignment type inference to allow channel aliases to be assigned to scalar variables
driver/errors/errors.h Added NOMINAL_SHUTDOWN_ERROR sentinel to distinguish expected shutdown from actual error conditions
driver/pipeline/acquisition.cpp Suppressed error logging for NOMINAL_SHUTDOWN_ERROR to avoid false error messages during expected shutdowns

Sequence Diagram

sequenceDiagram
    participant User as Arc Script
    participant Analyzer as Statement Analyzer
    participant Compiler as Statement Compiler
    participant Runtime as Arc Runtime
    participant Pipeline as Acquisition Pipeline
    participant EventLoop as Event Loop (Windows)

    Note over User,EventLoop: Channel Alias Assignment Flow
    User->>Analyzer: local_ref := sensor (channel alias)
    Analyzer->>Analyzer: Store channel type
    User->>Analyzer: value = local_ref (assign to scalar)
    Analyzer->>Analyzer: InferFromExpression()
    Analyzer->>Analyzer: UnwrapChan() unwrap chan f64 to f64
    Analyzer->>Analyzer: Type check f64 == f64
    
    User->>Compiler: Compile assignment
    Compiler->>Compiler: Generate channel read opcode
    Compiler->>Compiler: Generate local set for scalar
    
    Note over Runtime,Pipeline: Runtime Shutdown Flow
    Runtime->>Pipeline: read() returns false (closed)
    Pipeline->>Pipeline: Return NOMINAL_SHUTDOWN_ERROR
    Pipeline->>Pipeline: Check matches(NOMINAL_SHUTDOWN_ERROR)
    Pipeline->>Pipeline: Skip error logging
    Pipeline->>Pipeline: Break acquisition loop
    
    Note over EventLoop: Windows Event Loop Watch Fix
    User->>EventLoop: watch(notifier1)
    EventLoop->>EventLoop: watched_handle_ = notifier1
    EventLoop-->>User: true
    User->>EventLoop: watch(notifier1) again
    EventLoop->>EventLoop: Check watched_handle_ == handle
    EventLoop->>EventLoop: Same handle, allow re-watch
    EventLoop-->>User: true (idempotent)
Loading

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@emilbon99 emilbon99 changed the base branch from main to sy-3687-synnax-v0506 February 5, 2026 22:46
Comment thread core/pkg/service/arc/runtime/task.go Outdated
Comment thread core/pkg/service/arc/runtime/task.go Outdated
Comment thread core/pkg/service/arc/runtime/task_test.go
Comment thread arc/cpp/runtime/loop/loop_test.cpp
Comment thread arc/cpp/runtime/loop/loop_windows.cpp

@pjdotson pjdotson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

@pjdotson pjdotson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Address comment then good to merge.

zap.Int("seqNum", res.SeqNum),
zap.Error(res.Err),
)
} else if !res.Authorized {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should be returning nil in the sink even when res.Err != nil? Just want to make sure this is intended behavior because I am too tired to try to understand this too deeply right now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial thought was yes intentional but now I'm rethinking

@emilbon99 emilbon99 merged commit e04d8a1 into sy-3687-synnax-v0506 Feb 6, 2026
14 of 15 checks passed
@emilbon99 emilbon99 deleted the sy-3685-arc-channel-and-value-assignment-bugs branch February 6, 2026 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants